diff --git a/internal/sync/bootstrap/attachments.go b/internal/sync/bootstrap/attachments.go index c08207d3..95738220 100644 --- a/internal/sync/bootstrap/attachments.go +++ b/internal/sync/bootstrap/attachments.go @@ -75,25 +75,9 @@ func promptForAttachment(options Options) ([]syncdomain.ResourceID, bool, error) // selection that this repository already syncs, or that LaunchDarkly does not // have, is an error. The local checks come first, so they need no read. func readNewAttachments(options Options, selections []syncdomain.ResourceID) ([]syncdomain.SyncedAttachment, error) { - lock, err := syncmanifest.ReadLock(options.Store) - if err != nil { + if err := CheckNotSynced(options.Store, selections); err != nil { return nil, err } - for _, id := range selections { - exists, err := options.Store.AttachmentExists(id) - switch { - case err != nil: - return nil, err - case exists: - path, err := synclocal.AttachmentPath(id) - if err != nil { - return nil, err - } - return nil, fmt.Errorf("%s is already synced. Edit %s and run ldcli sync %ss.", id.Label(), path, id.Kind) - case slices.ContainsFunc(lock.Resources, func(resource syncmanifest.Resource) bool { return resource.ID() == id }): - return nil, fmt.Errorf("%s has a sync.lock entry but no local file. Run ldcli sync %ss to restore it.", id.Label(), id.Kind) - } - } attachments := make([]syncdomain.SyncedAttachment, 0, len(selections)) for _, id := range selections { @@ -109,6 +93,32 @@ func readNewAttachments(options Options, selections []syncdomain.ResourceID) ([] return attachments, nil } +// CheckNotSynced returns an error for a tool or skill that this repository +// already syncs, because it has a local file or a sync.lock entry. It reads +// only local files. +func CheckNotSynced(store synclocal.Store, ids []syncdomain.ResourceID) error { + lock, err := syncmanifest.ReadLock(store) + if err != nil { + return err + } + for _, id := range ids { + exists, err := store.AttachmentExists(id) + switch { + case err != nil: + return err + case exists: + path, err := synclocal.AttachmentPath(id) + if err != nil { + return err + } + return fmt.Errorf("%s is already synced. Edit %s and run ldcli sync %ss or ldcli sync all.", id.Label(), path, id.Kind) + case slices.ContainsFunc(lock.Resources, func(resource syncmanifest.Resource) bool { return resource.ID() == id }): + return fmt.Errorf("%s has a sync.lock entry but no local file. Run ldcli sync %ss or ldcli sync all to restore it.", id.Label(), id.Kind) + } + } + return nil +} + // writeAttachments writes the files and records their baseline. If the // baseline save fails, it removes the files again. func writeAttachments(options Options, attachments []syncdomain.SyncedAttachment) error { diff --git a/internal/sync/bootstrap/attachments_test.go b/internal/sync/bootstrap/attachments_test.go index b6e16caa..603b7761 100644 --- a/internal/sync/bootstrap/attachments_test.go +++ b/internal/sync/bootstrap/attachments_test.go @@ -65,11 +65,11 @@ func TestRunRefusesAToolBeforeAnyWrite(t *testing.T) { "missing in LaunchDarkly": {want: "tool project/search does not exist in LaunchDarkly."}, "already synced": { local: true, launchDarkly: []syncdomain.Attachment{search}, - want: "tool project/search is already synced. Edit .launchdarkly/project/tools/search.json and run ldcli sync tools.", + want: "tool project/search is already synced. Edit .launchdarkly/project/tools/search.json and run ldcli sync tools or ldcli sync all.", }, "lock entry without a file": { tracked: true, launchDarkly: []syncdomain.Attachment{search}, - want: "tool project/search has a sync.lock entry but no local file. Run ldcli sync tools to restore it.", + want: "tool project/search has a sync.lock entry but no local file. Run ldcli sync tools or ldcli sync all to restore it.", }, } diff --git a/internal/sync/create/attachment.go b/internal/sync/create/attachment.go new file mode 100644 index 00000000..04fb735a --- /dev/null +++ b/internal/sync/create/attachment.go @@ -0,0 +1,194 @@ +package create + +import ( + "encoding/json" + "errors" + "fmt" + "io" + "slices" + "strings" + + "github.com/charmbracelet/huh" + + syncdomain "github.com/launchdarkly/ldcli/internal/sync" + syncinteractive "github.com/launchdarkly/ldcli/internal/sync/interactive" +) + +// definitionKeys are top-level keys of a tool file or of an MCP tool +// definition. A --json-schema value with one of them is a pasted definition, +// not an input schema. +var definitionKeys = []string{"formatVersion", "inputSchema", "schema"} + +// Check applies the content rules of a create of a tool or a skill. It reads +// nothing, so an add runs it before any request. +func (input Input) Check(kind syncdomain.AttachmentKind) error { + if err := input.checkContentFlag(kind); err != nil { + return err + } + if kind == syncdomain.AttachmentTool { + _, err := parseSchema(*input.JSONSchema) + return err + } + switch { + case strings.TrimSpace(*input.Content) == "": + return errors.New("--content cannot be blank") + case input.Name != nil && strings.TrimSpace(*input.Name) == "": + return errors.New("--name cannot be blank") + } + return nil +} + +// checkContentFlag makes sure that the flag that starts a create is set when +// another create flag is. +func (input Input) checkContentFlag(kind syncdomain.AttachmentKind) error { + flag, content := "--content", input.Content + if kind == syncdomain.AttachmentTool { + flag, content = "--json-schema", input.JSONSchema + } + if content != nil { + return nil + } + + var passed []string + if input.Name != nil { + passed = append(passed, "--name") + } + if input.Description != nil { + passed = append(passed, "--description") + } + switch len(passed) { + case 0: + return fmt.Errorf("add creates a %s only when %s is set", kind, flag) + case 1: + return fmt.Errorf("%s needs %s. add creates a %s only when %s is set.", passed[0], flag, kind, flag) + default: + return fmt.Errorf("%s need %s. add creates a %s only when %s is set.", joinFlags(passed), flag, kind, flag) + } +} + +// parseSchema decodes the value of --json-schema, which must be one JSON +// object. +func parseSchema(value string) (map[string]any, error) { + decoder := json.NewDecoder(strings.NewReader(value)) + var decoded any + if err := decoder.Decode(&decoded); err != nil { + return nil, fmt.Errorf("--json-schema must be one JSON object: %w", err) + } + schema, isObject := decoded.(map[string]any) + if !isObject || decoder.Decode(new(any)) != io.EOF { + return nil, errors.New("--json-schema must be one JSON object") + } + if slices.ContainsFunc(definitionKeys, func(key string) bool { _, ok := schema[key]; return ok }) { + return nil, errors.New(`--json-schema is the input schema only. Pass the value of "inputSchema", ` + + "and pass the description with --description.") + } + return schema, nil +} + +// Ask returns the input with the name and the description of a new skill +// that the flags do not give. A tool has no questions. Without questions, the +// input does not change, so the name comes from the key and the description +// is empty. An answer that keeps the name of the key does not set a name. The +// bool result is true when the user cancels. +func (input Input) Ask(id syncdomain.ResourceID, questions Questions) (Input, bool, error) { + if id.Kind != syncdomain.KindSkill || !questions.Allowed { + return input, false, nil + } + + keyName := syncdomain.DisplayName(id.LookupKey) + name, description := keyName, "" + var fields []huh.Field + if input.Name == nil { + fields = append(fields, huh.NewInput().Title("Skill name").Value(&name).Validate(RequiredValue("skill name"))) + } + if input.Description == nil { + fields = append(fields, huh.NewInput().Title("Skill description").Value(&description)) + } + if len(fields) == 0 { + return input, false, nil + } + if canceled, err := syncinteractive.RunForm(questions.Input, questions.Output, fields...); err != nil || canceled { + return input, canceled, err + } + + if input.Name == nil && name != keyName { + input.Name = &name + } + if input.Description == nil { + input.Description = &description + } + return input, false, nil +} + +// Attachment returns the tool or the skill that the input creates. It has +// upsert, so that its file can create it again. A skill without a name takes +// the name of its key, and its Markdown ends with a newline, so that content +// from "$(cat file)" has the fingerprint of the file. +func (input Input) Attachment(id syncdomain.ResourceID) (syncdomain.Attachment, error) { + kind := syncdomain.AttachmentKind(id.Kind) + if err := input.Check(kind); err != nil { + return syncdomain.Attachment{}, err + } + + attachment := syncdomain.Attachment{Kind: kind, Upsert: true} + var name, description string + if input.Description != nil { + description = *input.Description + } + if kind == syncdomain.AttachmentTool { + schema, err := parseSchema(*input.JSONSchema) + if err != nil { + return syncdomain.Attachment{}, err + } + attachment.Tool = &syncdomain.Tool{Key: id.LookupKey, Schema: schema} + if description != "" { + attachment.Tool.Description = &description + } + } else { + skill := syncdomain.Skill{Key: id.LookupKey, Description: description, Markdown: withFinalNewline(*input.Content)} + if input.Name != nil { + skill.Name = *input.Name + } + skill.Name = skill.CreateName() + name, attachment.Skill = skill.Name, &skill + } + if err := syncdomain.ValidateCreate(id.Kind, id.LookupKey, name, description); err != nil { + return syncdomain.Attachment{}, err + } + return attachment, nil +} + +// Matches reports whether LaunchDarkly already has the value of each flag +// that the user passed: the schema or the Markdown, and the description when +// it is set. created is the attachment of the input. The fields without a +// flag, such as tool tags and the name of a skill, come from LaunchDarkly. +func (input Input) Matches(created, existing syncdomain.Attachment) bool { + candidate := existing + switch { + case created.Tool != nil && existing.Tool != nil: + tool := *existing.Tool + tool.Schema = created.Tool.Schema + if input.Description != nil { + tool.Description = created.Tool.Description + } + candidate.Tool = &tool + case created.Skill != nil && existing.Skill != nil: + skill := *existing.Skill + skill.Markdown = created.Skill.Markdown + if input.Description != nil { + skill.Description = created.Skill.Description + } + candidate.Skill = &skill + default: + return false + } + return syncdomain.SameAttachmentContent(candidate, existing) +} + +// withFinalNewline returns text that ends with a newline. +func withFinalNewline(text string) string { + if strings.HasSuffix(text, "\n") { + return text + } + return text + "\n" +} diff --git a/internal/sync/create/create.go b/internal/sync/create/create.go new file mode 100644 index 00000000..f2fcfa7e --- /dev/null +++ b/internal/sync/create/create.go @@ -0,0 +1,116 @@ +// Package create collects and checks a resource that an add command creates +// from flags. It checks the flags before any read, and asks in a terminal for +// each value that the flags do not give. +package create + +import ( + "fmt" + "io" + "strings" + + "github.com/charmbracelet/huh" + + syncdomain "github.com/launchdarkly/ldcli/internal/sync" + syncinteractive "github.com/launchdarkly/ldcli/internal/sync/interactive" +) + +// Input is the content that the create flags of an add give. A nil field is a +// flag that the command line did not set. +type Input struct { + // Content is the Markdown of a skill. + Content *string + // JSONSchema is the input schema of a tool. + JSONSchema *string + // Name is the name of a skill. + Name *string + Description *string +} + +// Questions are the streams that ask the user for the values that the flags +// do not give. Allowed is false when the command cannot ask, because of +// --no-input or because the streams are not a terminal. +type Questions struct { + Allowed bool + Input io.Reader + Output io.Writer + Projects syncinteractive.ProjectSearcher +} + +// Target returns the identity of the new resource. An empty projectKey or key +// comes from a question. Without questions, the error names every missing +// flag. The bool result is true when the user cancels. +func Target(kind syncdomain.Kind, projectKey, key string, questions Questions) (syncdomain.ResourceID, bool, error) { + id := syncdomain.ResourceID{Kind: kind, ProjectKey: projectKey, LookupKey: key} + var missing []string + if projectKey == "" { + missing = append(missing, "--project") + } + if key == "" { + missing = append(missing, "--key") + } + switch { + case len(missing) == 0: + return id, false, nil + case !questions.Allowed: + return id, false, fmt.Errorf("%s creation requires %s without interactive input", kind, joinFlags(missing)) + } + + if id.ProjectKey == "" { + project, canceled, err := syncinteractive.SelectProject(questions.Input, questions.Output, questions.Projects) + if err != nil || canceled { + return id, canceled, err + } + id.ProjectKey = project.Key + } + if id.LookupKey == "" { + field := huh.NewInput().Title(capitalize(string(kind)) + " key").Value(&id.LookupKey).Validate(keyValidator(kind)) + if canceled, err := syncinteractive.RunForm(questions.Input, questions.Output, field); err != nil || canceled { + return id, canceled, err + } + } + return id, false, nil +} + +// keyValidator returns a form validator that applies the create rules of +// LaunchDarkly to a new key of kind. +func keyValidator(kind syncdomain.Kind) func(string) error { + return func(key string) error { + if err := syncdomain.ValidateKey(key); err != nil { + return fmt.Errorf("%s key %w", kind, err) + } + return syncdomain.ValidateCreate(kind, key, "", "") + } +} + +// RequiredValue returns a form validator that rejects a blank value. +func RequiredValue(label string) func(string) error { + return func(value string) error { + if strings.TrimSpace(value) == "" { + return fmt.Errorf("%s is required", label) + } + return nil + } +} + +// joinFlags joins flag names into a list, for example "--project and --key". +func joinFlags(flags []string) string { + last := len(flags) - 1 + switch { + case last < 0: + return "" + case last == 0: + return flags[0] + case last == 1: + return flags[0] + " and " + flags[1] + default: + return strings.Join(flags[:last], ", ") + ", and " + flags[last] + } +} + +// capitalize returns text with an uppercase first letter. +func capitalize(text string) string { + if text == "" { + return "" + } + return strings.ToUpper(text[:1]) + text[1:] +} diff --git a/internal/sync/create/create_test.go b/internal/sync/create/create_test.go new file mode 100644 index 00000000..0accc520 --- /dev/null +++ b/internal/sync/create/create_test.go @@ -0,0 +1,253 @@ +package create + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + syncdomain "github.com/launchdarkly/ldcli/internal/sync" +) + +var ( + refundID = syncdomain.ResourceID{Kind: syncdomain.KindTool, ProjectKey: "demo", LookupKey: "refund"} + triageID = syncdomain.ResourceID{Kind: syncdomain.KindSkill, ProjectKey: "demo", LookupKey: "incident-triage"} +) + +func TestCheckNeedsTheFlagThatStartsTheCreate(t *testing.T) { + tests := map[string]struct { + kind syncdomain.AttachmentKind + input Input + want string + }{ + "tool description": { + kind: syncdomain.AttachmentTool, + input: Input{Description: pointer("Issue a refund")}, + want: "--description needs --json-schema. add creates a tool only when --json-schema is set.", + }, + "skill name": { + kind: syncdomain.AttachmentSkill, + input: Input{Name: pointer("Triage")}, + want: "--name needs --content. add creates a skill only when --content is set.", + }, + "skill name and description": { + kind: syncdomain.AttachmentSkill, + input: Input{Name: pointer("Triage"), Description: pointer("")}, + want: "--name and --description need --content. add creates a skill only when --content is set.", + }, + "no flag": { + kind: syncdomain.AttachmentTool, + want: "add creates a tool only when --json-schema is set", + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + require.EqualError(t, test.input.Check(test.kind), test.want) + }) + } +} + +func TestCheckDecodesTheJSONSchemaStrictly(t *testing.T) { + const definition = `--json-schema is the input schema only. Pass the value of "inputSchema", ` + + "and pass the description with --description." + tests := map[string]struct { + schema string + want string + }{ + "object": {schema: `{"type": "object", "properties": {"schema": {"type": "string"}}}`}, + "empty object": {schema: `{}`}, + "empty": {schema: ``, want: "--json-schema must be one JSON object: EOF"}, + "invalid": {schema: `{"type":`, want: "--json-schema must be one JSON object: unexpected EOF"}, + "array": {schema: `[{"type": "object"}]`, want: "--json-schema must be one JSON object"}, + "null": {schema: `null`, want: "--json-schema must be one JSON object"}, + "two values": {schema: `{"type": "object"} {}`, want: "--json-schema must be one JSON object"}, + "MCP definition": {schema: `{"name": "refund", "inputSchema": {"type": "object"}}`, want: definition}, + "tool file": {schema: `{"formatVersion": 1, "key": "refund", "schema": {}}`, want: definition}, + "tool file without format": {schema: `{"key": "refund", "schema": {"type": "object"}}`, want: definition}, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + err := Input{JSONSchema: pointer(test.schema)}.Check(syncdomain.AttachmentTool) + if test.want == "" { + require.NoError(t, err) + return + } + require.EqualError(t, err, test.want) + }) + } +} + +func TestCheckRejectsBlankSkillValues(t *testing.T) { + require.EqualError(t, Input{Content: pointer(" \n")}.Check(syncdomain.AttachmentSkill), "--content cannot be blank") + require.EqualError(t, Input{Content: pointer("Triage."), Name: pointer(" ")}.Check(syncdomain.AttachmentSkill), + "--name cannot be blank") + require.NoError(t, Input{Content: pointer("Triage."), Description: pointer("")}.Check(syncdomain.AttachmentSkill)) +} + +func TestAttachmentBuildsATool(t *testing.T) { + input := Input{JSONSchema: pointer(`{"type": "object", "required": ["orderId"]}`), Description: pointer("Issue a refund")} + + tool, err := input.Attachment(refundID) + + require.NoError(t, err) + assert.Equal(t, syncdomain.Attachment{ + Kind: syncdomain.AttachmentTool, + Upsert: true, + Tool: &syncdomain.Tool{ + Key: "refund", Description: pointer("Issue a refund"), + Schema: map[string]any{"type": "object", "required": []any{"orderId"}}, + }, + }, tool) + + input.Description = pointer("") + tool, err = input.Attachment(refundID) + require.NoError(t, err) + assert.Nil(t, tool.Tool.Description, "an empty description is no description, as in a tool file") +} + +func TestAttachmentBuildsASkill(t *testing.T) { + tests := map[string]struct { + input Input + want syncdomain.Skill + }{ + "name from the key": { + input: Input{Content: pointer("Sort by severity.")}, + want: syncdomain.Skill{Key: "incident-triage", Name: "Incident triage", Markdown: "Sort by severity.\n"}, + }, + "name and description from flags": { + input: Input{Content: pointer("Sort by severity.\n"), Name: pointer("Triage"), Description: pointer("Sort incidents")}, + want: syncdomain.Skill{ + Key: "incident-triage", Name: "Triage", Description: "Sort incidents", Markdown: "Sort by severity.\n", + }, + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + skill, err := test.input.Attachment(triageID) + + require.NoError(t, err) + assert.Equal(t, syncdomain.Attachment{Kind: syncdomain.AttachmentSkill, Upsert: true, Skill: &test.want}, skill) + }) + } +} + +func TestAttachmentAppliesTheCreateRules(t *testing.T) { + skill := Input{Content: pointer("Sort by severity.")} + + _, err := skill.Attachment(syncdomain.ResourceID{Kind: syncdomain.KindSkill, ProjectKey: "demo", LookupKey: "Triage"}) + require.EqualError(t, err, + `skill key "Triage" must use only lowercase letters, digits, and hyphens, and start with a letter or digit`) + + skill.Description = pointer(strings.Repeat("d", 1025)) + _, err = skill.Attachment(triageID) + require.EqualError(t, err, "skill description must be at most 1024 characters") + + skill.Description, skill.Name = nil, pointer(strings.Repeat("n", 257)) + _, err = skill.Attachment(triageID) + require.EqualError(t, err, "skill name must be at most 256 characters") + + tool := Input{JSONSchema: pointer(`{}`), Description: pointer(strings.Repeat("d", 1025))} + _, err = tool.Attachment(refundID) + require.EqualError(t, err, "tool description must be at most 1024 characters") +} + +func TestMatchesComparesOnlyThePassedFields(t *testing.T) { + tagged := syncdomain.Attachment{Kind: syncdomain.AttachmentTool, Version: 3, Tool: &syncdomain.Tool{ + Key: "refund", Description: pointer("Issue a refund"), Schema: map[string]any{"type": "object"}, + Tags: []string{"billing"}, CustomParameters: map[string]any{"timeout": float64(5)}, + }} + named := syncdomain.Attachment{Kind: syncdomain.AttachmentSkill, Version: 2, Skill: &syncdomain.Skill{ + Key: "incident-triage", Name: "On-call triage", Description: "Sort incidents", Markdown: "Sort by severity.\n", + }} + tests := map[string]struct { + input Input + id syncdomain.ResourceID + existing syncdomain.Attachment + want bool + }{ + "tool without --description": { + input: Input{JSONSchema: pointer(`{"type": "object"}`)}, id: refundID, existing: tagged, want: true, + }, + "tool with the same description": { + input: Input{JSONSchema: pointer(`{"type":"object"}`), Description: pointer("Issue a refund")}, + id: refundID, existing: tagged, want: true, + }, + "tool with another description": { + input: Input{JSONSchema: pointer(`{"type": "object"}`), Description: pointer("Refund")}, id: refundID, existing: tagged, + }, + "tool with an empty description": { + input: Input{JSONSchema: pointer(`{"type": "object"}`), Description: pointer("")}, id: refundID, existing: tagged, + }, + "tool with another schema": { + input: Input{JSONSchema: pointer(`{"type": "object", "required": ["orderId"]}`)}, id: refundID, existing: tagged, + }, + "skill with another name": { + input: Input{Content: pointer("Sort by severity."), Name: pointer("Triage"), Description: pointer("Sort incidents")}, + id: triageID, existing: named, want: true, + }, + "skill with another description": { + input: Input{Content: pointer("Sort by severity."), Description: pointer("")}, id: triageID, existing: named, + }, + "skill with other Markdown": { + input: Input{Content: pointer("Sort by age.")}, id: triageID, existing: named, + }, + "other kind": { + input: Input{Content: pointer("Sort by severity.")}, id: triageID, existing: tagged, + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + created, err := test.input.Attachment(test.id) + require.NoError(t, err) + + assert.Equal(t, test.want, test.input.Matches(created, test.existing)) + }) + } +} + +func TestTargetWithoutQuestionsNamesEveryMissingFlag(t *testing.T) { + _, _, err := Target(syncdomain.KindTool, "", "", Questions{}) + require.EqualError(t, err, "tool creation requires --project and --key without interactive input") + + _, _, err = Target(syncdomain.KindSkill, "demo", "", Questions{}) + require.EqualError(t, err, "skill creation requires --key without interactive input") + + id, canceled, err := Target(syncdomain.KindTool, "demo", "refund", Questions{}) + require.NoError(t, err) + assert.False(t, canceled) + assert.Equal(t, refundID, id) +} + +func TestAskWithoutQuestionsKeepsTheInput(t *testing.T) { + input := Input{Content: pointer("Sort by severity.")} + + asked, canceled, err := input.Ask(triageID, Questions{}) + + require.NoError(t, err) + assert.False(t, canceled) + assert.Equal(t, input, asked, "the name comes from the key, and the description is empty") +} + +func TestKeyValidatorAppliesTheCreateRules(t *testing.T) { + require.EqualError(t, keyValidator(syncdomain.KindTool)(""), "tool key must not be empty") + require.EqualError(t, keyValidator(syncdomain.KindSkill)("a/b"), "skill key must be a single path segment") + require.EqualError(t, keyValidator(syncdomain.KindSkill)("Triage"), + `skill key "Triage" must use only lowercase letters, digits, and hyphens, and start with a letter or digit`) + require.NoError(t, keyValidator(syncdomain.KindTool)("Refund_v2")) +} + +func TestRequiredValueRejectsBlankInput(t *testing.T) { + require.Error(t, RequiredValue("variation name")(" ")) + require.NoError(t, RequiredValue("variation name")("Custom name")) +} + +func TestJoinFlags(t *testing.T) { + assert.Equal(t, "--key", joinFlags([]string{"--key"})) + assert.Equal(t, "--project and --key", joinFlags([]string{"--project", "--key"})) + assert.Equal(t, "--project, --config-key, and --key", joinFlags([]string{"--project", "--config-key", "--key"})) +} + +func pointer[T any](value T) *T { + return &value +} diff --git a/internal/sync/link/link.go b/internal/sync/link/link.go index 6692aa29..28cf904d 100644 --- a/internal/sync/link/link.go +++ b/internal/sync/link/link.go @@ -18,6 +18,7 @@ import ( syncdomain "github.com/launchdarkly/ldcli/internal/sync" syncapi "github.com/launchdarkly/ldcli/internal/sync/api" syncconsole "github.com/launchdarkly/ldcli/internal/sync/console" + synccreate "github.com/launchdarkly/ldcli/internal/sync/create" syncinteractive "github.com/launchdarkly/ldcli/internal/sync/interactive" synclocal "github.com/launchdarkly/ldcli/internal/sync/local" syncreference "github.com/launchdarkly/ldcli/internal/sync/reference" @@ -213,11 +214,11 @@ func promptForSelection(options Options, prompt linkedPrompt) (Selection, string } if name == "" { name = syncdomain.DisplayName(key) - fields = append(fields, huh.NewInput().Title("Variation name").Value(&name).Validate(requiredValue("variation name"))) + fields = append(fields, huh.NewInput().Title("Variation name").Value(&name).Validate(synccreate.RequiredValue("variation name"))) } if len(prompt.parsed.Messages) == 0 { fields = append(fields, huh.NewText().Title("Prompt content").Lines(8).Value(&content). - Validate(requiredValue("prompt content"))) + Validate(synccreate.RequiredValue("prompt content"))) } if len(fields) != 0 { if canceled, err := syncinteractive.RunForm(options.Input, options.Output, fields...); err != nil || canceled { @@ -355,13 +356,3 @@ func validateDerivedKey(key string) error { } return nil } - -// requiredValue returns a form validator that rejects a blank value. -func requiredValue(label string) func(string) error { - return func(value string) error { - if strings.TrimSpace(value) == "" { - return fmt.Errorf("%s is required", label) - } - return nil - } -} diff --git a/internal/sync/link/link_test.go b/internal/sync/link/link_test.go index 906e55bd..6705904d 100644 --- a/internal/sync/link/link_test.go +++ b/internal/sync/link/link_test.go @@ -249,11 +249,6 @@ func (catalog *missingProjectCatalog) SearchConfigs( return syncapi.Page[syncapi.Config]{}, errors.New(`{"code":"not_found","statusCode":404}`) } -func TestRequiredValueRejectsBlankInput(t *testing.T) { - require.Error(t, requiredValue("variation name")(" ")) - require.NoError(t, requiredValue("variation name")("Custom name")) -} - type fakeCatalog struct { config syncapi.Config model syncapi.ModelConfig diff --git a/internal/sync/local/attachment.go b/internal/sync/local/attachment.go index 96ae98c0..7521ecd8 100644 --- a/internal/sync/local/attachment.go +++ b/internal/sync/local/attachment.go @@ -222,10 +222,16 @@ 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: - // LaunchDarkly owns the name of a skill, so the file does not get it. - // A pull keeps the name of an existing file. + // LaunchDarkly owns the name of a skill, so a file from LaunchDarkly + // does not get it, and a pull keeps the name of an existing file. A + // file with upsert can still create the skill, so it keeps the name + // that the create sends. skill := *attachment.Skill - return renderSkillFile(skillFrontMatter{Key: skill.Key, Description: &skill.Description, Upsert: attachment.Upsert}, skill.Markdown) + metadata := skillFrontMatter{Key: skill.Key, Description: &skill.Description, Upsert: attachment.Upsert} + if attachment.Upsert { + metadata.Name = skill.Name + } + return renderSkillFile(metadata, skill.Markdown) default: return nil, fmt.Errorf("%s %q has no content", attachment.Kind, attachment.Key()) } diff --git a/internal/sync/prompt/acceptance_test.go b/internal/sync/prompt/acceptance_test.go index f0b4dda9..539c6eef 100644 --- a/internal/sync/prompt/acceptance_test.go +++ b/internal/sync/prompt/acceptance_test.go @@ -37,6 +37,13 @@ type directAPI struct { skills map[string]versionedSkill manifest *syncapi.SyncManifest requests []string + // createErr fails each create of a tool or a skill after onCreate runs. + // onCreate can add the resource, as another working copy does. + createErr error + onCreate func() + // onRequest runs before each request, so that it can change LaunchDarkly + // between two reads, as another working copy does. + onRequest func(method, path string) } type notFoundAPI struct{} @@ -79,9 +86,19 @@ func (api *directAPI) MakeRequest( _ bool, ) ([]byte, error) { api.requests = append(api.requests, method+" "+path) + if api.onRequest != nil { + api.onRequest(method, path) + } if strings.HasSuffix(path, "/configs/sync/manifests") { return handleManifestRequest(api.manifest, method, query, body) } + isCreate := method == http.MethodPost && (strings.HasSuffix(path, "/ai-tools") || strings.HasSuffix(path, "/ai-configs/skills")) + if isCreate && api.createErr != nil { + if api.onCreate != nil { + api.onCreate() + } + return nil, api.createErr + } if strings.HasSuffix(path, "/ai-tools") && method == http.MethodPost { var tool versionedTool if err := json.Unmarshal(body, &tool.Tool); err != nil { @@ -1935,6 +1952,14 @@ func runSyncAction( // runScoped runs options through Runner.Run without a terminal, and returns // the JSON output and the notes. A question fails the run. func runScoped(t *testing.T, root string, api *directAPI, options syncprompt.Options) (string, string, error) { + t.Helper() + options.NoInput, options.Input = true, failingReader{} + return runWith(t, root, api, syncprompt.NewRunner(api), options) +} + +// runWith runs options through runner with JSON output, and returns the +// output and the notes. +func runWith(t *testing.T, root string, api *directAPI, runner syncprompt.Runner, options syncprompt.Options) (string, string, error) { t.Helper() if manifestsByRoot[root] == nil { manifestsByRoot[root] = &syncapi.SyncManifest{Source: "git:example/repo", Items: []syncapi.SyncManifestResource{}} @@ -1942,9 +1967,8 @@ func runScoped(t *testing.T, root string, api *directAPI, options syncprompt.Opt api.manifest = manifestsByRoot[root] var output, notes bytes.Buffer options.WorkingDirectory, options.AccessToken, options.BaseURI = root, "token", "https://example.test" - options.OutputKind, options.NoInput, options.Input = "json", true, failingReader{} - options.Output, options.ErrorOutput = &output, ¬es - err := syncprompt.NewRunner(api).Run(options) + options.OutputKind, options.Output, options.ErrorOutput = "json", &output, ¬es + err := runner.Run(options) return output.String(), notes.String(), err } diff --git a/internal/sync/prompt/action.go b/internal/sync/prompt/action.go index edb7e63e..1d58e69b 100644 --- a/internal/sync/prompt/action.go +++ b/internal/sync/prompt/action.go @@ -7,6 +7,7 @@ import ( "io" syncdomain "github.com/launchdarkly/ldcli/internal/sync" + synccreate "github.com/launchdarkly/ldcli/internal/sync/create" synclink "github.com/launchdarkly/ldcli/internal/sync/link" syncreference "github.com/launchdarkly/ldcli/internal/sync/reference" ) @@ -31,6 +32,10 @@ type AddAction struct { Resources []syncdomain.ResourceID Filter syncdomain.Filter DryRun bool + // Create is the content of one tool or skill that add creates when + // LaunchDarkly does not have it. A nil Create adds only resources that + // LaunchDarkly has. + Create *synccreate.Input } // AttachAction adds a tool or a skill to a local variation, and then syncs. @@ -68,7 +73,20 @@ func (action SyncAction) validate(scope syncdomain.Scope) error { } func (action AddAction) validate(scope syncdomain.Scope) error { - return validateSelections(action.Resources, action.Filter, scope.KeyKind()) + kind := scope.KeyKind() + if err := validateSelections(action.Resources, action.Filter, kind); err != nil { + return err + } + if action.Create == nil { + return nil + } + if !syncdomain.AttachmentKind(kind).Valid() { + return errors.New("add creates only tools and skills") + } + if len(selections(action.Resources, action.Filter, kind)) > 1 { + return fmt.Errorf("add creates one %s at a time. Pass at most one --key.", kind) + } + return action.Create.Check(syncdomain.AttachmentKind(kind)) } func (action DetachAction) validate(scope syncdomain.Scope) error { diff --git a/internal/sync/prompt/attachment_versions.go b/internal/sync/prompt/attachment_versions.go index 6cc10620..0bebd829 100644 --- a/internal/sync/prompt/attachment_versions.go +++ b/internal/sync/prompt/attachment_versions.go @@ -97,14 +97,16 @@ func (resolver *attachmentResolver) settledAttachments( // version of each attachment. func (resolver *attachmentResolver) resolveVariation(projectKey string, variation syncdomain.Variation) (syncdomain.Variation, error) { return variation.PinAttachments(func(local syncdomain.Attachment) (int, error) { - published, err := resolver.resolve(projectKey, local) + published, err := resolver.resolve(projectKey, local, false) return published.Version, err }) } // resolve returns the LaunchDarkly attachment that has the local content. If -// the content differs, resolve writes it and reads the new version. -func (resolver *attachmentResolver) resolve(projectKey string, local syncdomain.Attachment) (syncdomain.Attachment, error) { +// the content differs, resolve writes it and reads the new version. create is +// true when the plan found no copy in LaunchDarkly, and then resolve never +// updates a copy with other content. +func (resolver *attachmentResolver) resolve(projectKey string, local syncdomain.Attachment, create bool) (syncdomain.Attachment, error) { id := local.ID(projectKey) if err, failed := resolver.failures[id]; failed { return syncdomain.Attachment{}, err @@ -113,7 +115,7 @@ func (resolver *attachmentResolver) resolve(projectKey string, local syncdomain. return attachment, nil } - attachment, err := resolver.publish(projectKey, local) + attachment, err := resolver.publish(projectKey, local, create) if err != nil { resolver.failures[id] = err return syncdomain.Attachment{}, err @@ -122,12 +124,15 @@ func (resolver *attachmentResolver) resolve(projectKey string, local syncdomain. return attachment, nil } -func (resolver *attachmentResolver) publish(projectKey string, local syncdomain.Attachment) (syncdomain.Attachment, error) { +func (resolver *attachmentResolver) publish(projectKey string, local syncdomain.Attachment, create bool) (syncdomain.Attachment, error) { remote, readErr := resolver.client.ReadAttachment(projectKey, local.Kind, local.Key()) var writeErr error switch { case readErr == nil && syncdomain.SameAttachmentContent(local, remote): return remote, nil + case readErr == nil && create: + // Another working copy created it after the plan. + return syncdomain.Attachment{}, fmt.Errorf("%s %q changed concurrently", local.Kind, local.Key()) case readErr == nil: writeErr = resolver.client.UpdateAttachment(projectKey, local) case syncapi.IsNotFound(readErr) && local.Upsert: diff --git a/internal/sync/prompt/create.go b/internal/sync/prompt/create.go new file mode 100644 index 00000000..f768a39b --- /dev/null +++ b/internal/sync/prompt/create.go @@ -0,0 +1,179 @@ +package prompt + +import ( + "errors" + "fmt" + + syncdomain "github.com/launchdarkly/ldcli/internal/sync" + syncapi "github.com/launchdarkly/ldcli/internal/sync/api" + syncbootstrap "github.com/launchdarkly/ldcli/internal/sync/bootstrap" + syncconsole "github.com/launchdarkly/ldcli/internal/sync/console" + synccreate "github.com/launchdarkly/ldcli/internal/sync/create" + synclocal "github.com/launchdarkly/ldcli/internal/sync/local" +) + +// runCreate creates one tool or skill from the create flags of add. Add never +// changes a resource that LaunchDarkly has. It starts syncing one that has the +// content of the flags, and refuses one with other content. +func (runner Runner) runCreate(options Options, workspace syncWorkspace, action AddAction) error { + client := runner.api(options) + questions := synccreate.Questions{ + // The forms write to the output stream, so check that stream. + Allowed: !options.NoInput && runner.isTerminal(options.Input, options.Output), + Input: options.Input, + Output: options.Output, + Projects: client, + } + var key string + if len(action.Filter.Keys) == 1 { + key = action.Filter.Keys[0] + } + id, canceled, err := synccreate.Target(options.Scope.KeyKind(), action.Filter.Project, key, questions) + if err != nil || canceled { + return err + } + + // The values of the flags must pass the create rules before the first read. + input := *action.Create + created, err := input.Attachment(id) + if err != nil { + return err + } + if err := syncbootstrap.CheckNotSynced(workspace.local, []ResourceID{id}); err != nil { + return err + } + existing, err := client.ReadAttachment(id.ProjectKey, created.Kind, id.LookupKey) + switch { + case err == nil: + return runner.adoptAttachment(options, workspace, action, id, created, existing) + case !syncapi.IsNotFound(err): + return err + } + + // Only a skill that add creates needs a name and a description. + if input, canceled, err = input.Ask(id, questions); err != nil || canceled { + return err + } + if created, err = input.Attachment(id); err != nil { + return err + } + return runner.createAttachment(options, workspace, client, syncdomain.SyncedAttachment{ProjectKey: id.ProjectKey, Attachment: created}) +} + +// adoptAttachment starts syncing a tool or skill that LaunchDarkly already +// has, with the add of a resource that exists. It refuses when a flag that the +// user passed differs from LaunchDarkly. +func (runner Runner) adoptAttachment( + options Options, + workspace syncWorkspace, + action AddAction, + id ResourceID, + created, existing syncdomain.Attachment, +) error { + if !action.Create.Matches(created, existing) { + return fmt.Errorf("%s already exists in LaunchDarkly with different content, and add does not change it. "+ + "To sync the LaunchDarkly version, run %s, then edit the file and run ldcli sync %ss or ldcli sync all.", id.Label(), addCommand(id), id.Kind) + } + adopt := AddAction{Filter: syncdomain.Filter{Project: id.ProjectKey, Keys: []string{id.LookupKey}}, DryRun: action.DryRun} + if err := runner.runAdd(options, workspace, adopt); err != nil { + return err + } + + notes := syncconsole.New(options.ErrorOutput) + started := "started" + if action.DryRun { + started = "will start" + } + _ = notes.Printf("Note: %s already exists in LaunchDarkly with this content, so add %s syncing it.\n", id.Label(), started) + if name := action.Create.Name; name != nil && existing.Skill != nil && *name != existing.Skill.Name { + _ = notes.Printf("Note: %s keeps its LaunchDarkly name %q.\n", id.Label(), existing.Skill.Name) + } + return nil +} + +// createAttachment creates a tool or skill that LaunchDarkly does not have. It +// plans the create in memory and asks before it writes the file, so that a +// cancel leaves nothing and a watch cannot create the resource before the +// answer. +func (runner Runner) createAttachment( + options Options, + workspace syncWorkspace, + client syncapi.Client, + created syncdomain.SyncedAttachment, +) error { + path, err := synclocal.AttachmentPath(created.ID()) + if err != nil { + return err + } + reviewed := reviewedCreate(options.Scope, created) + if options.dryRun() { + if err := writePlanOutput(options.Output, options.OutputKind, reviewed.plan, options.Diff); err != nil { + return err + } + return reviewed.plan.BlockingError() + } + proceed, err := reviewAndConfirmPlan(options, reviewed.plan, runner.interactive(options)) + if err != nil || !proceed { + return err + } + + // A file can appear while the question waits. + if err := syncbootstrap.CheckNotSynced(workspace.local, []ResourceID{created.ID()}); err != nil { + return err + } + written, err := workspace.local.ReplaceAttachments([]syncdomain.SyncedAttachment{created}) + if err != nil { + return err + } + return runner.applyCreate(options, workspace, client, reviewed, path, synclocal.Creation{CreatedPaths: written}) +} + +// reviewedCreate is the state that the user reviews before a create: the +// create row of the new resource, with no file yet. +func reviewedCreate(scope syncdomain.Scope, created syncdomain.SyncedAttachment) workspaceState { + id := created.ID() + row := planAttachment(id, &created.Attachment, nil, "", false) + return workspaceState{projectKeys: []string{id.ProjectKey}, plan: Plan{Scope: scope, Resources: []PlannedResource{row}}} +} + +// applyCreate applies the reviewed create after it wrote the new file at path. +// The apply is the normal sync of the new resource only, so that other pending +// changes stay as they are. If the state changed after the review, or if +// LaunchDarkly rejects the create, applyCreate removes the file. After a write +// with no response, it keeps the file, so that the next sync creates the +// resource or finds it. +func (runner Runner) applyCreate( + options Options, + workspace syncWorkspace, + client syncapi.Client, + reviewed workspaceState, + path string, + creation synclocal.Creation, +) error { + notes := syncconsole.New(options.ErrorOutput) + remove := func(cause error) error { + if err := workspace.local.RollbackCreation(creation); err != nil { + return errors.Join(cause, err) + } + _ = notes.Printf("Note: Removed %s.\n", path) + return cause + } + + id := reviewed.plan.Resources[0].ID + current, err := workspace.loadState(client, options.Scope, syncdomain.Filter{Project: id.ProjectKey, Keys: []string{id.LookupKey}}) + if err == nil { + err = checkUnchanged(options, reviewed, current) + } + if err != nil { + return remove(err) + } + + outcomes, err := runner.applyPlan(options, workspace, client, current, nil) + switch { + case err == nil: + _ = notes.Printf("Note: Created %s.\n", path) + case len(outcomes) == 1 && outcomes[0].Status == OutcomeFailed && !syncapi.MutationMayHaveSucceeded(err): + return remove(err) + } + return err +} diff --git a/internal/sync/prompt/create_test.go b/internal/sync/prompt/create_test.go new file mode 100644 index 00000000..31f53747 --- /dev/null +++ b/internal/sync/prompt/create_test.go @@ -0,0 +1,524 @@ +package prompt_test + +import ( + "errors" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + syncdomain "github.com/launchdarkly/ldcli/internal/sync" + synccreate "github.com/launchdarkly/ldcli/internal/sync/create" + syncprompt "github.com/launchdarkly/ldcli/internal/sync/prompt" +) + +var refundID = syncdomain.ResourceID{Kind: syncdomain.KindTool, ProjectKey: "production", LookupKey: "refund"} + +const refundCreated = `{"resources": [{"resourceKind": "tool", "projectKey": "production", "lookupKey": "refund", + "action": "create_server", "status": "succeeded", "version": 1}]}` + +func TestCreateMakesAToolFromFlags(t *testing.T) { + root := initRepository(t) + api := &directAPI{} + action := createAction("refund", synccreate.Input{ + JSONSchema: pointer(`{"type":"object","properties":{"orderId":{"type":"string"}}}`), + Description: pointer("Issue a refund"), + }) + + stdout, notes, err := runScoped(t, root, api, syncprompt.Options{Scope: syncdomain.ScopeTools, Action: action, Yes: true}) + + require.NoError(t, err) + assert.JSONEq(t, refundCreated, stdout) + assert.Equal(t, "Note: Created .launchdarkly/production/tools/refund.json.\n", notes) + assert.Equal(t, `{ + "formatVersion": 1, + "upsert": true, + "key": "refund", + "description": "Issue a refund", + "schema": { + "properties": { + "orderId": { + "type": "string" + } + }, + "type": "object" + } +} +`, readLocalFile(t, root, "tools/refund.json")) + assert.Equal(t, pointer("Issue a refund"), api.tools["refund"].Description) + assert.Equal(t, []syncdomain.ResourceID{refundID}, lockResourceIDs(readLock(t, root))) + + stdout, err = runSyncAction(t, root, api, syncdomain.ScopeTools, syncprompt.SyncAction{DryRun: true}, false) + require.NoError(t, err) + assert.Equal(t, []string{"tool refund in_sync"}, planRows(t, stdout)) +} + +func TestCreateMakesASkillFromFlags(t *testing.T) { + tests := map[string]struct { + name *string + wantName string + }{ + "name from the key": {wantName: "Incident triage"}, + "name from --name": {name: pointer("Triage"), wantName: "Triage"}, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + root := initRepository(t) + api := &directAPI{} + // "$(cat file)" drops the final newline of the file. + action := createAction("incident-triage", synccreate.Input{ + Content: pointer("Sort incidents by severity."), Name: test.name, Description: pointer("Sort incidents"), + }) + + stdout, notes, err := runScoped(t, root, api, syncprompt.Options{Scope: syncdomain.ScopeSkills, Action: action, Yes: true}) + + require.NoError(t, err) + assert.JSONEq(t, `{"resources": [{"resourceKind": "skill", "projectKey": "production", + "lookupKey": "incident-triage", "action": "create_server", "status": "succeeded", "version": 1}]}`, stdout) + assert.Equal(t, "Note: Created .launchdarkly/production/skills/incident-triage.md.\n", notes) + assert.Equal(t, "---\nkey: incident-triage\ndescription: Sort incidents\nname: "+test.wantName+"\nupsert: true\n---\n\n"+ + "Sort incidents by severity.\n", readLocalFile(t, root, "skills/incident-triage.md")) + assert.Equal(t, syncdomain.Skill{ + Key: "incident-triage", Name: test.wantName, Description: "Sort incidents", Markdown: "Sort incidents by severity.\n", + }, api.skills["incident-triage"].Skill) + + stdout, err = runSyncAction(t, root, api, syncdomain.ScopeSkills, syncprompt.SyncAction{DryRun: true}, false) + require.NoError(t, err) + assert.Equal(t, []string{"skill incident-triage in_sync"}, planRows(t, stdout)) + }) + } +} + +func TestCreateLeavesOtherPendingChangesAsTheyAre(t *testing.T) { + // The workspace has a tool file with upsert that LaunchDarkly does not + // have, and a local edit of a synced tool. + root, api := writeStandaloneAttachmentWorkspace(t) + writeLocalFile(t, root, "tools/search.json", `{"formatVersion": 1, "key": "search", "description": "Local", "schema": {"type": "object"}}`) + before := lockResourceIDs(readLock(t, root)) + issueCredit := syncdomain.ResourceID{Kind: syncdomain.KindTool, ProjectKey: "production", LookupKey: "issue-credit"} + + _, _, err := runScoped(t, root, api, syncprompt.Options{ + Scope: syncdomain.ScopeTools, Action: createAction("issue-credit", synccreate.Input{JSONSchema: pointer(`{"type": "object"}`)}), Yes: true, + }) + + require.NoError(t, err) + assert.Contains(t, api.tools, "issue-credit") + assert.NotContains(t, api.tools, "refund", "the other new tool waits for a sync") + assert.Equal(t, 1, countRequests(api.requests, "POST", "/ai-tools")) + assert.Zero(t, countRequests(api.requests, "PATCH", "/ai-tools/search")) + assert.ElementsMatch(t, append(before, issueCredit), lockResourceIDs(readLock(t, root))) +} + +func TestCreateAdoptsATaggedToolWithTheSameSchema(t *testing.T) { + root := initRepository(t) + api := &directAPI{tools: map[string]versionedTool{"refund": {Tool: syncdomain.Tool{ + Key: "refund", Description: pointer("Issue a refund"), Schema: map[string]any{"type": "object"}, Tags: []string{"billing"}, + }, Version: 4}}} + action := createAction("refund", synccreate.Input{JSONSchema: pointer(`{"type": "object"}`)}) + + stdout, notes, err := runScoped(t, root, api, syncprompt.Options{Scope: syncdomain.ScopeTools, Action: action, Yes: true}) + + require.NoError(t, err) + assert.Equal(t, "Bootstrapped 1 tool file in .launchdarkly.\n", stdout) + assert.Equal(t, "Note: tool production/refund already exists in LaunchDarkly with this content, so add started syncing it.\n", notes) + requireOnlyReads(t, api.requests) + file := readLocalFile(t, root, "tools/refund.json") + assert.Contains(t, file, `"billing"`, "the file takes the fields without a flag from LaunchDarkly") + assert.NotContains(t, file, "upsert") + assert.Equal(t, []syncdomain.ResourceID{refundID}, lockResourceIDs(readLock(t, root))) +} + +func TestCreateAdoptsASkillAndKeepsItsLaunchDarklyName(t *testing.T) { + root := initRepository(t) + api := &directAPI{skills: map[string]versionedSkill{"triage": {Skill: syncdomain.Skill{ + Key: "triage", Name: "Triage", Description: "Triage a ticket", Markdown: "Pick a queue.\n", + }, Version: 3}}} + action := createAction("triage", synccreate.Input{ + Content: pointer("Pick a queue."), Name: pointer("Queue triage"), Description: pointer("Triage a ticket"), + }) + + _, notes, err := runScoped(t, root, api, syncprompt.Options{Scope: syncdomain.ScopeSkills, Action: action, Yes: true}) + + require.NoError(t, err) + assert.Equal(t, "Note: skill production/triage already exists in LaunchDarkly with this content, so add started syncing it.\n"+ + "Note: skill production/triage keeps its LaunchDarkly name \"Triage\".\n", notes) + requireOnlyReads(t, api.requests) + assert.Equal(t, "---\nkey: triage\ndescription: Triage a ticket\n---\n\nPick a queue.\n", readLocalFile(t, root, "skills/triage.md")) +} + +func TestCreateRefusesALaunchDarklyResourceWithOtherContent(t *testing.T) { + refusal := "%s already exists in LaunchDarkly with different content, and add does not change it. " + + "To sync the LaunchDarkly version, run ldcli sync %ss add --project production --key %s, then edit the file and run ldcli sync %ss or ldcli sync all." + tests := map[string]struct { + scope syncdomain.Scope + action syncprompt.AddAction + want string + }{ + "tool schema": { + scope: syncdomain.ScopeTools, + action: createAction("lookup", synccreate.Input{JSONSchema: pointer(`{"type": "object", "required": ["id"]}`)}), + want: fmt.Sprintf(refusal, "tool production/lookup", "tool", "lookup", "tool"), + }, + "tool description": { + scope: syncdomain.ScopeTools, + action: createAction("lookup", synccreate.Input{JSONSchema: pointer(`{"type": "object"}`), Description: pointer("Look up")}), + want: fmt.Sprintf(refusal, "tool production/lookup", "tool", "lookup", "tool"), + }, + "skill Markdown": { + scope: syncdomain.ScopeSkills, + action: createAction("triage", synccreate.Input{Content: pointer("Pick a team.")}), + want: fmt.Sprintf(refusal, "skill production/triage", "skill", "triage", "skill"), + }, + "skill description": { + scope: syncdomain.ScopeSkills, + action: createAction("triage", synccreate.Input{Content: pointer("Pick a queue."), Description: pointer("")}), + want: fmt.Sprintf(refusal, "skill production/triage", "skill", "triage", "skill"), + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + root := initRepository(t) + api := &directAPI{ + tools: map[string]versionedTool{ + "lookup": {Tool: syncdomain.Tool{Key: "lookup", Schema: map[string]any{"type": "object"}}, Version: 1}, + }, + skills: map[string]versionedSkill{"triage": {Skill: syncdomain.Skill{ + Key: "triage", Name: "Triage", Description: "Triage a ticket", Markdown: "Pick a queue.\n", + }, Version: 1}}, + } + + _, _, err := runScoped(t, root, api, syncprompt.Options{Scope: test.scope, Action: test.action, Yes: true}) + + require.EqualError(t, err, test.want) + requireOnlyReads(t, api.requests) + assert.NoDirExists(t, filepath.Join(root, syncdomain.RootDir)) + }) + } +} + +func TestAddWithoutAContentFlagNeverCreates(t *testing.T) { + root := initRepository(t) + api := &directAPI{} + action := syncprompt.AddAction{Filter: syncdomain.Filter{Project: "production", Keys: []string{"refund"}}} + + _, _, err := runScoped(t, root, api, syncprompt.Options{Scope: syncdomain.ScopeTools, Action: action, Yes: true}) + + require.EqualError(t, err, "tool production/refund does not exist in LaunchDarkly.") + requireOnlyReads(t, api.requests) + assert.NoDirExists(t, filepath.Join(root, syncdomain.RootDir)) +} + +func TestCreateChecksTheFlagsBeforeAnyRequest(t *testing.T) { + tool := func(input synccreate.Input) syncprompt.AddAction { return createAction("refund", input) } + tests := map[string]struct { + scope syncdomain.Scope + action syncprompt.AddAction + want string + }{ + "schema that is not an object": { + scope: syncdomain.ScopeTools, action: tool(synccreate.Input{JSONSchema: pointer(`[]`)}), + want: "--json-schema must be one JSON object", + }, + "pasted tool file": { + scope: syncdomain.ScopeTools, action: tool(synccreate.Input{JSONSchema: pointer(`{"formatVersion": 1, "schema": {}}`)}), + want: `--json-schema is the input schema only. Pass the value of "inputSchema", and pass the description with --description.`, + }, + "description without a schema": { + scope: syncdomain.ScopeTools, action: tool(synccreate.Input{Description: pointer("Issue a refund")}), + want: "--description needs --json-schema. add creates a tool only when --json-schema is set.", + }, + "long description": { + scope: syncdomain.ScopeTools, + action: tool(synccreate.Input{JSONSchema: pointer(`{}`), Description: pointer(strings.Repeat("d", 1025))}), + want: "tool description must be at most 1024 characters", + }, + "two keys": { + scope: syncdomain.ScopeTools, + action: syncprompt.AddAction{ + Filter: syncdomain.Filter{Project: "production", Keys: []string{"refund", "credit"}}, + Create: &synccreate.Input{JSONSchema: pointer(`{}`)}, + }, + want: "add creates one tool at a time. Pass at most one --key.", + }, + "blank Markdown": { + scope: syncdomain.ScopeSkills, action: createAction("handoff", synccreate.Input{Content: pointer(" \n")}), + want: "--content cannot be blank", + }, + "skill key with uppercase letters": { + scope: syncdomain.ScopeSkills, action: createAction("Handoff", synccreate.Input{Content: pointer("Ask for a person.")}), + want: `skill key "Handoff" must use only lowercase letters, digits, and hyphens, and start with a letter or digit`, + }, + "no key": { + scope: syncdomain.ScopeTools, + action: syncprompt.AddAction{ + Filter: syncdomain.Filter{Project: "production"}, Create: &synccreate.Input{JSONSchema: pointer(`{}`)}, + }, + want: "tool creation requires --key without interactive input", + }, + "no project and no key": { + scope: syncdomain.ScopeSkills, + action: syncprompt.AddAction{Create: &synccreate.Input{Content: pointer("Ask for a person.")}}, + want: "skill creation requires --project and --key without interactive input", + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + root := initRepository(t) + api := &directAPI{} + + _, _, err := runScoped(t, root, api, syncprompt.Options{Scope: test.scope, Action: test.action, Yes: true}) + + require.EqualError(t, err, test.want) + assert.Empty(t, api.requests) + assert.NoDirExists(t, filepath.Join(root, syncdomain.RootDir)) + }) + } +} + +// The refusals name ldcli sync tools and ldcli sync all, and each of them +// restores a standalone file that only sync.lock has. +func TestCreateRefusesAResourceThatIsAlreadySynced(t *testing.T) { + tool := synccreate.Input{JSONSchema: pointer(`{"type": "object"}`)} + for _, scope := range []syncdomain.Scope{syncdomain.ScopeTools, syncdomain.ScopeAll} { + root, api := writeStandaloneAttachmentWorkspace(t) + _, _, err := runScoped(t, root, api, syncprompt.Options{Scope: syncdomain.ScopeAll, Action: syncprompt.SyncAction{}, Yes: true}) + require.NoError(t, err) + api.requests = nil + lookupFile := filepath.Join(root, syncdomain.RootDir, "production", "tools", "lookup.json") + + _, _, err = runScoped(t, root, api, syncprompt.Options{Scope: syncdomain.ScopeTools, Action: createAction("lookup", tool), Yes: true}) + require.EqualError(t, err, "tool production/lookup is already synced. "+ + "Edit .launchdarkly/production/tools/lookup.json and run ldcli sync tools or ldcli sync all.") + + require.NoError(t, os.Remove(lookupFile)) + _, _, err = runScoped(t, root, api, syncprompt.Options{Scope: syncdomain.ScopeTools, Action: createAction("lookup", tool), Yes: true}) + require.EqualError(t, err, "tool production/lookup has a sync.lock entry but no local file. "+ + "Run ldcli sync tools or ldcli sync all to restore it.") + assert.Empty(t, api.requests) + + _, _, err = runScoped(t, root, api, syncprompt.Options{Scope: scope, Action: syncprompt.SyncAction{}, Yes: true}) + require.NoError(t, err) + assert.FileExists(t, lookupFile, "ldcli sync %s restores the file", scope) + } +} + +func TestCreateDryRunPlansTheCreateAndWritesNothing(t *testing.T) { + root := initRepository(t) + api := &directAPI{} + action := createAction("refund", synccreate.Input{JSONSchema: pointer(`{"type": "object"}`)}) + action.DryRun = true + + stdout, _, err := runScoped(t, root, api, syncprompt.Options{Scope: syncdomain.ScopeTools, Action: action}) + + require.NoError(t, err) + assert.Equal(t, []string{"tool refund create_server"}, planRows(t, stdout)) + assert.Contains(t, stdout, `"schema"`, "the plan shows the content of the new tool") + assert.Equal(t, []string{"GET https://example.test/api/v2/projects/production/ai-tools/refund"}, api.requests) + assert.NoDirExists(t, filepath.Join(root, syncdomain.RootDir)) +} + +// A create asks before it writes the file. So a cancel leaves nothing, and a +// watch that runs during the question finds no file to create. +func TestCreateAsksBeforeItWritesTheFile(t *testing.T) { + for _, answer := range []string{"n", "y"} { + t.Run(answer, func(t *testing.T) { + root := initRepository(t) + api := &directAPI{} + path := filepath.Join(root, syncdomain.RootDir, "production", "tools", "refund.json") + input := &answerReader{answer: answer + "\n", path: path} + + _, notes, err := runWith(t, root, api, syncprompt.NewTerminalRunner(api), syncprompt.Options{ + Scope: syncdomain.ScopeTools, Action: createAction("refund", synccreate.Input{JSONSchema: pointer(`{"type": "object"}`)}), + Input: input, + }) + + require.NoError(t, err) + require.True(t, input.answered) + assert.False(t, input.fileAtQuestion, "the file does not exist before the answer") + assert.Contains(t, notes, "1 change: 1 tool create.") + if answer == "n" { + assert.Contains(t, notes, "Sync canceled.") + assert.NoDirExists(t, filepath.Join(root, syncdomain.RootDir)) + assert.Zero(t, countRequests(api.requests, "POST", "/ai-tools")) + return + } + assert.FileExists(t, path) + assert.Equal(t, 1, countRequests(api.requests, "POST", "/ai-tools")) + }) + } +} + +func TestCreateWithoutATerminalNeedsYes(t *testing.T) { + root := initRepository(t) + api := &directAPI{} + + _, _, err := runScoped(t, root, api, syncprompt.Options{ + Scope: syncdomain.ScopeTools, Action: createAction("refund", synccreate.Input{JSONSchema: pointer(`{"type": "object"}`)}), + }) + + require.EqualError(t, err, "interactive confirmation requires a terminal; rerun with --yes to apply non-interactively") + assert.NoDirExists(t, filepath.Join(root, syncdomain.RootDir)) + assert.Zero(t, countRequests(api.requests, "POST", "/ai-tools")) +} + +func TestCreateStopsWhenTheStateChangesAfterTheReview(t *testing.T) { + root := initRepository(t) + api := &directAPI{} + input := &answerReader{answer: "y\n", onAnswer: func() { + // Another working copy creates the tool while the question waits. + api.tools = map[string]versionedTool{"refund": {Tool: syncdomain.Tool{Key: "refund", Schema: map[string]any{"type": "string"}}, Version: 1}} + }} + + _, notes, err := runWith(t, root, api, syncprompt.NewTerminalRunner(api), syncprompt.Options{ + Scope: syncdomain.ScopeTools, Action: createAction("refund", synccreate.Input{JSONSchema: pointer(`{"type": "object"}`)}), + Input: input, + }) + + require.EqualError(t, err, "sync state changed after review; run sync again") + assert.Contains(t, notes, "Note: Removed .launchdarkly/production/tools/refund.json.\n") + assert.NoDirExists(t, filepath.Join(root, syncdomain.RootDir)) + assert.Zero(t, countRequests(api.requests, "POST", "/ai-tools")) +} + +func TestCreateRemovesTheFileWhenLaunchDarklyRejectsTheCreate(t *testing.T) { + root := initRepository(t) + api := &directAPI{createErr: errors.New(`{"code":"invalid_request","message":"schema is invalid","statusCode":400}`)} + + stdout, notes, err := runScoped(t, root, api, syncprompt.Options{ + Scope: syncdomain.ScopeTools, Action: createAction("refund", synccreate.Input{JSONSchema: pointer(`{"type": "object"}`)}), Yes: true, + }) + + require.ErrorContains(t, err, "schema is invalid") + assert.Contains(t, stdout, `"status": "failed"`) + assert.Equal(t, "Note: Removed .launchdarkly/production/tools/refund.json.\n", notes) + assert.NoDirExists(t, filepath.Join(root, syncdomain.RootDir)) +} + +func TestCreateKeepsTheFileAfterAWriteWithNoResponse(t *testing.T) { + root := initRepository(t) + api := &directAPI{createErr: errors.New("connection reset by peer")} + action := createAction("refund", synccreate.Input{JSONSchema: pointer(`{"type": "object"}`)}) + + _, notes, err := runScoped(t, root, api, syncprompt.Options{Scope: syncdomain.ScopeTools, Action: action, Yes: true}) + + require.ErrorContains(t, err, "connection reset by peer") + assert.Empty(t, notes) + assert.Contains(t, readLocalFile(t, root, "tools/refund.json"), `"upsert": true`) + assert.Empty(t, lockResourceIDs(readLock(t, root))) + + // The next sync creates the tool from the file that add kept. + api.createErr = nil + stdout, err := runSyncAction(t, root, api, syncdomain.ScopeTools, syncprompt.SyncAction{}, true) + require.NoError(t, err) + assert.JSONEq(t, refundCreated, stdout) +} + +func TestCreateReadsAgainAfterAConflict(t *testing.T) { + root := initRepository(t) + api := &directAPI{createErr: errors.New(`{"code":"conflict","message":"tool exists","statusCode":409}`)} + api.onCreate = func() { + // Another working copy created the same tool first. + api.tools = map[string]versionedTool{"refund": {Tool: syncdomain.Tool{Key: "refund", Schema: map[string]any{"type": "object"}}, Version: 1}} + } + action := createAction("refund", synccreate.Input{JSONSchema: pointer(`{"type": "object"}`)}) + + stdout, notes, err := runScoped(t, root, api, syncprompt.Options{Scope: syncdomain.ScopeTools, Action: action, Yes: true}) + + require.NoError(t, err) + assert.JSONEq(t, refundCreated, stdout) + assert.Equal(t, "Note: Created .launchdarkly/production/tools/refund.json.\n", notes) + assert.Equal(t, []syncdomain.ResourceID{refundID}, lockResourceIDs(readLock(t, root))) +} + +// The plan saw no tool, and another working copy creates one with other +// content just before the read of the publish. Neither add nor sync changes +// that tool. +func TestCreateRowDoesNotUpdateAToolThatAppearsBeforeThePublish(t *testing.T) { + theirs := versionedTool{Tool: syncdomain.Tool{Key: "refund", Description: pointer("Theirs"), Schema: map[string]any{"type": "object"}}, Version: 1} + for name, test := range map[string]struct { + // publishRead is the number of the read of the tool in which publish + // looks before it writes. + publishRead int + run func(root string, api *directAPI) (string, error) + notes string + }{ + "add": { + publishRead: 3, + run: func(root string, api *directAPI) (string, error) { + action := createAction("refund", synccreate.Input{JSONSchema: pointer(`{"type": "object"}`)}) + _, notes, err := runScoped(t, root, api, syncprompt.Options{Scope: syncdomain.ScopeTools, Action: action, Yes: true}) + return notes, err + }, + notes: "Note: Removed .launchdarkly/production/tools/refund.json.\n", + }, + "sync": { + publishRead: 3, + run: func(root string, api *directAPI) (string, error) { + writeLocalFile(t, root, "tools/refund.json", `{"formatVersion": 1, "upsert": true, "key": "refund", "schema": {"type": "object"}}`) + _, notes, err := runScoped(t, root, api, syncprompt.Options{ + Scope: syncdomain.ScopeTools, Action: syncprompt.SyncAction{Filter: syncdomain.Filter{Project: "production", Keys: []string{"refund"}}}, Yes: true, + }) + return notes, err + }, + }, + } { + t.Run(name, func(t *testing.T) { + root := initRepository(t) + api := &directAPI{} + reads := 0 + api.onRequest = func(method, path string) { + if method == http.MethodGet && strings.HasSuffix(path, "/ai-tools/refund") { + if reads++; reads == test.publishRead { + api.tools = map[string]versionedTool{"refund": theirs} + } + } + } + + notes, err := test.run(root, api) + + require.ErrorContains(t, err, `tool "refund" changed concurrently`) + assert.Equal(t, test.publishRead, reads, "the read of the publish is the last read") + assert.Zero(t, countRequests(api.requests, "PATCH", "/ai-tools/refund")) + assert.Zero(t, countRequests(api.requests, "POST", "/ai-tools")) + assert.Equal(t, theirs, api.tools["refund"]) + assert.Equal(t, test.notes, notes) + }) + } +} + +// createAction is the add that creates the tool or the skill key in the +// project "production". +func createAction(key string, input synccreate.Input) syncprompt.AddAction { + return syncprompt.AddAction{Filter: syncdomain.Filter{Project: "production", Keys: []string{key}}, Create: &input} +} + +// answerReader answers one confirmation. When the runner reads the answer, it +// records whether the file at path exists, and then runs onAnswer. +type answerReader struct { + answer string + path string + onAnswer func() + answered bool + fileAtQuestion bool +} + +func (reader *answerReader) Read(buffer []byte) (int, error) { + if reader.answered { + return 0, io.EOF + } + reader.answered = true + if reader.path != "" { + _, err := os.Stat(reader.path) + reader.fileAtQuestion = err == nil + } + if reader.onAnswer != nil { + reader.onAnswer() + } + return copy(buffer, reader.answer), nil +} diff --git a/internal/sync/prompt/execute.go b/internal/sync/prompt/execute.go index 78cca748..2b1aee5f 100644 --- a/internal/sync/prompt/execute.go +++ b/internal/sync/prompt/execute.go @@ -261,10 +261,10 @@ func applyAttachmentChange( case ActionCreateServer, ActionUpdateServer: // A create row exists only with upsert, or after the user chose the // local side of a conflict in which LaunchDarkly has no copy. Both - // allow the create. + // allow the create, but not the update of a copy that appears later. local := *resource.LocalAttachment local.Upsert = local.Upsert || resource.Action == ActionCreateServer - published, err := attachments.resolve(resource.ID.ProjectKey, local) + published, err := attachments.resolve(resource.ID.ProjectKey, local, resource.Action == ActionCreateServer) return published.Version, err case ActionUpdateLocal: server := syncdomain.SyncedAttachment{ProjectKey: resource.ID.ProjectKey, Attachment: *resource.ServerAttachment} diff --git a/internal/sync/prompt/execute_test.go b/internal/sync/prompt/execute_test.go index f8dc5a3e..66865331 100644 --- a/internal/sync/prompt/execute_test.go +++ b/internal/sync/prompt/execute_test.go @@ -327,7 +327,7 @@ func TestPublishReadsAgainAfterACreateConflict(t *testing.T) { transport := &createConflictAPI{remote: test.remote} client := syncapi.NewClient(transport, "token", "https://example.com") - published, err := newAttachmentResolver(client).resolve("project", local) + published, err := newAttachmentResolver(client).resolve("project", local, true) if test.wantError != "" { require.ErrorContains(t, err, test.wantError) @@ -345,7 +345,7 @@ 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) + _, err := newAttachmentResolver(client).resolve("project", skill, false) require.EqualError(t, err, `skill "handoff" does not exist in LaunchDarkly; add upsert: true to its front matter to create it during sync`) } diff --git a/internal/sync/prompt/export_test.go b/internal/sync/prompt/export_test.go new file mode 100644 index 00000000..405e08c9 --- /dev/null +++ b/internal/sync/prompt/export_test.go @@ -0,0 +1,15 @@ +package prompt + +import ( + "io" + + "github.com/launchdarkly/ldcli/internal/resources" +) + +// NewTerminalRunner returns a runner whose streams count as a terminal, so +// that a test can answer its questions. +func NewTerminalRunner(client resources.Client) Runner { + runner := NewRunner(client) + runner.isTerminal = func(io.Reader, io.Writer) bool { return true } + return runner +} diff --git a/internal/sync/prompt/runner.go b/internal/sync/prompt/runner.go index ebec7b2c..4e7437c9 100644 --- a/internal/sync/prompt/runner.go +++ b/internal/sync/prompt/runner.go @@ -146,6 +146,9 @@ func (runner Runner) runSync(options Options, workspace syncWorkspace, action Sy } func (runner Runner) runAdd(options Options, workspace syncWorkspace, action AddAction) error { + if action.Create != nil { + return runner.runCreate(options, workspace, action) + } exists, err := workspace.local.Exists() if err != nil { return err diff --git a/internal/sync/prompt/sync.go b/internal/sync/prompt/sync.go index a7463404..93fb8fea 100644 --- a/internal/sync/prompt/sync.go +++ b/internal/sync/prompt/sync.go @@ -72,7 +72,8 @@ func (runner Runner) runWorkspaceSync(options Options, workspace syncWorkspace, if err := checkUnchanged(options, reviewed, current); err != nil { return err } - return runner.applyPlan(options, workspace, client, current, conflicts.resolutions) + _, err = runner.applyPlan(options, workspace, client, current, conflicts.resolutions) + return err } // checkUnchanged returns an error if the state changed after the review. @@ -90,14 +91,14 @@ func checkUnchanged(options Options, reviewed, current workspaceState) error { } // applyPlan applies the conflict choices to the current plan, executes it, -// and records the new baseline. +// records the new baseline, and returns the outcome of each row. func (runner Runner) applyPlan( options Options, workspace syncWorkspace, client syncapi.Client, current workspaceState, resolutions map[syncdomain.ResourceID]ConflictResolution, -) error { +) ([]ResourceOutcome, error) { plan := applyConflictResolutions(current.plan, resolutions) outcomes, next, err := executePlan(workspace.root, workspace.local, client, current.baseline.Lock, plan, current.localFiles) @@ -112,5 +113,5 @@ func (runner Runner) applyPlan( failures = append(failures, err) } writeRepinNotes(options.ErrorOutput, plan, outcomes) - return errors.Join(failures...) + return outcomes, errors.Join(failures...) }