From 276a9523e7a52d88fed45a1d0767340091074a82 Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Thu, 22 Oct 2026 16:03:55 +0000 Subject: [PATCH 1/3] feat: apply tool and skill rows in a sync executePlan now applies the tool and skill rows of a plan. A push or a create publishes through the attachment resolver, so a tool is written once and every variation in the same plan pins the new version. A pull writes the standalone file and keeps its local fields. A deletion runs after every variation row, and keeps a file that a local variation still uses. A sync no longer drops the sync.lock rows of tools and skills that no variation uses. Results show the new version of a tool or skill, and the versions that a pushed variation pins. JSON results add version. --- internal/sync/local/attachment.go | 13 +- internal/sync/local/attachment_test.go | 30 +++- internal/sync/local/replace.go | 72 ++++++--- internal/sync/prompt/acceptance_test.go | 83 +++++++++- internal/sync/prompt/execute.go | 192 +++++++++++++++++------- internal/sync/prompt/execute_test.go | 113 +++++++++++++- internal/sync/prompt/output.go | 25 ++- internal/sync/prompt/output_test.go | 20 ++- internal/sync/prompt/review.go | 15 +- internal/sync/prompt/sync.go | 4 +- 10 files changed, 467 insertions(+), 100 deletions(-) diff --git a/internal/sync/local/attachment.go b/internal/sync/local/attachment.go index 2f44b79e..89235950 100644 --- a/internal/sync/local/attachment.go +++ b/internal/sync/local/attachment.go @@ -347,17 +347,14 @@ func (store Store) OrphanedAttachments() ([]OrphanedAttachment, error) { return orphaned, nil } -// DeleteAttachments deletes tool and skill files in one transaction. -func (store Store) DeleteAttachments(attachments []OrphanedAttachment) ([]string, error) { - paths := make([]string, 0, len(attachments)) - for _, attachment := range attachments { - relativePath, err := attachmentPath(attachment.ProjectKey, attachment.Kind, attachment.Key) +// 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 } - if relativePath != attachment.Path { - return nil, fmt.Errorf("attachment path %q does not match %q", attachment.Path, relativePath) - } 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 822d4af9..ce973489 100644 --- a/internal/sync/local/attachment_test.go +++ b/internal/sync/local/attachment_test.go @@ -277,7 +277,7 @@ func TestOrphanedAttachmentsRequireEveryLocalReferenceToBeRemoved(t *testing.T) ProjectKey: "project", Kind: syncdomain.AttachmentTool, Key: "search", Path: "project/tools/search.json", }}, orphaned) - deleted, err := store.DeleteAttachments(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")) @@ -298,7 +298,7 @@ func TestOrphanedAttachmentsFindsFlatSkillFile(t *testing.T) { ProjectKey: "project", Kind: syncdomain.AttachmentSkill, Key: "support", Path: "project/skills/support.md", }}, orphaned) - deleted, err := store.DeleteAttachments(orphaned) + 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) @@ -355,6 +355,32 @@ func TestCompileAttachmentsRejectsSymlink(t *testing.T) { require.ErrorContains(t, err, "symbolic links are not supported") } +func TestReplaceAttachmentsWritesStandaloneFilesAndKeepsLocalFields(t *testing.T) { + root := t.TempDir() + writeManagedFile(t, root, "project/tools/search.json", `{"formatVersion": 1, "upsert": true, "key": "search", "schema": {}}`) + description := "Search the docs" + search := syncdomain.Attachment{Kind: syncdomain.AttachmentTool, Version: 4, Tool: &syncdomain.Tool{ + Key: "search", Description: &description, Schema: map[string]any{"type": "object"}, + }} + triage := syncdomain.Attachment{Kind: syncdomain.AttachmentSkill, Skill: &syncdomain.Skill{ + Key: "triage", Name: "Triage", Description: "Triage", Markdown: "Pick a queue.\n", + }} + + written, err := NewStore(root).ReplaceAttachments([]syncdomain.SyncedAttachment{ + {ProjectKey: "project", Attachment: search}, {ProjectKey: "project", Attachment: triage}, + }) + + require.NoError(t, err) + assert.Equal(t, []string{"project/tools/search.json", "project/skills/triage.md"}, written) + attachments, err := CompileAttachments(root, func(syncdomain.ResourceID) bool { return true }) + require.NoError(t, err) + require.Len(t, attachments, 2) + assert.Equal(t, "Pick a queue.\n", attachments[0].Attachment.Skill.Markdown) + assert.Empty(t, attachments[0].Attachment.Skill.Name, "the file does not get the LaunchDarkly name") + assert.Equal(t, "Search the docs", *attachments[1].Attachment.Tool.Description) + assert.True(t, attachments[1].Attachment.Upsert, "the file keeps its upsert flag") +} + func writeManagedFile(t *testing.T, root, relativePath, content string) { t.Helper() path := filepath.Join(root, syncdomain.RootDir, filepath.FromSlash(relativePath)) diff --git a/internal/sync/local/replace.go b/internal/sync/local/replace.go index 3d83aa5b..b10f1910 100644 --- a/internal/sync/local/replace.go +++ b/internal/sync/local/replace.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" + syncdomain "github.com/launchdarkly/ldcli/internal/sync" syncreference "github.com/launchdarkly/ldcli/internal/sync/reference" ) @@ -37,27 +38,65 @@ func (store Store) ReplaceVariations(replacements []VariationReplacement) ([]str if err != nil { return nil, err } - if err := stageFiles(files); err != nil { + if err := replaceFiles(files); err != nil { return nil, err } - defer removeStagedFiles(files) - if err := verifyOriginals(files); err != nil { + var paths []string + for _, file := range files { + if file.isVariationFile { + paths = append(paths, file.displayPath) + } + } + return paths, nil +} + +// ReplaceAttachments writes the content of each tool and skill file in one +// transaction, like ReplaceVariations. A missing file is created, and an +// existing file keeps its local fields. It returns the paths that it wrote. +func (store Store) ReplaceAttachments(attachments []syncdomain.SyncedAttachment) ([]string, error) { + rendered := make([]RenderedFile, 0, len(attachments)) + for _, attachment := range attachments { + filePath, err := attachmentPath(attachment.ProjectKey, attachment.Attachment.Kind, attachment.Attachment.Key()) + if err != nil { + return nil, err + } + content, err := renderAttachment(attachment.Attachment) + if err != nil { + return nil, err + } + rendered = append(rendered, RenderedFile{Path: filePath, Content: content}) + } + files, err := store.prepareAttachmentFiles(rendered) + if err != nil { return nil, err } - if err := commitFiles(files); err != nil { + if err := replaceFiles(files); err != nil { return nil, err } - var paths []string + paths := make([]string, 0, len(files)) for _, file := range files { - if file.isVariationFile { - paths = append(paths, file.displayPath) - } + paths = append(paths, file.displayPath) } return paths, nil } +// replaceFiles writes the staged files in one transaction. It writes every +// file to a temporary path, makes sure that no original changed, and then +// renames each file into place. +func replaceFiles(files []stagedFile) error { + if err := stageFiles(files); err != nil { + return err + } + defer removeStagedFiles(files) + + if err := verifyOriginals(files); err != nil { + return err + } + return commitFiles(files) +} + // ReplaceFileAtomically replaces the file at path only if its content is // still original. displayPath names the file in errors. func ReplaceFileAtomically(path, displayPath string, original, replacement []byte, mode os.FileMode) error { @@ -136,7 +175,11 @@ func (store Store) prepareReplacements(replacements []VariationReplacement) ([]s }) } - attachments, err := store.prepareAttachmentFiles(attachmentSources) + rendered, err := renderAttachmentFiles(attachmentSources) + if err != nil { + return nil, err + } + attachments, err := store.prepareAttachmentFiles(rendered) if err != nil { return nil, err } @@ -188,14 +231,9 @@ func (store Store) prepareLinkedFile(ref Reference, replacement VariationReplace }, nil } -// prepareAttachmentFiles renders each tool and skill file that changed. A new -// file is created. An existing file keeps its local fields. -func (store Store) prepareAttachmentFiles(files []VariationFile) ([]stagedFile, error) { - rendered, err := renderAttachmentFiles(files) - if err != nil { - return nil, err - } - +// prepareAttachmentFiles stages each rendered tool and skill file that +// changed. A new file is created. An existing file keeps its local fields. +func (store Store) prepareAttachmentFiles(rendered []RenderedFile) ([]stagedFile, error) { var staged []stagedFile for _, file := range rendered { path := store.absolute(file.Path) diff --git a/internal/sync/prompt/acceptance_test.go b/internal/sync/prompt/acceptance_test.go index afc948c4..e244361c 100644 --- a/internal/sync/prompt/acceptance_test.go +++ b/internal/sync/prompt/acceptance_test.go @@ -573,7 +573,14 @@ func TestPromptDeletesUnreferencedLocalToolWithYes(t *testing.T) { assert.Contains(t, api.tools, "search") _, err = os.Stat(filepath.Join(root, ".launchdarkly", "production", "tools", "search.json")) require.ErrorIs(t, err, os.ErrNotExist) - assertManifestFingerprint(t, root, detached) + // A sync keeps the row of a tool that no variation uses. Only a detach + // removes it. + 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) } func TestPromptUpdateResolvesOmittedModelConfigVersionToLatest(t *testing.T) { @@ -1180,7 +1187,7 @@ func TestPromptPlansToolAndSkillRowsByScope(t *testing.T) { t.Run(string(scope), func(t *testing.T) { api.requests = nil - stdout, err := runSyncAction(t, root, api, syncprompt.SyncAction{DryRun: true, Scope: scope}) + stdout, err := runSyncAction(t, root, api, syncprompt.SyncAction{DryRun: true, Scope: scope}, false) require.NoError(t, err) assert.Equal(t, want, planRows(t, stdout)) @@ -1197,7 +1204,7 @@ func TestPromptFiltersToolRowsBeforeAnyRead(t *testing.T) { root, api := writeStandaloneAttachmentWorkspace(t) filter := syncdomain.Filter{Project: "production", Keys: []string{"lookup"}} - stdout, err := runSyncAction(t, root, api, syncprompt.SyncAction{DryRun: true, Scope: syncdomain.ScopeTools, Filter: filter}) + stdout, err := runSyncAction(t, root, api, syncprompt.SyncAction{DryRun: true, Scope: syncdomain.ScopeTools, Filter: filter}, false) require.NoError(t, err) assert.Equal(t, []string{"tool lookup update_manifest"}, planRows(t, stdout)) @@ -1221,12 +1228,74 @@ func TestPromptFiltersToolRowsBeforeAnyRead(t *testing.T) { }, } { api.requests = nil - _, err := runSyncAction(t, root, api, syncprompt.SyncAction{DryRun: true, Scope: syncdomain.ScopeTools, Filter: test.filter}) + _, err := runSyncAction(t, root, api, syncprompt.SyncAction{DryRun: true, Scope: syncdomain.ScopeTools, Filter: test.filter}, false) require.EqualError(t, err, test.want) assert.Empty(t, api.requests) } } +func TestPromptSyncsStandaloneToolsAndSkills(t *testing.T) { + root, api := writeStandaloneAttachmentWorkspace(t) + writeLocalFile(t, root, "tools/search.json", `{"formatVersion": 1, "key": "search", "description": "Search", "schema": {"type": "object"}}`) + all := syncprompt.SyncAction{Scope: syncdomain.ScopeAll} + + // One plan pushes the tool once, creates the new tool, records the rest, + // and then pins the new tool version in the variation. + stdout, err := runSyncAction(t, root, api, all, true) + require.NoError(t, err) + assert.Contains(t, stdout, `"lookupKey": "search", + "action": "update_server", + "status": "succeeded", + "version": 2`) + assert.Equal(t, 1, countRequests(api.requests, "PATCH", "/ai-tools/search")) + assert.Equal(t, 1, countRequests(api.requests, "POST", "/ai-tools")) + assert.Equal(t, 1, countRequests(api.requests, "PATCH", "/configs/sync/manifests")) + assert.Less(t, slices.Index(api.requests, "PATCH https://example.test/api/v2/projects/production/ai-tools/search"), + slices.Index(api.requests, "PATCH https://example.test/api/v2/projects/production/ai-configs/support/variations/default")) + assert.Equal(t, 1, api.tools["refund"].Version) + assert.Equal(t, []syncdomain.AttachmentRef{{Key: "search", Version: 2}}, api.variation.Tools) + assert.Len(t, readLock(t, root).Resources, 6, "the lock keeps every tool and skill, also the ones that no variation uses") + + api.requests = nil + stdout, err = runSyncAction(t, root, api, syncprompt.SyncAction{DryRun: true, Scope: syncdomain.ScopeAll}, false) + require.NoError(t, err) + for _, row := range planRows(t, stdout) { + assert.True(t, strings.HasSuffix(row, " in_sync"), row) + } + + // A change in LaunchDarkly pulls into the standalone file. + lookup := api.tools["lookup"] + lookup.Description = pointer("Look up an order") + lookup.Version++ + api.tools["lookup"] = lookup + _, err = runSyncAction(t, root, api, syncprompt.SyncAction{Scope: syncdomain.ScopeTools}, true) + require.NoError(t, err) + content, err := os.ReadFile(filepath.Join(root, syncdomain.RootDir, "production", "tools", "lookup.json")) + require.NoError(t, err) + assert.Contains(t, string(content), "Look up an order") + + // A hand-written skill file with upsert creates the skill. + writeLocalFile(t, root, "skills/handoff.md", "---\nkey: handoff\ndescription: Hand off\nname: Human handoff\nupsert: true\n---\n\n# Handoff\n") + _, err = runSyncAction(t, root, api, syncprompt.SyncAction{Scope: syncdomain.ScopeSkills}, true) + require.NoError(t, err) + assert.Equal(t, "Human handoff", api.skills["handoff"].Name) + + // A deletion in LaunchDarkly deletes the standalone file and its baseline. + delete(api.tools, "lookup") + _, err = runSyncAction(t, root, api, syncprompt.SyncAction{Scope: syncdomain.ScopeTools}, true) + require.NoError(t, err) + _, err = os.Stat(filepath.Join(root, syncdomain.RootDir, "production", "tools", "lookup.json")) + assert.ErrorIs(t, err, os.ErrNotExist) + assert.NotContains(t, string(must(os.ReadFile(filepath.Join(root, syncdomain.RootDir, "sync.lock")))), "key: lookup") +} + +func must[T any](value T, err error) T { + if err != nil { + panic(err) + } + return value +} + // writeStandaloneAttachmentWorkspace syncs a variation that uses the tool // "search" and the skill "summarize". It also writes three tool and skill // files that no variation uses: "lookup" and "triage", which LaunchDarkly @@ -1266,13 +1335,15 @@ func writeStandaloneAttachmentWorkspace(t *testing.T) (string, *directAPI) { } } -func runSyncAction(t *testing.T, root string, api *directAPI, action syncprompt.SyncAction) (string, error) { +// runSyncAction runs one sync through Runner.Run, because no command selects +// a scope yet. It returns the JSON output. +func runSyncAction(t *testing.T, root string, api *directAPI, action syncprompt.SyncAction, yes bool) (string, error) { t.Helper() api.manifest = manifestsByRoot[root] var output bytes.Buffer err := syncprompt.NewRunner(api).Run(syncprompt.Options{ WorkingDirectory: root, AccessToken: "token", BaseURI: "https://example.test", OutputKind: "json", - Action: action, Input: strings.NewReader(""), Output: &output, ErrorOutput: io.Discard, + Action: action, Yes: yes, Input: strings.NewReader(""), Output: &output, ErrorOutput: io.Discard, }) return output.String(), err } diff --git a/internal/sync/prompt/execute.go b/internal/sync/prompt/execute.go index 1696254e..efad982f 100644 --- a/internal/sync/prompt/execute.go +++ b/internal/sync/prompt/execute.go @@ -3,6 +3,7 @@ package prompt import ( "errors" "fmt" + "slices" syncdomain "github.com/launchdarkly/ldcli/internal/sync" syncapi "github.com/launchdarkly/ldcli/internal/sync/api" @@ -13,8 +14,7 @@ import ( // executePlan applies each action of the plan, and returns the outcomes and // the new manifest. The manifest records only the actions that succeed. The // manifest argument does not change. localFiles has each local variation as -// its file stores it. filter is the filter of the plan. A filtered plan keeps -// every tool and skill entry, because it does not see the other variations. +// its file stores it. func executePlan( repositoryRoot string, localStore synclocal.Store, @@ -22,7 +22,6 @@ func executePlan( manifest syncmanifest.Manifest, plan Plan, localFiles map[ResourceID]syncdomain.SyncedResource, - filter syncdomain.Filter, ) ([]ResourceOutcome, syncmanifest.Manifest, error) { // A conflict choice applies to the full plan. Write nothing until every // conflict has a choice, so that a shared attachment cannot change while @@ -32,16 +31,17 @@ func executePlan( } next := manifest.Clone() - outcomes := make([]ResourceOutcome, 0, len(plan.Resources)) + outcomes := make([]ResourceOutcome, len(plan.Resources)) var failures []error attachments := newAttachmentResolver(client) - fail := func(outcome *ResourceOutcome, err error) { - outcome.Status, outcome.Error = OutcomeFailed, err.Error() - failures = append(failures, fmt.Errorf("%s: %w", outcome.ID, err)) - } - - for _, resource := range plan.Resources { + execute := func(index int) { + resource := plan.Resources[index] outcome := ResourceOutcome{ID: resource.ID, Action: resource.Action, Status: OutcomeSucceeded} + fail := func(err error) { + outcome.Status, outcome.Error = OutcomeFailed, err.Error() + failures = append(failures, fmt.Errorf("%s: %w", outcome.ID, err)) + } + switch { case resource.Action == ActionInSync: case resource.Action == ActionUpdateManifest: @@ -49,14 +49,23 @@ func executePlan( case resource.Action == ActionRemoveManifest: next.Remove(resource.ID) case resource.Action.changesServer() || resource.Action.changesLocal(): - var localFile *syncdomain.Variation - if file, ok := localFiles[resource.ID]; ok { - localFile = &file.Variation + var pins []syncdomain.AttachmentRef + var version int + var err error + if resource.ID.Kind == syncdomain.KindVariation { + var localFile *syncdomain.Variation + if file, ok := localFiles[resource.ID]; ok { + localFile = &file.Variation + } + pins, err = applyResourceChange(repositoryRoot, localStore, client, attachments, resource, localFile) + } else { + version, err = applyAttachmentChange(repositoryRoot, localStore, attachments, resource) } - if err := applyResourceChange(repositoryRoot, localStore, client, attachments, resource, localFile); err != nil { - fail(&outcome, err) + if err != nil { + fail(err) break } + outcome.pins, outcome.Version = pins, version recordSuccessfulChange(&next, resource) default: outcome.Status, outcome.Error = OutcomeSkipped, "resource is not executable" @@ -65,11 +74,26 @@ func executePlan( if outcome.Status == OutcomeSucceeded { if variation := attachmentManifestState(resource); variation != nil { if err := next.SetAttachments(resource.ID.ProjectKey, variation.Attachments); err != nil { - fail(&outcome, err) + fail(err) } } } - outcomes = append(outcomes, outcome) + outcomes[index] = outcome + } + + // The plan puts tools and skills before the variations that pin them. A + // deletion of a tool or a skill runs last instead, so that the pull of a + // variation that drops the reference runs first. + var deletions []int + for index, resource := range plan.Resources { + if resource.ID.Kind != syncdomain.KindVariation && resource.Action == ActionDeleteLocal { + deletions = append(deletions, index) + continue + } + execute(index) + } + for _, index := range deletions { + execute(index) } if len(failures) == 0 { @@ -77,9 +101,6 @@ func executePlan( if err != nil { failures = append(failures, err) } else { - if filter.IsZero() { - next.RemoveUnusedAttachments(variations) - } next.SetRefs(variations) } } @@ -112,7 +133,9 @@ func recordSuccessfulChange(manifest *syncmanifest.Manifest, resource PlannedRes } // applyResourceChange applies one action that writes to LaunchDarkly or to a -// local file. localFile is the variation as its file stores it, or nil. +// local file. localFile is the variation as its file stores it, or nil. It +// returns the tool and skill versions that the variation pins when the action +// writes the variation to LaunchDarkly. func applyResourceChange( repositoryRoot string, localStore synclocal.Store, @@ -120,7 +143,7 @@ func applyResourceChange( attachments *attachmentResolver, resource PlannedResource, localFile *syncdomain.Variation, -) error { +) ([]syncdomain.AttachmentRef, error) { if resource.Action.changesServer() { return applyServerChange(client, attachments, resource) } @@ -134,10 +157,10 @@ func applyResourceChange( resource.Server = &variation } if err := applyLocalChange(localStore, resource); err != nil { - return err + return nil, err } if err := verifyLocalResult(repositoryRoot, resource); err != nil { - return err + return nil, err } // The local file now matches LaunchDarkly. If LaunchDarkly pins an older @@ -145,69 +168,125 @@ func applyResourceChange( if resource.ServerHasStaleAttachmentPins && server != nil { configKey, _, err := resource.ID.VariationKeys() if err != nil { - return err + return nil, err } pinned, err := server.PinnedToLatest() if err != nil { - return err + return nil, err } - return client.UpdateVariation(resource.ID.ProjectKey, configKey, pinned) + return pinsOf(pinned), client.UpdateVariation(resource.ID.ProjectKey, configKey, pinned) } - return nil + return nil, nil } // applyServerChange writes one variation to LaunchDarkly. If the write fails // without a response, or a create gets a 409 because the variation exists // now, it reads the variation to find whether LaunchDarkly has the content. -func applyServerChange(client syncapi.Client, attachments *attachmentResolver, resource PlannedResource) error { +func applyServerChange( + client syncapi.Client, + attachments *attachmentResolver, + resource PlannedResource, +) ([]syncdomain.AttachmentRef, error) { configKey, variationKey, err := resource.ID.VariationKeys() if err != nil { - return err + return nil, err } projectKey := resource.ID.ProjectKey + var variation syncdomain.Variation var writeErr error switch resource.Action { case ActionCreateServer: - variation, err := attachments.resolveVariation(projectKey, *resource.Local) - if err != nil { - return err + if variation, err = attachments.resolveVariation(projectKey, *resource.Local); err != nil { + return nil, err } writeErr = client.CreateVariation(projectKey, configKey, variation) case ActionUpdateServer: - variation, err := attachments.resolveVariation(projectKey, withExplicitDetach(*resource.Local, resource.Server)) - if err != nil { - return err + local := withExplicitDetach(*resource.Local, resource.Server) + if variation, err = attachments.resolveVariation(projectKey, local); err != nil { + return nil, err } writeErr = client.UpdateVariation(projectKey, configKey, variation) default: - return fmt.Errorf("action %q does not change LaunchDarkly", resource.Action) + return nil, fmt.Errorf("action %q does not change LaunchDarkly", resource.Action) + } + if writeErr == nil { + return pinsOf(variation), nil } createConflict := resource.Action == ActionCreateServer && syncapi.IsConflict(writeErr) - if writeErr == nil || !syncapi.MutationMayHaveSucceeded(writeErr) && !createConflict { - return writeErr + if !syncapi.MutationMayHaveSucceeded(writeErr) && !createConflict { + return nil, writeErr } state, err := client.ReadVariation(projectKey, configKey, variationKey) if err != nil { - return errors.Join(writeErr, fmt.Errorf("verify server variation: %w", err)) + return nil, errors.Join(writeErr, fmt.Errorf("verify server variation: %w", err)) } actual := "" if state.Exists { if err := newAttachmentCache(client).hydrate(projectKey, &state.Variation); err != nil { - return errors.Join(writeErr, err) + return nil, errors.Join(writeErr, err) } if actual, err = syncdomain.FingerprintVariation(projectKey, resource.ID.LookupKey, state.Variation); err != nil { - return errors.Join(writeErr, err) + return nil, errors.Join(writeErr, err) } } switch actual { case resource.LocalFingerprint: - return nil + return pinsOf(variation), nil case resource.ServerFingerprint: - return writeErr + return nil, writeErr default: - return fmt.Errorf("server variation changed concurrently after an uncertain write: %w", writeErr) + return nil, fmt.Errorf("server variation changed concurrently after an uncertain write: %w", writeErr) + } +} + +// pinsOf returns the tool and skill references of a variation with their versions. +func pinsOf(variation syncdomain.Variation) []syncdomain.AttachmentRef { + return slices.Concat(variation.Tools, variation.Skills) +} + +// applyAttachmentChange applies the action of one tool or skill row, and +// returns the version that LaunchDarkly has after the action. +func applyAttachmentChange( + repositoryRoot string, + localStore synclocal.Store, + attachments *attachmentResolver, + resource PlannedResource, +) (int, error) { + switch resource.Action { + 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. + local := *resource.LocalAttachment + local.Upsert = local.Upsert || resource.Action == ActionCreateServer + published, err := attachments.resolve(resource.ID.ProjectKey, local) + return published.Version, err + case ActionUpdateLocal: + server := syncdomain.SyncedAttachment{ProjectKey: resource.ID.ProjectKey, Attachment: *resource.ServerAttachment} + if _, err := localStore.ReplaceAttachments([]syncdomain.SyncedAttachment{server}); err != nil { + return 0, err + } + return server.Attachment.Version, verifyLocalResult(repositoryRoot, resource) + case ActionDeleteLocal: + // The pull of a variation that drops the reference ran first. If that + // pull failed, the variation still uses the file, so keep it. + variations, err := compileWorkspace(repositoryRoot) + if err != nil { + return 0, err + } + if index := slices.IndexFunc(variations, func(variation syncdomain.SyncedResource) bool { + return uses(variation, resource.ID) + }); index >= 0 { + return 0, fmt.Errorf("%s still uses it", variations[index].ID().Label()) + } + if _, err := localStore.DeleteAttachments([]syncdomain.ResourceID{resource.ID}); err != nil { + return 0, err + } + return 0, verifyLocalResult(repositoryRoot, resource) + default: + return 0, fmt.Errorf("action %q does not change a tool or a skill", resource.Action) } } @@ -256,8 +335,8 @@ func applyLocalChange(store synclocal.Store, resource PlannedResource) error { } // verifyLocalResult makes sure that the local file has the state that the -// action wrote. For ActionUpdateLocal, resource.Server is in the form of the -// local file. +// action wrote. For ActionUpdateLocal on a variation, resource.Server is in +// the form of the local file. func verifyLocalResult(repositoryRoot string, resource PlannedResource) error { actual, err := readLocalFingerprint(repositoryRoot, resource.ID) if err != nil { @@ -265,21 +344,32 @@ func verifyLocalResult(repositoryRoot string, resource PlannedResource) error { } expected := "" - if resource.Action == ActionUpdateLocal { + switch { + case resource.Action != ActionUpdateLocal: + case resource.ID.Kind == syncdomain.KindVariation: expected, err = syncdomain.FingerprintVariation(resource.ID.ProjectKey, resource.ID.LookupKey, *resource.Server) if err != nil { return err } + default: + expected = resource.ServerFingerprint } if actual != expected { - return errors.New("local variation did not match the expected state after sync") + return fmt.Errorf("local %s did not match the expected state after sync", resource.ID.Kind) } return nil } -// readLocalFingerprint returns the fingerprint of one local variation, or an -// empty string when the variation has no local file. +// readLocalFingerprint returns the fingerprint of one local resource, or an +// empty string when the resource has no local file. func readLocalFingerprint(repositoryRoot string, id ResourceID) (string, error) { + if id.Kind != syncdomain.KindVariation { + attachments, err := synclocal.CompileAttachments(repositoryRoot, func(candidate ResourceID) bool { return candidate == id }) + if err != nil || len(attachments) == 0 { + return "", err + } + return syncdomain.FingerprintAttachment(id.ProjectKey, attachments[0].Attachment) + } variations, err := compileWorkspace(repositoryRoot) if err != nil { return "", err diff --git a/internal/sync/prompt/execute_test.go b/internal/sync/prompt/execute_test.go index b4bac9c5..06abaeec 100644 --- a/internal/sync/prompt/execute_test.go +++ b/internal/sync/prompt/execute_test.go @@ -4,6 +4,8 @@ import ( "encoding/json" "errors" "net/url" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -31,7 +33,7 @@ func TestExecutePlanDoesNotMutateReviewedManifest(t *testing.T) { LocalFingerprint: "updated", }}} - _, updatedManifest, err := executePlan("", synclocal.Store{}, syncapi.Client{}, reviewedManifest, plan, nil, syncdomain.Filter{}) + _, updatedManifest, err := executePlan("", synclocal.Store{}, syncapi.Client{}, reviewedManifest, plan, nil) require.NoError(t, err) assert.Equal(t, "reviewed", reviewedManifest.Resources[0].Fingerprint) @@ -48,7 +50,7 @@ func TestApplyServerChangeDoesNotRereadAfterDefinitiveAPIError(t *testing.T) { } client := syncapi.NewClient(transport, "token", "https://example.com") - err := applyServerChange(client, newAttachmentResolver(client), resource) + _, err := applyServerChange(client, newAttachmentResolver(client), resource) require.ErrorContains(t, err, `"statusCode":400`) assert.Zero(t, transport.reads) @@ -77,7 +79,7 @@ func TestExecutePlanDoesNotMutateWhileConflictIsUnresolved(t *testing.T) { }, }} - outcomes, _, err := executePlan("", synclocal.Store{}, client, syncmanifest.New(), plan, nil, syncdomain.Filter{}) + outcomes, _, err := executePlan("", synclocal.Store{}, client, syncmanifest.New(), plan, nil) require.ErrorContains(t, err, "cannot sync conflicted resource") assert.Empty(t, outcomes) @@ -119,7 +121,7 @@ func TestExecutePlanSkipsEveryConsumerWhenSharedAttachmentFails(t *testing.T) { syncdomain.ResourceID{Kind: syncdomain.KindTool, ProjectKey: "project", LookupKey: "search"}, "sha256:reviewed", ) - outcomes, manifest, err := executePlan("", synclocal.Store{}, client, originalManifest, plan, nil, syncdomain.Filter{}) + outcomes, manifest, err := executePlan("", synclocal.Store{}, client, originalManifest, plan, nil) require.Error(t, err) require.Len(t, outcomes, 2) @@ -191,7 +193,7 @@ func TestApplyResourceChangeRefreshesPinsWithCompleteServerModel(t *testing.T) { transport := &variationUpdateAPI{} client := syncapi.NewClient(transport, "token", "https://example.com") - err = applyResourceChange( + pins, err := applyResourceChange( root, store, client, @@ -211,6 +213,7 @@ func TestApplyResourceChangeRefreshesPinsWithCompleteServerModel(t *testing.T) { assert.Equal(t, map[string]any{"modelName": "claude-sonnet", "temperature": 0.2}, request.Model) require.Len(t, request.Tools, 1) assert.Equal(t, 2, request.Tools[0].Version) + assert.Equal(t, []syncdomain.AttachmentRef{{Key: "search", Version: 2}}, pins) } type variationUpdateAPI struct { @@ -367,7 +370,7 @@ func TestApplyServerChangeReadsAgainAfterAVariationCreateConflict(t *testing.T) require.NoError(t, err) client := syncapi.NewClient(&createConflictAPI{remote: string(remote)}, "token", "https://example.com") - err = applyServerChange(client, newAttachmentResolver(client), resource) + _, err = applyServerChange(client, newAttachmentResolver(client), resource) if test.wantError != "" { require.ErrorContains(t, err, test.wantError) @@ -402,3 +405,101 @@ func (api *createConflictAPI) MakeRequest(_ string, method string, _ string, _ s func (*createConflictAPI) MakeUnauthenticatedRequest(string, string, []byte) ([]byte, error) { return nil, nil } + +func TestExecutePlanAppliesToolRows(t *testing.T) { + tool := func(description string, version int) *syncdomain.Attachment { + return &syncdomain.Attachment{Kind: syncdomain.AttachmentTool, Version: version, Tool: &syncdomain.Tool{ + Key: "search", Description: &description, Schema: map[string]any{"type": "object"}, + }} + } + baseline := *tool("Old", 0) + id := baseline.ID("production") + + tests := map[string]struct { + local, server *syncdomain.Attachment + wantAction Action + wantVersion int + wantDescription string + }{ + "push": {local: tool("New", 0), server: tool("Old", 2), wantAction: ActionUpdateServer, wantVersion: 3, wantDescription: "New"}, + "pull": {local: tool("Old", 0), server: tool("New", 5), wantAction: ActionUpdateLocal, wantVersion: 5, wantDescription: "New"}, + "delete": {local: tool("Old", 0), wantAction: ActionDeleteLocal}, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + root := t.TempDir() + store := synclocal.NewStore(root) + _, err := store.ReplaceAttachments([]syncdomain.SyncedAttachment{{ProjectKey: "production", Attachment: *test.local}}) + require.NoError(t, err) + resource := planAttachment(id, test.local, test.server, attachmentFingerprint(t, baseline), true) + require.Equal(t, test.wantAction, resource.Action) + transport := &attachmentMutationAPI{current: *tool("Old", 0).Tool, version: 2} + client := syncapi.NewClient(transport, "token", "https://example.com") + manifest := syncmanifest.New() + manifest.SetFingerprint(id, resource.BaselineFingerprint) + + outcomes, next, err := executePlan(root, store, client, manifest, Plan{Resources: []PlannedResource{resource}}, nil) + + require.NoError(t, err) + require.Len(t, outcomes, 1) + assert.Equal(t, OutcomeSucceeded, outcomes[0].Status) + assert.Equal(t, test.wantVersion, outcomes[0].Version) + local, err := synclocal.CompileAttachments(root, func(syncdomain.ResourceID) bool { return true }) + require.NoError(t, err) + if test.wantDescription == "" { + assert.Empty(t, local) + assert.Empty(t, next.Resources, "a deletion removes the baseline") + return + } + require.Len(t, local, 1) + assert.Equal(t, test.wantDescription, *local[0].Attachment.Tool.Description) + assert.Equal(t, attachmentFingerprint(t, *tool("New", 0)), next.Resources[0].Fingerprint) + }) + } +} + +// A tool deletion runs after every variation row, so that the pull of a +// variation that drops the tool runs first. Without that pull, the variation +// still uses the file, and the deletion keeps it. +func TestExecutePlanDeletesAToolOnlyAfterNoVariationUsesIt(t *testing.T) { + tool := testAttachment(syncdomain.AttachmentTool, "local") + withTool := testVariation("support") + withTool.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + withTool.Attachments = []syncdomain.Attachment{tool} + withoutTool := testVariation("support") + deletion := planAttachment(tool.ID("production"), &tool, nil, attachmentFingerprint(t, tool), true) + require.Equal(t, ActionDeleteLocal, deletion.Action) + pull := PlannedResource{ID: testResourceID(), Action: ActionUpdateLocal, Local: &withTool, Server: &withoutTool} + variationDeletion := PlannedResource{ID: testResourceID(), Action: ActionDeleteLocal, Local: &withTool} + + for name, test := range map[string]struct { + resources []PlannedResource + wantError string + }{ + "the variation pull drops the tool": {resources: []PlannedResource{deletion, pull}}, + "the variation is deleted": {resources: []PlannedResource{deletion, variationDeletion}}, + "no pull": {resources: []PlannedResource{deletion}, wantError: "variation production/support/default still uses it"}, + } { + t.Run(name, func(t *testing.T) { + root := t.TempDir() + store := synclocal.NewStore(root) + _, err := store.Add([]synclocal.VariationFile{{ProjectKey: "production", ConfigKey: "support", Variation: withTool}}) + require.NoError(t, err) + + outcomes, _, err := executePlan(root, store, syncapi.Client{}, syncmanifest.New(), Plan{Resources: test.resources}, nil) + + toolFile := filepath.Join(root, syncdomain.RootDir, "production", "tools", "search.json") + _, statErr := os.Stat(toolFile) + if test.wantError != "" { + require.ErrorContains(t, err, test.wantError) + assert.Equal(t, OutcomeFailed, outcomes[0].Status) + assert.NoError(t, statErr) + return + } + require.NoError(t, err) + assert.ErrorIs(t, statErr, os.ErrNotExist) + assert.Equal(t, []ResourceID{deletion.ID, pull.ID}, []ResourceID{outcomes[0].ID, outcomes[1].ID}, "outcomes stay in plan order") + }) + } +} diff --git a/internal/sync/prompt/output.go b/internal/sync/prompt/output.go index e3b06a21..90bf18cb 100644 --- a/internal/sync/prompt/output.go +++ b/internal/sync/prompt/output.go @@ -38,6 +38,12 @@ type ResourceOutcome struct { Action Action `json:"action"` Status OutcomeStatus `json:"status"` Error string `json:"error,omitempty"` + // Version is the version that LaunchDarkly has after a write to a tool or + // a skill, or after a pull of one. + Version int `json:"version,omitempty"` + // pins are the tool and skill versions of a variation that sync wrote to + // LaunchDarkly. + pins []syncdomain.AttachmentRef } // resourceOutput is the identity of a resource in JSON output. @@ -122,7 +128,7 @@ func writeOutcomeOutput(out io.Writer, outputKind string, plan Plan, outcomes [] default: rows = append(rows, reviewRow{ kind: outcome.ID.Kind, - cells: []string{actionWordFor(resource).result, outcome.ID.String(), planReason(resource)}, + cells: []string{actionWordFor(resource).result, outcome.ID.String(), resultReason(resource, outcome)}, }) } } @@ -343,6 +349,23 @@ func planReason(resource PlannedResource) string { } } +// resultReason gives the short reason of a result row. A write names the new +// version of a tool or a skill, or the versions that a variation pins. +// Otherwise the result has the reason of its plan row. +func resultReason(resource PlannedResource, outcome ResourceOutcome) string { + if outcome.Version != 0 { + return fmt.Sprintf("version %d", outcome.Version) + } + if len(outcome.pins) == 0 { + return planReason(resource) + } + pins := make([]string, 0, len(outcome.pins)) + for _, pin := range outcome.pins { + pins = append(pins, fmt.Sprintf("%s v%d", pin.Key, pin.Version)) + } + return "pins " + strings.Join(pins, ", ") +} + // changedFieldsReason names the fields that the diff of a row changes, for // example "instructions, tools changed". A tool or skill row names the fields // of the tool or the skill, for example "description changed". diff --git a/internal/sync/prompt/output_test.go b/internal/sync/prompt/output_test.go index f3a1f21b..4563ae0a 100644 --- a/internal/sync/prompt/output_test.go +++ b/internal/sync/prompt/output_test.go @@ -90,12 +90,22 @@ func TestPlanAndOutcomeJSONIsStable(t *testing.T) { `, output.String()) output.Reset() + tool := ResourceID{Kind: syncdomain.KindTool, ProjectKey: "production", LookupKey: "search"} require.NoError(t, writeOutcomeOutput(&output, "json", plan, []ResourceOutcome{ - {ID: pushed, Action: ActionUpdateServer, Status: OutcomeSucceeded}, + {ID: tool, Action: ActionUpdateServer, Status: OutcomeSucceeded, Version: 4}, + {ID: pushed, Action: ActionUpdateServer, Status: OutcomeSucceeded, pins: []syncdomain.AttachmentRef{{Key: "search", Version: 4}}}, {ID: stale, Action: ActionUpdateLocal, Status: OutcomeFailed, Error: "write failed"}, })) assert.Equal(t, `{ "resources": [ + { + "resourceKind": "tool", + "projectKey": "production", + "lookupKey": "search", + "action": "update_server", + "status": "succeeded", + "version": 4 + }, { "resourceKind": "variation", "projectKey": "production", @@ -317,7 +327,10 @@ func TestWriteOutcomeOutputMirrorsThePlan(t *testing.T) { plan := reviewTestPlan() outcomes := []ResourceOutcome{ {ID: plan.Resources[0].ID, Action: ActionUpdateManifest, Status: OutcomeSucceeded}, - {ID: plan.Resources[1].ID, Action: ActionUpdateServer, Status: OutcomeSucceeded}, + { + ID: plan.Resources[1].ID, Action: ActionUpdateServer, Status: OutcomeSucceeded, + pins: []syncdomain.AttachmentRef{{Key: "search", Version: 4}, {Key: "summarize", Version: 2}}, + }, {ID: plan.Resources[2].ID, Action: ActionUpdateLocal, Status: OutcomeFailed, Error: "write failed"}, {ID: plan.Resources[4].ID, Action: ActionInSync, Status: OutcomeSucceeded}, } @@ -328,11 +341,12 @@ func TestWriteOutcomeOutputMirrorsThePlan(t *testing.T) { assert.Equal(t, `Tools recorded production/lookup same as LaunchDarkly Prompts - pushed production/support/default instructions changed + pushed production/support/default pins search v4, summarize v2 Failed: variation production/support/second: write failed 2 applied, 1 failed. 1 in sync. `, output.String()) + assert.Equal(t, "version 4", resultReason(PlannedResource{Action: ActionUpdateServer}, ResourceOutcome{Version: 4})) } func TestWritePlanDescribesMissingLaunchDarklyVariation(t *testing.T) { diff --git a/internal/sync/prompt/review.go b/internal/sync/prompt/review.go index 64ad9d04..3f75abb2 100644 --- a/internal/sync/prompt/review.go +++ b/internal/sync/prompt/review.go @@ -57,9 +57,12 @@ func reviewAndConfirmPlan(options Options, plan Plan, interactive bool) (bool, e // 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. +// 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 { - if !options.syncAction().Filter.IsZero() { + action := options.syncAction() + if !action.Filter.IsZero() || action.Scope.Includes(syncdomain.KindTool) || action.Scope.Includes(syncdomain.KindSkill) { return nil } orphaned, err := store.OrphanedAttachments() @@ -112,7 +115,13 @@ func cleanupOrphanedAttachments(options Options, store synclocal.Store, interact return nil } - deleted, err := store.DeleteAttachments(orphaned) + 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 } diff --git a/internal/sync/prompt/sync.go b/internal/sync/prompt/sync.go index 235151da..34998336 100644 --- a/internal/sync/prompt/sync.go +++ b/internal/sync/prompt/sync.go @@ -104,9 +104,7 @@ func (runner Runner) applyPlan( interactive bool, ) error { plan := applyConflictResolutions(current.plan, resolutions) - outcomes, next, err := executePlan( - workspace.root, workspace.local, client, current.baseline.Lock, plan, current.localFiles, options.syncAction().Filter, - ) + outcomes, next, err := executePlan(workspace.root, workspace.local, client, current.baseline.Lock, plan, current.localFiles) failures := []error{err} if _, err := workspace.baselines.Save(current.baseline, next); err != nil { From ffb76209dc9678a79a25f4970cf9076b52dbf78b Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Thu, 22 Oct 2026 16:11:43 +0000 Subject: [PATCH 2/3] fix: keep the tool and skill content that a row before settled A tool or skill row runs before the variations that use it. A variation row then wrote its own copy of the attachment from the plan. A push that newly attached a pulled tool published the old local copy over the LaunchDarkly change. A pull that newly referenced a pushed tool wrote the old LaunchDarkly copy over the local edit. sync.lock then recorded the old content. Each variation row now uses the content that this execution settled for its tools and skills, from a tool or skill row or from an earlier push. If one of them failed, the variation row fails too. sync.lock records the settled content. --- internal/sync/prompt/attachment_versions.go | 65 +++++++++ internal/sync/prompt/execute.go | 15 +- internal/sync/prompt/execute_test.go | 144 ++++++++++++++++++++ 3 files changed, 219 insertions(+), 5 deletions(-) diff --git a/internal/sync/prompt/attachment_versions.go b/internal/sync/prompt/attachment_versions.go index 4f1d8d3d..6cc10620 100644 --- a/internal/sync/prompt/attachment_versions.go +++ b/internal/sync/prompt/attachment_versions.go @@ -3,6 +3,7 @@ package prompt import ( "errors" "fmt" + "slices" syncdomain "github.com/launchdarkly/ldcli/internal/sync" syncapi "github.com/launchdarkly/ldcli/internal/sync/api" @@ -28,6 +29,70 @@ func newAttachmentResolver(client syncapi.Client) *attachmentResolver { } } +// settle records the result of a tool or skill row. A pull settles the +// LaunchDarkly content. A push settles through resolve. +func (resolver *attachmentResolver) settle(id ResourceID, attachment syncdomain.Attachment, err error) { + if err != nil { + resolver.failures[id] = err + return + } + resolver.resolved[id] = attachment +} + +// settledRow returns the variation row with the content that this execution +// already gave each of its tools and skills. Without it, a variation pull +// writes the old LaunchDarkly copy over a tool that a row before it pushed, +// and a variation push publishes the old local copy over a tool that a row +// before it pulled. If a tool or skill of the row failed, the row fails too. +func (resolver *attachmentResolver) settledRow(resource PlannedResource) (PlannedResource, error) { + side, fingerprint := &resource.Local, &resource.LocalFingerprint + switch { + case resource.Action == ActionUpdateLocal: + side, fingerprint = &resource.Server, &resource.ServerFingerprint + case !resource.Action.changesServer(): + return resource, nil + } + if *side == nil { + return resource, nil + } + for _, attachment := range (*side).Attachments { + if err, failed := resolver.failures[attachment.ID(resource.ID.ProjectKey)]; failed { + return resource, err + } + } + + attachments, changed := resolver.settledAttachments(resource.ID.ProjectKey, (*side).Attachments) + if !changed { + return resource, nil + } + variation := **side + variation.Attachments = attachments + value, err := syncdomain.FingerprintVariation(resource.ID.ProjectKey, resource.ID.LookupKey, variation) + if err != nil { + return resource, err + } + *side, *fingerprint = &variation, value + return resource, nil +} + +// settledAttachments returns a copy of the attachments in which each one that +// this execution settled has the settled content. It reports whether any +// content changed. Each attachment keeps its upsert flag. +func (resolver *attachmentResolver) settledAttachments( + projectKey string, + attachments []syncdomain.Attachment, +) ([]syncdomain.Attachment, bool) { + result := slices.Clone(attachments) + changed := false + for index, attachment := range result { + if settled, ok := resolver.resolved[attachment.ID(projectKey)]; ok && !syncdomain.SameAttachmentContent(settled, attachment) { + settled.Upsert = attachment.Upsert + result[index], changed = settled, true + } + } + return result, changed +} + // resolveVariation returns a copy of the variation that pins the published // version of each attachment. func (resolver *attachmentResolver) resolveVariation(projectKey string, variation syncdomain.Variation) (syncdomain.Variation, error) { diff --git a/internal/sync/prompt/execute.go b/internal/sync/prompt/execute.go index efad982f..e4aa82bd 100644 --- a/internal/sync/prompt/execute.go +++ b/internal/sync/prompt/execute.go @@ -57,7 +57,9 @@ func executePlan( if file, ok := localFiles[resource.ID]; ok { localFile = &file.Variation } - pins, err = applyResourceChange(repositoryRoot, localStore, client, attachments, resource, localFile) + if resource, err = attachments.settledRow(resource); err == nil { + pins, err = applyResourceChange(repositoryRoot, localStore, client, attachments, resource, localFile) + } } else { version, err = applyAttachmentChange(repositoryRoot, localStore, attachments, resource) } @@ -73,7 +75,8 @@ func executePlan( if outcome.Status == OutcomeSucceeded { if variation := attachmentManifestState(resource); variation != nil { - if err := next.SetAttachments(resource.ID.ProjectKey, variation.Attachments); err != nil { + settled, _ := attachments.settledAttachments(resource.ID.ProjectKey, variation.Attachments) + if err := next.SetAttachments(resource.ID.ProjectKey, settled); err != nil { fail(err) } } @@ -265,10 +268,12 @@ func applyAttachmentChange( return published.Version, err case ActionUpdateLocal: server := syncdomain.SyncedAttachment{ProjectKey: resource.ID.ProjectKey, Attachment: *resource.ServerAttachment} - if _, err := localStore.ReplaceAttachments([]syncdomain.SyncedAttachment{server}); err != nil { - return 0, err + _, err := localStore.ReplaceAttachments([]syncdomain.SyncedAttachment{server}) + if err == nil { + err = verifyLocalResult(repositoryRoot, resource) } - return server.Attachment.Version, verifyLocalResult(repositoryRoot, resource) + attachments.settle(resource.ID, server.Attachment, err) + return server.Attachment.Version, err case ActionDeleteLocal: // The pull of a variation that drops the reference ran first. If that // pull failed, the variation still uses the file, so keep it. diff --git a/internal/sync/prompt/execute_test.go b/internal/sync/prompt/execute_test.go index 06abaeec..f8dc5a3e 100644 --- a/internal/sync/prompt/execute_test.go +++ b/internal/sync/prompt/execute_test.go @@ -6,6 +6,7 @@ import ( "net/url" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -503,3 +504,146 @@ func TestExecutePlanDeletesAToolOnlyAfterNoVariationUsesIt(t *testing.T) { }) } } + +// A tool row runs before the variations that use the tool. A variation push +// that newly attaches the tool must pin the content that the tool row pulled, +// and must not publish its older local copy. +func TestExecutePlanPushPinsAToolThatTheSamePlanPulled(t *testing.T) { + old, pulled := testAttachment(syncdomain.AttachmentTool, "old"), testAttachment(syncdomain.AttachmentTool, "new") + pulled.Version = 5 + withTool := testVariation("support") + withTool.Tools = []syncdomain.AttachmentRef{{Key: "search"}} + withTool.Attachments = []syncdomain.Attachment{old} + withoutTool := testVariation("support") + root := t.TempDir() + store := synclocal.NewStore(root) + _, err := store.Add([]synclocal.VariationFile{{ProjectKey: "production", ConfigKey: "support", Variation: withTool}}) + require.NoError(t, err) + + toolRow := planAttachment(old.ID("production"), &old, &pulled, attachmentFingerprint(t, old), true) + require.Equal(t, ActionUpdateLocal, toolRow.Action) + push := PlannedResource{ + ID: testResourceID(), Action: ActionUpdateServer, Local: &withTool, Server: &withoutTool, + LocalFingerprint: fingerprintOf(t, withTool), + } + current := testAttachment(syncdomain.AttachmentTool, "new") + api := &settleAPI{tool: attachmentMutationAPI{current: *current.Tool, version: 5}} + manifest := syncmanifest.New() + manifest.SetFingerprint(toolRow.ID, toolRow.BaselineFingerprint) + + outcomes, next, err := executePlan(root, store, syncapi.NewClient(api, "token", "https://example.com"), + manifest, Plan{Resources: []PlannedResource{toolRow, push}}, nil) + + require.NoError(t, err) + assert.Equal(t, 0, api.tool.updates, "the push must not publish the old local copy") + require.Len(t, api.variations, 1) + assert.JSONEq(t, `[{"key":"search","version":5}]`, string(api.variations[0]["tools"])) + assert.Equal(t, []syncdomain.AttachmentRef{{Key: "search", Version: 5}}, outcomes[1].pins) + assert.Equal(t, attachmentFingerprint(t, pulled), fingerprintIn(next, toolRow.ID), "sync.lock records the pulled tool") + withPulled := withTool + withPulled.Attachments = []syncdomain.Attachment{pulled} + assert.Equal(t, fingerprintOf(t, withPulled), fingerprintIn(next, push.ID)) + assert.Equal(t, "new", *readLocalTool(t, root).Description) +} + +// A variation pull that newly references a tool must keep the edit that a +// tool row before it pushed. If that push fails, the pull fails too, so that +// it cannot write the old LaunchDarkly copy over the edit. +func TestExecutePlanPullKeepsAToolThatTheSamePlanPushed(t *testing.T) { + for name, test := range map[string]struct { + updateErr error + wantError string + }{ + "the tool push succeeds": {}, + "the tool push fails": {updateErr: errors.New(`{"code":"forbidden","statusCode":403}`), wantError: "forbidden"}, + } { + t.Run(name, func(t *testing.T) { + edited, remote := testAttachment(syncdomain.AttachmentTool, "new"), testAttachment(syncdomain.AttachmentTool, "old") + remote.Version = 2 + withTool := testVariation("support") + withTool.Tools = []syncdomain.AttachmentRef{{Key: "search", Version: 2}} + withTool.Attachments = []syncdomain.Attachment{remote} + withoutTool := testVariation("support") + root := t.TempDir() + store := synclocal.NewStore(root) + _, err := store.Add([]synclocal.VariationFile{{ProjectKey: "production", ConfigKey: "support", Variation: withoutTool}}) + require.NoError(t, err) + _, err = store.ReplaceAttachments([]syncdomain.SyncedAttachment{{ProjectKey: "production", Attachment: edited}}) + require.NoError(t, err) + + toolRow := planAttachment(edited.ID("production"), &edited, &remote, attachmentFingerprint(t, remote), true) + require.Equal(t, ActionUpdateServer, toolRow.Action) + pull := PlannedResource{ + ID: testResourceID(), Action: ActionUpdateLocal, Local: &withoutTool, Server: &withTool, + ServerFingerprint: fingerprintOf(t, withTool), + } + current := testAttachment(syncdomain.AttachmentTool, "old") + api := &settleAPI{tool: attachmentMutationAPI{current: *current.Tool, version: 2, updateErr: test.updateErr}} + + outcomes, next, err := executePlan(root, store, syncapi.NewClient(api, "token", "https://example.com"), + syncmanifest.New(), Plan{Resources: []PlannedResource{toolRow, pull}}, nil) + + assert.Equal(t, "new", *readLocalTool(t, root).Description, "the local edit stays") + if test.wantError != "" { + require.ErrorContains(t, err, test.wantError) + assert.Equal(t, OutcomeFailed, outcomes[1].Status, "the pull must not run") + return + } + require.NoError(t, err) + assert.Equal(t, "new", *api.tool.current.Description) + assert.Equal(t, attachmentFingerprint(t, edited), fingerprintIn(next, toolRow.ID)) + withEdit := withTool + withEdit.Attachments = []syncdomain.Attachment{edited} + assert.Equal(t, fingerprintOf(t, withEdit), fingerprintIn(next, pull.ID)) + }) + } +} + +// settleAPI serves the tool "search" like attachmentMutationAPI, and records +// the body of each variation update. +type settleAPI struct { + tool attachmentMutationAPI + variations []map[string]json.RawMessage +} + +func (api *settleAPI) MakeRequest( + accessToken, method, path, contentType string, query url.Values, body []byte, isBeta bool, +) ([]byte, error) { + if strings.Contains(path, "/ai-tools/") { + return api.tool.MakeRequest(accessToken, method, path, contentType, query, body, isBeta) + } + var request map[string]json.RawMessage + if err := json.Unmarshal(body, &request); err != nil { + return nil, err + } + api.variations = append(api.variations, request) + return []byte(`{}`), nil +} + +func (*settleAPI) MakeUnauthenticatedRequest(string, string, []byte) ([]byte, error) { + return nil, nil +} + +func fingerprintOf(t *testing.T, variation syncdomain.Variation) string { + t.Helper() + value, err := syncdomain.FingerprintVariation("production", "support/default", variation) + require.NoError(t, err) + return value +} + +func fingerprintIn(manifest syncmanifest.Manifest, id ResourceID) string { + for _, resource := range manifest.Resources { + if resource.ID() == id { + return resource.Fingerprint + } + } + return "" +} + +func readLocalTool(t *testing.T, root string) syncdomain.Tool { + t.Helper() + local, err := synclocal.CompileAttachments(root, func(syncdomain.ResourceID) bool { return true }) + require.NoError(t, err) + require.Len(t, local, 1) + return *local[0].Attachment.Tool +} From 72609b6b174fdb712d2023b65f4ede566fb9f2bc Mon Sep 17 00:00:00 2001 From: Cliff Tawiah Date: Thu, 22 Oct 2026 17:53:42 +0000 Subject: [PATCH 3/3] test: check that LaunchDarkly keeps the skill name after a create A rename of the skill in LaunchDarkly plans no change. A push of a local edit sends no name, so the LaunchDarkly name stays, and the file keeps its local name: and upsert: lines. --- internal/sync/prompt/acceptance_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/internal/sync/prompt/acceptance_test.go b/internal/sync/prompt/acceptance_test.go index e244361c..7e845bde 100644 --- a/internal/sync/prompt/acceptance_test.go +++ b/internal/sync/prompt/acceptance_test.go @@ -1280,6 +1280,27 @@ func TestPromptSyncsStandaloneToolsAndSkills(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Human handoff", api.skills["handoff"].Name) + // LaunchDarkly owns the skill name after the create. A rename there is not + // a change, and a push of a local edit does not send the local name. + handoff := api.skills["handoff"] + handoff.Name = "Renamed in LaunchDarkly" + api.skills["handoff"] = handoff + stdout, err = runSyncAction(t, root, api, syncprompt.SyncAction{DryRun: true, Scope: syncdomain.ScopeSkills}, false) + require.NoError(t, err) + assert.Contains(t, planRows(t, stdout), "skill handoff in_sync", "a rename in LaunchDarkly is not a change") + writeLocalFile(t, root, "skills/handoff.md", + "---\nkey: handoff\ndescription: Hand off to a person\nname: Human handoff\nupsert: true\n---\n\n# Handoff\n") + api.requests = nil + _, err = runSyncAction(t, root, api, syncprompt.SyncAction{Scope: syncdomain.ScopeSkills}, true) + require.NoError(t, err) + assert.Equal(t, 1, countRequests(api.requests, "PATCH", "/ai-configs/skills/handoff")) + assert.Equal(t, "Hand off to a person", api.skills["handoff"].Description) + assert.Equal(t, "Renamed in LaunchDarkly", api.skills["handoff"].Name, "the push must not send the local name") + content, err = os.ReadFile(filepath.Join(root, syncdomain.RootDir, "production", "skills", "handoff.md")) + require.NoError(t, err) + assert.Contains(t, string(content), "name: Human handoff\n", "the file keeps its local name") + assert.Contains(t, string(content), "upsert: true\n") + // A deletion in LaunchDarkly deletes the standalone file and its baseline. delete(api.tools, "lookup") _, err = runSyncAction(t, root, api, syncprompt.SyncAction{Scope: syncdomain.ScopeTools}, true)