feat: fall back to manifest.json when get-manifest hook is unavailable by srtaalej · Pull Request #629 · slackapi/slack-cli

srtaalej · 2026-08-10T17:10:22Z

Summary

GetManifestLocal now falls back to reading manifest.json directly from the project root when no get-manifest hook is available
Renamed readManifestFile to getManifestFromFile to match the getManifestFromHook naming convention

Closes #627

Context

Projects without a get-manifest hook but with a static manifest.json had no way to read the local manifest. This adds manifest.json as a fallback source when the hook isn't defined, allowing the CLI to manage the manifest directly for projects that don't use the hook.

The hook remains the preferred source when available — a hook means the SDK manages the manifest, while a bare manifest.json means it's CLI-managed.

Test plan

make test passes
Existing tests updated to verify hook-first, file-fallback behavior

Manual testing

Create a Bolt JS app using slack create with the bolt-js-starter-template:

slack create my-test-app --template bolt-js-starter-template
cd my-test-app

Test 1: Hook path still works (existing behavior)

slack manifest info --skip-update

Should display the manifest via the get-manifest hook.

Test 2: Fallback to manifest.json when no hook is defined

# Replace hooks with an empty definition
echo '{"hooks":{}}' > .slack/hooks.json

# CLI should fall back to reading manifest.json directly
slack manifest info --skip-update

# Restore
git checkout .slack/hooks.json

Should display the manifest read from manifest.json.

…reads

…n test

codecov · 2026-08-10T17:22:49Z

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.20%. Comparing base (73dde37) to head (5a0ec27).

Files with missing lines	Patch %	Lines
internal/app/app.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #629      +/-   ##
==========================================
+ Coverage   78.18%   78.20%   +0.02%     
==========================================
  Files         239      239              
  Lines       18136    18149      +13     
==========================================
+ Hits        14179    14193      +14     
+ Misses       3957     3956       -1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zimeg

@srtaalej Super appreciate the changes toward this 🌟

I left a comment on precedence of manifestations and am thinking that the fallback manifest.json file allows us to consider the manifest "CLI-managed" while with a hook ought mean not.

This pattern has a slow rollout with hooks needing to be updated too, but I'm most optimistic for the changes it'll unlock with confidence in changing a project manifest.

zimeg · 2026-08-10T22:48:23Z

+	if exists, _ := afero.Exists(c.fs, manifestPath); exists {
+		return c.readManifestFile(manifestPath)
+	}
+	return c.getManifestFromHook(ctx, sdkConfig, hookExecutor)


🔭 thought: We should reverse this order to check if a get-manifest hook exists and fallback to reading "manifest.json" file. Deprecating the hook might happen at the hook package while the CLI continues to support it.

zimeg

🔍 Before testing more I want to cut scope on this change. I think other PRs can follow to ensure we're not clobbering an SDK managed manifest!

zimeg · 2026-08-11T22:44:42Z

 	if !exists {
-		return WriteBackResult{
-			Warning: fmt.Sprintf("No %s found in project root — merged manifest was not written locally", manifestFileName),
-		}, nil
+		fresh, err := marshalFresh(manifest)
+		if err != nil {
+			return WriteBackResult{}, fmt.Errorf("failed to serialize merged manifest: %w", err)
+		}
+		if err := atomicWriteFile(fs, manifestPath, fresh, 0644); err != nil {
+			return WriteBackResult{}, fmt.Errorf("failed to write %s: %w", manifestFileName, err)
+		}
+		return WriteBackResult{Written: true, FilePath: manifestPath}, nil
 	}


🦠 suggestion: Let's keep this change for a separate PR!

zimeg · 2026-08-11T22:48:59Z

