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..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 @@ -306,6 +312,7 @@ func applyConflictResolutions(plan Plan, resolutions map[ResourceID]ConflictReso resource.Action = resolvedConflictAction(*resource, resolution) } } + checkResolvedAttachmentDeletes(resolved.Resources) return resolved } @@ -313,13 +320,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..c75bf9b6d 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 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 { + 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. %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 +// 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..109ec2bd4 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,273 @@ 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 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.", + }, + "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 { + 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) + }) + } +} + +// 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") + 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 := choose(ConflictUseLaunchDarkly) + assert.Equal(t, ActionDeleteLocal, pulled.Resources[0].Action) + assert.Equal(t, ActionUpdateLocal, pulled.Resources[1].Action) + require.NoError(t, pulled.BlockingError()) + + 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.") + 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"}} + } + 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..4383abd0a 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,175 @@ 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 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 { + 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 && dropsReference(row, 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 + } + } +} + +// 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) + 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 +408,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 +438,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 +457,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"}}