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..7e845bde 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,95 @@ 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) + + // 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) + 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 +1356,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/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 1696254e..e4aa82bd 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,25 @@ 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 + } + 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) } - 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" @@ -64,12 +75,28 @@ 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) + settled, _ := attachments.settledAttachments(resource.ID.ProjectKey, variation.Attachments) + if err := next.SetAttachments(resource.ID.ProjectKey, settled); err != nil { + 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 +104,6 @@ func executePlan( if err != nil { failures = append(failures, err) } else { - if filter.IsZero() { - next.RemoveUnusedAttachments(variations) - } next.SetRefs(variations) } } @@ -112,7 +136,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 +146,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 +160,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 +171,127 @@ 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} + _, err := localStore.ReplaceAttachments([]syncdomain.SyncedAttachment{server}) + if err == nil { + err = 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. + 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 +340,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 +349,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..f8dc5a3e 100644 --- a/internal/sync/prompt/execute_test.go +++ b/internal/sync/prompt/execute_test.go @@ -4,6 +4,9 @@ import ( "encoding/json" "errors" "net/url" + "os" + "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -31,7 +34,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 +51,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 +80,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 +122,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 +194,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 +214,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 +371,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 +406,244 @@ 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") + }) + } +} + +// 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 +} 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 {