From 58e107ccf4858b5c55f30bd73d76a10413fb998c Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Wed, 13 May 2026 16:27:59 -0700 Subject: [PATCH 01/13] feat: add two-way manifest sync between project and app settings When the manifest-sync experiment is enabled, the CLI detects per-field differences between the local manifest.json and the remote app settings, lets the user resolve them interactively, and writes the merged result to both sides. This replaces the binary "overwrite?" prompt when divergence is detected during install/deploy. Adds `slack manifest sync` command and `slack sync` alias. Gated behind --experiment=manifest-sync. --- cmd/manifest/manifest.go | 1 + cmd/manifest/sync.go | 45 +++++++ cmd/root.go | 1 + internal/experiment/experiment.go | 4 + internal/pkg/apps/install.go | 15 ++- internal/pkg/manifest/diff.go | 100 +++++++++++++++ internal/pkg/manifest/diff_test.go | 143 +++++++++++++++++++++ internal/pkg/manifest/display.go | 155 +++++++++++++++++++++++ internal/pkg/manifest/flatten.go | 50 ++++++++ internal/pkg/manifest/flatten_test.go | 107 ++++++++++++++++ internal/pkg/manifest/merge.go | 159 ++++++++++++++++++++++++ internal/pkg/manifest/merge_test.go | 125 +++++++++++++++++++ internal/pkg/manifest/sync.go | 110 ++++++++++++++++ internal/pkg/manifest/writeback.go | 151 ++++++++++++++++++++++ internal/pkg/manifest/writeback_test.go | 85 +++++++++++++ 15 files changed, 1248 insertions(+), 3 deletions(-) create mode 100644 cmd/manifest/sync.go create mode 100644 internal/pkg/manifest/diff.go create mode 100644 internal/pkg/manifest/diff_test.go create mode 100644 internal/pkg/manifest/display.go create mode 100644 internal/pkg/manifest/flatten.go create mode 100644 internal/pkg/manifest/flatten_test.go create mode 100644 internal/pkg/manifest/merge.go create mode 100644 internal/pkg/manifest/merge_test.go create mode 100644 internal/pkg/manifest/sync.go create mode 100644 internal/pkg/manifest/writeback.go create mode 100644 internal/pkg/manifest/writeback_test.go diff --git a/cmd/manifest/manifest.go b/cmd/manifest/manifest.go index 9f9d3a57..5fe13fec 100644 --- a/cmd/manifest/manifest.go +++ b/cmd/manifest/manifest.go @@ -79,6 +79,7 @@ func NewCommand(clients *shared.ClientFactory) *cobra.Command { // Add child commands cmd.AddCommand(NewInfoCommand(clients)) + cmd.AddCommand(NewSyncCommand(clients)) cmd.AddCommand(NewValidateCommand(clients)) cmd.Flags().StringVar( diff --git a/cmd/manifest/sync.go b/cmd/manifest/sync.go new file mode 100644 index 00000000..a522bfb8 --- /dev/null +++ b/cmd/manifest/sync.go @@ -0,0 +1,45 @@ +package manifest + +import ( + "github.com/opentracing/opentracing-go" + "github.com/slackapi/slack-cli/internal/app" + "github.com/slackapi/slack-cli/internal/cmdutil" + "github.com/slackapi/slack-cli/internal/pkg/manifest" + "github.com/slackapi/slack-cli/internal/prompts" + "github.com/slackapi/slack-cli/internal/shared" + "github.com/slackapi/slack-cli/internal/style" + "github.com/spf13/cobra" +) + +var manifestSyncFunc = manifest.Sync + +func NewSyncCommand(clients *shared.ClientFactory) *cobra.Command { + cmd := &cobra.Command{ + Use: "sync", + Short: "Sync the app manifest between project and app settings", + Long: "Compare the local project manifest with app settings, resolve differences, and sync both to the same state.", + Example: style.ExampleCommandsf([]style.ExampleCommand{ + {Command: "manifest sync", Meaning: "Sync project manifest with app settings"}, + }), + Args: cobra.NoArgs, + PreRunE: func(cmd *cobra.Command, args []string) error { + return cmdutil.IsValidProjectDirectory(clients) + }, + RunE: func(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() + span, ctx := opentracing.StartSpanFromContext(ctx, "cmd.manifest.sync") + defer span.Finish() + + selection, err := appSelectPromptFunc(ctx, clients, prompts.ShowAllEnvironments, prompts.ShowInstalledAppsOnly) + if err != nil { + return err + } + + clients.Config.ManifestEnv = app.SetManifestEnvTeamVars(clients.Config.ManifestEnv, selection.App.TeamDomain, selection.App.IsDev) + + _, err = manifestSyncFunc(ctx, clients, selection.App, selection.Auth) + return err + }, + } + return cmd +} diff --git a/cmd/root.go b/cmd/root.go index b638502b..af2a36f8 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -80,6 +80,7 @@ var AliasMap = map[string]*AliasInfo{ "logout": {CommandFactory: auth.NewLogoutCommand, CanonicalName: "auth logout", ParentName: "auth"}, "run": {CommandFactory: platform.NewRunCommand, CanonicalName: "platform run", ParentName: "platform"}, "samples": {CommandFactory: project.NewSamplesCommand, CanonicalName: "project samples", ParentName: "project"}, + "sync": {CommandFactory: manifest.NewSyncCommand, CanonicalName: "manifest sync", ParentName: "manifest"}, "uninstall": {CommandFactory: app.NewUninstallCommand, CanonicalName: "app uninstall", ParentName: "app"}, } var processName = cmdutil.GetProcessName() diff --git a/internal/experiment/experiment.go b/internal/experiment/experiment.go index 80c661f9..2cf1687e 100644 --- a/internal/experiment/experiment.go +++ b/internal/experiment/experiment.go @@ -33,6 +33,9 @@ const ( // Lipgloss experiment shows pretty styles. Lipgloss Experiment = "lipgloss" + // ManifestSync experiment enables two-way manifest sync between local and remote. + ManifestSync Experiment = "manifest-sync" + // Placeholder experiment is a placeholder for testing and does nothing... or does it? Placeholder Experiment = "placeholder" @@ -47,6 +50,7 @@ const ( // Please also add here 👇 var AllExperiments = []Experiment{ Lipgloss, + ManifestSync, Placeholder, Sandboxes, SetIcon, diff --git a/internal/pkg/apps/install.go b/internal/pkg/apps/install.go index 76c69324..e660b9d3 100644 --- a/internal/pkg/apps/install.go +++ b/internal/pkg/apps/install.go @@ -25,7 +25,7 @@ import ( "github.com/slackapi/slack-cli/internal/config" "github.com/slackapi/slack-cli/internal/experiment" "github.com/slackapi/slack-cli/internal/icon" - "github.com/slackapi/slack-cli/internal/pkg/manifest" + manifestpkg "github.com/slackapi/slack-cli/internal/pkg/manifest" "github.com/slackapi/slack-cli/internal/shared" "github.com/slackapi/slack-cli/internal/shared/types" "github.com/slackapi/slack-cli/internal/slackerror" @@ -273,11 +273,11 @@ func printNonSuccessInstallState(ctx context.Context, clients *shared.ClientFact func validateManifestForInstall(ctx context.Context, clients *shared.ClientFactory, token string, app types.App, appManifest types.AppManifest) error { validationResult, err := clients.API().ValidateAppManifest(ctx, token, appManifest, app.AppID) - if retryValidate := manifest.HandleConnectorNotInstalled(ctx, clients, token, err); retryValidate { + if retryValidate := manifestpkg.HandleConnectorNotInstalled(ctx, clients, token, err); retryValidate { validationResult, err = clients.API().ValidateAppManifest(ctx, token, appManifest, app.AppID) } - if err := manifest.HandleConnectorApprovalRequired(ctx, clients, token, err); err != nil { + if err := manifestpkg.HandleConnectorApprovalRequired(ctx, clients, token, err); err != nil { return err } @@ -764,6 +764,15 @@ func shouldUpdateManifest(ctx context.Context, clients *shared.ClientFactory, ap default: notice = style.Yellow("The manifest on app settings has been changed since last update") } + + if clients.Config.WithExperimentOn(experiment.ManifestSync) { + _, err := manifestpkg.Sync(ctx, clients, app, auth) + if err != nil { + return false, err + } + return false, nil + } + clients.IO.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{ Emoji: "books", Text: "App Manifest", diff --git a/internal/pkg/manifest/diff.go b/internal/pkg/manifest/diff.go new file mode 100644 index 00000000..0922d091 --- /dev/null +++ b/internal/pkg/manifest/diff.go @@ -0,0 +1,100 @@ +package manifest + +import ( + "encoding/json" + "fmt" + + "github.com/slackapi/slack-cli/internal/shared/types" +) + +// DiffType describes how a field differs between local and remote. +type DiffType int + +const ( + DiffModified DiffType = iota // Both sides have the field but with different values + DiffLocalOnly // Field exists only in local (added locally or deleted remotely) + DiffRemoteOnly // Field exists only in remote (added remotely or deleted locally) +) + +// FieldDiff represents a single difference between local and remote manifests. +type FieldDiff struct { + Path string + Type DiffType + LocalValue any + RemoteValue any +} + +// DiffResult holds all differences found between two manifests. +type DiffResult struct { + Diffs []FieldDiff +} + +// HasDifferences returns true if any differences were found. +func (dr *DiffResult) HasDifferences() bool { + return len(dr.Diffs) > 0 +} + +// Diff performs a two-way comparison between local and remote manifests, +// returning all fields that differ between them. +func Diff(local, remote types.AppManifest) (*DiffResult, error) { + localFlat, err := Flatten(local) + if err != nil { + return nil, fmt.Errorf("failed to flatten local manifest: %w", err) + } + remoteFlat, err := Flatten(remote) + if err != nil { + return nil, fmt.Errorf("failed to flatten remote manifest: %w", err) + } + return diffFlat(localFlat, remoteFlat), nil +} + +func diffFlat(local, remote map[string]any) *DiffResult { + result := &DiffResult{} + seen := make(map[string]bool) + + for path, localVal := range local { + seen[path] = true + remoteVal, exists := remote[path] + if !exists { + result.Diffs = append(result.Diffs, FieldDiff{ + Path: path, + Type: DiffLocalOnly, + LocalValue: localVal, + }) + continue + } + if !valuesEqual(localVal, remoteVal) { + result.Diffs = append(result.Diffs, FieldDiff{ + Path: path, + Type: DiffModified, + LocalValue: localVal, + RemoteValue: remoteVal, + }) + } + } + + for path, remoteVal := range remote { + if seen[path] { + continue + } + result.Diffs = append(result.Diffs, FieldDiff{ + Path: path, + Type: DiffRemoteOnly, + RemoteValue: remoteVal, + }) + } + + return result +} + +func valuesEqual(a, b any) bool { + aJSON, err := json.Marshal(a) + if err != nil { + return false + } + bJSON, err := json.Marshal(b) + if err != nil { + return false + } + return string(aJSON) == string(bJSON) +} diff --git a/internal/pkg/manifest/diff_test.go b/internal/pkg/manifest/diff_test.go new file mode 100644 index 00000000..a661c083 --- /dev/null +++ b/internal/pkg/manifest/diff_test.go @@ -0,0 +1,143 @@ +package manifest + +import ( + "testing" + + "github.com/slackapi/slack-cli/internal/shared/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_Diff(t *testing.T) { + tests := map[string]struct { + local types.AppManifest + remote types.AppManifest + expected []FieldDiff + }{ + "identical manifests produce no diffs": { + local: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + }, + remote: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + }, + expected: nil, + }, + "modified field detected": { + local: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App", Description: "Local desc"}, + }, + remote: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App", Description: "Remote desc"}, + }, + expected: []FieldDiff{ + {Path: "display_information.description", Type: DiffModified, LocalValue: "Local desc", RemoteValue: "Remote desc"}, + }, + }, + "local-only field detected": { + local: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App", Description: "Has desc"}, + }, + remote: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + }, + expected: []FieldDiff{ + {Path: "display_information.description", Type: DiffLocalOnly, LocalValue: "Has desc"}, + }, + }, + "remote-only field detected": { + local: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + }, + remote: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App", Description: "Remote only"}, + }, + expected: []FieldDiff{ + {Path: "display_information.description", Type: DiffRemoteOnly, RemoteValue: "Remote only"}, + }, + }, + "function added locally": { + local: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + Functions: map[string]types.ManifestFunction{ + "greet": {Title: "Greet", Description: "Hello"}, + }, + }, + remote: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + }, + expected: []FieldDiff{ + {Path: "functions.greet.description", Type: DiffLocalOnly, LocalValue: "Hello"}, + {Path: "functions.greet.title", Type: DiffLocalOnly, LocalValue: "Greet"}, + }, + }, + "array values compared as wholes": { + local: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + OAuthConfig: &types.OAuthConfig{ + Scopes: &types.ManifestScopes{ + Bot: []string{"chat:write", "users:read"}, + }, + }, + }, + remote: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + OAuthConfig: &types.OAuthConfig{ + Scopes: &types.ManifestScopes{ + Bot: []string{"chat:write", "files:read"}, + }, + }, + }, + expected: []FieldDiff{ + { + Path: "oauth_config.scopes.bot", + Type: DiffModified, + LocalValue: []any{"chat:write", "users:read"}, + RemoteValue: []any{"chat:write", "files:read"}, + }, + }, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + result, err := Diff(tc.local, tc.remote) + require.NoError(t, err) + if tc.expected == nil { + assert.False(t, result.HasDifferences()) + return + } + assert.True(t, result.HasDifferences()) + for _, expectedDiff := range tc.expected { + found := false + for _, actualDiff := range result.Diffs { + if actualDiff.Path == expectedDiff.Path { + found = true + assert.Equal(t, expectedDiff.Type, actualDiff.Type, "diff type mismatch for path %s", expectedDiff.Path) + if expectedDiff.LocalValue != nil { + assert.Equal(t, expectedDiff.LocalValue, actualDiff.LocalValue, "local value mismatch for path %s", expectedDiff.Path) + } + if expectedDiff.RemoteValue != nil { + assert.Equal(t, expectedDiff.RemoteValue, actualDiff.RemoteValue, "remote value mismatch for path %s", expectedDiff.Path) + } + break + } + } + assert.True(t, found, "expected diff not found for path %s", expectedDiff.Path) + } + }) + } +} + +func Test_DiffResult_HasDifferences(t *testing.T) { + t.Run("empty result has no differences", func(t *testing.T) { + result := &DiffResult{} + assert.False(t, result.HasDifferences()) + }) + + t.Run("result with diffs has differences", func(t *testing.T) { + result := &DiffResult{ + Diffs: []FieldDiff{{Path: "test", Type: DiffModified}}, + } + assert.True(t, result.HasDifferences()) + }) +} diff --git a/internal/pkg/manifest/display.go b/internal/pkg/manifest/display.go new file mode 100644 index 00000000..2a4d854a --- /dev/null +++ b/internal/pkg/manifest/display.go @@ -0,0 +1,155 @@ +package manifest + +import ( + "context" + "encoding/json" + "fmt" + "sort" + + "github.com/slackapi/slack-cli/internal/iostreams" + "github.com/slackapi/slack-cli/internal/style" +) + +// DisplayDiffs prints the differences to the terminal. +func DisplayDiffs(ctx context.Context, io iostreams.IOStreamer, diffs *DiffResult) { + if !diffs.HasDifferences() { + return + } + + sorted := make([]FieldDiff, len(diffs.Diffs)) + copy(sorted, diffs.Diffs) + sort.Slice(sorted, func(i, j int) bool { + return sorted[i].Path < sorted[j].Path + }) + + io.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{ + Emoji: "books", + Text: "App Manifest", + Secondary: []string{ + fmt.Sprintf("Found %d difference(s) between project and app settings", len(sorted)), + }, + })) + + for _, d := range sorted { + io.PrintInfo(ctx, false, "") + switch d.Type { + case DiffModified: + io.PrintInfo(ctx, false, " %s", style.Bold(d.Path)) + io.PrintInfo(ctx, false, " Project: %s", formatValue(d.LocalValue)) + io.PrintInfo(ctx, false, " App settings: %s", formatValue(d.RemoteValue)) + case DiffLocalOnly: + io.PrintInfo(ctx, false, " %s %s", style.Bold(d.Path), "(only in project)") + io.PrintInfo(ctx, false, " Value: %s", formatValue(d.LocalValue)) + case DiffRemoteOnly: + io.PrintInfo(ctx, false, " %s %s", style.Bold(d.Path), "(only in app settings)") + io.PrintInfo(ctx, false, " Value: %s", formatValue(d.RemoteValue)) + } + } + io.PrintInfo(ctx, false, "") +} + +// PromptResolutionStrategy asks the user how they want to resolve differences. +func PromptResolutionStrategy(ctx context.Context, io iostreams.IOStreamer) (MergeStrategy, error) { + options := []string{ + "Use all project values", + "Use all app settings values", + "Choose for each difference", + } + resp, err := io.SelectPrompt(ctx, "How would you like to resolve these differences?", options, iostreams.SelectPromptConfig{ + Required: true, + }) + if err != nil { + return 0, err + } + switch resp.Index { + case 0: + return MergeAllLocal, nil + case 1: + return MergeAllRemote, nil + default: + return MergePerField, nil + } +} + +// PromptFieldResolutions asks the user to resolve each difference individually. +func PromptFieldResolutions(ctx context.Context, io iostreams.IOStreamer, diffs *DiffResult) ([]FieldResolution, error) { + sorted := make([]FieldDiff, len(diffs.Diffs)) + copy(sorted, diffs.Diffs) + sort.Slice(sorted, func(i, j int) bool { + return sorted[i].Path < sorted[j].Path + }) + + resolutions := make([]FieldResolution, 0, len(sorted)) + for _, d := range sorted { + var options []string + switch d.Type { + case DiffModified: + options = []string{ + fmt.Sprintf("Use project: %s", formatValue(d.LocalValue)), + fmt.Sprintf("Use app settings: %s", formatValue(d.RemoteValue)), + } + case DiffLocalOnly: + options = []string{ + "Keep (include in merged manifest)", + "Remove (exclude from merged manifest)", + } + case DiffRemoteOnly: + options = []string{ + "Remove (exclude from merged manifest)", + "Keep (include in merged manifest)", + } + } + + resp, err := io.SelectPrompt(ctx, d.Path, options, iostreams.SelectPromptConfig{ + Required: true, + }) + if err != nil { + return nil, err + } + + var resolution Resolution + switch d.Type { + case DiffModified: + if resp.Index == 0 { + resolution = ResolveLocal + } else { + resolution = ResolveRemote + } + case DiffLocalOnly: + if resp.Index == 0 { + resolution = ResolveLocal + } else { + resolution = ResolveRemote + } + case DiffRemoteOnly: + if resp.Index == 0 { + resolution = ResolveLocal + } else { + resolution = ResolveRemote + } + } + + resolutions = append(resolutions, FieldResolution{Path: d.Path, Resolution: resolution}) + } + return resolutions, nil +} + +func formatValue(v any) string { + if v == nil { + return "(not present)" + } + switch val := v.(type) { + case string: + return fmt.Sprintf("%q", val) + default: + data, err := json.Marshal(val) + if err != nil { + return fmt.Sprintf("%v", val) + } + s := string(data) + if len(s) > 80 { + return s[:77] + "..." + } + return s + } +} diff --git a/internal/pkg/manifest/flatten.go b/internal/pkg/manifest/flatten.go new file mode 100644 index 00000000..8d706105 --- /dev/null +++ b/internal/pkg/manifest/flatten.go @@ -0,0 +1,50 @@ +package manifest + +import ( + "encoding/json" + "fmt" + "sort" +) + +// Flatten converts a manifest (as JSON-serializable struct) into a flat map +// where keys are dot-notation paths and values are the leaf values. +// Arrays are treated as leaf values (not recursed into individually) because +// array element identity is ambiguous without a key field. +func Flatten(manifest any) (map[string]any, error) { + data, err := json.Marshal(manifest) + if err != nil { + return nil, fmt.Errorf("failed to marshal manifest: %w", err) + } + var raw map[string]any + if err := json.Unmarshal(data, &raw); err != nil { + return nil, fmt.Errorf("failed to unmarshal manifest: %w", err) + } + result := make(map[string]any) + flattenRecursive("", raw, result) + return result, nil +} + +func flattenRecursive(prefix string, data map[string]any, result map[string]any) { + for key, value := range data { + fullKey := key + if prefix != "" { + fullKey = prefix + "." + key + } + switch v := value.(type) { + case map[string]any: + flattenRecursive(fullKey, v, result) + default: + result[fullKey] = value + } + } +} + +// SortedKeys returns the keys of a flat map in sorted order for deterministic output. +func SortedKeys(m map[string]any) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} diff --git a/internal/pkg/manifest/flatten_test.go b/internal/pkg/manifest/flatten_test.go new file mode 100644 index 00000000..d490d721 --- /dev/null +++ b/internal/pkg/manifest/flatten_test.go @@ -0,0 +1,107 @@ +package manifest + +import ( + "testing" + + "github.com/slackapi/slack-cli/internal/shared/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_Flatten(t *testing.T) { + tests := map[string]struct { + manifest types.AppManifest + expected map[string]any + }{ + "flattens display_information fields": { + manifest: types.AppManifest{ + DisplayInformation: types.DisplayInformation{ + Name: "My App", + Description: "A test app", + }, + }, + expected: map[string]any{ + "display_information.name": "My App", + "display_information.description": "A test app", + }, + }, + "flattens nested settings": { + manifest: types.AppManifest{ + DisplayInformation: types.DisplayInformation{ + Name: "App", + }, + Settings: &types.AppSettings{ + FunctionRuntime: types.LocallyRun, + }, + }, + expected: map[string]any{ + "display_information.name": "App", + "settings.function_runtime": "local", + }, + }, + "flattens functions map": { + manifest: types.AppManifest{ + DisplayInformation: types.DisplayInformation{ + Name: "App", + }, + Functions: map[string]types.ManifestFunction{ + "greet": { + Title: "Greet", + Description: "Greets a user", + }, + }, + }, + expected: map[string]any{ + "display_information.name": "App", + "functions.greet.title": "Greet", + "functions.greet.description": "Greets a user", + }, + }, + "treats arrays as leaf values": { + manifest: types.AppManifest{ + DisplayInformation: types.DisplayInformation{ + Name: "App", + }, + OAuthConfig: &types.OAuthConfig{ + Scopes: &types.ManifestScopes{ + Bot: []string{"chat:write", "channels:read"}, + }, + }, + }, + expected: map[string]any{ + "display_information.name": "App", + "oauth_config.scopes.bot": []any{"chat:write", "channels:read"}, + }, + }, + "empty manifest has only display_information.name": { + manifest: types.AppManifest{ + DisplayInformation: types.DisplayInformation{ + Name: "App", + }, + }, + expected: map[string]any{ + "display_information.name": "App", + }, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + result, err := Flatten(tc.manifest) + require.NoError(t, err) + for key, expectedVal := range tc.expected { + assert.Contains(t, result, key) + assert.Equal(t, expectedVal, result[key], "mismatch at key %s", key) + } + }) + } +} + +func Test_SortedKeys(t *testing.T) { + m := map[string]any{ + "z.field": "val", + "a.field": "val", + "m.field": "val", + } + keys := SortedKeys(m) + assert.Equal(t, []string{"a.field", "m.field", "z.field"}, keys) +} diff --git a/internal/pkg/manifest/merge.go b/internal/pkg/manifest/merge.go new file mode 100644 index 00000000..f17e73ac --- /dev/null +++ b/internal/pkg/manifest/merge.go @@ -0,0 +1,159 @@ +package manifest + +import ( + "encoding/json" + "fmt" + "maps" + + "github.com/slackapi/slack-cli/internal/shared/types" +) + +// Resolution represents which side the user chose for a given field. +type Resolution int + +const ( + ResolveLocal Resolution = iota // Use the local value + ResolveRemote // Use the remote value +) + +// MergeStrategy determines how all differences are resolved. +type MergeStrategy int + +const ( + MergeAllLocal MergeStrategy = iota // Use all local values + MergeAllRemote // Use all remote values + MergePerField // Resolve each field individually +) + +// FieldResolution pairs a diff path with the user's choice. +type FieldResolution struct { + Path string + Resolution Resolution +} + +// Merge applies resolutions to produce a final manifest. It starts with the +// remote manifest as a base, then applies local values for fields resolved as +// local and keeps remote values for fields resolved as remote. +func Merge(local, remote types.AppManifest, resolutions []FieldResolution) (types.AppManifest, error) { + localFlat, err := Flatten(local) + if err != nil { + return types.AppManifest{}, fmt.Errorf("failed to flatten local manifest: %w", err) + } + remoteFlat, err := Flatten(remote) + if err != nil { + return types.AppManifest{}, fmt.Errorf("failed to flatten remote manifest: %w", err) + } + + merged := make(map[string]any) + maps.Copy(merged, remoteFlat) + + // Apply resolutions + resolutionMap := make(map[string]Resolution) + for _, r := range resolutions { + resolutionMap[r.Path] = r.Resolution + } + + for path, resolution := range resolutionMap { + switch resolution { + case ResolveLocal: + if val, exists := localFlat[path]; exists { + merged[path] = val + } else { + delete(merged, path) + } + case ResolveRemote: + if val, exists := remoteFlat[path]; exists { + merged[path] = val + } else { + delete(merged, path) + } + } + } + + // Also include local-only paths that were resolved as local + for path, val := range localFlat { + if _, inRemote := remoteFlat[path]; !inRemote { + if res, hasResolution := resolutionMap[path]; hasResolution && res == ResolveLocal { + merged[path] = val + } + } + } + + return unflatten(merged) +} + +// MergeAllFrom resolves all differences from one side. +func MergeAllFrom(local, remote types.AppManifest, diffs *DiffResult, strategy MergeStrategy) (types.AppManifest, error) { + resolutions := make([]FieldResolution, 0, len(diffs.Diffs)) + for _, d := range diffs.Diffs { + var res Resolution + switch strategy { + case MergeAllLocal: + res = ResolveLocal + case MergeAllRemote: + res = ResolveRemote + } + resolutions = append(resolutions, FieldResolution{Path: d.Path, Resolution: res}) + } + return Merge(local, remote, resolutions) +} + +// unflatten converts a flat dot-notation map back into a nested structure, +// then marshals/unmarshals into AppManifest. +func unflatten(flat map[string]any) (types.AppManifest, error) { + nested := unflattenToMap(flat) + data, err := json.Marshal(nested) + if err != nil { + return types.AppManifest{}, fmt.Errorf("failed to marshal merged manifest: %w", err) + } + var result types.AppManifest + if err := json.Unmarshal(data, &result); err != nil { + return types.AppManifest{}, fmt.Errorf("failed to unmarshal merged manifest: %w", err) + } + return result, nil +} + +// unflattenToMap reconstructs a nested map from dot-notation paths. +func unflattenToMap(flat map[string]any) map[string]any { + result := make(map[string]any) + for path, value := range flat { + setNestedValue(result, path, value) + } + return result +} + +func setNestedValue(m map[string]any, path string, value any) { + parts := splitPath(path) + current := m + for i, part := range parts { + if i == len(parts)-1 { + current[part] = value + return + } + next, exists := current[part] + if !exists { + next = make(map[string]any) + current[part] = next + } + current = next.(map[string]any) + } +} + +func splitPath(path string) []string { + var parts []string + current := "" + for _, ch := range path { + if ch == '.' { + if current != "" { + parts = append(parts, current) + current = "" + } + } else { + current += string(ch) + } + } + if current != "" { + parts = append(parts, current) + } + return parts +} diff --git a/internal/pkg/manifest/merge_test.go b/internal/pkg/manifest/merge_test.go new file mode 100644 index 00000000..39b020d0 --- /dev/null +++ b/internal/pkg/manifest/merge_test.go @@ -0,0 +1,125 @@ +package manifest + +import ( + "testing" + + "github.com/slackapi/slack-cli/internal/shared/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_Merge(t *testing.T) { + tests := map[string]struct { + local types.AppManifest + remote types.AppManifest + resolutions []FieldResolution + expected types.AppManifest + }{ + "resolve all local keeps local values": { + local: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Local App", Description: "Local desc"}, + }, + remote: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Remote App", Description: "Remote desc"}, + }, + resolutions: []FieldResolution{ + {Path: "display_information.name", Resolution: ResolveLocal}, + {Path: "display_information.description", Resolution: ResolveLocal}, + }, + expected: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Local App", Description: "Local desc"}, + }, + }, + "resolve all remote keeps remote values": { + local: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Local App", Description: "Local desc"}, + }, + remote: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Remote App", Description: "Remote desc"}, + }, + resolutions: []FieldResolution{ + {Path: "display_information.name", Resolution: ResolveRemote}, + {Path: "display_information.description", Resolution: ResolveRemote}, + }, + expected: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Remote App", Description: "Remote desc"}, + }, + }, + "mixed resolution picks per field": { + local: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Local App", Description: "Local desc"}, + }, + remote: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Remote App", Description: "Remote desc"}, + }, + resolutions: []FieldResolution{ + {Path: "display_information.name", Resolution: ResolveLocal}, + {Path: "display_information.description", Resolution: ResolveRemote}, + }, + expected: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Local App", Description: "Remote desc"}, + }, + }, + "local-only field resolved as local is included": { + local: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App", Description: "Has desc"}, + }, + remote: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + }, + resolutions: []FieldResolution{ + {Path: "display_information.description", Resolution: ResolveLocal}, + }, + expected: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App", Description: "Has desc"}, + }, + }, + "local-only field resolved as remote is excluded": { + local: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App", Description: "Has desc"}, + }, + remote: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + }, + resolutions: []FieldResolution{ + {Path: "display_information.description", Resolution: ResolveRemote}, + }, + expected: types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + }, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + result, err := Merge(tc.local, tc.remote, tc.resolutions) + require.NoError(t, err) + assert.Equal(t, tc.expected.DisplayInformation.Name, result.DisplayInformation.Name) + assert.Equal(t, tc.expected.DisplayInformation.Description, result.DisplayInformation.Description) + }) + } +} + +func Test_MergeAllFrom(t *testing.T) { + local := types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Local", Description: "Local desc"}, + } + remote := types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Remote", Description: "Remote desc"}, + } + diffs, err := Diff(local, remote) + require.NoError(t, err) + + t.Run("merge all local", func(t *testing.T) { + result, err := MergeAllFrom(local, remote, diffs, MergeAllLocal) + require.NoError(t, err) + assert.Equal(t, "Local", result.DisplayInformation.Name) + assert.Equal(t, "Local desc", result.DisplayInformation.Description) + }) + + t.Run("merge all remote", func(t *testing.T) { + result, err := MergeAllFrom(local, remote, diffs, MergeAllRemote) + require.NoError(t, err) + assert.Equal(t, "Remote", result.DisplayInformation.Name) + assert.Equal(t, "Remote desc", result.DisplayInformation.Description) + }) +} diff --git a/internal/pkg/manifest/sync.go b/internal/pkg/manifest/sync.go new file mode 100644 index 00000000..c747ef1a --- /dev/null +++ b/internal/pkg/manifest/sync.go @@ -0,0 +1,110 @@ +package manifest + +import ( + "context" + "fmt" + + "github.com/slackapi/slack-cli/internal/shared" + "github.com/slackapi/slack-cli/internal/shared/types" + "github.com/slackapi/slack-cli/internal/slackerror" + "github.com/slackapi/slack-cli/internal/style" +) + +// SyncResult describes what happened during a sync operation. +type SyncResult struct { + Merged types.AppManifest + WriteBack WriteBackResult + HasDifferences bool +} + +// Sync performs two-way manifest sync between local and remote. It fetches +// both manifests, computes diffs, prompts the user for resolution, writes +// the merged result to both the API and the local file, and returns the result. +func Sync(ctx context.Context, clients *shared.ClientFactory, app types.App, auth types.SlackAuth) (*SyncResult, error) { + localManifest, err := clients.AppClient().Manifest.GetManifestLocal(ctx, clients.SDKConfig, clients.HookExecutor) + if err != nil { + return nil, slackerror.New("Failed to get local manifest").WithRootCause(err).WithCode(slackerror.ErrInvalidManifest) + } + + remoteManifest, err := clients.AppClient().Manifest.GetManifestRemote(ctx, auth.Token, app.AppID) + if err != nil { + return nil, slackerror.New("Failed to get remote manifest from app settings").WithRootCause(err) + } + + diffs, err := Diff(localManifest.AppManifest, remoteManifest.AppManifest) + if err != nil { + return nil, fmt.Errorf("failed to compute manifest differences: %w", err) + } + + if !diffs.HasDifferences() { + clients.IO.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{ + Emoji: "books", + Text: "App Manifest", + Secondary: []string{"Project manifest and app settings are in sync"}, + })) + return &SyncResult{Merged: localManifest.AppManifest, HasDifferences: false}, nil + } + + DisplayDiffs(ctx, clients.IO, diffs) + + if !clients.IO.IsTTY() { + return nil, slackerror.New(slackerror.ErrAppManifestUpdate). + WithRemediation("Run %s interactively to resolve manifest differences, or use %s to overwrite app settings", + style.CommandText("slack manifest sync"), + style.CommandText("--force"), + ) + } + + merged, err := resolveInteractively(ctx, clients, localManifest.AppManifest, remoteManifest.AppManifest, diffs) + if err != nil { + return nil, err + } + + // Push merged manifest to API + clients.IO.PrintInfo(ctx, false, "\n Syncing manifest...") + _, err = clients.API().UpdateApp(ctx, auth.Token, app.AppID, merged, true, true) + if err != nil { + return nil, slackerror.New("Failed to update app settings with merged manifest").WithRootCause(err) + } + clients.IO.PrintInfo(ctx, false, " %s Updated app settings", style.Green("✓")) + + // Write back to local file + workingDir := clients.SDKConfig.WorkingDirectory + writeResult, err := WriteManifestLocal(clients.Fs, workingDir, merged) + if err != nil { + return nil, err + } + if writeResult.Written { + clients.IO.PrintInfo(ctx, false, " %s Updated %s", style.Green("✓"), manifestFileName) + } else if writeResult.Warning != "" { + clients.IO.PrintInfo(ctx, false, " %s %s", style.Yellow("!"), writeResult.Warning) + } + + clients.IO.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{ + Emoji: "books", + Text: "App Manifest", + Secondary: []string{fmt.Sprintf("Finished manifest sync for %q", localManifest.DisplayInformation.Name)}, + })) + + return &SyncResult{Merged: merged, WriteBack: writeResult, HasDifferences: true}, nil +} + +func resolveInteractively(ctx context.Context, clients *shared.ClientFactory, local, remote types.AppManifest, diffs *DiffResult) (types.AppManifest, error) { + strategy, err := PromptResolutionStrategy(ctx, clients.IO) + if err != nil { + return types.AppManifest{}, err + } + + switch strategy { + case MergeAllLocal: + return MergeAllFrom(local, remote, diffs, MergeAllLocal) + case MergeAllRemote: + return MergeAllFrom(local, remote, diffs, MergeAllRemote) + default: + resolutions, err := PromptFieldResolutions(ctx, clients.IO, diffs) + if err != nil { + return types.AppManifest{}, err + } + return Merge(local, remote, resolutions) + } +} diff --git a/internal/pkg/manifest/writeback.go b/internal/pkg/manifest/writeback.go new file mode 100644 index 00000000..e32f0503 --- /dev/null +++ b/internal/pkg/manifest/writeback.go @@ -0,0 +1,151 @@ +package manifest + +import ( + "bytes" + "encoding/json" + "fmt" + "os" + "path/filepath" + + "github.com/slackapi/slack-cli/internal/shared/types" + "github.com/spf13/afero" +) + +const manifestFileName = "manifest.json" + +// WriteBackResult describes what happened during write-back. +type WriteBackResult struct { + Written bool + FilePath string + Warning string +} + +// WriteManifestLocal writes the merged manifest back to the project's +// manifest.json file, preserving the original file's key ordering by +// using the same JSON structure. +func WriteManifestLocal(fs afero.Fs, workingDir string, manifest types.AppManifest) (WriteBackResult, error) { + manifestPath := filepath.Join(workingDir, manifestFileName) + + exists, err := afero.Exists(fs, manifestPath) + if err != nil { + return WriteBackResult{}, fmt.Errorf("failed to check manifest file: %w", err) + } + if !exists { + return WriteBackResult{ + Warning: fmt.Sprintf("No %s found in project root — merged manifest was not written locally", manifestFileName), + }, nil + } + + original, err := afero.ReadFile(fs, manifestPath) + if err != nil { + return WriteBackResult{}, fmt.Errorf("failed to read %s: %w", manifestFileName, err) + } + + merged, err := marshalPreservingOrder(original, manifest) + if err != nil { + return WriteBackResult{}, fmt.Errorf("failed to serialize merged manifest: %w", err) + } + + if err := afero.WriteFile(fs, manifestPath, merged, os.FileMode(0644)); err != nil { + return WriteBackResult{}, fmt.Errorf("failed to write %s: %w", manifestFileName, err) + } + + return WriteBackResult{Written: true, FilePath: manifestPath}, nil +} + +// marshalPreservingOrder serializes the manifest to JSON, preserving the key +// order from the original file. It does this by unmarshaling the original into +// an ordered structure, applying the new values, then re-serializing. +func marshalPreservingOrder(original []byte, manifest types.AppManifest) ([]byte, error) { + var originalKeys []string + if err := extractTopLevelKeyOrder(original, &originalKeys); err != nil { + return marshalFresh(manifest) + } + + newData, err := json.Marshal(manifest) + if err != nil { + return nil, err + } + var newMap map[string]json.RawMessage + if err := json.Unmarshal(newData, &newMap); err != nil { + return marshalFresh(manifest) + } + + // Build output with original key order, then append any new keys + type kv struct { + Key string + Value json.RawMessage + } + var ordered []kv + seen := make(map[string]bool) + + for _, key := range originalKeys { + if val, exists := newMap[key]; exists { + ordered = append(ordered, kv{Key: key, Value: val}) + seen[key] = true + } + } + for key, val := range newMap { + if !seen[key] { + ordered = append(ordered, kv{Key: key, Value: val}) + } + } + + // Manually build JSON with preserved order + buf := []byte("{\n") + for i, item := range ordered { + keyJSON, _ := json.Marshal(item.Key) + indented, _ := json.MarshalIndent(json.RawMessage(item.Value), " ", " ") + buf = fmt.Appendf(buf, " %s: %s", keyJSON, indented) + if i < len(ordered)-1 { + buf = append(buf, ',') + } + buf = append(buf, '\n') + } + buf = append(buf, '}') + buf = append(buf, '\n') + + return buf, nil +} + +func extractTopLevelKeyOrder(data []byte, keys *[]string) error { + var raw map[string]json.RawMessage + if err := json.Unmarshal(data, &raw); err != nil { + return err + } + // json.Unmarshal into map doesn't preserve order, so we need to parse tokens + dec := json.NewDecoder(bytes.NewReader(data)) + // Read opening brace + t, err := dec.Token() + if err != nil { + return err + } + if delim, ok := t.(json.Delim); !ok || delim != '{' { + return fmt.Errorf("expected opening brace") + } + for dec.More() { + t, err := dec.Token() + if err != nil { + return err + } + key, ok := t.(string) + if !ok { + continue + } + *keys = append(*keys, key) + // Skip the value + var skip json.RawMessage + if err := dec.Decode(&skip); err != nil { + return err + } + } + return nil +} + +func marshalFresh(manifest types.AppManifest) ([]byte, error) { + data, err := json.MarshalIndent(manifest, "", " ") + if err != nil { + return nil, err + } + return append(data, '\n'), nil +} diff --git a/internal/pkg/manifest/writeback_test.go b/internal/pkg/manifest/writeback_test.go new file mode 100644 index 00000000..46ffea41 --- /dev/null +++ b/internal/pkg/manifest/writeback_test.go @@ -0,0 +1,85 @@ +package manifest + +import ( + "testing" + + "github.com/slackapi/slack-cli/internal/shared/types" + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_WriteManifestLocal(t *testing.T) { + t.Run("writes merged manifest to existing file", func(t *testing.T) { + fs := afero.NewMemMapFs() + original := `{ + "display_information": { + "name": "Original App" + } +} +` + require.NoError(t, afero.WriteFile(fs, "/project/manifest.json", []byte(original), 0644)) + + manifest := types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Merged App", Description: "New desc"}, + } + + result, err := WriteManifestLocal(fs, "/project", manifest) + require.NoError(t, err) + assert.True(t, result.Written) + assert.Equal(t, "/project/manifest.json", result.FilePath) + + content, err := afero.ReadFile(fs, "/project/manifest.json") + require.NoError(t, err) + assert.Contains(t, string(content), "Merged App") + assert.Contains(t, string(content), "New desc") + }) + + t.Run("preserves original key order", func(t *testing.T) { + fs := afero.NewMemMapFs() + original := `{ + "settings": {"function_runtime": "local"}, + "display_information": {"name": "App"} +} +` + require.NoError(t, afero.WriteFile(fs, "/project/manifest.json", []byte(original), 0644)) + + manifest := types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + Settings: &types.AppSettings{FunctionRuntime: types.LocallyRun}, + } + + result, err := WriteManifestLocal(fs, "/project", manifest) + require.NoError(t, err) + assert.True(t, result.Written) + + content, err := afero.ReadFile(fs, "/project/manifest.json") + require.NoError(t, err) + contentStr := string(content) + // settings should come before display_information (matching original order) + settingsIdx := indexOf(contentStr, "settings") + displayIdx := indexOf(contentStr, "display_information") + assert.Less(t, settingsIdx, displayIdx, "key order should be preserved from original file") + }) + + t.Run("returns warning when manifest.json does not exist", func(t *testing.T) { + fs := afero.NewMemMapFs() + manifest := types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + } + + result, err := WriteManifestLocal(fs, "/project", manifest) + require.NoError(t, err) + assert.False(t, result.Written) + assert.Contains(t, result.Warning, "No manifest.json found") + }) +} + +func indexOf(s, substr string) int { + for i := range s { + if i+len(substr) <= len(s) && s[i:i+len(substr)] == substr { + return i + } + } + return -1 +} From 38f720fc06f0e5aebcb1aa7496f73daa1f7ae15e Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Thu, 11 Jun 2026 16:09:04 -0700 Subject: [PATCH 02/13] fix: refuse manifest sync when project source is remote Manifest sync would write to manifest.json regardless of the project's configured manifest source. Projects with ManifestSourceRemote treat app settings as the source of truth, so writing locally creates divergence. Refuse early with a clear remediation pointing at slack app settings. --- internal/pkg/manifest/sync.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/internal/pkg/manifest/sync.go b/internal/pkg/manifest/sync.go index c747ef1a..06675c80 100644 --- a/internal/pkg/manifest/sync.go +++ b/internal/pkg/manifest/sync.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/slackapi/slack-cli/internal/config" "github.com/slackapi/slack-cli/internal/shared" "github.com/slackapi/slack-cli/internal/shared/types" "github.com/slackapi/slack-cli/internal/slackerror" @@ -21,6 +22,19 @@ type SyncResult struct { // both manifests, computes diffs, prompts the user for resolution, writes // the merged result to both the API and the local file, and returns the result. func Sync(ctx context.Context, clients *shared.ClientFactory, app types.App, auth types.SlackAuth) (*SyncResult, error) { + manifestSource, err := clients.Config.ProjectConfig.GetManifestSource(ctx) + if err != nil { + return nil, err + } + if manifestSource.Equals(config.ManifestSourceRemote) { + return nil, slackerror.New(slackerror.ErrAppManifestUpdate). + WithMessage("Manifest sync is unavailable for projects with remote source of truth"). + WithRemediation("This project's manifest source is %s. Edit the manifest at %s.", + config.ManifestSourceRemote.Human(), + style.CommandText("slack app settings"), + ) + } + localManifest, err := clients.AppClient().Manifest.GetManifestLocal(ctx, clients.SDKConfig, clients.HookExecutor) if err != nil { return nil, slackerror.New("Failed to get local manifest").WithRootCause(err).WithCode(slackerror.ErrInvalidManifest) From 6f49023c361b1fef8410b9cbcfbef6915e2e15a7 Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Thu, 11 Jun 2026 22:20:51 -0700 Subject: [PATCH 03/13] fix: gate manifest sync command behind experiment Hides the slack manifest sync subcommand and the slack sync alias from --help, and refuses execution unless the manifest-sync experiment is enabled. Adds ErrExperimentRequired so the user gets a clear remediation pointing at the --experiment flag. --- cmd/manifest/sync.go | 16 ++++++++--- cmd/manifest/sync_test.go | 51 +++++++++++++++++++++++++++++++++++ internal/slackerror/errors.go | 6 +++++ 3 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 cmd/manifest/sync_test.go diff --git a/cmd/manifest/sync.go b/cmd/manifest/sync.go index a522bfb8..d9ab880b 100644 --- a/cmd/manifest/sync.go +++ b/cmd/manifest/sync.go @@ -4,9 +4,11 @@ import ( "github.com/opentracing/opentracing-go" "github.com/slackapi/slack-cli/internal/app" "github.com/slackapi/slack-cli/internal/cmdutil" + "github.com/slackapi/slack-cli/internal/experiment" "github.com/slackapi/slack-cli/internal/pkg/manifest" "github.com/slackapi/slack-cli/internal/prompts" "github.com/slackapi/slack-cli/internal/shared" + "github.com/slackapi/slack-cli/internal/slackerror" "github.com/slackapi/slack-cli/internal/style" "github.com/spf13/cobra" ) @@ -15,14 +17,22 @@ var manifestSyncFunc = manifest.Sync func NewSyncCommand(clients *shared.ClientFactory) *cobra.Command { cmd := &cobra.Command{ - Use: "sync", - Short: "Sync the app manifest between project and app settings", - Long: "Compare the local project manifest with app settings, resolve differences, and sync both to the same state.", + Use: "sync", + Short: "Sync the app manifest between project and app settings", + Long: "Compare the local project manifest with app settings, resolve differences, and sync both to the same state.", + Hidden: true, Example: style.ExampleCommandsf([]style.ExampleCommand{ {Command: "manifest sync", Meaning: "Sync project manifest with app settings"}, }), Args: cobra.NoArgs, PreRunE: func(cmd *cobra.Command, args []string) error { + if !clients.Config.WithExperimentOn(experiment.ManifestSync) { + return slackerror.New(slackerror.ErrExperimentRequired). + WithRemediation("Enable the %s experiment with %s", + style.Highlight(string(experiment.ManifestSync)), + style.CommandText("--experiment manifest-sync"), + ) + } return cmdutil.IsValidProjectDirectory(clients) }, RunE: func(cmd *cobra.Command, args []string) error { diff --git a/cmd/manifest/sync_test.go b/cmd/manifest/sync_test.go new file mode 100644 index 00000000..63c12951 --- /dev/null +++ b/cmd/manifest/sync_test.go @@ -0,0 +1,51 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package manifest + +import ( + "context" + "testing" + + "github.com/slackapi/slack-cli/internal/experiment" + "github.com/slackapi/slack-cli/internal/shared" + "github.com/slackapi/slack-cli/internal/slackerror" + "github.com/slackapi/slack-cli/test/testutil" + "github.com/spf13/cobra" +) + +func TestSyncCommand(t *testing.T) { + testutil.TableTestCommand(t, testutil.CommandTests{ + "errors when the manifest-sync experiment is off": { + Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { + cm.AddDefaultMocks() + cf.Config.LoadExperiments(ctx, cf.IO.PrintDebug) + }, + ExpectedError: slackerror.New(slackerror.ErrExperimentRequired), + }, + "passes the experiment gate when manifest-sync is enabled via flag": { + Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) { + cm.AddDefaultMocks() + cf.Config.ExperimentsFlag = []string{string(experiment.ManifestSync)} + cf.Config.LoadExperiments(ctx, cf.IO.PrintDebug) + }, + // We expect the command to fail downstream of the gate (no app + // selected, no SDK config), but NOT with ErrCommandUnavailable — + // the gate itself should pass. + ExpectedErrorStrings: []string{}, + }, + }, func(clients *shared.ClientFactory) *cobra.Command { + return NewSyncCommand(clients) + }) +} diff --git a/internal/slackerror/errors.go b/internal/slackerror/errors.go index 1959724f..985929ae 100644 --- a/internal/slackerror/errors.go +++ b/internal/slackerror/errors.go @@ -104,6 +104,7 @@ const ( ErrDotEnvPlaceholderNotFound = "dotenv_placeholder_not_found" ErrDotEnvVarMarshal = "dotenv_var_marshal_error" ErrEnterpriseNotFound = "enterprise_not_found" + ErrExperimentRequired = "experiment_required" ErrFailForSomeRequests = "failed_for_some_requests" ErrFailToGetTeamsForRestrictedUser = "fail_to_get_teams_for_restricted_user" ErrFailedAddingCollaborator = "failed_adding_collaborator" @@ -733,6 +734,11 @@ Otherwise start your app for local development with: %s`, Message: "The `enterprise` was not found", }, + ErrExperimentRequired: { + Code: ErrExperimentRequired, + Message: "This command requires an experiment to be enabled", + }, + ErrFailForSomeRequests: { Code: ErrFailForSomeRequests, Message: "At least one request was not cancelled", From f531346b27998c9da07221cf09ac1b4df37bc9fb Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Fri, 12 Jun 2026 12:44:12 -0700 Subject: [PATCH 04/13] fix: return error on path collisions when unflattening manifests setNestedValue used an unchecked type assertion that panicked when flattened paths disagreed about whether a node is a scalar or an object (e.g. "a.b" alongside "a.b.c"). Replace the assertion with the comma-ok form and propagate ErrInvalidManifest. Also catch the mirror case where a leaf would silently overwrite an existing nested object. --- internal/pkg/manifest/merge.go | 32 ++++++++++++++++++++++------- internal/pkg/manifest/merge_test.go | 28 +++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/internal/pkg/manifest/merge.go b/internal/pkg/manifest/merge.go index f17e73ac..48ff7cba 100644 --- a/internal/pkg/manifest/merge.go +++ b/internal/pkg/manifest/merge.go @@ -6,6 +6,7 @@ import ( "maps" "github.com/slackapi/slack-cli/internal/shared/types" + "github.com/slackapi/slack-cli/internal/slackerror" ) // Resolution represents which side the user chose for a given field. @@ -101,7 +102,10 @@ func MergeAllFrom(local, remote types.AppManifest, diffs *DiffResult, strategy M // unflatten converts a flat dot-notation map back into a nested structure, // then marshals/unmarshals into AppManifest. func unflatten(flat map[string]any) (types.AppManifest, error) { - nested := unflattenToMap(flat) + nested, err := unflattenToMap(flat) + if err != nil { + return types.AppManifest{}, err + } data, err := json.Marshal(nested) if err != nil { return types.AppManifest{}, fmt.Errorf("failed to marshal merged manifest: %w", err) @@ -114,29 +118,43 @@ func unflatten(flat map[string]any) (types.AppManifest, error) { } // unflattenToMap reconstructs a nested map from dot-notation paths. -func unflattenToMap(flat map[string]any) map[string]any { +func unflattenToMap(flat map[string]any) (map[string]any, error) { result := make(map[string]any) for path, value := range flat { - setNestedValue(result, path, value) + if err := setNestedValue(result, path, value); err != nil { + return nil, err + } } - return result + return result, nil } -func setNestedValue(m map[string]any, path string, value any) { +func setNestedValue(m map[string]any, path string, value any) error { parts := splitPath(path) current := m for i, part := range parts { if i == len(parts)-1 { + if existing, exists := current[part]; exists { + if _, isMap := existing.(map[string]any); isMap { + return slackerror.New(slackerror.ErrInvalidManifest). + WithMessage("Conflicting types at manifest path %q: leaf value would overwrite a nested object", path) + } + } current[part] = value - return + return nil } next, exists := current[part] if !exists { next = make(map[string]any) current[part] = next } - current = next.(map[string]any) + nextMap, ok := next.(map[string]any) + if !ok { + return slackerror.New(slackerror.ErrInvalidManifest). + WithMessage("Conflicting types at manifest path %q: cannot descend into a non-object value", path) + } + current = nextMap } + return nil } func splitPath(path string) []string { diff --git a/internal/pkg/manifest/merge_test.go b/internal/pkg/manifest/merge_test.go index 39b020d0..06cbd8a5 100644 --- a/internal/pkg/manifest/merge_test.go +++ b/internal/pkg/manifest/merge_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/slackapi/slack-cli/internal/shared/types" + "github.com/slackapi/slack-cli/internal/slackerror" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -99,6 +100,33 @@ func Test_Merge(t *testing.T) { } } +func Test_unflatten_PathCollision(t *testing.T) { + tests := map[string]struct { + flat map[string]any + }{ + "leaf at parent path collides with deeper path": { + flat: map[string]any{ + "a.b": "scalar", + "a.b.c": "deep", + }, + }, + "deeper path collides with leaf at parent": { + flat: map[string]any{ + "a.b.c": "deep", + "a.b": "scalar", + }, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + _, err := unflatten(tc.flat) + require.Error(t, err, "expected path-collision error, got nil") + slackErr := slackerror.ToSlackError(err) + assert.Equal(t, slackerror.ErrInvalidManifest, slackErr.Code) + }) + } +} + func Test_MergeAllFrom(t *testing.T) { local := types.AppManifest{ DisplayInformation: types.DisplayInformation{Name: "Local", Description: "Local desc"}, From 52a2fbfced4de9f39e0d49517718084dcbc602e4 Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Fri, 12 Jun 2026 13:07:42 -0700 Subject: [PATCH 05/13] fix: write manifest.json atomically via temp file rename afero.WriteFile truncates the destination before writing, so an interrupted write could leave manifest.json empty or half-written right after the API had already been updated. Switch to a sibling temp file plus rename so the destination is only ever the old contents or the new contents. --- internal/pkg/manifest/writeback.go | 16 +++++++++++++++- internal/pkg/manifest/writeback_test.go | 19 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/internal/pkg/manifest/writeback.go b/internal/pkg/manifest/writeback.go index e32f0503..53b6f6f1 100644 --- a/internal/pkg/manifest/writeback.go +++ b/internal/pkg/manifest/writeback.go @@ -46,13 +46,27 @@ func WriteManifestLocal(fs afero.Fs, workingDir string, manifest types.AppManife return WriteBackResult{}, fmt.Errorf("failed to serialize merged manifest: %w", err) } - if err := afero.WriteFile(fs, manifestPath, merged, os.FileMode(0644)); err != nil { + if err := atomicWriteFile(fs, manifestPath, merged, 0644); err != nil { return WriteBackResult{}, fmt.Errorf("failed to write %s: %w", manifestFileName, err) } return WriteBackResult{Written: true, FilePath: manifestPath}, nil } +// atomicWriteFile writes to a sibling temp file and renames it over the +// destination so an interrupted write cannot leave the destination truncated. +func atomicWriteFile(fs afero.Fs, dest string, data []byte, mode os.FileMode) error { + tmp := dest + ".tmp" + if err := afero.WriteFile(fs, tmp, data, mode); err != nil { + return err + } + if err := fs.Rename(tmp, dest); err != nil { + _ = fs.Remove(tmp) + return err + } + return nil +} + // marshalPreservingOrder serializes the manifest to JSON, preserving the key // order from the original file. It does this by unmarshaling the original into // an ordered structure, applying the new values, then re-serializing. diff --git a/internal/pkg/manifest/writeback_test.go b/internal/pkg/manifest/writeback_test.go index 46ffea41..c5256040 100644 --- a/internal/pkg/manifest/writeback_test.go +++ b/internal/pkg/manifest/writeback_test.go @@ -62,6 +62,25 @@ func Test_WriteManifestLocal(t *testing.T) { assert.Less(t, settingsIdx, displayIdx, "key order should be preserved from original file") }) + t.Run("does not leave a temporary file after success", func(t *testing.T) { + fs := afero.NewMemMapFs() + original := `{"display_information":{"name":"Original"}}` + require.NoError(t, afero.WriteFile(fs, "/project/manifest.json", []byte(original), 0644)) + + manifest := types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "Merged"}, + } + + _, err := WriteManifestLocal(fs, "/project", manifest) + require.NoError(t, err) + + entries, err := afero.ReadDir(fs, "/project") + require.NoError(t, err) + for _, e := range entries { + assert.NotContains(t, e.Name(), ".tmp", "temporary file should be cleaned up after atomic write") + } + }) + t.Run("returns warning when manifest.json does not exist", func(t *testing.T) { fs := afero.NewMemMapFs() manifest := types.AppManifest{ From be92c9657cabf3f623220e9faef0db852cdf6520 Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Fri, 12 Jun 2026 13:08:55 -0700 Subject: [PATCH 06/13] fix: refresh cached manifest hash after sync pushes to app settings After Sync's UpdateApp succeeds, the cached manifest hash still reflected the pre-sync upstream, so a follow-up install or deploy would treat the merged result as drift and prompt the user to sync again. Persist the new hash right after the API push so the cache matches the current upstream state. --- internal/pkg/manifest/sync.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/pkg/manifest/sync.go b/internal/pkg/manifest/sync.go index 06675c80..e31d3c71 100644 --- a/internal/pkg/manifest/sync.go +++ b/internal/pkg/manifest/sync.go @@ -82,6 +82,15 @@ func Sync(ctx context.Context, clients *shared.ClientFactory, app types.App, aut } clients.IO.PrintInfo(ctx, false, " %s Updated app settings", style.Green("✓")) + // Refresh the cached manifest hash so install/deploy don't see drift. + hash, err := clients.Config.ProjectConfig.Cache().NewManifestHash(ctx, merged) + if err != nil { + return nil, slackerror.New("Failed to hash merged manifest").WithRootCause(err) + } + if err := clients.Config.ProjectConfig.Cache().SetManifestHash(ctx, app.AppID, hash); err != nil { + return nil, slackerror.New("Failed to cache merged manifest hash").WithRootCause(err) + } + // Write back to local file workingDir := clients.SDKConfig.WorkingDirectory writeResult, err := WriteManifestLocal(clients.Fs, workingDir, merged) From 4dc53cc5972d0e575dffd0e45109226eaa798df0 Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Fri, 12 Jun 2026 13:10:57 -0700 Subject: [PATCH 07/13] fix: honor --force flag in non-TTY manifest sync The non-TTY remediation pointed users at --force, but the flag was ignored. --force is a global persistent flag, so the message was quietly misleading. Treat --force as MergeAllLocal so CI flows can push their project manifest to app settings non-interactively. --- internal/pkg/manifest/sync.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/internal/pkg/manifest/sync.go b/internal/pkg/manifest/sync.go index e31d3c71..297a143c 100644 --- a/internal/pkg/manifest/sync.go +++ b/internal/pkg/manifest/sync.go @@ -61,17 +61,24 @@ func Sync(ctx context.Context, clients *shared.ClientFactory, app types.App, aut DisplayDiffs(ctx, clients.IO, diffs) - if !clients.IO.IsTTY() { + var merged types.AppManifest + switch { + case clients.Config.ForceFlag: + merged, err = MergeAllFrom(localManifest.AppManifest, remoteManifest.AppManifest, diffs, MergeAllLocal) + if err != nil { + return nil, err + } + case !clients.IO.IsTTY(): return nil, slackerror.New(slackerror.ErrAppManifestUpdate). - WithRemediation("Run %s interactively to resolve manifest differences, or use %s to overwrite app settings", + WithRemediation("Run %s interactively to resolve manifest differences, or pass %s to push the project manifest to app settings", style.CommandText("slack manifest sync"), style.CommandText("--force"), ) - } - - merged, err := resolveInteractively(ctx, clients, localManifest.AppManifest, remoteManifest.AppManifest, diffs) - if err != nil { - return nil, err + default: + merged, err = resolveInteractively(ctx, clients, localManifest.AppManifest, remoteManifest.AppManifest, diffs) + if err != nil { + return nil, err + } } // Push merged manifest to API From faf52bc10fc00e444b317a896deeceaa8b123946 Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Fri, 12 Jun 2026 13:11:28 -0700 Subject: [PATCH 08/13] fix: surface marshal errors in writeback instead of dropping them marshalPreservingOrder discarded errors from json.Marshal and json.MarshalIndent with _, so a marshal failure on a malformed key or value would write malformed JSON like '" : "' to disk and return success. Propagate the errors so the user sees a real failure and the file is not overwritten. --- internal/pkg/manifest/writeback.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/pkg/manifest/writeback.go b/internal/pkg/manifest/writeback.go index 53b6f6f1..ea08a6d8 100644 --- a/internal/pkg/manifest/writeback.go +++ b/internal/pkg/manifest/writeback.go @@ -108,8 +108,14 @@ func marshalPreservingOrder(original []byte, manifest types.AppManifest) ([]byte // Manually build JSON with preserved order buf := []byte("{\n") for i, item := range ordered { - keyJSON, _ := json.Marshal(item.Key) - indented, _ := json.MarshalIndent(json.RawMessage(item.Value), " ", " ") + keyJSON, err := json.Marshal(item.Key) + if err != nil { + return nil, fmt.Errorf("failed to marshal manifest key %q: %w", item.Key, err) + } + indented, err := json.MarshalIndent(json.RawMessage(item.Value), " ", " ") + if err != nil { + return nil, fmt.Errorf("failed to marshal manifest value at %q: %w", item.Key, err) + } buf = fmt.Appendf(buf, " %s: %s", keyJSON, indented) if i < len(ordered)-1 { buf = append(buf, ',') From c911ada0ef2d51a58e4f2e5dce1cfcde28662a41 Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Fri, 12 Jun 2026 13:13:15 -0700 Subject: [PATCH 09/13] fix: surface a warning when manifest.json key order is rewritten marshalPreservingOrder silently fell back to default key ordering when the original file could not be parsed (BOM, trailing comma, malformed JSON), so users had no idea their carefully-ordered manifest had been reshuffled. Return a fellBack signal and surface it on WriteBackResult.Warning so the user is informed. --- internal/pkg/manifest/writeback.go | 31 +++++++++++++++---------- internal/pkg/manifest/writeback_test.go | 15 ++++++++++++ 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/internal/pkg/manifest/writeback.go b/internal/pkg/manifest/writeback.go index ea08a6d8..3d7d8f3b 100644 --- a/internal/pkg/manifest/writeback.go +++ b/internal/pkg/manifest/writeback.go @@ -41,7 +41,7 @@ func WriteManifestLocal(fs afero.Fs, workingDir string, manifest types.AppManife return WriteBackResult{}, fmt.Errorf("failed to read %s: %w", manifestFileName, err) } - merged, err := marshalPreservingOrder(original, manifest) + merged, fellBack, err := marshalPreservingOrder(original, manifest) if err != nil { return WriteBackResult{}, fmt.Errorf("failed to serialize merged manifest: %w", err) } @@ -50,7 +50,11 @@ func WriteManifestLocal(fs afero.Fs, workingDir string, manifest types.AppManife return WriteBackResult{}, fmt.Errorf("failed to write %s: %w", manifestFileName, err) } - return WriteBackResult{Written: true, FilePath: manifestPath}, nil + result := WriteBackResult{Written: true, FilePath: manifestPath} + if fellBack { + result.Warning = fmt.Sprintf("Could not parse the original %s, so its key order was not preserved", manifestFileName) + } + return result, nil } // atomicWriteFile writes to a sibling temp file and renames it over the @@ -67,22 +71,25 @@ func atomicWriteFile(fs afero.Fs, dest string, data []byte, mode os.FileMode) er return nil } -// marshalPreservingOrder serializes the manifest to JSON, preserving the key -// order from the original file. It does this by unmarshaling the original into -// an ordered structure, applying the new values, then re-serializing. -func marshalPreservingOrder(original []byte, manifest types.AppManifest) ([]byte, error) { +// marshalPreservingOrder serializes the manifest to JSON, preserving the +// top-level key order from the original file. The returned fellBack is true +// when the original could not be parsed and the result was emitted with +// default key order instead. +func marshalPreservingOrder(original []byte, manifest types.AppManifest) (data []byte, fellBack bool, err error) { var originalKeys []string if err := extractTopLevelKeyOrder(original, &originalKeys); err != nil { - return marshalFresh(manifest) + fresh, err := marshalFresh(manifest) + return fresh, true, err } newData, err := json.Marshal(manifest) if err != nil { - return nil, err + return nil, false, err } var newMap map[string]json.RawMessage if err := json.Unmarshal(newData, &newMap); err != nil { - return marshalFresh(manifest) + fresh, err := marshalFresh(manifest) + return fresh, true, err } // Build output with original key order, then append any new keys @@ -110,11 +117,11 @@ func marshalPreservingOrder(original []byte, manifest types.AppManifest) ([]byte for i, item := range ordered { keyJSON, err := json.Marshal(item.Key) if err != nil { - return nil, fmt.Errorf("failed to marshal manifest key %q: %w", item.Key, err) + return nil, false, fmt.Errorf("failed to marshal manifest key %q: %w", item.Key, err) } indented, err := json.MarshalIndent(json.RawMessage(item.Value), " ", " ") if err != nil { - return nil, fmt.Errorf("failed to marshal manifest value at %q: %w", item.Key, err) + return nil, false, fmt.Errorf("failed to marshal manifest value at %q: %w", item.Key, err) } buf = fmt.Appendf(buf, " %s: %s", keyJSON, indented) if i < len(ordered)-1 { @@ -125,7 +132,7 @@ func marshalPreservingOrder(original []byte, manifest types.AppManifest) ([]byte buf = append(buf, '}') buf = append(buf, '\n') - return buf, nil + return buf, false, nil } func extractTopLevelKeyOrder(data []byte, keys *[]string) error { diff --git a/internal/pkg/manifest/writeback_test.go b/internal/pkg/manifest/writeback_test.go index c5256040..337664ba 100644 --- a/internal/pkg/manifest/writeback_test.go +++ b/internal/pkg/manifest/writeback_test.go @@ -81,6 +81,21 @@ func Test_WriteManifestLocal(t *testing.T) { } }) + t.Run("warns when the original manifest.json cannot be parsed", func(t *testing.T) { + fs := afero.NewMemMapFs() + // Original is malformed JSON — extractTopLevelKeyOrder will fail. + require.NoError(t, afero.WriteFile(fs, "/project/manifest.json", []byte("not json"), 0644)) + + manifest := types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + } + + result, err := WriteManifestLocal(fs, "/project", manifest) + require.NoError(t, err) + assert.True(t, result.Written) + assert.Contains(t, result.Warning, "key order was not preserved") + }) + t.Run("returns warning when manifest.json does not exist", func(t *testing.T) { fs := afero.NewMemMapFs() manifest := types.AppManifest{ From 68ff27ed43296421099a1bad5ef8a28fc5ffd764 Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Fri, 12 Jun 2026 13:14:32 -0700 Subject: [PATCH 10/13] fix: propagate marshal errors from valuesEqual instead of declaring inequality valuesEqual returned false when json.Marshal failed, so equal values with a marshal hiccup got flagged as a phantom diff that the user was then prompted to resolve. Surface the error up through Diff so the caller sees a real failure instead of fabricated differences. --- internal/pkg/manifest/diff.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/internal/pkg/manifest/diff.go b/internal/pkg/manifest/diff.go index 0922d091..43da2a06 100644 --- a/internal/pkg/manifest/diff.go +++ b/internal/pkg/manifest/diff.go @@ -45,10 +45,10 @@ func Diff(local, remote types.AppManifest) (*DiffResult, error) { if err != nil { return nil, fmt.Errorf("failed to flatten remote manifest: %w", err) } - return diffFlat(localFlat, remoteFlat), nil + return diffFlat(localFlat, remoteFlat) } -func diffFlat(local, remote map[string]any) *DiffResult { +func diffFlat(local, remote map[string]any) (*DiffResult, error) { result := &DiffResult{} seen := make(map[string]bool) @@ -63,7 +63,11 @@ func diffFlat(local, remote map[string]any) *DiffResult { }) continue } - if !valuesEqual(localVal, remoteVal) { + equal, err := valuesEqual(localVal, remoteVal) + if err != nil { + return nil, fmt.Errorf("failed to compare manifest values at %q: %w", path, err) + } + if !equal { result.Diffs = append(result.Diffs, FieldDiff{ Path: path, Type: DiffModified, @@ -84,17 +88,17 @@ func diffFlat(local, remote map[string]any) *DiffResult { }) } - return result + return result, nil } -func valuesEqual(a, b any) bool { +func valuesEqual(a, b any) (bool, error) { aJSON, err := json.Marshal(a) if err != nil { - return false + return false, err } bJSON, err := json.Marshal(b) if err != nil { - return false + return false, err } - return string(aJSON) == string(bJSON) + return string(aJSON) == string(bJSON), nil } From 49f12f36cd138ac8b2d023c31df0c348b87492ae Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Fri, 12 Jun 2026 13:16:26 -0700 Subject: [PATCH 11/13] fix: escape dots in flattened path segments to preserve dotted keys Manifest function and workflow IDs may contain literal dots (e.g. "slack.users.lookup"). The original flatten/splitPath used dot as both delimiter and a valid key character, so a key like that round- tripped to a 4-level nested map and silently corrupted the merge. Backslash-escape dots and backslashes in path segments and honor the escape in splitPath. --- internal/pkg/manifest/flatten.go | 17 +++++++++++++-- internal/pkg/manifest/flatten_test.go | 21 ++++++++++++++++++ internal/pkg/manifest/merge.go | 31 +++++++++++++++++++-------- 3 files changed, 58 insertions(+), 11 deletions(-) diff --git a/internal/pkg/manifest/flatten.go b/internal/pkg/manifest/flatten.go index 8d706105..a7988471 100644 --- a/internal/pkg/manifest/flatten.go +++ b/internal/pkg/manifest/flatten.go @@ -4,12 +4,17 @@ import ( "encoding/json" "fmt" "sort" + "strings" ) // Flatten converts a manifest (as JSON-serializable struct) into a flat map // where keys are dot-notation paths and values are the leaf values. // Arrays are treated as leaf values (not recursed into individually) because // array element identity is ambiguous without a key field. +// +// Keys that contain literal dots (e.g. function IDs like "slack.users.lookup") +// have those dots backslash-escaped in the path so flatten/unflatten round- +// trip cleanly. splitPath honors the same escape sequence. func Flatten(manifest any) (map[string]any, error) { data, err := json.Marshal(manifest) if err != nil { @@ -26,9 +31,9 @@ func Flatten(manifest any) (map[string]any, error) { func flattenRecursive(prefix string, data map[string]any, result map[string]any) { for key, value := range data { - fullKey := key + fullKey := escapePathSegment(key) if prefix != "" { - fullKey = prefix + "." + key + fullKey = prefix + "." + fullKey } switch v := value.(type) { case map[string]any: @@ -39,6 +44,14 @@ func flattenRecursive(prefix string, data map[string]any, result map[string]any) } } +// escapePathSegment backslash-escapes the path delimiter and the escape +// character so a key containing a literal dot survives flatten/splitPath. +func escapePathSegment(s string) string { + s = strings.ReplaceAll(s, `\`, `\\`) + s = strings.ReplaceAll(s, `.`, `\.`) + return s +} + // SortedKeys returns the keys of a flat map in sorted order for deterministic output. func SortedKeys(m map[string]any) []string { keys := make([]string, 0, len(m)) diff --git a/internal/pkg/manifest/flatten_test.go b/internal/pkg/manifest/flatten_test.go index d490d721..d271639d 100644 --- a/internal/pkg/manifest/flatten_test.go +++ b/internal/pkg/manifest/flatten_test.go @@ -96,6 +96,27 @@ func Test_Flatten(t *testing.T) { } } +func Test_Flatten_DottedKeyRoundTrip(t *testing.T) { + // Manifest function IDs may contain literal dots (e.g. "slack.users.lookup"). + // Flatten followed by unflatten must reproduce the original structure. + manifest := types.AppManifest{ + DisplayInformation: types.DisplayInformation{Name: "App"}, + Functions: map[string]types.ManifestFunction{ + "slack.users.lookup": {Title: "Lookup", Description: "Lookup a user"}, + }, + } + + flat, err := Flatten(manifest) + require.NoError(t, err) + + round, err := unflatten(flat) + require.NoError(t, err) + + assert.Contains(t, round.Functions, "slack.users.lookup") + assert.Equal(t, "Lookup", round.Functions["slack.users.lookup"].Title) + assert.Equal(t, "Lookup a user", round.Functions["slack.users.lookup"].Description) +} + func Test_SortedKeys(t *testing.T) { m := map[string]any{ "z.field": "val", diff --git a/internal/pkg/manifest/merge.go b/internal/pkg/manifest/merge.go index 48ff7cba..2cb62697 100644 --- a/internal/pkg/manifest/merge.go +++ b/internal/pkg/manifest/merge.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "maps" + "strings" "github.com/slackapi/slack-cli/internal/shared/types" "github.com/slackapi/slack-cli/internal/slackerror" @@ -157,21 +158,33 @@ func setNestedValue(m map[string]any, path string, value any) error { return nil } +// splitPath splits a flatten-produced dot-delimited path into segments, +// honoring backslash-escaped dots so that keys containing literal dots +// (e.g. function IDs like "slack.users.lookup") round-trip correctly. func splitPath(path string) []string { var parts []string - current := "" + var current strings.Builder + escaped := false for _, ch := range path { - if ch == '.' { - if current != "" { - parts = append(parts, current) - current = "" + if escaped { + current.WriteRune(ch) + escaped = false + continue + } + switch ch { + case '\\': + escaped = true + case '.': + if current.Len() > 0 { + parts = append(parts, current.String()) + current.Reset() } - } else { - current += string(ch) + default: + current.WriteRune(ch) } } - if current != "" { - parts = append(parts, current) + if current.Len() > 0 { + parts = append(parts, current.String()) } return parts } From d57b96564fa753dd3aba5c8504857e873d4f5777 Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Fri, 12 Jun 2026 14:28:05 -0700 Subject: [PATCH 12/13] chore: add Apache 2.0 license headers to new manifest files Required by the license_check workflow. --- cmd/manifest/sync.go | 14 ++++++++++++++ internal/pkg/manifest/diff.go | 14 ++++++++++++++ internal/pkg/manifest/diff_test.go | 14 ++++++++++++++ internal/pkg/manifest/display.go | 14 ++++++++++++++ internal/pkg/manifest/flatten.go | 14 ++++++++++++++ internal/pkg/manifest/flatten_test.go | 14 ++++++++++++++ internal/pkg/manifest/merge.go | 14 ++++++++++++++ internal/pkg/manifest/merge_test.go | 14 ++++++++++++++ internal/pkg/manifest/sync.go | 14 ++++++++++++++ internal/pkg/manifest/writeback.go | 14 ++++++++++++++ internal/pkg/manifest/writeback_test.go | 14 ++++++++++++++ 11 files changed, 154 insertions(+) diff --git a/cmd/manifest/sync.go b/cmd/manifest/sync.go index d9ab880b..68449a7a 100644 --- a/cmd/manifest/sync.go +++ b/cmd/manifest/sync.go @@ -1,3 +1,17 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package manifest import ( diff --git a/internal/pkg/manifest/diff.go b/internal/pkg/manifest/diff.go index 43da2a06..7fc4e9c2 100644 --- a/internal/pkg/manifest/diff.go +++ b/internal/pkg/manifest/diff.go @@ -1,3 +1,17 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package manifest import ( diff --git a/internal/pkg/manifest/diff_test.go b/internal/pkg/manifest/diff_test.go index a661c083..1d7f00b8 100644 --- a/internal/pkg/manifest/diff_test.go +++ b/internal/pkg/manifest/diff_test.go @@ -1,3 +1,17 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package manifest import ( diff --git a/internal/pkg/manifest/display.go b/internal/pkg/manifest/display.go index 2a4d854a..a6110af1 100644 --- a/internal/pkg/manifest/display.go +++ b/internal/pkg/manifest/display.go @@ -1,3 +1,17 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package manifest import ( diff --git a/internal/pkg/manifest/flatten.go b/internal/pkg/manifest/flatten.go index a7988471..21f4853a 100644 --- a/internal/pkg/manifest/flatten.go +++ b/internal/pkg/manifest/flatten.go @@ -1,3 +1,17 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package manifest import ( diff --git a/internal/pkg/manifest/flatten_test.go b/internal/pkg/manifest/flatten_test.go index d271639d..ec057513 100644 --- a/internal/pkg/manifest/flatten_test.go +++ b/internal/pkg/manifest/flatten_test.go @@ -1,3 +1,17 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package manifest import ( diff --git a/internal/pkg/manifest/merge.go b/internal/pkg/manifest/merge.go index 2cb62697..1495b434 100644 --- a/internal/pkg/manifest/merge.go +++ b/internal/pkg/manifest/merge.go @@ -1,3 +1,17 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package manifest import ( diff --git a/internal/pkg/manifest/merge_test.go b/internal/pkg/manifest/merge_test.go index 06cbd8a5..f25d3467 100644 --- a/internal/pkg/manifest/merge_test.go +++ b/internal/pkg/manifest/merge_test.go @@ -1,3 +1,17 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package manifest import ( diff --git a/internal/pkg/manifest/sync.go b/internal/pkg/manifest/sync.go index 297a143c..59d09d96 100644 --- a/internal/pkg/manifest/sync.go +++ b/internal/pkg/manifest/sync.go @@ -1,3 +1,17 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package manifest import ( diff --git a/internal/pkg/manifest/writeback.go b/internal/pkg/manifest/writeback.go index 3d7d8f3b..725b2208 100644 --- a/internal/pkg/manifest/writeback.go +++ b/internal/pkg/manifest/writeback.go @@ -1,3 +1,17 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package manifest import ( diff --git a/internal/pkg/manifest/writeback_test.go b/internal/pkg/manifest/writeback_test.go index 337664ba..475327af 100644 --- a/internal/pkg/manifest/writeback_test.go +++ b/internal/pkg/manifest/writeback_test.go @@ -1,3 +1,17 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package manifest import ( From 22e3a29e8ef50539dfa8dc7cd1180bf9906b4e7f Mon Sep 17 00:00:00 2001 From: Michael Brooks Date: Fri, 12 Jun 2026 14:47:49 -0700 Subject: [PATCH 13/13] refactor: move manifest package out of internal/pkg internal/pkg is a generic bucket the project is migrating away from. Move the existing internal/pkg/manifest package (validate plus the new sync/diff/merge/flatten/writeback/display logic) up to internal/manifest and update its three importers. --- cmd/manifest/sync.go | 2 +- cmd/manifest/validate.go | 2 +- internal/{pkg => }/manifest/diff.go | 0 internal/{pkg => }/manifest/diff_test.go | 0 internal/{pkg => }/manifest/display.go | 0 internal/{pkg => }/manifest/flatten.go | 0 internal/{pkg => }/manifest/flatten_test.go | 0 internal/{pkg => }/manifest/merge.go | 0 internal/{pkg => }/manifest/merge_test.go | 0 internal/{pkg => }/manifest/sync.go | 0 internal/{pkg => }/manifest/validate.go | 0 internal/{pkg => }/manifest/validate_test.go | 0 internal/{pkg => }/manifest/writeback.go | 0 internal/{pkg => }/manifest/writeback_test.go | 0 internal/pkg/apps/install.go | 2 +- 15 files changed, 3 insertions(+), 3 deletions(-) rename internal/{pkg => }/manifest/diff.go (100%) rename internal/{pkg => }/manifest/diff_test.go (100%) rename internal/{pkg => }/manifest/display.go (100%) rename internal/{pkg => }/manifest/flatten.go (100%) rename internal/{pkg => }/manifest/flatten_test.go (100%) rename internal/{pkg => }/manifest/merge.go (100%) rename internal/{pkg => }/manifest/merge_test.go (100%) rename internal/{pkg => }/manifest/sync.go (100%) rename internal/{pkg => }/manifest/validate.go (100%) rename internal/{pkg => }/manifest/validate_test.go (100%) rename internal/{pkg => }/manifest/writeback.go (100%) rename internal/{pkg => }/manifest/writeback_test.go (100%) diff --git a/cmd/manifest/sync.go b/cmd/manifest/sync.go index 68449a7a..16bc5072 100644 --- a/cmd/manifest/sync.go +++ b/cmd/manifest/sync.go @@ -19,7 +19,7 @@ import ( "github.com/slackapi/slack-cli/internal/app" "github.com/slackapi/slack-cli/internal/cmdutil" "github.com/slackapi/slack-cli/internal/experiment" - "github.com/slackapi/slack-cli/internal/pkg/manifest" + "github.com/slackapi/slack-cli/internal/manifest" "github.com/slackapi/slack-cli/internal/prompts" "github.com/slackapi/slack-cli/internal/shared" "github.com/slackapi/slack-cli/internal/slackerror" diff --git a/cmd/manifest/validate.go b/cmd/manifest/validate.go index 6ce962fc..71bdb084 100644 --- a/cmd/manifest/validate.go +++ b/cmd/manifest/validate.go @@ -23,7 +23,7 @@ import ( "github.com/slackapi/slack-cli/internal/app" "github.com/slackapi/slack-cli/internal/cmdutil" "github.com/slackapi/slack-cli/internal/iostreams" - "github.com/slackapi/slack-cli/internal/pkg/manifest" + "github.com/slackapi/slack-cli/internal/manifest" "github.com/slackapi/slack-cli/internal/prompts" "github.com/slackapi/slack-cli/internal/shared" "github.com/slackapi/slack-cli/internal/shared/types" diff --git a/internal/pkg/manifest/diff.go b/internal/manifest/diff.go similarity index 100% rename from internal/pkg/manifest/diff.go rename to internal/manifest/diff.go diff --git a/internal/pkg/manifest/diff_test.go b/internal/manifest/diff_test.go similarity index 100% rename from internal/pkg/manifest/diff_test.go rename to internal/manifest/diff_test.go diff --git a/internal/pkg/manifest/display.go b/internal/manifest/display.go similarity index 100% rename from internal/pkg/manifest/display.go rename to internal/manifest/display.go diff --git a/internal/pkg/manifest/flatten.go b/internal/manifest/flatten.go similarity index 100% rename from internal/pkg/manifest/flatten.go rename to internal/manifest/flatten.go diff --git a/internal/pkg/manifest/flatten_test.go b/internal/manifest/flatten_test.go similarity index 100% rename from internal/pkg/manifest/flatten_test.go rename to internal/manifest/flatten_test.go diff --git a/internal/pkg/manifest/merge.go b/internal/manifest/merge.go similarity index 100% rename from internal/pkg/manifest/merge.go rename to internal/manifest/merge.go diff --git a/internal/pkg/manifest/merge_test.go b/internal/manifest/merge_test.go similarity index 100% rename from internal/pkg/manifest/merge_test.go rename to internal/manifest/merge_test.go diff --git a/internal/pkg/manifest/sync.go b/internal/manifest/sync.go similarity index 100% rename from internal/pkg/manifest/sync.go rename to internal/manifest/sync.go diff --git a/internal/pkg/manifest/validate.go b/internal/manifest/validate.go similarity index 100% rename from internal/pkg/manifest/validate.go rename to internal/manifest/validate.go diff --git a/internal/pkg/manifest/validate_test.go b/internal/manifest/validate_test.go similarity index 100% rename from internal/pkg/manifest/validate_test.go rename to internal/manifest/validate_test.go diff --git a/internal/pkg/manifest/writeback.go b/internal/manifest/writeback.go similarity index 100% rename from internal/pkg/manifest/writeback.go rename to internal/manifest/writeback.go diff --git a/internal/pkg/manifest/writeback_test.go b/internal/manifest/writeback_test.go similarity index 100% rename from internal/pkg/manifest/writeback_test.go rename to internal/manifest/writeback_test.go diff --git a/internal/pkg/apps/install.go b/internal/pkg/apps/install.go index e660b9d3..93f61285 100644 --- a/internal/pkg/apps/install.go +++ b/internal/pkg/apps/install.go @@ -25,7 +25,7 @@ import ( "github.com/slackapi/slack-cli/internal/config" "github.com/slackapi/slack-cli/internal/experiment" "github.com/slackapi/slack-cli/internal/icon" - manifestpkg "github.com/slackapi/slack-cli/internal/pkg/manifest" + manifestpkg "github.com/slackapi/slack-cli/internal/manifest" "github.com/slackapi/slack-cli/internal/shared" "github.com/slackapi/slack-cli/internal/shared/types" "github.com/slackapi/slack-cli/internal/slackerror"