Refactor: move ExpandEnvArgs to config and normalizeScopeKind to config

Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/69beac78-a2d6-4a9a-b20c-0c5ceaea730d

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Author: Copilot

internal/config/docker_helpers.go

@@ -6,8 +6,12 @@ import (
 	"os/exec"
 	"regexp"
 	"strings"
+
+	"github.com/github/gh-aw-mcpg/internal/logger"
 )
 
+var logDockerHelpers = logger.New("config:docker_helpers")
+
 // containerIDPattern validates that a container ID only contains valid characters (hex digits)
 // Container IDs are 64 character hex strings, but short form (12 chars) is also valid
 var containerIDPattern = regexp.MustCompile(`^[a-f0-9]{12,64}$`)
@@ -113,3 +117,33 @@ func checkLogDirMounted(containerID, logDir string) bool {
 	// Check if the log directory is in the mounts
 	return strings.Contains(output, logDir)
 }
+
+// ExpandEnvArgs expands Docker -e flags that reference environment variables.
+// Converts "-e VAR_NAME" to "-e VAR_NAME=value" by reading from the process environment.
+func ExpandEnvArgs(args []string) []string {
+	logDockerHelpers.Printf("Expanding env args: input_count=%d", len(args))
+	result := make([]string, 0, len(args))
+	for i := 0; i < len(args); i++ {
+		arg := args[i]
+
+		// Check if this is a -e flag
+		if arg == "-e" && i+1 < len(args) {
+			nextArg := args[i+1]
+			// If next arg doesn't contain '=', it's a variable reference
+			if len(nextArg) > 0 && !strings.Contains(nextArg, "=") {
+				// Look up the variable in the environment
+				if value, exists := os.LookupEnv(nextArg); exists {
+					logDockerHelpers.Printf("Expanding env var: name=%s", nextArg)
+					result = append(result, "-e")
+					result = append(result, fmt.Sprintf("%s=%s", nextArg, value))
+					i++ // Skip the next arg since we processed it
+					continue
+				}
+				logDockerHelpers.Printf("Env var not found in process environment: name=%s", nextArg)
+			}
+		}
+		result = append(result, arg)
+	}
+	logDockerHelpers.Printf("Env args expansion complete: output_count=%d", len(result))
+	return result
+}

internal/config/docker_helpers_and_env_test.go

@@ -119,6 +119,150 @@ func TestValidateContainerID_SecurityCritical(t *testing.T) {
 	}
 }
 
