refactor: consolidate version access to slackcontext.Version(ctx) by mwbrooks · Pull Request #431 · slackapi/slack-cli

mwbrooks · 2026-03-23T21:59:51Z

Changelog

N/A

Summary

This pull request consolidates version access to slackcontext.Version(ctx).

Currently, the version is accessed with version.Raw(), version.Get(), clients.config.Version, and slackcontext.Version(ctx). This is brittle, because it requires all version fields to be set to the same version. Additionally, the version value is passed as a parameter through some functions, which creates more risk of the incorrect version bring used.

Migrates version.Raw() callers to slackcontext.Version(ctx).
Standardizes on version.Raw(), previously it was a mix - most used .Raw(), but the recoveryFunc used .Get(). This could lead to difficulty associating log traces.
Removes clients.config.Version.
Replaces version string parameter with ctx parameter.
Simplifies mocks that required the version parameter.
Version now set once in main.go.

Open Questions

Please weigh-in on this question around whether to use .Get() or .Raw().

Test Steps

1. Version output

$ ./bin/slack --version
# → Confirm: the version string is displayed correctly (e.g. v0.x.x or a dev build SHA)

2. Version in debug output

$ ./bin/slack auth list --verbose --skip-update 2>&1 | grep -i version
# → Confirm: the CLI version appears in "Resolving logstash host"
# → Confirm: the CLI version appears in "FlushToLogstash"
# → Confirm no warnings about "version could not be found in context"

3.. User-Agent in API debug output

$ ./bin/slack auth login --verbose --skip-update 2>&1 | grep "User-Agent"
# - Confirm: the CLI version appears in "HTTP Request User-Agent"
# CTRL+C

4.. Log file version

$ ./bin/slack auth list
$ cat ~/.slack/logs/slack-debug-*.log | grep "Slack-CLI Version"
# → Confirm: last line in the log file contains the correct Slack-CLI Version value (not empty)
# → Example: "Slack-CLI Version: [v3.15.0]"

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

The CLIVersion field and SetVersion functional option on ClientFactory were a workaround for a circular dependency that no longer exists. The API client reads version from context via slackcontext.Version(ctx), making this indirection unnecessary. Replace with direct version.Raw() calls.

Consumers now call version.Raw() directly instead of reading from Config.Version, which was a redundant copy set in two places.

Remove re-added clients.Config.Version assignments that were intentionally deleted in the Config.Version field refactor.

Replace direct version.Raw() calls with slackcontext.Version(ctx) across all callers for improved testability via context-based dependency injection. - slackcontext.Version(ctx) now falls back to version.Raw() when the context is missing the version, returning both the fallback value and an error so callers can log a warning - ResolveLogstashHost no longer takes a cliVersion parameter; it reads the version from context internally - Tracking tests use context-based version injection instead of mutating the global version.Version variable

codecov · 2026-03-23T22:03:10Z

Codecov Report

❌ Patch coverage is 56.00000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.23%. Comparing base (97fbfe8) to head (aec060b).

Files with missing lines	Patch %	Lines
internal/iostreams/logfile.go	0.00%	4 Missing ⚠️
main.go	0.00%	3 Missing ⚠️
internal/auth/auth.go	60.00%	1 Missing and 1 partial ⚠️
internal/tracking/tracking.go	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #431      +/-   ##
==========================================
- Coverage   70.27%   70.23%   -0.04%     
==========================================
  Files         220      220              
  Lines       18498    18506       +8     
==========================================
- Hits        12999    12998       -1     
- Misses       4324     4331       +7     
- Partials     1175     1177       +2

☔ View full report in Codecov by Sentry.
📢 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.

mwbrooks

Comments for the wise reviewers!

mwbrooks · 2026-03-23T22:01:54Z

cmd/app/add_test.go


 	clientsMock.Auth.On("ResolveAPIHost", mock.Anything, mock.Anything, mock.Anything).
 		Return("api host")
-	clientsMock.Auth.On("ResolveLogstashHost", mock.Anything, mock.Anything, mock.Anything).


note: This is the bulk of the changes - the version parameter was removed from the ResolveLogstashHost, which simplifies our testing mocks.

🌟 praise: @mwbrooks Amazing use of the slackcontext values for gathering this now!

- ResolveLogstashHost(ctx, clients.Config.APIHostResolved, clients.Config.Version) + ResolveLogstashHost(ctx, clients.Config.APIHostResolved)

mwbrooks · 2026-03-23T22:02:24Z

cmd/root.go

-	// TODO(slackcontext) Consolidate storing CLI version to slackcontext
-	clients.Config.Version = version.Raw()


