fix: plan a tool or skill row when a variation newly references it

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")

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 {

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