Ensure errors don't cause silent failure, make authN failure messages friendlier (#89)

* Ensure errors don't cause silent failure, make authN failure messages friendlier

* Make CommandHarness accessible in non-integration tests

Author: jeri-temporal

temporalcloudcli/cloud.go

@@ -74,27 +74,42 @@ func (b *CloudOptionsBuilder) Build(ctx context.Context) (*CloudOptions, error)
 	return cloudOpts, nil
 }
 
+type (
+	errSlot    struct{ err error }
+	errSlotKey struct{}
+)
+
+// errorContext will add err to the errSlot on ctx if it has one before returning v and err
+func errorContext[T any](ctx context.Context, v T, err error) (T, error) {
+	if ctx != nil {
+		if slot, ok := ctx.Value(errSlotKey{}).(*errSlot); ok {
+			slot.err = err
+		}
+	}
+	return v, err
+}
+
 func (c *CloudOptions) GetAPIKey(ctx context.Context) (string, error) {
 	loadClientOauthRes, err := cliext.LoadClientOAuth(cliext.LoadClientOAuthOptions{
 		ConfigFilePath: c.ConfigFile,
 		ProfileName:    c.Profile,
 		EnvLookup:      envconfig.EnvLookupOS,
 	})
 	if err != nil {
-		return "", fmt.Errorf("failed to load login configuration: %w, please run `temporal cloud login --reset`", err)
+		return errorContext(ctx, "", NewFriendlyError("failed to load login configuration, please run `temporal cloud login --reset`", err))
 	}
 
 	// check if we have had a valid token in the past
 	if loadClientOauthRes.OAuth == nil || loadClientOauthRes.OAuth.ClientConfig == nil {
-		return "", fmt.Errorf("no login session found, please run `temporal cloud login`")
+		return errorContext(ctx, "", NewFriendlyError("no login session found, please run `temporal cloud login`", nil))
 	}
 
 	token, refreshed, err := GetToken(ctx, loadClientOauthRes.OAuth.ClientConfig, loadClientOauthRes.OAuth.Token)
 	if err != nil {
 		if errors.Is(err, ErrLoginRequired) {
-			return "", fmt.Errorf("login session expired, please run `temporal cloud login`: %w", err)
+			return errorContext(ctx, "", NewFriendlyError("login session expired, please run `temporal cloud login`", err))
 		}
-		return "", fmt.Errorf("failed to get access token: %w", err)
+		return errorContext(ctx, "", NewFriendlyErrorf("failed to get access token: %v", err))
 	}
 	if refreshed {
 		loadClientOauthRes.OAuth.Token = token
@@ -104,7 +119,7 @@ func (c *CloudOptions) GetAPIKey(ctx context.Context) (string, error) {
 			ProfileName:    c.Profile,
 			EnvLookup:      envconfig.EnvLookupOS,
 		}); err != nil {
-			return "", fmt.Errorf("failed to write config file: %w", err)
+			return errorContext(ctx, "", NewFriendlyErrorf("failed to write config file: %v", err))
 		}
 	}
 	return token.AccessToken, nil

temporalcloudcli/commands.go