🪓 Removing the TODO to consolidate the version to slackcontext.

mwbrooks · 2026-03-23T22:05:27Z

internal/auth/auth.go

+	cliVersion, err := slackcontext.Version(ctx)
+	if err != nil {
+		c.io.PrintDebug(ctx, "Warning: %s", err.Error())
+	}


note: slackcontext.Version(ctx) will return an error when ctx doesn't include the version field. This could happen if the function is called with context.Background() or t.Context(), which doesn't have the Slack CLI context properties.

When an error happens, slackcontext.Version(ctx) falls back on version.Raw(), which is set by the compiler. So, it should be very reliable at returning the correct version even if the context was incorrectly passed into the function. However, still want to handle the error by printing an warning to the debug logs, so that it can be known and fixed.

falliing back on version.Raw() is nice!

mwbrooks · 2026-03-23T22:05:55Z

internal/config/config.go

 	TeamFlag                string
 	TokenFlag               string
 	NoColor                 bool
-	Version                 string


🪓 shared.Config.Version

mwbrooks · 2026-03-23T22:07:02Z

internal/slackcontext/slackcontext.go

+// Version returns the CLI version associated with `ctx`. If the version is not
+// found in the context, it falls back to version.Raw() and returns an error to
+// signal that the context was not properly initialized.


note: The important design decision is written in this comment.

mwbrooks · 2026-03-23T22:10:26Z

internal/tracking/tracking_test.go

