diff --git a/internal/sync/prompt/acceptance_test.go b/internal/sync/prompt/acceptance_test.go index ea22d9ee..ee26b09e 100644 --- a/internal/sync/prompt/acceptance_test.go +++ b/internal/sync/prompt/acceptance_test.go @@ -682,6 +682,236 @@ func TestPromptUpdateResolvesOmittedModelConfigVersionToLatest(t *testing.T) { requireOnlyReads(t, api.requests) } +// A push of a variation that newly attaches a tracked tool must not publish +// the local copy over a change in LaunchDarkly. A dry run plans a pull of the +// tool and changes nothing. The sync then pulls the tool, and the variation +// pins the pulled version. +func TestPromptPushOfANewToolReferenceKeepsTheLaunchDarklyChange(t *testing.T) { + root := initRepository(t) + baseline := variation("Baseline") + writeManifest(t, root, baseline) + trackTool(t, root, searchTool("old")) + local := baseline + local.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + local.Attachments = []syncdomain.Attachment{toolAttachment(searchTool("old"), 0)} + writeVariation(t, root, local, true) + api := &directAPI{variation: pointer(baseline), tools: map[string]versionedTool{"search": {Tool: searchTool("new"), Version: 2}}} + + stdout, _, err := runPrompt(t, root, api, "--dry-run", "--output", "json") + require.NoError(t, err) + assert.Equal(t, []string{"tool search update_local", "variation support/default update_server"}, planRows(t, stdout)) + requireOnlyReads(t, api.requests) + assert.Contains(t, readLocalFile(t, root, "tools/search.json"), `"description": "old"`) + + _, _, err = runPrompt(t, root, api, "--yes") + + require.NoError(t, err) + assert.Zero(t, countRequests(api.requests, "PATCH", "/ai-tools/search"), "the push must not publish the old local copy") + assert.Equal(t, "new", *api.tools["search"].Description) + assert.Equal(t, []syncdomain.AttachmentRef{{Key: "search", Version: 2}}, api.variation.Tools) + assert.Contains(t, readLocalFile(t, root, "tools/search.json"), `"description": "new"`) +} + +// A pull of a variation that newly references a tracked tool must not write +// the LaunchDarkly copy over a local edit. A dry run plans a push of the tool +// and changes nothing. The sync then pushes the tool, and the file keeps the +// edit. The next sync only repins the variation. +func TestPromptPullOfANewToolReferenceKeepsTheLocalEdit(t *testing.T) { + root := initRepository(t) + baseline := variation("Baseline") + writeVariation(t, root, baseline, true) + writeManifest(t, root, baseline) + trackTool(t, root, searchTool("old")) + writeLocalFile(t, root, "tools/search.json", + `{"formatVersion": 1, "key": "search", "description": "edited", "schema": {"type": "object"}}`) + server := baseline + server.Tools = []syncdomain.AttachmentRef{{Key: "search", Version: 1}} + api := &directAPI{variation: &server, tools: map[string]versionedTool{"search": {Tool: searchTool("old"), Version: 1}}} + + stdout, _, err := runPrompt(t, root, api, "--dry-run", "--output", "json") + require.NoError(t, err) + assert.Equal(t, []string{"tool search update_server", "variation support/default update_local"}, planRows(t, stdout)) + requireOnlyReads(t, api.requests) + assert.Contains(t, readLocalFile(t, root, "tools/search.json"), `"description": "edited"`) + + _, _, err = runPrompt(t, root, api, "--yes") + + require.NoError(t, err) + assert.Contains(t, readLocalFile(t, root, "tools/search.json"), `"description": "edited"`, "the local edit stays") + assert.Equal(t, "edited", *api.tools["search"].Description) + assert.Equal(t, 2, api.tools["search"].Version) + stdout, _, err = runPrompt(t, root, api, "--dry-run", "--output", "json") + require.NoError(t, err) + assert.Equal(t, []string{"variation support/default update_server"}, planRows(t, stdout)) + assert.Contains(t, stdout, `"attachment versions"`, "the variation only repins the tool") +} + +// The same holds for a skill. +func TestPromptPullOfANewSkillReferenceKeepsTheLocalEdit(t *testing.T) { + root := initRepository(t) + baseline := variation("Baseline") + writeVariation(t, root, baseline, true) + writeManifest(t, root, baseline) + summarize := syncdomain.Skill{Key: "summarize", Name: "Summarize", Description: "Summarize", Markdown: "Old.\n"} + trackAttachment(t, root, syncdomain.Attachment{Kind: syncdomain.AttachmentSkill, Version: 1, Skill: &summarize}) + writeLocalFile(t, root, "skills/summarize.md", "---\nkey: summarize\ndescription: Summarize\n---\n\nEdited.\n") + server := baseline + server.Skills = []syncdomain.AttachmentRef{{Key: "summarize", Version: 1}} + api := &directAPI{variation: &server, skills: map[string]versionedSkill{"summarize": {Skill: summarize, Version: 1}}} + + _, _, err := runPrompt(t, root, api, "--yes") + + require.NoError(t, err) + assert.Contains(t, readLocalFile(t, root, "skills/summarize.md"), "Edited.", "the local edit stays") + assert.Equal(t, "Edited.\n", api.skills["summarize"].Markdown) +} + +// When both sides of a newly referenced tool changed, the tool row is a +// conflict, and one choice covers the tool and the variation. +func TestPromptNewToolReferenceWithTwoChangesIsAConflict(t *testing.T) { + root := initRepository(t) + baseline := variation("Baseline") + writeManifest(t, root, baseline) + trackTool(t, root, searchTool("old")) + local := baseline + local.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + local.Attachments = []syncdomain.Attachment{toolAttachment(searchTool("edited"), 0)} + writeVariation(t, root, local, true) + api := &directAPI{variation: pointer(baseline), tools: map[string]versionedTool{"search": {Tool: searchTool("new"), Version: 2}}} + + _, stderr, err := runPrompt(t, root, api, "--yes", "--no-input") + + require.ErrorContains(t, err, "conflict resolution requires --conflict or --resolve without interactive input") + assert.Contains(t, stderr, "Conflict: tool production/search changed on both sides. "+ + "It shares a changed tool or skill with variation production/support/default, so one choice covers them all.") + requireOnlyReads(t, api.requests) +} + +// A tool that only the local version of a conflicted variation newly uses +// keeps its own row and action. A choice of LaunchDarkly, which does not use +// the tool, drops that row, so the choice does not pull the tool over its +// local edit. +func TestPromptConflictChoiceDoesNotPullAToolThatOnlyTheLocalVersionUses(t *testing.T) { + root := initRepository(t) + baseline := variation("Baseline") + writeManifest(t, root, baseline) + trackTool(t, root, searchTool("old")) + local := variation("Local") + local.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + local.Attachments = []syncdomain.Attachment{toolAttachment(searchTool("edited"), 0)} + writeVariation(t, root, local, true) + server := variation("LaunchDarkly") + api := &directAPI{variation: &server, tools: map[string]versionedTool{"search": {Tool: searchTool("old"), Version: 1}}} + + stdout, _, err := runPrompt(t, root, api, "--dry-run", "--output", "json") + require.ErrorContains(t, err, "cannot sync conflicted resource variation production/support/default") + assert.Equal(t, []string{"tool search update_server", "variation support/default conflict"}, planRows(t, stdout)) + + stdout, _, err = runPrompt(t, root, api, "--conflict", "launchdarkly", "--yes", "--no-input", "--output", "json") + + require.NoError(t, err) + assert.Equal(t, []string{"variation support/default update_local"}, planRows(t, stdout), "the choice does not pull the tool") + assert.Equal(t, "old", *api.tools["search"].Description) + assert.Zero(t, countRequests(api.requests, "PATCH", "/ai-tools/search")) +} + +// A choice of local for a conflicted variation that newly uses a tool must not +// publish the old local copy over a LaunchDarkly change of the tool. The tool +// row keeps its own pull, and the variation pins the pulled version. +func TestPromptConflictChoiceOfLocalKeepsTheLaunchDarklyChangeOfANewTool(t *testing.T) { + root := initRepository(t) + baseline := variation("Baseline") + writeManifest(t, root, baseline) + trackTool(t, root, searchTool("old")) + local := variation("Local") + local.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + local.Attachments = []syncdomain.Attachment{toolAttachment(searchTool("old"), 0)} + writeVariation(t, root, local, true) + server := variation("LaunchDarkly") + api := &directAPI{variation: &server, tools: map[string]versionedTool{"search": {Tool: searchTool("new"), Version: 2}}} + + _, _, err := runPrompt(t, root, api, "--conflict", "local", "--yes", "--no-input") + + require.NoError(t, err) + assert.Zero(t, countRequests(api.requests, "PATCH", "/ai-tools/search"), "the push must not publish the old local copy") + assert.Equal(t, "new", *api.tools["search"].Description) + assert.Equal(t, "Local", api.variation.Name) + assert.Equal(t, []syncdomain.AttachmentRef{{Key: "search", Version: 2}}, api.variation.Tools) + assert.Contains(t, readLocalFile(t, root, "tools/search.json"), `"description": "new"`) +} + +// When a tool that a conflicted variation newly uses changed on both sides, +// the tool and the variation form one conflict with one choice. +func TestPromptConflictWithANewToolThatChangedOnBothSidesIsOneChoice(t *testing.T) { + root := initRepository(t) + baseline := variation("Baseline") + writeManifest(t, root, baseline) + trackTool(t, root, searchTool("old")) + local := variation("Local") + local.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + local.Attachments = []syncdomain.Attachment{toolAttachment(searchTool("edited"), 0)} + writeVariation(t, root, local, true) + server := variation("LaunchDarkly") + api := &directAPI{variation: &server, tools: map[string]versionedTool{"search": {Tool: searchTool("new"), Version: 2}}} + + _, stderr, err := runPrompt(t, root, api, "--yes", "--no-input") + require.ErrorContains(t, err, "conflict resolution requires --conflict or --resolve without interactive input") + assert.Contains(t, stderr, "Conflict: tool production/search changed on both sides. "+ + "It shares a changed tool or skill with variation production/support/default, so one choice covers them all.") + + _, _, err = runPrompt(t, root, api, "--conflict", "local", "--yes", "--no-input") + require.NoError(t, err) + assert.Equal(t, "edited", *api.tools["search"].Description) + assert.Equal(t, []syncdomain.AttachmentRef{{Key: "search", Version: 3}}, api.variation.Tools) +} + +// A newly referenced tool whose two sides are equal cannot be overwritten, so +// the plan has no row for it. +func TestPromptNewToolReferenceWithEqualSidesPlansNoToolRow(t *testing.T) { + root := initRepository(t) + baseline := variation("Baseline") + writeManifest(t, root, baseline) + trackTool(t, root, searchTool("same")) + local := baseline + local.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + local.Attachments = []syncdomain.Attachment{toolAttachment(searchTool("same"), 0)} + writeVariation(t, root, local, true) + api := &directAPI{variation: pointer(baseline), tools: map[string]versionedTool{"search": {Tool: searchTool("same"), Version: 1}}} + + stdout, _, err := runPrompt(t, root, api, "--dry-run", "--output", "json") + + require.NoError(t, err) + assert.Equal(t, []string{"variation support/default update_server"}, planRows(t, stdout)) +} + +func searchTool(description string) syncdomain.Tool { + return syncdomain.Tool{Key: "search", Description: pointer(description), Schema: map[string]any{"type": "object"}} +} + +// trackTool records the tool in sync.lock and in the remote manifest, as a +// sync that synced it does. +func trackTool(t *testing.T, root string, tool syncdomain.Tool) { + t.Helper() + trackAttachment(t, root, toolAttachment(tool, 1)) +} + +func trackAttachment(t *testing.T, root string, attachment syncdomain.Attachment) { + t.Helper() + lock := readLock(t, root) + require.NoError(t, lock.SetAttachments("production", []syncdomain.Attachment{attachment})) + require.NoError(t, syncmanifest.WriteLock(synclocal.NewStore(root), lock)) + fingerprint, err := syncdomain.FingerprintAttachment("production", attachment) + require.NoError(t, err) + manifestsByRoot[root].Items = append(manifestsByRoot[root].Items, syncapi.SyncManifestResource{ + ResourceKind: syncdomain.Kind(attachment.Kind), ResourceLookupKey: attachment.Key(), Fingerprint: fingerprint, Version: 1, + }) +} + +func readLocalFile(t *testing.T, root, projectPath string) string { + t.Helper() + return string(must(os.ReadFile(filepath.Join(root, syncdomain.RootDir, "production", filepath.FromSlash(projectPath))))) +} + func TestPromptPushesLocalChangeAndAdvancesManifest(t *testing.T) { root := initRepository(t) baseline := variation("Baseline") diff --git a/internal/sync/prompt/conflict.go b/internal/sync/prompt/conflict.go index 83eede28..273fa0f5 100644 --- a/internal/sync/prompt/conflict.go +++ b/internal/sync/prompt/conflict.go @@ -200,8 +200,11 @@ func groupConflicts(plan Plan) []conflictGroup { for next := 0; next < len(members); next++ { for candidate := range plan.Resources { row := plan.Resources[candidate] + // A row that only conflicts need keeps its own action, unless + // it is a conflict itself. applyConflictResolutions decides it. if visited[candidate] || row.Action == ActionError || + (len(row.conflictUses) != 0 && row.Action != ActionConflict) || (row.ID.Kind != syncdomain.KindVariation && row.Action == ActionDeleteLocal) || !sharesChangedAttachment(plan.Resources[members[next]], row) { continue @@ -305,17 +308,34 @@ func writeConflictChoice(output io.Writer, choice conflictChoice) { // resource has the action for its chosen side. The original plan stays as // the reviewed record. func applyConflictResolutions(plan Plan, resolutions map[ResourceID]ConflictResolution) Plan { - resolved := Plan{Resources: slices.Clone(plan.Resources)} - for index := range resolved.Resources { - resource := &resolved.Resources[index] + resolved := Plan{Resources: make([]PlannedResource, 0, len(plan.Resources))} + for _, resource := range plan.Resources { + if !chosenSideUses(resource, resolutions) { + continue + } if resolution, ok := resolutions[resource.ID]; ok { - resource.Action = resolvedConflictAction(*resource, resolution) + resource.Action = resolvedConflictAction(resource, resolution) } + resolved.Resources = append(resolved.Resources, resource) } checkResolvedAttachmentDeletes(resolved.Resources) return resolved } +// chosenSideUses reports whether a row stays after the conflict choices. A +// row that only conflicts need stays when the side that the user chose for +// one of them uses its tool or skill. Then the row keeps its own action, so +// that the variation does not write its own copy over the other side. +func chosenSideUses(resource PlannedResource, resolutions map[ResourceID]ConflictResolution) bool { + if len(resource.conflictUses) == 0 || resource.Action == ActionConflict { + return true + } + return slices.ContainsFunc(resource.conflictUses, func(use conflictUse) bool { + resolution := resolutions[use.variation] + return use.local && resolution == ConflictUseLocal || !use.local && resolution == ConflictUseLaunchDarkly + }) +} + // resolvedConflictAction returns the action that makes the chosen side win. func resolvedConflictAction(resource PlannedResource, resolution ConflictResolution) Action { switch resolution { diff --git a/internal/sync/prompt/plan.go b/internal/sync/prompt/plan.go index ad6e281f..11b8f518 100644 --- a/internal/sync/prompt/plan.go +++ b/internal/sync/prompt/plan.go @@ -73,6 +73,9 @@ type PlannedResource struct { // changedAttachments are the tools and skills whose content differs // between the local file and LaunchDarkly. changedAttachments []ResourceID + // conflictUses are the conflicted variations that newly use the tool or + // skill of this row on one side, when no push or pull needs the row. + conflictUses []conflictUse // restoreRef is the link that a restored variation file keeps. restoreRef *syncdomain.Reference } @@ -87,6 +90,28 @@ func (resource PlannedResource) hasServer() bool { return resource.Server != nil || resource.ServerAttachment != nil } +// writtenSides returns the variation that a variation row writes, and the +// variation that it replaces. Both are nil for a row that writes none. +func (resource PlannedResource) writtenSides() (source, target *syncdomain.Variation) { + switch { + case resource.ID.Kind != syncdomain.KindVariation: + return nil, nil + case resource.Action.changesServer(): + return resource.Local, resource.Server + case resource.Action == ActionUpdateLocal: + return resource.Server, resource.Local + default: + return nil, nil + } +} + +// conflictUse is a conflicted variation that newly uses a tool or skill on one +// side. local is true when that side is the local file. +type conflictUse struct { + variation ResourceID + local bool +} + // Plan is the sync decision for each resource, in identity order. type Plan struct { Resources []PlannedResource diff --git a/internal/sync/prompt/state.go b/internal/sync/prompt/state.go index 4383abd0..9de01381 100644 --- a/internal/sync/prompt/state.go +++ b/internal/sync/prompt/state.go @@ -83,10 +83,15 @@ func loadWorkspacePlan( return Plan{}, nil, err } plan.Resources = append(plan.Resources, rows...) - slices.SortFunc(plan.Resources, func(left, right PlannedResource) int { - return syncdomain.CompareResourceIDs(left.ID, right.ID) - }) } + referenced, err := planReferencedAttachments(repositoryRoot, baseline.Lock, attachments, plan.Resources) + if err != nil { + return Plan{}, nil, err + } + plan.Resources = append(plan.Resources, referenced...) + slices.SortFunc(plan.Resources, func(left, right PlannedResource) int { + return syncdomain.CompareResourceIDs(left.ID, right.ID) + }) checkAttachmentBaselines(plan.Resources, baseline.Lock) checkAttachmentDeletes(plan.Resources, variations) @@ -180,6 +185,85 @@ func planAttachmentRows( return planAttachments(tracked, local, server), nil } +// planReferencedAttachments plans a row for each tool and skill that a +// variation row writes to a side that does not use it yet, when the tool or +// skill exists on both sides with different content. Without the row, the +// variation row would write its own copy over the other side. The row decides +// the content, and executePlan applies it before the variation rows. +// checkAttachmentBaselines covers the tools and skills that both sides use. +// +// A conflicted variation writes the side that the user chooses, so each side +// of it counts. A row that only conflicts need records them in conflictUses, +// and applyConflictResolutions keeps it only for the chosen side. +func planReferencedAttachments( + repositoryRoot string, + lock syncmanifest.Manifest, + attachments *attachmentCache, + resources []PlannedResource, +) ([]PlannedResource, error) { + written := make(map[ResourceID]bool) + conflictUses := make(map[ResourceID][]conflictUse) + for _, resource := range resources { + for _, id := range newReferences(resource) { + written[id] = true + } + if resource.ID.Kind == syncdomain.KindVariation && resource.Action == ActionConflict { + for _, id := range newAttachments(resource.ID.ProjectKey, resource.Local, resource.Server) { + conflictUses[id] = append(conflictUses[id], conflictUse{variation: resource.ID, local: true}) + } + for _, id := range newAttachments(resource.ID.ProjectKey, resource.Server, resource.Local) { + conflictUses[id] = append(conflictUses[id], conflictUse{variation: resource.ID}) + } + } + } + referenced := func(id ResourceID) bool { + return (written[id] || len(conflictUses[id]) != 0) && + !slices.ContainsFunc(resources, func(resource PlannedResource) bool { return resource.ID == id }) + } + + rows, err := planAttachmentRows(repositoryRoot, lock, attachments, referenced) + if err != nil { + return nil, err + } + rows = slices.DeleteFunc(rows, func(row PlannedResource) bool { + return !row.hasLocal() || !row.hasServer() || row.LocalFingerprint == row.ServerFingerprint + }) + for index := range rows { + if !written[rows[index].ID] { + rows[index].conflictUses = conflictUses[rows[index].ID] + } + } + return rows, nil +} + +// newReferences returns each tool and skill that a push or a pull writes to a +// side that does not use it yet. +func newReferences(resource PlannedResource) []ResourceID { + source, target := resource.writtenSides() + return newAttachments(resource.ID.ProjectKey, source, target) +} + +// newAttachments returns each tool and skill that source uses and target does +// not. +func newAttachments(projectKey string, source, target *syncdomain.Variation) []ResourceID { + if source == nil { + return nil + } + var ids []ResourceID + for _, attachment := range source.Attachments { + if target == nil || !hasAttachment(*target, attachment) { + ids = append(ids, attachment.ID(projectKey)) + } + } + return ids +} + +// hasAttachment reports whether a variation uses the tool or skill of attachment. +func hasAttachment(variation syncdomain.Variation, attachment syncdomain.Attachment) bool { + _, ok := variation.Attachment(attachment.Kind, attachment.Key()) + return ok +} + // checkAttachmentBaselines makes sure that a variation row does not move a // tool or a skill past a change on the other side. sync.lock records the // state of each tool and skill on its own. A row becomes a conflict when the @@ -193,18 +277,8 @@ func checkAttachmentBaselines(resources []PlannedResource, lock syncmanifest.Man for index := range resources { resource := &resources[index] - var source, target *syncdomain.Variation - switch { - case resource.ID.Kind != syncdomain.KindVariation: - continue - case resource.Action.changesServer(): - source, target = resource.Local, resource.Server - case resource.Action == ActionUpdateLocal: - source, target = resource.Server, resource.Local - default: - continue - } - if target == nil { + source, target := resource.writtenSides() + if source == nil || target == nil { continue } for _, id := range resource.changedAttachments {