From ece353aa99b9576aa3742b6b4cea6ffab61c047b Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Thu, 22 Oct 2026 23:55:22 +0000 Subject: [PATCH 1/2] fix: exclude expanded tools from prompt diffs --- internal/sync/api/catalog.go | 4 +- internal/sync/api/catalog_test.go | 31 ++++++++++++ internal/sync/fingerprint.go | 27 +++++++--- internal/sync/fingerprint_test.go | 42 ++++++++++++++++ internal/sync/prompt/acceptance_test.go | 67 +++++++++++++++++++++++++ internal/sync/prompt/diff.go | 1 + internal/sync/prompt/plan_test.go | 31 ++++++++++++ 7 files changed, 195 insertions(+), 8 deletions(-) diff --git a/internal/sync/api/catalog.go b/internal/sync/api/catalog.go index d503a3ee8..170aa4008 100644 --- a/internal/sync/api/catalog.go +++ b/internal/sync/api/catalog.go @@ -156,7 +156,9 @@ func (config *Config) UnmarshalJSON(data []byte) error { config.Variations = make([]syncdomain.Variation, 0, len(response.Variations)) for _, variation := range response.Variations { if variation.State != "archived" { - config.Variations = append(config.Variations, variation.Variation) + canonical := variation.Variation + canonical.Model = syncdomain.CanonicalModel(canonical.Model, canonical.Tools, canonical.Skills) + config.Variations = append(config.Variations, canonical) } } return nil diff --git a/internal/sync/api/catalog_test.go b/internal/sync/api/catalog_test.go index c83cfa2cd..9e4532386 100644 --- a/internal/sync/api/catalog_test.go +++ b/internal/sync/api/catalog_test.go @@ -167,6 +167,37 @@ func TestCatalogClientConfigGetsExactConfigAndAppliesMode(t *testing.T) { assert.False(t, request.IsBeta) } +func TestCatalogClientConfigRemovesExpandedToolsFromModelParameters(t *testing.T) { + transport := &recordingClient{Responses: [][]byte{[]byte(`{ + "key": "support", + "mode": "agent", + "variations": [{ + "key": "helpful", + "name": "Helpful", + "model": { + "modelName": "claude", + "parameters": { + "temperature": 0.2, + "tools": [{"type": "function", "name": "search"}] + } + }, + "tools": [{"key": "search", "version": 3}] + }] + }`)}} + + config, err := NewClient(transport, "token", "https://example.com").Config("project", "support") + + require.NoError(t, err) + require.Len(t, config.Variations, 1) + assert.Equal(t, map[string]any{ + "modelName": "claude", + "parameters": map[string]any{ + "temperature": 0.2, + }, + }, config.Variations[0].Model) + assert.Equal(t, []syncdomain.AttachmentRef{{Key: "search", Version: 3}}, config.Variations[0].Tools) +} + func TestCatalogClientConfigRejectsInvalidResponse(t *testing.T) { client := NewClient( &recordingClient{Responses: [][]byte{[]byte(`not json`)}}, diff --git a/internal/sync/fingerprint.go b/internal/sync/fingerprint.go index de9f1045a..bd11adf72 100644 --- a/internal/sync/fingerprint.go +++ b/internal/sync/fingerprint.go @@ -76,7 +76,7 @@ func (variation Variation) canonical() Variation { variation.Attachments[index] = CanonicalAttachment(variation.Attachments[index]) } variation.SortAttachments() - variation.Model = canonicalModel(variation.Model) + variation.Model = CanonicalModel(variation.Model, variation.Tools, variation.Skills) switch variation.Mode { case VariationModeAgent: @@ -108,14 +108,27 @@ func unversionedRefs(refs []AttachmentRef) []AttachmentRef { return unversioned } -// canonicalModel removes the empty "parameters" and "custom" objects that the -// variation API adds. Other empty objects can change behavior, so they remain. -func canonicalModel(model map[string]any) map[string]any { +// CanonicalModel returns a copy without defaults and expanded attachment +// content that the variation API adds. Tool and skill references own that +// content, so model parameters do not. +func CanonicalModel(model map[string]any, tools, skills []AttachmentRef) map[string]any { model = maps.Clone(model) - for _, key := range []string{"parameters", "custom"} { - if value, ok := model[key].(map[string]any); ok && len(value) == 0 { - delete(model, key) + if parameters, ok := model["parameters"].(map[string]any); ok { + parameters = maps.Clone(parameters) + if len(tools) != 0 { + delete(parameters, "tools") } + if len(skills) != 0 { + delete(parameters, "skills") + } + if len(parameters) == 0 { + delete(model, "parameters") + } else { + model["parameters"] = parameters + } + } + if custom, ok := model["custom"].(map[string]any); ok && len(custom) == 0 { + delete(model, "custom") } if len(model) == 0 { return nil diff --git a/internal/sync/fingerprint_test.go b/internal/sync/fingerprint_test.go index b0fdb6b08..cba4a940e 100644 --- a/internal/sync/fingerprint_test.go +++ b/internal/sync/fingerprint_test.go @@ -137,6 +137,48 @@ func TestFingerprintVariationNormalizesModelDefaults(t *testing.T) { require.Contains(t, withDefaults.Model, "custom") } +func TestFingerprintVariationExcludesExpandedAttachmentParameters(t *testing.T) { + base := Variation{ + Mode: VariationModeAgent, + Key: "agent", + Name: "Agent", + Model: map[string]any{ + "modelName": "claude", + "parameters": map[string]any{ + "temperature": 0.2, + }, + }, + Tools: []AttachmentRef{{Key: "search"}}, + } + expanded := base + expanded.Model = map[string]any{ + "modelName": "claude", + "parameters": map[string]any{ + "temperature": 0.2, + "tools": []any{map[string]any{"type": "function", "name": "search"}}, + }, + } + changed := expanded + changed.Model = map[string]any{ + "modelName": "claude", + "parameters": map[string]any{ + "temperature": 0.7, + "tools": []any{map[string]any{"type": "function", "name": "search"}}, + }, + } + + baseFingerprint, err := FingerprintVariation("project", "config/agent", base) + require.NoError(t, err) + expandedFingerprint, err := FingerprintVariation("project", "config/agent", expanded) + require.NoError(t, err) + changedFingerprint, err := FingerprintVariation("project", "config/agent", changed) + require.NoError(t, err) + + require.Equal(t, baseFingerprint, expandedFingerprint) + require.NotEqual(t, baseFingerprint, changedFingerprint) + require.Contains(t, expanded.Model["parameters"], "tools") +} + func TestFingerprintVariationPreservesMeaningfulModelChanges(t *testing.T) { base := Variation{ Mode: VariationModeAgent, diff --git a/internal/sync/prompt/acceptance_test.go b/internal/sync/prompt/acceptance_test.go index 7e845bdea..ea22d9ee7 100644 --- a/internal/sync/prompt/acceptance_test.go +++ b/internal/sync/prompt/acceptance_test.go @@ -272,6 +272,73 @@ func TestPromptFirstSyncAdoptsMatchingStateWithoutMutation(t *testing.T) { requireOnlyReads(t, api.requests) } +func TestPromptDiffExcludesExpandedToolsFromModelParameters(t *testing.T) { + root := initRepository(t) + description := "Search the documentation" + tool := syncdomain.Tool{ + Key: "search", + Description: &description, + Schema: map[string]any{"type": "object"}, + } + local := variation("Support") + local.Instructions = "Use the local instructions." + local.ModelConfigKey = "claude" + local.ModelConfigVersion = 2 + local.Model = map[string]any{ + "modelName": "claude-sonnet-4", + "parameters": map[string]any{ + "temperature": 0.2, + }, + } + local.Tools = []syncdomain.AttachmentRef{{Key: "search", Version: 3}} + local.Attachments = []syncdomain.Attachment{toolAttachment(tool, 3)} + writeVariation(t, root, local, false) + writeManifest(t, root, local) + + server := local + server.Instructions = "Use the LaunchDarkly instructions." + server.Attachments = nil + server.Model = map[string]any{ + "modelName": "claude-sonnet-4", + "parameters": map[string]any{ + "temperature": 0.2, + "tools": []any{map[string]any{ + "type": "function", + "name": "search", + "description": description, + "parameters": map[string]any{"type": "object"}, + }}, + }, + } + api := &directAPI{ + variation: &server, + tools: map[string]versionedTool{ + "search": {Tool: tool, Version: 3}, + }, + } + + stdout, _, err := runPrompt(t, root, api, "--dry-run", "--diff", "--output", "plaintext") + + require.NoError(t, err) + assert.Contains(t, stdout, "instructions changed in LaunchDarkly") + assert.NotContains(t, stdout, "model changed in LaunchDarkly") + assert.NotContains(t, stdout, `"type": "function"`) + assert.NotContains(t, stdout, `Tool "search"`) + + _, _, err = runPrompt(t, root, api, "--yes") + require.NoError(t, err) + resources, err := synclocal.CompileWorkspace(root) + require.NoError(t, err) + require.Len(t, resources, 1) + assert.Equal(t, map[string]any{ + "modelName": "claude-sonnet-4", + "parameters": map[string]any{ + "temperature": 0.2, + }, + }, resources[0].Variation.Model) + assert.Equal(t, []syncdomain.AttachmentRef{{Key: "search"}}, resources[0].Variation.Tools) +} + func TestPromptFirstSyncCreatesUpsertVariation(t *testing.T) { root := initRepository(t) local := variation("New") diff --git a/internal/sync/prompt/diff.go b/internal/sync/prompt/diff.go index fa7820f57..5b2f44867 100644 --- a/internal/sync/prompt/diff.go +++ b/internal/sync/prompt/diff.go @@ -73,6 +73,7 @@ func variationJSON(variation *syncdomain.Variation) json.RawMessage { return nil } withoutRefs := *variation + withoutRefs.Model = syncdomain.CanonicalModel(withoutRefs.Model, withoutRefs.Tools, withoutRefs.Skills) withoutRefs.Tools, withoutRefs.Skills = nil, nil data, _ := json.Marshal(withoutRefs) return data diff --git a/internal/sync/prompt/plan_test.go b/internal/sync/prompt/plan_test.go index 010c5fb6a..b931d001a 100644 --- a/internal/sync/prompt/plan_test.go +++ b/internal/sync/prompt/plan_test.go @@ -124,6 +124,37 @@ func TestBuildPlanOmitsDiffForEquivalentAPIDefaults(t *testing.T) { require.Empty(t, plan.Resources[0].Diff) } +func TestBuildPlanKeepsRealModelParameterChangesWithExpandedTools(t *testing.T) { + local := testVariation("example") + local.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + local.Model = map[string]any{ + "modelName": "gpt-5", + "parameters": map[string]any{ + "temperature": 0.2, + }, + } + server := local + server.Model = map[string]any{ + "modelName": "gpt-5", + "parameters": map[string]any{ + "temperature": 0.7, + "tools": []any{map[string]any{"type": "function", "name": "search"}}, + }, + } + + id := testResourceID() + manifest := syncmanifest.New() + manifest.SetFingerprint(id, fingerprintOf(t, local)) + plan := BuildPlan(manifest, localResources(&local, false), map[ResourceID]ServerResource{ + id: {Variation: &server, ConfigMode: syncdomain.VariationModeAgent}, + }) + + require.Equal(t, ActionUpdateLocal, plan.Resources[0].Action) + require.Equal(t, "model changed in LaunchDarkly", planReason(plan.Resources[0])) + require.NotContains(t, string(plan.Resources[0].Diff["variation"].Before), `"tools"`) + require.Contains(t, string(plan.Resources[0].Diff["variation"].Before), `"temperature":0.7`) +} + func TestBuildPlanRejectsParentConfigModeMismatch(t *testing.T) { local := testVariation("local") id := testResourceID() From cd1cd8e851af7e1ce858ece9cdecfc7f77f8f143 Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Thu, 22 Oct 2026 23:58:53 +0000 Subject: [PATCH 2/2] fix: preserve model skill parameters --- internal/sync/api/catalog.go | 2 +- internal/sync/api/catalog_test.go | 8 ++++++-- internal/sync/fingerprint.go | 12 ++++-------- internal/sync/fingerprint_test.go | 18 +++++++++++++++++- internal/sync/prompt/diff.go | 2 +- 5 files changed, 29 insertions(+), 13 deletions(-) diff --git a/internal/sync/api/catalog.go b/internal/sync/api/catalog.go index 170aa4008..ae7872b7b 100644 --- a/internal/sync/api/catalog.go +++ b/internal/sync/api/catalog.go @@ -157,7 +157,7 @@ func (config *Config) UnmarshalJSON(data []byte) error { for _, variation := range response.Variations { if variation.State != "archived" { canonical := variation.Variation - canonical.Model = syncdomain.CanonicalModel(canonical.Model, canonical.Tools, canonical.Skills) + canonical.Model = syncdomain.CanonicalModel(canonical.Model, canonical.Tools) config.Variations = append(config.Variations, canonical) } } diff --git a/internal/sync/api/catalog_test.go b/internal/sync/api/catalog_test.go index 9e4532386..54c53b2d3 100644 --- a/internal/sync/api/catalog_test.go +++ b/internal/sync/api/catalog_test.go @@ -178,10 +178,12 @@ func TestCatalogClientConfigRemovesExpandedToolsFromModelParameters(t *testing.T "modelName": "claude", "parameters": { "temperature": 0.2, - "tools": [{"type": "function", "name": "search"}] + "tools": [{"type": "function", "name": "search"}], + "skills": [{"key": "support"}] } }, - "tools": [{"key": "search", "version": 3}] + "tools": [{"key": "search", "version": 3}], + "skills": [{"key": "support", "version": 2}] }] }`)}} @@ -193,9 +195,11 @@ func TestCatalogClientConfigRemovesExpandedToolsFromModelParameters(t *testing.T "modelName": "claude", "parameters": map[string]any{ "temperature": 0.2, + "skills": []any{map[string]any{"key": "support"}}, }, }, config.Variations[0].Model) assert.Equal(t, []syncdomain.AttachmentRef{{Key: "search", Version: 3}}, config.Variations[0].Tools) + assert.Equal(t, []syncdomain.AttachmentRef{{Key: "support", Version: 2}}, config.Variations[0].Skills) } func TestCatalogClientConfigRejectsInvalidResponse(t *testing.T) { diff --git a/internal/sync/fingerprint.go b/internal/sync/fingerprint.go index bd11adf72..889c9790d 100644 --- a/internal/sync/fingerprint.go +++ b/internal/sync/fingerprint.go @@ -76,7 +76,7 @@ func (variation Variation) canonical() Variation { variation.Attachments[index] = CanonicalAttachment(variation.Attachments[index]) } variation.SortAttachments() - variation.Model = CanonicalModel(variation.Model, variation.Tools, variation.Skills) + variation.Model = CanonicalModel(variation.Model, variation.Tools) switch variation.Mode { case VariationModeAgent: @@ -108,19 +108,15 @@ func unversionedRefs(refs []AttachmentRef) []AttachmentRef { return unversioned } -// CanonicalModel returns a copy without defaults and expanded attachment -// content that the variation API adds. Tool and skill references own that -// content, so model parameters do not. -func CanonicalModel(model map[string]any, tools, skills []AttachmentRef) map[string]any { +// CanonicalModel returns a copy without defaults and expanded tool content +// that the variation API adds. Tool references own that content. +func CanonicalModel(model map[string]any, tools []AttachmentRef) map[string]any { model = maps.Clone(model) if parameters, ok := model["parameters"].(map[string]any); ok { parameters = maps.Clone(parameters) if len(tools) != 0 { delete(parameters, "tools") } - if len(skills) != 0 { - delete(parameters, "skills") - } if len(parameters) == 0 { delete(model, "parameters") } else { diff --git a/internal/sync/fingerprint_test.go b/internal/sync/fingerprint_test.go index cba4a940e..30a40b07d 100644 --- a/internal/sync/fingerprint_test.go +++ b/internal/sync/fingerprint_test.go @@ -137,7 +137,7 @@ func TestFingerprintVariationNormalizesModelDefaults(t *testing.T) { require.Contains(t, withDefaults.Model, "custom") } -func TestFingerprintVariationExcludesExpandedAttachmentParameters(t *testing.T) { +func TestFingerprintVariationExcludesExpandedToolParameters(t *testing.T) { base := Variation{ Mode: VariationModeAgent, Key: "agent", @@ -177,6 +177,22 @@ func TestFingerprintVariationExcludesExpandedAttachmentParameters(t *testing.T) require.Equal(t, baseFingerprint, expandedFingerprint) require.NotEqual(t, baseFingerprint, changedFingerprint) require.Contains(t, expanded.Model["parameters"], "tools") + + withoutSkillParameter := base + withoutSkillParameter.Skills = []AttachmentRef{{Key: "support"}} + withSkillParameter := withoutSkillParameter + withSkillParameter.Model = map[string]any{ + "modelName": "claude", + "parameters": map[string]any{ + "temperature": 0.2, + "skills": []any{map[string]any{"key": "support"}}, + }, + } + withoutSkillFingerprint, err := FingerprintVariation("project", "config/agent", withoutSkillParameter) + require.NoError(t, err) + withSkillFingerprint, err := FingerprintVariation("project", "config/agent", withSkillParameter) + require.NoError(t, err) + require.NotEqual(t, withoutSkillFingerprint, withSkillFingerprint) } func TestFingerprintVariationPreservesMeaningfulModelChanges(t *testing.T) { diff --git a/internal/sync/prompt/diff.go b/internal/sync/prompt/diff.go index 5b2f44867..8e37710b2 100644 --- a/internal/sync/prompt/diff.go +++ b/internal/sync/prompt/diff.go @@ -73,7 +73,7 @@ func variationJSON(variation *syncdomain.Variation) json.RawMessage { return nil } withoutRefs := *variation - withoutRefs.Model = syncdomain.CanonicalModel(withoutRefs.Model, withoutRefs.Tools, withoutRefs.Skills) + withoutRefs.Model = syncdomain.CanonicalModel(withoutRefs.Model, withoutRefs.Tools) withoutRefs.Tools, withoutRefs.Skills = nil, nil data, _ := json.Marshal(withoutRefs) return data