+// TestExpandEnvArgs tests ExpandEnvArgs with various -e flag combinations.
+func TestExpandEnvArgs(t *testing.T) {
+	tests := []struct {
+		name    string
+		args    []string
+		envVars map[string]string
+		want    []string
+	}{
+		{
+			name: "nil input returns empty slice",
+			args: nil,
+			want: []string{},
+		},
+		{
+			name: "empty input returns empty slice",
+			args: []string{},
+			want: []string{},
+		},
+		{
+			name: "args without -e flag pass through unchanged",
+			args: []string{"run", "--rm", "-i", "ghcr.io/org/image:latest"},
+			want: []string{"run", "--rm", "-i", "ghcr.io/org/image:latest"},
+		},
+		{
+			name:    "-e VAR_NAME is expanded when env var is set",
+			args:    []string{"-e", "MY_TOKEN"},
+			envVars: map[string]string{"MY_TOKEN": "secret-value"},
+			want:    []string{"-e", "MY_TOKEN=secret-value"},
+		},
+		{
+			name: "-e VAR_NAME is left unchanged when env var is not set",
+			args: []string{"-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
+			want: []string{"-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
+		},
+		{
+			name:    "-e VAR=VALUE (already has =) is passed through unchanged",
+			args:    []string{"-e", "MY_VAR=already-set"},
+			envVars: map[string]string{"MY_VAR": "other-value"},
+			want:    []string{"-e", "MY_VAR=already-set"},
+		},
+		{
+			name: "-e at end of args (no following value) is passed through",
+			args: []string{"run", "-e"},
+			want: []string{"run", "-e"},
+		},
+		{
+			name:    "multiple -e flags are all expanded",
+			args:    []string{"-e", "TOKEN_A", "-e", "TOKEN_B"},
+			envVars: map[string]string{"TOKEN_A": "val-a", "TOKEN_B": "val-b"},
+			want:    []string{"-e", "TOKEN_A=val-a", "-e", "TOKEN_B=val-b"},
+		},
+		{
+			name:    "mix of set and unset env vars in -e flags",
+			args:    []string{"-e", "SET_VAR", "-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
+			envVars: map[string]string{"SET_VAR": "value"},
+			want:    []string{"-e", "SET_VAR=value", "-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
+		},
+		{
+			name:    "realistic docker run command with env var expansion",
+			args:    []string{"run", "--rm", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN", "-i", "ghcr.io/github/github-mcp-server:latest"},
+			envVars: map[string]string{"GITHUB_PERSONAL_ACCESS_TOKEN": "ghp_abc123"},
+			want:    []string{"run", "--rm", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN=ghp_abc123", "-i", "ghcr.io/github/github-mcp-server:latest"},
+		},
+		{
+			name: "-e VAR where VAR is empty string is passed through unchanged",
+			args: []string{"-e", ""},
+			want: []string{"-e", ""},
+		},
+		{
+			name:    "env var with empty value is expanded with empty value",
+			args:    []string{"-e", "EMPTY_VAR"},
+			envVars: map[string]string{"EMPTY_VAR": ""},
+			want:    []string{"-e", "EMPTY_VAR="},
+		},
+		{
+			name:    "env var value containing = is expanded correctly",
+			args:    []string{"-e", "URL_VAR"},
+			envVars: map[string]string{"URL_VAR": "https://example.com?key=val"},
+			want:    []string{"-e", "URL_VAR=https://example.com?key=val"},
+		},
+		{
+			name:    "non -e flags between expanded flags are preserved",
+			args:    []string{"-e", "VAR_A", "--name", "mycontainer", "-e", "VAR_B"},
+			envVars: map[string]string{"VAR_A": "alpha", "VAR_B": "beta"},
+			want:    []string{"-e", "VAR_A=alpha", "--name", "mycontainer", "-e", "VAR_B=beta"},
+		},
+		{
+			name:    "same var referenced multiple times is expanded each time",
+			args:    []string{"-e", "SHARED_VAR", "-e", "SHARED_VAR"},
+			envVars: map[string]string{"SHARED_VAR": "shared"},
+			want:    []string{"-e", "SHARED_VAR=shared", "-e", "SHARED_VAR=shared"},
+		},
+		{
+			name: "-e immediately followed by another -e flag where first -e var is unset",
+			args: []string{"-e", "-e"},
+			// The first "-e" tries to expand the second "-e" as a var name.
+			// Since no env var named "-e" is set, the expansion is skipped.
+			// Both "-e" args are emitted as-is.
+			want: []string{"-e", "-e"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			for k, v := range tt.envVars {
+				t.Setenv(k, v)
+			}
+
+			got := ExpandEnvArgs(tt.args)
+
+			require.NotNil(t, got, "ExpandEnvArgs should never return nil")
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
+// TestExpandEnvArgs_DoesNotMutateInput verifies that the original args slice
+// is not modified by ExpandEnvArgs.
+func TestExpandEnvArgs_DoesNotMutateInput(t *testing.T) {
+	t.Setenv("MY_SECRET", "secret-value")
+
+	original := []string{"-e", "MY_SECRET", "--rm"}
+	// Make a copy to compare against after the call
+	copyOfOriginal := make([]string, len(original))
+	copy(copyOfOriginal, original)
+
+	ExpandEnvArgs(original)
+
+	assert.Equal(t, copyOfOriginal, original, "ExpandEnvArgs must not mutate the input slice")
+}
+
+// TestExpandEnvArgs_OutputIsIndependentOfInput verifies that modifications to
+// the returned slice do not affect the original input.
+func TestExpandEnvArgs_OutputIsIndependentOfInput(t *testing.T) {
+	t.Setenv("SOME_VAR", "value")
+
+	args := []string{"run", "-e", "SOME_VAR"}
+	result := ExpandEnvArgs(args)
+
+	// Modifying the result should not affect the original
+	result[0] = "MODIFIED"
+	assert.Equal(t, "run", args[0], "Modifying result should not affect original slice")
+}
+
 // TestGetGatewayPortFromEnv tests the env-based gateway port parsing.
 func TestGetGatewayPortFromEnv_Comprehensive(t *testing.T) {
 	tests := []struct {

internal/config/guard_policy.go

@@ -700,3 +700,22 @@ func validateGuardPolicies(cfg *Config) error {
 	}
 	return nil
 }
+
+// NormalizeScopeKind returns a copy of the policy map with the scope_kind field
+// normalized to lowercase trimmed string form. Other fields are preserved as-is.
+func NormalizeScopeKind(policy map[string]interface{}) map[string]interface{} {
+	if policy == nil {
+		return nil
+	}
+
+	normalized := make(map[string]interface{}, len(policy))
+	for key, value := range policy {
+		normalized[key] = value
+	}
+
+	if scopeKind, ok := normalized["scope_kind"].(string); ok {
+		normalized["scope_kind"] = strings.ToLower(strings.TrimSpace(scopeKind))
+	}
+
+	return normalized
+}

internal/mcp/connection.go

@@ -14,6 +14,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/github/gh-aw-mcpg/internal/config"
 	"github.com/github/gh-aw-mcpg/internal/difc"
 	"github.com/github/gh-aw-mcpg/internal/logger"
 	"github.com/github/gh-aw-mcpg/internal/logger/sanitize"
@@ -105,7 +106,7 @@ func NewConnection(ctx context.Context, serverID, command string, args []string,
 
 	// Expand Docker -e flags that reference environment variables
 	// Docker's `-e VAR_NAME` expects VAR_NAME to be in the environment
-	expandedArgs := ExpandEnvArgs(args)
+	expandedArgs := config.ExpandEnvArgs(args)
 	logConn.Printf("Expanded args for Docker env: %v", sanitize.SanitizeArgs(expandedArgs))
 
 	// Create command transport

internal/mcp/connection_test.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/github/gh-aw-mcpg/internal/config"
 	"github.com/github/gh-aw-mcpg/internal/difc"
 	"github.com/github/gh-aw-mcpg/internal/logger"
 	"github.com/stretchr/testify/assert"
@@ -256,7 +257,7 @@ func TestExpandDockerEnvArgs(t *testing.T) {
 				}
 			})
 
-			result := ExpandEnvArgs(tt.args)
+			result := config.ExpandEnvArgs(tt.args)
 			assert.Equal(t, tt.expected, result)
 		})
 	}