From 1dad2112a9a41d945d7183ac3628958e142ba558 Mon Sep 17 00:00:00 2001 From: Ross Nelson Date: Wed, 8 Jul 2026 11:55:19 -0400 Subject: [PATCH 1/5] Add staged connection diagnosis for dial failures Connection failures now render a classified error with an inline DNS/TCP/TLS diagnosis and one suggested fix, instead of a bare 'context deadline exceeded' or an empty message. Fixes #224 Fixes #851 --- cliext/client.go | 30 +++ internal/temporalcli/client.go | 63 ++++- internal/temporalcli/client_test.go | 205 ++++++++++++++- internal/temporalcli/commands.go | 11 +- internal/temporalcli/connectdiag.go | 268 +++++++++++++++++++ internal/temporalcli/connectdiag_test.go | 311 +++++++++++++++++++++++ internal/temporalcli/connecterror.go | 216 ++++++++++++++++ 7 files changed, 1096 insertions(+), 8 deletions(-) create mode 100644 internal/temporalcli/connectdiag.go create mode 100644 internal/temporalcli/connectdiag_test.go create mode 100644 internal/temporalcli/connecterror.go diff --git a/cliext/client.go b/cliext/client.go index 48eaa7508..e723c1d73 100644 --- a/cliext/client.go +++ b/cliext/client.go @@ -2,7 +2,9 @@ package cliext import ( "context" + "errors" "fmt" + "io/fs" "log/slog" "net/http" "strings" @@ -34,6 +36,19 @@ type ClientOptionsBuilder struct { // configured. Callers can use it to decode payloads outside the gRPC // interceptor chain (e.g. payloads nested inside opaque proto bytes). PayloadCodec converter.PayloadCodec + + // ResolvedAddress is populated by Build with the final server address, + // after profile resolution and namespace templating. + ResolvedAddress string + // ResolvedAddressSource is populated by Build with where the final address + // came from: "flag", "profile", or "default". + ResolvedAddressSource string + // ResolvedProfileName is populated by Build with the config profile name + // used, if any. + ResolvedProfileName string + // HasAPIKey is populated by Build and reports whether API-key credentials + // are configured (via flag, profile, or OAuth). + HasAPIKey bool } type oauthCredentials struct { @@ -108,8 +123,12 @@ func (b *ClientOptionsBuilder) Build(ctx context.Context) (client.Options, error // override the profile version unless it was _explicitly_ set. if cfg.FlagSet != nil && cfg.FlagSet.Changed("address") { profile.Address = cfg.Address + b.ResolvedAddressSource = "flag" } else if profile.Address == "" { profile.Address = cfg.Address + b.ResolvedAddressSource = "default" + } else { + b.ResolvedAddressSource = "profile" } var namespaceExplicitlySet bool if cfg.FlagSet != nil && cfg.FlagSet.Changed("namespace") { @@ -126,6 +145,7 @@ func (b *ClientOptionsBuilder) Build(ctx context.Context) (client.Options, error if addressHasNamespaceTemplate { profile.Address = strings.ReplaceAll(profile.Address, addressNamespaceTemplate, profile.Namespace) } + b.ResolvedAddress = profile.Address // Set API key on profile if provided if cfg.ApiKey != "" { @@ -204,9 +224,18 @@ func (b *ClientOptionsBuilder) Build(ctx context.Context) (client.Options, error profile.Codec.Auth = cfg.CodecAuth } + b.ResolvedProfileName = common.Profile + b.HasAPIKey = profile.APIKey != "" + // Convert profile to client options. clientOpts, err := profile.ToClientOptions(envconfig.ToClientOptionsRequest{}) if err != nil { + // File-path errors (e.g. an unreadable TLS cert) are common enough to + // deserve naming the offending file. + var pathErr *fs.PathError + if errors.As(err, &pathErr) { + return client.Options{}, fmt.Errorf("failed to build client options: cannot read file %q: %w", pathErr.Path, err) + } return client.Options{}, fmt.Errorf("failed to build client options: %w", err) } @@ -249,6 +278,7 @@ func (b *ClientOptionsBuilder) Build(ctx context.Context) (client.Options, error profileName: result.ProfileName, } clientOpts.Credentials = client.NewAPIKeyDynamicCredentials(creds.getToken) + b.HasAPIKey = true } } diff --git a/internal/temporalcli/client.go b/internal/temporalcli/client.go index 230ff5a35..81dfe97c7 100644 --- a/internal/temporalcli/client.go +++ b/internal/temporalcli/client.go @@ -2,7 +2,9 @@ package temporalcli import ( "context" + "errors" "fmt" + "io/fs" "os" "os/user" @@ -56,6 +58,17 @@ func dialClientWithCodec(cctx *CommandContext, c *cliext.ClientOptions) (client. } clientOpts, err := builder.Build(cctx) if err != nil { + // An unreadable TLS/credential file is a connection-setup problem + // worth a suggestion, even though no dial happened. + var pathErr *fs.PathError + if errors.As(err, &pathErr) { + diag := &connectDiagnosis{ + Address: builder.ResolvedAddress, + Cause: causeCertFileUnreadable, + Detail: pathErr.Path, + } + return nil, nil, newConnectError(diag, connectMetaFromBuilder(cctx, builder), err) + } return nil, nil, err } @@ -87,7 +100,7 @@ func dialClientWithCodec(cctx *CommandContext, c *cliext.ClientOptions) (client. cl, err := client.DialContext(dialCtx, clientOpts) if err != nil { - return nil, nil, err + return nil, nil, dialConnectError(cctx, dialCtx, builder, clientOpts, err) } // Since this namespace value is used by many commands after this call, @@ -97,6 +110,54 @@ func dialClientWithCodec(cctx *CommandContext, c *cliext.ClientOptions) (client. return cl, builder.PayloadCodec, nil } +// dialConnectError enriches a client.DialContext failure with a staged +// connection diagnosis and a suggested fix. See connectdiag.go. +func dialConnectError( + cctx *CommandContext, + dialCtx context.Context, + builder *cliext.ClientOptionsBuilder, + clientOpts client.Options, + origErr error, +) error { + // If a CLI-side timeout fired, surface its descriptive cause, which is + // otherwise lost ("context deadline exceeded" wins over "command timed + // out after 5s"). + if dialCtx.Err() != nil { + if cause := context.Cause(dialCtx); cause != nil && !errors.Is(cause, dialCtx.Err()) { + origErr = fmt.Errorf("%w (%v)", origErr, cause) + } + } + // Never probe after an interrupt or when the whole command timed out. + if cctx.Err() != nil { + return origErr + } + if v, _ := cctx.Options.EnvLookup.LookupEnv("TEMPORAL_CLI_DISABLE_CONNECT_DIAGNOSIS"); v != "" { + return fmt.Errorf("failed connecting to Temporal server at %v: %w", clientOpts.HostPort, origErr) + } + // The probe has its own internal budget, but never let it exceed the + // user's explicit connect timeout. + probeCtx := context.Context(cctx) + if timeout := cctx.RootCommand.CommonOptions.ClientConnectTimeout.Duration(); timeout > 0 { + var cancel context.CancelFunc + probeCtx, cancel = context.WithTimeout(cctx, timeout) + defer cancel() + } + diag := diagnoseConnection(probeCtx, clientOpts.HostPort, clientOpts.ConnectionOptions.TLS, origErr) + meta := connectMetaFromBuilder(cctx, builder) + meta.TLSConfigured = clientOpts.ConnectionOptions.TLS != nil + return newConnectError(diag, meta, origErr) +} + +func connectMetaFromBuilder(cctx *CommandContext, builder *cliext.ClientOptionsBuilder) connectMeta { + return connectMeta{ + Args: cctx.Options.Args, + Address: builder.ResolvedAddress, + AddressSource: builder.ResolvedAddressSource, + ProfileName: builder.ResolvedProfileName, + HasAPIKey: builder.HasAPIKey, + } +} + func fixedHeaderOverrideInterceptor( ctx context.Context, method string, req, reply any, diff --git a/internal/temporalcli/client_test.go b/internal/temporalcli/client_test.go index 2aa7ae391..6593e57b5 100644 --- a/internal/temporalcli/client_test.go +++ b/internal/temporalcli/client_test.go @@ -1,6 +1,15 @@ package temporalcli_test import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" "net" "testing" "time" @@ -9,11 +18,13 @@ import ( "github.com/stretchr/testify/require" ) -func TestClientConnectTimeout(t *testing.T) { - // Start a listener that accepts connections but never responds +// startBlackHoleListener starts a listener that accepts connections but never +// responds. +func startBlackHoleListener(t *testing.T) net.Listener { + t.Helper() ln, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) - defer ln.Close() + t.Cleanup(func() { ln.Close() }) go func() { for { conn, err := ln.Accept() @@ -24,6 +35,62 @@ func TestClientConnectTimeout(t *testing.T) { select {} } }() + return ln +} + +// startMTLSListener starts a TLS listener that requires client certificates, +// returning its address and the server CA cert as PEM. +func startMTLSListener(t *testing.T) (addr, caPEM string) { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + template := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "client-test"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + IsCA: true, + } + der, err := x509.CreateCertificate(rand.Reader, &template, &template, &key.PublicKey, key) + require.NoError(t, err) + leaf, err := x509.ParseCertificate(der) + require.NoError(t, err) + pool := x509.NewCertPool() + pool.AddCert(leaf) + + ln, err := tls.Listen("tcp", "127.0.0.1:0", &tls.Config{ + Certificates: []tls.Certificate{{Certificate: [][]byte{der}, PrivateKey: key}}, + ClientAuth: tls.RequireAndVerifyClientCert, + ClientCAs: pool, + }) + require.NoError(t, err) + t.Cleanup(func() { ln.Close() }) + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + go func() { + if tlsConn, ok := conn.(*tls.Conn); ok { + _ = tlsConn.HandshakeContext(context.Background()) + buf := make([]byte, 1) + _ = tlsConn.SetReadDeadline(time.Now().Add(2 * time.Second)) + _, _ = tlsConn.Read(buf) + } + conn.Close() + }() + } + }() + return ln.Addr().String(), string(pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der})) +} + +func TestClientConnectTimeout(t *testing.T) { + ln := startBlackHoleListener(t) h := NewCommandHarness(t) @@ -36,6 +103,136 @@ func TestClientConnectTimeout(t *testing.T) { elapsed := time.Since(start) require.Error(t, res.Err) + assert.Contains(t, res.Err.Error(), "failed connecting to Temporal server at") assert.Contains(t, res.Err.Error(), "deadline exceeded") - assert.Less(t, elapsed, time.Second, "dial should have timed out quickly") + // The friendly cause attached to the connect timeout must survive. + assert.Contains(t, res.Err.Error(), "command timed out after 50ms") + assert.Less(t, elapsed, 2*time.Second, "dial and diagnosis should respect the connect timeout") +} + +func TestConnectDiagnosis_MTLSRequired(t *testing.T) { + addr, caPEM := startMTLSListener(t) + + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + "--address", addr, + "--tls", + "--tls-ca-data", caPEM, + "--client-connect-timeout", "2s", + ) + + require.Error(t, res.Err) + msg := res.Err.Error() + assert.Contains(t, msg, "Connecting to "+addr) + assert.Contains(t, msg, "✓ TCP connection established") + assert.Contains(t, msg, "✗ TLS handshake failed: server requires mTLS") + assert.Contains(t, msg, "--tls-cert-path YourCert.pem") + assert.Contains(t, msg, "--tls-key-path YourKey.pem") +} + +func TestConnectDiagnosis_Refused(t *testing.T) { + // Grab a port and close it so nothing is listening. + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := ln.Addr().String() + require.NoError(t, ln.Close()) + + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + "--address", addr, + "--client-connect-timeout", "2s", + ) + + require.Error(t, res.Err) + msg := res.Err.Error() + assert.Contains(t, msg, "connection refused") + assert.Contains(t, msg, "✗ TCP connection refused") + assert.Contains(t, msg, "Nothing is listening at "+addr) +} + +func TestConnectDiagnosis_DNSFailure(t *testing.T) { + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + // .invalid is reserved (RFC 2606) and can never resolve. + "--address", "does-not-exist.invalid:7233", + "--client-connect-timeout", "5s", + ) + + require.Error(t, res.Err) + msg := res.Err.Error() + assert.Contains(t, msg, "could not resolve host") + assert.Contains(t, msg, "✗ DNS lookup") + assert.Contains(t, msg, `Could not resolve "does-not-exist.invalid"`) +} + +func TestConnectDiagnosis_JSONOutputHasNoANSI(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := ln.Addr().String() + require.NoError(t, ln.Close()) + + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + "--address", addr, + "--client-connect-timeout", "2s", + "-o", "json", + ) + + require.Error(t, res.Err) + assert.NotContains(t, res.Err.Error(), "\x1b[", "diagnosis must not contain ANSI escapes in JSON mode") + assert.Empty(t, res.Stdout.String(), "connection failures must not write to stdout") +} + +func TestConnectDiagnosis_Disabled(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := ln.Addr().String() + require.NoError(t, ln.Close()) + + h := NewCommandHarness(t) + h.Options.EnvLookup = EnvLookupMap{"TEMPORAL_CLI_DISABLE_CONNECT_DIAGNOSIS": "1"} + res := h.Execute( + "workflow", "list", + "--address", addr, + "--client-connect-timeout", "2s", + ) + + require.Error(t, res.Err) + msg := res.Err.Error() + assert.Contains(t, msg, "failed connecting to Temporal server at "+addr) + assert.NotContains(t, msg, "✗", "diagnosis must be suppressed when disabled") +} + +func TestConnectDiagnosis_CertFileMissing(t *testing.T) { + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + "--address", "127.0.0.1:7233", + "--tls-cert-path", "/definitely/does/not/exist.pem", + "--tls-key-path", "/definitely/does/not/exist.key", + ) + + require.Error(t, res.Err) + msg := res.Err.Error() + assert.Contains(t, msg, "cannot read file") + assert.Contains(t, msg, "/definitely/does/not/exist") + assert.NotContains(t, msg, "✓", "no probe stages expected when the dial never happened") +} + +func TestConnectDiagnosis_CommandTimeoutCauseSurvives(t *testing.T) { + ln := startBlackHoleListener(t) + + h := NewCommandHarness(t) + res := h.Execute( + "workflow", "list", + "--address", ln.Addr().String(), + "--command-timeout", "1s", + ) + + require.Error(t, res.Err) + assert.Contains(t, res.Err.Error(), "command timed out after 1s") } diff --git a/internal/temporalcli/commands.go b/internal/temporalcli/commands.go index ecb52515d..df5fcdb19 100644 --- a/internal/temporalcli/commands.go +++ b/internal/temporalcli/commands.go @@ -140,10 +140,15 @@ func (c *CommandContext) preprocessOptions() error { // Setup default fail callback if c.Options.Fail == nil { c.Options.Fail = func(err error) { - // If context is closed, say that the program was interrupted and ignore - // the actual error + // If context is closed, say that the program was interrupted and + // ignore the actual error — unless the context carries a + // descriptive cause such as a --command-timeout expiry. if c.Err() != nil { - err = fmt.Errorf("program interrupted") + if cause := context.Cause(c); cause != nil && !errors.Is(cause, context.Canceled) { + err = cause + } else { + err = fmt.Errorf("program interrupted") + } } fmt.Fprintf(c.Options.Stderr, "Error: %v\n", err) os.Exit(1) diff --git a/internal/temporalcli/connectdiag.go b/internal/temporalcli/connectdiag.go new file mode 100644 index 000000000..e6eda32a7 --- /dev/null +++ b/internal/temporalcli/connectdiag.go @@ -0,0 +1,268 @@ +package temporalcli + +import ( + "context" + "crypto/tls" + "crypto/x509" + "errors" + "fmt" + "net" + "strings" + "syscall" + "time" + + "go.temporal.io/api/serviceerror" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// connectCause classifies why a connection to the server failed. It drives +// the suggested fix shown alongside the staged diagnosis. +type connectCause int + +const ( + causeUnknown connectCause = iota + causeDNS + causeTCPRefused + causeTCPTimeout + // causeServerPlaintext means TLS was configured but the server responded + // with a non-TLS handshake. + causeServerPlaintext + // causeServerSpeaksTLS means TLS was not configured but the server + // completed a TLS handshake when offered one. + causeServerSpeaksTLS + causeClientCertRequired + causeCAVerify + causeHostnameMismatch + // causeCertFileUnreadable is set by the caller when building client + // options fails on a file read; the probe never runs for it. + causeCertFileUnreadable + causeAuth + causeTimeout +) + +type diagStatus int + +const ( + diagOK diagStatus = iota + diagFail +) + +type diagStage struct { + Status diagStatus + Label string +} + +// connectDiagnosis is the result of probing a failed connection. +type connectDiagnosis struct { + Address string + Stages []diagStage + Cause connectCause + // Detail carries cause-specific info (e.g. an unreadable file path or the + // raw TLS alert text) for use in stage labels and suggestions. + Detail string +} + +const ( + connectDiagnosisBudget = 3 * time.Second + connectDiagnosisReadProbe = 500 * time.Millisecond +) + +// diagnoseConnection probes address in stages (DNS, TCP, TLS) to pinpoint why +// a dial failed, and classifies origErr for anything past the transport. It +// must only be called on an already-failed dial: it makes fresh network +// connections (including, when TLS is not configured, one anonymous TLS +// handshake to detect a TLS-only server, which may appear in server logs). +func diagnoseConnection(ctx context.Context, address string, tlsCfg *tls.Config, origErr error) *connectDiagnosis { + d := &connectDiagnosis{Address: address, Cause: causeUnknown} + ctx, cancel := context.WithTimeout(ctx, connectDiagnosisBudget) + defer cancel() + + host, _, err := net.SplitHostPort(address) + if err != nil { + // Not a host:port we can probe; fall back to classifying the original + // error only. + d.Cause, d.Detail = classifyGRPCError(origErr) + return d + } + + // Stage: DNS (skipped for IP literals) + if net.ParseIP(host) == nil { + addrs, err := net.DefaultResolver.LookupHost(ctx, host) + if err != nil { + d.fail(fmt.Sprintf("DNS lookup for %q failed: %v", host, dnsErrShort(err))) + d.Cause = causeDNS + return d + } + plural := "es" + if len(addrs) == 1 { + plural = "" + } + d.ok(fmt.Sprintf("DNS resolved (%d address%s)", len(addrs), plural)) + } + + // Stage: TCP + conn, err := (&net.Dialer{}).DialContext(ctx, "tcp", address) + if err != nil { + if errors.Is(err, syscall.ECONNREFUSED) { + d.fail("TCP connection refused: nothing is listening at " + address) + d.Cause = causeTCPRefused + } else if isTimeout(err) { + d.fail("TCP connection timed out") + d.Cause = causeTCPTimeout + } else { + d.fail(fmt.Sprintf("TCP connection failed: %v", err)) + d.Cause = causeTCPTimeout + } + return d + } + defer conn.Close() + d.ok("TCP connection established") + + if tlsCfg != nil { + d.probeTLS(ctx, conn, host, tlsCfg) + if d.Cause != causeUnknown { + return d + } + } else if cause, detail := probeServerSpeaksTLS(ctx, conn, host); cause != causeUnknown { + d.fail("server expects TLS, but the CLI is connecting without it" + detail) + d.Cause = cause + return d + } + + // Stage: gRPC — no re-dial; classify the original error. + d.Cause, d.Detail = classifyGRPCError(origErr) + d.fail("gRPC connection failed: " + shortErr(origErr)) + return d +} + +// probeTLS performs a TLS handshake over conn using the client's own config +// and classifies the failure, if any. On handshake success it briefly reads to +// catch post-handshake rejections: TLS 1.3 servers requiring client +// certificates complete the handshake first and only then send an alert. +func (d *connectDiagnosis) probeTLS(ctx context.Context, conn net.Conn, host string, tlsCfg *tls.Config) { + cfg := tlsCfg.Clone() + if cfg.ServerName == "" && !cfg.InsecureSkipVerify { + cfg.ServerName = host + } + tlsConn := tls.Client(conn, cfg) + if err := tlsConn.HandshakeContext(ctx); err != nil { + d.classifyTLSError(err) + return + } + // Handshake OK; a short read distinguishes an mTLS rejection alert from a + // genuinely healthy connection (where the read just times out). + _ = tlsConn.SetReadDeadline(time.Now().Add(connectDiagnosisReadProbe)) + buf := make([]byte, 1) + _, err := tlsConn.Read(buf) + if err != nil && !isTimeout(err) { + if cause := classifyTLSAlert(err); cause != causeUnknown { + d.classifyTLSError(err) + return + } + } + d.ok("TLS handshake succeeded") +} + +func (d *connectDiagnosis) classifyTLSError(err error) { + var recordErr tls.RecordHeaderError + var certErr *tls.CertificateVerificationError + var unknownAuthErr x509.UnknownAuthorityError + var hostnameErr x509.HostnameError + switch { + case errors.As(err, &recordErr): + d.fail("TLS handshake failed: server did not respond with TLS (it may be a plaintext gRPC endpoint)") + d.Cause = causeServerPlaintext + case errors.As(err, &hostnameErr): + d.fail("TLS handshake failed: server certificate is not valid for this host: " + shortErr(err)) + d.Cause = causeHostnameMismatch + case errors.As(err, &unknownAuthErr), errors.As(err, &certErr): + d.fail("TLS handshake failed: cannot verify server certificate: " + shortErr(err)) + d.Cause = causeCAVerify + case classifyTLSAlert(err) == causeClientCertRequired: + d.fail("TLS handshake failed: server requires mTLS, no valid client certificate was provided") + d.Cause = causeClientCertRequired + default: + d.fail("TLS handshake failed: " + shortErr(err)) + d.Cause = causeUnknown + d.Detail = err.Error() + } +} + +// classifyTLSAlert detects remote TLS alerts that indicate the server wants a +// (different) client certificate. Go does not export alert types for remote +// errors, so this matches the alert descriptions crypto/tls emits: alert 116 +// "certificate required" (TLS 1.3), alert 42 "bad certificate", and alert 40 +// "handshake failure" (how TLS 1.2 servers commonly reject missing client +// certs). Matching is scoped to TLS probe errors only; if these strings drift +// in a future Go release, unit tests pin them and the diagnosis degrades to +// showing the raw error without a suggestion. +func classifyTLSAlert(err error) connectCause { + msg := err.Error() + if strings.Contains(msg, "certificate required") || + strings.Contains(msg, "bad certificate") || + strings.Contains(msg, "handshake failure") { + return causeClientCertRequired + } + return causeUnknown +} + +// probeServerSpeaksTLS opportunistically offers a TLS handshake to a server +// the CLI is configured to reach in plaintext. If the server negotiates TLS +// (or rejects us at the certificate step, which still means it spoke TLS), the +// mismatch is the likely root cause. +func probeServerSpeaksTLS(ctx context.Context, conn net.Conn, host string) (connectCause, string) { + tlsConn := tls.Client(conn, &tls.Config{InsecureSkipVerify: true, ServerName: host}) + err := tlsConn.HandshakeContext(ctx) + if err == nil { + return causeServerSpeaksTLS, "" + } + if classifyTLSAlert(err) == causeClientCertRequired { + return causeServerSpeaksTLS, " (and it requires client certificates)" + } + return causeUnknown, "" +} + +func classifyGRPCError(err error) (connectCause, string) { + var deadlineErr *serviceerror.DeadlineExceeded + if errors.As(err, &deadlineErr) || errors.Is(err, context.DeadlineExceeded) { + return causeTimeout, "" + } + if unwrapped := errors.Unwrap(err); unwrapped != nil { + if st, ok := status.FromError(unwrapped); ok { + switch st.Code() { + case codes.Unauthenticated, codes.PermissionDenied: + return causeAuth, st.Message() + case codes.DeadlineExceeded: + return causeTimeout, "" + } + } + } + return causeUnknown, "" +} + +func (d *connectDiagnosis) ok(label string) { d.Stages = append(d.Stages, diagStage{diagOK, label}) } +func (d *connectDiagnosis) fail(label string) { + d.Stages = append(d.Stages, diagStage{diagFail, label}) +} + +func isTimeout(err error) bool { + var netErr net.Error + return errors.Is(err, context.DeadlineExceeded) || + (errors.As(err, &netErr) && netErr.Timeout()) +} + +// shortErr renders an error compactly for a stage line, trimming the SDK's +// "failed reaching server:" prefix that would be redundant inside a +// connection diagnosis. +func shortErr(err error) string { + return strings.TrimPrefix(err.Error(), "failed reaching server: ") +} + +func dnsErrShort(err error) string { + var dnsErr *net.DNSError + if errors.As(err, &dnsErr) && dnsErr.IsNotFound { + return "no such host" + } + return err.Error() +} diff --git a/internal/temporalcli/connectdiag_test.go b/internal/temporalcli/connectdiag_test.go new file mode 100644 index 000000000..5b10e8f1d --- /dev/null +++ b/internal/temporalcli/connectdiag_test.go @@ -0,0 +1,311 @@ +package temporalcli + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "fmt" + "math/big" + "net" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.temporal.io/api/serviceerror" +) + +// newTestCert generates a self-signed server certificate for 127.0.0.1. +func newTestCert(t *testing.T) tls.Certificate { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + template := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "connectdiag-test"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + DNSNames: []string{"localhost"}, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + IsCA: true, + } + der, err := x509.CreateCertificate(rand.Reader, &template, &template, &key.PublicKey, key) + require.NoError(t, err) + return tls.Certificate{Certificate: [][]byte{der}, PrivateKey: key, Leaf: mustParseCert(t, der)} +} + +func mustParseCert(t *testing.T, der []byte) *x509.Certificate { + t.Helper() + cert, err := x509.ParseCertificate(der) + require.NoError(t, err) + return cert +} + +func certPool(t *testing.T, cert tls.Certificate) *x509.CertPool { + t.Helper() + pool := x509.NewCertPool() + pool.AddCert(cert.Leaf) + return pool +} + +// serveTLS accepts connections and completes TLS handshakes until the +// listener closes. Returned address is host:port. +func serveTLS(t *testing.T, cfg *tls.Config) string { + t.Helper() + ln, err := tls.Listen("tcp", "127.0.0.1:0", cfg) + require.NoError(t, err) + t.Cleanup(func() { ln.Close() }) + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + go func() { + // Drive the handshake; keep the connection open briefly so + // post-handshake alerts (TLS 1.3 client-cert enforcement) + // reach the client, then close. + if tlsConn, ok := conn.(*tls.Conn); ok { + _ = tlsConn.HandshakeContext(context.Background()) + buf := make([]byte, 1) + _ = tlsConn.SetReadDeadline(time.Now().Add(2 * time.Second)) + _, _ = tlsConn.Read(buf) + } + conn.Close() + }() + } + }() + return ln.Addr().String() +} + +func testDiagnose(t *testing.T, address string, tlsCfg *tls.Config) *connectDiagnosis { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + // Mirror the SDK's eager GetSystemInfo failure shape. + origErr := fmt.Errorf("failed reaching server: %w", serviceerror.NewDeadlineExceeded("context deadline exceeded")) + return diagnoseConnection(ctx, address, tlsCfg, origErr) +} + +func TestDiagnoseConnection_ClientCertRequired(t *testing.T) { + cert := newTestCert(t) + addr := serveTLS(t, &tls.Config{ + Certificates: []tls.Certificate{cert}, + ClientAuth: tls.RequireAndVerifyClientCert, + ClientCAs: certPool(t, cert), + }) + + // Client trusts the server CA but has no client cert. + d := testDiagnose(t, addr, &tls.Config{RootCAs: certPool(t, cert)}) + // This test also pins the Go crypto/tls alert text ("certificate + // required" / "bad certificate" / "handshake failure") that + // classifyTLSAlert depends on; if it fails after a Go upgrade, revisit + // classifyTLSAlert. + assert.Equal(t, causeClientCertRequired, d.Cause) + requireStage(t, d, diagOK, "TCP connection established") + requireStage(t, d, diagFail, "server requires mTLS") +} + +func TestDiagnoseConnection_ServerPlaintext(t *testing.T) { + // Plain TCP listener that immediately writes a non-TLS response. + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { ln.Close() }) + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + _, _ = conn.Write([]byte("HTTP/1.1 400 Bad Request\r\n\r\n")) + conn.Close() + } + }() + + d := testDiagnose(t, ln.Addr().String(), &tls.Config{InsecureSkipVerify: true}) + assert.Equal(t, causeServerPlaintext, d.Cause) + requireStage(t, d, diagFail, "did not respond with TLS") +} + +func TestDiagnoseConnection_ServerSpeaksTLS(t *testing.T) { + cert := newTestCert(t) + addr := serveTLS(t, &tls.Config{Certificates: []tls.Certificate{cert}}) + + // No TLS configured on the client. + d := testDiagnose(t, addr, nil) + assert.Equal(t, causeServerSpeaksTLS, d.Cause) + requireStage(t, d, diagFail, "server expects TLS") +} + +func TestDiagnoseConnection_Refused(t *testing.T) { + // Grab a port and close it so nothing is listening. + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := ln.Addr().String() + require.NoError(t, ln.Close()) + + d := testDiagnose(t, addr, nil) + assert.Equal(t, causeTCPRefused, d.Cause) + requireStage(t, d, diagFail, "TCP connection refused") +} + +func TestDiagnoseConnection_DNSFailure(t *testing.T) { + // .invalid is reserved (RFC 2606) and can never resolve. + d := testDiagnose(t, "does-not-exist.invalid:7233", nil) + assert.Equal(t, causeDNS, d.Cause) + requireStage(t, d, diagFail, "DNS lookup") +} + +func TestDiagnoseConnection_UnknownCA(t *testing.T) { + cert := newTestCert(t) + addr := serveTLS(t, &tls.Config{Certificates: []tls.Certificate{cert}}) + + // Client does not trust the server's self-signed cert. + d := testDiagnose(t, addr, &tls.Config{}) + assert.Equal(t, causeCAVerify, d.Cause) + requireStage(t, d, diagFail, "cannot verify server certificate") +} + +func TestDiagnoseConnection_HealthyTLSFallsThroughToGRPC(t *testing.T) { + cert := newTestCert(t) + addr := serveTLS(t, &tls.Config{Certificates: []tls.Certificate{cert}}) + + // TLS is fine; the original (gRPC-level) error should be classified. + d := testDiagnose(t, addr, &tls.Config{RootCAs: certPool(t, cert)}) + assert.Equal(t, causeTimeout, d.Cause) + requireStage(t, d, diagOK, "TLS handshake succeeded") + requireStage(t, d, diagFail, "gRPC connection failed") +} + +func requireStage(t *testing.T, d *connectDiagnosis, status diagStatus, labelSubstr string) { + t.Helper() + for _, stage := range d.Stages { + if stage.Status == status && strings.Contains(stage.Label, labelSubstr) { + return + } + } + t.Fatalf("no stage with status %v containing %q in %+v", status, labelSubstr, d.Stages) +} + +func TestSuggestFix(t *testing.T) { + cloudMeta := connectMeta{ + Args: []string{"workflow", "list", "--address", "foo.bar.tmprl.cloud:7233"}, + Address: "foo.bar.tmprl.cloud:7233", + } + tests := []struct { + name string + diag *connectDiagnosis + meta connectMeta + contains []string + }{ + { + name: "mTLS on cloud endpoint", + diag: &connectDiagnosis{Cause: causeClientCertRequired}, + meta: cloudMeta, + contains: []string{"Temporal Cloud", "--tls-cert-path YourCert.pem", "--tls-key-path YourKey.pem", "temporal config set --prop tls.client_cert_path"}, + }, + { + name: "mTLS on generic endpoint", + diag: &connectDiagnosis{Cause: causeClientCertRequired}, + meta: connectMeta{Args: []string{"workflow", "list"}, Address: "myhost:7233"}, + contains: []string{"requires client certificates", "--tls-cert-path YourCert.pem"}, + }, + { + name: "refused on local default port", + diag: &connectDiagnosis{Cause: causeTCPRefused}, + meta: connectMeta{Address: "127.0.0.1:7233"}, + contains: []string{"temporal server start-dev"}, + }, + { + name: "refused elsewhere", + diag: &connectDiagnosis{Cause: causeTCPRefused}, + meta: connectMeta{Address: "myhost:9999"}, + contains: []string{"Nothing is listening at myhost:9999"}, + }, + { + name: "dns failure from profile address", + diag: &connectDiagnosis{Cause: causeDNS}, + meta: connectMeta{Address: "typo.example.com:7233", AddressSource: "profile", ProfileName: "prod"}, + contains: []string{`Could not resolve "typo.example.com"`, `profile "prod"`, "temporal config get --prop address"}, + }, + { + name: "server speaks TLS", + diag: &connectDiagnosis{Cause: causeServerSpeaksTLS}, + meta: connectMeta{Args: []string{"workflow", "list"}, Address: "myhost:7233"}, + contains: []string{"Add --tls"}, + }, + { + name: "server plaintext", + diag: &connectDiagnosis{Cause: causeServerPlaintext}, + meta: connectMeta{Address: "myhost:7233", TLSConfigured: true}, + contains: []string{"does not appear to use TLS"}, + }, + { + name: "api key rejected", + diag: &connectDiagnosis{Cause: causeAuth}, + meta: connectMeta{Address: "us-west-2.aws.api.temporal.io:7233", HasAPIKey: true}, + contains: []string{"rejected the provided API key"}, + }, + { + name: "cert file unreadable", + diag: &connectDiagnosis{Cause: causeCertFileUnreadable, Detail: "/nope.pem"}, + meta: connectMeta{Address: "myhost:7233"}, + contains: []string{`Cannot read "/nope.pem"`}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := suggestFix(tt.diag, tt.meta) + for _, want := range tt.contains { + assert.Contains(t, got, want) + } + }) + } +} + +func TestReconstructCommand(t *testing.T) { + got := reconstructCommand( + []string{"workflow", "list", "--address", "foo:7233", "--query", "WorkflowType = 'x'"}, + "--tls-cert-path YourCert.pem", + ) + assert.Equal(t, + "temporal workflow list \\\n"+ + " --address foo:7233 \\\n"+ + " --query \"WorkflowType = 'x'\" \\\n"+ + " --tls-cert-path YourCert.pem", + got) +} + +func TestConnectErrorRendering(t *testing.T) { + origErr := fmt.Errorf("failed reaching server: context deadline exceeded") + d := &connectDiagnosis{ + Address: "foo.bar.tmprl.cloud:7233", + Cause: causeClientCertRequired, + Stages: []diagStage{ + {diagOK, "DNS resolved (3 addresses)"}, + {diagOK, "TCP connection established"}, + {diagFail, "TLS handshake failed: server requires mTLS, no valid client certificate was provided"}, + }, + } + err := newConnectError(d, connectMeta{ + Args: []string{"workflow", "list", "--address", "foo.bar.tmprl.cloud:7233"}, + Address: "foo.bar.tmprl.cloud:7233", + }, origErr) + + msg := err.Error() + assert.Contains(t, msg, "failed connecting to Temporal server at foo.bar.tmprl.cloud:7233: TLS handshake failed: server requires client certificate (mTLS)") + assert.Contains(t, msg, "Connecting to foo.bar.tmprl.cloud:7233") + assert.Contains(t, msg, "✓ DNS resolved (3 addresses)") + assert.Contains(t, msg, "✗ TLS handshake failed") + assert.Contains(t, msg, "--tls-cert-path YourCert.pem") + // Unwrap preserves the original error. + assert.ErrorContains(t, err.Unwrap(), "context deadline exceeded") +} diff --git a/internal/temporalcli/connecterror.go b/internal/temporalcli/connecterror.go new file mode 100644 index 000000000..76866cf0d --- /dev/null +++ b/internal/temporalcli/connecterror.go @@ -0,0 +1,216 @@ +package temporalcli + +import ( + "fmt" + "net" + "strings" + + "github.com/fatih/color" +) + +// connectError is returned by dialClient when connecting to the server fails. +// Error() renders the full multi-line diagnosis (summary, probe stages, and a +// suggested fix); Unwrap() preserves the original dial error for +// errors.Is/As. +type connectError struct { + rendered string + cause error +} + +func (e *connectError) Error() string { return e.rendered } +func (e *connectError) Unwrap() error { return e.cause } + +// connectMeta carries the request context the suggestion engine needs to +// propose a concrete fix (e.g. reconstructing the exact command the user +// typed with the missing flags appended). +type connectMeta struct { + // Args are the CLI args as typed (without the binary name). + Args []string + Address string + AddressSource string // "flag", "profile", or "default" + ProfileName string + HasAPIKey bool + TLSConfigured bool +} + +// newConnectError renders a connection failure into a connectError. It must +// be called while command execution is active so that color state (JSON mode, +// --color) is applied correctly; the rendering is captured eagerly. +func newConnectError(d *connectDiagnosis, meta connectMeta, origErr error) *connectError { + var b strings.Builder + b.WriteString("failed connecting to Temporal server at ") + b.WriteString(meta.Address) + b.WriteString(": ") + b.WriteString(connectSummary(d, origErr)) + if len(d.Stages) > 0 { + b.WriteString("\n\n Connecting to ") + b.WriteString(meta.Address) + for _, stage := range d.Stages { + if stage.Status == diagOK { + b.WriteString("\n " + color.GreenString("✓") + " " + stage.Label) + } else { + b.WriteString("\n " + color.RedString("✗") + " " + stage.Label) + } + } + } + if suggestion := suggestFix(d, meta); suggestion != "" { + b.WriteString("\n\n") + b.WriteString(indentLines(suggestion, " ")) + } + return &connectError{rendered: b.String(), cause: origErr} +} + +// connectSummary is the one-line cause appended to the first error line. For +// unclassified failures and timeouts it keeps the original error text so +// scripts matching on strings like "context deadline exceeded" keep working. +func connectSummary(d *connectDiagnosis, origErr error) string { + switch d.Cause { + case causeDNS: + return "could not resolve host" + case causeTCPRefused: + return "connection refused" + case causeTCPTimeout: + return "connection timed out" + case causeServerPlaintext: + return "TLS is enabled but the server did not respond with TLS" + case causeServerSpeaksTLS: + return "the server requires TLS but the CLI is connecting without it" + case causeClientCertRequired: + return "TLS handshake failed: server requires client certificate (mTLS)" + case causeCAVerify: + return "cannot verify server TLS certificate" + case causeHostnameMismatch: + return "server TLS certificate does not match host" + case causeCertFileUnreadable: + return fmt.Sprintf("cannot read file %q", d.Detail) + case causeAuth: + if d.Detail != "" { + return "authentication failed: " + d.Detail + } + return "authentication failed" + default: + return shortErr(origErr) + } +} + +// suggestFix maps a classified failure to one concrete next step. First match +// wins; an empty return means no suggestion (the stages still render). +func suggestFix(d *connectDiagnosis, meta connectMeta) string { + host, port, _ := net.SplitHostPort(meta.Address) + switch d.Cause { + case causeCertFileUnreadable: + return fmt.Sprintf("Cannot read %q — check that the path exists and is readable.", d.Detail) + case causeClientCertRequired: + var b strings.Builder + if isCloudHost(host) { + b.WriteString("This looks like a Temporal Cloud endpoint secured with mTLS. Provide client certificates:") + } else { + b.WriteString("The server requires client certificates (mTLS). Provide them:") + } + b.WriteString("\n\n") + b.WriteString(indentLines(reconstructCommand(meta.Args, "--tls-cert-path YourCert.pem", "--tls-key-path YourKey.pem"), " ")) + b.WriteString("\n\nOr configure once:\n\n") + b.WriteString(" temporal config set --prop tls.client_cert_path YourCert.pem\n") + b.WriteString(" temporal config set --prop tls.client_key_path YourKey.pem") + if strings.HasSuffix(host, ".api.temporal.io") { + b.WriteString("\n\nIf your namespace uses API-key auth instead, pass --api-key YourApiKey.") + } + return b.String() + case causeAuth: + if meta.HasAPIKey { + return "The server rejected the provided API key. Verify the key is valid and that the address is your namespace's gRPC endpoint (for Temporal Cloud API keys, a regional endpoint like us-west-2.aws.api.temporal.io:7233)." + } + return "The server rejected the request as unauthenticated. If it requires an API key, pass --api-key; if it requires mTLS, pass --tls-cert-path and --tls-key-path." + case causeServerPlaintext: + return fmt.Sprintf("The server at %s does not appear to use TLS. Remove --tls and certificate flags, or double-check the address and port.", meta.Address) + case causeServerSpeaksTLS: + var b strings.Builder + b.WriteString("The server requires TLS. Add --tls:") + b.WriteString("\n\n") + b.WriteString(indentLines(reconstructCommand(meta.Args, "--tls"), " ")) + if isCloudHost(host) { + b.WriteString("\n\nTemporal Cloud endpoints also need client credentials: --tls-cert-path/--tls-key-path or --api-key.") + } + return b.String() + case causeDNS: + s := fmt.Sprintf("Could not resolve %q — check the server address.", host) + if meta.AddressSource == "profile" { + profile := meta.ProfileName + if profile == "" { + profile = "default" + } + s += fmt.Sprintf("\nThe address comes from config profile %q — inspect it with:\n\n temporal config get --prop address", profile) + } + return s + case causeTCPRefused: + if isLoopbackHost(host) && port == "7233" { + return fmt.Sprintf("No Temporal server is running at %s. Start a local dev server with:\n\n temporal server start-dev", meta.Address) + } + return fmt.Sprintf("Nothing is listening at %s — verify the address and that the server is running.", meta.Address) + case causeCAVerify: + return "The server's TLS certificate is not trusted by your system roots. If the server uses a private CA, pass --tls-ca-path YourServerCA.pem." + case causeHostnameMismatch: + return fmt.Sprintf("The server's TLS certificate is not valid for %q. If you connect via an IP or alternate name, set --tls-server-name to the name in the certificate.", host) + case causeTCPTimeout, causeTimeout: + return fmt.Sprintf("The connection stalled — a firewall or proxy may be blocking traffic to %s. Verify the address, port, and network path. Bound the wait with --client-connect-timeout.", meta.Address) + } + return "" +} + +func isCloudHost(host string) bool { + return strings.HasSuffix(host, ".tmprl.cloud") || strings.HasSuffix(host, ".api.temporal.io") +} + +func isLoopbackHost(host string) bool { + if host == "localhost" { + return true + } + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +} + +// reconstructCommand re-renders the command the user typed, one option per +// line (matching the style used in help text), with extraFlags appended. +func reconstructCommand(args []string, extraFlags ...string) string { + // Split leading command words from options. + var words, opts []string + for i, arg := range args { + if strings.HasPrefix(arg, "-") { + opts = args[i:] + break + } + words = append(words, arg) + } + var lines []string + lines = append(lines, "temporal "+strings.Join(words, " ")) + for i := 0; i < len(opts); i++ { + line := opts[i] + // Attach the option's value, if the next arg isn't another option. + if !strings.Contains(line, "=") && i+1 < len(opts) && !strings.HasPrefix(opts[i+1], "-") { + i++ + line += " " + quoteArg(opts[i]) + } + lines = append(lines, " "+line) + } + for _, flag := range extraFlags { + lines = append(lines, " "+flag) + } + return strings.Join(lines, " \\\n") +} + +func quoteArg(arg string) string { + if strings.ContainsAny(arg, " \t\"'") { + return fmt.Sprintf("%q", arg) + } + return arg +} + +func indentLines(s, indent string) string { + lines := strings.Split(s, "\n") + for i, line := range lines { + if line != "" { + lines[i] = indent + line + } + } + return strings.Join(lines, "\n") +} From f5ae5ead140e776b0ee88f68d05b281409f7a510 Mon Sep 17 00:00:00 2001 From: Ross Nelson Date: Wed, 8 Jul 2026 12:27:04 -0400 Subject: [PATCH 2/5] Add unit tests for error classification and summary rendering Covers classifyGRPCError, connectSummary's grep-compatibility contract, and an end-to-end case where the failing address comes from a config profile (exercising the new cliext builder metadata). --- internal/temporalcli/client_test.go | 28 +++++++++ internal/temporalcli/connectdiag_test.go | 72 ++++++++++++++++++++++++ 2 files changed, 100 insertions(+) diff --git a/internal/temporalcli/client_test.go b/internal/temporalcli/client_test.go index 6593e57b5..aa37fa370 100644 --- a/internal/temporalcli/client_test.go +++ b/internal/temporalcli/client_test.go @@ -11,6 +11,7 @@ import ( "encoding/pem" "math/big" "net" + "os" "testing" "time" @@ -223,6 +224,33 @@ func TestConnectDiagnosis_CertFileMissing(t *testing.T) { assert.NotContains(t, msg, "✓", "no probe stages expected when the dial never happened") } +func TestConnectDiagnosis_ProfileAddressNamed(t *testing.T) { + // When the failing address comes from a config profile, the suggestion + // should say so and point at `temporal config get`. + f, err := os.CreateTemp("", "") + require.NoError(t, err) + t.Cleanup(func() { os.Remove(f.Name()) }) + _, err = f.WriteString(` +[profile.default] +address = "does-not-exist.invalid:7233" +`) + require.NoError(t, err) + require.NoError(t, f.Close()) + + h := NewCommandHarness(t) + h.Options.EnvLookup = EnvLookupMap{"TEMPORAL_CONFIG_FILE": f.Name()} + res := h.Execute( + "workflow", "list", + "--client-connect-timeout", "5s", + ) + + require.Error(t, res.Err) + msg := res.Err.Error() + assert.Contains(t, msg, "failed connecting to Temporal server at does-not-exist.invalid:7233") + assert.Contains(t, msg, `The address comes from config profile "default"`) + assert.Contains(t, msg, "temporal config get --prop address") +} + func TestConnectDiagnosis_CommandTimeoutCauseSurvives(t *testing.T) { ln := startBlackHoleListener(t) diff --git a/internal/temporalcli/connectdiag_test.go b/internal/temporalcli/connectdiag_test.go index 5b10e8f1d..12e5d3414 100644 --- a/internal/temporalcli/connectdiag_test.go +++ b/internal/temporalcli/connectdiag_test.go @@ -18,6 +18,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.temporal.io/api/serviceerror" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) // newTestCert generates a self-signed server certificate for 127.0.0.1. @@ -195,6 +197,76 @@ func requireStage(t *testing.T, d *connectDiagnosis, status diagStatus, labelSub t.Fatalf("no stage with status %v containing %q in %+v", status, labelSubstr, d.Stages) } +func TestClassifyGRPCError(t *testing.T) { + tests := []struct { + name string + err error + cause connectCause + detail string + }{ + { + name: "serviceerror deadline exceeded", + err: fmt.Errorf("failed reaching server: %w", serviceerror.NewDeadlineExceeded("context deadline exceeded")), + cause: causeTimeout, + }, + { + name: "plain context deadline", + err: fmt.Errorf("failed reaching server: %w", context.DeadlineExceeded), + cause: causeTimeout, + }, + { + name: "unauthenticated status", + err: fmt.Errorf("failed reaching server: %w", status.Error(codes.Unauthenticated, "bad credentials")), + cause: causeAuth, + detail: "bad credentials", + }, + { + name: "permission denied status", + err: fmt.Errorf("failed reaching server: %w", status.Error(codes.PermissionDenied, "nope")), + cause: causeAuth, + }, + { + name: "unclassified error", + err: fmt.Errorf("something else entirely"), + cause: causeUnknown, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cause, detail := classifyGRPCError(tt.err) + assert.Equal(t, tt.cause, cause) + if tt.detail != "" { + assert.Contains(t, detail, tt.detail) + } + }) + } +} + +func TestConnectSummary(t *testing.T) { + origErr := fmt.Errorf("failed reaching server: context deadline exceeded") + tests := []struct { + cause connectCause + want string + }{ + {causeDNS, "could not resolve host"}, + {causeTCPRefused, "connection refused"}, + {causeTCPTimeout, "connection timed out"}, + {causeServerPlaintext, "TLS is enabled but the server did not respond with TLS"}, + {causeServerSpeaksTLS, "the server requires TLS but the CLI is connecting without it"}, + {causeClientCertRequired, "server requires client certificate (mTLS)"}, + {causeCAVerify, "cannot verify server TLS certificate"}, + {causeHostnameMismatch, "server TLS certificate does not match host"}, + // Unclassified failures keep the original error text (minus the SDK + // prefix) so scripts matching on it keep working. + {causeUnknown, "context deadline exceeded"}, + {causeTimeout, "context deadline exceeded"}, + } + for _, tt := range tests { + got := connectSummary(&connectDiagnosis{Cause: tt.cause}, origErr) + assert.Contains(t, got, tt.want, "cause %v", tt.cause) + } +} + func TestSuggestFix(t *testing.T) { cloudMeta := connectMeta{ Args: []string{"workflow", "list", "--address", "foo.bar.tmprl.cloud:7233"}, From f55c4ef8277289cdbac46d41b7f4c441c9778190 Mon Sep 17 00:00:00 2001 From: Ross Nelson Date: Sat, 11 Jul 2026 15:57:08 -0400 Subject: [PATCH 3/5] Fix Windows CI: connection-refused detection and plaintext test race - errors.Is(err, syscall.ECONNREFUSED) doesn't match Windows' WSAECONNREFUSED; fall back to matching the error message. - The plaintext test server closed with the client's ClientHello unread, sending an RST that on Windows discards the buffered HTTP response before the probe reads it; drain before closing. --- internal/temporalcli/connectdiag.go | 11 ++++++++++- internal/temporalcli/connectdiag_test.go | 16 ++++++++++++++-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/internal/temporalcli/connectdiag.go b/internal/temporalcli/connectdiag.go index e6eda32a7..3a5593416 100644 --- a/internal/temporalcli/connectdiag.go +++ b/internal/temporalcli/connectdiag.go @@ -104,7 +104,7 @@ func diagnoseConnection(ctx context.Context, address string, tlsCfg *tls.Config, // Stage: TCP conn, err := (&net.Dialer{}).DialContext(ctx, "tcp", address) if err != nil { - if errors.Is(err, syscall.ECONNREFUSED) { + if isConnRefused(err) { d.fail("TCP connection refused: nothing is listening at " + address) d.Cause = causeTCPRefused } else if isTimeout(err) { @@ -246,6 +246,15 @@ func (d *connectDiagnosis) fail(label string) { d.Stages = append(d.Stages, diagStage{diagFail, label}) } +// isConnRefused reports whether err is a refused TCP connection. errors.Is +// against syscall.ECONNREFUSED matches on unix, but Windows dials fail with +// WSAECONNREFUSED ("No connection could be made because the target machine +// actively refused it"), which Go does not map to syscall.ECONNREFUSED. +func isConnRefused(err error) bool { + return errors.Is(err, syscall.ECONNREFUSED) || + strings.Contains(err.Error(), "refused") +} + func isTimeout(err error) bool { var netErr net.Error return errors.Is(err, context.DeadlineExceeded) || diff --git a/internal/temporalcli/connectdiag_test.go b/internal/temporalcli/connectdiag_test.go index 12e5d3414..456528747 100644 --- a/internal/temporalcli/connectdiag_test.go +++ b/internal/temporalcli/connectdiag_test.go @@ -127,8 +127,20 @@ func TestDiagnoseConnection_ServerPlaintext(t *testing.T) { if err != nil { return } - _, _ = conn.Write([]byte("HTTP/1.1 400 Bad Request\r\n\r\n")) - conn.Close() + go func() { + defer conn.Close() + _, _ = conn.Write([]byte("HTTP/1.1 400 Bad Request\r\n\r\n")) + // Drain the client's ClientHello before closing: closing with + // unread data pending sends an RST, which on Windows discards + // the response before the client can read it. + _ = conn.SetReadDeadline(time.Now().Add(2 * time.Second)) + buf := make([]byte, 1024) + for { + if _, err := conn.Read(buf); err != nil { + return + } + } + }() } }() From e23832e7a92d0b7ed8daf104b8e75db6ee454b35 Mon Sep 17 00:00:00 2001 From: Ross Nelson Date: Wed, 22 Jul 2026 15:13:26 -0400 Subject: [PATCH 4/5] Refactor connection errors through terminal reports --- cliext/client.go | 30 -- cmd/temporal/main.go | 4 +- internal/temporalcli/client.go | 108 +++++-- internal/temporalcli/client_test.go | 22 +- internal/temporalcli/commands.activity.go | 21 +- .../commands.activity_internal_test.go | 31 ++ .../temporalcli/commands.activity_test.go | 7 +- internal/temporalcli/commands.extension.go | 12 +- .../temporalcli/commands.extension_test.go | 9 +- internal/temporalcli/commands.go | 233 +++++++++++++-- .../temporalcli/commands.schedule_test.go | 3 +- internal/temporalcli/commands_test.go | 52 +++- internal/temporalcli/connectdiag.go | 20 +- internal/temporalcli/connectdiag_test.go | 88 ++++-- internal/temporalcli/connecterror.go | 190 ++++++------ internal/temporalcli/terminal.go | 276 ++++++++++++++++++ internal/temporalcli/terminal_test.go | 268 +++++++++++++++++ 17 files changed, 1121 insertions(+), 253 deletions(-) create mode 100644 internal/temporalcli/commands.activity_internal_test.go create mode 100644 internal/temporalcli/terminal.go create mode 100644 internal/temporalcli/terminal_test.go diff --git a/cliext/client.go b/cliext/client.go index e723c1d73..48eaa7508 100644 --- a/cliext/client.go +++ b/cliext/client.go @@ -2,9 +2,7 @@ package cliext import ( "context" - "errors" "fmt" - "io/fs" "log/slog" "net/http" "strings" @@ -36,19 +34,6 @@ type ClientOptionsBuilder struct { // configured. Callers can use it to decode payloads outside the gRPC // interceptor chain (e.g. payloads nested inside opaque proto bytes). PayloadCodec converter.PayloadCodec - - // ResolvedAddress is populated by Build with the final server address, - // after profile resolution and namespace templating. - ResolvedAddress string - // ResolvedAddressSource is populated by Build with where the final address - // came from: "flag", "profile", or "default". - ResolvedAddressSource string - // ResolvedProfileName is populated by Build with the config profile name - // used, if any. - ResolvedProfileName string - // HasAPIKey is populated by Build and reports whether API-key credentials - // are configured (via flag, profile, or OAuth). - HasAPIKey bool } type oauthCredentials struct { @@ -123,12 +108,8 @@ func (b *ClientOptionsBuilder) Build(ctx context.Context) (client.Options, error // override the profile version unless it was _explicitly_ set. if cfg.FlagSet != nil && cfg.FlagSet.Changed("address") { profile.Address = cfg.Address - b.ResolvedAddressSource = "flag" } else if profile.Address == "" { profile.Address = cfg.Address - b.ResolvedAddressSource = "default" - } else { - b.ResolvedAddressSource = "profile" } var namespaceExplicitlySet bool if cfg.FlagSet != nil && cfg.FlagSet.Changed("namespace") { @@ -145,7 +126,6 @@ func (b *ClientOptionsBuilder) Build(ctx context.Context) (client.Options, error if addressHasNamespaceTemplate { profile.Address = strings.ReplaceAll(profile.Address, addressNamespaceTemplate, profile.Namespace) } - b.ResolvedAddress = profile.Address // Set API key on profile if provided if cfg.ApiKey != "" { @@ -224,18 +204,9 @@ func (b *ClientOptionsBuilder) Build(ctx context.Context) (client.Options, error profile.Codec.Auth = cfg.CodecAuth } - b.ResolvedProfileName = common.Profile - b.HasAPIKey = profile.APIKey != "" - // Convert profile to client options. clientOpts, err := profile.ToClientOptions(envconfig.ToClientOptionsRequest{}) if err != nil { - // File-path errors (e.g. an unreadable TLS cert) are common enough to - // deserve naming the offending file. - var pathErr *fs.PathError - if errors.As(err, &pathErr) { - return client.Options{}, fmt.Errorf("failed to build client options: cannot read file %q: %w", pathErr.Path, err) - } return client.Options{}, fmt.Errorf("failed to build client options: %w", err) } @@ -278,7 +249,6 @@ func (b *ClientOptionsBuilder) Build(ctx context.Context) (client.Options, error profileName: result.ProfileName, } clientOpts.Credentials = client.NewAPIKeyDynamicCredentials(creds.getToken) - b.HasAPIKey = true } } diff --git a/cmd/temporal/main.go b/cmd/temporal/main.go index 0c482a6ed..8c14f2f83 100644 --- a/cmd/temporal/main.go +++ b/cmd/temporal/main.go @@ -2,6 +2,7 @@ package main import ( "context" + "os" "github.com/temporalio/cli/internal/temporalcli" @@ -15,5 +16,6 @@ import ( func main() { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - temporalcli.Execute(ctx, temporalcli.CommandOptions{}) + result := temporalcli.Execute(ctx, temporalcli.CommandOptions{}) + os.Exit(result.ExitStatus) } diff --git a/internal/temporalcli/client.go b/internal/temporalcli/client.go index 81dfe97c7..d96a41685 100644 --- a/internal/temporalcli/client.go +++ b/internal/temporalcli/client.go @@ -7,10 +7,12 @@ import ( "io/fs" "os" "os/user" + "strings" "github.com/temporalio/cli/cliext" "go.temporal.io/api/common/v1" "go.temporal.io/sdk/client" + "go.temporal.io/sdk/contrib/envconfig" "go.temporal.io/sdk/converter" "go.temporal.io/sdk/workflow" "google.golang.org/grpc" @@ -48,6 +50,7 @@ func dialClientWithCodec(cctx *CommandContext, c *cliext.ClientOptions) (client. } c.Identity = "temporal-cli:" + username + "@" + hostname } + captureEffectiveConnectionSecrets(cctx, c) // Build client options using cliext builder := &cliext.ClientOptionsBuilder{ @@ -63,11 +66,11 @@ func dialClientWithCodec(cctx *CommandContext, c *cliext.ClientOptions) (client. var pathErr *fs.PathError if errors.As(err, &pathErr) { diag := &connectDiagnosis{ - Address: builder.ResolvedAddress, + Address: c.Address, Cause: causeCertFileUnreadable, Detail: pathErr.Path, } - return nil, nil, newConnectError(diag, connectMetaFromBuilder(cctx, builder), err) + return nil, nil, newConnectError(diag, connectMetaFromOptions(cctx, c, client.Options{HostPort: c.Address}), err) } return nil, nil, err } @@ -100,7 +103,7 @@ func dialClientWithCodec(cctx *CommandContext, c *cliext.ClientOptions) (client. cl, err := client.DialContext(dialCtx, clientOpts) if err != nil { - return nil, nil, dialConnectError(cctx, dialCtx, builder, clientOpts, err) + return nil, nil, dialConnectError(cctx, dialCtx, c, clientOpts, err) } // Since this namespace value is used by many commands after this call, @@ -115,7 +118,7 @@ func dialClientWithCodec(cctx *CommandContext, c *cliext.ClientOptions) (client. func dialConnectError( cctx *CommandContext, dialCtx context.Context, - builder *cliext.ClientOptionsBuilder, + configured *cliext.ClientOptions, clientOpts client.Options, origErr error, ) error { @@ -134,27 +137,90 @@ func dialConnectError( if v, _ := cctx.Options.EnvLookup.LookupEnv("TEMPORAL_CLI_DISABLE_CONNECT_DIAGNOSIS"); v != "" { return fmt.Errorf("failed connecting to Temporal server at %v: %w", clientOpts.HostPort, origErr) } - // The probe has its own internal budget, but never let it exceed the - // user's explicit connect timeout. - probeCtx := context.Context(cctx) - if timeout := cctx.RootCommand.CommonOptions.ClientConnectTimeout.Duration(); timeout > 0 { - var cancel context.CancelFunc - probeCtx, cancel = context.WithTimeout(cctx, timeout) - defer cancel() - } - diag := diagnoseConnection(probeCtx, clientOpts.HostPort, clientOpts.ConnectionOptions.TLS, origErr) - meta := connectMetaFromBuilder(cctx, builder) + // Diagnosis uses the remaining command context and applies its own 3s cap. + // ClientConnectTimeout governs only the original dial. + diag := diagnoseConnection(cctx, clientOpts.HostPort, clientOpts.ConnectionOptions.TLS, origErr) + meta := connectMetaFromOptions(cctx, configured, clientOpts) meta.TLSConfigured = clientOpts.ConnectionOptions.TLS != nil return newConnectError(diag, meta, origErr) } -func connectMetaFromBuilder(cctx *CommandContext, builder *cliext.ClientOptionsBuilder) connectMeta { - return connectMeta{ - Args: cctx.Options.Args, - Address: builder.ResolvedAddress, - AddressSource: builder.ResolvedAddressSource, - ProfileName: builder.ResolvedProfileName, - HasAPIKey: builder.HasAPIKey, +func connectMetaFromOptions(cctx *CommandContext, configured *cliext.ClientOptions, resolved client.Options) connectMeta { + meta := connectMeta{Address: resolved.HostPort, HasAPIKey: cctx.hasAPIKey, HasOAuth: cctx.hasOAuth} + if configured != nil && configured.FlagSet != nil && configured.FlagSet.Changed("address") { + meta.AddressSource = "flag" + } else if configured != nil && resolved.HostPort != configured.Address { + meta.AddressSource = "profile" + } else { + meta.AddressSource = "default" + } + if cctx.RootCommand != nil { + meta.ProfileName = cctx.RootCommand.CommonOptions.Profile + } + return meta +} + +func captureEffectiveConnectionSecrets(cctx *CommandContext, configured *cliext.ClientOptions) { + if configured != nil { + cctx.registerSecrets( + configured.ApiKey, + configured.CodecAuth, + configured.TlsCertData, + configured.TlsKeyData, + configured.TlsCaData, + ) + for _, value := range append(append([]string(nil), configured.CodecHeader...), configured.GrpcMeta...) { + cctx.registerSecrets(value) + if _, secret, ok := strings.Cut(value, "="); ok { + cctx.registerSecrets(secret) + } + } + if configured.ApiKey != "" { + cctx.hasAPIKey = true + } + } + if cctx.RootCommand == nil { + return + } + common := cctx.RootCommand.CommonOptions + if !common.DisableConfigFile || !common.DisableConfigEnv { + profile, err := envconfig.LoadClientConfigProfile(envconfig.LoadClientConfigProfileOptions{ + ConfigFilePath: common.ConfigFile, ConfigFileProfile: common.Profile, + DisableFile: common.DisableConfigFile, DisableEnv: common.DisableConfigEnv, + EnvLookup: cctx.Options.EnvLookup, + }) + if err == nil { + cctx.registerSecrets(profile.APIKey) + cctx.hasAPIKey = cctx.hasAPIKey || profile.APIKey != "" + if profile.TLS != nil { + cctx.registerSecrets(string(profile.TLS.ClientCertData), string(profile.TLS.ClientKeyData), string(profile.TLS.ServerCACertData)) + } + if profile.Codec != nil { + cctx.registerSecrets(profile.Codec.Auth) + } + for _, value := range profile.GRPCMeta { + cctx.registerSecrets(value) + } + } + } + // Match ClientOptionsBuilder precedence: an explicit --api-key suppresses + // OAuth loading, while a profile API key can still be replaced by a usable + // OAuth token from the same profile. + if common.DisableConfigFile || configured != nil && configured.ApiKey != "" { + return + } + oauth, err := cliext.LoadClientOAuth(cliext.LoadClientOAuthOptions{ + ConfigFilePath: common.ConfigFile, ProfileName: common.Profile, EnvLookup: cctx.Options.EnvLookup, + }) + if err != nil || oauth.OAuth == nil { + return + } + if oauth.OAuth.ClientConfig != nil { + cctx.registerSecrets(oauth.OAuth.ClientConfig.ClientSecret) + } + if oauth.OAuth.Token != nil { + cctx.registerSecrets(oauth.OAuth.Token.AccessToken, oauth.OAuth.Token.RefreshToken) + cctx.hasOAuth = oauth.OAuth.Token.AccessToken != "" } } diff --git a/internal/temporalcli/client_test.go b/internal/temporalcli/client_test.go index aa37fa370..c83039313 100644 --- a/internal/temporalcli/client_test.go +++ b/internal/temporalcli/client_test.go @@ -108,7 +108,7 @@ func TestClientConnectTimeout(t *testing.T) { assert.Contains(t, res.Err.Error(), "deadline exceeded") // The friendly cause attached to the connect timeout must survive. assert.Contains(t, res.Err.Error(), "command timed out after 50ms") - assert.Less(t, elapsed, 2*time.Second, "dial and diagnosis should respect the connect timeout") + assert.Less(t, elapsed, 4*time.Second, "diagnosis should respect its independent three-second cap") } func TestConnectDiagnosis_MTLSRequired(t *testing.T) { @@ -124,12 +124,12 @@ func TestConnectDiagnosis_MTLSRequired(t *testing.T) { ) require.Error(t, res.Err) - msg := res.Err.Error() + msg := res.Stderr.String() assert.Contains(t, msg, "Connecting to "+addr) assert.Contains(t, msg, "✓ TCP connection established") assert.Contains(t, msg, "✗ TLS handshake failed: server requires mTLS") - assert.Contains(t, msg, "--tls-cert-path YourCert.pem") - assert.Contains(t, msg, "--tls-key-path YourKey.pem") + assert.Contains(t, msg, "tls.client_cert_path --value YourCert.pem") + assert.Contains(t, msg, "tls.client_key_path --value YourKey.pem") } func TestConnectDiagnosis_Refused(t *testing.T) { @@ -147,7 +147,7 @@ func TestConnectDiagnosis_Refused(t *testing.T) { ) require.Error(t, res.Err) - msg := res.Err.Error() + msg := res.Stderr.String() assert.Contains(t, msg, "connection refused") assert.Contains(t, msg, "✗ TCP connection refused") assert.Contains(t, msg, "Nothing is listening at "+addr) @@ -163,7 +163,7 @@ func TestConnectDiagnosis_DNSFailure(t *testing.T) { ) require.Error(t, res.Err) - msg := res.Err.Error() + msg := res.Stderr.String() assert.Contains(t, msg, "could not resolve host") assert.Contains(t, msg, "✗ DNS lookup") assert.Contains(t, msg, `Could not resolve "does-not-exist.invalid"`) @@ -184,7 +184,7 @@ func TestConnectDiagnosis_JSONOutputHasNoANSI(t *testing.T) { ) require.Error(t, res.Err) - assert.NotContains(t, res.Err.Error(), "\x1b[", "diagnosis must not contain ANSI escapes in JSON mode") + assert.NotContains(t, res.Stderr.String(), "\x1b[", "diagnosis must not contain ANSI escapes in JSON mode") assert.Empty(t, res.Stdout.String(), "connection failures must not write to stdout") } @@ -203,7 +203,7 @@ func TestConnectDiagnosis_Disabled(t *testing.T) { ) require.Error(t, res.Err) - msg := res.Err.Error() + msg := res.Stderr.String() assert.Contains(t, msg, "failed connecting to Temporal server at "+addr) assert.NotContains(t, msg, "✗", "diagnosis must be suppressed when disabled") } @@ -218,7 +218,7 @@ func TestConnectDiagnosis_CertFileMissing(t *testing.T) { ) require.Error(t, res.Err) - msg := res.Err.Error() + msg := res.Stderr.String() assert.Contains(t, msg, "cannot read file") assert.Contains(t, msg, "/definitely/does/not/exist") assert.NotContains(t, msg, "✓", "no probe stages expected when the dial never happened") @@ -245,10 +245,10 @@ address = "does-not-exist.invalid:7233" ) require.Error(t, res.Err) - msg := res.Err.Error() + msg := res.Stderr.String() assert.Contains(t, msg, "failed connecting to Temporal server at does-not-exist.invalid:7233") assert.Contains(t, msg, `The address comes from config profile "default"`) - assert.Contains(t, msg, "temporal config get --prop address") + assert.Contains(t, msg, "temporal config get --prop address --profile default") } func TestConnectDiagnosis_CommandTimeoutCauseSurvives(t *testing.T) { diff --git a/internal/temporalcli/commands.activity.go b/internal/temporalcli/commands.activity.go index 582f5d622..ba7ecc90d 100644 --- a/internal/temporalcli/commands.activity.go +++ b/internal/temporalcli/commands.activity.go @@ -223,7 +223,7 @@ func getActivityResult(cctx *CommandContext, cl client.Client, namespace, activi if err != nil { var notFound *serviceerror.NotFound if errors.As(err, ¬Found) { - return fmt.Errorf("activity not found: %s", activityID) + return &activityNotFoundError{activityID: activityID, cause: err} } return fmt.Errorf("failed polling activity result: %w", err) } @@ -241,6 +241,25 @@ func getActivityResult(cctx *CommandContext, cl client.Client, namespace, activi } } +// activityNotFoundError retains the server's typed NotFound error while +// carrying the audited display semantics for this command family. +type activityNotFoundError struct { + activityID string + cause error +} + +func (e *activityNotFoundError) Error() string { + return fmt.Sprintf("activity not found: %s", e.activityID) +} + +func (e *activityNotFoundError) Unwrap() error { + return e.cause +} + +func (e *activityNotFoundError) report() errorReport { + return errorReport{Summary: "standalone Activity not found"} +} + // Matches the SDK's pollActivityTimeout in internal_activity_client.go. const pollActivityTimeout = 60 * time.Second diff --git a/internal/temporalcli/commands.activity_internal_test.go b/internal/temporalcli/commands.activity_internal_test.go new file mode 100644 index 000000000..43829453d --- /dev/null +++ b/internal/temporalcli/commands.activity_internal_test.go @@ -0,0 +1,31 @@ +package temporalcli + +import ( + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.temporal.io/api/serviceerror" +) + +func TestActivityNotFoundErrorPreservesCauseAndHasConservativeReport(t *testing.T) { + cause, ok := serviceerror.NewNotFound("server detail").(*serviceerror.NotFound) + require.True(t, ok) + wrappedCause := fmt.Errorf("poll failed: %w", cause) + err := &activityNotFoundError{activityID: "activity-id", cause: wrappedCause} + + var notFound *serviceerror.NotFound + assert.ErrorAs(t, err, ¬Found) + assert.Same(t, cause, notFound) + assert.Equal(t, "activity not found: activity-id", err.Error()) + + report := err.report() + assert.Equal(t, "standalone Activity not found", report.Summary) + assert.Empty(t, report.Context) + assert.Empty(t, report.Checks) + assert.Nil(t, report.Action) + assert.True(t, errors.Is(err, wrappedCause)) + assert.True(t, errors.Is(err, cause)) +} diff --git a/internal/temporalcli/commands.activity_test.go b/internal/temporalcli/commands.activity_test.go index ec6b50a4f..1bdfd4e55 100644 --- a/internal/temporalcli/commands.activity_test.go +++ b/internal/temporalcli/commands.activity_test.go @@ -819,7 +819,12 @@ func (s *SharedServerSuite) TestActivity_Result_NotFound() { ) s.Error(res.Err) s.Contains(res.Err.Error(), "not found") - s.NotContains(res.Stdout.String(), "FAILED") + var notFound *serviceerror.NotFound + s.ErrorAs(res.Err, ¬Found) + s.Equal(1, res.Runtime.ExitStatus) + s.Contains(res.Stderr.String(), "Error: standalone Activity not found") + s.NotContains(res.Stderr.String(), "Try") + s.Empty(res.Stdout.String()) } func (s *SharedServerSuite) TestActivity_Describe() { diff --git a/internal/temporalcli/commands.extension.go b/internal/temporalcli/commands.extension.go index 794242263..2ac7a1df6 100644 --- a/internal/temporalcli/commands.extension.go +++ b/internal/temporalcli/commands.extension.go @@ -25,6 +25,14 @@ var cliArgsToParseForExtension = map[string]bool{ "command-timeout": true, } +// ExtensionNonZeroExit preserves a started extension's exit status and marks +// its output as extension-owned, so the parent does not render another error. +type ExtensionNonZeroExit struct { + *exec.ExitError +} + +func (err ExtensionNonZeroExit) Unwrap() error { return err.ExitError } + // tryExecuteExtension tries to execute an extension command if the command is not a built-in command. // It returns an error if the extension command fails, and a boolean indicating whether an extension was executed. func tryExecuteExtension(cctx *CommandContext, tcmd *TemporalCommand) (error, bool) { @@ -77,8 +85,8 @@ func tryExecuteExtension(cctx *CommandContext, tcmd *TemporalCommand) (error, bo if ctx.Err() != nil { return fmt.Errorf("program interrupted"), true } - if _, ok := err.(*exec.ExitError); ok { - return nil, true + if exitError, ok := err.(*exec.ExitError); ok { + return ExtensionNonZeroExit{exitError}, true } return fmt.Errorf("extension %s failed: %w", extPath, err), true } diff --git a/internal/temporalcli/commands.extension_test.go b/internal/temporalcli/commands.extension_test.go index 266a8fcd1..2d438f146 100644 --- a/internal/temporalcli/commands.extension_test.go +++ b/internal/temporalcli/commands.extension_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/temporalio/cli/internal/temporalcli" "golang.org/x/tools/imports" ) @@ -237,7 +238,8 @@ func TestExtension_FailsOnNonExecutableCommand(t *testing.T) { res := h.Execute("foo") - assert.Contains(t, res.Stdout.String(), "Usage:") // help text is shown + assert.Empty(t, res.Stdout.String()) + assert.Contains(t, res.Stderr.String(), "Usage:") assert.EqualError(t, res.Err, "unknown command") } @@ -248,7 +250,10 @@ func TestExtension_PassesThroughNonZeroExit(t *testing.T) { res := h.Execute("foo") assert.Equal(t, "Args: temporal-foo \n", res.Stdout.String()) - assert.NoError(t, res.Err) + var extensionErr temporalcli.ExtensionNonZeroExit + assert.ErrorAs(t, res.Err, &extensionErr) + assert.Equal(t, 42, res.Runtime.ExitStatus) + assert.Empty(t, res.Stderr.String(), "extension owns its stderr") } func TestExtension_FailsOnCommandTimeout(t *testing.T) { diff --git a/internal/temporalcli/commands.go b/internal/temporalcli/commands.go index df5fcdb19..7ec38e187 100644 --- a/internal/temporalcli/commands.go +++ b/internal/temporalcli/commands.go @@ -58,10 +58,15 @@ type CommandContext struct { // Is set to true if any command actually started running. This is a hack to workaround the fact // that cobra does not properly exit nonzero if an unknown command/subcommand is given. ActuallyRanCommand bool + preRunSucceeded bool // Root/current command only set inside of pre-run RootCommand *TemporalCommand CurrentCommand *cobra.Command + commandCancel context.CancelFunc + secretValues []string + hasAPIKey bool + hasOAuth bool } type IOStreams struct { @@ -81,7 +86,8 @@ type CommandOptions struct { // If nil, [envconfig.EnvLookupOS] is used. EnvLookup envconfig.EnvLookup - // Defaults to logging error then os.Exit(1) + // Fail is used by generated Run callbacks to hand their final error to the + // command runtime. Execute replaces it with a command-scoped recorder. Fail func(error) AdditionalClientGRPCDialOptions []grpc.DialOption @@ -137,24 +143,6 @@ func (c *CommandContext) preprocessOptions() error { c.Options.Stderr = os.Stderr } - // Setup default fail callback - if c.Options.Fail == nil { - c.Options.Fail = func(err error) { - // If context is closed, say that the program was interrupted and - // ignore the actual error — unless the context carries a - // descriptive cause such as a --command-timeout expiry. - if c.Err() != nil { - if cause := context.Cause(c); cause != nil && !errors.Is(cause, context.Canceled) { - err = cause - } else { - err = fmt.Errorf("program interrupted") - } - } - fmt.Fprintf(c.Options.Stderr, "Error: %v\n", err) - os.Exit(1) - } - } - // Update options according to the env file. MUST BE DONE LAST. // // Why last? Callers need the CommandContext to be usable no matter what, @@ -360,10 +348,16 @@ func (c *CommandContext) promptString(message string, expected string, autoConfi return line == expected, nil } -// Execute runs the Temporal CLI with the given context and options. This -// intentionally does not return an error but rather invokes Fail on the -// options. -func Execute(ctx context.Context, options CommandOptions) { +// Execute runs the Temporal CLI and returns its terminal outcome. It never +// exits for a terminal command failure; cmd/temporal/main.go owns that process +// exit boundary. Existing success-output broken-pipe exits in printer are a +// separate compatibility path outside terminal error handling. +func Execute(ctx context.Context, options CommandOptions) Result { + var recorder commandErrorRecorder + options.Fail = recorder.Record + origNoColor := color.NoColor + defer func() { color.NoColor = origNoColor }() + // Create context and run. We always get a context and cancel func back even // if an error was returned. This is so we can use the context to print an // error message using the appropriate Fail() method, regardless of why the @@ -373,9 +367,17 @@ func Execute(ctx context.Context, options CommandOptions) { // config file, or some other issue in their environment.) cctx, cancel, err := NewCommandContext(ctx, options) defer cancel() + defer func() { + if cctx.commandCancel != nil { + cctx.commandCancel() + } + }() if err == nil { cmd := NewTemporalCommand(cctx) + // The terminal handler owns both error and input-error usage rendering. + cmd.Command.SilenceErrors = true + cmd.Command.SilenceUsage = true cmd.Command.SetArgs(cctx.Options.Args) cmd.Command.SetOut(cctx.Options.Stdout) cmd.Command.SetErr(cctx.Options.Stderr) @@ -383,20 +385,36 @@ func Execute(ctx context.Context, options CommandOptions) { // Try extension first. err, cctx.ActuallyRanCommand = tryExecuteExtension(cctx, cmd) if err != nil { - cctx.Options.Fail(err) - return + return finishCommand(cctx, err, usageForArgs(&cmd.Command, cctx.Options.Args)) } // Run builtin command if no extension handled the command. if !cctx.ActuallyRanCommand { - err = cmd.Command.ExecuteContext(cctx) + if unknownCommandPath(&cmd.Command, cctx.Options.Args) { + return finishCommand(cctx, fmt.Errorf("unknown command"), usageForArgs(&cmd.Command, nil)) + } + var cobraErr error + recorded := runRecordedCommand(func() { cobraErr = cmd.Command.ExecuteContext(cctx) }) + if recorded { + return finishCommand(cctx, recorder.Err(), "") + } + if cobraErr != nil { + usage := "" + if !cctx.ActuallyRanCommand || cctx.preRunSucceeded { + usage = usageForArgs(&cmd.Command, cctx.Options.Args) + } + return finishCommand(cctx, cobraErr, usage) + } + if err = recorder.Err(); err != nil { + return finishCommand(cctx, err, "") + } } } if err != nil { // Either we failed to create the context, OR the command itself failed. // Either way, we need to print an error message. - cctx.Options.Fail(err) + return finishCommand(cctx, err, "") } // If no command ever actually got run, exit nonzero with an error. This is @@ -413,9 +431,165 @@ func Execute(ctx context.Context, options CommandOptions) { if slices.ContainsFunc(cctx.Options.Args, func(a string) bool { return slices.Contains(zeroExitArgs, a) }) { + return Result{} + } + return finishCommand(cctx, fmt.Errorf("unknown command"), "") + } + return Result{} +} + +func usageForArgs(root *cobra.Command, args []string) string { + command, _, findErr := root.Find(args) + if findErr != nil || command == nil { + command = root + } + return command.UsageString() +} + +func unknownCommandPath(root *cobra.Command, args []string) bool { + current := root + for i := 0; i < len(args); i++ { + arg := args[i] + if strings.HasPrefix(arg, "-") { + name, inline := parseFlagArg(arg) + flag, takesValue := lookupFlag(current, name) + if flag == nil { + // Cobra owns invalid-flag classification. Guessing whether an + // unknown flag consumes the next token can misreport that token + // as an unknown command. + return false + } + if takesValue && !inline { + i++ + } + continue + } + var next *cobra.Command + for _, child := range current.Commands() { + if child.Name() == arg || slices.Contains(child.Aliases, arg) { + next = child + break + } + } + if next == nil { + return current.HasAvailableSubCommands() && current.Run == nil && current.RunE == nil + } + current = next + } + return false +} + +func finishCommand(cctx *CommandContext, err error, usage string) Result { + var extensionErr ExtensionNonZeroExit + if errors.As(err, &extensionErr) { + return Result{CommandErr: err, ExitStatus: extensionErr.ExitCode()} + } + displayErr := err + if cctx.Err() != nil { + if cause := context.Cause(cctx); cause != nil && !errors.Is(cause, context.Canceled) { + displayErr = cause + } else { + displayErr = fmt.Errorf("program interrupted") + } + } + return handleTerminalError(err, terminalOptions{ + Stderr: cctx.Options.Stderr, + Color: cctx.terminalColorEnabled(), + KnownSecrets: cctx.knownSecrets(), + Usage: usage, + DisplayErr: displayErr, + }) +} + +func (c *CommandContext) terminalColorEnabled() bool { + jsonOutput := c.JSONOutput + colorEnabled := !color.NoColor + for i := 0; i < len(c.Options.Args); i++ { + name, value, inline := strings.Cut(c.Options.Args[i], "=") + if !inline && i+1 < len(c.Options.Args) && (name == "-o" || name == "--output" || name == "--color") { + i++ + value = c.Options.Args[i] + } + switch name { + case "-o", "--output": + jsonOutput = jsonOutput || value == "json" || value == "jsonl" + case "--color": + if value == "always" { + colorEnabled = true + } else if value == "never" { + colorEnabled = false + } + } + } + if c.RootCommand != nil { + jsonOutput = jsonOutput || c.RootCommand.Output.Value == "json" || c.RootCommand.Output.Value == "jsonl" + switch c.RootCommand.Color.Value { + case "always": + colorEnabled = true + case "never": + colorEnabled = false + } + } + return colorEnabled && !jsonOutput +} + +func (c *CommandContext) knownSecrets() []string { + secretFlags := map[string]bool{ + "api-key": true, "codec-auth": true, "codec-header": true, + "grpc-meta": true, "tls-cert-data": true, "tls-key-data": true, + "tls-ca-data": true, + } + secrets := append([]string(nil), c.secretValues...) + if c.configEnvironmentEnabled() { + for _, name := range []string{ + "TEMPORAL_API_KEY", "TEMPORAL_CODEC_AUTH", "TEMPORAL_TLS_CERT_DATA", + "TEMPORAL_TLS_KEY_DATA", "TEMPORAL_TLS_CA_DATA", + } { + if value, ok := c.Options.EnvLookup.LookupEnv(name); ok && value != "" { + secrets = append(secrets, value) + } + } + } + if c.CurrentCommand == nil { + return secrets + } + appendFlagValues := func(flag *pflag.Flag) { + if values, ok := flag.Value.(pflag.SliceValue); ok { + for _, value := range values.GetSlice() { + secrets = append(secrets, value) + if _, secret, found := strings.Cut(value, "="); found && secret != "" { + secrets = append(secrets, secret) + } + } return } - cctx.Options.Fail(fmt.Errorf("unknown command")) + secrets = append(secrets, flag.Value.String()) + } + c.CurrentCommand.Flags().VisitAll(func(flag *pflag.Flag) { + if secretFlags[flag.Name] && flag.Changed { + appendFlagValues(flag) + } + }) + c.CurrentCommand.InheritedFlags().VisitAll(func(flag *pflag.Flag) { + if secretFlags[flag.Name] && flag.Changed { + appendFlagValues(flag) + } + }) + return secrets +} + +func (c *CommandContext) configEnvironmentEnabled() bool { + if c.RootCommand != nil { + return !c.RootCommand.CommonOptions.DisableConfigEnv + } + return !slices.Contains(c.Options.Args, "--disable-config-env") +} + +func (c *CommandContext) registerSecrets(values ...string) { + for _, value := range values { + if value != "" { + c.secretValues = append(c.secretValues, value) + } } } @@ -517,6 +691,7 @@ func (c *TemporalCommand) initCommand(cctx *CommandContext) { } } } + cctx.preRunSucceeded = res == nil return res } c.Command.PersistentPostRun = func(*cobra.Command, []string) { @@ -581,7 +756,7 @@ func (c *TemporalCommand) preRun(cctx *CommandContext) error { } cctx.JSONShorthandPayloads = !c.NoJsonShorthandPayloads if c.CommandTimeout.Duration() > 0 { - cctx.Context, _ = context.WithTimeoutCause( + cctx.Context, cctx.commandCancel = context.WithTimeoutCause( cctx.Context, c.CommandTimeout.Duration(), fmt.Errorf("command timed out after %v", c.CommandTimeout.Duration()), diff --git a/internal/temporalcli/commands.schedule_test.go b/internal/temporalcli/commands.schedule_test.go index d3bf978d9..d49a33f52 100644 --- a/internal/temporalcli/commands.schedule_test.go +++ b/internal/temporalcli/commands.schedule_test.go @@ -38,9 +38,8 @@ func (s *SharedServerSuite) createSchedule(args ...string) (schedId, schedWfId s "--address", s.Address(), "-s", schedId, }, - Fail: func(error) {}, } - temporalcli.Execute(ctx, options) + _ = temporalcli.Execute(ctx, options) }) res = s.Execute(append([]string{ "schedule", "create", diff --git a/internal/temporalcli/commands_test.go b/internal/temporalcli/commands_test.go index 1b1a18e51..0a171d0e0 100644 --- a/internal/temporalcli/commands_test.go +++ b/internal/temporalcli/commands_test.go @@ -6,6 +6,8 @@ import ( "fmt" "io" "log/slog" + "os" + "path/filepath" "regexp" "slices" "strings" @@ -120,6 +122,38 @@ func TestAssertContainsOnSameLine(t *testing.T) { require.NoError(t, AssertContainsOnSameLine("a a", "a", "a")) } +func TestExecuteReturnsStatusAndKeepsTerminalErrorsOnStderr(t *testing.T) { + badEnvFile := filepath.Join(t.TempDir(), "invalid.yaml") + require.NoError(t, os.WriteFile(badEnvFile, []byte("invalid: ["), 0o600)) + t.Run("help succeeds without terminal rendering", func(t *testing.T) { + res := NewCommandHarness(t).Execute("--help") + assert.NoError(t, res.Err) + assert.Zero(t, res.Runtime.ExitStatus) + assert.Empty(t, res.Stderr.String()) + assert.Contains(t, res.Stdout.String(), "Usage:") + }) + + for _, test := range []struct { + name string + args []string + wantUsage bool + }{ + {name: "parse failure", args: []string{"workflow", "describe", "--not-a-flag"}, wantUsage: true}, + {name: "required flag failure", args: []string{"workflow", "describe"}, wantUsage: true}, + {name: "pre-run/configuration failure", args: []string{"workflow", "list", "--env-file", badEnvFile}}, + {name: "runtime failure", args: []string{"config", "get", "--disable-config-file", "--disable-config-env"}}, + } { + t.Run(test.name, func(t *testing.T) { + res := NewCommandHarness(t).Execute(test.args...) + assert.Error(t, res.Err) + assert.Equal(t, 1, res.Runtime.ExitStatus) + assert.Equal(t, 1, strings.Count(res.Stderr.String(), "Error:")) + assert.Equal(t, test.wantUsage, strings.Contains(res.Stderr.String(), "Usage:")) + assert.Empty(t, res.Stdout.String()) + }) + } +} + func (h *CommandHarness) Eventually( condition func() bool, waitFor time.Duration, @@ -145,9 +179,10 @@ func (h *CommandHarness) T() *testing.T { } type CommandResult struct { - Err error - Stdout bytes.Buffer - Stderr bytes.Buffer + Err error + Runtime temporalcli.Result + Stdout bytes.Buffer + Stderr bytes.Buffer } func (h *CommandHarness) Execute(args ...string) *CommandResult { @@ -167,20 +202,13 @@ func (h *CommandHarness) Execute(args ...string) *CommandResult { if options.DeprecatedEnvConfig.DisableEnvConfig { options.DeprecatedEnvConfig.EnvConfigName = "default" } - // Capture error - options.Fail = func(err error) { - if res.Err != nil { - panic("fail called twice, just failed with " + err.Error()) - } - res.Err = err - } - // Run ctx, cancel := context.WithCancel(h.Context) h.t.Cleanup(cancel) defer cancel() h.t.Logf("Calling: %v", strings.Join(args, " ")) - temporalcli.Execute(ctx, options) + res.Runtime = temporalcli.Execute(ctx, options) + res.Err = res.Runtime.CommandErr if res.Stdout.Len() > 0 { h.t.Logf("Stdout:\n-----\n%s\n-----", &res.Stdout) } diff --git a/internal/temporalcli/connectdiag.go b/internal/temporalcli/connectdiag.go index 3a5593416..1009b135d 100644 --- a/internal/temporalcli/connectdiag.go +++ b/internal/temporalcli/connectdiag.go @@ -132,7 +132,11 @@ func diagnoseConnection(ctx context.Context, address string, tlsCfg *tls.Config, // Stage: gRPC — no re-dial; classify the original error. d.Cause, d.Detail = classifyGRPCError(origErr) - d.fail("gRPC connection failed: " + shortErr(origErr)) + if d.Cause == causeAuth { + d.fail("gRPC authentication failed") + } else { + d.fail("gRPC connection failed: " + shortErr(origErr)) + } return d } @@ -152,7 +156,8 @@ func (d *connectDiagnosis) probeTLS(ctx context.Context, conn net.Conn, host str } // Handshake OK; a short read distinguishes an mTLS rejection alert from a // genuinely healthy connection (where the read just times out). - _ = tlsConn.SetReadDeadline(time.Now().Add(connectDiagnosisReadProbe)) + stopCancel := armReadDeadline(ctx, tlsConn) + defer stopCancel() buf := make([]byte, 1) _, err := tlsConn.Read(buf) if err != nil && !isTimeout(err) { @@ -164,6 +169,15 @@ func (d *connectDiagnosis) probeTLS(ctx context.Context, conn net.Conn, host str d.ok("TLS handshake succeeded") } +func armReadDeadline(ctx context.Context, conn net.Conn) func() bool { + readDeadline := time.Now().Add(connectDiagnosisReadProbe) + if contextDeadline, ok := ctx.Deadline(); ok && contextDeadline.Before(readDeadline) { + readDeadline = contextDeadline + } + _ = conn.SetReadDeadline(readDeadline) + return context.AfterFunc(ctx, func() { _ = conn.SetReadDeadline(time.Now()) }) +} + func (d *connectDiagnosis) classifyTLSError(err error) { var recordErr tls.RecordHeaderError var certErr *tls.CertificateVerificationError @@ -232,7 +246,7 @@ func classifyGRPCError(err error) (connectCause, string) { if st, ok := status.FromError(unwrapped); ok { switch st.Code() { case codes.Unauthenticated, codes.PermissionDenied: - return causeAuth, st.Message() + return causeAuth, "" case codes.DeadlineExceeded: return causeTimeout, "" } diff --git a/internal/temporalcli/connectdiag_test.go b/internal/temporalcli/connectdiag_test.go index 456528747..babb900f0 100644 --- a/internal/temporalcli/connectdiag_test.go +++ b/internal/temporalcli/connectdiag_test.go @@ -8,10 +8,13 @@ import ( "crypto/tls" "crypto/x509" "crypto/x509/pkix" + "errors" "fmt" + "io" "math/big" "net" "strings" + "sync" "testing" "time" @@ -227,10 +230,9 @@ func TestClassifyGRPCError(t *testing.T) { cause: causeTimeout, }, { - name: "unauthenticated status", - err: fmt.Errorf("failed reaching server: %w", status.Error(codes.Unauthenticated, "bad credentials")), - cause: causeAuth, - detail: "bad credentials", + name: "unauthenticated status", + err: fmt.Errorf("failed reaching server: %w", status.Error(codes.Unauthenticated, "bad credentials")), + cause: causeAuth, }, { name: "permission denied status", @@ -279,10 +281,47 @@ func TestConnectSummary(t *testing.T) { } } +func TestAuthSummaryDoesNotCopyServerControlledStatusMessage(t *testing.T) { + d := &connectDiagnosis{Cause: causeAuth, Detail: "attacker text\nrun this"} + assert.Equal(t, "authentication failed", connectSummary(d, errors.New("fallback attacker text"))) +} + +func TestArmReadDeadlineUsesEarlierContextDeadlineAndCancellation(t *testing.T) { + ctx, cancel := context.WithTimeout(t.Context(), 50*time.Millisecond) + defer cancel() + conn := &deadlineRecordingConn{} + stop := armReadDeadline(ctx, conn) + defer stop() + require.Eventually(t, func() bool { return !conn.deadline().IsZero() }, time.Second, time.Millisecond) + assert.WithinDuration(t, time.Now().Add(50*time.Millisecond), conn.deadline(), 25*time.Millisecond) + cancel() + require.Eventually(t, func() bool { return time.Until(conn.deadline()) <= 0 }, time.Second, time.Millisecond) +} + +type deadlineRecordingConn struct { + mu sync.Mutex + d time.Time +} + +func (c *deadlineRecordingConn) Read([]byte) (int, error) { return 0, io.EOF } +func (c *deadlineRecordingConn) Write(p []byte) (int, error) { return len(p), nil } +func (c *deadlineRecordingConn) Close() error { return nil } +func (c *deadlineRecordingConn) LocalAddr() net.Addr { return nil } +func (c *deadlineRecordingConn) RemoteAddr() net.Addr { return nil } +func (c *deadlineRecordingConn) SetDeadline(time.Time) error { return nil } +func (c *deadlineRecordingConn) SetWriteDeadline(time.Time) error { return nil } +func (c *deadlineRecordingConn) SetReadDeadline(d time.Time) error { + c.mu.Lock() + c.d = d + c.mu.Unlock() + return nil +} +func (c *deadlineRecordingConn) deadline() time.Time { c.mu.Lock(); defer c.mu.Unlock(); return c.d } + func TestSuggestFix(t *testing.T) { cloudMeta := connectMeta{ - Args: []string{"workflow", "list", "--address", "foo.bar.tmprl.cloud:7233"}, - Address: "foo.bar.tmprl.cloud:7233", + CommandPath: []string{"workflow", "list"}, + Address: "foo.bar.tmprl.cloud:7233", } tests := []struct { name string @@ -294,13 +333,13 @@ func TestSuggestFix(t *testing.T) { name: "mTLS on cloud endpoint", diag: &connectDiagnosis{Cause: causeClientCertRequired}, meta: cloudMeta, - contains: []string{"Temporal Cloud", "--tls-cert-path YourCert.pem", "--tls-key-path YourKey.pem", "temporal config set --prop tls.client_cert_path"}, + contains: []string{"Temporal Cloud", "tls.client_cert_path --value YourCert.pem", "tls.client_key_path --value YourKey.pem"}, }, { name: "mTLS on generic endpoint", diag: &connectDiagnosis{Cause: causeClientCertRequired}, - meta: connectMeta{Args: []string{"workflow", "list"}, Address: "myhost:7233"}, - contains: []string{"requires client certificates", "--tls-cert-path YourCert.pem"}, + meta: connectMeta{CommandPath: []string{"workflow", "list"}, Address: "myhost:7233"}, + contains: []string{"requires client certificates", "tls.client_cert_path --value YourCert.pem"}, }, { name: "refused on local default port", @@ -318,13 +357,13 @@ func TestSuggestFix(t *testing.T) { name: "dns failure from profile address", diag: &connectDiagnosis{Cause: causeDNS}, meta: connectMeta{Address: "typo.example.com:7233", AddressSource: "profile", ProfileName: "prod"}, - contains: []string{`Could not resolve "typo.example.com"`, `profile "prod"`, "temporal config get --prop address"}, + contains: []string{`Could not resolve "typo.example.com"`, `profile "prod"`, "temporal config get --prop address --profile prod"}, }, { name: "server speaks TLS", diag: &connectDiagnosis{Cause: causeServerSpeaksTLS}, - meta: connectMeta{Args: []string{"workflow", "list"}, Address: "myhost:7233"}, - contains: []string{"Add --tls"}, + meta: connectMeta{CommandPath: []string{"workflow", "list"}, Address: "myhost:7233"}, + contains: []string{"requires TLS", "temporal config set --prop tls --value true"}, }, { name: "server plaintext", @@ -336,7 +375,7 @@ func TestSuggestFix(t *testing.T) { name: "api key rejected", diag: &connectDiagnosis{Cause: causeAuth}, meta: connectMeta{Address: "us-west-2.aws.api.temporal.io:7233", HasAPIKey: true}, - contains: []string{"rejected the provided API key"}, + contains: []string{"rejected the configured API key"}, }, { name: "cert file unreadable", @@ -347,7 +386,7 @@ func TestSuggestFix(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := suggestFix(tt.diag, tt.meta) + got := string(renderErrorText(errorReport{Summary: "failure", Action: suggestAction(tt.diag, tt.meta)}, renderOptions{})) for _, want := range tt.contains { assert.Contains(t, got, want) } @@ -355,19 +394,6 @@ func TestSuggestFix(t *testing.T) { } } -func TestReconstructCommand(t *testing.T) { - got := reconstructCommand( - []string{"workflow", "list", "--address", "foo:7233", "--query", "WorkflowType = 'x'"}, - "--tls-cert-path YourCert.pem", - ) - assert.Equal(t, - "temporal workflow list \\\n"+ - " --address foo:7233 \\\n"+ - " --query \"WorkflowType = 'x'\" \\\n"+ - " --tls-cert-path YourCert.pem", - got) -} - func TestConnectErrorRendering(t *testing.T) { origErr := fmt.Errorf("failed reaching server: context deadline exceeded") d := &connectDiagnosis{ @@ -380,16 +406,16 @@ func TestConnectErrorRendering(t *testing.T) { }, } err := newConnectError(d, connectMeta{ - Args: []string{"workflow", "list", "--address", "foo.bar.tmprl.cloud:7233"}, - Address: "foo.bar.tmprl.cloud:7233", + CommandPath: []string{"workflow", "list"}, + Address: "foo.bar.tmprl.cloud:7233", }, origErr) - msg := err.Error() + msg := string(renderErrorText(err.report(), renderOptions{})) assert.Contains(t, msg, "failed connecting to Temporal server at foo.bar.tmprl.cloud:7233: TLS handshake failed: server requires client certificate (mTLS)") assert.Contains(t, msg, "Connecting to foo.bar.tmprl.cloud:7233") assert.Contains(t, msg, "✓ DNS resolved (3 addresses)") assert.Contains(t, msg, "✗ TLS handshake failed") - assert.Contains(t, msg, "--tls-cert-path YourCert.pem") + assert.Contains(t, msg, "tls.client_cert_path --value YourCert.pem") // Unwrap preserves the original error. assert.ErrorContains(t, err.Unwrap(), "context deadline exceeded") } diff --git a/internal/temporalcli/connecterror.go b/internal/temporalcli/connecterror.go index 76866cf0d..5f01086f6 100644 --- a/internal/temporalcli/connecterror.go +++ b/internal/temporalcli/connecterror.go @@ -4,60 +4,57 @@ import ( "fmt" "net" "strings" - - "github.com/fatih/color" ) // connectError is returned by dialClient when connecting to the server fails. -// Error() renders the full multi-line diagnosis (summary, probe stages, and a -// suggested fix); Unwrap() preserves the original dial error for -// errors.Is/As. +// It retains semantic observations for terminal normalization; Error stays a +// concise, uncolored compatibility string and Unwrap preserves the cause. type connectError struct { - rendered string - cause error + diagnosis connectDiagnosis + meta connectMeta + cause error } -func (e *connectError) Error() string { return e.rendered } +func (e *connectError) Error() string { + return fmt.Sprintf("failed connecting to Temporal server at %s: %s", e.meta.Address, connectSummary(&e.diagnosis, e.cause)) +} func (e *connectError) Unwrap() error { return e.cause } -// connectMeta carries the request context the suggestion engine needs to -// propose a concrete fix (e.g. reconstructing the exact command the user -// typed with the missing flags appended). +// connectMeta contains only allowlisted connection provenance. Raw argv and +// credential values must never enter this value. type connectMeta struct { - // Args are the CLI args as typed (without the binary name). - Args []string + // CommandPath is retained only as non-sensitive provenance for callers that + // still populate it. Suggestions never replay the current command. + CommandPath []string Address string AddressSource string // "flag", "profile", or "default" ProfileName string HasAPIKey bool + HasOAuth bool TLSConfigured bool } -// newConnectError renders a connection failure into a connectError. It must -// be called while command execution is active so that color state (JSON mode, -// --color) is applied correctly; the rendering is captured eagerly. func newConnectError(d *connectDiagnosis, meta connectMeta, origErr error) *connectError { - var b strings.Builder - b.WriteString("failed connecting to Temporal server at ") - b.WriteString(meta.Address) - b.WriteString(": ") - b.WriteString(connectSummary(d, origErr)) - if len(d.Stages) > 0 { - b.WriteString("\n\n Connecting to ") - b.WriteString(meta.Address) - for _, stage := range d.Stages { - if stage.Status == diagOK { - b.WriteString("\n " + color.GreenString("✓") + " " + stage.Label) - } else { - b.WriteString("\n " + color.RedString("✗") + " " + stage.Label) - } - } + if meta.Address == "" { + meta.Address = d.Address + } + return &connectError{diagnosis: *d, meta: meta, cause: origErr} +} + +func (e *connectError) report() errorReport { + report := errorReport{ + Summary: e.Error(), + CheckHeading: "Connecting to " + e.meta.Address, + Action: suggestAction(&e.diagnosis, e.meta), } - if suggestion := suggestFix(d, meta); suggestion != "" { - b.WriteString("\n\n") - b.WriteString(indentLines(suggestion, " ")) + for _, stage := range e.diagnosis.Stages { + outcome := checkFailed + if stage.Status == diagOK { + outcome = checkSucceeded + } + report.Checks = append(report.Checks, errorCheck{Outcome: outcome, Message: stage.Label}) } - return &connectError{rendered: b.String(), cause: origErr} + return report } // connectSummary is the one-line cause appended to the first error line. For @@ -72,7 +69,7 @@ func connectSummary(d *connectDiagnosis, origErr error) string { case causeTCPTimeout: return "connection timed out" case causeServerPlaintext: - return "TLS is enabled but the server did not respond with TLS" + return "TLS is enabled but the server did not respond with TLS; check tls configuration" case causeServerSpeaksTLS: return "the server requires TLS but the CLI is connecting without it" case causeClientCertRequired: @@ -84,54 +81,49 @@ func connectSummary(d *connectDiagnosis, origErr error) string { case causeCertFileUnreadable: return fmt.Sprintf("cannot read file %q", d.Detail) case causeAuth: - if d.Detail != "" { - return "authentication failed: " + d.Detail - } return "authentication failed" default: return shortErr(origErr) } } -// suggestFix maps a classified failure to one concrete next step. First match -// wins; an empty return means no suggestion (the stages still render). -func suggestFix(d *connectDiagnosis, meta connectMeta) string { +// suggestAction maps a classified failure to one typed next step. Invocation +// arguments are known literals plus safe command metadata, never raw argv. +func suggestAction(d *connectDiagnosis, meta connectMeta) *displayAction { host, port, _ := net.SplitHostPort(meta.Address) switch d.Cause { case causeCertFileUnreadable: - return fmt.Sprintf("Cannot read %q — check that the path exists and is readable.", d.Detail) + return &displayAction{Label: fmt.Sprintf("Cannot read %q — check that the path exists and is readable.", d.Detail)} case causeClientCertRequired: - var b strings.Builder + message := "The server requires client certificates (mTLS). Configure both certificate paths:" if isCloudHost(host) { - b.WriteString("This looks like a Temporal Cloud endpoint secured with mTLS. Provide client certificates:") - } else { - b.WriteString("The server requires client certificates (mTLS). Provide them:") + message = "This looks like a Temporal Cloud endpoint secured with mTLS. Provide client certificates:" } - b.WriteString("\n\n") - b.WriteString(indentLines(reconstructCommand(meta.Args, "--tls-cert-path YourCert.pem", "--tls-key-path YourKey.pem"), " ")) - b.WriteString("\n\nOr configure once:\n\n") - b.WriteString(" temporal config set --prop tls.client_cert_path YourCert.pem\n") - b.WriteString(" temporal config set --prop tls.client_key_path YourKey.pem") if strings.HasSuffix(host, ".api.temporal.io") { - b.WriteString("\n\nIf your namespace uses API-key auth instead, pass --api-key YourApiKey.") + message += " If the namespace uses API-key auth instead, pass --api-key." } - return b.String() + return configAction(message, meta, + []string{"--prop", "tls.client_cert_path", "--value", "YourCert.pem"}, + []string{"--prop", "tls.client_key_path", "--value", "YourKey.pem"}) case causeAuth: + if meta.HasAPIKey && meta.HasOAuth { + return &displayAction{Label: "The server rejected the configured API key or OAuth credentials. Verify the active credential and namespace gRPC endpoint."} + } if meta.HasAPIKey { - return "The server rejected the provided API key. Verify the key is valid and that the address is your namespace's gRPC endpoint (for Temporal Cloud API keys, a regional endpoint like us-west-2.aws.api.temporal.io:7233)." + return &displayAction{Label: "The server rejected the configured API key. Verify it and the namespace gRPC endpoint."} + } + if meta.HasOAuth { + return &displayAction{Label: "The server rejected the configured OAuth credentials. Verify them and the namespace gRPC endpoint."} } - return "The server rejected the request as unauthenticated. If it requires an API key, pass --api-key; if it requires mTLS, pass --tls-cert-path and --tls-key-path." + return &displayAction{Label: "The server rejected the request as unauthenticated. Configure an API key or mTLS credentials."} case causeServerPlaintext: - return fmt.Sprintf("The server at %s does not appear to use TLS. Remove --tls and certificate flags, or double-check the address and port.", meta.Address) + return &displayAction{Label: fmt.Sprintf("The server at %s does not appear to use TLS. Remove TLS settings or check the address.", meta.Address)} case causeServerSpeaksTLS: - var b strings.Builder - b.WriteString("The server requires TLS. Add --tls:") - b.WriteString("\n\n") - b.WriteString(indentLines(reconstructCommand(meta.Args, "--tls"), " ")) + message := "The server requires TLS. Add --tls:" if isCloudHost(host) { - b.WriteString("\n\nTemporal Cloud endpoints also need client credentials: --tls-cert-path/--tls-key-path or --api-key.") + message += " Temporal Cloud also requires client credentials." } - return b.String() + return configAction(message, meta, []string{"--prop", "tls", "--value", "true"}) case causeDNS: s := fmt.Sprintf("Could not resolve %q — check the server address.", host) if meta.AddressSource == "profile" { @@ -139,22 +131,42 @@ func suggestFix(d *connectDiagnosis, meta connectMeta) string { if profile == "" { profile = "default" } - s += fmt.Sprintf("\nThe address comes from config profile %q — inspect it with:\n\n temporal config get --prop address", profile) + s += fmt.Sprintf(" The address comes from config profile %q.", profile) + return &displayAction{Label: s, Invocations: []displayInvocation{{Command: []string{"temporal", "config", "get"}, Args: appendProfile([]string{"--prop", "address"}, profile)}}} } - return s + return &displayAction{Label: s} case causeTCPRefused: if isLoopbackHost(host) && port == "7233" { - return fmt.Sprintf("No Temporal server is running at %s. Start a local dev server with:\n\n temporal server start-dev", meta.Address) + return &displayAction{Label: fmt.Sprintf("No Temporal server is running at %s. Start a local dev server:", meta.Address), Invocations: []displayInvocation{{Command: []string{"temporal", "server", "start-dev"}}}} } - return fmt.Sprintf("Nothing is listening at %s — verify the address and that the server is running.", meta.Address) + return &displayAction{Label: fmt.Sprintf("Nothing is listening at %s — verify the address and that the server is running.", meta.Address)} case causeCAVerify: - return "The server's TLS certificate is not trusted by your system roots. If the server uses a private CA, pass --tls-ca-path YourServerCA.pem." + return configAction("The server certificate is not trusted. Configure its private CA:", meta, []string{"--prop", "tls.server_ca_cert_path", "--value", "YourServerCA.pem"}) case causeHostnameMismatch: - return fmt.Sprintf("The server's TLS certificate is not valid for %q. If you connect via an IP or alternate name, set --tls-server-name to the name in the certificate.", host) + return &displayAction{Label: fmt.Sprintf("The server certificate is not valid for %q. Set --tls-server-name to a certificate name.", host)} case causeTCPTimeout, causeTimeout: - return fmt.Sprintf("The connection stalled — a firewall or proxy may be blocking traffic to %s. Verify the address, port, and network path. Bound the wait with --client-connect-timeout.", meta.Address) + return &displayAction{Label: fmt.Sprintf("The connection stalled. Verify the address and network path to %s.", meta.Address)} + } + return nil +} + +func configAction(label string, meta connectMeta, propertyArgs ...[]string) *displayAction { + action := &displayAction{Label: label} + for _, args := range propertyArgs { + action.Invocations = append(action.Invocations, displayInvocation{ + Command: []string{"temporal", "config", "set"}, + Args: appendProfile(args, meta.ProfileName), + }) } - return "" + return action +} + +func appendProfile(args []string, profile string) []string { + result := append([]string(nil), args...) + if profile != "" { + result = append(result, "--profile", profile) + } + return result } func isCloudHost(host string) bool { @@ -169,42 +181,6 @@ func isLoopbackHost(host string) bool { return ip != nil && ip.IsLoopback() } -// reconstructCommand re-renders the command the user typed, one option per -// line (matching the style used in help text), with extraFlags appended. -func reconstructCommand(args []string, extraFlags ...string) string { - // Split leading command words from options. - var words, opts []string - for i, arg := range args { - if strings.HasPrefix(arg, "-") { - opts = args[i:] - break - } - words = append(words, arg) - } - var lines []string - lines = append(lines, "temporal "+strings.Join(words, " ")) - for i := 0; i < len(opts); i++ { - line := opts[i] - // Attach the option's value, if the next arg isn't another option. - if !strings.Contains(line, "=") && i+1 < len(opts) && !strings.HasPrefix(opts[i+1], "-") { - i++ - line += " " + quoteArg(opts[i]) - } - lines = append(lines, " "+line) - } - for _, flag := range extraFlags { - lines = append(lines, " "+flag) - } - return strings.Join(lines, " \\\n") -} - -func quoteArg(arg string) string { - if strings.ContainsAny(arg, " \t\"'") { - return fmt.Sprintf("%q", arg) - } - return arg -} - func indentLines(s, indent string) string { lines := strings.Split(s, "\n") for i, line := range lines { diff --git a/internal/temporalcli/terminal.go b/internal/temporalcli/terminal.go new file mode 100644 index 000000000..cb0ecaf4f --- /dev/null +++ b/internal/temporalcli/terminal.go @@ -0,0 +1,276 @@ +package temporalcli + +import ( + "errors" + "fmt" + "io" + "runtime" + "strings" + "unicode" +) + +const defaultFailureExitStatus = 1 + +// Result describes the terminal outcome without exiting the process. The +// original command error remains separate from any failure writing stderr. +type Result struct { + CommandErr error + PresentationErr error + ExitStatus int +} + +type checkOutcome int + +const ( + checkSucceeded checkOutcome = iota + checkFailed +) + +type safeField struct { + Label string + Value string +} + +type errorCheck struct { + Outcome checkOutcome + Message string +} + +type displayInvocation struct { + Command []string + Args []string +} + +type displayAction struct { + Label string + Invocations []displayInvocation +} + +type errorReport struct { + Summary string + Context []safeField + CheckHeading string + Checks []errorCheck + Action *displayAction + Usage string +} + +type renderOptions struct { + Color bool + Shell displayShell +} + +type displayShell int + +const ( + displayShellPOSIX displayShell = iota + displayShellPowerShell +) + +type terminalOptions struct { + Stderr io.Writer + Color bool + KnownSecrets []string + Usage string + DisplayErr error +} + +type commandErrorRecorder struct { + err error +} + +func (r *commandErrorRecorder) Record(err error) { + if r.err == nil { + r.err = err + } + panic(recordedCommandError{}) +} + +func (r *commandErrorRecorder) Err() error { return r.err } + +type recordedCommandError struct{} + +func runRecordedCommand(run func()) (panicked bool) { + defer func() { + if recovered := recover(); recovered != nil { + if _, ok := recovered.(recordedCommandError); !ok { + panic(recovered) + } + panicked = true + } + }() + run() + return false +} + +func handleTerminalError(commandErr error, options terminalOptions) Result { + result := Result{CommandErr: commandErr, ExitStatus: defaultFailureExitStatus} + displayErr := options.DisplayErr + if displayErr == nil { + displayErr = commandErr + } + report := normalizeError(displayErr) + report.Usage = options.Usage + report = redactReport(report, options.KnownSecrets) + shell := displayShellPOSIX + if runtime.GOOS == "windows" { + shell = displayShellPowerShell + } + rendered := renderErrorText(report, renderOptions{Color: options.Color, Shell: shell}) + n, err := options.Stderr.Write(rendered) + if err == nil && n != len(rendered) { + err = io.ErrShortWrite + } + result.PresentationErr = err + return result +} + +func normalizeError(err error) errorReport { + var connectionErr *connectError + if errors.As(err, &connectionErr) { + return connectionErr.report() + } + var activityErr *activityNotFoundError + if errors.As(err, &activityErr) { + return activityErr.report() + } + if err == nil || err.Error() == "" { + return errorReport{Summary: "unknown error"} + } + return errorReport{Summary: err.Error()} +} + +func redactReport(report errorReport, secrets []string) errorReport { + // This is defense in depth for exact values already known to the runtime, + // not a claim that arbitrary legacy error prose is generically sanitized. + // Structured adapters remain responsible for admitting only safe fields. + redact := func(value string) string { + for _, secret := range secrets { + if secret != "" { + value = strings.ReplaceAll(value, secret, "[REDACTED]") + } + } + return value + } + report.Summary = redact(report.Summary) + report.Usage = redact(report.Usage) + for i := range report.Context { + report.Context[i].Value = redact(report.Context[i].Value) + } + for i := range report.Checks { + report.Checks[i].Message = redact(report.Checks[i].Message) + } + if report.Action != nil { + report.Action.Label = redact(report.Action.Label) + for invocationIndex := range report.Action.Invocations { + invocation := &report.Action.Invocations[invocationIndex] + for i := range invocation.Command { + invocation.Command[i] = redact(invocation.Command[i]) + } + for i := range invocation.Args { + invocation.Args[i] = redact(invocation.Args[i]) + } + } + } + return report +} + +// renderErrorText is total over errorReport and performs no I/O or global +// color lookup. Reports are redacted before reaching this function. +func renderErrorText(report errorReport, options renderOptions) []byte { + if report.Summary == "" { + report.Summary = "unknown error" + } + var b strings.Builder + b.WriteString("Error: ") + b.WriteString(report.Summary) + b.WriteByte('\n') + for _, field := range report.Context { + fmt.Fprintf(&b, " %s: %s\n", field.Label, field.Value) + } + if len(report.Checks) > 0 { + heading := report.CheckHeading + if heading == "" { + heading = "Connecting" + } + fmt.Fprintf(&b, "\n %s\n", heading) + for _, check := range report.Checks { + symbol := "✓" + colorCode := "32" + if check.Outcome == checkFailed { + symbol = "✗" + colorCode = "31" + } + if options.Color { + symbol = "\x1b[" + colorCode + "m" + symbol + "\x1b[0m" + } + fmt.Fprintf(&b, " %s %s\n", symbol, check.Message) + } + } + if report.Action != nil { + b.WriteByte('\n') + if report.Action.Label != "" { + b.WriteString(indentLines(report.Action.Label, " ")) + b.WriteByte('\n') + } + for _, invocation := range report.Action.Invocations { + rendered, ok := renderInvocation(invocation, options.Shell) + if !ok { + continue + } + b.WriteByte('\n') + b.WriteString(" ") + b.WriteString(rendered) + b.WriteByte('\n') + } + } + if report.Usage != "" { + b.WriteByte('\n') + b.WriteString(strings.TrimLeft(report.Usage, "\n")) + if !strings.HasSuffix(report.Usage, "\n") { + b.WriteByte('\n') + } + } + return []byte(b.String()) +} + +func renderInvocation(invocation displayInvocation, shell displayShell) (string, bool) { + parts := append([]string(nil), invocation.Command...) + parts = append(parts, invocation.Args...) + if len(parts) == 0 { + return "", false + } + for i := range parts { + if containsControl(parts[i]) { + return "", false + } + if shell == displayShellPowerShell { + parts[i] = quotePowerShell(parts[i]) + } else { + parts[i] = quotePOSIX(parts[i]) + } + } + return strings.Join(parts, " "), true +} + +func containsControl(value string) bool { + return strings.IndexFunc(value, unicode.IsControl) >= 0 +} + +func quotePOSIX(value string) string { + if value != "" && strings.IndexFunc(value, func(r rune) bool { + return !(unicode.IsLetter(r) || unicode.IsDigit(r) || strings.ContainsRune("_@%+=:,./-", r)) + }) < 0 { + return value + } + return "'" + strings.ReplaceAll(value, "'", "'\"'\"'") + "'" +} + +func quotePowerShell(value string) string { + if value != "" && strings.IndexFunc(value, func(r rune) bool { + return !(unicode.IsLetter(r) || unicode.IsDigit(r) || strings.ContainsRune("_@%+=:,./-", r)) + }) < 0 { + return value + } + return "'" + strings.ReplaceAll(value, "'", "''") + "'" +} diff --git a/internal/temporalcli/terminal_test.go b/internal/temporalcli/terminal_test.go new file mode 100644 index 000000000..f9efe71a3 --- /dev/null +++ b/internal/temporalcli/terminal_test.go @@ -0,0 +1,268 @@ +package temporalcli + +import ( + "bytes" + "context" + "errors" + "io" + "os" + "path/filepath" + "testing" + + "github.com/fatih/color" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/temporalio/cli/cliext" + "golang.org/x/oauth2" +) + +func TestHandleTerminalErrorWritesOneRedactedReportAndRetainsBothErrors(t *testing.T) { + commandErr := errors.New("request rejected for token exact-secret") + writeErr := errors.New("stderr closed") + writer := &countingErrorWriter{err: writeErr} + + result := handleTerminalError(commandErr, terminalOptions{ + Stderr: writer, + KnownSecrets: []string{"exact-secret"}, + }) + + assert.Equal(t, 1, result.ExitStatus) + assert.ErrorIs(t, result.CommandErr, commandErr) + assert.ErrorIs(t, result.PresentationErr, writeErr) + assert.Equal(t, 1, writer.writes) + assert.Contains(t, string(writer.last), "Error: request rejected for token [REDACTED]") + assert.NotContains(t, string(writer.last), "exact-secret") +} + +func TestRenderErrorTextIsTotalAndUsesExplicitColorPolicy(t *testing.T) { + report := errorReport{ + Summary: "connection refused", + Checks: []errorCheck{{ + Outcome: checkFailed, + Message: "TCP connection refused", + }}, + } + + plain := string(renderErrorText(report, renderOptions{Color: false})) + colored := string(renderErrorText(report, renderOptions{Color: true})) + + assert.Equal(t, "Error: connection refused\n\n Connecting\n ✗ TCP connection refused\n", plain) + assert.NotContains(t, plain, "\x1b[") + assert.Contains(t, colored, "\x1b[") + assert.Equal(t, []byte("Error: unknown error\n"), renderErrorText(errorReport{}, renderOptions{})) +} + +func TestConnectErrorCarriesSemanticFactsWithoutRenderedStateOrRawArguments(t *testing.T) { + cause := errors.New("dial failed") + diagnosis := &connectDiagnosis{ + Address: "127.0.0.1:7233", + Cause: causeTCPRefused, + Stages: []diagStage{{Status: diagFail, Label: "TCP connection refused"}}, + } + err := newConnectError(diagnosis, connectMeta{Address: diagnosis.Address}, cause) + + assert.Equal(t, "failed connecting to Temporal server at 127.0.0.1:7233: connection refused", err.Error()) + assert.ErrorIs(t, err, cause) + report := normalizeError(err) + require.NotNil(t, report.Action) + require.Len(t, report.Action.Invocations, 1) + assert.Equal(t, []string{"temporal", "server", "start-dev"}, report.Action.Invocations[0].Command) + assert.NotContains(t, err.Error(), "\n") +} + +func TestRecorderSeamCapturesFirstLeafFailureWithoutChangingItsIdentity(t *testing.T) { + // Seam decision: generated Run callbacks already call Fail as their final + // operation. A command-scoped recorder therefore captures every leaf error + // without changing generated Run to RunE. RunE was rejected because Cobra + // skips post-run cleanup when RunE returns an error. + want := errors.New("leaf failure") + var recorder commandErrorRecorder + + assert.True(t, runRecordedCommand(func() { recorder.Record(want) })) + + assert.ErrorIs(t, recorder.Err(), want) +} + +func TestRecorderSeamStopsExecutionAfterFailureAndRunsCleanup(t *testing.T) { + var recorder commandErrorRecorder + workedAfterFailure := false + cleanupRan := false + func() { + defer func() { cleanupRan = true }() + assert.True(t, runRecordedCommand(func() { + recorder.Record(errors.New("stop here")) + workedAfterFailure = true + })) + }() + assert.False(t, workedAfterFailure) + assert.True(t, cleanupRan) +} + +func TestRenderInvocationUsesExplicitShellQuotingAndRejectsControls(t *testing.T) { + invocation := displayInvocation{Command: []string{"temporal", "config", "set"}, Args: []string{"--value", "space and 'quote'", "--profile", "-prod"}} + posix, ok := renderInvocation(invocation, displayShellPOSIX) + require.True(t, ok) + assert.Contains(t, posix, `'space and '"'"'quote'"'"''`) + assert.Contains(t, posix, "--profile -prod") + powerShell, ok := renderInvocation(invocation, displayShellPowerShell) + require.True(t, ok) + assert.Contains(t, powerShell, `'space and ''quote'''`) + _, ok = renderInvocation(displayInvocation{Command: []string{"temporal"}, Args: []string{"unsafe\nvalue"}}, displayShellPOSIX) + assert.False(t, ok) +} + +func TestFinishCommandPreservesOriginalErrorWhilePresentingCancellationCause(t *testing.T) { + original := errors.New("original command failure") + ctx, cancel := context.WithCancelCause(t.Context()) + cancel(errors.New("command timed out after 2s")) + var stderr bytes.Buffer + cctx := &CommandContext{Context: ctx, Options: CommandOptions{IOStreams: IOStreams{Stderr: &stderr}, EnvLookup: testEnvLookup{}}} + result := finishCommand(cctx, original, "") + assert.ErrorIs(t, result.CommandErr, original) + assert.Contains(t, stderr.String(), "command timed out after 2s") + assert.NotContains(t, stderr.String(), original.Error()) +} + +func TestRecorderSeamGeneratedLeavesStopAfterRecording(t *testing.T) { + source, err := os.ReadFile("commands.gen.go") + require.NoError(t, err) + allCalls := bytes.Count(source, []byte("cctx.Options.Fail(err)")) + terminalCalls := bytes.Count(source, []byte("cctx.Options.Fail(err)\n\t\t}")) + + assert.Greater(t, allCalls, 100, "expected to inspect every generated command leaf") + assert.Equal(t, allCalls, terminalCalls, "every generated Fail call must be the final operation in its branch") +} + +func TestExecuteRestoresColorAcrossFailureSources(t *testing.T) { + old := color.NoColor + t.Cleanup(func() { color.NoColor = old }) + + for name, args := range map[string][]string{ + "argument": {"workflow", "describe", "--color", "always"}, + "pre-run": {"workflow", "list", "--time-format", "invalid", "--color", "always"}, + "runtime": {"config", "get", "--disable-config-file", "--disable-config-env", "--color", "always"}, + } { + t.Run(name, func(t *testing.T) { + color.NoColor = true + var stdout, stderr bytes.Buffer + result := Execute(t.Context(), CommandOptions{ + Args: args, + IOStreams: IOStreams{Stdout: &stdout, Stderr: &stderr}, + DeprecatedEnvConfig: DeprecatedEnvConfig{DisableEnvConfig: true}, + }) + assert.Error(t, result.CommandErr) + assert.True(t, color.NoColor, "global color policy must be restored") + }) + } +} + +func TestEarlyJSONPolicyDisablesTerminalColor(t *testing.T) { + cctx := &CommandContext{Options: CommandOptions{Args: []string{"workflow", "describe", "--output=jsonl", "--color=always"}}} + assert.False(t, cctx.terminalColorEnabled()) +} + +func TestUnknownFlagWithFollowingValueIsNotMisclassifiedAsUnknownCommand(t *testing.T) { + var stdout, stderr bytes.Buffer + result := Execute(t.Context(), CommandOptions{ + Args: []string{"--definitely-invalid", "value"}, + IOStreams: IOStreams{Stdout: &stdout, Stderr: &stderr}, + DeprecatedEnvConfig: DeprecatedEnvConfig{DisableEnvConfig: true}, + }) + require.Error(t, result.CommandErr) + assert.Contains(t, result.CommandErr.Error(), "unknown flag: --definitely-invalid") + assert.NotContains(t, result.CommandErr.Error(), "unknown command") +} + +func TestKnownSecretsExtractsAvailableScalarSliceHeaderAndEnvironmentValues(t *testing.T) { + cmd := &cobra.Command{} + cmd.Flags().String("api-key", "", "") + cmd.Flags().StringArray("grpc-meta", nil, "") + require.NoError(t, cmd.Flags().Parse([]string{"--api-key", "flag-secret", "--grpc-meta", "Authorization=header-secret"})) + cctx := &CommandContext{ + CurrentCommand: cmd, + Options: CommandOptions{EnvLookup: testEnvLookup{ + "TEMPORAL_CODEC_AUTH": "env-secret", + }}, + } + + secrets := cctx.knownSecrets() + assert.Contains(t, secrets, "flag-secret") + assert.Contains(t, secrets, "Authorization=header-secret") + assert.Contains(t, secrets, "header-secret") + assert.Contains(t, secrets, "env-secret") +} + +func TestKnownSecretsIgnoresDisabledConfigEnvironment(t *testing.T) { + cctx := &CommandContext{Options: CommandOptions{ + Args: []string{"--disable-config-env"}, + EnvLookup: testEnvLookup{"TEMPORAL_API_KEY": "disabled-env-secret"}, + }} + assert.NotContains(t, cctx.knownSecrets(), "disabled-env-secret") +} + +func TestCaptureEffectiveConnectionSecretsIncludesProfileValues(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "temporal.toml") + require.NoError(t, os.WriteFile(configPath, []byte(`[profile.prod] +api_key = "profile-api-secret" +codec = { endpoint = "https://codec.example", auth = "profile-codec-secret" } +grpc_meta = { authorization = "profile-header-secret" } +tls = { client_cert_data = "profile-cert-secret", client_key_data = "profile-key-secret", server_ca_cert_data = "profile-ca-secret" } +`), 0o600)) + cctx := &CommandContext{ + Options: CommandOptions{EnvLookup: testEnvLookup{}}, + RootCommand: &TemporalCommand{CommonOptions: cliext.CommonOptions{ + ConfigFile: configPath, Profile: "prod", DisableConfigEnv: true, + }}, + } + captureEffectiveConnectionSecrets(cctx, &cliext.ClientOptions{}) + for _, secret := range []string{"profile-api-secret", "profile-codec-secret", "profile-header-secret", "profile-cert-secret", "profile-key-secret", "profile-ca-secret"} { + assert.Contains(t, cctx.knownSecrets(), secret) + } + assert.True(t, cctx.hasAPIKey) +} + +func TestCaptureEffectiveConnectionSecretsIncludesOAuthValues(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "temporal.toml") + require.NoError(t, cliext.StoreClientOAuth(cliext.StoreClientOAuthOptions{ + ConfigFilePath: configPath, + ProfileName: "prod", + OAuth: &cliext.OAuthConfig{ + ClientConfig: &oauth2.Config{ClientID: "client", ClientSecret: "oauth-client-secret"}, + Token: &oauth2.Token{AccessToken: "oauth-access-secret", RefreshToken: "oauth-refresh-secret"}, + }, + })) + cctx := &CommandContext{ + Options: CommandOptions{EnvLookup: testEnvLookup{}}, + RootCommand: &TemporalCommand{CommonOptions: cliext.CommonOptions{ + ConfigFile: configPath, Profile: "prod", DisableConfigEnv: true, + }}, + } + captureEffectiveConnectionSecrets(cctx, &cliext.ClientOptions{}) + assert.ElementsMatch(t, []string{"oauth-client-secret", "oauth-access-secret", "oauth-refresh-secret"}, cctx.knownSecrets()) + assert.True(t, cctx.hasOAuth) + assert.False(t, cctx.hasAPIKey) +} + +type testEnvLookup map[string]string + +func (e testEnvLookup) LookupEnv(name string) (string, bool) { + value, ok := e[name] + return value, ok +} + +func (e testEnvLookup) Environ() []string { return nil } + +type countingErrorWriter struct { + err error + writes int + last []byte +} + +func (w *countingErrorWriter) Write(p []byte) (int, error) { + w.writes++ + w.last = append([]byte(nil), p...) + return 0, w.err +} + +var _ io.Writer = (*countingErrorWriter)(nil) From 880f6b9c418783775c9ee3230aa7f9bcd7e947ea Mon Sep 17 00:00:00 2001 From: Ross Nelson Date: Wed, 22 Jul 2026 17:15:08 -0400 Subject: [PATCH 5/5] Document structured error handling transition --- CONTRIBUTING.md | 10 + docs/structured-error-handling.md | 304 ++++++++++++++++++++++++++++++ 2 files changed, 314 insertions(+) create mode 100644 docs/structured-error-handling.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9eeea5b26..8170c4ed7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -12,6 +12,10 @@ Uses normal `go test`, e.g.: go test ./... +The root command does not enter the nested `cliext` module. Run `(cd cliext && go test ./...)` separately. + +The current transition branch has a recorded standalone `cliext` compile mismatch. Until that mismatch is fixed, the nested command must match the baseline exception. Do not report it as green. + See other tests for how to leverage things like the command harness and dev server suite. Example to run a single test case: @@ -20,11 +24,17 @@ Example to run a single test case: ## Adding/updating commands +Follow [Structured Error Handling](docs/structured-error-handling.md) when a command adds or changes terminal failure behavior. + First, update [commands.yaml](internal/temporalcli/commands.yaml) following the rules in that file. Then to regenerate the [commands.gen.go](internal/temporalcli/commands.gen.go) file from code, run: go run ./cmd/gen-commands -input internal/temporalcli/commands.yaml -pkg temporalcli -context "*CommandContext" > internal/temporalcli/commands.gen.go +Generated commands currently inherit the existing `cctx.Options.Fail(err)` adapter. Regeneration may retain those calls until Phase A of [Structured Error Handling](docs/structured-error-handling.md#transition-gates) changes the generator. + +Do not add handwritten or direct `Fail` calls. Do not create a new generator pattern that emits them or copy the adapter into new code. Phase A removes generated `Fail` calls entirely. + This will expect every non-parent command to have a `run` method, so for new commands developers will have to implement `run` on the new command in a separate file before it will compile. diff --git a/docs/structured-error-handling.md b/docs/structured-error-handling.md new file mode 100644 index 000000000..873e97cdf --- /dev/null +++ b/docs/structured-error-handling.md @@ -0,0 +1,304 @@ +# Structured Error Handling + +PR [#1114](https://github.com/temporalio/cli/pull/1114) proves a useful error-handling direction, but transition work remains before it can become the repository standard. + +This guide defines the pattern that command families may adopt and the work needed to complete that transition. + +- **Last updated:** 2026-07-22 +- **Status:** transition architecture +- **Verdict:** PR [#1114](https://github.com/temporalio/cli/pull/1114) is a reference slice, not a finished general framework + +## What to adopt now + +Command and domain code should keep returning ordinary wrapped Go errors. The terminal boundary should convert those errors into safe display facts. It should also own one stderr render and the final status. + +Connection failures and Standalone Activity `NotFound` prove parts of this direction. However, the current branch still contains transition mechanisms that new code must not copy. + +- Adopt native wrapped errors, semantic domain facts, explicit terminal adapters, typed token actions, bounded diagnosis, and conservative fallback now. +- Do not copy the panic-based `Fail` recorder, duplicate config resolution, global color mutation, shallow redaction, family-owned report methods, or generic helper coupling. +- Do not add a provider interface yet. Keep explicit terminal adapters until three genuinely different families show a useful shared boundary. +- Do not roll the pattern across more command families until phases A through C in [Transition gates](#transition-gates) pass. + +Key files: + +- [`internal/temporalcli/terminal.go`](../internal/temporalcli/terminal.go) contains the transition report, renderer, recorder, and `Result`. +- [`internal/temporalcli/commands.go`](../internal/temporalcli/commands.go) owns command execution and final terminal handling. +- [`internal/temporalcli/connecterror.go`](../internal/temporalcli/connecterror.go) carries connection facts and actions in the reference slice. +- [`internal/temporalcli/connectdiag.go`](../internal/temporalcli/connectdiag.go) performs connection-specific diagnosis. +- [`internal/temporalcli/client.go`](../internal/temporalcli/client.go) resolves connection options and starts diagnosis after a failed dial. + +## Current flow is useful but transitional + +```mermaid +flowchart LR + A[Generated command callback] -->|calls Fail| B[Panic recorder] + B -->|native error| C[Terminal normalizer] + D[Failed Temporal dial] -->|DNS TCP TLS facts| E[Connection error] + E --> C + F[Activity polling] -->|typed not found error| C + C -->|mutable report| G[Exact-value redaction] + G -->|explicit render options| H[Text renderer] + H -->|one checked write| I[stderr] + C -->|Result and status| J[main] +``` + +The terminal boundary already returns a [`Result`](../internal/temporalcli/terminal.go). It separates `CommandErr` from `PresentationErr`. Started extensions keep their own output and child status. + +The renderer accepts explicit color and shell settings. However, command execution still mutates the package-level `color.NoColor` value. Concurrent `Execute` calls are therefore unsupported. + +Connection diagnosis is family-specific and runs only after a failure. It starts after the real dial fails and uses the command context. A three-second cap bounds its work. Set `TEMPORAL_CLI_DISABLE_CONNECT_DIAGNOSIS` to disable it. + +## How terminal failures flow + +### Reporting a built-in command failure + +**Trigger:** A built-in command reaches its final error. + +1. **Preserve the cause**: Command code returns the error or adds lowercase operation context with `%w`. +2. **Carry semantic facts**: A domain error stores stable facts that callers may inspect. It does not store color, indentation, or command text. +3. **Adapt at the terminal**: An explicit family adapter converts supported facts into controlled display fields. +4. **Render once**: The current terminal boundary exact-redacts registered secret values and renders one byte slice. It then attempts one checked stderr write. Phase D adds deep-copy and control sanitization for every display field. +5. **Return ownership**: `Result.CommandErr` keeps the error chain, `Result.PresentationErr` keeps a write failure, and `ExitStatus` carries the status. + +**When no adapter matches:** Preserve concise legacy text and omit context, checks, and actions that have not passed a family security review. + +### Diagnosing a failed connection + +**Trigger:** The real Temporal client dial fails while the command context is still active. + +1. **Check eligibility**: Skip diagnosis after cancellation or when the kill switch is set. +2. **Apply a family budget**: Bound the connection probe by the command deadline and a three-second maximum. +3. **Collect observations**: Run DNS, TCP, and TLS checks against the configured target. +4. **Avoid privileged retries**: Do not send an API key, OAuth credential, or Temporal RPC. A TLS handshake may present configured client certificates. +5. **Return facts**: Preserve the original dial error and attach only supported observations. + +**When diagnosis is unavailable:** Current behavior preserves the original error. The current model has no `unknown` or `skipped` observation state. Deadline classification also remains a known accuracy gap. Phase E adds those states and the related accuracy tests. + +A later probe must never be presented as proof of the original failure. + +### Passing through a started extension failure + +**Trigger:** A `temporal-*` extension starts and exits nonzero. + +1. **Keep child streams**: The extension owns its stdout and stderr. +2. **Return the child status**: The parent returns `ExtensionNonZeroExit` with the child exit code. +3. **Skip parent rendering**: The parent does not add another error report. + +**When the child never starts:** The parent owns the failure and uses the normal terminal path. + +## Ownership boundary for new code + +### Copy and adopt now + +- Return native Go errors through the call graph. +- Add context with `%w` so `errors.Is` and `errors.As` keep working. +- Put stable semantic facts and the wrapped cause in a small domain error when callers need typed behavior. +- Add an explicit terminal-owned adapter for each admitted family. +- Render actions from typed command and argument tokens. Use long flags in displayed commands. +- Keep diagnosis beside the failing operation, with a family budget, cancellation, documented effects, and a rollback control. +- Preserve a conservative unknown fallback. +- Write parent-owned terminal output once to stderr and return status explicitly. + +### Do not copy from the transition slice + +- Do not add handwritten or direct callers to the panic-based `CommandOptions.Fail` recorder. Do not add a new generator pattern that emits them. Existing generated commands temporarily inherit this adapter. Phase A changes the generator and removes generated `Fail` calls entirely. +- Do not resolve configuration or collect secrets a second time for display. Build one canonical resolved snapshot during normal option resolution. +- Do not read or mutate `color.NoColor` in new terminal code. Color must be command-local before concurrent execution is supported. +- Do not mutate a shared report while redacting it. Deep-copy controlled display data before sanitization. +- Do not add speculative fields. Remove fields that have no rendered or tested use. +- Do not make domain errors render their own reports. Keep family adapters at the terminal boundary. +- Do not couple families through generic report helpers or add a generic diagnosis framework. + +## How to add an error family + +A family is eligible only when it has a recurring user problem and stable typed evidence. + +1. **Name the owner and scope**: Identify the command family, the failure source, and any partial stdout behavior. +2. **Preserve the native chain**: Add operation context with `%w`; use a small domain error only for stable semantic facts. +3. **Define the fallback**: State the concise message and behavior when classification does not match. +4. **Add one terminal adapter**: Match typed errors or exported status codes before text. Copy only audited facts. +5. **Add an action only when safe**: Use typed tokens, long flags, canonical provenance, and no raw argv. +6. **Bound effects**: If the family diagnoses, specify its deadline, cancellation, external traffic, credentials, and kill switch or rollback. +7. **Prove compatibility**: Test `errors.Is` or `errors.As`, stderr and stdout, status, redaction, control characters, cancellation, and fallback. +8. **Pass the current phase gates**: Broader rollout is blocked until phases A through C pass. Project-standard status is blocked until phases D and E pass. + +## Safe Go examples + +Error text starts with lowercase and has no trailing punctuation. Wrap the cause with `%w`. + +```go +return fmt.Errorf("describe namespace %q: %w", namespace, err) +``` + +A domain error carries facts and a cause, not display layout. + +```go +type namespaceNotFoundError struct { + namespace string + cause error +} + +func (e *namespaceNotFoundError) Error() string { + return fmt.Sprintf("namespace not found: %s", e.namespace) +} + +func (e *namespaceNotFoundError) Unwrap() error { + return e.cause +} +``` + +The terminal adapter owns the user-facing summary. Keep it lowercase and free of punctuation. + +```go +func adaptNamespaceNotFound(err *namespaceNotFoundError) errorReport { + return errorReport{Summary: "namespace not found"} +} +``` + +Suggested commands use typed tokens and long flags. + +```go +displayInvocation{ + Command: []string{"temporal", "namespace", "describe"}, + Args: []string{"--namespace", namespace}, +} +``` + +## Security rules for displayed data + +Structured display data is a security boundary, not a copy of the error object. + +- Admit only fields that the family has classified and reviewed. +- Never replay raw argv. It may contain API keys, codec credentials, headers, or shell control characters. +- Build provenance from the canonical effective configuration. Record presence booleans separately for API key and OAuth. +- Never place API keys, authorization metadata, OAuth tokens, client secrets, certificate or key bytes, arbitrary URLs, or environment snapshots in a report or action. +- Reject NUL, newline, and every other control character before rendering any display field or invocation token. +- Quote each invocation token for the selected display shell. Producers supply unquoted tokens. +- Treat exact known-secret replacement as a final check, not a general scrubber. It cannot remove an unknown secret from legacy prose. +- Deep-copy the report before sanitization so rendering cannot alter domain state or another consumer's value. + +The current implementation rejects control characters in invocation tokens. It does not yet enforce the same rule across every summary, context value, check, action label, and usage field. That gap blocks project-standard status. + +## Diagnostics remain family-specific + +Diagnosis belongs beside the operation it examines. The terminal renderer must not run network, filesystem, or environment checks. + +For connection failures: + +- Start only after the real dial fails and while the command context remains active. +- Derive the diagnosis context from the command context, not the expired dial context. +- Use the earlier command deadline or a three-second family cap. +- Stop immediately on cancellation. +- Limit probes to documented DNS, TCP, and TLS activity against the configured target. +- Permit configured client certificates during TLS. Never send API-key or OAuth credentials or perform a Temporal RPC. +- Keep `TEMPORAL_CLI_DISABLE_CONNECT_DIAGNOSIS` as the rollback control. +- Describe probe effects because they may add DNS traffic, connections, TLS handshakes, and server log entries during an outage. + +The current model records only successful and failed stages. Phase E targets `unknown` and `skipped` observations. It also targets accurate typed deadline handling rather than describing current behavior. + +Do not create a generic diagnoser interface. Reconsider shared machinery only after three different families need materially different diagnosis behavior. + +## Output and exit contract + +| Outcome | stdout | stderr | status | +|---|---|---|---:| +| success or explicit help | successful data or help | empty unless a documented warning applies | `0` | +| parent-owned built-in failure | unchanged successful or documented partial data | one human report | `1` | +| started extension nonzero exit | child-owned | child-owned | child status | +| parent failure before extension start | empty unless already documented | one parent report | `1` | +| stderr write failure | unchanged | one attempted write | intended status | + +`PresentationErr` records a report write failure without replacing `CommandErr`. The binary currently ignores `PresentationErr` after `Execute` returns. Explicit ownership and observability therefore remain open transition work. + +The `-o json` and `-o jsonl` flags govern stdout data. They do not create a machine-readable stderr error contract. Parent-rendered errors remain human text. They must contain no ANSI when JSON output is requested. + +Usage belongs only to input failures for which the terminal can identify the affected command. Those failures render one error and one usage block on stderr. Pre-run, runtime, connection, and other operational failures do not add usage. + +## Current limitations block broad adoption + +- `Execute` is single-flight because command setup mutates `color.NoColor`. +- The panic recorder depends on every generated `Fail` call remaining terminal and synchronous. +- Connection display reconstructs provenance and known secrets instead of consuming one canonical resolved snapshot. +- Redaction mutates shallow report data and exact replacement does not make arbitrary fallback prose safe. +- Control-character rejection is complete for invocation tokens only. +- Authentication, authorization, and deadline diagnosis need stronger typed evidence and separate accuracy tests. +- Automatic connection probes can add pressure and server log noise during an outage. +- `PresentationErr` has no explicit process-level reporting or telemetry owner. +- Some report fields and helper couplings belong to the transition slice rather than the durable pattern. +- Direct `os.Exit` calls in successful-output broken-pipe handling remain outside this terminal-failure boundary. + +## Transition gates + +| Phase | Owner | Required result | Measurable exit criteria | +|---|---|---|---| +| A: isolate and retire recorder | command runtime and generator owners | Built-in failures return through normal control flow | No handwritten or direct `Fail` callers and no new generator pattern emits them; the changed generator removes generated `Fail` calls entirely; sentinel cleanup tests pass; panic recorder removed | +| B: canonical config and redaction snapshot | client config and security owners | One effective snapshot supplies provenance and known secrets | Flag, environment, profile, disabled-source, API-key, OAuth, header, and TLS precedence tests pass; duplicate resolution removed | +| C: command-local color and concurrency | command runtime owner | `Execute` no longer reads or writes global color state | Parallel conflicting-color `Execute` test passes under `go test -race`; single-flight restriction removed | +| D: terminal ownership and sanitization | terminal and security owners | Terminal adapters own conversion and sanitize deep copies | Family `report()` methods and unused fields removed; all display fields reject controls; mutation-isolation tests pass; `PresentationErr` owner documented | +| E: diagnosis correctness and operations | connection and resilience owners | Connection diagnosis is accurate and operationally bounded | Authn, authz, deadline, cancellation, wall-clock, no-credential, kill-switch, and outage-pressure tests pass; effects documented | +| F: third-family checkpoint | architecture owner and three family owners | Evidence decides whether extraction helps | Three genuinely different families are implemented; coupling is measured; keep explicit adapters unless a package or interface reduces proven duplication | + +Broader family rollout is blocked on A through C. Declaring this the project standard is blocked on D and E. A provider interface or package extraction is reconsidered only at F. + +## Validation slices and rollout + +PR [#1114](https://github.com/temporalio/cli/pull/1114) is the connection transition slice. Copy its staged user experience, bounded family diagnosis, cause preservation, and typed action direction. Do not copy its transition implementation as a general framework. + +Standalone Activity `NotFound` is the second validation slice. It proves that a typed server error can retain its `errors.As` chain. The terminal can also add conservative text without a speculative action. Two slices are not enough to justify an interface. + +Rollout order: + +1. Complete phases A through C before admitting more families. +2. Use connection and Activity tests to protect the two validation slices during the transition. +3. Complete phases D and E before publishing this pattern as the repository standard. +4. Admit one genuinely different third family with its own owner and compatibility matrix. +5. Run phase F and keep explicit adapters unless the three-family evidence supports extraction. + +## Where the transition lives + +### Core contracts + +| Contract | Location | Current role | +|---|---|---| +| `Result` and renderer | [`internal/temporalcli/terminal.go`](../internal/temporalcli/terminal.go) | Separates command, presentation, and status outcomes | +| command runtime | [`internal/temporalcli/commands.go`](../internal/temporalcli/commands.go) | Owns Cobra execution, usage, terminal handling, and transition recorder | +| process exit | [`cmd/temporal/main.go`](../cmd/temporal/main.go) | Converts `Result.ExitStatus` into process status | +| extension boundary | [`internal/temporalcli/commands.extension.go`](../internal/temporalcli/commands.extension.go) | Preserves child streams and child status | +| connection facts and actions | [`internal/temporalcli/connecterror.go`](../internal/temporalcli/connecterror.go) | Reference family adapter, with transition coupling still present | +| connection probes | [`internal/temporalcli/connectdiag.go`](../internal/temporalcli/connectdiag.go) | Three-second DNS, TCP, and TLS diagnosis | +| effective client setup | [`internal/temporalcli/client.go`](../internal/temporalcli/client.go) | Dial boundary and current duplicate display provenance resolution | + +### Test anchors + +- [`internal/temporalcli/terminal_test.go`](../internal/temporalcli/terminal_test.go) covers one-write behavior, error identity, shell quoting, redaction, color restoration, and recorder assumptions. +- [`internal/temporalcli/connectdiag_test.go`](../internal/temporalcli/connectdiag_test.go) covers transport classification, deadline interruption, actions, and rendered checks. +- [`internal/temporalcli/client_test.go`](../internal/temporalcli/client_test.go) covers dial timeouts, kill-switch behavior, JSON color, profile provenance, and command timeout. +- [`internal/temporalcli/commands.extension_test.go`](../internal/temporalcli/commands.extension_test.go) covers extension streams, status, timeout, and cancellation. +- [`internal/temporalcli/commands.activity_internal_test.go`](../internal/temporalcli/commands.activity_internal_test.go) covers Activity `NotFound` cause preservation. + +### Verification commands + +```sh +# This root command does not enter the nested cliext module. +go test ./... +# The current branch has a known standalone cliext compile mismatch. Until it is fixed, +# this command must match the recorded baseline exception and must not be called green. +(cd cliext && go test ./...) +# This currently exposes the documented global-color race and is expected to fail +# until Phase C. It becomes a required green gate in Phase C. +go test -race ./internal/temporalcli +make gen +git diff --exit-code +make gen-docs +``` + +Run focused transition tests while changing the terminal path: + +```sh +go test ./internal/temporalcli -run 'Test(HandleTerminalError|RenderErrorText|RecorderSeam|RenderInvocation|FinishCommand|KnownSecrets|CaptureEffective|DiagnoseConnection|ArmReadDeadline|ConnectDiagnosis_|Extension_|ActivityNotFound)' -count=1 +``` + +## Where to learn more + +- [`CONTRIBUTING.md`](../CONTRIBUTING.md) describes repository build, test, generation, and command contribution workflows. +- [PR #1114](https://github.com/temporalio/cli/pull/1114) is the reference connection transition slice.