From b2288616885a6641fd16ecffc8f9a609b0d418e8 Mon Sep 17 00:00:00 2001 From: Eden Zimbelman Date: Tue, 24 Feb 2026 22:24:12 -0800 Subject: [PATCH 1/7] fix: avoid hang before command exit of jaeger paths Co-Authored-By: Claude --- internal/config/dotenv.go | 13 ++++++++++--- internal/slackcontext/slackcontext_test.go | 4 ++-- internal/tracer/tracer.go | 4 ++-- main.go | 17 +++++++++++++++-- 4 files changed, 29 insertions(+), 9 deletions(-) diff --git a/internal/config/dotenv.go b/internal/config/dotenv.go index a186e3d7..9afd109c 100644 --- a/internal/config/dotenv.go +++ b/internal/config/dotenv.go @@ -19,9 +19,18 @@ import ( "github.com/joho/godotenv" "github.com/slackapi/slack-cli/internal/pkg/version" + "github.com/slackapi/slack-cli/internal/shared/types" "github.com/spf13/afero" ) +// IsTelemetryDisabled checks environment variables to determine if telemetry +// should be disabled. This can be called before Config is initialized. +func IsTelemetryDisabled(os types.Os) bool { + var disableTelemetry = strings.TrimSpace(os.Getenv(slackDisableTelemetryEnv)) + var testVersion = strings.TrimSpace(os.Getenv(version.EnvTestVersion)) + return (disableTelemetry != "" && disableTelemetry != "false" && disableTelemetry != "0") || testVersion != "" +} + // GetDotEnvFileVariables collects only the variables in the .env file func (c *Config) GetDotEnvFileVariables() (map[string]string, error) { variables := map[string]string{} @@ -60,9 +69,7 @@ func (c *Config) LoadEnvironmentVariables() error { } // Disable telemetry if either disable-telemetry or test-version environment variables - var disableTelemetry = strings.TrimSpace(c.os.Getenv(slackDisableTelemetryEnv)) - var testVersion = strings.TrimSpace(c.os.Getenv(version.EnvTestVersion)) - if (disableTelemetry != "" && disableTelemetry != "false" && disableTelemetry != "0") || testVersion != "" { + if IsTelemetryDisabled(c.os) { c.DisableTelemetryFlag = true } diff --git a/internal/slackcontext/slackcontext_test.go b/internal/slackcontext/slackcontext_test.go index d09b724d..d1e73611 100644 --- a/internal/slackcontext/slackcontext_test.go +++ b/internal/slackcontext/slackcontext_test.go @@ -112,7 +112,7 @@ func Test_SlackContext_SetOpenTracingTraceID(t *testing.T) { } func Test_SlackContext_OpenTracingTracer(t *testing.T) { - _, _tracer := tracer.SetupTracer(true) + _, _tracer := tracer.SetupTracer(true, false) tests := map[string]struct { expectedTracer opentracing.Tracer @@ -139,7 +139,7 @@ func Test_SlackContext_OpenTracingTracer(t *testing.T) { } func Test_SlackContext_SetOpenTracingTracer(t *testing.T) { - _, _tracer := tracer.SetupTracer(true) + _, _tracer := tracer.SetupTracer(true, false) tests := map[string]struct { expectedTracer opentracing.Tracer diff --git a/internal/tracer/tracer.go b/internal/tracer/tracer.go index 93dacc2c..82a7ca19 100644 --- a/internal/tracer/tracer.go +++ b/internal/tracer/tracer.go @@ -24,7 +24,7 @@ import ( ) // SetupTracer sets up the tracer to send data to honeycomb -func SetupTracer(isDev bool) (io.Closer, opentracing.Tracer) { +func SetupTracer(isDev bool, disableTelemetry bool) (io.Closer, opentracing.Tracer) { var collectorEndpoint = "https://slackb.com/traces/v1/jaeger" if isDev { collectorEndpoint = "https://dev.slackb.com/traces/v1/jaeger" @@ -33,7 +33,7 @@ func SetupTracer(isDev bool) (io.Closer, opentracing.Tracer) { // Recommended configuration for production. var jCfg = jaegercfg.Configuration{ ServiceName: "slack-cli", // Don't change this. Required to distinguish logs & traces coming from the CLI - Disabled: false, + Disabled: disableTelemetry, Reporter: &jaegercfg.ReporterConfig{ LogSpans: false, CollectorEndpoint: collectorEndpoint, diff --git a/main.go b/main.go index 601b4b26..a1972255 100644 --- a/main.go +++ b/main.go @@ -19,11 +19,14 @@ import ( "os" "runtime/debug" "strings" + "time" "github.com/google/uuid" "github.com/opentracing/opentracing-go" "github.com/slackapi/slack-cli/cmd" + "github.com/slackapi/slack-cli/internal/config" "github.com/slackapi/slack-cli/internal/goutils" + "github.com/slackapi/slack-cli/internal/slackdeps" "github.com/slackapi/slack-cli/internal/iostreams" "github.com/slackapi/slack-cli/internal/ioutils" "github.com/slackapi/slack-cli/internal/pkg/version" @@ -43,8 +46,18 @@ func main() { // - This would allow us to choose the correct API host based on flags // - Uncomment `isDevTarget` if we refactor to cmd/root.go and update to call `ResolveAPIHost` // var isDevTarget = shared.NewClientFactory().AuthClient().UserDefaultAuthIsProd(ctx) // TODO - hack, remove shared.clients - var jaegerCloser, tracer = tracer.SetupTracer(false) // Always setup open tracing on prod - defer jaegerCloser.Close() + var jaegerCloser, tracer = tracer.SetupTracer(false, config.IsTelemetryDisabled(slackdeps.NewOs())) + defer func() { + done := make(chan struct{}) + go func() { + jaegerCloser.Close() + close(done) + }() + select { + case <-done: + case <-time.After(2 * time.Second): + } + }() ctx = slackcontext.SetOpenTracingTracer(ctx, tracer) // Set context values From 3e1cdf3bf3a45390d2419a6343b16f976371719d Mon Sep 17 00:00:00 2001 From: Eden Zimbelman Date: Tue, 24 Feb 2026 22:28:01 -0800 Subject: [PATCH 2/7] chore: lint --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index a1972255..f930b175 100644 --- a/main.go +++ b/main.go @@ -26,12 +26,12 @@ import ( "github.com/slackapi/slack-cli/cmd" "github.com/slackapi/slack-cli/internal/config" "github.com/slackapi/slack-cli/internal/goutils" - "github.com/slackapi/slack-cli/internal/slackdeps" "github.com/slackapi/slack-cli/internal/iostreams" "github.com/slackapi/slack-cli/internal/ioutils" "github.com/slackapi/slack-cli/internal/pkg/version" "github.com/slackapi/slack-cli/internal/shared" "github.com/slackapi/slack-cli/internal/slackcontext" + "github.com/slackapi/slack-cli/internal/slackdeps" "github.com/slackapi/slack-cli/internal/tracer" "github.com/uber/jaeger-client-go" ) From 39faaec6caf9b98ac950391f2afabf6cdfdf8877 Mon Sep 17 00:00:00 2001 From: Eden Zimbelman Date: Tue, 24 Feb 2026 22:49:32 -0800 Subject: [PATCH 3/7] fix: replace the collection endpoint while keeping trace ids --- internal/tracer/tracer.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/tracer/tracer.go b/internal/tracer/tracer.go index 82a7ca19..5025d2c4 100644 --- a/internal/tracer/tracer.go +++ b/internal/tracer/tracer.go @@ -29,11 +29,14 @@ func SetupTracer(isDev bool, disableTelemetry bool) (io.Closer, opentracing.Trac if isDev { collectorEndpoint = "https://dev.slackb.com/traces/v1/jaeger" } + if disableTelemetry { + collectorEndpoint = "" + } // Recommended configuration for production. var jCfg = jaegercfg.Configuration{ ServiceName: "slack-cli", // Don't change this. Required to distinguish logs & traces coming from the CLI - Disabled: disableTelemetry, + Disabled: false, // Keep tracer active so span contexts and trace IDs are still generated Reporter: &jaegercfg.ReporterConfig{ LogSpans: false, CollectorEndpoint: collectorEndpoint, From a9df972a761ff92af499a1a0852c022c4c656cba Mon Sep 17 00:00:00 2001 From: Eden Zimbelman Date: Wed, 25 Feb 2026 00:15:46 -0800 Subject: [PATCH 4/7] fix: avoid blank endpoints --- internal/tracer/tracer.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/internal/tracer/tracer.go b/internal/tracer/tracer.go index 5025d2c4..fa744660 100644 --- a/internal/tracer/tracer.go +++ b/internal/tracer/tracer.go @@ -29,10 +29,6 @@ func SetupTracer(isDev bool, disableTelemetry bool) (io.Closer, opentracing.Trac if isDev { collectorEndpoint = "https://dev.slackb.com/traces/v1/jaeger" } - if disableTelemetry { - collectorEndpoint = "" - } - // Recommended configuration for production. var jCfg = jaegercfg.Configuration{ ServiceName: "slack-cli", // Don't change this. Required to distinguish logs & traces coming from the CLI From 6658b488fc51bbb1ecd02fdd0a6b645e3d6101f7 Mon Sep 17 00:00:00 2001 From: Eden Zimbelman Date: Wed, 25 Feb 2026 00:58:45 -0800 Subject: [PATCH 5/7] fix: add timeout to jaeger report --- internal/config/dotenv.go | 13 +++-------- internal/slackcontext/slackcontext_test.go | 4 ++-- internal/tracer/tracer.go | 27 ++++++++++++---------- main.go | 4 +--- 4 files changed, 21 insertions(+), 27 deletions(-) diff --git a/internal/config/dotenv.go b/internal/config/dotenv.go index 9afd109c..a186e3d7 100644 --- a/internal/config/dotenv.go +++ b/internal/config/dotenv.go @@ -19,18 +19,9 @@ import ( "github.com/joho/godotenv" "github.com/slackapi/slack-cli/internal/pkg/version" - "github.com/slackapi/slack-cli/internal/shared/types" "github.com/spf13/afero" ) -// IsTelemetryDisabled checks environment variables to determine if telemetry -// should be disabled. This can be called before Config is initialized. -func IsTelemetryDisabled(os types.Os) bool { - var disableTelemetry = strings.TrimSpace(os.Getenv(slackDisableTelemetryEnv)) - var testVersion = strings.TrimSpace(os.Getenv(version.EnvTestVersion)) - return (disableTelemetry != "" && disableTelemetry != "false" && disableTelemetry != "0") || testVersion != "" -} - // GetDotEnvFileVariables collects only the variables in the .env file func (c *Config) GetDotEnvFileVariables() (map[string]string, error) { variables := map[string]string{} @@ -69,7 +60,9 @@ func (c *Config) LoadEnvironmentVariables() error { } // Disable telemetry if either disable-telemetry or test-version environment variables - if IsTelemetryDisabled(c.os) { + var disableTelemetry = strings.TrimSpace(c.os.Getenv(slackDisableTelemetryEnv)) + var testVersion = strings.TrimSpace(c.os.Getenv(version.EnvTestVersion)) + if (disableTelemetry != "" && disableTelemetry != "false" && disableTelemetry != "0") || testVersion != "" { c.DisableTelemetryFlag = true } diff --git a/internal/slackcontext/slackcontext_test.go b/internal/slackcontext/slackcontext_test.go index d1e73611..d09b724d 100644 --- a/internal/slackcontext/slackcontext_test.go +++ b/internal/slackcontext/slackcontext_test.go @@ -112,7 +112,7 @@ func Test_SlackContext_SetOpenTracingTraceID(t *testing.T) { } func Test_SlackContext_OpenTracingTracer(t *testing.T) { - _, _tracer := tracer.SetupTracer(true, false) + _, _tracer := tracer.SetupTracer(true) tests := map[string]struct { expectedTracer opentracing.Tracer @@ -139,7 +139,7 @@ func Test_SlackContext_OpenTracingTracer(t *testing.T) { } func Test_SlackContext_SetOpenTracingTracer(t *testing.T) { - _, _tracer := tracer.SetupTracer(true, false) + _, _tracer := tracer.SetupTracer(true) tests := map[string]struct { expectedTracer opentracing.Tracer diff --git a/internal/tracer/tracer.go b/internal/tracer/tracer.go index fa744660..e975f7f2 100644 --- a/internal/tracer/tracer.go +++ b/internal/tracer/tracer.go @@ -20,35 +20,38 @@ import ( "time" "github.com/opentracing/opentracing-go" + jaeger "github.com/uber/jaeger-client-go" jaegercfg "github.com/uber/jaeger-client-go/config" + "github.com/uber/jaeger-client-go/transport" ) // SetupTracer sets up the tracer to send data to honeycomb -func SetupTracer(isDev bool, disableTelemetry bool) (io.Closer, opentracing.Tracer) { - var collectorEndpoint = "https://slackb.com/traces/v1/jaeger" +func SetupTracer(isDev bool) (io.Closer, opentracing.Tracer) { + collectorEndpoint := "https://slackb.com/traces/v1/jaeger" if isDev { collectorEndpoint = "https://dev.slackb.com/traces/v1/jaeger" } // Recommended configuration for production. - var jCfg = jaegercfg.Configuration{ + jCfg := jaegercfg.Configuration{ ServiceName: "slack-cli", // Don't change this. Required to distinguish logs & traces coming from the CLI Disabled: false, // Keep tracer active so span contexts and trace IDs are still generated - Reporter: &jaegercfg.ReporterConfig{ - LogSpans: false, - CollectorEndpoint: collectorEndpoint, - BufferFlushInterval: 100 * time.Millisecond, - // Having a larger value here results in longer execution of every single CLI command - // We may need to adjust the size here if we observe lost data in metrics. - QueueSize: 1, - }, Sampler: &jaegercfg.SamplerConfig{ Type: "const", Param: 1, }, } + // Create HTTP transport with a short timeout to prevent hangs on unreachable hosts + sender := transport.NewHTTPTransport(collectorEndpoint, transport.HTTPTimeout(1*time.Second)) + reporter := jaeger.NewRemoteReporter(sender, + jaeger.ReporterOptions.BufferFlushInterval(100*time.Millisecond), + // Having a larger value here results in longer execution of every single CLI command + // We may need to adjust the size here if we observe lost data in metrics. + jaeger.ReporterOptions.QueueSize(1), + ) + // Initialize tracer with a logger and a metrics factory - var tracer, jaegerCloser, traceErr = jCfg.NewTracer() + tracer, jaegerCloser, traceErr := jCfg.NewTracer(jaegercfg.Reporter(reporter)) if traceErr != nil { log.Fatalf("Could not initialize jaeger tracer: %s", traceErr.Error()) } diff --git a/main.go b/main.go index f930b175..fe37b2d2 100644 --- a/main.go +++ b/main.go @@ -24,14 +24,12 @@ import ( "github.com/google/uuid" "github.com/opentracing/opentracing-go" "github.com/slackapi/slack-cli/cmd" - "github.com/slackapi/slack-cli/internal/config" "github.com/slackapi/slack-cli/internal/goutils" "github.com/slackapi/slack-cli/internal/iostreams" "github.com/slackapi/slack-cli/internal/ioutils" "github.com/slackapi/slack-cli/internal/pkg/version" "github.com/slackapi/slack-cli/internal/shared" "github.com/slackapi/slack-cli/internal/slackcontext" - "github.com/slackapi/slack-cli/internal/slackdeps" "github.com/slackapi/slack-cli/internal/tracer" "github.com/uber/jaeger-client-go" ) @@ -46,7 +44,7 @@ func main() { // - This would allow us to choose the correct API host based on flags // - Uncomment `isDevTarget` if we refactor to cmd/root.go and update to call `ResolveAPIHost` // var isDevTarget = shared.NewClientFactory().AuthClient().UserDefaultAuthIsProd(ctx) // TODO - hack, remove shared.clients - var jaegerCloser, tracer = tracer.SetupTracer(false, config.IsTelemetryDisabled(slackdeps.NewOs())) + var jaegerCloser, tracer = tracer.SetupTracer(false) defer func() { done := make(chan struct{}) go func() { From 7d08c08a52c2ddaa9ea4ae140446300d95f1c47c Mon Sep 17 00:00:00 2001 From: Eden Zimbelman Date: Wed, 25 Feb 2026 02:13:08 -0800 Subject: [PATCH 6/7] fix: guard against infinite traversials --- internal/shared/clients.go | 7 ++++++- internal/shared/clients_test.go | 6 ++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/internal/shared/clients.go b/internal/shared/clients.go index ee51b049..f9ccd86b 100644 --- a/internal/shared/clients.go +++ b/internal/shared/clients.go @@ -222,7 +222,12 @@ func (c *ClientFactory) InitSDKConfig(ctx context.Context, dirPath string) error return slackerror.New(slackerror.ErrHooksJSONLocation) } // Move upward one directory level - dirPath = filepath.Dir(dirPath) + parentDir := filepath.Dir(dirPath) + if parentDir == dirPath { + // Reached a filesystem root not covered above (e.g. D:\ when SYSTEMROOT is on C:\) + return slackerror.New(slackerror.ErrHooksJSONLocation) + } + dirPath = parentDir } configFileBytes, err := afero.ReadFile(c.Fs, hooksJSONFilePath) if err != nil { diff --git a/internal/shared/clients_test.go b/internal/shared/clients_test.go index 77897b1b..c7360213 100644 --- a/internal/shared/clients_test.go +++ b/internal/shared/clients_test.go @@ -110,6 +110,12 @@ func Test_ClientFactory_InitSDKConfig(t *testing.T) { mockWorkingDirectory: filepath.Join("path", "outside", "home", "to", "project"), expectedError: slackerror.New(slackerror.ErrHooksJSONLocation), }, + "errors if traversal reaches a filesystem root": { + mockHooksJSONContent: "{}", + mockHooksJSONFilePath: filepath.Join(string(filepath.Separator), "other", "volume", "project", "package.json"), + mockWorkingDirectory: filepath.Join(string(filepath.Separator), "other", "volume", "project"), + expectedError: slackerror.New(slackerror.ErrHooksJSONLocation), + }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { From c78228f13181619f254dda47404dc4db1fbdf0c7 Mon Sep 17 00:00:00 2001 From: Eden Zimbelman Date: Wed, 25 Feb 2026 03:03:56 -0800 Subject: [PATCH 7/7] revert: jaeger --- internal/tracer/tracer.go | 28 +++++++++++++--------------- main.go | 15 ++------------- 2 files changed, 15 insertions(+), 28 deletions(-) diff --git a/internal/tracer/tracer.go b/internal/tracer/tracer.go index e975f7f2..93dacc2c 100644 --- a/internal/tracer/tracer.go +++ b/internal/tracer/tracer.go @@ -20,38 +20,36 @@ import ( "time" "github.com/opentracing/opentracing-go" - jaeger "github.com/uber/jaeger-client-go" jaegercfg "github.com/uber/jaeger-client-go/config" - "github.com/uber/jaeger-client-go/transport" ) // SetupTracer sets up the tracer to send data to honeycomb func SetupTracer(isDev bool) (io.Closer, opentracing.Tracer) { - collectorEndpoint := "https://slackb.com/traces/v1/jaeger" + var collectorEndpoint = "https://slackb.com/traces/v1/jaeger" if isDev { collectorEndpoint = "https://dev.slackb.com/traces/v1/jaeger" } + // Recommended configuration for production. - jCfg := jaegercfg.Configuration{ + var jCfg = jaegercfg.Configuration{ ServiceName: "slack-cli", // Don't change this. Required to distinguish logs & traces coming from the CLI - Disabled: false, // Keep tracer active so span contexts and trace IDs are still generated + Disabled: false, + Reporter: &jaegercfg.ReporterConfig{ + LogSpans: false, + CollectorEndpoint: collectorEndpoint, + BufferFlushInterval: 100 * time.Millisecond, + // Having a larger value here results in longer execution of every single CLI command + // We may need to adjust the size here if we observe lost data in metrics. + QueueSize: 1, + }, Sampler: &jaegercfg.SamplerConfig{ Type: "const", Param: 1, }, } - // Create HTTP transport with a short timeout to prevent hangs on unreachable hosts - sender := transport.NewHTTPTransport(collectorEndpoint, transport.HTTPTimeout(1*time.Second)) - reporter := jaeger.NewRemoteReporter(sender, - jaeger.ReporterOptions.BufferFlushInterval(100*time.Millisecond), - // Having a larger value here results in longer execution of every single CLI command - // We may need to adjust the size here if we observe lost data in metrics. - jaeger.ReporterOptions.QueueSize(1), - ) - // Initialize tracer with a logger and a metrics factory - tracer, jaegerCloser, traceErr := jCfg.NewTracer(jaegercfg.Reporter(reporter)) + var tracer, jaegerCloser, traceErr = jCfg.NewTracer() if traceErr != nil { log.Fatalf("Could not initialize jaeger tracer: %s", traceErr.Error()) } diff --git a/main.go b/main.go index fe37b2d2..601b4b26 100644 --- a/main.go +++ b/main.go @@ -19,7 +19,6 @@ import ( "os" "runtime/debug" "strings" - "time" "github.com/google/uuid" "github.com/opentracing/opentracing-go" @@ -44,18 +43,8 @@ func main() { // - This would allow us to choose the correct API host based on flags // - Uncomment `isDevTarget` if we refactor to cmd/root.go and update to call `ResolveAPIHost` // var isDevTarget = shared.NewClientFactory().AuthClient().UserDefaultAuthIsProd(ctx) // TODO - hack, remove shared.clients - var jaegerCloser, tracer = tracer.SetupTracer(false) - defer func() { - done := make(chan struct{}) - go func() { - jaegerCloser.Close() - close(done) - }() - select { - case <-done: - case <-time.After(2 * time.Second): - } - }() + var jaegerCloser, tracer = tracer.SetupTracer(false) // Always setup open tracing on prod + defer jaegerCloser.Close() ctx = slackcontext.SetOpenTracingTracer(ctx, tracer) // Set context values