From daee44a7664ee5b7e6ab27fa81dbdab1448a3e22 Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Thu, 22 Oct 2026 05:24:33 +0000 Subject: [PATCH 1/4] feat: plan standalone tool and skill rows by sync scope A sync can now plan tools and skills as rows of their own. A scope selects the kinds to plan: prompts, tools, skills, or all. A tool or skill row uses the same three-way rules, fingerprints, and sync.lock rows as a variation, and sorts before the variations that pin it. No command selects a scope yet, so sync prompts plans the same rows as before, with one fix: a variation row that would push or pull a tool or skill past an unsynced change on the other side is now a conflict. --- internal/sync/attachment.go | 11 + internal/sync/local/attachment.go | 30 +++ internal/sync/local/attachment_test.go | 57 +++++ internal/sync/prompt/acceptance_test.go | 152 ++++++++++++- internal/sync/prompt/action.go | 6 +- internal/sync/prompt/conflict.go | 4 +- internal/sync/prompt/conflict_test.go | 23 ++ internal/sync/prompt/diff.go | 47 +++- internal/sync/prompt/output.go | 35 ++- internal/sync/prompt/output_test.go | 18 +- internal/sync/prompt/plan.go | 137 ++++++++++- internal/sync/prompt/plan_test.go | 220 ++++++++++++++++++ internal/sync/prompt/state.go | 289 +++++++++++++++++++++--- internal/sync/prompt/sync.go | 5 +- internal/sync/resource.go | 46 ++++ internal/sync/scope.go | 40 ++++ internal/sync/scope_test.go | 35 +++ 17 files changed, 1087 insertions(+), 68 deletions(-) diff --git a/internal/sync/attachment.go b/internal/sync/attachment.go index ffe642f7e..5cfdf1811 100644 --- a/internal/sync/attachment.go +++ b/internal/sync/attachment.go @@ -76,6 +76,17 @@ func (attachment Attachment) ID(projectKey string) ResourceID { return ResourceID{Kind: Kind(attachment.Kind), ProjectKey: projectKey, LookupKey: attachment.Key()} } +// SyncedAttachment is one tool or skill file of the local workspace. +type SyncedAttachment struct { + ProjectKey string + Attachment Attachment +} + +// ID returns the identity of the tool or the skill. +func (attachment SyncedAttachment) ID() ResourceID { + return attachment.Attachment.ID(attachment.ProjectKey) +} + // CanonicalAttachment removes the runtime version, local-only flags, and // server-owned metadata. Two attachments with the same canonical form have // the same content. diff --git a/internal/sync/local/attachment.go b/internal/sync/local/attachment.go index c88a22df8..34952563a 100644 --- a/internal/sync/local/attachment.go +++ b/internal/sync/local/attachment.go @@ -41,6 +41,36 @@ type OrphanedAttachment struct { Path string } +// CompileAttachments reads each tool and skill file that include accepts. It +// does not read the other files, so an invalid file outside include does not +// stop the read. Like CompileWorkspace, it rejects a symbolic link in a +// managed path. +func CompileAttachments(repositoryRoot string, include func(syncdomain.ResourceID) bool) ([]syncdomain.SyncedAttachment, error) { + root, err := filepath.EvalSymlinks(repositoryRoot) + if err != nil { + return nil, fmt.Errorf("resolve repository root: %w", err) + } + files, err := SourceFiles(root) + if err != nil { + return nil, err + } + + fsys := workspaceFS{root: root} + var attachments []syncdomain.SyncedAttachment + for _, file := range files { + id, ok := ParseManagedPath(file) + if !ok || id.Kind == syncdomain.KindVariation || !include(id) { + continue + } + attachment, err := readAttachment(fsys, id.ProjectKey, syncdomain.AttachmentKind(id.Kind), id.LookupKey) + if err != nil { + return nil, ParseError{Path: file, Err: err} + } + attachments = append(attachments, syncdomain.SyncedAttachment{ProjectKey: id.ProjectKey, Attachment: attachment}) + } + return attachments, nil +} + // readAttachment reads the file of one tool or skill. func readAttachment(fsys fs.FS, projectKey string, kind syncdomain.AttachmentKind, key string) (syncdomain.Attachment, error) { switch kind { diff --git a/internal/sync/local/attachment_test.go b/internal/sync/local/attachment_test.go index 3d245f3cc..7f7fc189d 100644 --- a/internal/sync/local/attachment_test.go +++ b/internal/sync/local/attachment_test.go @@ -267,6 +267,63 @@ func TestOrphanedAttachmentsFindsFlatSkillFile(t *testing.T) { require.ErrorIs(t, statErr, os.ErrNotExist) } +func TestCompileAttachmentsReadsStandaloneAndReferencedFiles(t *testing.T) { + root := t.TempDir() + search := syncdomain.Tool{Key: "search", Schema: map[string]any{"type": "object"}} + variation := syncdomain.Variation{ + Mode: syncdomain.VariationModeAgent, Key: "default", Name: "Default", Instructions: "Help.", + Tools: []syncdomain.AttachmentRef{{Key: "search"}}, + Attachments: []syncdomain.Attachment{{Kind: syncdomain.AttachmentTool, Tool: &search}}, + } + _, err := NewStore(root).Add([]VariationFile{{ProjectKey: "project", ConfigKey: "config", Variation: variation}}) + require.NoError(t, err) + writeManagedFile(t, root, "project/tools/refund.json", `{"formatVersion": 1, "upsert": true, "key": "refund", "schema": {}}`) + writeManagedFile(t, root, "project/skills/triage.md", "---\nkey: triage\ndescription: Triage\n---\n\nPick a queue.\n") + writeManagedFile(t, root, "other/tools/broken.json", "not json") + + attachments, err := CompileAttachments(root, func(id syncdomain.ResourceID) bool { return id.ProjectKey == "project" }) + + require.NoError(t, err) + var ids []string + for _, attachment := range attachments { + ids = append(ids, attachment.ID().Label()) + } + assert.Equal(t, []string{"skill project/triage", "tool project/refund", "tool project/search"}, ids) + assert.True(t, attachments[1].Attachment.Upsert) +} + +func TestCompileAttachmentsReportsTheInvalidFile(t *testing.T) { + root := t.TempDir() + writeManagedFile(t, root, "project/tools/broken.json", "not json") + + _, err := CompileAttachments(root, func(syncdomain.ResourceID) bool { return true }) + + var parseErr ParseError + require.ErrorAs(t, err, &parseErr) + assert.Equal(t, ".launchdarkly/project/tools/broken.json", parseErr.Path) + assert.ErrorContains(t, err, `parse tool "broken"`) +} + +func TestCompileAttachmentsRejectsSymlink(t *testing.T) { + root := t.TempDir() + external := filepath.Join(root, "external.json") + require.NoError(t, os.WriteFile(external, []byte(`{"formatVersion":1,"key":"search","schema":{}}`), 0o644)) + toolPath := filepath.Join(root, ".launchdarkly", "project", "tools", "search.json") + require.NoError(t, os.MkdirAll(filepath.Dir(toolPath), 0o755)) + require.NoError(t, os.Symlink(external, toolPath)) + + _, err := CompileAttachments(root, func(syncdomain.ResourceID) bool { return true }) + + require.ErrorContains(t, err, "symbolic links are not supported") +} + +func writeManagedFile(t *testing.T, root, relativePath, content string) { + t.Helper() + path := filepath.Join(root, syncdomain.RootDir, filepath.FromSlash(relativePath)) + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) +} + func TestCompileWorkspaceRejectsAttachmentSymlink(t *testing.T) { root := t.TempDir() wrapperPath := filepath.Join(root, ".launchdarkly", "project", "configs", "config", "default.prompt.md") diff --git a/internal/sync/prompt/acceptance_test.go b/internal/sync/prompt/acceptance_test.go index 44a92ecc5..470c1a24c 100644 --- a/internal/sync/prompt/acceptance_test.go +++ b/internal/sync/prompt/acceptance_test.go @@ -1,9 +1,11 @@ package prompt_test import ( + "bytes" "encoding/json" "errors" "fmt" + "io" "net/http" "net/url" "os" @@ -24,6 +26,7 @@ import ( syncapi "github.com/launchdarkly/ldcli/internal/sync/api" synclocal "github.com/launchdarkly/ldcli/internal/sync/local" syncmanifest "github.com/launchdarkly/ldcli/internal/sync/manifest" + syncprompt "github.com/launchdarkly/ldcli/internal/sync/prompt" syncreference "github.com/launchdarkly/ldcli/internal/sync/reference" ) @@ -112,7 +115,7 @@ func (api *directAPI) MakeRequest( key := path[strings.LastIndex(path, "/")+1:] skill, ok := api.skills[key] if !ok { - return nil, fmt.Errorf("skill not found") + return nil, fmt.Errorf(`{"code":"not_found","message":"skill not found","statusCode":404}`) } if method == "PATCH" { if err := json.Unmarshal(body, &skill.Skill); err != nil { @@ -1123,6 +1126,153 @@ func readLock(t *testing.T, root string) syncmanifest.Manifest { return lock } +func TestPromptPlansToolAndSkillRowsByScope(t *testing.T) { + root, api := writeStandaloneAttachmentWorkspace(t) + tools := []string{"tool lookup update_manifest", "tool refund create_server", "tool search in_sync"} + skills := []string{"skill summarize in_sync", "skill triage update_manifest"} + variations := []string{"variation support/default in_sync"} + + for scope, want := range map[syncdomain.Scope][]string{ + syncdomain.ScopePrompts: variations, + syncdomain.ScopeTools: tools, + syncdomain.ScopeSkills: skills, + syncdomain.ScopeAll: slices.Concat(skills, tools, variations), + } { + t.Run(string(scope), func(t *testing.T) { + api.requests = nil + + stdout, err := runSyncAction(t, root, api, syncprompt.SyncAction{DryRun: true, Scope: scope}) + + require.NoError(t, err) + assert.Equal(t, want, planRows(t, stdout)) + requireOnlyReads(t, api.requests) + if scope == syncdomain.ScopeAll { + assert.Equal(t, 1, countRequests(api.requests, "GET", "/ai-tools/search"), + "the tool row and the variation share one read") + } + }) + } +} + +func TestPromptFiltersToolRowsBeforeAnyRead(t *testing.T) { + root, api := writeStandaloneAttachmentWorkspace(t) + filter := syncdomain.Filter{Project: "production", Keys: []string{"lookup"}} + + stdout, err := runSyncAction(t, root, api, syncprompt.SyncAction{DryRun: true, Scope: syncdomain.ScopeTools, Filter: filter}) + + require.NoError(t, err) + assert.Equal(t, []string{"tool lookup update_manifest"}, planRows(t, stdout)) + assert.Equal(t, []string{ + "GET https://example.test/api/v2/projects/production/configs/sync/manifests", + "GET https://example.test/api/v2/projects/production/ai-tools/lookup", + }, api.requests) + + for _, test := range []struct { + filter syncdomain.Filter + want string + }{ + { + syncdomain.Filter{Project: "production", Keys: []string{"serch"}}, + "tool production/serch is not synced in this repository. " + + "To start syncing it, run ldcli sync tools add --project production --key serch.", + }, + { + syncdomain.Filter{Project: "demo"}, + `project "demo" has no synced resources in this repository. To start, run ldcli sync tools add --project demo.`, + }, + } { + api.requests = nil + _, err := runSyncAction(t, root, api, syncprompt.SyncAction{DryRun: true, Scope: syncdomain.ScopeTools, Filter: test.filter}) + require.EqualError(t, err, test.want) + assert.Empty(t, api.requests) + } +} + +// writeStandaloneAttachmentWorkspace syncs a variation that uses the tool +// "search" and the skill "summarize". It also writes three tool and skill +// files that no variation uses: "lookup" and "triage", which LaunchDarkly +// has, and "refund", which LaunchDarkly does not have and which has upsert. +func writeStandaloneAttachmentWorkspace(t *testing.T) (string, *directAPI) { + t.Helper() + root := initRepository(t) + search := syncdomain.Tool{Key: "search", Schema: map[string]any{"type": "object"}} + summarize := syncdomain.Skill{Key: "summarize", Description: "Summarize a ticket", Markdown: "Write three bullet points.\n"} + used := variation("Baseline") + used.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + used.Skills = []syncdomain.AttachmentRef{{Key: "summarize"}} + used.Attachments = []syncdomain.Attachment{ + toolAttachment(search, 0), {Kind: syncdomain.AttachmentSkill, Skill: &summarize}, + } + writeVariation(t, root, used, true) + writeManifest(t, root, used) + + lookup := syncdomain.Tool{Key: "lookup", Schema: map[string]any{"type": "object"}} + triage := syncdomain.Skill{Key: "triage", Name: "Triage", Description: "Triage a ticket", Markdown: "Pick a queue.\n"} + writeLocalFile(t, root, "tools/lookup.json", `{"formatVersion": 1, "key": "lookup", "schema": {"type": "object"}}`) + writeLocalFile(t, root, "tools/refund.json", `{"formatVersion": 1, "upsert": true, "key": "refund", "schema": {"type": "object"}}`) + writeLocalFile(t, root, "skills/triage.md", "---\nkey: triage\ndescription: Triage a ticket\n---\n\nPick a queue.\n") + + server := used + server.Tools = []syncdomain.AttachmentRef{{Key: "search", Version: 1}} + server.Skills = []syncdomain.AttachmentRef{{Key: "summarize", Version: 1}} + server.Attachments = nil + summarizeInLaunchDarkly := summarize + summarizeInLaunchDarkly.Name = "Summarize" + return root, &directAPI{ + variation: &server, + tools: map[string]versionedTool{"search": {Tool: search, Version: 1}, "lookup": {Tool: lookup, Version: 2}}, + skills: map[string]versionedSkill{ + "summarize": {Skill: summarizeInLaunchDarkly, Version: 1}, "triage": {Skill: triage, Version: 3}, + }, + } +} + +func runSyncAction(t *testing.T, root string, api *directAPI, action syncprompt.SyncAction) (string, error) { + t.Helper() + api.manifest = manifestsByRoot[root] + var output bytes.Buffer + err := syncprompt.NewRunner(api).Run(syncprompt.Options{ + WorkingDirectory: root, AccessToken: "token", BaseURI: "https://example.test", OutputKind: "json", + Action: action, Input: strings.NewReader(""), Output: &output, ErrorOutput: io.Discard, + }) + return output.String(), err +} + +// planRows returns "kind lookup-key action" for each row of a JSON plan. +func planRows(t *testing.T, plan string) []string { + t.Helper() + var output struct { + Resources []struct { + ResourceKind string `json:"resourceKind"` + LookupKey string `json:"lookupKey"` + Action string `json:"action"` + } `json:"resources"` + } + require.NoError(t, json.Unmarshal([]byte(plan), &output)) + rows := make([]string, 0, len(output.Resources)) + for _, resource := range output.Resources { + rows = append(rows, resource.ResourceKind+" "+resource.LookupKey+" "+resource.Action) + } + return rows +} + +func countRequests(requests []string, method, pathSuffix string) int { + count := 0 + for _, request := range requests { + if strings.HasPrefix(request, method+" ") && strings.HasSuffix(request, pathSuffix) { + count++ + } + } + return count +} + +func writeLocalFile(t *testing.T, root, projectPath, content string) { + t.Helper() + path := filepath.Join(root, syncdomain.RootDir, "production", filepath.FromSlash(projectPath)) + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) +} + func TestPromptRestoresTrackedMissingFileInsteadOfArchiving(t *testing.T) { root := initRepository(t) baseline := variation("Baseline") diff --git a/internal/sync/prompt/action.go b/internal/sync/prompt/action.go index eafc6d21e..8b31dfbba 100644 --- a/internal/sync/prompt/action.go +++ b/internal/sync/prompt/action.go @@ -17,10 +17,12 @@ type CommandAction interface { } // SyncAction reconciles the workspace once, or each time a watched file -// changes. Filter limits the sync to the resources that it names. +// changes. Scope is the resource kinds that the sync plans, and Filter limits +// the sync to the resources that it names. type SyncAction struct { Watch bool DryRun bool + Scope syncdomain.Scope Filter syncdomain.Filter } @@ -64,7 +66,7 @@ func (action SyncAction) validate() error { if action.Watch && action.DryRun { return errors.New("watch does not support --dry-run") } - return action.Filter.Validate(syncdomain.KindVariation) + return action.Filter.Validate(action.Scope.KeyKind()) } func (action AddAction) validate() error { diff --git a/internal/sync/prompt/conflict.go b/internal/sync/prompt/conflict.go index aa2ff9503..59c7e6e1d 100644 --- a/internal/sync/prompt/conflict.go +++ b/internal/sync/prompt/conflict.go @@ -313,13 +313,13 @@ func applyConflictResolutions(plan Plan, resolutions map[ResourceID]ConflictReso func resolvedConflictAction(resource PlannedResource, resolution ConflictResolution) Action { switch resolution { case ConflictUseLaunchDarkly: - if resource.Server == nil { + if !resource.hasServer() { return ActionDeleteLocal } return ActionUpdateLocal case ConflictUseLocal: // A conflict always has a local file, because a missing file is restored. - if resource.Server == nil { + if !resource.hasServer() { return ActionCreateServer } return ActionUpdateServer diff --git a/internal/sync/prompt/conflict_test.go b/internal/sync/prompt/conflict_test.go index ce80552b7..cc6a7c7ba 100644 --- a/internal/sync/prompt/conflict_test.go +++ b/internal/sync/prompt/conflict_test.go @@ -268,6 +268,29 @@ func TestGroupConflictsConnectsMixedChangesAcrossSharedAttachments(t *testing.T) assert.Len(t, groups[0].resources, 4) } +func TestGroupConflictsJoinsAToolRowAndTheVariationsThatUseIt(t *testing.T) { + description := "Search the docs" + local := syncdomain.Attachment{Kind: syncdomain.AttachmentTool, Tool: &syncdomain.Tool{ + Key: "search", Description: &description, Schema: map[string]any{"type": "object"}, + }} + server := syncdomain.Attachment{Kind: syncdomain.AttachmentTool, Tool: &syncdomain.Tool{Key: "search", Schema: map[string]any{"type": "object"}}} + toolID := local.ID("project") + tool := planAttachment(toolID, &local, &server, "sha256:baseline", true) + require.Equal(t, ActionConflict, tool.Action) + variation := func(lookupKey string) PlannedResource { + return PlannedResource{ + ID: ResourceID{Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: lookupKey}, + Action: ActionUpdateServer, changedAttachments: []ResourceID{toolID}, + } + } + + groups := groupConflicts(Plan{Resources: []PlannedResource{tool, variation("config/first"), variation("config/second")}}) + + require.Len(t, groups, 1) + assert.Len(t, groups[0].resources, 3) + assert.Equal(t, toolID, groups[0].resources[0].ID) +} + func TestRunWorkspaceSyncAppliesConflictChoiceAfterRevalidation(t *testing.T) { root := t.TempDir() baseline, local, server := testVariation("baseline"), testVariation("local"), testVariation("server") diff --git a/internal/sync/prompt/diff.go b/internal/sync/prompt/diff.go index 4949a12bc..fa7820f57 100644 --- a/internal/sync/prompt/diff.go +++ b/internal/sync/prompt/diff.go @@ -23,16 +23,40 @@ func variationDiff(before, after *syncdomain.Variation) variationDiffFields { if before == nil && after == nil { return nil } + var beforeAttachments, afterAttachments []syncdomain.Attachment + if before != nil { + beforeAttachments = before.Attachments + } + if after != nil { + afterAttachments = after.Attachments + } + return diffParts(variationJSON(before), variationJSON(after), beforeAttachments, afterAttachments) +} + +// attachmentDiff returns the "tools" or the "skills" part of one tool or skill +// row, so that the review shows it like the attachment of a variation. +func attachmentDiff(before, after *syncdomain.Attachment) variationDiffFields { + var beforeAttachments, afterAttachments []syncdomain.Attachment + if before != nil { + beforeAttachments = []syncdomain.Attachment{*before} + } + if after != nil { + afterAttachments = []syncdomain.Attachment{*after} + } + return diffParts(nil, nil, beforeAttachments, afterAttachments) +} +// diffParts returns each part that differs, or nil. +func diffParts(beforeVariation, afterVariation json.RawMessage, beforeAttachments, afterAttachments []syncdomain.Attachment) variationDiffFields { fields := variationDiffFields{} addField := func(name string, before, after json.RawMessage) { if string(before) != string(after) { fields[name] = variationFieldDiff{Before: before, After: after} } } - addField("variation", variationJSON(before), variationJSON(after)) - beforeTools, beforeSkills := attachmentJSON(before) - afterTools, afterSkills := attachmentJSON(after) + addField("variation", beforeVariation, afterVariation) + beforeTools, beforeSkills := attachmentJSON(beforeAttachments) + afterTools, afterSkills := attachmentJSON(afterAttachments) addField("tools", beforeTools, afterTools) addField("skills", beforeSkills, afterSkills) @@ -55,13 +79,10 @@ func variationJSON(variation *syncdomain.Variation) json.RawMessage { } // attachmentJSON returns the canonical content of the tools and of the skills. -func attachmentJSON(variation *syncdomain.Variation) (tools, skills json.RawMessage) { - if variation == nil { - return nil, nil - } +func attachmentJSON(attachments []syncdomain.Attachment) (tools, skills json.RawMessage) { var toolContent []syncdomain.Tool var skillContent []syncdomain.Skill - for _, attachment := range variation.Attachments { + for _, attachment := range attachments { canonical := syncdomain.CanonicalAttachment(attachment) switch { case canonical.Tool != nil: @@ -106,8 +127,16 @@ func attachmentPinDiff(variation *syncdomain.Variation) (current, latest json.Ra } // changedAttachmentIDs returns the tools and skills whose content differs -// between the server and the local variation. +// between the server and the local variation. A tool or skill row returns its +// own identity when its two sides differ, so that a conflict choice for it +// also covers each variation that changes the same content. func changedAttachmentIDs(resource PlannedResource) []ResourceID { + if resource.ID.Kind != syncdomain.KindVariation { + if resource.LocalFingerprint != resource.ServerFingerprint { + return []ResourceID{resource.ID} + } + return nil + } before := attachmentsByID(resource.ID.ProjectKey, resource.Server) after := attachmentsByID(resource.ID.ProjectKey, resource.Local) diff --git a/internal/sync/prompt/output.go b/internal/sync/prompt/output.go index 9aa605a91..7fa43a8ef 100644 --- a/internal/sync/prompt/output.go +++ b/internal/sync/prompt/output.go @@ -321,11 +321,11 @@ func planReason(resource PlannedResource) string { case repinsOnly(resource): return repinReason(resource.Diff["attachment versions"]) case resource.Action == ActionUpdateServer: - return changedFieldsReason(resource.Diff, "changed") - case resource.Action == ActionUpdateLocal && resource.Local == nil: + return changedFieldsReason(resource, "changed") + case resource.Action == ActionUpdateLocal && !resource.hasLocal(): return "local file missing" case resource.Action == ActionUpdateLocal: - return changedFieldsReason(resource.Diff, "changed in LaunchDarkly") + return changedFieldsReason(resource, "changed in LaunchDarkly") case resource.Action == ActionDeleteLocal: return "deleted in LaunchDarkly" case resource.Action == ActionUpdateManifest: @@ -341,16 +341,22 @@ func planReason(resource PlannedResource) string { } } -// changedFieldsReason names the fields that a diff changes, for example -// "instructions, tools changed". -func changedFieldsReason(diff variationDiffFields, verb string) string { +// changedFieldsReason names the fields that the diff of a row changes, for +// example "instructions, tools changed". A tool or skill row names the fields +// of the tool or the skill, for example "description changed". +func changedFieldsReason(resource PlannedResource, verb string) string { var fields []string - if part, ok := diff["variation"]; ok { + if part, ok := resource.Diff["variation"]; ok { fields = append(fields, changedKeys(part.Before, part.After)...) } - for _, part := range []string{"tools", "skills"} { - if _, ok := diff[part]; ok { - fields = append(fields, part) + for _, name := range []string{"tools", "skills"} { + part, ok := resource.Diff[name] + switch { + case !ok: + case resource.ID.Kind == syncdomain.KindVariation: + fields = append(fields, name) + default: + fields = append(fields, changedKeys(onlyItem(part.Before), onlyItem(part.After))...) } } if len(fields) == 0 { @@ -375,6 +381,15 @@ func changedKeys(before, after json.RawMessage) []string { return keys } +// onlyItem returns the one item of a JSON list, or nil. +func onlyItem(list json.RawMessage) json.RawMessage { + var items []json.RawMessage + if json.Unmarshal(list, &items) != nil || len(items) != 1 { + return nil + } + return items[0] +} + // repinReason names each tool and skill that a repin moves, for example // "search to v4". func repinReason(versions variationFieldDiff) string { diff --git a/internal/sync/prompt/output_test.go b/internal/sync/prompt/output_test.go index 66dc1df42..f7fba07ce 100644 --- a/internal/sync/prompt/output_test.go +++ b/internal/sync/prompt/output_test.go @@ -258,9 +258,21 @@ func TestPlanReasonNamesTheChange(t *testing.T) { "variation does not exist in LaunchDarkly", }, "tools": { - PlannedResource{Action: ActionUpdateLocal, Local: &variation, Diff: variationDiffFields{"tools": {}}}, + PlannedResource{ID: testResourceID(), Action: ActionUpdateLocal, Local: &variation, Diff: variationDiffFields{"tools": {}}}, "tools changed in LaunchDarkly", }, + "tool fields": { + PlannedResource{ + ID: ResourceID{Kind: syncdomain.KindTool, ProjectKey: "production", LookupKey: "search"}, Action: ActionUpdateServer, + Diff: attachmentDiff( + &syncdomain.Attachment{Kind: syncdomain.AttachmentTool, Tool: &syncdomain.Tool{Key: "search", Schema: map[string]any{}}}, + &syncdomain.Attachment{Kind: syncdomain.AttachmentTool, Tool: &syncdomain.Tool{ + Key: "search", Description: pointerTo("Search"), Schema: map[string]any{"type": "object"}, + }}, + ), + }, + "description, schema changed", + }, "repin": { PlannedResource{ Action: ActionUpdateServer, ServerHasStaleAttachmentPins: true, @@ -496,3 +508,7 @@ func reviewTestPlan() Plan { {ID: variationID("support/fourth"), Action: ActionInSync}, }} } + +func pointerTo[T any](value T) *T { + return &value +} diff --git a/internal/sync/prompt/plan.go b/internal/sync/prompt/plan.go index 9e60f200b..1fc96d133 100644 --- a/internal/sync/prompt/plan.go +++ b/internal/sync/prompt/plan.go @@ -3,6 +3,7 @@ package prompt import ( "errors" "fmt" + "maps" "slices" syncdomain "github.com/launchdarkly/ldcli/internal/sync" @@ -46,7 +47,8 @@ type ServerResource struct { } // PlannedResource is the local and the server state of one resource, and the -// action that reconciles them. +// action that reconciles them. A variation row has Local and Server. A tool or +// skill row has LocalAttachment and ServerAttachment. A nil side does not exist. type PlannedResource struct { ID ResourceID Action Action @@ -57,6 +59,8 @@ type PlannedResource struct { ServerMode syncdomain.VariationMode Local *syncdomain.Variation Server *syncdomain.Variation + LocalAttachment *syncdomain.Attachment + ServerAttachment *syncdomain.Attachment // SyncedElsewhere is true when another working copy synced the resource // after this working copy wrote its sync.lock file. The plan is still // correct, because it compares against this working copy's lock. @@ -73,6 +77,16 @@ type PlannedResource struct { restoreRef *syncdomain.Reference } +// hasLocal reports whether the resource has a local file. +func (resource PlannedResource) hasLocal() bool { + return resource.Local != nil || resource.LocalAttachment != nil +} + +// hasServer reports whether LaunchDarkly has the resource. +func (resource PlannedResource) hasServer() bool { + return resource.Server != nil || resource.ServerAttachment != nil +} + // Plan is the sync decision for each resource, in identity order. type Plan struct { Resources []PlannedResource @@ -155,6 +169,10 @@ func planResource( if resource.Action == ActionError { resource.Error = "variation does not exist in LaunchDarkly; set upsert: true to create it" } + if resource.Action == ActionCreateServer { + _, variationKey, _ := id.VariationKeys() + rejectInvalidCreate(&resource, syncdomain.ValidateCreate(id.Kind, variationKey, local.Variation.Name, "")) + } // The fingerprints do not include versions. If LaunchDarkly pins an older // attachment version, sync updates the pin even when the content matches. currentPins, latestPins, stalePins := attachmentPinDiff(resource.Server) @@ -178,11 +196,122 @@ func planResource( return resource } +// planAttachments plans one row for each tool and skill that has a local file +// or an entry in the baseline. server must have an entry for each of them, +// which is nil when LaunchDarkly does not have the attachment. +func planAttachments( + baseline syncmanifest.Manifest, + local []syncdomain.SyncedAttachment, + server map[ResourceID]*syncdomain.Attachment, +) []PlannedResource { + localByID := make(map[ResourceID]*syncdomain.Attachment, len(local)) + for index := range local { + localByID[local[index].ID()] = &local[index].Attachment + } + baselineByID := make(map[ResourceID]string, len(baseline.Resources)) + for _, resource := range baseline.Resources { + if resource.ResourceKind != syncdomain.KindVariation { + baselineByID[resource.ID()] = resource.Fingerprint + } + } + + ids := slices.Collect(maps.Keys(localByID)) + for id := range baselineByID { + if _, isLocal := localByID[id]; !isLocal { + ids = append(ids, id) + } + } + slices.SortFunc(ids, syncdomain.CompareResourceIDs) + + resources := make([]PlannedResource, 0, len(ids)) + for _, id := range ids { + baselineFingerprint, tracked := baselineByID[id] + resources = append(resources, planAttachment(id, localByID[id], server[id], baselineFingerprint, tracked)) + } + return resources +} + +// planAttachment fingerprints both sides of one tool or skill and chooses its +// action with the same rules as a variation. +func planAttachment( + id ResourceID, + local, server *syncdomain.Attachment, + baselineFingerprint string, + tracked bool, +) PlannedResource { + resource := PlannedResource{ + ID: id, BaselineFingerprint: baselineFingerprint, LocalAttachment: local, ServerAttachment: server, + } + fail := func(message string) PlannedResource { + resource.Action, resource.Error = ActionError, message + return resource + } + + var err error + if local != nil { + resource.Upsert = local.Upsert + if resource.LocalFingerprint, err = syncdomain.FingerprintAttachment(id.ProjectKey, *local); err != nil { + return fail(err.Error()) + } + } + if server != nil { + if resource.ServerFingerprint, err = syncdomain.FingerprintAttachment(id.ProjectKey, *server); err != nil { + return fail(fmt.Sprintf("invalid server %s: %s", id.Kind, err)) + } + } + + resource.Action = chooseAction(tracked, resource) + switch resource.Action { + case ActionError: + resource.Error = missingAttachmentError(id) + case ActionCreateServer: + rejectInvalidCreate(&resource, validateAttachmentCreate(*local)) + } + if resource.LocalFingerprint != resource.ServerFingerprint { + resource.Diff = attachmentDiff(server, local) + } + resource.changedAttachments = changedAttachmentIDs(resource) + return resource +} + +// missingAttachmentError explains how to create a tool or skill that exists +// only in a local file, or how to stop syncing it. +func missingAttachmentError(id ResourceID) string { + consent := `add "upsert": true to its file` + if id.Kind == syncdomain.KindSkill { + consent = "add upsert: true to its front matter" + } + return fmt.Sprintf("%s %q does not exist in LaunchDarkly. To create it during sync, %s. "+ + "To stop syncing it, run ldcli sync %ss detach --project %s --key %s.", + id.Kind, id.LookupKey, consent, id.Kind, id.ProjectKey, id.LookupKey) +} + +// validateAttachmentCreate checks a new tool or skill against the create rules +// of LaunchDarkly. +func validateAttachmentCreate(attachment syncdomain.Attachment) error { + var description string + switch { + case attachment.Tool != nil && attachment.Tool.Description != nil: + description = *attachment.Tool.Description + case attachment.Skill != nil: + description = attachment.Skill.Description + } + return syncdomain.ValidateCreate(syncdomain.Kind(attachment.Kind), attachment.Key(), "", description) +} + +// rejectInvalidCreate turns a create that LaunchDarkly would reject into an +// error, so that the apply does not stop halfway. +func rejectInvalidCreate(resource *PlannedResource, err error) { + if err != nil { + resource.Action, resource.Error = ActionError, err.Error() + } +} + // chooseAction compares each fingerprint with the baseline to find which side -// changed. It returns ActionError for a new local variation without upsert. +// changed. It returns ActionError for a new local resource without upsert. func chooseAction(tracked bool, resource PlannedResource) Action { - localExists := resource.Local != nil - serverExists := resource.Server != nil + localExists := resource.hasLocal() + serverExists := resource.hasServer() // Without a baseline, sync cannot tell which side changed. It adopts equal // state, creates a new local variation that has upsert, and reports a diff --git a/internal/sync/prompt/plan_test.go b/internal/sync/prompt/plan_test.go index 42a25f736..93f7bf2ca 100644 --- a/internal/sync/prompt/plan_test.go +++ b/internal/sync/prompt/plan_test.go @@ -1,8 +1,11 @@ package prompt import ( + "slices" + "strings" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" syncdomain "github.com/launchdarkly/ldcli/internal/sync" @@ -194,3 +197,220 @@ func localResources(variation *syncdomain.Variation, upsert bool) []syncdomain.S Upsert: upsert, }} } + +func TestPlanAttachmentThreeWayRules(t *testing.T) { + for _, kind := range []syncdomain.AttachmentKind{syncdomain.AttachmentTool, syncdomain.AttachmentSkill} { + baseline, localChange, serverChange := testAttachment(kind, "baseline"), testAttachment(kind, "local"), testAttachment(kind, "server") + withUpsert := localChange + withUpsert.Upsert = true + id := baseline.ID("production") + + tests := []struct { + name string + tracked bool + local *syncdomain.Attachment + server *syncdomain.Attachment + expected Action + }{ + {"untracked equal content is adopted", false, &baseline, &baseline, ActionUpdateManifest}, + {"untracked different content conflicts", false, &localChange, &serverChange, ActionConflict}, + {"untracked upsert creates", false, &withUpsert, nil, ActionCreateServer}, + {"untracked without upsert is an error", false, &localChange, nil, ActionError}, + {"unchanged", true, &baseline, &baseline, ActionInSync}, + {"local edit", true, &localChange, &baseline, ActionUpdateServer}, + {"server edit", true, &baseline, &serverChange, ActionUpdateLocal}, + {"same edit", true, &localChange, &localChange, ActionUpdateManifest}, + {"divergent edits", true, &localChange, &serverChange, ActionConflict}, + {"missing local file is restored", true, nil, &baseline, ActionUpdateLocal}, + {"server deletion", true, &baseline, nil, ActionDeleteLocal}, + {"local edit after server deletion", true, &localChange, nil, ActionConflict}, + {"both deleted", true, nil, nil, ActionRemoveManifest}, + } + + for _, test := range tests { + t.Run(string(kind)+" "+test.name, func(t *testing.T) { + baselineFingerprint := "" + if test.tracked { + baselineFingerprint = attachmentFingerprint(t, baseline) + } + + resource := planAttachment(id, test.local, test.server, baselineFingerprint, test.tracked) + + require.Equal(t, test.expected, resource.Action) + if resource.LocalFingerprint != resource.ServerFingerprint { + require.Equal(t, []ResourceID{id}, resource.changedAttachments) + require.Contains(t, resource.Diff, string(kind)+"s") + } + }) + } + } +} + +func TestPlanAttachmentExplainsHowToCreateOrStopSyncing(t *testing.T) { + tool, skill := testAttachment(syncdomain.AttachmentTool, "local"), testAttachment(syncdomain.AttachmentSkill, "local") + + assert.Equal(t, `tool "search" does not exist in LaunchDarkly. To create it during sync, add "upsert": true to its file. `+ + "To stop syncing it, run ldcli sync tools detach --project production --key search.", + planAttachment(tool.ID("production"), &tool, nil, "", false).Error) + assert.Equal(t, `skill "summarize" does not exist in LaunchDarkly. To create it during sync, add upsert: true to its front matter. `+ + "To stop syncing it, run ldcli sync skills detach --project production --key summarize.", + planAttachment(skill.ID("production"), &skill, nil, "", false).Error) +} + +func TestPlanRejectsACreateThatLaunchDarklyWouldReject(t *testing.T) { + skill := testAttachment(syncdomain.AttachmentSkill, "local") + skill.Skill.Key, skill.Upsert = "Summarize_Ticket", true + resource := planAttachment(skill.ID("production"), &skill, nil, "", false) + assert.Equal(t, ActionError, resource.Action) + assert.Equal(t, `skill key "Summarize_Ticket" must use only lowercase letters, digits, and hyphens, and start with a letter or digit`, resource.Error) + + variation := testVariation("local") + variation.Key = "default variation" + plan := BuildPlan(syncmanifest.New(), []syncdomain.SyncedResource{{ + Kind: syncdomain.KindVariation, ProjectKey: "production", LookupKey: "support/default variation", Upsert: true, Variation: variation, + }}, map[ResourceID]ServerResource{ + syncdomain.VariationID("production", "support", "default variation"): {ConfigMode: syncdomain.VariationModeAgent}, + }) + assert.Equal(t, ActionError, plan.Resources[0].Action) + assert.Contains(t, plan.Resources[0].Error, `variation key "default variation" must use only letters`) +} + +func TestValidateCreateMirrorsLaunchDarklyRules(t *testing.T) { + long := strings.Repeat("a", 65) + tests := []struct { + kind syncdomain.Kind + key, description string + wantErrorContains string + }{ + {kind: syncdomain.KindSkill, key: "incident-summary"}, + {kind: syncdomain.KindSkill, key: "-summary", wantErrorContains: "start with a letter or digit"}, + {kind: syncdomain.KindSkill, key: long, wantErrorContains: "at most 64 characters"}, + {kind: syncdomain.KindSkill, key: "com1", wantErrorContains: "reserved file name on Windows"}, + {kind: syncdomain.KindTool, key: "lookup_order.v2"}, + {kind: syncdomain.KindTool, key: "lookup order", wantErrorContains: "must use only letters"}, + {kind: syncdomain.KindTool, key: strings.Repeat("a", 257), wantErrorContains: "at most 256 characters"}, + {kind: syncdomain.KindTool, key: "lookup", description: strings.Repeat("d", 1025), wantErrorContains: "description must be at most 1024"}, + } + + for _, test := range tests { + err := syncdomain.ValidateCreate(test.kind, test.key, "", test.description) + if test.wantErrorContains == "" { + assert.NoError(t, err, test.key) + } else { + assert.ErrorContains(t, err, test.wantErrorContains, test.key) + } + } +} + +// Sync applies a plan in identity order. Skills and tools must come before the +// variations that pin them. +func TestPlanOrderAppliesSkillsAndToolsBeforeVariations(t *testing.T) { + variation := testResourceID() + tool := ResourceID{Kind: syncdomain.KindTool, ProjectKey: "production", LookupKey: "search"} + skill := ResourceID{Kind: syncdomain.KindSkill, ProjectKey: "production", LookupKey: "summarize"} + ids := []ResourceID{variation, tool, skill} + + slices.SortFunc(ids, syncdomain.CompareResourceIDs) + + assert.Equal(t, []ResourceID{skill, tool, variation}, ids) +} + +// A sync must not publish a tool edit through one variation and write the old +// content back to the same file through another variation. +func TestCheckAttachmentBaselinesStopsAVariationFromOverwritingAnUnsyncedEdit(t *testing.T) { + old, edited := testAttachment(syncdomain.AttachmentTool, "old"), testAttachment(syncdomain.AttachmentTool, "edited") + withTool := func(name string, tool syncdomain.Attachment) *syncdomain.Variation { + variation := testVariation(name) + variation.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + variation.Attachments = []syncdomain.Attachment{tool} + return &variation + } + pushEdit := PlannedResource{ID: testResourceID(), Action: ActionUpdateServer, Local: withTool("first", edited), Server: withTool("first", old)} + pullOld := PlannedResource{ID: testResourceID(), Action: ActionUpdateLocal, Local: withTool("second", edited), Server: withTool("second", old)} + pullOld.ID.LookupKey = "support/second" + withoutTool := testVariation("third") + createTool := PlannedResource{ID: testResourceID(), Action: ActionUpdateServer, Local: withTool("third", edited), Server: &withoutTool} + createTool.ID.LookupKey = "support/third" + resources := []PlannedResource{pushEdit, pullOld, createTool} + for index := range resources { + resources[index].changedAttachments = changedAttachmentIDs(resources[index]) + } + lock := syncmanifest.New() + lock.SetFingerprint(old.ID("production"), attachmentFingerprint(t, old)) + + checkAttachmentBaselines(resources, lock) + + assert.Equal(t, ActionUpdateServer, resources[0].Action, "LaunchDarkly still has the recorded tool") + assert.Equal(t, ActionConflict, resources[1].Action, "the pull would overwrite the local edit") + assert.Equal(t, ActionUpdateServer, resources[2].Action, "LaunchDarkly has no copy of the tool on this variation") + groups := groupConflicts(Plan{Resources: resources}) + require.Len(t, groups, 1) + assert.Len(t, groups[0].resources, 3, "one choice covers every variation that writes the tool") + + untracked := []PlannedResource{pushEdit} + untracked[0].changedAttachments = changedAttachmentIDs(pushEdit) + checkAttachmentBaselines(untracked, syncmanifest.New()) + assert.Equal(t, ActionConflict, untracked[0].Action, "without a baseline, two versions of the tool conflict") +} + +func TestCheckAttachmentDeletesKeepsAToolThatAVariationStillUses(t *testing.T) { + tool := testAttachment(syncdomain.AttachmentTool, "local") + user := testVariation("user") + user.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + user.Attachments = []syncdomain.Attachment{tool} + variations := []syncdomain.SyncedResource{{ + Kind: syncdomain.KindVariation, ProjectKey: "production", LookupKey: "support/default", Variation: user, + }} + deletion := PlannedResource{ID: tool.ID("production"), Action: ActionDeleteLocal} + withoutTool := testVariation("user") + + tests := map[string]struct { + variationRow *PlannedResource + wantAction Action + wantError string + }{ + "the variation is not in the plan": { + wantAction: ActionError, + wantError: "tool production/search was deleted in LaunchDarkly, but variation production/support/default still uses it. " + + "Run ldcli sync all to pull the variation first.", + }, + "the variation pulls a version without the tool": { + variationRow: &PlannedResource{ID: testResourceID(), Action: ActionUpdateLocal, Server: &withoutTool}, + wantAction: ActionDeleteLocal, + }, + "the variation pushes": { + variationRow: &PlannedResource{ID: testResourceID(), Action: ActionUpdateServer, Local: &user}, + wantAction: ActionError, + wantError: "tool production/search was deleted in LaunchDarkly, but variation production/support/default still uses it. " + + "Remove it from the variation first.", + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + resources := []PlannedResource{deletion} + if test.variationRow != nil { + resources = append(resources, *test.variationRow) + } + + checkAttachmentDeletes(resources, variations) + + assert.Equal(t, test.wantAction, resources[0].Action) + assert.Equal(t, test.wantError, resources[0].Error) + }) + } +} + +func testAttachment(kind syncdomain.AttachmentKind, description string) syncdomain.Attachment { + if kind == syncdomain.AttachmentSkill { + return syncdomain.Attachment{Kind: kind, Skill: &syncdomain.Skill{Key: "summarize", Description: description, Markdown: "# Summarize\n"}} + } + return syncdomain.Attachment{Kind: kind, Tool: &syncdomain.Tool{Key: "search", Description: &description, Schema: map[string]any{"type": "object"}}} +} + +func attachmentFingerprint(t *testing.T, attachment syncdomain.Attachment) string { + t.Helper() + fingerprint, err := syncdomain.FingerprintAttachment("production", attachment) + require.NoError(t, err) + return fingerprint +} diff --git a/internal/sync/prompt/state.go b/internal/sync/prompt/state.go index f86f61e89..d4bf41681 100644 --- a/internal/sync/prompt/state.go +++ b/internal/sync/prompt/state.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "slices" + "strings" syncdomain "github.com/launchdarkly/ldcli/internal/sync" syncapi "github.com/launchdarkly/ldcli/internal/sync/api" @@ -23,13 +24,17 @@ type workspaceState struct { } // loadState reads the baseline, the local files, and LaunchDarkly, and builds -// the plan of the resources that the filter keeps. -func (workspace syncWorkspace) loadState(client syncapi.Client, filter syncdomain.Filter) (workspaceState, error) { +// the plan of the resources in the scope that the filter keeps. +func (workspace syncWorkspace) loadState( + client syncapi.Client, + scope syncdomain.Scope, + filter syncdomain.Filter, +) (workspaceState, error) { synced, err := workspace.syncedResources() if err != nil { return workspaceState{}, err } - if err := checkFilter(filter, synced); err != nil { + if err := checkFilter(scope, filter, synced); err != nil { return workspaceState{}, err } projectKeys := projectKeys(synced, filter) @@ -39,28 +44,69 @@ func (workspace syncWorkspace) loadState(client syncapi.Client, filter syncdomai if err != nil { return workspaceState{}, err } - plan, localFiles, err := loadWorkspacePlan(workspace.root, baseline, client, filter) + plan, localFiles, err := loadWorkspacePlan(workspace.root, baseline, client, scope, filter) if err != nil { return workspaceState{}, err } return workspaceState{projectKeys: projectKeys, baseline: baseline, plan: plan, localFiles: localFiles}, nil } -// loadWorkspacePlan reads the local variations and their LaunchDarkly -// versions, and compares both with the lock. It also returns the local -// variations as their files store them. It drops each variation that the -// filter does not keep before it reads LaunchDarkly. +// loadWorkspacePlan plans the variations, tools, and skills of the scope. It +// drops each resource that the filter does not keep before it reads +// LaunchDarkly. It also returns the local variations as their files store them. func loadWorkspacePlan( repositoryRoot string, baseline syncmanifest.Baseline, client syncapi.Client, + scope syncdomain.Scope, filter syncdomain.Filter, ) (Plan, map[ResourceID]syncdomain.SyncedResource, error) { - localFiles, err := compileWorkspace(repositoryRoot) + variations, err := compileWorkspace(repositoryRoot) if err != nil { return Plan{}, nil, err } - localFiles = slices.DeleteFunc(localFiles, func(resource syncdomain.SyncedResource) bool { + // A tool that a variation uses can also have a row of its own. The cache + // reads it from LaunchDarkly once. + attachments := newAttachmentCache(client) + + var plan Plan + var localFiles map[ResourceID]syncdomain.SyncedResource + if scope.Includes(syncdomain.KindVariation) { + if plan, localFiles, err = planVariationRows(repositoryRoot, baseline, client, attachments, filter, variations); err != nil { + return Plan{}, nil, err + } + } + if scope.Includes(syncdomain.KindTool) || scope.Includes(syncdomain.KindSkill) { + include := func(id ResourceID) bool { return scope.Includes(id.Kind) && filter.Includes(id) } + rows, err := planAttachmentRows(repositoryRoot, baseline.Lock, attachments, include) + if err != nil { + 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) + }) + } + + checkAttachmentBaselines(plan.Resources, baseline.Lock) + checkAttachmentDeletes(plan.Resources, variations) + for index := range plan.Resources { + plan.Resources[index].SyncedElsewhere = baseline.Stale(plan.Resources[index].ID) + } + return plan, localFiles, nil +} + +// planVariationRows reads the variations that the filter keeps from +// LaunchDarkly, and compares both sides with the lock. +func planVariationRows( + repositoryRoot string, + baseline syncmanifest.Baseline, + client syncapi.Client, + attachments *attachmentCache, + filter syncdomain.Filter, + variations []syncdomain.SyncedResource, +) (Plan, map[ResourceID]syncdomain.SyncedResource, error) { + localFiles := slices.DeleteFunc(slices.Clone(variations), func(resource syncdomain.SyncedResource) bool { return !filter.Includes(resource.ID()) }) local, err := canonicalizeLocalVariationModels(localFiles, client.ModelConfig) @@ -84,7 +130,6 @@ func loadWorkspacePlan( ids[resource.ID()] = struct{}{} } - attachments := newAttachmentCache(client) server := make(map[ResourceID]ServerResource, len(ids)) for id := range ids { resource, err := readServerResource(client, attachments, id) @@ -95,12 +140,143 @@ func loadWorkspacePlan( } plan := BuildPlan(tracked, local, server) for index := range plan.Resources { - plan.Resources[index].SyncedElsewhere = baseline.Stale(plan.Resources[index].ID) planRestore(repositoryRoot, baseline, &plan.Resources[index]) } return plan, localFilesByID, nil } +// planAttachmentRows reads each tool and skill that include accepts and that +// has a local file or a lock entry, and compares both sides with the lock. +func planAttachmentRows( + repositoryRoot string, + lock syncmanifest.Manifest, + attachments *attachmentCache, + include func(ResourceID) bool, +) ([]PlannedResource, error) { + local, err := synclocal.CompileAttachments(repositoryRoot, include) + if err != nil { + return nil, err + } + tracked := syncmanifest.New() + for _, resource := range lock.Resources { + if resource.ResourceKind != syncdomain.KindVariation && include(resource.ID()) { + tracked.Resources = append(tracked.Resources, resource) + } + } + + ids := make([]ResourceID, 0, len(local)+len(tracked.Resources)) + for _, attachment := range local { + ids = append(ids, attachment.ID()) + } + for _, resource := range tracked.Resources { + ids = append(ids, resource.ID()) + } + server := make(map[ResourceID]*syncdomain.Attachment, len(ids)) + for _, id := range ids { + if server[id], err = attachments.read(id); err != nil { + return nil, err + } + } + return planAttachments(tracked, local, server), nil +} + +// 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 +// side that it overwrites changed since that state, or when the attachment has +// no state and both sides have different content. +func checkAttachmentBaselines(resources []PlannedResource, lock syncmanifest.Manifest) { + baselines := make(map[ResourceID]string, len(lock.Resources)) + for _, resource := range lock.Resources { + baselines[resource.ID()] = resource.Fingerprint + } + + 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 { + continue + } + for _, id := range resource.changedAttachments { + kind := syncdomain.AttachmentKind(id.Kind) + overwritten, ok := target.Attachment(kind, id.LookupKey) + if !ok { + continue + } + // Without a baseline, sync cannot tell which side changed, so two + // different versions conflict. A new attachment on one side does not. + _, unsynced := source.Attachment(kind, id.LookupKey) + if baseline, tracked := baselines[id]; tracked { + fingerprint, err := syncdomain.FingerprintAttachment(id.ProjectKey, overwritten) + unsynced = err != nil || fingerprint != baseline + } + if unsynced { + resource.Action = ActionConflict + break + } + } + } +} + +// checkAttachmentDeletes keeps a tool or a skill that LaunchDarkly deleted +// while a local variation still uses it. The deletion becomes an error, unless +// the same plan pulls a version of that variation without the reference. +func checkAttachmentDeletes(resources []PlannedResource, variations []syncdomain.SyncedResource) { + variationRows := make(map[ResourceID]PlannedResource) + for _, resource := range resources { + if resource.ID.Kind == syncdomain.KindVariation { + variationRows[resource.ID] = resource + } + } + + for index := range resources { + resource := &resources[index] + if resource.ID.Kind == syncdomain.KindVariation || resource.Action != ActionDeleteLocal { + continue + } + for _, variation := range variations { + if !uses(variation, resource.ID) { + continue + } + row, planned := variationRows[variation.ID()] + if planned && row.Action == ActionUpdateLocal && !references(row.Server, resource.ID) { + continue + } + fix := "Run ldcli sync all to pull the variation first." + if planned { + fix = "Remove it from the variation first." + } + resource.Action = ActionError + resource.Error = fmt.Sprintf("%s was deleted in LaunchDarkly, but %s still uses it. %s", + resource.ID.Label(), variation.ID().Label(), fix) + break + } + } +} + +// uses reports whether a local variation uses a tool or a skill. +func uses(variation syncdomain.SyncedResource, id ResourceID) bool { + _, ok := variation.Variation.Attachment(syncdomain.AttachmentKind(id.Kind), id.LookupKey) + return ok && variation.ProjectKey == id.ProjectKey +} + +// references reports whether a server variation references a tool or a skill. +func references(variation *syncdomain.Variation, id ResourceID) bool { + return variation != nil && slices.ContainsFunc(variation.Refs(syncdomain.AttachmentKind(id.Kind)), func(ref syncdomain.AttachmentRef) bool { + return ref.Key == id.LookupKey + }) +} + // planRestore checks a variation whose local file is missing. The restored // file must keep the link that sync.lock records. When sync cannot restore // the link safely, the resource becomes an error and sync writes nothing. @@ -200,21 +376,26 @@ func projectKeys(synced []ResourceID, filter syncdomain.Filter) []string { } // checkFilter returns an error when the filter names a project, a config, or -// a variation that has no local file and no sync.lock entry, so that a typo -// never gives an empty plan that looks like success. -func checkFilter(filter syncdomain.Filter, synced []ResourceID) error { +// a resource of the scope that has no local file and no sync.lock entry, so +// that a typo never gives an empty plan that looks like success. +func checkFilter(scope syncdomain.Scope, filter syncdomain.Filter, synced []ResourceID) error { if filter.IsZero() { return nil } + synced = slices.DeleteFunc(slices.Clone(synced), func(id ResourceID) bool { return !scope.Includes(id.Kind) }) if !slices.ContainsFunc(synced, func(id ResourceID) bool { return id.ProjectKey == filter.Project }) { - return fmt.Errorf("project %q has no synced resources in this repository. "+ - "To start, run ldcli sync prompts add --project %s.", filter.Project, filter.Project) - } - for _, key := range filter.Keys { - if id := syncdomain.VariationID(filter.Project, filter.ConfigKey, key); !slices.Contains(synced, id) { - return fmt.Errorf("%s is not synced in this repository. To start syncing it, "+ - "run ldcli sync prompts add --project %s --config-key %s --key %s.", - id.Label(), filter.Project, filter.ConfigKey, key) + var commands []string + for _, kind := range []syncdomain.Kind{syncdomain.KindVariation, syncdomain.KindTool, syncdomain.KindSkill} { + if scope.Includes(kind) { + commands = append(commands, fmt.Sprintf("ldcli sync %ss add --project %s", kindNoun(kind), filter.Project)) + } + } + return fmt.Errorf("project %q has no synced resources in this repository. To start, run %s.", + filter.Project, strings.Join(commands, " or ")) + } + for _, id := range filter.IDs(scope.KeyKind()) { + if !slices.Contains(synced, id) { + return fmt.Errorf("%s is not synced in this repository. To start syncing it, run %s.", id.Label(), addCommand(id)) } } if !slices.ContainsFunc(synced, filter.Includes) { @@ -225,6 +406,14 @@ func checkFilter(filter syncdomain.Filter, synced []ResourceID) error { return nil } +// addCommand is the command that starts syncing one resource. +func addCommand(id ResourceID) string { + if configKey, variationKey, err := id.VariationKeys(); err == nil { + return fmt.Sprintf("ldcli sync prompts add --project %s --config-key %s --key %s", id.ProjectKey, configKey, variationKey) + } + return fmt.Sprintf("ldcli sync %ss add --project %s --key %s", id.Kind, id.ProjectKey, id.LookupKey) +} + // compileWorkspace reads the local variations. A workspace without a managed // directory has no variations. func compileWorkspace(repositoryRoot string) ([]syncdomain.SyncedResource, error) { @@ -236,28 +425,54 @@ func compileWorkspace(repositoryRoot string) ([]syncdomain.SyncedResource, error } // attachmentCache reads each tool and skill from LaunchDarkly once while it -// builds a plan, because many variations can share one attachment. +// builds a plan, because many variations can share one attachment, and a tool +// or a skill can also have a row of its own. type attachmentCache struct { - client syncapi.Client - attachments map[syncdomain.ResourceID]syncdomain.Attachment + client syncapi.Client + results map[syncdomain.ResourceID]attachmentRead +} + +type attachmentRead struct { + attachment syncdomain.Attachment + err error } func newAttachmentCache(client syncapi.Client) *attachmentCache { - return &attachmentCache{client: client, attachments: make(map[syncdomain.ResourceID]syncdomain.Attachment)} + return &attachmentCache{client: client, results: make(map[syncdomain.ResourceID]attachmentRead)} +} + +// get reads the latest version of an attachment. It keeps each result and +// each 404. +func (cache *attachmentCache) get(id ResourceID) (syncdomain.Attachment, error) { + if result, ok := cache.results[id]; ok { + return result.attachment, result.err + } + attachment, err := cache.client.ReadAttachment(id.ProjectKey, syncdomain.AttachmentKind(id.Kind), id.LookupKey) + if err == nil || syncapi.IsNotFound(err) { + cache.results[id] = attachmentRead{attachment: attachment, err: err} + } + return attachment, err +} + +// read returns the latest version of an attachment, or nil when LaunchDarkly +// does not have it. +func (cache *attachmentCache) read(id ResourceID) (*syncdomain.Attachment, error) { + attachment, err := cache.get(id) + switch { + case syncapi.IsNotFound(err): + return nil, nil + case err != nil: + return nil, err + default: + return &attachment, nil + } } +// hydrate reads every attachment that a server variation references. A +// missing attachment is an error. func (cache *attachmentCache) hydrate(projectKey string, variation *syncdomain.Variation) error { return variation.HydrateAttachments(func(kind syncdomain.AttachmentKind, key string) (syncdomain.Attachment, error) { - id := syncdomain.ResourceID{Kind: syncdomain.Kind(kind), ProjectKey: projectKey, LookupKey: key} - if attachment, ok := cache.attachments[id]; ok { - return attachment, nil - } - attachment, err := cache.client.ReadAttachment(projectKey, kind, key) - if err != nil { - return syncdomain.Attachment{}, err - } - cache.attachments[id] = attachment - return attachment, nil + return cache.get(syncdomain.ResourceID{Kind: syncdomain.Kind(kind), ProjectKey: projectKey, LookupKey: key}) }) } diff --git a/internal/sync/prompt/sync.go b/internal/sync/prompt/sync.go index 8588ecd65..235151dae 100644 --- a/internal/sync/prompt/sync.go +++ b/internal/sync/prompt/sync.go @@ -31,7 +31,8 @@ func (runner Runner) runWorkspaceSync(options Options, workspace syncWorkspace, watched = &watchedSources{watcher: watcher, snapshot: snapshot, debounce: watchDebounce} } - reviewed, err := workspace.loadState(client, options.syncAction().Filter) + target := options.syncAction() + reviewed, err := workspace.loadState(client, target.Scope, target.Filter) if err != nil { return err } @@ -67,7 +68,7 @@ func (runner Runner) runWorkspaceSync(options Options, workspace syncWorkspace, } // Read both sides again, so that no action uses state from before the review. - current, err := workspace.loadState(client, options.syncAction().Filter) + current, err := workspace.loadState(client, target.Scope, target.Filter) if err != nil { return err } diff --git a/internal/sync/resource.go b/internal/sync/resource.go index bcd7a204e..0ec01adf0 100644 --- a/internal/sync/resource.go +++ b/internal/sync/resource.go @@ -6,6 +6,7 @@ package sync import ( "errors" "fmt" + "regexp" "strings" ) @@ -72,6 +73,10 @@ func (id ResourceID) VariationKeys() (configKey, variationKey string, err error) // CompareResourceIDs orders identities by kind, project, and lookup key. Plans, // manifests, and output use this order so that results are deterministic. +// +// Sync applies a plan in this order. The kind names sort skills and tools +// before the variations that pin them, so a variation pins the version that +// the same sync published. func CompareResourceIDs(left, right ResourceID) int { if result := strings.Compare(string(left.Kind), string(right.Kind)); result != 0 { return result @@ -97,6 +102,47 @@ func ValidateKey(key string) error { } } +// These limits copy the create validation of the LaunchDarkly API for +// variations, tools, and skills (AIConfigVariationPost, AIToolPost, and +// AgentSkillPost). LaunchDarkly stays the authority. The copy lets a plan +// report a bad key before the apply starts. Lengths count bytes, as +// LaunchDarkly does. +const ( + createKeyMaxLength = 256 + skillKeyMaxLength = 64 + createNameMaxLength = 256 + createDescriptionMaxLength = 1024 +) + +var ( + createKeyPattern = regexp.MustCompile(`^[a-zA-Z0-9][._\-a-zA-Z0-9]*$`) + skillKeyPattern = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*$`) + windowsReservedNames = regexp.MustCompile(`^(con|prn|aux|nul|com[0-9]|lpt[0-9])$`) +) + +// ValidateCreate checks the key, the name, and the description of a resource +// that sync creates in LaunchDarkly. +func ValidateCreate(kind Kind, key, name, description string) error { + switch { + case kind == KindSkill && !skillKeyPattern.MatchString(key): + return fmt.Errorf("skill key %q must use only lowercase letters, digits, and hyphens, and start with a letter or digit", key) + case kind == KindSkill && len(key) > skillKeyMaxLength: + return fmt.Errorf("skill key %q must be at most %d characters", key, skillKeyMaxLength) + case kind == KindSkill && windowsReservedNames.MatchString(key): + return fmt.Errorf("skill key %q is a reserved file name on Windows", key) + case kind != KindSkill && !createKeyPattern.MatchString(key): + return fmt.Errorf("%s key %q must use only letters, digits, \".\", \"_\", and \"-\", and start with a letter or digit", kind, key) + case len(key) > createKeyMaxLength: + return fmt.Errorf("%s key %q must be at most %d characters", kind, key, createKeyMaxLength) + case len(name) > createNameMaxLength: + return fmt.Errorf("%s name must be at most %d characters", kind, createNameMaxLength) + case len(description) > createDescriptionMaxLength: + return fmt.Errorf("%s description must be at most %d characters", kind, createDescriptionMaxLength) + default: + return nil + } +} + // Reference links a variation to a prompt file elsewhere in the repository. // Format names the adapter that reads and writes the file. type Reference struct { diff --git a/internal/sync/scope.go b/internal/sync/scope.go index 238a302ca..343a5b441 100644 --- a/internal/sync/scope.go +++ b/internal/sync/scope.go @@ -7,6 +7,46 @@ import ( "strings" ) +// Scope is the resource kinds that one sync command plans. +type Scope string + +const ( + ScopePrompts Scope = "prompts" + ScopeTools Scope = "tools" + ScopeSkills Scope = "skills" + ScopeAll Scope = "all" +) + +// Includes reports whether the scope plans resources of kind. The empty scope +// plans prompt variations. +func (scope Scope) Includes(kind Kind) bool { + switch scope { + case "", ScopePrompts: + return kind == KindVariation + case ScopeTools: + return kind == KindTool + case ScopeSkills: + return kind == KindSkill + case ScopeAll: + return true + default: + return false + } +} + +// KeyKind returns the kind of resource that --key names in the scope. A sync +// of all kinds takes no --key. +func (scope Scope) KeyKind() Kind { + switch scope { + case ScopeTools: + return KindTool + case ScopeSkills: + return KindSkill + default: + return KindVariation + } +} + // Filter is the resources that the --project, --config-key, and --key flags // name. A sync keeps only those resources. An add or a detach uses them // instead of asking. The zero Filter names every resource. diff --git a/internal/sync/scope_test.go b/internal/sync/scope_test.go index 50cd50a57..77732cec1 100644 --- a/internal/sync/scope_test.go +++ b/internal/sync/scope_test.go @@ -7,6 +7,29 @@ import ( "github.com/stretchr/testify/require" ) +func TestScopePlansItsKinds(t *testing.T) { + tests := map[Scope][]Kind{ + "": {KindVariation}, + ScopePrompts: {KindVariation}, + ScopeTools: {KindTool}, + ScopeSkills: {KindSkill}, + ScopeAll: {KindSkill, KindTool, KindVariation}, + } + + for scope, want := range tests { + var kinds []Kind + for _, kind := range []Kind{KindSkill, KindTool, KindVariation} { + if scope.Includes(kind) { + kinds = append(kinds, kind) + } + } + assert.Equal(t, want, kinds, scope) + } + assert.Equal(t, KindTool, ScopeTools.KeyKind()) + assert.Equal(t, KindSkill, ScopeSkills.KeyKind()) + assert.Equal(t, KindVariation, ScopePrompts.KeyKind()) +} + func TestFilterValidateNeedsParentsAndRejectsBadValues(t *testing.T) { tests := map[string]struct { filter Filter @@ -69,6 +92,18 @@ func TestFilterIncludesOnlyTheNamedResources(t *testing.T) { } } +func TestFilterNamesToolsByKey(t *testing.T) { + filter := Filter{Project: "demo", Keys: []string{"search"}} + search := ResourceID{Kind: KindTool, ProjectKey: "demo", LookupKey: "search"} + + require.NoError(t, filter.Validate(KindTool)) + assert.Equal(t, []ResourceID{search}, filter.IDs(KindTool)) + assert.True(t, filter.Includes(search)) + assert.False(t, filter.Includes(ResourceID{Kind: KindTool, ProjectKey: "demo", LookupKey: "lookup"})) + require.EqualError(t, Filter{Project: "demo", Keys: []string{"search", "search"}}.Validate(KindTool), + "tool demo/search was selected more than once") +} + func TestFilterIDsNamesEachKey(t *testing.T) { filter := Filter{Project: "demo", ConfigKey: "support", Keys: []string{"first", "second"}} From cfe199dd8547ed0b3642cf9caa9493a8d65bf86a Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Thu, 22 Oct 2026 16:01:33 +0000 Subject: [PATCH 2/4] fix: do not offer upsert for a missing skill before skills support it The error for a missing skill told the user to add upsert: true to the front matter. A skill file has no upsert key yet, so that edit made the file fail to parse. The error now gives only the detach step for a skill. A tool still gets the upsert step. --- internal/sync/prompt/plan.go | 16 ++++++++-------- internal/sync/prompt/plan_test.go | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/sync/prompt/plan.go b/internal/sync/prompt/plan.go index 1fc96d133..c75bf9b6d 100644 --- a/internal/sync/prompt/plan.go +++ b/internal/sync/prompt/plan.go @@ -274,16 +274,16 @@ func planAttachment( return resource } -// missingAttachmentError explains how to create a tool or skill that exists -// only in a local file, or how to stop syncing it. +// missingAttachmentError explains how to stop syncing a tool or skill that +// exists only in a local file. For a tool, it also explains how to create it. +// A skill file has no upsert key yet, so sync cannot create a skill. func missingAttachmentError(id ResourceID) string { - consent := `add "upsert": true to its file` - if id.Kind == syncdomain.KindSkill { - consent = "add upsert: true to its front matter" + create := "" + if id.Kind == syncdomain.KindTool { + create = `To create it during sync, add "upsert": true to its file. ` } - return fmt.Sprintf("%s %q does not exist in LaunchDarkly. To create it during sync, %s. "+ - "To stop syncing it, run ldcli sync %ss detach --project %s --key %s.", - id.Kind, id.LookupKey, consent, id.Kind, id.ProjectKey, id.LookupKey) + return fmt.Sprintf("%s %q does not exist in LaunchDarkly. %sTo stop syncing it, run ldcli sync %ss detach --project %s --key %s.", + id.Kind, id.LookupKey, create, id.Kind, id.ProjectKey, id.LookupKey) } // validateAttachmentCreate checks a new tool or skill against the create rules diff --git a/internal/sync/prompt/plan_test.go b/internal/sync/prompt/plan_test.go index 93f7bf2ca..38dcb74a5 100644 --- a/internal/sync/prompt/plan_test.go +++ b/internal/sync/prompt/plan_test.go @@ -252,7 +252,7 @@ func TestPlanAttachmentExplainsHowToCreateOrStopSyncing(t *testing.T) { assert.Equal(t, `tool "search" does not exist in LaunchDarkly. To create it during sync, add "upsert": true to its file. `+ "To stop syncing it, run ldcli sync tools detach --project production --key search.", planAttachment(tool.ID("production"), &tool, nil, "", false).Error) - assert.Equal(t, `skill "summarize" does not exist in LaunchDarkly. To create it during sync, add upsert: true to its front matter. `+ + assert.Equal(t, `skill "summarize" does not exist in LaunchDarkly. `+ "To stop syncing it, run ldcli sync skills detach --project production --key summarize.", planAttachment(skill.ID("production"), &skill, nil, "", false).Error) } From 3def00687e150d5bfa785827cd096f1384a8ae75 Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Thu, 22 Oct 2026 16:01:40 +0000 Subject: [PATCH 3/4] fix: let a tool or skill delete run when its variation also goes away A delete of a tool or a skill that a local variation still uses becomes an error. Before, only a pull of a version without the reference kept the delete. A delete of the variation in the same plan, or a conflict that LaunchDarkly wins, also made the delete an error, and that error stopped the whole plan. A variation delete now keeps the tool or skill delete. A conflict keeps it if the LaunchDarkly version has no reference. After the conflict choices, sync checks the deletes again. If the local version of the variation wins, the delete becomes an error. --- internal/sync/prompt/conflict.go | 1 + internal/sync/prompt/plan_test.go | 38 +++++++++++++++++++++++++++++++ internal/sync/prompt/state.go | 36 +++++++++++++++++++++++++++-- 3 files changed, 73 insertions(+), 2 deletions(-) diff --git a/internal/sync/prompt/conflict.go b/internal/sync/prompt/conflict.go index 59c7e6e1d..49acf75d9 100644 --- a/internal/sync/prompt/conflict.go +++ b/internal/sync/prompt/conflict.go @@ -306,6 +306,7 @@ func applyConflictResolutions(plan Plan, resolutions map[ResourceID]ConflictReso resource.Action = resolvedConflictAction(*resource, resolution) } } + checkResolvedAttachmentDeletes(resolved.Resources) return resolved } diff --git a/internal/sync/prompt/plan_test.go b/internal/sync/prompt/plan_test.go index 38dcb74a5..fd88ec9b0 100644 --- a/internal/sync/prompt/plan_test.go +++ b/internal/sync/prompt/plan_test.go @@ -384,6 +384,20 @@ func TestCheckAttachmentDeletesKeepsAToolThatAVariationStillUses(t *testing.T) { wantError: "tool production/search was deleted in LaunchDarkly, but variation production/support/default still uses it. " + "Remove it from the variation first.", }, + "the variation is deleted": { + variationRow: &PlannedResource{ID: testResourceID(), Action: ActionDeleteLocal, Local: &user}, + wantAction: ActionDeleteLocal, + }, + "the variation conflicts, and LaunchDarkly has no tool": { + variationRow: &PlannedResource{ID: testResourceID(), Action: ActionConflict, Local: &user, Server: &withoutTool}, + wantAction: ActionDeleteLocal, + }, + "the variation conflicts, and LaunchDarkly keeps the tool": { + variationRow: &PlannedResource{ID: testResourceID(), Action: ActionConflict, Local: &user, Server: &user}, + wantAction: ActionError, + wantError: "tool production/search was deleted in LaunchDarkly, but variation production/support/default still uses it. " + + "Remove it from the variation first.", + }, } for name, test := range tests { @@ -401,6 +415,30 @@ func TestCheckAttachmentDeletesKeepsAToolThatAVariationStillUses(t *testing.T) { } } +func TestApplyConflictResolutionsChecksToolDeletesAgain(t *testing.T) { + tool := testAttachment(syncdomain.AttachmentTool, "local") + user := testVariation("user") + user.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + user.Attachments = []syncdomain.Attachment{tool} + withoutTool := testVariation("user") + plan := Plan{Resources: []PlannedResource{ + {ID: tool.ID("production"), Action: ActionDeleteLocal}, + {ID: testResourceID(), Action: ActionConflict, Local: &user, Server: &withoutTool}, + }} + + pulled := applyConflictResolutions(plan, map[ResourceID]ConflictResolution{testResourceID(): ConflictUseLaunchDarkly}) + assert.Equal(t, ActionDeleteLocal, pulled.Resources[0].Action) + assert.Equal(t, ActionUpdateLocal, pulled.Resources[1].Action) + require.NoError(t, pulled.BlockingError()) + + pushed := applyConflictResolutions(plan, map[ResourceID]ConflictResolution{testResourceID(): ConflictUseLocal}) + assert.Equal(t, ActionError, pushed.Resources[0].Action) + assert.Equal(t, ActionUpdateServer, pushed.Resources[1].Action) + require.EqualError(t, pushed.BlockingError(), "cannot sync tool production/search: tool production/search was deleted in LaunchDarkly, "+ + "but variation production/support/default still uses it. Remove it from the variation first.") + assert.Equal(t, ActionDeleteLocal, plan.Resources[0].Action, "the reviewed plan does not change") +} + func testAttachment(kind syncdomain.AttachmentKind, description string) syncdomain.Attachment { if kind == syncdomain.AttachmentSkill { return syncdomain.Attachment{Kind: kind, Skill: &syncdomain.Skill{Key: "summarize", Description: description, Markdown: "# Summarize\n"}} diff --git a/internal/sync/prompt/state.go b/internal/sync/prompt/state.go index d4bf41681..4383abd0a 100644 --- a/internal/sync/prompt/state.go +++ b/internal/sync/prompt/state.go @@ -230,7 +230,8 @@ func checkAttachmentBaselines(resources []PlannedResource, lock syncmanifest.Man // checkAttachmentDeletes keeps a tool or a skill that LaunchDarkly deleted // while a local variation still uses it. The deletion becomes an error, unless -// the same plan pulls a version of that variation without the reference. +// the variation row stops using it. A conflict can stop using it if +// LaunchDarkly wins, so checkResolvedAttachmentDeletes checks the choice. func checkAttachmentDeletes(resources []PlannedResource, variations []syncdomain.SyncedResource) { variationRows := make(map[ResourceID]PlannedResource) for _, resource := range resources { @@ -249,7 +250,7 @@ func checkAttachmentDeletes(resources []PlannedResource, variations []syncdomain continue } row, planned := variationRows[variation.ID()] - if planned && row.Action == ActionUpdateLocal && !references(row.Server, resource.ID) { + if planned && dropsReference(row, resource.ID) { continue } fix := "Run ldcli sync all to pull the variation first." @@ -264,6 +265,37 @@ func checkAttachmentDeletes(resources []PlannedResource, variations []syncdomain } } +// checkResolvedAttachmentDeletes checks the deletions again after the conflict +// choices. A variation whose local version wins still uses its tools and +// skills. The plan has a row for each variation that a conflict choice +// changes, so the check needs no other variations. +func checkResolvedAttachmentDeletes(resources []PlannedResource) { + var variations []syncdomain.SyncedResource + for _, resource := range resources { + if resource.ID.Kind == syncdomain.KindVariation && resource.Local != nil { + variations = append(variations, syncdomain.SyncedResource{ + Kind: resource.ID.Kind, ProjectKey: resource.ID.ProjectKey, LookupKey: resource.ID.LookupKey, Variation: *resource.Local, + }) + } + } + checkAttachmentDeletes(resources, variations) +} + +// dropsReference reports whether a variation row stops using a tool or a +// skill. A deletion does, and so does a pull of a version without the +// reference. A conflict does if LaunchDarkly wins, and the conflict blocks +// the plan until the user chooses. +func dropsReference(row PlannedResource, id ResourceID) bool { + switch row.Action { + case ActionDeleteLocal: + return true + case ActionUpdateLocal, ActionConflict: + return !references(row.Server, id) + default: + return false + } +} + // uses reports whether a local variation uses a tool or a skill. func uses(variation syncdomain.SyncedResource, id ResourceID) bool { _, ok := variation.Variation.Attachment(syncdomain.AttachmentKind(id.Kind), id.LookupKey) From 125ca05a985107fa12218463911cb16cd4dd4a13 Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Thu, 22 Oct 2026 16:14:42 +0000 Subject: [PATCH 4/4] fix: keep a deleted tool or skill out of a conflict group A conflict choice applies to every row in the group of the conflict. A tool or skill delete joined the group of a variation conflict that uses it. A local choice then made the delete a create, and sync made the tool again in LaunchDarkly. A deleted tool or skill now stays out of conflict groups. After the choice, sync checks the delete again. A local choice makes the delete an error, and a LaunchDarkly choice keeps the delete. --- internal/sync/prompt/conflict.go | 12 +++++++++--- internal/sync/prompt/plan_test.go | 29 ++++++++++++++++++++++------- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/internal/sync/prompt/conflict.go b/internal/sync/prompt/conflict.go index 49acf75d9..83eede289 100644 --- a/internal/sync/prompt/conflict.go +++ b/internal/sync/prompt/conflict.go @@ -8,6 +8,7 @@ import ( "slices" "strings" + syncdomain "github.com/launchdarkly/ldcli/internal/sync" syncconsole "github.com/launchdarkly/ldcli/internal/sync/console" syncinteractive "github.com/launchdarkly/ldcli/internal/sync/interactive" ) @@ -183,7 +184,10 @@ func writeConflict(output io.Writer, group conflictGroup) error { // groupConflicts returns one group for each conflict. A group also has every // other variation that shares a changed attachment with it, directly or -// through another variation in the group. +// through another variation in the group. An error row does not join a group. +// A tool or skill that LaunchDarkly deleted does not join either, because a +// local choice would create it again. checkResolvedAttachmentDeletes decides +// that row after the choice. func groupConflicts(plan Plan) []conflictGroup { var groups []conflictGroup visited := make([]bool, len(plan.Resources)) @@ -195,9 +199,11 @@ func groupConflicts(plan Plan) []conflictGroup { members := []int{start} for next := 0; next < len(members); next++ { for candidate := range plan.Resources { + row := plan.Resources[candidate] if visited[candidate] || - plan.Resources[candidate].Action == ActionError || - !sharesChangedAttachment(plan.Resources[members[next]], plan.Resources[candidate]) { + row.Action == ActionError || + (row.ID.Kind != syncdomain.KindVariation && row.Action == ActionDeleteLocal) || + !sharesChangedAttachment(plan.Resources[members[next]], row) { continue } visited[candidate] = true diff --git a/internal/sync/prompt/plan_test.go b/internal/sync/prompt/plan_test.go index fd88ec9b0..109ec2bd4 100644 --- a/internal/sync/prompt/plan_test.go +++ b/internal/sync/prompt/plan_test.go @@ -415,24 +415,39 @@ func TestCheckAttachmentDeletesKeepsAToolThatAVariationStillUses(t *testing.T) { } } +// A deleted tool stays out of the conflict group of a variation that uses it, +// so that a local choice cannot create the tool again. The check after the +// choice decides the delete. func TestApplyConflictResolutionsChecksToolDeletesAgain(t *testing.T) { tool := testAttachment(syncdomain.AttachmentTool, "local") user := testVariation("user") user.Tools = []syncdomain.AttachmentRef{{Key: "search"}} user.Attachments = []syncdomain.Attachment{tool} withoutTool := testVariation("user") - plan := Plan{Resources: []PlannedResource{ - {ID: tool.ID("production"), Action: ActionDeleteLocal}, - {ID: testResourceID(), Action: ActionConflict, Local: &user, Server: &withoutTool}, - }} + deletion := planAttachment(tool.ID("production"), &tool, nil, attachmentFingerprint(t, tool), true) + require.Equal(t, ActionDeleteLocal, deletion.Action) + conflict := PlannedResource{ID: testResourceID(), Action: ActionConflict, Local: &user, Server: &withoutTool} + conflict.changedAttachments = changedAttachmentIDs(conflict) + plan := Plan{Resources: []PlannedResource{deletion, conflict}} + + groups := groupConflicts(plan) + require.Len(t, groups, 1) + require.Len(t, groups[0].resources, 1, "the deleted tool does not take the choice") + choose := func(resolution ConflictResolution) Plan { + resolutions := make(map[ResourceID]ConflictResolution) + for _, member := range groups[0].resources { + resolutions[member.ID] = resolution + } + return applyConflictResolutions(plan, resolutions) + } - pulled := applyConflictResolutions(plan, map[ResourceID]ConflictResolution{testResourceID(): ConflictUseLaunchDarkly}) + pulled := choose(ConflictUseLaunchDarkly) assert.Equal(t, ActionDeleteLocal, pulled.Resources[0].Action) assert.Equal(t, ActionUpdateLocal, pulled.Resources[1].Action) require.NoError(t, pulled.BlockingError()) - pushed := applyConflictResolutions(plan, map[ResourceID]ConflictResolution{testResourceID(): ConflictUseLocal}) - assert.Equal(t, ActionError, pushed.Resources[0].Action) + pushed := choose(ConflictUseLocal) + assert.Equal(t, ActionError, pushed.Resources[0].Action, "the local choice must not create the deleted tool") assert.Equal(t, ActionUpdateServer, pushed.Resources[1].Action) require.EqualError(t, pushed.BlockingError(), "cannot sync tool production/search: tool production/search was deleted in LaunchDarkly, "+ "but variation production/support/default still uses it. Remove it from the variation first.")