Tighten error matching, improve login context handling, and tests (#281)

* fix(review): tighten error matching, add safety comments, revert noise

- tool_projects_errors.go: match "status code: 4xx" instead of bare
  "401"/"403" to avoid false positives on IDs or timestamps
- tool_login.go: remove `_ = ctx` suppression, add TODO for context
  propagation to polling goroutine, document happens-before on
  loginState.err via channel close
- root.go: add maintenance comment linking flagNeedsNextArg to init()
- helpers.go: revert unrelated Attempt struct alignment change

https://claude.ai/code/session_01EpXZqTmgybtjgmSALukH8d

* fix(review): ListProjects returns *APIError, add reauth + argv tests

- projects.go: use checkAndPrintError instead of manual status check so
  ListProjects errors are structured *APIError — the errors.As path in
  shouldSuggestReauthAfterListProjectsFailure now matches directly
- tool_projects_errors_test.go: unit tests for reauth hint logic covering
  APIError 401/403, plain error fallback, and false-positive resistance
- root_argv_test.go: document boolean-flag-between-subcommands limitation

https://claude.ai/code/session_01EpXZqTmgybtjgmSALukH8d

* fix(mcp): cancel login polling goroutine on MCP session close

Thread the request context into the login polling goroutine so it
stops promptly when the MCP transport closes, instead of running for
up to ~4 minutes after the client disconnects.

WaitForAPIKey blocks with time.Sleep and doesn't accept a context, so
we run it in an inner goroutine and select on both its result channel
and ctx.Done(). The inner goroutine is bounded by loginMaxAttempts.

https://claude.ai/code/session_01EpXZqTmgybtjgmSALukH8d

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: leggetter

pkg/cmd/root.go

@@ -146,6 +146,8 @@ func argvContainsGatewayMCP(argv []string) bool {
 	return false
 }
 
+// flagNeedsNextArg lists global flags that consume the next argv token as their value.
+// Keep in sync with the PersistentFlags registered in init() below.
 var flagNeedsNextArg = map[string]bool{
 	"profile":         true,
 	"p":               true,

pkg/cmd/root_argv_test.go

@@ -21,6 +21,10 @@ func TestArgvContainsGatewayMCP(t *testing.T) {
 		{"wrong order", []string{"hookdeck", "mcp", "gateway"}, false},
 		{"too short", []string{"hookdeck", "gateway"}, false},
 		{"api key flag before", []string{"hookdeck", "--api-key", "k", "gateway", "mcp"}, true},
+		// Boolean flags (--insecure, --version) are not in flagNeedsNextArg, so
+		// globalPositionalArgs treats them as single-token flags and skips them.
+		{"bool flag before gateway", []string{"hookdeck", "--insecure", "gateway", "mcp"}, true},
+		{"bool flag between gateway and mcp", []string{"hookdeck", "gateway", "--insecure", "mcp"}, false},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

pkg/gateway/mcp/tool_login.go

@@ -26,6 +26,10 @@ const (
 
 // loginState tracks a background login poll so that repeated calls to
 // hookdeck_login don't start duplicate auth flows.
+//
+// Synchronization: err is written by the goroutine before close(done).
+// The handler only reads err after receiving from done, so the channel
+// close provides the happens-before guarantee — no separate mutex needed.
 type loginState struct {
 	browserURL string        // URL the user must open
 	done       chan struct{} // closed when polling finishes
@@ -37,7 +41,6 @@ func handleLogin(client *hookdeck.Client, cfg *config.Config) mcpsdk.ToolHandler
 	var state *loginState
 
 	return func(ctx context.Context, req *mcpsdk.CallToolRequest) (*mcpsdk.CallToolResult, error) {
-		_ = ctx
 		in, err := parseInput(req.Params.Arguments)
 		if err != nil {
 			return ErrorResult(err.Error()), nil
@@ -118,14 +121,34 @@ func handleLogin(client *hookdeck.Client, cfg *config.Config) mcpsdk.ToolHandler
 		}
 
 		// Poll in the background so we return the URL to the agent immediately.
-		go func(s *loginState) {
+		// WaitForAPIKey blocks with time.Sleep; run it in a goroutine and
+		// select on ctx so we abandon the attempt when the session closes.
+		go func(s *loginState, ctx context.Context) {
 			defer close(s.done)
 
-			response, err := session.WaitForAPIKey(loginPollInterval, loginMaxAttempts)
-			if err != nil {
-				s.err = err
-				log.WithError(err).Debug("Login polling failed")
+			type pollResult struct {
+				resp *hookdeck.PollAPIKeyResponse
+				err  error
+			}
+			ch := make(chan pollResult, 1)
+			go func() {
+				resp, err := session.WaitForAPIKey(loginPollInterval, loginMaxAttempts)
+				ch <- pollResult{resp, err}
+			}()
+
+			var response *hookdeck.PollAPIKeyResponse
+			select {
+			case <-ctx.Done():
+				s.err = fmt.Errorf("login cancelled: MCP session closed")
+				log.Debug("Login polling cancelled — MCP session closed")
 				return
+			case r := <-ch:
+				if r.err != nil {
+					s.err = r.err
+					log.WithError(r.err).Debug("Login polling failed")
+					return
+				}
+				response = r.resp
 			}
 
 			if err := validators.APIKey(response.APIKey); err != nil {
@@ -164,7 +187,7 @@ func handleLogin(client *hookdeck.Client, cfg *config.Config) mcpsdk.ToolHandler
 				"user":    response.UserName,
 				"project": response.ProjectName,
 			}).Info("MCP login completed successfully")
-		}(state)
+		}(state, ctx)
 
 		// Return the URL immediately so the agent can show it to the user.
 		return TextResult(fmt.Sprintf(

pkg/gateway/mcp/tool_projects_errors.go

@@ -26,9 +26,11 @@ func shouldSuggestReauthAfterListProjectsFailure(err error) bool {
 		}
 		return strings.Contains(strings.ToLower(apiErr.Message), "fatal")
 	}
-	// ListProjects wraps some failures as plain fmt.Errorf with status in the text.
+	// ListProjects wraps some failures as plain fmt.Errorf with the status code
+	// in the text (e.g. "unexpected http status code: 403 <nil>").
+	// Match "status code: 4xx" to avoid false positives on IDs containing "401"/"403".
 	msg := strings.ToLower(err.Error())
-	if strings.Contains(msg, "403") || strings.Contains(msg, "401") {
+	if strings.Contains(msg, "status code: 403") || strings.Contains(msg, "status code: 401") {
 		return true
 	}
 	return strings.Contains(msg, "fatal")

pkg/gateway/mcp/tool_projects_errors_test.go

@@ -0,0 +1,63 @@
+package mcp
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/hookdeck/hookdeck-cli/pkg/hookdeck"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestShouldSuggestReauthAfterListProjectsFailure(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{
+			name: "APIError 403",
+			err:  &hookdeck.APIError{StatusCode: 403, Message: "not allowed"},
+			want: true,
+		},
+		{
+			name: "APIError 401",
+			err:  &hookdeck.APIError{StatusCode: 401, Message: "unauthorized"},
+			want: true,
+		},
+		{
+			name: "APIError 500 no match",
+			err:  &hookdeck.APIError{StatusCode: 500, Message: "internal error"},
+			want: false,
+		},
+		{
+			name: "APIError with fatal message",
+			err:  &hookdeck.APIError{StatusCode: 500, Message: "fatal: cannot proceed"},
+			want: true,
+		},
+		{
+			name: "plain error with status code 403",
+			err:  fmt.Errorf("unexpected http status code: 403 <nil>"),
+			want: true,
+		},
+		{
+			name: "plain error with status code 401",
+			err:  fmt.Errorf("unexpected http status code: 401 <nil>"),
+			want: true,
+		},
+		{
+			name: "plain error without status code",
+			err:  fmt.Errorf("network timeout"),
+			want: false,
+		},
+		{
+			name: "plain error with 403 in ID should not match",
+			err:  fmt.Errorf("project proj_403_abc not found"),
+			want: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, shouldSuggestReauthAfterListProjectsFailure(tt.err))
+		})
+	}
+}

pkg/hookdeck/projects.go

@@ -2,8 +2,6 @@ package hookdeck
 
 import (
 	"context"
-	"fmt"
-	"net/http"
 )
 
 type Project struct {
@@ -17,8 +15,8 @@ func (c *Client) ListProjects() ([]Project, error) {
 	if err != nil {
 		return []Project{}, err
 	}
-	if res.StatusCode != http.StatusOK {
-		return []Project{}, fmt.Errorf("unexpected http status code: %d %s", res.StatusCode, err)
+	if err := checkAndPrintError(res); err != nil {
+		return []Project{}, err
 	}
 	projects := []Project{}
 	postprocessJsonResponse(res, &projects)

test/acceptance/helpers.go

@@ -759,10 +759,10 @@ type Request struct {
 
 // Attempt represents a Hookdeck attempt for testing
 type Attempt struct {
-	ID            string `json:"id"`
-	EventID       string `json:"event_id"`
-	AttemptNumber int    `json:"attempt_number"`
-	Status        string `json:"status"`
+	ID           string `json:"id"`
+	EventID      string `json:"event_id"`
+	AttemptNumber int   `json:"attempt_number"`
+	Status       string `json:"status"`
 }
 
 // createTestConnection creates a basic test connection and returns its ID