-			setup: func(cfg *config.Config) {
-				cfg.Version = "v4.2.0"
+			ctxSetup: func(ctx context.Context) context.Context {
+				return slackcontext.SetVersion(ctx, "v4.2.0")


note: This is a safer mock now :)

mwbrooks · 2026-03-23T22:44:41Z

main.go

 	// Set context values
 	sessionID := uuid.New().String()
-	cliVersion := version.Get()
+	cliVersion := version.Raw()


💬 discussion: This is an important decision that we should feel confident in.

Originally, the version used by slackcontext and log traces was version.Get() (always prefixes the 'v'). But, Logstash and API calls used version.Raw() (git tag, which has the 'v' by convention but not guaranteed).

This wasn't a problem because our git tags always have a prefixed 'v' (e.g. v3.15.0). However, if we created a git tag without the 'v' then version would be different in different places.

:two-cents: Consistency is more important than format. This PR does introduce the consistency, I hope, because we consolidate the version access through slackcontext.Version(ctx). w.r.t the version format, I went with version.Raw() so that we know our git tag is the source of truth. If someone creates a git tag with a different format, at least we know it all lines up. But, I'm open to either.

Thoughts?

I agree with using Raw() and i like the idea of having git tag as our source of truth! it means its less likely for our logstash, api calls, etc to have the wrong version. also if we did mistakenly make a git tag w/out the 'v', that version would be consistent everywhere else so it would be easier to correct

💡 praise: Thanks for calling this out! I'd been stumped on what might be best too.

👾 thought: I was leaning toward using version.Get to guarantee that releases use the "v" prefix to avoid errors with the User-Agent format:

slack-cli/v3.4.5-example-2-ga7e71fe (os: darwin)

But agree that consistent formats is better! We might lean on CI to package releases to avoid nuance as well 🎁

🔭 question: Should we also remove version.Get in a follow up PR? It might be a chance to ask less from the regex package:

slack-cli/internal/version/version.go

Lines 43 to 50 in 2cf691e

// Get the version and format it (e.g. `v1.0.0`)

func Get() string {

version := Version

if match, _ := regexp.MatchString(`^[^v]`, version); match {

version = "v" + version

}

return version

}

mwbrooks · 2026-03-23T22:45:16Z

main.go


 		var ctx = context.Background()
 		ctx = slackcontext.SetSessionID(ctx, uuid.New().String())
+		ctx = slackcontext.SetVersion(ctx, version.Raw())


note: Cannot re-use the cliVersion above because this is the recovery() function.

…refactor-1

… into mwbrooks-cli-version-refactor-1

srtaalej

i think consolidating into slackcontext.Version(ctx) was the right call! LGTM ⭐

zimeg

@mwbrooks Nice changes to these processes 📺 ✨

I left a handful of comments toward these changes without blockers! When the time is best please do merge 🚢

Most comments are around packages - I'm curious about a simple version package most.

zimeg · 2026-03-25T17:07:15Z

cmd/app/add_test.go


 	clientsMock.Auth.On("ResolveAPIHost", mock.Anything, mock.Anything, mock.Anything).
 		Return("api host")
-	clientsMock.Auth.On("ResolveLogstashHost", mock.Anything, mock.Anything, mock.Anything).


🌟 praise: @mwbrooks Amazing use of the slackcontext values for gathering this now!

- ResolveLogstashHost(ctx, clients.Config.APIHostResolved, clients.Config.Version) + ResolveLogstashHost(ctx, clients.Config.APIHostResolved)

zimeg · 2026-03-25T17:26:43Z

internal/tracking/tracking.go

+	if err != nil {
+		ioStream.PrintDebug(ctx, "Warning: %s", err.Error())
+	}
+	versionString, _ := strings.CutPrefix(cliVersion, "v")


Suggested change

versionString, _ := strings.CutPrefix(cliVersion, "v")

versionString := strings.TrimPrefix(cliVersion, "v")

🔭 suggestion(non-blocking): The unhandled "found" value might be more confusing than a different method here? No blocker since that's unchanged from before but it might be a good time to improve this for later.

🔗 https://pkg.go.dev/strings#TrimPrefix

zimeg · 2026-03-25T21:54:12Z

main.go

 	// Set context values
 	sessionID := uuid.New().String()
-	cliVersion := version.Get()
+	cliVersion := version.Raw()


💡 praise: Thanks for calling this out! I'd been stumped on what might be best too.

👾 thought: I was leaning toward using version.Get to guarantee that releases use the "v" prefix to avoid errors with the User-Agent format:

slack-cli/v3.4.5-example-2-ga7e71fe (os: darwin)

But agree that consistent formats is better! We might lean on CI to package releases to avoid nuance as well 🎁

🔭 question: Should we also remove version.Get in a follow up PR? It might be a chance to ask less from the regex package:

slack-cli/internal/version/version.go

Lines 43 to 50 in 2cf691e

// Get the version and format it (e.g. `v1.0.0`)

func Get() string {

version := Version

if match, _ := regexp.MatchString(`^[^v]`, version); match {

version = "v" + version

}

return version

}

mwbrooks added 6 commits March 20, 2026 22:27

Merge branch 'main' into mwbrooks-cleanup-cli-version

24e129b

Merge branch 'main' into mwbrooks-cleanup-cli-version

1483592

refactor: remove vestigial Config.Version field

0147ef4

Consumers now call version.Raw() directly instead of reading from Config.Version, which was a redundant copy set in two places.

merge: resolve conflicts from main

7543390

Remove re-added clients.Config.Version assignments that were intentionally deleted in the Config.Version field refactor.

mwbrooks added this to the Next Release milestone Mar 23, 2026

mwbrooks self-assigned this Mar 23, 2026

mwbrooks added code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels Mar 23, 2026

mwbrooks added 2 commits March 23, 2026 15:09

refactor: update slackcontext.Version fallback error message

354894c

refactor: use version.Raw() consistently in main.go

1a0cadd

mwbrooks commented Mar 23, 2026

View reviewed changes

mwbrooks added 3 commits March 24, 2026 09:13

Merge branch 'main' into mwbrooks-cli-version-refactor-1

79f6933

Merge remote-tracking branch 'origin/main' into mwbrooks-cli-version-…

eb0b45e

…refactor-1

Merge remote-tracking branch 'origin/mwbrooks-cli-version-refactor-1'…

aec060b

… into mwbrooks-cli-version-refactor-1

mwbrooks marked this pull request as ready for review March 24, 2026 16:47

mwbrooks requested a review from a team as a code owner March 24, 2026 16:47

srtaalej approved these changes Mar 25, 2026

View reviewed changes

zimeg approved these changes Mar 25, 2026

View reviewed changes

		// TODO(slackcontext) Consolidate storing CLI version to slackcontext
		clients.Config.Version = version.Raw()

	// Get the version and format it (e.g. `v1.0.0`)
	func Get() string {
	version := Version
	if match, _ := regexp.MatchString(`^[^v]`, version); match {
	version = "v" + version
	}
	return version
	}

	versionString, _ := strings.CutPrefix(cliVersion, "v")
	versionString := strings.TrimPrefix(cliVersion, "v")

Conversation

mwbrooks commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Summary

Open Questions

Test Steps

1. Version output

2. Version in debug output

3.. User-Agent in API debug output

4.. Log file version

Requirements

Uh oh!

codecov bot commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mwbrooks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwbrooks Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srtaalej left a comment

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mwbrooks commented Mar 23, 2026 •

edited

Loading

codecov bot commented Mar 23, 2026 •

edited

Loading

mwbrooks Mar 23, 2026 •

edited

Loading