diff --git a/internal/sync/api/attachment.go b/internal/sync/api/attachment.go index b75467a2c..89a50d543 100644 --- a/internal/sync/api/attachment.go +++ b/internal/sync/api/attachment.go @@ -29,11 +29,19 @@ type toolMutationRequest struct { Tags []string `json:"tags"` } +// skillMutationRequest updates a skill. It has no name, because LaunchDarkly +// owns the name of a skill after it creates the skill. type skillMutationRequest struct { Description string `json:"description"` Markdown string `json:"markdown"` } +type skillCreateRequest struct { + Key string `json:"key"` + Name string `json:"name"` + skillMutationRequest +} + // SearchAttachments returns one page of the latest tools or skills that match query. func (client Client) SearchAttachments( projectKey string, @@ -100,11 +108,8 @@ func (client Client) ReadAttachment(projectKey string, kind syncdomain.Attachmen return attachment, nil } -// CreateAttachment creates the first version of a tool. Sync cannot create a skill. +// CreateAttachment creates the first version of a tool or a skill. func (client Client) CreateAttachment(projectKey string, attachment syncdomain.Attachment) error { - if attachment.Kind != syncdomain.AttachmentTool { - return fmt.Errorf("creating %ss is not supported", attachment.Kind) - } return client.mutateAttachment(projectKey, attachment, true) } @@ -135,7 +140,11 @@ func (client Client) mutateAttachment(projectKey string, attachment syncdomain.A } request.body = body case attachment.Kind == syncdomain.AttachmentSkill && attachment.Skill != nil: - request.body = skillMutationRequest{Description: attachment.Skill.Description, Markdown: attachment.Skill.Markdown} + body := skillMutationRequest{Description: attachment.Skill.Description, Markdown: attachment.Skill.Markdown} + request.body = body + if create { + request.body = skillCreateRequest{Key: attachment.Key(), Name: attachment.Skill.CreateName(), skillMutationRequest: body} + } default: return fmt.Errorf("%s %q has no content", attachment.Kind, attachment.Key()) } diff --git a/internal/sync/api/attachment_test.go b/internal/sync/api/attachment_test.go index 91eea00fe..3f7387061 100644 --- a/internal/sync/api/attachment_test.go +++ b/internal/sync/api/attachment_test.go @@ -183,6 +183,36 @@ func TestCreateToolIncludesItsKey(t *testing.T) { assert.Equal(t, "search", body["key"]) } +func TestCreateSkillSendsKeyNameAndContent(t *testing.T) { + tests := map[string]struct { + name string + want string + }{ + "name from the skill file": {name: "Human handoff", want: "Human handoff"}, + "name from the key": {want: "Incident summary"}, + } + + for title, test := range tests { + t.Run(title, func(t *testing.T) { + transport := &recordingClient{Responses: [][]byte{[]byte(`{}`)}} + client := NewClient(transport, "token", "https://example.com") + skill := syncdomain.Skill{Key: "incident-summary", Name: test.name, Description: "Summarize", Markdown: "# Summary\n"} + + err := client.CreateAttachment("project", syncdomain.Attachment{Kind: syncdomain.AttachmentSkill, Skill: &skill}) + + require.NoError(t, err) + require.Len(t, transport.Requests, 1) + assert.Equal(t, http.MethodPost, transport.Requests[0].Method) + assert.Equal(t, "https://example.com/api/v2/projects/project/ai-configs/skills", transport.Requests[0].Path) + var body map[string]any + require.NoError(t, json.Unmarshal(transport.Requests[0].Body, &body)) + assert.Equal(t, map[string]any{ + "key": "incident-summary", "name": test.want, "description": "Summarize", "markdown": "# Summary\n", + }, body) + }) + } +} + func TestReadAttachmentRequiresMatchingVersionedIdentity(t *testing.T) { for name, response := range map[string]string{ "key": `{"key":"other","schema":{},"version":2}`, diff --git a/internal/sync/attachment.go b/internal/sync/attachment.go index 5cfdf1811..8062cd15b 100644 --- a/internal/sync/attachment.go +++ b/internal/sync/attachment.go @@ -38,7 +38,8 @@ type Tool struct { } // Skill is one Markdown file and its catalog metadata. Sync owns Key, -// Description, and Markdown. LaunchDarkly owns Name. +// Description, and Markdown. LaunchDarkly owns Name. A local skill file can +// set Name, and sync sends it only when it creates the skill. type Skill struct { Key string `json:"key"` Name string `json:"name"` @@ -46,6 +47,15 @@ type Skill struct { Markdown string `json:"markdown"` } +// CreateName returns the name of a new skill: the name in the skill file, or a +// name made from the key. +func (skill Skill) CreateName() string { + if skill.Name != "" { + return skill.Name + } + return DisplayName(skill.Key) +} + // Attachment is the content of one tool or one skill, with the latest version // that sync read from LaunchDarkly. Exactly one of Tool and Skill is set. // @@ -53,7 +63,8 @@ type Skill struct { type Attachment struct { Kind AttachmentKind Version int - // Upsert lets sync create the tool in LaunchDarkly when it is absent. + // Upsert lets sync create the tool or the skill in LaunchDarkly when it + // is absent. Upsert bool Tool *Tool Skill *Skill diff --git a/internal/sync/link/link.go b/internal/sync/link/link.go index 525d738ac..6692aa292 100644 --- a/internal/sync/link/link.go +++ b/internal/sync/link/link.go @@ -12,7 +12,6 @@ import ( "path/filepath" "slices" "strings" - "unicode" "github.com/charmbracelet/huh" @@ -151,7 +150,7 @@ func targetSelection(options Options, prompt linkedPrompt, target Target) (Selec return Selection{}, "", fmt.Errorf("model config %q was not found", target.ModelConfigKey) } - name := cmp.Or(prompt.parsed.Name, target.Name, displayName(variationKey)) + name := cmp.Or(prompt.parsed.Name, target.Name, syncdomain.DisplayName(variationKey)) return Selection{ Project: syncapi.Project{Key: projectKey}, Config: config, @@ -213,7 +212,7 @@ func promptForSelection(options Options, prompt linkedPrompt) (Selection, string fields = append(fields, huh.NewInput().Title("Variation key").Value(&key).Validate(validateDerivedKey)) } if name == "" { - name = displayName(key) + name = syncdomain.DisplayName(key) fields = append(fields, huh.NewInput().Title("Variation name").Value(&name).Validate(requiredValue("variation name"))) } if len(prompt.parsed.Messages) == 0 { @@ -366,12 +365,3 @@ func requiredValue(label string) func(string) error { return nil } } - -// displayName converts a key such as "support-agent" to "Support agent". -func displayName(key string) string { - runes := []rune(strings.NewReplacer("-", " ", "_", " ").Replace(key)) - if len(runes) != 0 { - runes[0] = unicode.ToUpper(runes[0]) - } - return string(runes) -} diff --git a/internal/sync/local/attachment.go b/internal/sync/local/attachment.go index 34952563a..2f44b79e6 100644 --- a/internal/sync/local/attachment.go +++ b/internal/sync/local/attachment.go @@ -19,7 +19,9 @@ import ( ) // A tool file is JSON. A skill file is Markdown with YAML front matter that -// has the key and the description. +// has the key and the description. Both can set upsert, and a skill file can +// set the name that sync sends when it creates the skill. Neither is part of +// the fingerprint. const toolFormatVersion = 1 type toolFile struct { @@ -31,6 +33,8 @@ type toolFile struct { type skillFrontMatter struct { Key string `yaml:"key"` Description *string `yaml:"description"` + Name string `yaml:"name,omitempty"` + Upsert bool `yaml:"upsert,omitempty"` } // OrphanedAttachment is a tool or skill file that no variation uses. @@ -121,17 +125,10 @@ func readSkill(fsys fs.FS, projectKey, key string) (syncdomain.Attachment, error return syncdomain.Attachment{}, err } - var metadata skillFrontMatter - body, err := parseYAMLFrontMatter(data, &metadata) + metadata, body, err := parseSkillFile(data) if err != nil { return syncdomain.Attachment{}, fmt.Errorf("parse skill %q: %w", key, err) } - // Rendering puts one blank line after the front matter. Remove it. - if bytes.HasPrefix(body, []byte("\r\n")) { - body = body[2:] - } else { - body = bytes.TrimPrefix(body, []byte("\n")) - } switch { case metadata.Key != key: @@ -141,8 +138,23 @@ func readSkill(fsys fs.FS, projectKey, key string) (syncdomain.Attachment, error case strings.TrimSpace(string(body)) == "": return syncdomain.Attachment{}, fmt.Errorf("skill %q markdown is required", key) } - skill := syncdomain.Skill{Key: key, Description: *metadata.Description, Markdown: string(body)} - return syncdomain.Attachment{Kind: syncdomain.AttachmentSkill, Skill: &skill}, nil + skill := syncdomain.Skill{Key: key, Name: metadata.Name, Description: *metadata.Description, Markdown: string(body)} + return syncdomain.Attachment{Kind: syncdomain.AttachmentSkill, Upsert: metadata.Upsert, Skill: &skill}, nil +} + +// parseSkillFile decodes the front matter of a skill file and returns the +// Markdown that follows it. +func parseSkillFile(data []byte) (skillFrontMatter, []byte, error) { + var metadata skillFrontMatter + body, err := parseYAMLFrontMatter(data, &metadata) + if err != nil { + return skillFrontMatter{}, nil, err + } + // Rendering puts one blank line after the front matter. Remove it. + if bytes.HasPrefix(body, []byte("\r\n")) { + return metadata, body[2:], nil + } + return metadata, bytes.TrimPrefix(body, []byte("\n")), nil } // readAttachmentFile reads one tool or skill file. It rejects a symbolic link @@ -210,7 +222,10 @@ func renderAttachment(attachment syncdomain.Attachment) ([]byte, error) { } return renderToolFile(toolFile{FormatVersion: toolFormatVersion, Upsert: attachment.Upsert, Tool: *attachment.Tool}) case attachment.Kind == syncdomain.AttachmentSkill && attachment.Skill != nil: - return renderSkill(*attachment.Skill) + // LaunchDarkly owns the name of a skill, so the file does not get it. + // A pull keeps the name of an existing file. + skill := *attachment.Skill + return renderSkillFile(skillFrontMatter{Key: skill.Key, Description: &skill.Description, Upsert: attachment.Upsert}, skill.Markdown) default: return nil, fmt.Errorf("%s %q has no content", attachment.Kind, attachment.Key()) } @@ -227,41 +242,55 @@ func renderToolFile(file toolFile) ([]byte, error) { return content.Bytes(), nil } -func renderSkill(skill syncdomain.Skill) ([]byte, error) { - if strings.TrimSpace(skill.Markdown) == "" { - return nil, fmt.Errorf("skill %q markdown is required", skill.Key) +func renderSkillFile(metadata skillFrontMatter, markdown string) ([]byte, error) { + if strings.TrimSpace(markdown) == "" { + return nil, fmt.Errorf("skill %q markdown is required", metadata.Key) } - metadata, err := marshalYAML(skillFrontMatter{Key: skill.Key, Description: &skill.Description}) + frontMatter, err := marshalYAML(metadata) if err != nil { - return nil, fmt.Errorf("marshal skill %q metadata: %w", skill.Key, err) + return nil, fmt.Errorf("marshal skill %q metadata: %w", metadata.Key, err) } var content bytes.Buffer content.WriteString("---\n") - content.Write(metadata) + content.Write(frontMatter) content.WriteString("---\n\n") - content.WriteString(skill.Markdown) + content.WriteString(markdown) return content.Bytes(), nil } -// preserveToolUpsert keeps the upsert flag of an existing tool file. The flag -// is a local choice, so new content from LaunchDarkly does not remove it. -func preserveToolUpsert(filePath string, original, replacement []byte) ([]byte, error) { - if !strings.HasSuffix(filePath, toolFileSuffix) { - return replacement, nil +// preserveLocalFields keeps the local choices of an existing tool or skill +// file: the upsert flag, and the name of a skill. New content from +// LaunchDarkly does not remove them. +func preserveLocalFields(filePath string, original, replacement []byte) ([]byte, error) { + if strings.HasSuffix(filePath, toolFileSuffix) { + var local, updated toolFile + if err := json.Unmarshal(original, &local); err != nil { + return nil, fmt.Errorf("parse existing tool %q: %w", filePath, err) + } + if !local.Upsert { + return replacement, nil + } + if err := json.Unmarshal(replacement, &updated); err != nil { + return nil, fmt.Errorf("parse replacement tool %q: %w", filePath, err) + } + updated.Upsert = true + return renderToolFile(updated) } - var local, updated toolFile - if err := json.Unmarshal(original, &local); err != nil { - return nil, fmt.Errorf("parse existing tool %q: %w", filePath, err) + + local, _, err := parseSkillFile(original) + if err != nil { + return nil, fmt.Errorf("parse existing skill %q: %w", filePath, err) } - if !local.Upsert { + if local.Name == "" && !local.Upsert { return replacement, nil } - if err := json.Unmarshal(replacement, &updated); err != nil { - return nil, fmt.Errorf("parse replacement tool %q: %w", filePath, err) + updated, markdown, err := parseSkillFile(replacement) + if err != nil { + return nil, fmt.Errorf("parse replacement skill %q: %w", filePath, err) } - updated.Upsert = true - return renderToolFile(updated) + updated.Name, updated.Upsert = local.Name, updated.Upsert || local.Upsert + return renderSkillFile(updated, string(markdown)) } // OrphanedAttachments returns each tool and skill file that no local diff --git a/internal/sync/local/attachment_test.go b/internal/sync/local/attachment_test.go index 7f7fc189d..822d4af9c 100644 --- a/internal/sync/local/attachment_test.go +++ b/internal/sync/local/attachment_test.go @@ -63,12 +63,33 @@ Help the customer. } func TestRenderSkillIncludesReadOnlyKeyAndEditableDescription(t *testing.T) { - rendered, err := renderSkill(syncdomain.Skill{ - Key: "support", Description: "Follow the standard support process.", Markdown: "Follow the support process.\n", - }) + skill := syncdomain.Skill{ + Key: "support", Name: "Support", Description: "Follow the standard support process.", Markdown: "Follow the support process.\n", + } + + rendered, err := renderAttachment(syncdomain.Attachment{Kind: syncdomain.AttachmentSkill, Skill: &skill}) + require.NoError(t, err) + assert.Equal(t, "---\nkey: support\ndescription: Follow the standard support process.\n---\n\nFollow the support process.\n", string(rendered), + "LaunchDarkly owns the name, so the file does not get it") + + rendered, err = renderAttachment(syncdomain.Attachment{Kind: syncdomain.AttachmentSkill, Upsert: true, Skill: &skill}) + require.NoError(t, err) + assert.Contains(t, string(rendered), "\nupsert: true\n") +} + +func TestReadSkillReadsTheOptionalNameAndUpsert(t *testing.T) { + fsys := fstest.MapFS{ + ".launchdarkly/project/skills/handoff.md": { + Data: []byte("---\nkey: handoff\ndescription: Hand off\nname: Human handoff\nupsert: true\n---\n\n# Handoff\n"), + }, + } + + attachment, err := readSkill(fsys, "project", "handoff") require.NoError(t, err) - assert.Equal(t, "---\nkey: support\ndescription: Follow the standard support process.\n---\n\nFollow the support process.\n", string(rendered)) + assert.True(t, attachment.Upsert) + assert.Equal(t, "Human handoff", attachment.Skill.Name) + assert.Equal(t, "# Handoff\n", attachment.Skill.Markdown) } func TestReadSkillRejectsKeyThatDoesNotMatchFilename(t *testing.T) { @@ -144,7 +165,7 @@ func TestPreserveToolUpsertAcrossServerWrites(t *testing.T) { }) require.NoError(t, err) - preserved, err := preserveToolUpsert("tools/search.json", original, replacement) + preserved, err := preserveLocalFields("tools/search.json", original, replacement) require.NoError(t, err) var file toolFile @@ -153,6 +174,23 @@ func TestPreserveToolUpsertAcrossServerWrites(t *testing.T) { assert.Equal(t, "New", *file.Description) } +func TestPreserveSkillNameAndUpsertAcrossServerWrites(t *testing.T) { + original := []byte("---\nkey: handoff\ndescription: Old\nname: Human handoff\nupsert: true\n---\n\n# Old\n") + server := syncdomain.Skill{Key: "handoff", Name: "Renamed in LaunchDarkly", Description: "New", Markdown: "# New\n"} + replacement, err := renderAttachment(syncdomain.Attachment{Kind: syncdomain.AttachmentSkill, Skill: &server}) + require.NoError(t, err) + + preserved, err := preserveLocalFields("project/skills/handoff.md", original, replacement) + + require.NoError(t, err) + assert.Equal(t, "---\nkey: handoff\ndescription: New\nname: Human handoff\nupsert: true\n---\n\n# New\n", string(preserved)) + + plain := []byte("---\nkey: handoff\ndescription: Old\n---\n\n# Old\n") + preserved, err = preserveLocalFields("project/skills/handoff.md", plain, replacement) + require.NoError(t, err) + assert.Equal(t, replacement, preserved) +} + func TestReplaceVariationsRemovesNewAttachmentWhenVariationUpdateFails(t *testing.T) { root := t.TempDir() store := NewStore(root) diff --git a/internal/sync/local/replace.go b/internal/sync/local/replace.go index d8aa4ed31..3d83aa5b0 100644 --- a/internal/sync/local/replace.go +++ b/internal/sync/local/replace.go @@ -189,7 +189,7 @@ func (store Store) prepareLinkedFile(ref Reference, replacement VariationReplace } // prepareAttachmentFiles renders each tool and skill file that changed. A new -// file is created. A tool file keeps its local upsert flag. +// file is created. An existing file keeps its local fields. func (store Store) prepareAttachmentFiles(files []VariationFile) ([]stagedFile, error) { rendered, err := renderAttachmentFiles(files) if err != nil { @@ -214,7 +214,7 @@ func (store Store) prepareAttachmentFiles(files []VariationFile) ([]stagedFile, if err != nil { return nil, fmt.Errorf("inspect attachment %q: %w", file.Path, err) } - if next.replacement, err = preserveToolUpsert(file.Path, original, file.Content); err != nil { + if next.replacement, err = preserveLocalFields(file.Path, original, file.Content); err != nil { return nil, err } next.original, next.originalExists, next.mode = original, true, info.Mode().Perm() diff --git a/internal/sync/name.go b/internal/sync/name.go new file mode 100644 index 000000000..a51e5f44e --- /dev/null +++ b/internal/sync/name.go @@ -0,0 +1,15 @@ +package sync + +import ( + "strings" + "unicode" +) + +// DisplayName converts a key such as "support-agent" to "Support agent". +func DisplayName(key string) string { + runes := []rune(strings.NewReplacer("-", " ", "_", " ").Replace(key)) + if len(runes) != 0 { + runes[0] = unicode.ToUpper(runes[0]) + } + return string(runes) +} diff --git a/internal/sync/prompt/acceptance_test.go b/internal/sync/prompt/acceptance_test.go index 470c1a24c..afc948c46 100644 --- a/internal/sync/prompt/acceptance_test.go +++ b/internal/sync/prompt/acceptance_test.go @@ -111,6 +111,18 @@ func (api *directAPI) MakeRequest( } return json.Marshal(tool) } + if strings.HasSuffix(path, "/ai-configs/skills") && method == http.MethodPost { + var skill versionedSkill + if err := json.Unmarshal(body, &skill.Skill); err != nil { + return nil, err + } + skill.Version = 1 + if api.skills == nil { + api.skills = make(map[string]versionedSkill) + } + api.skills[skill.Key] = skill + return json.Marshal(skill) + } if strings.Contains(path, "/ai-configs/skills/") { key := path[strings.LastIndex(path, "/")+1:] skill, ok := api.skills[key] @@ -413,6 +425,33 @@ func TestPromptCreatesMissingToolWhenUpsertIsEnabled(t *testing.T) { require.Equal(t, []syncdomain.AttachmentRef{{Key: "my-second-tool", Version: 1}}, api.variation.Tools) } +func TestPromptCreatesMissingSkillWhenUpsertIsEnabled(t *testing.T) { + root := initRepository(t) + baseline := variation("Baseline") + writeVariation(t, root, baseline, true) + writeManifest(t, root, baseline) + withSkill := baseline + withSkill.Skills = []syncdomain.AttachmentRef{{Key: "handoff"}} + _, err := synclocal.NewStore(root).ReplaceVariations([]synclocal.VariationReplacement{{ + ProjectKey: "production", ConfigKey: "support", Variation: withSkill, + }}) + require.NoError(t, err) + skillFile := "---\nkey: handoff\ndescription: Hand off to a person\nname: Human handoff\nupsert: true\n---\n\n# Handoff\n" + writeLocalFile(t, root, "skills/handoff.md", skillFile) + api := &directAPI{variation: pointer(baseline)} + + _, _, err = runPrompt(t, root, api, "--yes") + + require.NoError(t, err) + assert.Equal(t, syncdomain.Skill{ + Key: "handoff", Name: "Human handoff", Description: "Hand off to a person", Markdown: "# Handoff\n", + }, api.skills["handoff"].Skill) + assert.Equal(t, []syncdomain.AttachmentRef{{Key: "handoff", Version: 1}}, api.variation.Skills) + content, err := os.ReadFile(filepath.Join(root, syncdomain.RootDir, "production", "skills", "handoff.md")) + require.NoError(t, err) + assert.Equal(t, skillFile, string(content)) +} + func TestPromptExplainsUpsertForMissingTool(t *testing.T) { root := initRepository(t) baseline := variation("Baseline") diff --git a/internal/sync/prompt/attachment_versions.go b/internal/sync/prompt/attachment_versions.go index dd66981d6..4f1d8d3df 100644 --- a/internal/sync/prompt/attachment_versions.go +++ b/internal/sync/prompt/attachment_versions.go @@ -58,24 +58,32 @@ func (resolver *attachmentResolver) resolve(projectKey string, local syncdomain. } func (resolver *attachmentResolver) publish(projectKey string, local syncdomain.Attachment) (syncdomain.Attachment, error) { - remote, err := resolver.client.ReadAttachment(projectKey, local.Kind, local.Key()) + remote, readErr := resolver.client.ReadAttachment(projectKey, local.Kind, local.Key()) var writeErr error switch { - case err == nil && syncdomain.SameAttachmentContent(local, remote): + case readErr == nil && syncdomain.SameAttachmentContent(local, remote): return remote, nil - case err == nil: + case readErr == nil: writeErr = resolver.client.UpdateAttachment(projectKey, local) - case syncapi.IsNotFound(err) && local.Kind == syncdomain.AttachmentTool && local.Upsert: + case syncapi.IsNotFound(readErr) && local.Upsert: writeErr = resolver.client.CreateAttachment(projectKey, local) - case syncapi.IsNotFound(err) && local.Kind == syncdomain.AttachmentTool: + case syncapi.IsNotFound(readErr) && local.Kind == syncdomain.AttachmentTool: return syncdomain.Attachment{}, fmt.Errorf( "tool %q does not exist in LaunchDarkly; add \"upsert\": true to its local JSON file to create it during sync", local.Key(), ) + case syncapi.IsNotFound(readErr): + return syncdomain.Attachment{}, fmt.Errorf( + "skill %q does not exist in LaunchDarkly; add upsert: true to its front matter to create it during sync", + local.Key(), + ) default: - return syncdomain.Attachment{}, err + return syncdomain.Attachment{}, readErr } - if writeErr != nil && !syncapi.MutationMayHaveSucceeded(writeErr) { + // A 409 from a create means that the attachment exists now, for example + // because another working copy created it. The read below decides. + createConflict := readErr != nil && syncapi.IsConflict(writeErr) + if writeErr != nil && !syncapi.MutationMayHaveSucceeded(writeErr) && !createConflict { return syncdomain.Attachment{}, writeErr } @@ -86,8 +94,11 @@ func (resolver *attachmentResolver) publish(projectKey string, local syncdomain. return syncdomain.Attachment{}, errors.Join(writeErr, err) } if !syncdomain.SameAttachmentContent(local, observed) { - if writeErr == nil { + switch { + case writeErr == nil: writeErr = fmt.Errorf("%s %q changed concurrently", local.Kind, local.Key()) + case createConflict: + writeErr = fmt.Errorf("%s %q changed concurrently: %w", local.Kind, local.Key(), writeErr) } return syncdomain.Attachment{}, writeErr } diff --git a/internal/sync/prompt/execute.go b/internal/sync/prompt/execute.go index 94cd4ddc3..1696254e4 100644 --- a/internal/sync/prompt/execute.go +++ b/internal/sync/prompt/execute.go @@ -157,8 +157,8 @@ func applyResourceChange( } // applyServerChange writes one variation to LaunchDarkly. If the write fails -// without a response, it reads the variation to find whether the write -// succeeded. +// without a response, or a create gets a 409 because the variation exists +// now, it reads the variation to find whether LaunchDarkly has the content. func applyServerChange(client syncapi.Client, attachments *attachmentResolver, resource PlannedResource) error { configKey, variationKey, err := resource.ID.VariationKeys() if err != nil { @@ -183,7 +183,8 @@ func applyServerChange(client syncapi.Client, attachments *attachmentResolver, r default: return fmt.Errorf("action %q does not change LaunchDarkly", resource.Action) } - if writeErr == nil || !syncapi.MutationMayHaveSucceeded(writeErr) { + createConflict := resource.Action == ActionCreateServer && syncapi.IsConflict(writeErr) + if writeErr == nil || !syncapi.MutationMayHaveSucceeded(writeErr) && !createConflict { return writeErr } diff --git a/internal/sync/prompt/execute_test.go b/internal/sync/prompt/execute_test.go index d4f757281..b4bac9c5f 100644 --- a/internal/sync/prompt/execute_test.go +++ b/internal/sync/prompt/execute_test.go @@ -300,3 +300,105 @@ func (api *definitiveMutationAPI) MakeRequest( func (*definitiveMutationAPI) MakeUnauthenticatedRequest(string, string, []byte) ([]byte, error) { return nil, nil } + +// Another working copy can create the same tool or skill first. A create that +// gets a 409 reads the attachment again, and succeeds when LaunchDarkly has +// the local content. +func TestPublishReadsAgainAfterACreateConflict(t *testing.T) { + description := "Search the docs" + local := syncdomain.Attachment{Kind: syncdomain.AttachmentTool, Upsert: true, Tool: &syncdomain.Tool{ + Key: "search", Description: &description, Schema: map[string]any{"type": "object"}, + }} + + tests := map[string]struct { + remote string + wantError string + }{ + "same content": {remote: `{"key":"search","description":"Search the docs","schema":{"type":"object"},"version":1}`}, + "different content": {remote: `{"key":"search","schema":{"type":"string"},"version":1}`, wantError: `tool "search" changed concurrently`}, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + transport := &createConflictAPI{remote: test.remote} + client := syncapi.NewClient(transport, "token", "https://example.com") + + published, err := newAttachmentResolver(client).resolve("project", local) + + if test.wantError != "" { + require.ErrorContains(t, err, test.wantError) + require.ErrorContains(t, err, `"statusCode":409`) + return + } + require.NoError(t, err) + assert.Equal(t, 1, published.Version) + assert.Equal(t, []string{"GET", "POST", "GET"}, transport.methods) + }) + } +} + +func TestPublishCreatesASkillOnlyWithUpsert(t *testing.T) { + skill := syncdomain.Attachment{Kind: syncdomain.AttachmentSkill, Skill: &syncdomain.Skill{Key: "handoff", Markdown: "# Handoff\n"}} + client := syncapi.NewClient(&createConflictAPI{}, "token", "https://example.com") + + _, err := newAttachmentResolver(client).resolve("project", skill) + + require.EqualError(t, err, `skill "handoff" does not exist in LaunchDarkly; add upsert: true to its front matter to create it during sync`) +} + +func TestApplyServerChangeReadsAgainAfterAVariationCreateConflict(t *testing.T) { + local := testVariation("New") + resource := PlannedResource{ID: testResourceID(), Action: ActionCreateServer, Local: &local} + var err error + resource.LocalFingerprint, err = syncdomain.FingerprintVariation(resource.ID.ProjectKey, resource.ID.LookupKey, local) + require.NoError(t, err) + + tests := map[string]struct { + remote syncdomain.Variation + wantError string + }{ + "same content": {remote: local}, + "different content": {remote: testVariation("Other"), wantError: "server variation changed concurrently"}, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + remote, err := json.Marshal(map[string]any{"key": "support", "mode": "agent", "variations": []syncdomain.Variation{test.remote}}) + require.NoError(t, err) + client := syncapi.NewClient(&createConflictAPI{remote: string(remote)}, "token", "https://example.com") + + err = applyServerChange(client, newAttachmentResolver(client), resource) + + if test.wantError != "" { + require.ErrorContains(t, err, test.wantError) + return + } + require.NoError(t, err) + }) + } +} + +// createConflictAPI answers each create with a 409. A read returns remote +// after the create, and a 404 before it. +type createConflictAPI struct { + remote string + created bool + methods []string +} + +func (api *createConflictAPI) MakeRequest(_ string, method string, _ string, _ string, _ url.Values, _ []byte, _ bool) ([]byte, error) { + api.methods = append(api.methods, method) + switch { + case method == "POST": + api.created = true + return nil, errors.New(`{"code":"conflict","statusCode":409}`) + case api.created: + return []byte(api.remote), nil + default: + return nil, errors.New(`{"code":"not_found","statusCode":404}`) + } +} + +func (*createConflictAPI) MakeUnauthenticatedRequest(string, string, []byte) ([]byte, error) { + return nil, nil +} diff --git a/internal/sync/prompt/output.go b/internal/sync/prompt/output.go index 7fa43a8ef..e3b06a21a 100644 --- a/internal/sync/prompt/output.go +++ b/internal/sync/prompt/output.go @@ -316,6 +316,8 @@ func repinsOnly(resource PlannedResource) bool { // "instructions changed in LaunchDarkly". func planReason(resource PlannedResource) string { switch { + case resource.Action == ActionCreateServer && resource.LocalAttachment != nil && resource.LocalAttachment.Skill != nil: + return fmt.Sprintf("new, name %q", resource.LocalAttachment.Skill.CreateName()) case resource.Action == ActionCreateServer: return "new" case repinsOnly(resource): diff --git a/internal/sync/prompt/output_test.go b/internal/sync/prompt/output_test.go index f7fba07ce..f3a1f21bf 100644 --- a/internal/sync/prompt/output_test.go +++ b/internal/sync/prompt/output_test.go @@ -249,7 +249,19 @@ func TestPlanReasonNamesTheChange(t *testing.T) { resource PlannedResource want string }{ - "create": {PlannedResource{Action: ActionCreateServer}, "new"}, + "create": {PlannedResource{Action: ActionCreateServer}, "new"}, + "create a skill with a name": { + PlannedResource{Action: ActionCreateServer, LocalAttachment: &syncdomain.Attachment{ + Kind: syncdomain.AttachmentSkill, Skill: &syncdomain.Skill{Key: "handoff", Name: "Human handoff"}, + }}, + `new, name "Human handoff"`, + }, + "create a skill without a name": { + PlannedResource{Action: ActionCreateServer, LocalAttachment: &syncdomain.Attachment{ + Kind: syncdomain.AttachmentSkill, Skill: &syncdomain.Skill{Key: "incident-summary"}, + }}, + `new, name "Incident summary"`, + }, "restore": {PlannedResource{Action: ActionUpdateLocal}, "local file missing"}, "delete": {PlannedResource{Action: ActionDeleteLocal}, "deleted in LaunchDarkly"}, "forget": {PlannedResource{Action: ActionRemoveManifest}, "gone on both sides"}, diff --git a/internal/sync/prompt/plan.go b/internal/sync/prompt/plan.go index c75bf9b6d..ad6e281f3 100644 --- a/internal/sync/prompt/plan.go +++ b/internal/sync/prompt/plan.go @@ -274,29 +274,29 @@ func planAttachment( 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. +// 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 { - create := "" - if id.Kind == syncdomain.KindTool { - create = `To create it during sync, add "upsert": true to its file. ` + 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. %sTo stop syncing it, run ldcli sync %ss detach --project %s --key %s.", - id.Kind, id.LookupKey, create, id.Kind, id.ProjectKey, id.LookupKey) + 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 + var name, description string switch { case attachment.Tool != nil && attachment.Tool.Description != nil: description = *attachment.Tool.Description case attachment.Skill != nil: - description = attachment.Skill.Description + name, description = attachment.Skill.CreateName(), attachment.Skill.Description } - return syncdomain.ValidateCreate(syncdomain.Kind(attachment.Kind), attachment.Key(), "", description) + return syncdomain.ValidateCreate(syncdomain.Kind(attachment.Kind), attachment.Key(), name, description) } // rejectInvalidCreate turns a create that LaunchDarkly would reject into an diff --git a/internal/sync/prompt/plan_test.go b/internal/sync/prompt/plan_test.go index 109ec2bd4..010c5fb6a 100644 --- a/internal/sync/prompt/plan_test.go +++ b/internal/sync/prompt/plan_test.go @@ -246,13 +246,24 @@ func TestPlanAttachmentThreeWayRules(t *testing.T) { } } +// LaunchDarkly owns the name of a skill, so a rename there is not a change. +func TestPlanAttachmentIgnoresTheSkillName(t *testing.T) { + local, server := testAttachment(syncdomain.AttachmentSkill, "same"), testAttachment(syncdomain.AttachmentSkill, "same") + local.Skill.Name, server.Skill.Name = "Summarize", "Summarize a ticket" + + resource := planAttachment(local.ID("production"), &local, &server, attachmentFingerprint(t, local), true) + + assert.Equal(t, ActionInSync, resource.Action) + assert.Empty(t, resource.Diff) +} + 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. `+ + 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) }