From 0305eb628bb07ccc6678725f2951d2e3b0d565be Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Thu, 22 Oct 2026 20:48:23 +0000 Subject: [PATCH 1/2] feat: keep unused tools and skills, and detach any resource kind Sync no longer deletes tool and skill files that no variation uses, and it keeps their sync.lock and remote manifest entries. A detach of a variation also keeps its tools and skills. They sync on their own. Detach now takes resources of every kind. It refuses a tool or a skill that a local variation still uses. It lists every resource, marks the resources with local changes, and asks once. --yes skips the question, also with --archive. Without a terminal, detach needs --yes. Attach keeps the local content of a tool or skill file that no variation uses yet. --- cmd/sync/prompt.go | 11 +- internal/sync/detach/detach.go | 212 ++++++++++++++++++------ internal/sync/detach/detach_test.go | 150 ++++++++++++++++- internal/sync/local/attachment.go | 64 ------- internal/sync/local/attachment_test.go | 71 ++------ internal/sync/manifest/model.go | 18 -- internal/sync/manifest/model_test.go | 21 --- internal/sync/prompt/acceptance_test.go | 47 +++++- internal/sync/prompt/action.go | 14 +- internal/sync/prompt/attach.go | 32 ++-- internal/sync/prompt/review.go | 84 ---------- internal/sync/prompt/review_test.go | 102 ------------ internal/sync/prompt/runner.go | 3 +- internal/sync/prompt/runner_test.go | 34 +++- internal/sync/prompt/sync.go | 14 +- 15 files changed, 426 insertions(+), 451 deletions(-) diff --git a/cmd/sync/prompt.go b/cmd/sync/prompt.go index 70210b20a..beb0806fb 100644 --- a/cmd/sync/prompt.go +++ b/cmd/sync/prompt.go @@ -217,11 +217,14 @@ func newDetachCmd(client resources.Client) *cobra.Command { cmd := &cobra.Command{ Use: "detach [project-key/config-key/variation-key...]", Short: "Stop syncing local prompt variations", + Long: "Stop syncing local prompt variations. Detach deletes their local files and their sync.lock entries. " + + "LaunchDarkly keeps them, unless you pass --archive. Detach lists the variations and asks before it deletes files. " + + "Without a terminal, pass --yes. Tools and skills that the variation used keep syncing on their own.", Example: ` # Choose variations interactively ldcli sync prompts detach - # Stop syncing one variation - ldcli sync prompts detach --project production --config-key support --key default + # Stop syncing one variation without prompting + ldcli sync prompts detach --project production --config-key support --key default --yes --no-input # Stop syncing one variation and archive it in LaunchDarkly ldcli sync prompts detach --project production --config-key support --key default --archive --yes --no-input`, @@ -232,12 +235,12 @@ func newDetachCmd(client resources.Client) *cobra.Command { return nil, err } archive, _ := cmd.Flags().GetBool(archiveFlag) - return syncprompt.DetachAction{Variations: variations, Filter: targetFilter(cmd), Archive: archive}, nil + return syncprompt.DetachAction{Resources: variations, Filter: targetFilter(cmd), Archive: archive}, nil }), } addTargetFlags(cmd, questionFlagHelp) cmd.Flags().Bool(archiveFlag, false, "Also archive the variations in LaunchDarkly") - cmd.Flags().Bool(yesFlag, false, "Archive without confirmation") + cmd.Flags().Bool(yesFlag, false, "Detach, and archive, without confirmation") cmd.Flags().Bool(noInputFlag, false, "Fail instead of prompting for missing input") return cmd } diff --git a/internal/sync/detach/detach.go b/internal/sync/detach/detach.go index bd7b92622..53f760a0f 100644 --- a/internal/sync/detach/detach.go +++ b/internal/sync/detach/detach.go @@ -1,6 +1,6 @@ -// Package detach stops syncing selected variations. It removes their local -// files and their baseline. LaunchDarkly keeps the variations, unless the -// user asks to archive them. +// Package detach stops syncing selected resources of any kind. It removes +// their local files and their baseline. LaunchDarkly keeps the resources, +// unless the user asks to archive the variations. package detach import ( @@ -18,7 +18,7 @@ import ( syncmanifest "github.com/launchdarkly/ldcli/internal/sync/manifest" ) -const archiveQuestion = "\nArchive these variations in LaunchDarkly? [y/N] " +const detachQuestion = "\nDetach these resources? [y/N] " // Archiver reads and archives a variation in LaunchDarkly. type Archiver interface { @@ -35,17 +35,20 @@ type Options struct { ProjectKeys []string Input io.Reader Output io.Writer - // Selections are the variations to detach. If it is empty, Run asks the + // Kind is the kind of resource that the user chooses from when + // Selections is empty. + Kind syncdomain.Kind + // Selections are the resources to detach. If it is empty, Run asks the // user to choose from the synced resources that Filter keeps. Selections []syncdomain.ResourceID Filter syncdomain.Filter NoInput bool - // Archive also archives each selected variation in LaunchDarkly. Without - // Yes, the user must confirm the archive. + // Archive also archives each selected variation in LaunchDarkly. Archive bool Archiver Archiver - Yes bool - // Context stops the archive confirmation and the archive when it ends. + // Yes detaches, and archives, without a confirmation. + Yes bool + // Context stops the confirmation and the archive when it ends. Context context.Context // isTerminal reports whether the streams are a terminal. Tests replace it. @@ -60,7 +63,7 @@ func (options Options) context() context.Context { return options.Context } -// Run detaches the selected variations. +// Run detaches the selected resources after the user agrees. func Run(options Options) error { projectKeys := slices.Clone(options.ProjectKeys) for _, selection := range options.Selections { @@ -82,7 +85,9 @@ func Run(options Options) error { return err } } else { - synced = slices.DeleteFunc(synced, func(id syncdomain.ResourceID) bool { return !options.Filter.Includes(id) }) + synced = slices.DeleteFunc(synced, func(id syncdomain.ResourceID) bool { + return id.Kind != options.Kind || !options.Filter.Includes(id) + }) if len(synced) == 0 { _ = console.Line("No resources are currently synced.") return nil @@ -92,11 +97,22 @@ func Run(options Options) error { return err } } + + // A workspace with an invalid variation file does not compile. A detach + // of variations still works then, so the error only stops a check that + // needs the variations. + variations, compileErr := synclocal.CompileWorkspace(options.RepositoryRoot) + if errors.Is(compileErr, synclocal.ErrNoDirectory) { + compileErr = nil + } + if err := checkUnused(selected, variations, compileErr); err != nil { + return err + } + confirmed, err := confirmDetach(options, baseline.Lock, variations, selected) + if err != nil || !confirmed { + return err + } if options.Archive { - confirmed, err := confirmArchive(options, selected) - if err != nil || !confirmed { - return err - } // Archive first. If a later step fails, the same command can run again, // because it skips a variation that is already archived. if err := archiveVariations(options.context(), options.Archiver, selected); err != nil { @@ -113,12 +129,12 @@ func Run(options Options) error { _ = console.Line("Detached resources:") } for _, resource := range selected { - _ = console.Printf("- %s %s\n", resource.Kind, resource) + _ = console.Printf("- %s\n", resource.Label()) } return nil } -// promptForResources asks the user to choose one or more synced variations. +// promptForResources asks the user to choose one or more synced resources. // The bool result is true when the user cancels. func promptForResources(options Options, synced []syncdomain.ResourceID) ([]syncdomain.ResourceID, bool, error) { arguments := "--project and --key" @@ -141,28 +157,106 @@ func promptForResources(options Options, synced []syncdomain.ResourceID) ([]sync return syncinteractive.MultiSelect(options.Input, options.Output, "Select resources to detach", description, choices) } -// confirmArchive asks the user to agree to the archive, unless Yes is set. -func confirmArchive(options Options, selected []syncdomain.ResourceID) (bool, error) { +// checkUnused refuses to detach a tool or a skill that a local variation +// still uses, unless the same detach removes that variation. +func checkUnused(selected []syncdomain.ResourceID, variations []syncdomain.SyncedResource, compileErr error) error { + for _, id := range selected { + if id.Kind == syncdomain.KindVariation { + continue + } + if compileErr != nil { + return fmt.Errorf("find the variations that use %s: %w", id.Label(), compileErr) + } + for _, variation := range variations { + _, uses := variation.Variation.Attachment(syncdomain.AttachmentKind(id.Kind), id.LookupKey) + if uses && variation.ProjectKey == id.ProjectKey && !slices.Contains(selected, variation.ID()) { + return fmt.Errorf("%s is used by variation %s. Remove it from that variation first.", id.Label(), variation.ID()) + } + } + } + return nil +} + +// confirmDetach lists the selected resources and asks once, unless Yes is +// set. Without a terminal, it returns the error that suggests --yes. +func confirmDetach( + options Options, + lock syncmanifest.Manifest, + variations []syncdomain.SyncedResource, + selected []syncdomain.ResourceID, +) (bool, error) { if options.Yes { return true, nil } - console := syncconsole.New(options.Output) - _ = console.Line("Variations to archive in LaunchDarkly:") - for _, resource := range selected { - _ = console.Printf("- %s\n", resource) - } isTerminal := options.isTerminal if isTerminal == nil { isTerminal = syncinteractive.StreamsAreTerminal } interactive := !options.NoInput && isTerminal(options.Input, options.Output) - confirmed, err := syncinteractive.Confirm(options.context(), options.Input, options.Output, interactive, archiveQuestion) + if interactive { + console := syncconsole.New(options.Output) + if options.Archive { + _ = console.Line("Resources to detach and archive in LaunchDarkly:") + } else { + _ = console.Line("Resources to detach:") + } + changed := localChanges(options.RepositoryRoot, lock, variations, selected) + for _, id := range selected { + if changed[id] { + _ = console.Printf(" %s (local changes will be lost)\n", id.Label()) + } else { + _ = console.Printf(" %s\n", id.Label()) + } + } + } + + confirmed, err := syncinteractive.Confirm(options.context(), options.Input, options.Output, interactive, detachQuestion) if err == nil && !confirmed { - _ = console.Line("Detach canceled.") + _ = syncconsole.New(options.Output).Line("Detach canceled.") } return confirmed, err } +// localChanges reports each selected resource whose local file differs from +// its sync.lock entry, because the detach deletes that file. A file that sync +// cannot read counts as changed. A resource without a local file loses +// nothing. +func localChanges( + repositoryRoot string, + lock syncmanifest.Manifest, + variations []syncdomain.SyncedResource, + selected []syncdomain.ResourceID, +) map[syncdomain.ResourceID]bool { + local := make(map[syncdomain.ResourceID]string) + for _, variation := range variations { + if fingerprint, err := syncdomain.FingerprintVariation(variation.ProjectKey, variation.LookupKey, variation.Variation); err == nil { + local[variation.ID()] = fingerprint + } + } + isSelected := func(id syncdomain.ResourceID) bool { return slices.Contains(selected, id) } + attachments, _ := synclocal.CompileAttachments(repositoryRoot, isSelected) + for _, attachment := range attachments { + if fingerprint, err := syncdomain.FingerprintAttachment(attachment.ProjectKey, attachment.Attachment); err == nil { + local[attachment.ID()] = fingerprint + } + } + baseline := make(map[syncdomain.ResourceID]string, len(lock.Resources)) + for _, resource := range lock.Resources { + baseline[resource.ID()] = resource.Fingerprint + } + + files, _ := synclocal.SourceFiles(repositoryRoot) + changed := make(map[syncdomain.ResourceID]bool) + for _, file := range files { + id, ok := synclocal.ParseManagedPath(file) + if ok && isSelected(id) { + fingerprint, readable := local[id] + changed[id] = !readable || fingerprint != baseline[id] + } + } + return changed +} + // archiveVariations archives each variation that LaunchDarkly still has. It // reads each variation first, because the API rejects the archive of an // archived variation. A variation that is already archived or gone does not @@ -196,17 +290,17 @@ func validateSelections(synced, selected []syncdomain.ResourceID) error { seen := make(map[syncdomain.ResourceID]struct{}, len(selected)) for _, selection := range selected { if _, duplicate := seen[selection]; duplicate { - return fmt.Errorf("variation %s was selected more than once", selection) + return fmt.Errorf("%s was selected more than once", selection.Label()) } seen[selection] = struct{}{} if !slices.Contains(synced, selection) { - return fmt.Errorf("variation %s is not synced", selection) + return fmt.Errorf("%s is not synced", selection.Label()) } } return nil } -// loadResources returns each variation that the lock tracks or that has a +// loadResources returns each resource that the lock tracks or that has a // local file, in identity order. func loadResources( repositoryRoot string, @@ -224,12 +318,10 @@ func loadResources( var synced []syncdomain.ResourceID for _, resource := range baseline.Lock.Resources { - if resource.ResourceKind == syncdomain.KindVariation { - synced = append(synced, resource.ID()) - } + synced = append(synced, resource.ID()) } for _, file := range files { - if id, ok := synclocal.ParseManagedPath(file); ok && id.Kind == syncdomain.KindVariation { + if id, ok := synclocal.ParseManagedPath(file); ok { synced = append(synced, id) } } @@ -237,24 +329,14 @@ func loadResources( return slices.Compact(synced), baseline, nil } -// detachResources removes the selected variations from the baseline, and then -// deletes their local files. If the delete fails, it restores the baseline. +// detachResources removes the selected resources from the baseline, and then +// deletes their local files. Every other entry stays, also the tools and +// skills that only the detached variations used, because they sync on their +// own. If the delete fails, it restores the baseline. func detachResources(options Options, original syncmanifest.Baseline, selected []syncdomain.ResourceID) error { - isSelected := func(id syncdomain.ResourceID) bool { return slices.Contains(selected, id) } - - next := syncmanifest.New() - for _, resource := range original.Lock.Resources { - if !isSelected(resource.ID()) { - next.Resources = append(next.Resources, resource) - } - } - // Stop tracking each tool and skill that only the detached variations use. - // If the workspace does not compile, keep every attachment baseline. - if variations, err := synclocal.CompileWorkspace(options.RepositoryRoot); err == nil { - remaining := slices.DeleteFunc(variations, func(variation syncdomain.SyncedResource) bool { - return isSelected(variation.ID()) - }) - next.RemoveUnusedAttachments(remaining) + next := original.Lock.Clone() + for _, id := range selected { + next.Remove(id) } saved, err := options.Baselines.Save(original, next) if err != nil { @@ -266,23 +348,45 @@ func detachResources(options Options, original syncmanifest.Baseline, selected [ } var deletions []synclocal.VariationDeletion - for _, resource := range selected { - configKey, variationKey, err := resource.VariationKeys() + for _, id := range selected { + configKey, variationKey, err := id.VariationKeys() if err != nil { continue } - exists, err := options.Store.VariationExists(resource.ProjectKey, configKey, variationKey) + exists, err := options.Store.VariationExists(id.ProjectKey, configKey, variationKey) if err != nil { return restoreBaseline(err) } if exists { deletions = append(deletions, synclocal.VariationDeletion{ - ProjectKey: resource.ProjectKey, ConfigKey: configKey, VariationKey: variationKey, + ProjectKey: id.ProjectKey, ConfigKey: configKey, VariationKey: variationKey, }) } } + attachments, err := localAttachments(options.RepositoryRoot, selected) + if err != nil { + return restoreBaseline(err) + } if _, err := options.Store.DeleteVariations(deletions); err != nil { return restoreBaseline(err) } + if _, err := options.Store.DeleteAttachments(attachments); err != nil { + return restoreBaseline(err) + } return nil } + +// localAttachments returns each selected tool and skill that has a local file. +func localAttachments(repositoryRoot string, selected []syncdomain.ResourceID) ([]syncdomain.ResourceID, error) { + files, err := synclocal.SourceFiles(repositoryRoot) + if err != nil { + return nil, err + } + var attachments []syncdomain.ResourceID + for _, file := range files { + if id, ok := synclocal.ParseManagedPath(file); ok && id.Kind != syncdomain.KindVariation && slices.Contains(selected, id) { + attachments = append(attachments, id) + } + } + return attachments, nil +} diff --git a/internal/sync/detach/detach_test.go b/internal/sync/detach/detach_test.go index c996406f7..e484fed40 100644 --- a/internal/sync/detach/detach_test.go +++ b/internal/sync/detach/detach_test.go @@ -47,12 +47,14 @@ func TestLoadResourcesUnionsLocalAndManifestResources(t *testing.T) { require.NoError(t, err) assert.Equal(t, []syncdomain.ResourceID{ + {Kind: syncdomain.KindTool, ProjectKey: "project", LookupKey: "search"}, {Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/local"}, {Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/manifest-only"}, }, resources) } -func TestDetachResourcesPrunesUnreferencedAttachmentManifestEntries(t *testing.T) { +// A tool keeps syncing on its own after the variation that used it is detached. +func TestDetachResourcesKeepsAttachmentManifestEntries(t *testing.T) { root := t.TempDir() store := synclocal.NewStore(root) description := "Search documentation" @@ -89,9 +91,10 @@ func TestDetachResourcesPrunesUnreferencedAttachmentManifestEntries(t *testing.T require.NoError(t, err) baseline, err := manifestStore.Load([]string{"project"}) - manifest := baseline.Lock require.NoError(t, err) - assert.Empty(t, manifest.Resources) + assert.Equal(t, []syncmanifest.Resource{original.Resources[1]}, baseline.Lock.Resources) + _, err = os.Stat(filepath.Join(root, syncdomain.RootDir, "project", "tools", "search.json")) + require.NoError(t, err, "the tool file stays") } func TestDetachResourcesRemovesWrapperAndManifestButKeepsReferencedFile(t *testing.T) { @@ -118,7 +121,11 @@ func TestDetachResourcesRemovesWrapperAndManifestButKeepsReferencedFile(t *testi require.NoError(t, manifestStore.Write(original)) resource := syncdomain.ResourceID{Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/prompt"} - err = detachResources(Options{Store: store, Baselines: manifestStore}, syncmanifest.Baseline{Lock: original}, []syncdomain.ResourceID{resource}) + err = detachResources( + Options{RepositoryRoot: root, Store: store, Baselines: manifestStore}, + syncmanifest.Baseline{Lock: original}, + []syncdomain.ResourceID{resource}, + ) require.NoError(t, err) exists, err := store.VariationExists("project", "config", "prompt") @@ -145,7 +152,11 @@ func TestDetachResourcesRemovesManifestEntryWhenWrapperWasAlreadyDeleted(t *test require.NoError(t, manifestStore.Write(original)) resource := syncdomain.ResourceID{Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/deleted"} - err := detachResources(Options{Store: store, Baselines: manifestStore}, syncmanifest.Baseline{Lock: original}, []syncdomain.ResourceID{resource}) + err := detachResources( + Options{RepositoryRoot: root, Store: store, Baselines: manifestStore}, + syncmanifest.Baseline{Lock: original}, + []syncdomain.ResourceID{resource}, + ) require.NoError(t, err) baseline, err := manifestStore.Load([]string{"project"}) @@ -165,7 +176,7 @@ func TestDetachResourcesDeletesUnreadableWrapper(t *testing.T) { resource := syncdomain.ResourceID{Kind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/broken"} err := detachResources( - Options{Store: store, Baselines: manifestStore}, + Options{RepositoryRoot: root, Store: store, Baselines: manifestStore}, syncmanifest.Baseline{Lock: syncmanifest.New()}, []syncdomain.ResourceID{resource}, ) @@ -193,6 +204,7 @@ func TestRunRequiresTerminalWhenResourcesExist(t *testing.T) { Baselines: newMemoryManifestStore(), Input: bytes.NewBuffer(nil), Output: bytes.NewBuffer(nil), + Kind: syncdomain.KindVariation, }) require.ErrorContains(t, err, "requires a terminal") @@ -218,6 +230,7 @@ func TestRunUsesExplicitSelectionsWithoutTerminal(t *testing.T) { Output: bytes.NewBuffer(nil), Selections: []syncdomain.ResourceID{selection}, NoInput: true, + Yes: true, }) require.NoError(t, err) @@ -236,7 +249,7 @@ func TestRunListsOnlyTheResourcesThatTheFilterKeeps(t *testing.T) { require.NoError(t, err) options := Options{ RepositoryRoot: root, Store: store, Baselines: newMemoryManifestStore(), - Input: bytes.NewBuffer(nil), NoInput: true, + Input: bytes.NewBuffer(nil), NoInput: true, Kind: syncdomain.KindVariation, } var otherProject bytes.Buffer @@ -261,6 +274,7 @@ func TestRunReportsWhenNoResourcesAreSynced(t *testing.T) { Baselines: newMemoryManifestStore(), Input: bytes.NewBuffer(nil), Output: &output, + Kind: syncdomain.KindVariation, }) require.NoError(t, err) @@ -295,10 +309,11 @@ func TestRunArchive(t *testing.T) { wantArchived []string wantDetached bool }{ - "detach without archive keeps the variation in LaunchDarkly": {wantDetached: true}, + "detach without archive keeps the variation in LaunchDarkly": {yes: true, wantDetached: true}, "archive with yes": {archive: true, yes: true, wantArchived: []string{"project/config/prompt"}, wantDetached: true}, "variation already archived": {archive: true, yes: true, archiver: recordingArchiver{preArchived: true}, wantDetached: true}, "config gone in LaunchDarkly": {archive: true, yes: true, archiver: recordingArchiver{readErr: notFound}, wantDetached: true}, + "detach needs a confirmation": {wantError: "rerun with --yes"}, "archive needs a confirmation": {archive: true, wantError: "rerun with --yes"}, "archive failure stops detach": { archive: true, yes: true, archiver: recordingArchiver{err: errors.New("forbidden")}, @@ -372,6 +387,7 @@ func TestRunArchiveCanRunAgainAfterTheDeleteFails(t *testing.T) { workspace.requireTracked(t, "prompt", false) } +// One question covers the detach and the archive. func TestRunArchiveAsksForConfirmation(t *testing.T) { tests := map[string]struct { answer string @@ -395,7 +411,9 @@ func TestRunArchiveAsksForConfirmation(t *testing.T) { require.NoError(t, Run(options)) - assert.Contains(t, output.String(), "Archive these variations in LaunchDarkly? [y/N]") + assert.Contains(t, output.String(), "Resources to detach and archive in LaunchDarkly:\n variation project/config/prompt") + assert.Equal(t, 1, strings.Count(output.String(), "[y/N]")) + assert.Contains(t, output.String(), "Detach these resources? [y/N]") assert.Contains(t, output.String(), test.wantOutput) assert.Equal(t, test.wantArchived, archiver.archived) workspace.requireTracked(t, "prompt", test.wantArchived == nil) @@ -403,6 +421,79 @@ func TestRunArchiveAsksForConfirmation(t *testing.T) { } } +func TestRunDetachesToolsAndSkills(t *testing.T) { + workspace := newArchiveWorkspace(t, "prompt") + tool, skill := testTool(), testSkill() + workspace.addAttachments(t, tool, skill) + archiver := &recordingArchiver{} + options := workspace.options(archiver) + options.Selections = []syncdomain.ResourceID{tool.ID("project"), skill.ID("project")} + options.Yes = true + + require.NoError(t, Run(options)) + + assert.Empty(t, archiver.archived, "LaunchDarkly keeps the tool and the skill") + assert.Equal(t, "Detached resources:\n- tool project/search\n- skill project/summarize\n", workspace.output(options)) + local, err := synclocal.CompileAttachments(workspace.root, func(syncdomain.ResourceID) bool { return true }) + require.NoError(t, err) + assert.Empty(t, local) + assert.Equal(t, []syncdomain.ResourceID{syncdomain.VariationID("project", "config", "prompt")}, lockIDs(workspace.baselines.manifest)) +} + +func TestRunRefusesAToolThatAVariationStillUses(t *testing.T) { + workspace := newArchiveWorkspace(t) + tool := testTool() + variation := testVariation("prompt") + variation.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + variation.Attachments = []syncdomain.Attachment{tool} + _, err := workspace.store.Add([]synclocal.VariationFile{{ProjectKey: "project", ConfigKey: "config", Variation: variation}}) + require.NoError(t, err) + prompt := syncdomain.VariationID("project", "config", "prompt") + workspace.baselines.manifest.SetFingerprint(prompt, testFingerprint()) + require.NoError(t, workspace.baselines.manifest.SetAttachments("project", variation.Attachments)) + options := workspace.options(&recordingArchiver{}) + options.Yes = true + + options.Selections = []syncdomain.ResourceID{tool.ID("project")} + require.EqualError(t, Run(options), + "tool project/search is used by variation project/config/prompt. Remove it from that variation first.") + assert.Len(t, workspace.baselines.manifest.Resources, 2, "nothing changes") + + options.Selections = []syncdomain.ResourceID{prompt, tool.ID("project")} + require.NoError(t, Run(options), "a detach of the variation too frees the tool") + assert.Empty(t, workspace.baselines.manifest.Resources) +} + +func TestRunMarksResourcesWithLocalChanges(t *testing.T) { + workspace := newArchiveWorkspace(t, "edited", "synced") + tool := testTool() + workspace.addAttachments(t, tool) + variations, err := synclocal.CompileWorkspace(workspace.root) + require.NoError(t, err) + synced := syncdomain.VariationID("project", "config", "synced") + fingerprint, err := syncdomain.FingerprintVariation("project", synced.LookupKey, variations[1].Variation) + require.NoError(t, err) + workspace.baselines.manifest.SetFingerprint(synced, fingerprint) + lockOnly := syncdomain.VariationID("project", "config", "lock-only") + workspace.baselines.manifest.SetFingerprint(lockOnly, testFingerprint()) + + var output bytes.Buffer + options := workspace.options(&recordingArchiver{}) + options.Selections = append(options.Selections, tool.ID("project"), lockOnly) + options.NoInput, options.Input, options.Output = false, strings.NewReader("n\n"), &output + options.isTerminal = func(io.Reader, io.Writer) bool { return true } + + require.NoError(t, Run(options)) + + assert.Equal(t, "Resources to detach:\n"+ + " variation project/config/edited (local changes will be lost)\n"+ + " variation project/config/synced\n"+ + " tool project/search\n"+ + " variation project/config/lock-only\n"+ + "\nDetach these resources? [y/N] Detach canceled.\n", output.String()) + assert.Len(t, workspace.baselines.manifest.Resources, 4, "a no changes nothing") +} + // archiveWorkspace is a workspace whose variations have local files and // baseline entries. type archiveWorkspace struct { @@ -443,6 +534,47 @@ func (workspace archiveWorkspace) options(archiver Archiver) Options { } } +// addAttachments writes standalone tool and skill files with baseline entries +// that match them. +func (workspace archiveWorkspace) addAttachments(t *testing.T, attachments ...syncdomain.Attachment) { + t.Helper() + synced := make([]syncdomain.SyncedAttachment, 0, len(attachments)) + for _, attachment := range attachments { + synced = append(synced, syncdomain.SyncedAttachment{ProjectKey: "project", Attachment: attachment}) + } + _, err := workspace.store.ReplaceAttachments(synced) + require.NoError(t, err) + require.NoError(t, workspace.baselines.manifest.SetAttachments("project", attachments)) +} + +// output returns what Run wrote to the output of options. +func (archiveWorkspace) output(options Options) string { + return options.Output.(*bytes.Buffer).String() +} + +func lockIDs(manifest syncmanifest.Manifest) []syncdomain.ResourceID { + ids := make([]syncdomain.ResourceID, 0, len(manifest.Resources)) + for _, resource := range manifest.Resources { + ids = append(ids, resource.ID()) + } + return ids +} + +func testTool() syncdomain.Attachment { + description := "Search documentation" + return syncdomain.Attachment{ + Kind: syncdomain.AttachmentTool, + Tool: &syncdomain.Tool{Key: "search", Description: &description, Schema: map[string]any{"type": "object"}}, + } +} + +func testSkill() syncdomain.Attachment { + return syncdomain.Attachment{ + Kind: syncdomain.AttachmentSkill, + Skill: &syncdomain.Skill{Key: "summarize", Description: "Summarize", Markdown: "# Summarize\n"}, + } +} + // requireTracked checks both the local file and the baseline entry. func (workspace archiveWorkspace) requireTracked(t *testing.T, key string, tracked bool) { t.Helper() diff --git a/internal/sync/local/attachment.go b/internal/sync/local/attachment.go index 892359504..8f227d557 100644 --- a/internal/sync/local/attachment.go +++ b/internal/sync/local/attachment.go @@ -2,14 +2,12 @@ package local import ( "bytes" - "cmp" "encoding/json" "errors" "fmt" "io" "io/fs" "maps" - "os" "path" "path/filepath" "slices" @@ -37,14 +35,6 @@ type skillFrontMatter struct { Upsert bool `yaml:"upsert,omitempty"` } -// OrphanedAttachment is a tool or skill file that no variation uses. -type OrphanedAttachment struct { - ProjectKey string - Kind syncdomain.AttachmentKind - Key string - Path string -} - // CompileAttachments reads each tool and skill file that include accepts. It // does not read the other files, so an invalid file outside include does not // stop the read. Like CompileWorkspace, it rejects a symbolic link in a @@ -293,60 +283,6 @@ func preserveLocalFields(filePath string, original, replacement []byte) ([]byte, return renderSkillFile(updated, string(markdown)) } -// OrphanedAttachments returns each tool and skill file that no local -// variation uses. -func (store Store) OrphanedAttachments() ([]OrphanedAttachment, error) { - variations, err := CompileWorkspace(store.repositoryRoot) - if err != nil && !errors.Is(err, ErrNoDirectory) { - return nil, err - } - used := make(map[syncdomain.ResourceID]struct{}) - for _, variation := range variations { - for _, attachment := range variation.Variation.Attachments { - used[attachment.ID(variation.ProjectKey)] = struct{}{} - } - } - - var orphaned []OrphanedAttachment - err = filepath.WalkDir(store.root, func(filePath string, entry fs.DirEntry, err error) error { - if err != nil || entry.IsDir() { - return err - } - relativePath, err := filepath.Rel(store.repositoryRoot, filePath) - if err != nil { - return err - } - id, ok := ParseManagedPath(filepath.ToSlash(relativePath)) - if !ok || id.Kind == syncdomain.KindVariation { - return nil - } - if _, isUsed := used[id]; !isUsed { - orphaned = append(orphaned, OrphanedAttachment{ - ProjectKey: id.ProjectKey, - Kind: syncdomain.AttachmentKind(id.Kind), - Key: id.LookupKey, - Path: strings.TrimPrefix(filepath.ToSlash(relativePath), syncdomain.RootDir+"/"), - }) - } - return nil - }) - if errors.Is(err, os.ErrNotExist) { - return nil, nil - } - if err != nil { - return nil, fmt.Errorf("find unreferenced attachments: %w", err) - } - - slices.SortFunc(orphaned, func(left, right OrphanedAttachment) int { - return cmp.Or( - strings.Compare(left.ProjectKey, right.ProjectKey), - strings.Compare(string(left.Kind), string(right.Kind)), - strings.Compare(left.Key, right.Key), - ) - }) - return orphaned, nil -} - // DeleteAttachments deletes the files of tools and skills in one transaction. func (store Store) DeleteAttachments(ids []syncdomain.ResourceID) ([]string, error) { paths := make([]string, 0, len(ids)) diff --git a/internal/sync/local/attachment_test.go b/internal/sync/local/attachment_test.go index ce9734894..18cf9df0f 100644 --- a/internal/sync/local/attachment_test.go +++ b/internal/sync/local/attachment_test.go @@ -241,68 +241,27 @@ func TestReplaceVariationsLeavesAttachmentUnchangedWhenPreflightFails(t *testing assert.Equal(t, oldDescription, *attachment.Tool.Description) } -func TestOrphanedAttachmentsRequireEveryLocalReferenceToBeRemoved(t *testing.T) { +func TestDeleteAttachmentsDeletesToolAndSkillFilesByID(t *testing.T) { root := t.TempDir() store := NewStore(root) - tool := syncdomain.Tool{Key: "search", Schema: map[string]any{"type": "object"}} - first, second := localVariation("first"), localVariation("second") - for _, variation := range []*VariationFile{&first, &second} { - variation.Variation.Tools = []syncdomain.AttachmentRef{{Key: tool.Key}} - variation.Variation.Attachments = []syncdomain.Attachment{{ - Kind: syncdomain.AttachmentTool, Tool: &tool, - }} - } - _, err := store.Add([]VariationFile{first, second}) - require.NoError(t, err) - - first.Variation.Tools = nil - first.Variation.Attachments = nil - _, err = store.ReplaceVariations([]VariationReplacement{{ - ProjectKey: first.ProjectKey, ConfigKey: first.ConfigKey, Variation: first.Variation, - }}) - require.NoError(t, err) - orphaned, err := store.OrphanedAttachments() - require.NoError(t, err) - assert.Empty(t, orphaned) - - second.Variation.Tools = nil - second.Variation.Attachments = nil - _, err = store.ReplaceVariations([]VariationReplacement{{ - ProjectKey: second.ProjectKey, ConfigKey: second.ConfigKey, Variation: second.Variation, - }}) - require.NoError(t, err) - orphaned, err = store.OrphanedAttachments() - require.NoError(t, err) - require.Equal(t, []OrphanedAttachment{{ - ProjectKey: "project", Kind: syncdomain.AttachmentTool, Key: "search", Path: "project/tools/search.json", - }}, orphaned) - - deleted, err := store.DeleteAttachments([]syncdomain.ResourceID{{Kind: syncdomain.KindTool, ProjectKey: "project", LookupKey: "search"}}) - require.NoError(t, err) - assert.Equal(t, []string{"project/tools/search.json"}, deleted) - _, statErr := os.Stat(filepath.Join(root, ".launchdarkly", "project", "tools", "search.json")) - require.ErrorIs(t, statErr, os.ErrNotExist) -} - -func TestOrphanedAttachmentsFindsFlatSkillFile(t *testing.T) { - root := t.TempDir() + toolPath := filepath.Join(root, ".launchdarkly", "project", "tools", "search.json") skillPath := filepath.Join(root, ".launchdarkly", "project", "skills", "support.md") - require.NoError(t, os.MkdirAll(filepath.Dir(skillPath), 0o755)) - require.NoError(t, os.WriteFile(skillPath, []byte("# Support\n"), 0o644)) - - store := NewStore(root) - orphaned, err := store.OrphanedAttachments() + for _, file := range []string{toolPath, skillPath} { + require.NoError(t, os.MkdirAll(filepath.Dir(file), 0o755)) + require.NoError(t, os.WriteFile(file, []byte("content\n"), 0o644)) + } - require.NoError(t, err) - require.Equal(t, []OrphanedAttachment{{ - ProjectKey: "project", Kind: syncdomain.AttachmentSkill, Key: "support", Path: "project/skills/support.md", - }}, orphaned) + deleted, err := store.DeleteAttachments([]syncdomain.ResourceID{ + {Kind: syncdomain.KindTool, ProjectKey: "project", LookupKey: "search"}, + {Kind: syncdomain.KindSkill, ProjectKey: "project", LookupKey: "support"}, + }) - deleted, err := store.DeleteAttachments([]syncdomain.ResourceID{{Kind: syncdomain.KindSkill, ProjectKey: "project", LookupKey: "support"}}) require.NoError(t, err) - assert.Equal(t, []string{"project/skills/support.md"}, deleted) - _, statErr := os.Stat(skillPath) - require.ErrorIs(t, statErr, os.ErrNotExist) + assert.Equal(t, []string{"project/tools/search.json", "project/skills/support.md"}, deleted) + for _, file := range []string{toolPath, skillPath} { + _, statErr := os.Stat(file) + require.ErrorIs(t, statErr, os.ErrNotExist) + } } func TestCompileAttachmentsReadsStandaloneAndReferencedFiles(t *testing.T) { diff --git a/internal/sync/manifest/model.go b/internal/sync/manifest/model.go index 4290f8963..d3c64345d 100644 --- a/internal/sync/manifest/model.go +++ b/internal/sync/manifest/model.go @@ -116,24 +116,6 @@ func (manifest *Manifest) setAttachments(projectKey string, attachments []syncdo return nil } -// RemoveUnusedAttachments stops tracking each tool and skill that none of the -// variations references. -func (manifest *Manifest) RemoveUnusedAttachments(variations []syncdomain.SyncedResource) { - used := make(map[syncdomain.ResourceID]struct{}) - for _, variation := range variations { - for _, attachment := range variation.Variation.Attachments { - used[attachment.ID(variation.ProjectKey)] = struct{}{} - } - } - manifest.Resources = slices.DeleteFunc(manifest.Resources, func(resource Resource) bool { - if resource.ResourceKind == syncdomain.KindVariation { - return false - } - _, ok := used[resource.ID()] - return !ok - }) -} - // WithChanges returns a copy of the manifest with the entry changes from // before to after. An entry that is the same in before and after keeps the // value that the manifest has, which can come from another working copy. diff --git a/internal/sync/manifest/model_test.go b/internal/sync/manifest/model_test.go index 4f92975f3..0efbd8682 100644 --- a/internal/sync/manifest/model_test.go +++ b/internal/sync/manifest/model_test.go @@ -59,24 +59,3 @@ func TestManifestPreservesExistingAttachmentBaselineWhenAddingConsumer(t *testin require.Len(t, manifest.Resources, 1) assert.Equal(t, "sha256:existing", manifest.Resources[0].Fingerprint) } - -func TestManifestRemovesOnlyUnreferencedAttachments(t *testing.T) { - manifest := New() - manifest.Resources = []Resource{ - {ResourceKind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/default"}, - {ResourceKind: syncdomain.KindTool, ProjectKey: "project", LookupKey: "search"}, - {ResourceKind: syncdomain.KindSkill, ProjectKey: "project", LookupKey: "support"}, - } - search := syncdomain.Attachment{Kind: syncdomain.AttachmentTool, Tool: &syncdomain.Tool{Key: "search"}} - variations := []syncdomain.SyncedResource{{ - ProjectKey: "project", - Variation: syncdomain.Variation{Attachments: []syncdomain.Attachment{search}}, - }} - - manifest.RemoveUnusedAttachments(variations) - - assert.Equal(t, []Resource{ - {ResourceKind: syncdomain.KindVariation, ProjectKey: "project", LookupKey: "config/default"}, - {ResourceKind: syncdomain.KindTool, ProjectKey: "project", LookupKey: "search"}, - }, manifest.Resources) -} diff --git a/internal/sync/prompt/acceptance_test.go b/internal/sync/prompt/acceptance_test.go index ee26b09e5..9a4bb516e 100644 --- a/internal/sync/prompt/acceptance_test.go +++ b/internal/sync/prompt/acceptance_test.go @@ -611,7 +611,9 @@ func TestPromptPullsLatestToolAndAdvancesVariationPin(t *testing.T) { assertManifestFingerprint(t, root, expected) } -func TestPromptDeletesUnreferencedLocalToolWithYes(t *testing.T) { +// A tool file that no variation uses syncs on its own, so a sync keeps the +// file and both of its rows. +func TestPromptKeepsUnreferencedLocalToolWithYes(t *testing.T) { root := initRepository(t) tool := syncdomain.Tool{Key: "search", Schema: map[string]any{"type": "object"}} baseline := variation("Baseline") @@ -639,15 +641,52 @@ func TestPromptDeletesUnreferencedLocalToolWithYes(t *testing.T) { assert.Empty(t, api.variation.Tools) assert.Contains(t, api.tools, "search") _, err = os.Stat(filepath.Join(root, ".launchdarkly", "production", "tools", "search.json")) - require.ErrorIs(t, err, os.ErrNotExist) - // A sync keeps the row of a tool that no variation uses. Only a detach - // removes it. + require.NoError(t, err, "the tool file stays") searchFingerprint, err := syncdomain.FingerprintAttachment("production", toolAttachment(tool, 2)) require.NoError(t, err) assert.Equal(t, []syncmanifest.Resource{ {ResourceKind: syncdomain.KindTool, ProjectKey: "production", LookupKey: "search", Fingerprint: searchFingerprint}, {ResourceKind: syncdomain.KindVariation, ProjectKey: "production", LookupKey: "support/default", Fingerprint: fingerprint(t, detached)}, }, readLock(t, root).Resources) + assert.Contains(t, manifestsByRoot[root].Items, syncapi.SyncManifestResource{ + ResourceKind: syncdomain.KindTool, ResourceLookupKey: "search", Fingerprint: searchFingerprint, Version: 1, + }) +} + +// A detach of a variation keeps the tools that it used, with their files +// and rows. Without --yes and without a terminal, detach changes nothing. +func TestPromptDetachKeepsToolsOfTheVariation(t *testing.T) { + root := initRepository(t) + tool := syncdomain.Tool{Key: "search", Schema: map[string]any{"type": "object"}} + baseline := variation("Baseline") + baseline.Tools = []syncdomain.AttachmentRef{{Key: "search", Version: 1}} + baseline.Attachments = []syncdomain.Attachment{toolAttachment(tool, 1)} + writeVariation(t, root, baseline, true) + writeManifest(t, root, baseline) + api := &directAPI{variation: pointer(baseline), tools: map[string]versionedTool{"search": {Tool: tool, Version: 1}}} + detach := []string{"detach", "--project", "production", "--config-key", "support", "--key", "default", "--no-input"} + toolFile := filepath.Join(root, syncdomain.RootDir, "production", "tools", "search.json") + + _, _, err := runPrompt(t, root, api, detach...) + require.ErrorContains(t, err, "rerun with --yes") + assert.Len(t, readLock(t, root).Resources, 2, "nothing changes") + + stdout, _, err := runPrompt(t, root, api, append(detach, "--yes")...) + require.NoError(t, err) + assert.Equal(t, "Detached resources:\n- variation production/support/default\n", stdout) + _, err = os.Stat(toolFile) + require.NoError(t, err, "the tool file stays") + assert.Equal(t, []syncdomain.ResourceID{{Kind: syncdomain.KindTool, ProjectKey: "production", LookupKey: "search"}}, + lockResourceIDs(readLock(t, root))) + assert.Len(t, manifestsByRoot[root].Items, 1) +} + +func lockResourceIDs(lock syncmanifest.Manifest) []syncdomain.ResourceID { + ids := make([]syncdomain.ResourceID, 0, len(lock.Resources)) + for _, resource := range lock.Resources { + ids = append(ids, resource.ID()) + } + return ids } func TestPromptUpdateResolvesOmittedModelConfigVersionToLatest(t *testing.T) { diff --git a/internal/sync/prompt/action.go b/internal/sync/prompt/action.go index 8b31dfbba..df9c7cbee 100644 --- a/internal/sync/prompt/action.go +++ b/internal/sync/prompt/action.go @@ -44,13 +44,13 @@ type AttachAction struct { Target *syncdomain.ResourceID } -// DetachAction stops syncing selected local variations. With Archive, it -// also archives them in LaunchDarkly. The variations come from positional -// selectors or from the target flags in Filter. +// DetachAction stops syncing selected local resources. With Archive, it +// also archives the variations in LaunchDarkly. The resources come from +// positional selectors or from the target flags in Filter. type DetachAction struct { - Variations []syncdomain.ResourceID - Filter syncdomain.Filter - Archive bool + Resources []syncdomain.ResourceID + Filter syncdomain.Filter + Archive bool } // LinkAction creates a variation whose prompt is an external file, and then @@ -74,7 +74,7 @@ func (action AddAction) validate() error { } func (action DetachAction) validate() error { - return validateSelections(action.Variations, action.Filter) + return validateSelections(action.Resources, action.Filter) } func (action AttachAction) validate() error { diff --git a/internal/sync/prompt/attach.go b/internal/sync/prompt/attach.go index 54174ccbf..443b55726 100644 --- a/internal/sync/prompt/attach.go +++ b/internal/sync/prompt/attach.go @@ -50,11 +50,16 @@ func attachToVariation(store synclocal.Store, client syncapi.Client, options att return err } - // Other local variations can share the attachment file. Keep its local - // content, so that attach never overwrites a local edit. The next sync - // shows a conflict if the content differs from LaunchDarkly. - if local, ok := localAttachment(variations, attachment.ID(projectKey)); ok { - attachment = local + // The attachment can already have a local file, which other variations use + // or which syncs on its own. Keep its local content, so that attach never + // overwrites a local edit. The next sync shows a conflict if the content + // differs from LaunchDarkly. + local, err := localAttachment(options.RepositoryRoot, attachment.ID(projectKey)) + if err != nil { + return err + } + if local != nil { + attachment = *local } variation := target.Variation variation.Attach(attachment) @@ -72,17 +77,14 @@ func attachToVariation(store synclocal.Store, client syncapi.Client, options att return err } -// localAttachment finds the local content of an attachment. -func localAttachment(variations []syncdomain.SyncedResource, id ResourceID) (syncdomain.Attachment, bool) { - for _, variation := range variations { - if variation.ProjectKey != id.ProjectKey { - continue - } - if attachment, ok := variation.Variation.Attachment(syncdomain.AttachmentKind(id.Kind), id.LookupKey); ok { - return attachment, true - } +// localAttachment reads the local file of an attachment. It returns nil when +// the attachment has no local file. +func localAttachment(repositoryRoot string, id ResourceID) (*syncdomain.Attachment, error) { + local, err := synclocal.CompileAttachments(repositoryRoot, func(candidate ResourceID) bool { return candidate == id }) + if err != nil || len(local) == 0 { + return nil, err } - return syncdomain.Attachment{}, false + return &local[0].Attachment, nil } // selectAttachmentProject returns the project of the attachment. A workspace diff --git a/internal/sync/prompt/review.go b/internal/sync/prompt/review.go index 3f75abb22..181fdbad9 100644 --- a/internal/sync/prompt/review.go +++ b/internal/sync/prompt/review.go @@ -2,13 +2,9 @@ package prompt import ( "errors" - "slices" - "strings" - syncdomain "github.com/launchdarkly/ldcli/internal/sync" syncconsole "github.com/launchdarkly/ldcli/internal/sync/console" syncinteractive "github.com/launchdarkly/ldcli/internal/sync/interactive" - synclocal "github.com/launchdarkly/ldcli/internal/sync/local" ) const applyQuestion = "\nSync these changes? [y/N] " @@ -51,83 +47,3 @@ func reviewAndConfirmPlan(options Options, plan Plan, interactive bool) (bool, e } return confirmed, nil } - -// cleanupOrphanedAttachments shows the tool and skill files that no local -// variation uses, and deletes them after the user agrees. Without --yes and -// without a terminal, it keeps the files. Before it deletes a file, it makes -// sure again that no variation uses the file. A filtered sync keeps every -// file. A variation outside the filter can have a missing file that a full -// sync restores, and then its tools and skills only look unused. A sync that -// plans tools or skills also keeps every file, because each file is a row of -// its own. -func cleanupOrphanedAttachments(options Options, store synclocal.Store, interactive bool) error { - action := options.syncAction() - if !action.Filter.IsZero() || action.Scope.Includes(syncdomain.KindTool) || action.Scope.Includes(syncdomain.KindSkill) { - return nil - } - orphaned, err := store.OrphanedAttachments() - if err != nil || len(orphaned) == 0 { - return err - } - - console := syncconsole.New(options.ErrorOutput) - _ = console.Line("\nUnreferenced local attachment files:") - currentProject := "" - for _, attachment := range orphaned { - if attachment.ProjectKey != currentProject { - currentProject = attachment.ProjectKey - _ = console.Printf(" Project: %s\n", currentProject) - } - kind := string(attachment.Kind) - _ = console.Printf( - " %s %q\n %s/%s\n", strings.ToUpper(kind[:1])+kind[1:], attachment.Key, syncdomain.RootDir, attachment.Path, - ) - } - - if !options.Yes && !interactive { - _ = console.Line("Unreferenced attachment files kept. Rerun with --yes to delete them.") - return nil - } - if !options.Yes { - confirmed, err := syncinteractive.Confirm( - options.Context, options.Input, options.ErrorOutput, interactive, - "\nDelete these unreferenced local files? [y/N] ", - ) - if err != nil { - return err - } - if !confirmed { - _ = console.Line("Unreferenced attachment files kept.") - return nil - } - } - - // A file can become used again while the user reads the question. - current, err := store.OrphanedAttachments() - if err != nil { - return err - } - orphaned = slices.DeleteFunc(orphaned, func(attachment synclocal.OrphanedAttachment) bool { - return !slices.Contains(current, attachment) - }) - if len(orphaned) == 0 { - _ = console.Line("No selected attachment files remain unreferenced.") - return nil - } - - ids := make([]syncdomain.ResourceID, 0, len(orphaned)) - for _, attachment := range orphaned { - ids = append(ids, syncdomain.ResourceID{ - Kind: syncdomain.Kind(attachment.Kind), ProjectKey: attachment.ProjectKey, LookupKey: attachment.Key, - }) - } - deleted, err := store.DeleteAttachments(ids) - if err != nil { - return err - } - _ = console.Line("Deleted unreferenced attachment files:") - for _, file := range deleted { - _ = console.Printf("- %s/%s\n", syncdomain.RootDir, file) - } - return nil -} diff --git a/internal/sync/prompt/review_test.go b/internal/sync/prompt/review_test.go index ff88b0e08..78fbeaf18 100644 --- a/internal/sync/prompt/review_test.go +++ b/internal/sync/prompt/review_test.go @@ -4,8 +4,6 @@ import ( "bytes" "context" "io" - "os" - "path/filepath" "strings" "sync" "testing" @@ -14,9 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - syncdomain "github.com/launchdarkly/ldcli/internal/sync" syncinteractive "github.com/launchdarkly/ldcli/internal/sync/interactive" - synclocal "github.com/launchdarkly/ldcli/internal/sync/local" ) type notifyingWriter struct { @@ -32,17 +28,6 @@ func (writer *notifyingWriter) Write(data []byte) (int, error) { return writer.Writer.Write(data) } -type callbackReader struct { - io.Reader - once sync.Once - callback func() -} - -func (reader *callbackReader) Read(data []byte) (int, error) { - reader.once.Do(reader.callback) - return reader.Reader.Read(data) -} - func TestConfirmApply(t *testing.T) { tests := []struct { name string @@ -72,93 +57,6 @@ func TestConfirmApply(t *testing.T) { } } -func TestCleanupOrphanedAttachmentsRequiresConfirmationUnlessYes(t *testing.T) { - tests := []struct { - name string - input string - yes bool - interactive bool - wantExist bool - message string - }{ - {name: "declined", input: "n\n", interactive: true, wantExist: true, message: "files kept"}, - {name: "yes flag", yes: true, message: "Deleted unreferenced attachment files"}, - {name: "non-terminal", wantExist: true, message: "Rerun with --yes"}, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - root, store, _ := newOrphanedToolStore(t) - - var output bytes.Buffer - err := cleanupOrphanedAttachments(Options{ - Yes: test.yes, Input: strings.NewReader(test.input), ErrorOutput: &output, - }, store, test.interactive) - - require.NoError(t, err) - _, statErr := os.Stat(filepath.Join(root, ".launchdarkly", "project", "tools", "search.json")) - assert.Equal(t, test.wantExist, statErr == nil) - assert.Contains(t, output.String(), `Tool "search"`) - assert.Contains(t, output.String(), test.message) - if test.yes { - assert.NotContains(t, output.String(), "Delete these unreferenced local files?") - } - }) - } -} - -func TestCleanupOrphanedAttachmentsRechecksReferencesAfterConfirmation(t *testing.T) { - root, store, attachedVariation := newOrphanedToolStore(t) - input := &callbackReader{ - Reader: strings.NewReader("yes\n"), - callback: func() { - _, err := store.ReplaceVariations([]synclocal.VariationReplacement{{ - ProjectKey: "project", - ConfigKey: "config", - Variation: attachedVariation, - }}) - require.NoError(t, err) - }, - } - var output bytes.Buffer - - err := cleanupOrphanedAttachments(Options{ - Input: input, ErrorOutput: &output, - }, store, true) - - require.NoError(t, err) - _, statErr := os.Stat(filepath.Join(root, ".launchdarkly", "project", "tools", "search.json")) - require.NoError(t, statErr) - assert.Contains(t, output.String(), "No selected attachment files remain unreferenced.") -} - -func newOrphanedToolStore(t *testing.T) (string, synclocal.Store, syncdomain.Variation) { - t.Helper() - root := t.TempDir() - store := synclocal.NewStore(root) - tool := syncdomain.Tool{Key: "search", Schema: map[string]any{"type": "object"}} - attached := syncdomain.Variation{ - Mode: syncdomain.VariationModeAgent, Key: "default", Name: "Default", - Tools: []syncdomain.AttachmentRef{{Key: tool.Key}}, - Attachments: []syncdomain.Attachment{{ - Kind: syncdomain.AttachmentTool, Tool: &tool, - }}, - } - _, err := store.Add([]synclocal.VariationFile{{ - ProjectKey: "project", ConfigKey: "config", Variation: attached, - }}) - require.NoError(t, err) - - orphaned := attached - orphaned.Tools = nil - orphaned.Attachments = nil - _, err = store.ReplaceVariations([]synclocal.VariationReplacement{{ - ProjectKey: "project", ConfigKey: "config", Variation: orphaned, - }}) - require.NoError(t, err) - return root, store, attached -} - func TestReviewAndConfirmPlanStopsWhenWatchContextIsCanceled(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) input, inputWriter := io.Pipe() diff --git a/internal/sync/prompt/runner.go b/internal/sync/prompt/runner.go index e6917df12..4dfa69c34 100644 --- a/internal/sync/prompt/runner.go +++ b/internal/sync/prompt/runner.go @@ -155,8 +155,9 @@ func (runner Runner) runDetach(options Options, workspace syncWorkspace, action ProjectKeys: projectKeys(synced, syncdomain.Filter{}), Input: options.Input, Output: options.Output, + Kind: syncdomain.KindVariation, Filter: action.Filter, - Selections: selections(action.Variations, action.Filter), + Selections: selections(action.Resources, action.Filter), NoInput: options.NoInput, Archive: action.Archive, Archiver: runner.api(options), diff --git a/internal/sync/prompt/runner_test.go b/internal/sync/prompt/runner_test.go index ec0a45daa..0179931cf 100644 --- a/internal/sync/prompt/runner_test.go +++ b/internal/sync/prompt/runner_test.go @@ -161,18 +161,21 @@ func TestRunnerDetachesWithoutCallingTheAPI(t *testing.T) { root := initGitRepository(t) runner := NewRunner(noopResourceClient{}) called := false + selection := syncdomain.VariationID("project", "config", "default") runner.detach = func(options syncdetach.Options) error { called = true assert.NotZero(t, options.Store) assert.NotZero(t, options.Baselines) assert.Equal(t, os.Stdin, options.Input) assert.Equal(t, io.Discard, options.Output) + assert.Equal(t, syncdomain.KindVariation, options.Kind) + assert.Equal(t, []syncdomain.ResourceID{selection}, options.Selections) return nil } err := runner.Run(Options{ WorkingDirectory: root, - Action: DetachAction{}, + Action: DetachAction{Resources: []syncdomain.ResourceID{selection}}, Input: os.Stdin, Output: io.Discard, ErrorOutput: io.Discard, @@ -277,6 +280,35 @@ func TestAttachToVariationPreservesExistingSharedAttachmentEdits(t *testing.T) { } } +// A tool file that no variation uses yet also keeps its local edit. +func TestAttachToVariationPreservesStandaloneAttachmentEdits(t *testing.T) { + root := t.TempDir() + store := synclocal.NewStore(root) + localDescription, serverDescription := "Local edit", "Server content" + tool := syncdomain.Tool{Key: "search", Description: &localDescription, Schema: map[string]any{"type": "object"}} + _, err := store.Add([]synclocal.VariationFile{{ProjectKey: "project", ConfigKey: "config", Variation: testVariation("default")}}) + require.NoError(t, err) + _, err = store.ReplaceAttachments([]syncdomain.SyncedAttachment{{ + ProjectKey: "project", Attachment: syncdomain.Attachment{Kind: syncdomain.AttachmentTool, Tool: &tool}, + }}) + require.NoError(t, err) + transport := &attachmentMutationAPI{ + current: syncdomain.Tool{Key: "search", Description: &serverDescription, Schema: map[string]any{"type": "object"}}, + version: 2, + } + + err = attachToVariation(store, syncapi.NewClient(transport, "token", "https://example.com"), attachOptions{ + RepositoryRoot: root, ProjectKey: "project", VariationID: "config/default", + Kind: syncdomain.AttachmentTool, Key: "search", + }) + + require.NoError(t, err) + resources, err := synclocal.CompileWorkspace(root) + require.NoError(t, err) + require.Len(t, resources[0].Variation.Attachments, 1) + assert.Equal(t, localDescription, *resources[0].Variation.Attachments[0].Tool.Description) +} + func TestEmptyAttachmentSearchReturnsNoResults(t *testing.T) { client := syncapi.NewClient(emptyAttachmentClient{}, "token", "https://example.com") diff --git a/internal/sync/prompt/sync.go b/internal/sync/prompt/sync.go index 349983366..cec9598aa 100644 --- a/internal/sync/prompt/sync.go +++ b/internal/sync/prompt/sync.go @@ -61,9 +61,6 @@ func (runner Runner) runWorkspaceSync(options Options, workspace syncWorkspace, return err } if !proceed { - if !resolved.HasChanges() { - return cleanupOrphanedAttachments(options, workspace.local, interactive) - } return nil } @@ -75,7 +72,7 @@ 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, interactive) + return runner.applyPlan(options, workspace, client, current, conflicts.resolutions) } // checkUnchanged returns an error if the state changed after the review. @@ -93,15 +90,13 @@ func checkUnchanged(options Options, reviewed, current workspaceState) error { } // applyPlan applies the conflict choices to the current plan, executes it, -// and records the new baseline. It removes unused attachment files only when -// every step succeeds. +// and records the new baseline. func (runner Runner) applyPlan( options Options, workspace syncWorkspace, client syncapi.Client, current workspaceState, resolutions map[syncdomain.ResourceID]ConflictResolution, - interactive bool, ) error { plan := applyConflictResolutions(current.plan, resolutions) outcomes, next, err := executePlan(workspace.root, workspace.local, client, current.baseline.Lock, plan, current.localFiles) @@ -116,8 +111,5 @@ func (runner Runner) applyPlan( if err := writeOutcomeOutput(options.Output, options.OutputKind, plan, outcomes); err != nil { failures = append(failures, err) } - if err := errors.Join(failures...); err != nil { - return err - } - return cleanupOrphanedAttachments(options, workspace.local, interactive) + return errors.Join(failures...) } From bd1b7d883fcbcd875fe9c8b42c12d5579de14634 Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Fri, 23 Oct 2026 14:17:17 +0000 Subject: [PATCH 2/2] fix: archive only variations, and delete every detached file at once A detach with --archive tried to archive each selected resource as a variation. A tool or skill in the selection failed after the question, and variations before it were already archived while the local detach never ran. The archive now skips tools and skills, because LaunchDarkly keeps them. Detach deleted the variation files and the tool and skill files in two transactions. When the second one failed, the variation files were gone while the baseline came back, so local edits were lost. Store now has DeleteResources, which deletes the files of every kind in one transaction, and it replaces DeleteAttachments. --- internal/sync/detach/detach.go | 29 ++++++++--------- internal/sync/detach/detach_test.go | 44 ++++++++++++++++++++++++++ internal/sync/local/attachment.go | 13 -------- internal/sync/local/attachment_test.go | 12 ++++--- internal/sync/local/delete.go | 16 ++++++++++ internal/sync/local/layout.go | 13 ++++++++ internal/sync/prompt/execute.go | 2 +- 7 files changed, 95 insertions(+), 34 deletions(-) diff --git a/internal/sync/detach/detach.go b/internal/sync/detach/detach.go index 53f760a0f..8ee4aa2c6 100644 --- a/internal/sync/detach/detach.go +++ b/internal/sync/detach/detach.go @@ -264,6 +264,10 @@ func localChanges( // before the next archive when ctx ends. func archiveVariations(ctx context.Context, archiver Archiver, selected []syncdomain.ResourceID) error { for _, resource := range selected { + // LaunchDarkly keeps a detached tool or skill. + if resource.Kind != syncdomain.KindVariation { + continue + } if err := ctx.Err(); err != nil { return err } @@ -330,9 +334,10 @@ func loadResources( } // detachResources removes the selected resources from the baseline, and then -// deletes their local files. Every other entry stays, also the tools and -// skills that only the detached variations used, because they sync on their -// own. If the delete fails, it restores the baseline. +// deletes their local files in one transaction. Every other entry stays, also +// the tools and skills that only the detached variations used, because they +// sync on their own. If the delete fails, it keeps every file and restores the +// baseline. func detachResources(options Options, original syncmanifest.Baseline, selected []syncdomain.ResourceID) error { next := original.Lock.Clone() for _, id := range selected { @@ -347,7 +352,10 @@ func detachResources(options Options, original syncmanifest.Baseline, selected [ return errors.Join(cause, restoreErr) } - var deletions []synclocal.VariationDeletion + deletions, err := localAttachments(options.RepositoryRoot, selected) + if err != nil { + return restoreBaseline(err) + } for _, id := range selected { configKey, variationKey, err := id.VariationKeys() if err != nil { @@ -358,19 +366,10 @@ func detachResources(options Options, original syncmanifest.Baseline, selected [ return restoreBaseline(err) } if exists { - deletions = append(deletions, synclocal.VariationDeletion{ - ProjectKey: id.ProjectKey, ConfigKey: configKey, VariationKey: variationKey, - }) + deletions = append(deletions, id) } } - attachments, err := localAttachments(options.RepositoryRoot, selected) - if err != nil { - return restoreBaseline(err) - } - if _, err := options.Store.DeleteVariations(deletions); err != nil { - return restoreBaseline(err) - } - if _, err := options.Store.DeleteAttachments(attachments); err != nil { + if _, err := options.Store.DeleteResources(deletions); err != nil { return restoreBaseline(err) } return nil diff --git a/internal/sync/detach/detach_test.go b/internal/sync/detach/detach_test.go index e484fed40..d7865a1e1 100644 --- a/internal/sync/detach/detach_test.go +++ b/internal/sync/detach/detach_test.go @@ -440,6 +440,50 @@ func TestRunDetachesToolsAndSkills(t *testing.T) { assert.Equal(t, []syncdomain.ResourceID{syncdomain.VariationID("project", "config", "prompt")}, lockIDs(workspace.baselines.manifest)) } +// --archive archives only variations. LaunchDarkly keeps a detached tool or +// skill, so a tool in the same selection must not stop the archive or the +// detach. +func TestRunArchiveWithAToolArchivesOnlyTheVariation(t *testing.T) { + workspace := newArchiveWorkspace(t, "prompt") + tool := testTool() + workspace.addAttachments(t, tool) + archiver := &recordingArchiver{} + options := workspace.options(archiver) + options.Selections = append(options.Selections, tool.ID("project")) + options.Archive, options.Yes = true, true + + require.NoError(t, Run(options)) + + assert.Equal(t, []string{"project/config/prompt"}, archiver.archived) + workspace.requireTracked(t, "prompt", false) + assert.Empty(t, workspace.baselines.manifest.Resources) + local, err := synclocal.CompileAttachments(workspace.root, func(syncdomain.ResourceID) bool { return true }) + require.NoError(t, err) + assert.Empty(t, local) +} + +// Detach deletes the variation, tool, and skill files in one transaction. When +// one file cannot be deleted, every file stays, and so does the baseline. +func TestRunKeepsEveryFileWhenOneDeleteFails(t *testing.T) { + workspace := newArchiveWorkspace(t, "prompt") + tool := testTool() + workspace.addAttachments(t, tool) + // A symbolic link in place of the tool file makes its delete fail. + toolFile := filepath.Join(workspace.root, syncdomain.RootDir, "project", "tools", "search.json") + target := filepath.Join(t.TempDir(), "search.json") + require.NoError(t, os.Rename(toolFile, target)) + require.NoError(t, os.Symlink(target, toolFile)) + options := workspace.options(&recordingArchiver{}) + options.Selections = append(options.Selections, tool.ID("project")) + options.Yes = true + + require.Error(t, Run(options)) + + workspace.requireTracked(t, "prompt", true) + _, err := os.Lstat(toolFile) + require.NoError(t, err) +} + func TestRunRefusesAToolThatAVariationStillUses(t *testing.T) { workspace := newArchiveWorkspace(t) tool := testTool() diff --git a/internal/sync/local/attachment.go b/internal/sync/local/attachment.go index 8f227d557..ea3b57f53 100644 --- a/internal/sync/local/attachment.go +++ b/internal/sync/local/attachment.go @@ -282,16 +282,3 @@ func preserveLocalFields(filePath string, original, replacement []byte) ([]byte, updated.Name, updated.Upsert = local.Name, updated.Upsert || local.Upsert return renderSkillFile(updated, string(markdown)) } - -// DeleteAttachments deletes the files of tools and skills in one transaction. -func (store Store) DeleteAttachments(ids []syncdomain.ResourceID) ([]string, error) { - paths := make([]string, 0, len(ids)) - for _, id := range ids { - relativePath, err := attachmentPath(id.ProjectKey, syncdomain.AttachmentKind(id.Kind), id.LookupKey) - if err != nil { - return nil, err - } - paths = append(paths, relativePath) - } - return store.deleteFiles(paths) -} diff --git a/internal/sync/local/attachment_test.go b/internal/sync/local/attachment_test.go index 18cf9df0f..2d23151a0 100644 --- a/internal/sync/local/attachment_test.go +++ b/internal/sync/local/attachment_test.go @@ -241,24 +241,26 @@ func TestReplaceVariationsLeavesAttachmentUnchangedWhenPreflightFails(t *testing assert.Equal(t, oldDescription, *attachment.Tool.Description) } -func TestDeleteAttachmentsDeletesToolAndSkillFilesByID(t *testing.T) { +func TestDeleteResourcesDeletesTheFilesOfEveryKindByID(t *testing.T) { root := t.TempDir() store := NewStore(root) toolPath := filepath.Join(root, ".launchdarkly", "project", "tools", "search.json") skillPath := filepath.Join(root, ".launchdarkly", "project", "skills", "support.md") - for _, file := range []string{toolPath, skillPath} { + variationPath := filepath.Join(root, ".launchdarkly", "project", "configs", "config", "default.prompt.md") + for _, file := range []string{toolPath, skillPath, variationPath} { require.NoError(t, os.MkdirAll(filepath.Dir(file), 0o755)) require.NoError(t, os.WriteFile(file, []byte("content\n"), 0o644)) } - deleted, err := store.DeleteAttachments([]syncdomain.ResourceID{ + deleted, err := store.DeleteResources([]syncdomain.ResourceID{ {Kind: syncdomain.KindTool, ProjectKey: "project", LookupKey: "search"}, {Kind: syncdomain.KindSkill, ProjectKey: "project", LookupKey: "support"}, + syncdomain.VariationID("project", "config", "default"), }) require.NoError(t, err) - assert.Equal(t, []string{"project/tools/search.json", "project/skills/support.md"}, deleted) - for _, file := range []string{toolPath, skillPath} { + assert.Equal(t, []string{"project/tools/search.json", "project/skills/support.md", "project/configs/config/default.prompt.md"}, deleted) + for _, file := range []string{toolPath, skillPath, variationPath} { _, statErr := os.Stat(file) require.ErrorIs(t, statErr, os.ErrNotExist) } diff --git a/internal/sync/local/delete.go b/internal/sync/local/delete.go index 18750c2b2..bc6985153 100644 --- a/internal/sync/local/delete.go +++ b/internal/sync/local/delete.go @@ -5,6 +5,8 @@ import ( "fmt" "os" "path/filepath" + + syncdomain "github.com/launchdarkly/ldcli/internal/sync" ) // stagedDeletion is one file in a delete transaction. The file first moves to @@ -28,6 +30,20 @@ func (store Store) DeleteVariations(deletions []VariationDeletion) ([]string, er return store.deleteFiles(paths) } +// DeleteResources deletes the files of variations, tools, and skills in one +// transaction, so that a failure deletes none of them. It returns their paths. +func (store Store) DeleteResources(ids []syncdomain.ResourceID) ([]string, error) { + paths := make([]string, 0, len(ids)) + for _, id := range ids { + relativePath, err := resourcePath(id) + if err != nil { + return nil, err + } + paths = append(paths, relativePath) + } + return store.deleteFiles(paths) +} + // deleteFiles deletes a batch of managed files in one transaction. It first // moves every file to a backup beside it. If a move fails, it moves the // files back. When every file has moved, it removes the backups. diff --git a/internal/sync/local/layout.go b/internal/sync/local/layout.go index f992d7d35..29506dc6b 100644 --- a/internal/sync/local/layout.go +++ b/internal/sync/local/layout.go @@ -91,6 +91,19 @@ func attachmentPath(projectKey string, kind syncdomain.AttachmentKind, key strin } } +// resourcePath returns the path of the file of a variation, a tool, or a +// skill relative to the managed directory. +func resourcePath(id syncdomain.ResourceID) (string, error) { + if id.Kind != syncdomain.KindVariation { + return attachmentPath(id.ProjectKey, syncdomain.AttachmentKind(id.Kind), id.LookupKey) + } + configKey, variationKey, err := id.VariationKeys() + if err != nil { + return "", err + } + return variationPath(id.ProjectKey, configKey, variationKey) +} + // validateKey makes sure that key is a safe path segment. The name is the kind // of key, for example "project", and appears in the error. func validateKey(name, key string) error { diff --git a/internal/sync/prompt/execute.go b/internal/sync/prompt/execute.go index e4aa82bdb..78cca748a 100644 --- a/internal/sync/prompt/execute.go +++ b/internal/sync/prompt/execute.go @@ -286,7 +286,7 @@ func applyAttachmentChange( }); index >= 0 { return 0, fmt.Errorf("%s still uses it", variations[index].ID().Label()) } - if _, err := localStore.DeleteAttachments([]syncdomain.ResourceID{resource.ID}); err != nil { + if _, err := localStore.DeleteResources([]syncdomain.ResourceID{resource.ID}); err != nil { return 0, err } return 0, verifyLocalResult(repositoryRoot, resource)