From ce81c0b7475e62aa588e722cd655fccec8677bd6 Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Fri, 23 Oct 2026 14:18:01 +0000 Subject: [PATCH] refactor: share the selection of a new variation, and check any kind before an add The config and model config questions of link, and its model config lookup, move into the create package as SelectConfig, SelectModelConfig, and FindModelConfig. link calls them, and keeps the key, name, and content form, because the defaults of that form come from the linked file. The behavior of link does not change. CheckNotSynced now also checks a variation for a local file and a sync.lock entry, and names sync prompts in its errors. ResourcePath and ResourceExists replace the tool and skill versions. A config read keeps the keys of its archived variations, so that a create can refuse one. The next change uses these to create a variation from the flags of sync prompts add. --- internal/sync/api/catalog.go | 12 ++- internal/sync/api/catalog_test.go | 14 ++++ internal/sync/bootstrap/attachments.go | 27 ++++--- internal/sync/bootstrap/attachments_test.go | 21 ++++++ internal/sync/create/prompt.go | 82 +++++++++++++++++++++ internal/sync/create/prompt_test.go | 60 +++++++++++++++ internal/sync/link/link.go | 53 ++----------- internal/sync/local/layout.go | 9 ++- internal/sync/local/store.go | 7 +- internal/sync/prompt/create.go | 2 +- 10 files changed, 220 insertions(+), 67 deletions(-) create mode 100644 internal/sync/create/prompt.go create mode 100644 internal/sync/create/prompt_test.go diff --git a/internal/sync/api/catalog.go b/internal/sync/api/catalog.go index ae7872b7..96e29803 100644 --- a/internal/sync/api/catalog.go +++ b/internal/sync/api/catalog.go @@ -21,6 +21,9 @@ type Config struct { Name string `json:"name"` Mode syncdomain.VariationMode `json:"mode"` Variations []syncdomain.Variation `json:"variations"` + // ArchivedVariationKeys are the keys of the archived variations. A new + // variation cannot use one of them. + ArchivedVariationKeys []string `json:"-"` } // ModelConfig is a versioned set of model settings that a variation can use. @@ -136,8 +139,9 @@ func (client Client) ModelConfigs(projectKey string) ([]ModelConfig, error) { return decodeJSON[[]ModelConfig](response, "model configs response") } -// UnmarshalJSON drops archived variations. LaunchDarkly returns them in a -// config, but sync treats an archived variation as absent. +// UnmarshalJSON drops archived variations and keeps only their keys. +// LaunchDarkly returns them in a config, but sync treats an archived +// variation as absent. func (config *Config) UnmarshalJSON(data []byte) error { var response struct { Key string `json:"key"` @@ -155,7 +159,9 @@ func (config *Config) UnmarshalJSON(data []byte) error { *config = Config{Key: response.Key, Name: response.Name, Mode: response.Mode} config.Variations = make([]syncdomain.Variation, 0, len(response.Variations)) for _, variation := range response.Variations { - if variation.State != "archived" { + if variation.State == "archived" { + config.ArchivedVariationKeys = append(config.ArchivedVariationKeys, variation.Key) + } else { canonical := variation.Variation canonical.Model = syncdomain.CanonicalModel(canonical.Model, canonical.Tools) config.Variations = append(config.Variations, canonical) diff --git a/internal/sync/api/catalog_test.go b/internal/sync/api/catalog_test.go index 54c53b2d..3e7964e7 100644 --- a/internal/sync/api/catalog_test.go +++ b/internal/sync/api/catalog_test.go @@ -32,6 +32,20 @@ func TestCatalogClientSearchProjectsFiltersAndReturnsOnePage(t *testing.T) { assert.Equal(t, "20", request.Query.Get("offset")) } +func TestCatalogClientConfigKeepsTheKeysOfArchivedVariations(t *testing.T) { + transport := &recordingClient{Responses: [][]byte{[]byte(`{"key": "support", "mode": "agent", "variations": [ + {"key": "helpful", "name": "Helpful", "instructions": "Help the user."}, + {"key": "escalation", "name": "Escalation", "state": "archived", "instructions": "Escalate."} + ]}`)}} + + config, err := NewClient(transport, "token", "https://example.com").Config("project", "support") + + require.NoError(t, err) + require.Len(t, config.Variations, 1) + assert.Equal(t, "helpful", config.Variations[0].Key) + assert.Equal(t, []string{"escalation"}, config.ArchivedVariationKeys) +} + func TestCatalogClientSearchConfigsFiltersAndAppliesMode(t *testing.T) { transport := &recordingClient{Responses: [][]byte{ mustCatalogJSON(t, Page[Config]{ diff --git a/internal/sync/bootstrap/attachments.go b/internal/sync/bootstrap/attachments.go index 80aafaa9..311dd210 100644 --- a/internal/sync/bootstrap/attachments.go +++ b/internal/sync/bootstrap/attachments.go @@ -100,35 +100,44 @@ var createFlags = map[syncdomain.Kind]string{ syncdomain.KindSkill: "--content, and optionally --name and --description", } +// SyncCommand is the command that syncs the resources of kind, for example +// "ldcli sync tools". +func SyncCommand(kind syncdomain.Kind) string { + if kind == syncdomain.KindVariation { + return "ldcli sync prompts" + } + return fmt.Sprintf("ldcli sync %ss", kind) +} + // AddCommand is the command that starts syncing one resource. func AddCommand(id syncdomain.ResourceID) string { if configKey, variationKey, err := id.VariationKeys(); err == nil { - return fmt.Sprintf("ldcli sync prompts add --project %s --config-key %s --key %s", id.ProjectKey, configKey, variationKey) + return fmt.Sprintf("%s add --project %s --config-key %s --key %s", SyncCommand(id.Kind), id.ProjectKey, configKey, variationKey) } - return fmt.Sprintf("ldcli sync %ss add --project %s --key %s", id.Kind, id.ProjectKey, id.LookupKey) + return fmt.Sprintf("%s add --project %s --key %s", SyncCommand(id.Kind), id.ProjectKey, id.LookupKey) } -// 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. +// CheckNotSynced returns an error for a resource 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) + exists, err := store.ResourceExists(id) switch { case err != nil: return err case exists: - path, err := synclocal.AttachmentPath(id) + path, err := synclocal.ResourcePath(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) + return fmt.Errorf("%s is already synced. Edit %s and run %s or ldcli sync all.", id.Label(), path, SyncCommand(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 fmt.Errorf("%s has a sync.lock entry but no local file. Run %s or ldcli sync all to restore it.", id.Label(), SyncCommand(id.Kind)) } } return nil diff --git a/internal/sync/bootstrap/attachments_test.go b/internal/sync/bootstrap/attachments_test.go index da4ff104..bc72f3d4 100644 --- a/internal/sync/bootstrap/attachments_test.go +++ b/internal/sync/bootstrap/attachments_test.go @@ -108,6 +108,27 @@ func TestRunRefusesAToolBeforeAnyWrite(t *testing.T) { } } +func TestCheckNotSyncedRefusesAVariationWithAFileOrALockEntry(t *testing.T) { + root := t.TempDir() + store := synclocal.NewStore(root) + id := syncdomain.VariationID("project", "support", "default") + variation := syncdomain.Variation{Mode: syncdomain.VariationModeAgent, Key: "default", Name: "Default", Instructions: "Be helpful."} + _, err := store.Add([]synclocal.VariationFile{{ProjectKey: "project", ConfigKey: "support", Variation: variation}}) + require.NoError(t, err) + + require.EqualError(t, CheckNotSynced(store, []syncdomain.ResourceID{id}), "variation project/support/default is already synced. "+ + "Edit .launchdarkly/project/configs/support/default.prompt.md and run ldcli sync prompts or ldcli sync all.") + + require.NoError(t, os.Remove(filepath.Join(root, syncdomain.RootDir, "project", "configs", "support", "default.prompt.md"))) + fingerprint, err := syncdomain.FingerprintVariation(id.ProjectKey, id.LookupKey, variation) + require.NoError(t, err) + lock := syncmanifest.New() + lock.SetFingerprint(id, fingerprint) + require.NoError(t, syncmanifest.WriteLock(store, lock)) + require.EqualError(t, CheckNotSynced(store, []syncdomain.ResourceID{id}), + "variation project/support/default has a sync.lock entry but no local file. Run ldcli sync prompts or ldcli sync all to restore it.") +} + func TestRunNamesTheCreateFlagsOfAMissingSkill(t *testing.T) { id := syncdomain.ResourceID{Kind: syncdomain.KindSkill, ProjectKey: "project", LookupKey: "handoff"} diff --git a/internal/sync/create/prompt.go b/internal/sync/create/prompt.go new file mode 100644 index 00000000..94fccf6b --- /dev/null +++ b/internal/sync/create/prompt.go @@ -0,0 +1,82 @@ +package create + +import ( + "fmt" + "slices" + + syncdomain "github.com/launchdarkly/ldcli/internal/sync" + syncapi "github.com/launchdarkly/ldcli/internal/sync/api" + syncconsole "github.com/launchdarkly/ldcli/internal/sync/console" + syncinteractive "github.com/launchdarkly/ldcli/internal/sync/interactive" +) + +// Catalog finds the project, the config, and the model config of a new +// variation. +type Catalog interface { + syncinteractive.ProjectSearcher + syncinteractive.ConfigSearcher + Config(projectKey, configKey string) (syncapi.Config, error) + ModelConfigs(projectKey string) ([]syncapi.ModelConfig, error) +} + +// SelectConfig asks for the project of a new variation, unless projectKey is +// set, and for its config. modes keeps only the configs of those modes. The +// bool result is true when the user cancels. +func SelectConfig( + questions Questions, + catalog Catalog, + projectKey string, + modes []syncdomain.VariationMode, +) (syncapi.Project, syncapi.Config, bool, error) { + project := syncapi.Project{Key: projectKey} + if project.Key == "" { + var canceled bool + var err error + if project, canceled, err = syncinteractive.SelectProject(questions.Input, questions.Output, catalog); err != nil || canceled { + return syncapi.Project{}, syncapi.Config{}, canceled, err + } + } + config, canceled, err := syncinteractive.SelectConfig(questions.Input, questions.Output, catalog, project.Key, modes) + if syncapi.IsNotFound(err) && projectKey != "" { + // The config search is the first read in the project, so a 404 means + // that the project is missing. + return syncapi.Project{}, syncapi.Config{}, false, fmt.Errorf( + "project %q was not found in LaunchDarkly, or the access token cannot read it", projectKey, + ) + } + if err != nil || canceled { + return syncapi.Project{}, syncapi.Config{}, canceled, err + } + return project, config, false, nil +} + +// SelectModelConfig asks for the model config of a new variation. The bool +// result is true when the user cancels. +func SelectModelConfig(questions Questions, catalog Catalog, projectKey string) (syncapi.ModelConfig, bool, error) { + _ = syncconsole.New(questions.Output).Line("Loading model configs...") + modelConfigs, err := catalog.ModelConfigs(projectKey) + if err != nil { + return syncapi.ModelConfig{}, false, err + } + choices := make([]syncinteractive.Choice[syncapi.ModelConfig], 0, len(modelConfigs)) + for _, modelConfig := range modelConfigs { + choices = append(choices, syncinteractive.Choice[syncapi.ModelConfig]{ + Title: modelConfig.Name, Description: "Key: " + modelConfig.Key, Value: modelConfig, + }) + } + return syncinteractive.Select(questions.Input, questions.Output, "Choose a model config", choices) +} + +// FindModelConfig returns the model config with key in a project. The list +// has the latest version of each model config. +func FindModelConfig(catalog Catalog, projectKey, key string) (syncapi.ModelConfig, error) { + modelConfigs, err := catalog.ModelConfigs(projectKey) + if err != nil { + return syncapi.ModelConfig{}, err + } + index := slices.IndexFunc(modelConfigs, func(candidate syncapi.ModelConfig) bool { return candidate.Key == key }) + if index < 0 { + return syncapi.ModelConfig{}, fmt.Errorf("model config %q was not found", key) + } + return modelConfigs[index], nil +} diff --git a/internal/sync/create/prompt_test.go b/internal/sync/create/prompt_test.go new file mode 100644 index 00000000..ef59c11a --- /dev/null +++ b/internal/sync/create/prompt_test.go @@ -0,0 +1,60 @@ +package create + +import ( + "bytes" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + syncdomain "github.com/launchdarkly/ldcli/internal/sync" + syncapi "github.com/launchdarkly/ldcli/internal/sync/api" +) + +func TestFindModelConfigReturnsTheModelConfigWithTheKey(t *testing.T) { + catalog := &fakeCatalog{modelConfigs: []syncapi.ModelConfig{{Key: "gpt", Version: 3}, {Key: "claude", Version: 2}}} + + modelConfig, err := FindModelConfig(catalog, "demo", "claude") + require.NoError(t, err) + assert.Equal(t, syncapi.ModelConfig{Key: "claude", Version: 2}, modelConfig) + + _, err = FindModelConfig(catalog, "demo", "llama") + require.EqualError(t, err, `model config "llama" was not found`) +} + +func TestSelectConfigSkipsTheProjectQuestionAndNamesAMissingProject(t *testing.T) { + catalog := &fakeCatalog{} + questions := Questions{Allowed: true, Input: &bytes.Buffer{}, Output: &bytes.Buffer{}} + + _, _, _, err := SelectConfig(questions, catalog, "demo", nil) + + require.EqualError(t, err, `project "demo" was not found in LaunchDarkly, or the access token cannot read it`) + assert.Equal(t, []string{"demo"}, catalog.configSearches) +} + +// fakeCatalog has model configs. It fails a project search, and answers a +// config search with a 404, as LaunchDarkly does for a missing project. +type fakeCatalog struct { + modelConfigs []syncapi.ModelConfig + configSearches []string +} + +func (*fakeCatalog) SearchProjects(string, int, int) (syncapi.Page[syncapi.Project], error) { + return syncapi.Page[syncapi.Project]{}, errors.New("asked for the project") +} + +func (catalog *fakeCatalog) SearchConfigs( + projectKey, _ string, _ []syncdomain.VariationMode, _, _ int, +) (syncapi.Page[syncapi.Config], error) { + catalog.configSearches = append(catalog.configSearches, projectKey) + return syncapi.Page[syncapi.Config]{}, errors.New(`{"code":"not_found","statusCode":404}`) +} + +func (*fakeCatalog) Config(string, string) (syncapi.Config, error) { + return syncapi.Config{}, errors.New("read the config") +} + +func (catalog *fakeCatalog) ModelConfigs(string) ([]syncapi.ModelConfig, error) { + return catalog.modelConfigs, nil +} diff --git a/internal/sync/link/link.go b/internal/sync/link/link.go index 28cf904d..fe1c9eaf 100644 --- a/internal/sync/link/link.go +++ b/internal/sync/link/link.go @@ -17,7 +17,6 @@ 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" @@ -25,17 +24,9 @@ import ( "github.com/launchdarkly/ldcli/internal/sync/reference/adapters" ) -// Catalog finds the project, the config, and the model config of a new variation. -type Catalog interface { - syncinteractive.ProjectSearcher - syncinteractive.ConfigSearcher - Config(projectKey, configKey string) (syncapi.Config, error) - ModelConfigs(projectKey string) ([]syncapi.ModelConfig, error) -} - // Options are the dependencies and the input of one link. type Options struct { - Catalog Catalog + Catalog synccreate.Catalog Store synclocal.Store RepositoryRoot string WorkingDirectory string @@ -140,22 +131,16 @@ func targetSelection(options Options, prompt linkedPrompt, target Target) (Selec if err != nil { return Selection{}, "", err } - modelConfigs, err := options.Catalog.ModelConfigs(projectKey) + modelConfig, err := synccreate.FindModelConfig(options.Catalog, projectKey, target.ModelConfigKey) if err != nil { return Selection{}, "", err } - index := slices.IndexFunc(modelConfigs, func(candidate syncapi.ModelConfig) bool { - return candidate.Key == target.ModelConfigKey - }) - if index < 0 { - return Selection{}, "", fmt.Errorf("model config %q was not found", target.ModelConfigKey) - } name := cmp.Or(prompt.parsed.Name, target.Name, syncdomain.DisplayName(variationKey)) return Selection{ Project: syncapi.Project{Key: projectKey}, Config: config, - ModelConfig: modelConfigs[index], + ModelConfig: modelConfig, Key: variationKey, Name: name, }, target.Content, nil @@ -165,42 +150,16 @@ func targetSelection(options Options, prompt linkedPrompt, target Target) (Selec // that the linked file does not have. The bool result is true when the user // cancels. func promptForSelection(options Options, prompt linkedPrompt) (Selection, string, bool, error) { - project := syncapi.Project{Key: options.ProjectKey} - if project.Key == "" { - var canceled bool - var err error - if project, canceled, err = syncinteractive.SelectProject(options.Input, options.Output, options.Catalog); err != nil || canceled { - return Selection{}, "", canceled, err - } - } + questions := synccreate.Questions{Allowed: true, Input: options.Input, Output: options.Output} var modes []syncdomain.VariationMode if prompt.parsed.Mode != "" { modes = []syncdomain.VariationMode{prompt.parsed.Mode} } - config, canceled, err := syncinteractive.SelectConfig(options.Input, options.Output, options.Catalog, project.Key, modes) - if syncapi.IsNotFound(err) && options.ProjectKey != "" { - // The config search is the first read in the project, so a 404 means - // that the project is missing. - return Selection{}, "", false, fmt.Errorf( - "project %q was not found in LaunchDarkly, or the access token cannot read it", options.ProjectKey, - ) - } + project, config, canceled, err := synccreate.SelectConfig(questions, options.Catalog, options.ProjectKey, modes) if err != nil || canceled { return Selection{}, "", canceled, err } - - _ = syncconsole.New(options.Output).Line("Loading model configs...") - modelConfigs, err := options.Catalog.ModelConfigs(project.Key) - if err != nil { - return Selection{}, "", false, err - } - choices := make([]syncinteractive.Choice[syncapi.ModelConfig], 0, len(modelConfigs)) - for _, modelConfig := range modelConfigs { - choices = append(choices, syncinteractive.Choice[syncapi.ModelConfig]{ - Title: modelConfig.Name, Description: "Key: " + modelConfig.Key, Value: modelConfig, - }) - } - modelConfig, canceled, err := syncinteractive.Select(options.Input, options.Output, "Choose a model config", choices) + modelConfig, canceled, err := synccreate.SelectModelConfig(questions, options.Catalog, project.Key) if err != nil || canceled { return Selection{}, "", canceled, err } diff --git a/internal/sync/local/layout.go b/internal/sync/local/layout.go index b5e7ed5e..8e86cb55 100644 --- a/internal/sync/local/layout.go +++ b/internal/sync/local/layout.go @@ -62,10 +62,11 @@ func ParseManagedPath(file string) (syncdomain.ResourceID, bool) { return id, true } -// AttachmentPath returns the path of a tool or skill file relative to the -// repository root, in the form that ParseManagedPath reads. -func AttachmentPath(id syncdomain.ResourceID) (string, error) { - relativePath, err := attachmentPath(id.ProjectKey, syncdomain.AttachmentKind(id.Kind), id.LookupKey) +// ResourcePath returns the path of the file of a variation, a tool, or a +// skill relative to the repository root, in the form that ParseManagedPath +// reads. +func ResourcePath(id syncdomain.ResourceID) (string, error) { + relativePath, err := resourcePath(id) if err != nil { return "", err } diff --git a/internal/sync/local/store.go b/internal/sync/local/store.go index 2cfa1049..1a63687c 100644 --- a/internal/sync/local/store.go +++ b/internal/sync/local/store.go @@ -86,9 +86,10 @@ func (store Store) VariationExists(projectKey, configKey, variationKey string) ( return store.fileExists(relativePath, "variation "+variationKey) } -// AttachmentExists reports whether the file of a tool or a skill exists. -func (store Store) AttachmentExists(id syncdomain.ResourceID) (bool, error) { - relativePath, err := attachmentPath(id.ProjectKey, syncdomain.AttachmentKind(id.Kind), id.LookupKey) +// ResourceExists reports whether the file of a variation, a tool, or a skill +// exists. +func (store Store) ResourceExists(id syncdomain.ResourceID) (bool, error) { + relativePath, err := resourcePath(id) if err != nil { return false, err } diff --git a/internal/sync/prompt/create.go b/internal/sync/prompt/create.go index 128691f4..68484e3d 100644 --- a/internal/sync/prompt/create.go +++ b/internal/sync/prompt/create.go @@ -104,7 +104,7 @@ func (runner Runner) createAttachment( created syncdomain.SyncedAttachment, named bool, ) error { - path, err := synclocal.AttachmentPath(created.ID()) + path, err := synclocal.ResourcePath(created.ID()) if err != nil { return err }