From 4cd1c89934d534b87303c531a79aca049f365d1f Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Thu, 22 Oct 2026 21:39:33 +0000 Subject: [PATCH] feat: create a tool or a skill from the content of an add An add of a tool or a skill can now carry the content of a new resource: the input schema and the description of a tool, or the Markdown, the name, and the description of a skill. The new package internal/sync/create checks that content before any request, asks in a terminal for the values that are missing, and builds the resource with upsert. If LaunchDarkly has the resource with the content that the user passed, add starts syncing it through the existing add. If the content differs, add stops and changes nothing. Otherwise add plans the create in memory, asks for confirmation, and only then writes the file. It then applies the plan of that one resource through the normal sync path. A changed state or a rejected create removes the file again. A write with no response keeps the file, so that the next sync finishes the create. A skill file with upsert now keeps its name, because the create sends it. The checks of an add for a file or a sync.lock entry move into CheckNotSynced, so that the create uses the same checks. requiredValue moves from link into create. The command flags come in the next change. --- internal/sync/bootstrap/attachments.go | 44 +- internal/sync/bootstrap/attachments_test.go | 4 +- internal/sync/create/attachment.go | 194 ++++++++ internal/sync/create/create.go | 116 +++++ internal/sync/create/create_test.go | 253 ++++++++++ internal/sync/link/link.go | 15 +- internal/sync/link/link_test.go | 5 - internal/sync/local/attachment.go | 12 +- internal/sync/prompt/acceptance_test.go | 30 +- internal/sync/prompt/action.go | 20 +- internal/sync/prompt/attachment_versions.go | 15 +- internal/sync/prompt/create.go | 179 +++++++ internal/sync/prompt/create_test.go | 524 ++++++++++++++++++++ internal/sync/prompt/execute.go | 4 +- internal/sync/prompt/execute_test.go | 4 +- internal/sync/prompt/export_test.go | 15 + internal/sync/prompt/runner.go | 3 + internal/sync/prompt/sync.go | 9 +- 18 files changed, 1390 insertions(+), 56 deletions(-) create mode 100644 internal/sync/create/attachment.go create mode 100644 internal/sync/create/create.go create mode 100644 internal/sync/create/create_test.go create mode 100644 internal/sync/prompt/create.go create mode 100644 internal/sync/prompt/create_test.go create mode 100644 internal/sync/prompt/export_test.go 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...) }