feat: fall back to manifest.json when get-manifest hook is unavailable

internal/app/app.go

@@ -41,7 +41,7 @@ func NewClient(
 	os types.Os,
 ) *Client {
 	return &Client{
-		Manifest:           NewManifestClient(apiClient, config),
+		Manifest:           NewManifestClient(apiClient, config, fs),
 		AppClientInterface: NewAppClient(config, fs, os),
 	}
 }

internal/app/manifest.go

@@ -17,18 +17,23 @@ package app
 import (
 	"context"
 	"encoding/json"
+	"path/filepath"
 	"strings"
 
 	"github.com/slackapi/slack-cli/internal/api"
 	"github.com/slackapi/slack-cli/internal/config"
 	"github.com/slackapi/slack-cli/internal/hooks"
 	"github.com/slackapi/slack-cli/internal/shared/types"
 	"github.com/slackapi/slack-cli/internal/slackerror"
+	"github.com/spf13/afero"
 )
 
+const manifestFileName = "manifest.json"
+
 // ManifestClient can manage the state of the project's app manifest file
 type ManifestClient struct {
 	apiClient        api.APIInterface
+	fs               afero.Fs
 	domainAuthTokens string
 	Env              map[string]string
 }
@@ -59,23 +64,45 @@ func SetManifestEnvTeamVars(manifestEnv map[string]string, appTeamDomain string,
 func NewManifestClient(
 	apiClient api.APIInterface,
 	config *config.Config,
+	fs afero.Fs,
 ) *ManifestClient {
 	client := &ManifestClient{
 		apiClient:        apiClient,
+		fs:               fs,
 		domainAuthTokens: config.DomainAuthTokens,
 		Env:              config.ManifestEnv,
 	}
 	return client
 }
 
-// GetManifestLocal gathers manifest content from the "get-manifest" hook
+// GetManifestLocal reads the local manifest, preferring the "get-manifest" hook
+// when available. Falls back to reading manifest.json from the project root.
 func (c *ManifestClient) GetManifestLocal(ctx context.Context, sdkConfig hooks.SDKCLIConfig, hookExecutor hooks.HookExecutor) (types.SlackYaml, error) {
-	var sl types.SlackYaml
+	if sdkConfig.Hooks.GetManifest.IsAvailable() {
+		return c.getManifestFromHook(ctx, sdkConfig, hookExecutor)
+	}
+	return c.getManifestFromFile(sdkConfig)
+}
 
-	if !sdkConfig.Hooks.GetManifest.IsAvailable() {
-		return sl, slackerror.New(slackerror.ErrSDKHookNotFound).
-			WithMessage("The `get-manifest` script was not found")
+func (c *ManifestClient) getManifestFromFile(sdkConfig hooks.SDKCLIConfig) (types.SlackYaml, error) {
+	var sl types.SlackYaml
+	manifestPath := filepath.Join(sdkConfig.WorkingDirectory, manifestFileName)
+	data, err := afero.ReadFile(c.fs, manifestPath)
+	if err != nil {
+		return sl, slackerror.New("Failed to read manifest file").
+			WithRootCause(err).
+			WithCode(slackerror.ErrNoFile)
 	}
+	if err := json.Unmarshal(data, &sl); err != nil {
+		return sl, slackerror.New("Failed to parse manifest file").
+			WithRootCause(err).
+			WithCode(slackerror.ErrInvalidManifest)
+	}
+	return sl, nil
+}
+
+func (c *ManifestClient) getManifestFromHook(ctx context.Context, sdkConfig hooks.SDKCLIConfig, hookExecutor hooks.HookExecutor) (types.SlackYaml, error) {
+	var sl types.SlackYaml
 
 	var manifestHookOpts = hooks.HookExecOpts{
 		Args: map[string]string{
@@ -104,7 +131,6 @@ func (c *ManifestClient) GetManifestLocal(ctx context.Context, sdkConfig hooks.S
 	if start != -1 {
 		slackManifestInfo = slackManifestInfo[start:]
 	} else {
-		// the app manifest has to be a json so needs to have the character `{`
 		return sl, slackerror.New("Invalid app manifest format, must be valid JSON").
 			WithRootCause(err).
 			WithCode(slackerror.ErrInvalidManifest)

internal/app/manifest_test.go

@@ -24,6 +24,7 @@ import (
 	"github.com/slackapi/slack-cli/internal/slackcontext"
 	"github.com/slackapi/slack-cli/internal/slackdeps"
 	"github.com/slackapi/slack-cli/internal/slackerror"
+	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
@@ -68,77 +69,128 @@ func Test_AppManifest_SetManifestEnvTeamVars(t *testing.T) {
 }
 
 func Test_AppManifest_GetManifestLocal(t *testing.T) {
-	tests := map[string]struct {
-		mockManifestInfo string
-		mockManifestErr  error
-		expectedErr      error
-		expectedManifest types.SlackYaml
+	fallbackTests := map[string]struct {
+		hookCommand     string
+		hookOutput      string
+		manifestFile    string
+		expectedName    string
+		expectedErrCode string
+		expectHookCall  bool
 	}{
-		"errors if no get-manifest hook exists": {
-			expectedErr: slackerror.New(slackerror.ErrSDKHookNotFound),
-		},
-		"returns an existing manifest without errors": {
-			mockManifestInfo: `{"display_information":{"name":"my-example-app"}}`,
-			expectedManifest: types.SlackYaml{
-				AppManifest: types.AppManifest{
-					DisplayInformation: types.DisplayInformation{
-						Name: "my-example-app",
-					},
-				},
-			},
+		"prefers hook over manifest.json when hook is available": {
+			hookCommand:    "echo manifest",
+			hookOutput:     `{"display_information":{"name":"hook-app"}}`,
+			manifestFile:   `{"display_information":{"name":"file-app"}}`,
+			expectedName:   "hook-app",
+			expectHookCall: true,
 		},
-		"errors if the hook execution errors": {
-			mockManifestInfo: `{}`,
-			mockManifestErr:  slackerror.New(slackerror.ErrNoFile),
-			expectedErr:      slackerror.New(slackerror.ErrInvalidManifest),
+		"falls back to manifest.json when no hook exists": {
+			manifestFile: `{"display_information":{"name":"file-app"}}`,
+			expectedName: "file-app",
 		},
-		"parses a manifest with random leading characters": {
-			mockManifestInfo: `...{"display_information":{"name":"my-showcased-app"}}`,
-			expectedManifest: types.SlackYaml{
-				AppManifest: types.AppManifest{
-					DisplayInformation: types.DisplayInformation{
-						Name: "my-showcased-app",
-					},
-				},
-			},
+		"errors if no hook and no manifest.json": {
+			expectedErrCode: slackerror.ErrNoFile,
 		},
-		"errors if a manifest is not present in output": {
-			mockManifestInfo: `...unknown`,
-			expectedErr:      slackerror.New(slackerror.ErrInvalidManifest),
+		"errors if manifest.json contains invalid JSON": {
+			manifestFile:    `not json`,
+			expectedErrCode: slackerror.ErrInvalidManifest,
 		},
 	}
-	for name, tc := range tests {
+	for name, tc := range fallbackTests {
 		t.Run(name, func(t *testing.T) {
 			ctx := slackcontext.MockContext(t.Context())
-			mockManifestEnv := map[string]string{"EXAMPLE": "12"}
+			fsMock := slackdeps.NewFsMock()
+			osMock := slackdeps.NewOsMock()
+			osMock.AddDefaultMocks()
+			configMock := config.NewConfig(fsMock, osMock)
+			configMock.DomainAuthTokens = "api.slack.com"
 			mockSDKConfig := hooks.NewSDKConfigMock()
+			mockSDKConfig.WorkingDirectory = "/project"
+
+			if tc.hookCommand != "" {
+				mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: tc.hookCommand}
+			} else {
+				mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest"}
+			}
+
+			if tc.manifestFile != "" {
+				_ = fsMock.MkdirAll("/project", 0755)
+				_ = afero.WriteFile(fsMock, "/project/manifest.json", []byte(tc.manifestFile), 0644)
+			}
+
 			mockHookExecutor := &hooks.MockHookExecutor{}
-			if tc.mockManifestInfo != "" {
-				mockSDKConfig.Hooks.GetManifest = hooks.HookScript{
-					Name:    "GetManifest",
-					Command: "cat manifest.json",
-				}
+			if tc.hookCommand != "" {
 				mockHookExecutor.On("Execute", mock.Anything, mock.Anything).
-					Return(tc.mockManifestInfo, tc.mockManifestErr)
+					Return(tc.hookOutput, nil)
+			}
+
+			manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock)
+			result, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor)
+
+			if tc.expectedErrCode != "" {
+				require.Error(t, err)
+				assert.Equal(t, tc.expectedErrCode, err.(*slackerror.Error).Code)
 			} else {
-				mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest"}
+				require.NoError(t, err)
+				assert.Equal(t, tc.expectedName, result.DisplayInformation.Name)
+			}
+
+			if tc.expectHookCall {
+				mockHookExecutor.AssertCalled(t, "Execute", mock.Anything, mock.Anything)
+			} else {
+				mockHookExecutor.AssertNotCalled(t, "Execute", mock.Anything, mock.Anything)
 			}
+		})
+	}
+
+	hookTests := map[string]struct {
+		hookOutput   string
+		hookErr      error
+		expectedName string
+		expectedErr  string
+	}{
+		"returns manifest from hook output": {
+			hookOutput:   `{"display_information":{"name":"hook-app"}}`,
+			expectedName: "hook-app",
+		},
+		"parses hook output with leading characters": {
+			hookOutput:   `...{"display_information":{"name":"hook-app"}}`,
+			expectedName: "hook-app",
+		},
+		"errors if hook execution errors": {
+			hookOutput:  `{}`,
+			hookErr:     slackerror.New(slackerror.ErrNoFile),
+			expectedErr: slackerror.ErrInvalidManifest,
+		},
+		"errors if hook output has no JSON": {
+			hookOutput:  `...unknown`,
+			expectedErr: slackerror.ErrInvalidManifest,
+		},
+	}
+	for name, tc := range hookTests {
+		t.Run(name, func(t *testing.T) {
+			ctx := slackcontext.MockContext(t.Context())
 			fsMock := slackdeps.NewFsMock()
 			osMock := slackdeps.NewOsMock()
 			osMock.AddDefaultMocks()
 			configMock := config.NewConfig(fsMock, osMock)
 			configMock.DomainAuthTokens = "api.slack.com"
-			configMock.ManifestEnv = mockManifestEnv
-			manifestClient := NewManifestClient(&api.APIMock{}, configMock)
+			mockSDKConfig := hooks.NewSDKConfigMock()
+			mockSDKConfig.Hooks.GetManifest = hooks.HookScript{Name: "GetManifest", Command: "generate-manifest"}
+
+			mockHookExecutor := &hooks.MockHookExecutor{}
+			mockHookExecutor.On("Execute", mock.Anything, mock.Anything).
+				Return(tc.hookOutput, tc.hookErr)
+
+			manifestClient := NewManifestClient(&api.APIMock{}, configMock, fsMock)
 
-			actualManifest, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor)
-			if tc.expectedErr != nil {
+			result, err := manifestClient.GetManifestLocal(ctx, mockSDKConfig, mockHookExecutor)
+			if tc.expectedErr != "" {
 				require.Error(t, err)
-				assert.Equal(t,
-					tc.expectedErr.(*slackerror.Error).Code, err.(*slackerror.Error).Code)
+				assert.Equal(t, tc.expectedErr, err.(*slackerror.Error).Code)
 			} else {
 				require.NoError(t, err)
-				assert.Equal(t, tc.expectedManifest, actualManifest)
+				assert.Equal(t, tc.expectedName, result.DisplayInformation.Name)
 			}
 		})
 	}
@@ -186,7 +238,7 @@ func Test_AppManifest_GetManifestRemote(t *testing.T) {
 			apic := &api.APIMock{}
 			apic.On("ExportAppManifest", mock.Anything, mock.Anything, mock.Anything).
 				Return(api.ExportAppResult{Manifest: tc.mockManifestResponse}, tc.mockManifestError)
-			manifestClient := NewManifestClient(apic, configMock)
+			manifestClient := NewManifestClient(apic, configMock, fsMock)
 
 			manifest, err := manifestClient.GetManifestRemote(ctx, tc.mockToken, tc.mockAppID)
 			if tc.expectedError != nil {

internal/manifest/sync_test.go

@@ -173,52 +173,45 @@ func Test_Sync(t *testing.T) {
 		assert.Equal(t, slackerror.ErrAppManifestUpdate, slackErr.Code)
 	})
 
-	t.Run("force flag merges all local and pushes to API", func(t *testing.T) {
-		f := newSyncTestFixture(t)
-		f.projectConfig.On("GetManifestSource", mock.Anything).Return(config.ManifestSourceLocal, nil)
-		f.manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).
-			Return(localManifest, nil)
-		f.manifestMock.On("GetManifestRemote", mock.Anything, mock.Anything, mock.Anything).
-			Return(remoteManifest, nil)
-		f.clients.Config.ForceFlag = true
-		f.clientsMock.API.On("UpdateApp", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
-			Return(api.UpdateAppResult{}, nil)
-		f.cacheMock.On("NewManifestHash", mock.Anything, mock.Anything).Return(cache.Hash("newhash"), nil)
-		f.cacheMock.On("SetManifestHash", mock.Anything, mock.Anything, mock.Anything).Return(nil)
-		_ = afero.WriteFile(f.fs, "/project/manifest.json", []byte(`{"display_information":{"name":"App"}}`), 0644)
-
-		result, err := Sync(f.ctx, f.clients, testApp, testAuth)
-
-		require.NoError(t, err)
-		require.NotNil(t, result)
-		assert.True(t, result.HasDifferences)
-		assert.True(t, result.WriteBack.Written)
-		f.clientsMock.API.AssertCalled(t, "UpdateApp", mock.Anything, "xoxb-test", "A123", mock.Anything, true, true)
-	})
-
-	t.Run("force-remote flag merges all remote and pushes to API", func(t *testing.T) {
-		f := newSyncTestFixture(t)
-		f.projectConfig.On("GetManifestSource", mock.Anything).Return(config.ManifestSourceLocal, nil)
-		f.manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).
-			Return(localManifest, nil)
-		f.manifestMock.On("GetManifestRemote", mock.Anything, mock.Anything, mock.Anything).
-			Return(remoteManifest, nil)
-		f.clients.Config.ForceRemoteFlag = true
-		f.clientsMock.API.On("UpdateApp", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
-			Return(api.UpdateAppResult{}, nil)
-		f.cacheMock.On("NewManifestHash", mock.Anything, mock.Anything).Return(cache.Hash("newhash"), nil)
-		f.cacheMock.On("SetManifestHash", mock.Anything, mock.Anything, mock.Anything).Return(nil)
-		_ = afero.WriteFile(f.fs, "/project/manifest.json", []byte(`{"display_information":{"name":"App"}}`), 0644)
-
-		result, err := Sync(f.ctx, f.clients, testApp, testAuth)
-
-		require.NoError(t, err)
-		require.NotNil(t, result)
-		assert.True(t, result.HasDifferences)
-		assert.True(t, result.WriteBack.Written)
-		// Verify remote value was used — the merged manifest should have "Remote" description
-		assert.Equal(t, "Remote", result.Merged.DisplayInformation.Description)
-	})
+	mergeStrategyTests := map[string]struct {
+		forceFlag       bool
+		forceRemoteFlag bool
+		expectedDesc    string
+	}{
+		"force flag merges all local": {
+			forceFlag:    true,
+			expectedDesc: "Local",
+		},
+		"force-remote flag merges all remote": {
+			forceRemoteFlag: true,
+			expectedDesc:    "Remote",
+		},
+	}
+	for name, tc := range mergeStrategyTests {
+		t.Run(name, func(t *testing.T) {
+			f := newSyncTestFixture(t)
+			f.projectConfig.On("GetManifestSource", mock.Anything).Return(config.ManifestSourceLocal, nil)
+			f.manifestMock.On("GetManifestLocal", mock.Anything, mock.Anything, mock.Anything).
+				Return(localManifest, nil)
+			f.manifestMock.On("GetManifestRemote", mock.Anything, mock.Anything, mock.Anything).
+				Return(remoteManifest, nil)
+			f.clients.Config.ForceFlag = tc.forceFlag
+			f.clients.Config.ForceRemoteFlag = tc.forceRemoteFlag
+			f.clientsMock.API.On("UpdateApp", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
+				Return(api.UpdateAppResult{}, nil)
+			f.cacheMock.On("NewManifestHash", mock.Anything, mock.Anything).Return(cache.Hash("newhash"), nil)
+			f.cacheMock.On("SetManifestHash", mock.Anything, mock.Anything, mock.Anything).Return(nil)
+			_ = afero.WriteFile(f.fs, "/project/manifest.json", []byte(`{"display_information":{"name":"App"}}`), 0644)
+
+			result, err := Sync(f.ctx, f.clients, testApp, testAuth)
+
+			require.NoError(t, err)
+			require.NotNil(t, result)
+			assert.True(t, result.HasDifferences)
+			assert.True(t, result.WriteBack.Written)
+			assert.Equal(t, tc.expectedDesc, result.Merged.DisplayInformation.Description)
+		})
+	}
 
 	t.Run("API UpdateApp failure is propagated", func(t *testing.T) {
 		f := newSyncTestFixture(t)