+	return c.readManifestFile(manifestPath)
+}
+
+func (c *ManifestClient) readManifestFile(path string) (types.SlackYaml, error) {


Suggested change

func (c *ManifestClient) readManifestFile(path string) (types.SlackYaml, error) {

func (c *ManifestClient) getManifestFromFile(sdkConfig hooks.SDKCLIConfig) (types.SlackYaml, error) {

🌵 thought: I think we should match the function name convention here and also keep path specific logic contained within this function. I'm less confident of the second thought FWIW!

… tests

zimeg

@srtaalej Nice I think this'll be a big unlock before the experiment concludes! I'm leaving a few comments but logic LGTM.

🧪 Tests can perhaps be scoped more narrow to the hook and fallback changes? I'm interested in this so we keep confidence in both what's changing and what's not.
🔭 The sync command might have follow up to require a manifest hook doesn't exist. IMHO this is important so we're confident in writing back to expected JSON instead of whatever a hook might gather from.
🪝 We should discuss a plan to remove get-manifest from the Bolt hook implementations. I forget if we can confirm current CLI versions as part of hooks but we might prefer the plain JSON for apps going forward.

If test changes seem alright and so do discussion points above let me know for a final test and review! 🪬 ✨

zimeg · 2026-09-04T21:02:02Z

-				assert.Equal(t, tc.expectedManifest, actualManifest)
+				assert.Equal(t, tc.expectedName, result.DisplayInformation.Name)


🧪 suggestion: Might we keep the entire manifest assertion here? These unit tests are capturing a meaningful interface to build confidence to IMHO.

zimeg · 2026-09-04T21:04:01Z

+	hookTests := map[string]struct {
+		hookOutput   string
+		hookErr      error
+		expectedName string
+		expectedErr  string
+	}{


🧪 suggestion: Let's combine these test cases with the ones above for this function. This'll help us extend these later if needed with a solid foundation.

zimeg · 2026-09-04T21:12:32Z

-		// Verify remote value was used — the merged manifest should have "Remote" description
-		assert.Equal(t, "Remote", result.Merged.DisplayInformation.Description)
-	})
+	mergeStrategyTests := map[string]struct {


👁️‍🗨️ question: Were these test expectations changed? I'm noticing we don't assert a call to the UpdateApp function now which concerns me somewhat since logic adjacent is being changed in this PR too.

feat: add --manifest-source flag to run and deploy commands

07f7c15

srtaalej requested a review from a team as a code owner August 10, 2026 17:10

srtaalej self-assigned this Aug 10, 2026

feat: prefer manifest.json over get-manifest hook for local manifest …

159f342

…reads

srtaalej added enhancement M-T: A feature request for new functionality semver:minor Use on pull requests to describe the release version increment labels Aug 10, 2026

srtaalej changed the title ~~feat: add --manifest-source flag to run and deploy commands~~ feat: default to manifest.json over get-manifest hook Aug 10, 2026

test: rename misleading test case for manifest-source flag validation

4ed2068

srtaalej added this to the Next Release milestone Aug 10, 2026

srtaalej added 2 commits August 10, 2026 13:16

refactor: consolidate repetitive hook fallback tests into table-drive…

48bf21a

…n test

refactor: consolidate sync merge strategy tests into table-driven test

b2d74ed

srtaalej closed this Aug 10, 2026

srtaalej reopened this Aug 10, 2026

srtaalej closed this Aug 10, 2026

srtaalej deleted the ale-add-force-to-run branch August 10, 2026 17:41

srtaalej restored the ale-add-force-to-run branch August 10, 2026 17:41

srtaalej reopened this Aug 10, 2026

revert: remove --manifest-source flag (out of scope for this PR)

9a27637

zimeg requested changes Aug 10, 2026

View reviewed changes

fix: prefer get-manifest hook over manifest.json with file as fallback

da81af0

zimeg requested changes Aug 11, 2026

View reviewed changes

srtaalej and others added 3 commits August 14, 2026 16:08

fix: prefer manifest.json file over get-manifest hook and reduce scope

eacb9da

Merge branch 'main' into ale-add-force-to-run

99bda78

refactor: remove duplicate path construction and unrelated run.go diff

3ea43ab

srtaalej changed the title ~~feat: default to manifest.json over get-manifest hook~~ feat: fall back to manifest.json when get-manifest hook is unavailable Aug 17, 2026

srtaalej and others added 2 commits August 17, 2026 16:05

fix: use hook-first precedence with manifest.json as fallback

885dcec

Merge branch 'main' into ale-add-force-to-run

a4e02ed

srtaalej requested a review from zimeg August 27, 2026 15:15

Merge branch 'main' into ale-add-force-to-run

1721353

srtaalej and others added 2 commits August 28, 2026 15:54

Merge branch 'main' into ale-add-force-to-run

6ba97d4

refactor: remove dead guard, fix error code, and consolidate fallback…

3b0e5ca

… tests

zimeg modified the milestones: v4.7.0, Next Release Aug 28, 2026

srtaalej added 2 commits August 31, 2026 11:15

Merge branch 'main' into ale-add-force-to-run

d079dfc

Merge branch 'main' into ale-add-force-to-run

5a0ec27

zimeg reviewed Sep 4, 2026

View reviewed changes

Navigation Menu

Uh oh!

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

feat: fall back to manifest.json when get-manifest hook is unavailable#629
srtaalej wants to merge 17 commits into
mainfrom
ale-add-force-to-run

srtaalej commented Aug 10, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

zimeg left a comment

Uh oh!

zimeg Aug 10, 2026

Uh oh!

zimeg left a comment

Uh oh!

zimeg Aug 11, 2026

Uh oh!

zimeg Aug 11, 2026

Uh oh!

zimeg left a comment

Uh oh!

zimeg Sep 4, 2026

Uh oh!

zimeg Sep 4, 2026

Uh oh!

zimeg Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	func (c *ManifestClient) readManifestFile(path string) (types.SlackYaml, error) {
	func (c *ManifestClient) getManifestFromFile(sdkConfig hooks.SDKCLIConfig) (types.SlackYaml, error) {

		assert.Equal(t, tc.expectedManifest, actualManifest)
		assert.Equal(t, tc.expectedName, result.DisplayInformation.Name)

Uh oh!

Conversation

srtaalej commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Context

Test plan

Manual testing

Uh oh!

codecov Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

zimeg Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

zimeg Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

zimeg Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

zimeg Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

zimeg Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

zimeg Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

srtaalej commented Aug 10, 2026 •

edited

Loading

codecov Bot commented Aug 10, 2026 •

edited

Loading