@@ -4,6 +4,7 @@ import (
 	"bufio"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"log/slog"
@@ -185,6 +186,7 @@ func (cctx *CommandContext) BuildCloudClient(clientOpts ClientOptions) (*cloudcl
 
 	opts.GRPCDialOptions = []grpc.DialOption{
 		grpc.WithChainUnaryInterceptor(
+			grabOriginalErrorInterceptor,
 			ClearDeprecatedFieldsInterceptor,
 		),
 	}
@@ -196,6 +198,28 @@ func (cctx *CommandContext) BuildCloudClient(clientOpts ClientOptions) (*cloudcl
 	return cloudClient, nil
 }
 
+// grabOriginalErrorInterceptor is a gRPC unary client interceptor that adds
+// a "slot" for error storage to the context passed to invoker. If an error occurs
+// and a downstream function has stored an error in errSlot, the two will be grafted
+// together. This allows for upstream error handling to perform checks with errors.Is
+// or errors.As, since grpc itself doesn't wrap errors.
+func grabOriginalErrorInterceptor(
+	ctx context.Context,
+	method string,
+	req, reply any,
+	cc *grpc.ClientConn,
+	invoker grpc.UnaryInvoker,
+	opts ...grpc.CallOption,
+) error {
+	slot := &errSlot{}
+	ctx = context.WithValue(ctx, errSlotKey{}, slot)
+	err := invoker(ctx, method, req, reply, cc, opts...)
+	if err != nil {
+		return GraftErrors(err, slot.err)
+	}
+	return nil
+}
+
 // clearDeprecatedFieldsInterceptor is a gRPC unary client interceptor that strips
 // deprecated fields from every response. Any proto field whose name ends with
 // "_deprecated" or that is marked with [deprecated = true] is cleared; nested
@@ -244,10 +268,23 @@ func (c *CommandContext) preprocessOptions() error {
 			if c.Err() != nil {
 				err = fmt.Errorf("program interrupted")
 			}
+
 			if c.Logger != nil {
 				c.Logger.Error(err.Error())
-			} else {
-				fmt.Fprintln(os.Stderr, err)
+			}
+
+			// FriendlyErrors are intended for presentation directly to the user without requiring the full error context
+			// If we got one, we can print it and exit
+			if friendlyError, ok := errors.AsType[FriendlyError](err); ok {
+				fmt.Fprintln(c.Options.Stderr, friendlyError.FriendlyError())
+				os.Exit(1)
+			}
+
+			// We weren't able to obtain a simple message to print, so we make sure the full error context gets sent
+			// *somewhere*. If we don't have a logger or it's not sending messages anywhere (the default noop logger),
+			// then we print the full error to stderr to ensure we don't just fail silently.
+			if c.Logger == nil || !c.Logger.Enabled(nil, slog.LevelError) {
+				fmt.Fprintln(c.Options.Stderr, err)
 			}
 			os.Exit(1)
 		}

temporalcloudcli/commands.whoami.go

@@ -9,12 +9,12 @@ import (
 // run calls GetCurrentIdentity and prints the authenticated principal (User or
 // ServiceAccount) along with the associated API key, if any.
 func (c *CloudWhoamiCommand) run(cctx *CommandContext, _ []string) error {
-	cloudClient, err := cctx.BuildCloudClient(c.ClientOptions)
+	cloudClient, err := cctx.GetCloudClient(c.ClientOptions)
 	if err != nil {
 		return err
 	}
 
-	res, err := cloudClient.CloudService().GetCurrentIdentity(cctx.Context, &cloudservice.GetCurrentIdentityRequest{})
+	res, err := cloudClient.GetCurrentIdentity(cctx.Context, &cloudservice.GetCurrentIdentityRequest{})
 	if err != nil {
 		return err
 	}

temporalcloudcli/commands.whoami_unit_test.go

@@ -0,0 +1,49 @@
+package temporalcloudcli_test
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/mock"
+	cloudmock "github.com/temporalio/cloud-cli/internal/cloudservice/mock"
+	"github.com/temporalio/cloud-cli/temporalcloudcli"
+	cloudservice "go.temporal.io/cloud-sdk/api/cloudservice/v1"
+)
+
+func TestWhoami(t *testing.T) {
+	tests := []struct {
+		name                    string
+		cmd                     temporalcloudcli.CloudWhoamiCommand
+		cloudClientExpectations func(*cloudmock.MockCloudServiceClient)
+		expectedErr             string
+	}{
+		{
+			name: "WhoamiSuccess",
+			cmd:  temporalcloudcli.CloudWhoamiCommand{},
+			cloudClientExpectations: func(c *cloudmock.MockCloudServiceClient) {
+				c.EXPECT().
+					GetCurrentIdentity(mock.Anything, &cloudservice.GetCurrentIdentityRequest{}, mock.Anything).
+					Return(&cloudservice.GetCurrentIdentityResponse{}, nil)
+			},
+		},
+		{
+			name: "WhoamiNotLoggedIn",
+			cmd:  temporalcloudcli.CloudWhoamiCommand{},
+			cloudClientExpectations: func(c *cloudmock.MockCloudServiceClient) {
+				c.EXPECT().
+					GetCurrentIdentity(mock.Anything, &cloudservice.GetCurrentIdentityRequest{}, mock.Anything).
+					Return(nil, fmt.Errorf("no login session found, please run `temporal cloud login`"))
+			},
+			expectedErr: "no login session found, please run `temporal cloud login`",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			temporalcloudcli.TestCommand(t, &tt.cmd, temporalcloudcli.TestCommandOptions{
+				CloudClientExpectations: tt.cloudClientExpectations,
+				ExpectedError:           tt.expectedErr,
+			})
+		})
+	}
+}

temporalcloudcli/commands_test.go

@@ -4,17 +4,12 @@
 package temporalcloudcli_test
 
 import (
-	"bytes"
 	"context"
 	"errors"
-	"fmt"
 	"io"
 	"math/rand"
 	"os"
-	"regexp"
-	"strings"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -27,132 +22,6 @@ import (
 	"github.com/temporalio/cloud-cli/temporalcloudcli/internal/printer"
 )
 
-type CommandHarness struct {
-	*require.Assertions
-	t       *testing.T
-	Options temporalcloudcli.CommandOptions
-	// Defaults to a context closed on close or test complete
-	Context context.Context
-	// Can be used to cancel context given to commands (simulating interrupt)
-	CancelContext context.CancelFunc
-	Stdin         bytes.Buffer
-}
-
-func NewCommandHarness(t *testing.T) *CommandHarness {
-	h := &CommandHarness{Assertions: require.New(t), t: t}
-	h.Context, h.CancelContext = context.WithCancel(context.Background())
-	t.Cleanup(h.Close)
-	return h
-}
-
-// Reentrant, called after test by default, cancels context
-func (h *CommandHarness) Close() {
-	// Cancel context
-	if h.CancelContext != nil {
-		h.CancelContext()
-	}
-}
-
-// Pieces must appear in order on the line and not overlap
-func (h *CommandHarness) ContainsOnSameLine(text string, pieces ...string) {
-	h.NoError(AssertContainsOnSameLine(text, pieces...))
-}
-
-func AssertContainsOnSameLine(text string, pieces ...string) error {
-	// Build regex pattern based on pieces
-	pattern := ""
-	for _, piece := range pieces {
-		if pattern != "" {
-			pattern += ".*"
-		}
-		pattern += regexp.QuoteMeta(piece)
-	}
-	regex, err := regexp.Compile(pattern)
-	if err != nil {
-		return err
-	}
-	// Split into lines, then check each piece is present
-	lines := strings.Split(text, "\n")
-	for _, line := range lines {
-		if regex.MatchString(line) {
-			return nil
-		}
-	}
-	return fmt.Errorf("pieces not found in order on any line together")
-}
-
-func TestAssertContainsOnSameLine(t *testing.T) {
-	require.Error(t, AssertContainsOnSameLine("a b c", "b", "a"))
-	require.Error(t, AssertContainsOnSameLine("a\nb c", "a", "b"))
-	require.NoError(t, AssertContainsOnSameLine("aba", "b", "a"))
-	require.NoError(t, AssertContainsOnSameLine("a b a", "b", "a"))
-	require.NoError(t, AssertContainsOnSameLine("axb", "a", "b"))
-	require.NoError(t, AssertContainsOnSameLine("a a", "a", "a"))
-}
-
-func (h *CommandHarness) Eventually(
-	condition func() bool,
-	waitFor time.Duration,
-	tick time.Duration,
-	msgAndArgs ...interface{},
-) {
-	h.t.Helper()
-	// We cannot use require.Eventually because it was poorly developed to run the
-	// condition function in a goroutine which means it can run after complete or
-	// have other race conditions. Don't even need a complicated ticker because it
-	// doesn't need to be interruptible.
-	for start := time.Now(); time.Since(start) < waitFor; {
-		if condition() {
-			return
-		}
-		time.Sleep(tick)
-	}
-	h.Fail("condition did not evaluate to true within timeout", msgAndArgs...)
-}
-
-func (h *CommandHarness) T() *testing.T {
-	return h.t
-}
-
-type CommandResult struct {
-	Err    error
-	Stdout bytes.Buffer
-	Stderr bytes.Buffer
-}
-
-func (h *CommandHarness) Execute(args ...string) *CommandResult {
-	// Copy options, update as needed
-	res := &CommandResult{}
-	options := h.Options
-	// Set stdio
-	options.Stdin = &h.Stdin
-	options.Stdout = &res.Stdout
-	options.Stderr = &res.Stderr
-	// Set args
-	options.Args = args
-	// 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, " "))
-	temporalcloudcli.Execute(ctx, options)
-	if res.Stdout.Len() > 0 {
-		h.t.Logf("Stdout:\n-----\n%s\n-----", &res.Stdout)
-	}
-	if res.Stderr.Len() > 0 {
-		h.t.Logf("Stderr:\n-----\n%s\n-----", &res.Stderr)
-	}
-	return res
-}
-
 type EnvLookupMap map[string]string
 
 func (e EnvLookupMap) Environ() []string {