Address code review feedback

- Add DefaultAuthTimeout constant for configurability
- Fix string index check to handle idx >= 0 correctly
- Use cmd.Context() for proper cancellation support
- Add named constant for PKCE verifier length in test
- Remove unused context import

Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>

Author: Copilot

cmd/github-mcp-server/main.go

@@ -1,7 +1,6 @@
 package main
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"os"
@@ -33,23 +32,22 @@ var (
 		Use:   "stdio",
 		Short: "Start stdio server",
 		Long:  `Start a server that communicates via standard input/output streams using JSON-RPC messages.`,
-		RunE: func(_ *cobra.Command, _ []string) error {
+		RunE: func(cmd *cobra.Command, _ []string) error {
 			token := viper.GetString("personal_access_token")
 
 			// If no token provided, check if OAuth is configured
 			if token == "" {
 				oauthClientID := viper.GetString("oauth_client_id")
 				if oauthClientID != "" {
-					// Perform interactive OAuth flow
-					ctx := context.Background()
+					// Perform interactive OAuth flow with the command's context
 					oauthCfg := oauth.GetGitHubOAuthConfig(
 						oauthClientID,
 						viper.GetString("oauth_client_secret"),
 						getOAuthScopes(),
 						viper.GetString("host"), // Pass the gh-host configuration
 					)
 
-					result, err := oauth.StartInteractiveFlow(ctx, oauthCfg)
+					result, err := oauth.StartInteractiveFlow(cmd.Context(), oauthCfg)
 					if err != nil {
 						return fmt.Errorf("OAuth flow failed: %w", err)
 					}

internal/oauth/oauth.go

@@ -18,6 +18,11 @@ import (
 	"golang.org/x/oauth2"
 )
 
+const (
+	// DefaultAuthTimeout is the default timeout for the OAuth authorization flow
+	DefaultAuthTimeout = 5 * time.Minute
+)
+
 // Config holds the OAuth configuration
 type Config struct {
 	ClientID     string
@@ -133,8 +138,8 @@ func StartInteractiveFlow(ctx context.Context, cfg Config) (*Result, error) {
 		return nil, fmt.Errorf("callback error: %w", err)
 	case <-ctx.Done():
 		return nil, fmt.Errorf("context cancelled: %w", ctx.Err())
-	case <-time.After(5 * time.Minute):
-		return nil, fmt.Errorf("authorization timeout after 5 minutes")
+	case <-time.After(DefaultAuthTimeout):
+		return nil, fmt.Errorf("authorization timeout after %v", DefaultAuthTimeout)
 	}
 
 	// Shutdown server gracefully
@@ -297,7 +302,7 @@ func getOAuthEndpoints(host string) (authURL, tokenURL string) {
 	}
 
 	// Remove any trailing slashes or paths
-	if idx := strings.Index(hostname, "/"); idx > 0 {
+	if idx := strings.Index(hostname, "/"); idx >= 0 {
 		hostname = hostname[:idx]
 	}
 

internal/oauth/oauth_test.go

@@ -7,13 +7,19 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+const (
+	// expectedPKCEVerifierMinLength is the expected minimum length of a PKCE verifier
+	// Base64URL encoding of 32 bytes = 43 characters (32 * 8 / 6, rounded up)
+	expectedPKCEVerifierMinLength = 43
+)
+
 func TestGeneratePKCEVerifier(t *testing.T) {
 	verifier, err := generatePKCEVerifier()
 	require.NoError(t, err)
 	require.NotEmpty(t, verifier)
 
 	// Verifier should be at least 43 characters (base64url of 32 bytes)
-	assert.GreaterOrEqual(t, len(verifier), 43)
+	assert.GreaterOrEqual(t, len(verifier), expectedPKCEVerifierMinLength)
 
 	// Generate another one to ensure they're different
 	verifier2, err := generatePKCEVerifier()