Consolidate duplicate container detection into sys.DetectContainerID

internal/config/validation_env.go

@@ -1,12 +1,12 @@
 package config
 
 import (
-	"bufio"
 	"fmt"
 	"os"
 	"strings"
 
 	"github.com/github/gh-aw-mcpg/internal/logger"
+	"github.com/github/gh-aw-mcpg/internal/sys"
 )
 
 var logEnv = logger.New("config:validation_env")
@@ -69,7 +69,7 @@ func ValidateExecutionEnvironment() *EnvValidationResult {
 	result := &EnvValidationResult{}
 
 	// Check if running in a containerized environment
-	result.IsContainerized, result.ContainerID = detectContainerized()
+	result.IsContainerized, result.ContainerID = sys.DetectContainerID()
 	logEnv.Printf("Containerization check: isContainerized=%v, containerID=%s", result.IsContainerized, result.ContainerID)
 
 	// Check Docker daemon accessibility
@@ -147,46 +147,6 @@ func ValidateContainerizedEnvironment(containerID string) *EnvValidationResult {
 	return result
 }
 
-// detectContainerized checks if we're running inside a Docker container
-// It examines /proc/self/cgroup to detect container environment and extract container ID
-func detectContainerized() (bool, string) {
-	file, err := os.Open("/proc/self/cgroup")
-	if err != nil {
-		// If we can't read cgroup, we're likely not in a container
-		return false, ""
-	}
-	defer file.Close()
-
-	scanner := bufio.NewScanner(file)
-	for scanner.Scan() {
-		line := scanner.Text()
-		// Docker containers have docker paths in cgroup
-		if strings.Contains(line, "docker") || strings.Contains(line, "containerd") {
-			// Extract container ID from the path
-			// Format is typically: 0::/docker/<container_id>
-			parts := strings.Split(line, "/")
-			for i, part := range parts {
-				if (part == "docker" || part == "containerd") && i+1 < len(parts) {
-					containerID := parts[i+1]
-					// Container IDs are 64 hex characters (or 12 for short form)
-					if len(containerID) >= 12 {
-						return true, containerID
-					}
-				}
-			}
-			// Found docker/containerd reference but couldn't extract ID
-			return true, ""
-		}
-	}
-
-	// Also check for .dockerenv file
-	if _, err := os.Stat("/.dockerenv"); err == nil {
-		return true, ""
-	}
-
-	return false, ""
-}
-
 // checkRequiredEnvVars checks if all required environment variables are set
 func checkRequiredEnvVars() []string {
 	var missing []string

internal/config/validation_env_test.go

@@ -5,16 +5,17 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/github/gh-aw-mcpg/internal/sys"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestDetectContainerized(t *testing.T) {
 	// This test verifies the function doesn't panic and returns consistent results
-	isContainerized, containerID := detectContainerized()
+	isContainerized, containerID := sys.DetectContainerID()
 
 	// In a test environment, we're typically not containerized
 	// but we just verify the function works
-	t.Logf("detectContainerized: isContainerized=%v, containerID=%s", isContainerized, containerID)
+	t.Logf("DetectContainerID: isContainerized=%v, containerID=%s", isContainerized, containerID)
 
 	// If we detect a container, the ID should have some content
 	if isContainerized && containerID != "" {

internal/sys/container.go

@@ -1,6 +1,7 @@
 package sys
 
 import (
+	"bufio"
 	"os"
 	"strings"
 
@@ -9,35 +10,86 @@ import (
 
 var logSys = logger.New("sys:container")
 
+// containerIndicators lists the cgroup path substrings that indicate a container environment.
+var containerIndicators = []string{"docker", "containerd", "kubepods", "lxc"}
+
 // IsRunningInContainer detects if the current process is running inside a container.
 func IsRunningInContainer() bool {
+	detected, _ := DetectContainerID()
+	return detected
+}
+
+// DetectContainerID detects if the current process is running inside a container
+// and attempts to extract the container ID from cgroup entries.
+// It returns (isContainer, containerID). The containerID may be empty even when
+// a container is detected (e.g., via /.dockerenv or environment variable).
+func DetectContainerID() (bool, string) {
 	logSys.Print("Detecting container environment")
 
 	// Method 1: Check for /.dockerenv file (Docker-specific)
 	if _, err := os.Stat("/.dockerenv"); err == nil {
 		logSys.Print("Container detected via /.dockerenv")
-		return true
+		// Still try to extract container ID from cgroup
+		if id := extractContainerIDFromCgroup(); id != "" {
+			return true, id
+		}
+		return true, ""
 	}
 
 	// Method 2: Check /proc/1/cgroup for container indicators
 	data, err := os.ReadFile("/proc/1/cgroup")
 	if err == nil {
 		content := string(data)
-		if strings.Contains(content, "docker") ||
-			strings.Contains(content, "containerd") ||
-			strings.Contains(content, "kubepods") ||
-			strings.Contains(content, "lxc") {
-			logSys.Print("Container detected via /proc/1/cgroup")
-			return true
+		for _, indicator := range containerIndicators {
+			if strings.Contains(content, indicator) {
+				logSys.Print("Container detected via /proc/1/cgroup")
+				if id := extractContainerIDFromContent(content); id != "" {
+					return true, id
+				}
+				return true, ""
+			}
 		}
 	}
 
 	// Method 3: Check environment variable (set by Dockerfile)
 	if os.Getenv("RUNNING_IN_CONTAINER") == "true" {
 		logSys.Print("Container detected via RUNNING_IN_CONTAINER env var")
-		return true
+		return true, ""
 	}
 
 	logSys.Print("No container indicators found, running on host")
-	return false
+	return false, ""
+}
+
+// extractContainerIDFromCgroup reads /proc/1/cgroup and tries to extract a container ID.
+func extractContainerIDFromCgroup() string {
+	data, err := os.ReadFile("/proc/1/cgroup")
+	if err != nil {
+		return ""
+	}
+	return extractContainerIDFromContent(string(data))
+}
+
+// extractContainerIDFromContent parses cgroup content line-by-line and extracts a container ID.
+// It looks for path segments following "docker" or "containerd" that are at least 12 characters long.
+func extractContainerIDFromContent(content string) string {
+	scanner := bufio.NewScanner(strings.NewReader(content))
+	for scanner.Scan() {
+		line := scanner.Text()
+		if strings.Contains(line, "docker") || strings.Contains(line, "containerd") {
+			parts := strings.Split(line, "/")
+			for i, part := range parts {
+				if (part == "docker" || part == "containerd") && i+1 < len(parts) {
+					containerID := parts[i+1]
+					if len(containerID) >= 12 {
+						return containerID
+					}
+				}
+			}
+		}
+	}
+	if err := scanner.Err(); err != nil {
+		logSys.Printf("Error scanning cgroup content: %v", err)
+	}
+	return ""
 }

internal/sys/container_test.go

@@ -245,6 +245,84 @@ func TestIsRunningInContainer_Consistency(t *testing.T) {
 	}
 }
 
+func TestDetectContainerID_EnvVar(t *testing.T) {
+	// When RUNNING_IN_CONTAINER=true, DetectContainerID should return true with empty ID
+	t.Setenv("RUNNING_IN_CONTAINER", "true")
+
+	detected, _ := DetectContainerID()
+	assert.True(t, detected, "Should detect container via env var")
+}
+
+func TestDetectContainerID_ConsistentWithIsRunningInContainer(t *testing.T) {
+	// DetectContainerID and IsRunningInContainer should agree on detection
+	unsetEnvForTest(t, "RUNNING_IN_CONTAINER")
+
+	detected, containerID := DetectContainerID()
+	isRunning := IsRunningInContainer()
+
+	assert.Equal(t, detected, isRunning,
+		"DetectContainerID and IsRunningInContainer should return the same detection result")
+
+	// If detected and has an ID, ID should be at least 12 chars
+	if detected && containerID != "" {
+		assert.GreaterOrEqual(t, len(containerID), 12, "Container ID should be at least 12 characters")
+	}
+
+	t.Logf("DetectContainerID: detected=%v, containerID=%s", detected, containerID)
+	t.Logf("IsRunningInContainer: %v", isRunning)
+}
+
+func TestExtractContainerIDFromContent(t *testing.T) {
+	tests := []struct {
+		name     string
+		content  string
+		expected string
+	}{
+		{
+			name:     "docker with 64 char ID",
+			content:  "0::/docker/abcdef1234567890abcdef1234567890abcdef1234567890abcdef12345678",
+			expected: "abcdef1234567890abcdef1234567890abcdef1234567890abcdef12345678",
+		},
+		{
+			name:     "docker with 12 char ID",
+			content:  "0::/docker/abcdef123456",
+			expected: "abcdef123456",
+		},
+		{
+			name:     "containerd with ID",
+			content:  "0::/containerd/abcdef123456",
+			expected: "abcdef123456",
+		},
+		{
+			name:     "docker with short ID (less than 12)",
+			content:  "0::/docker/abc",
+			expected: "",
+		},
+		{
+			name:     "no container indicators",
+			content:  "0::/user.slice/user-1000.slice",
+			expected: "",
+		},
+		{
+			name:     "empty content",
+			content:  "",
+			expected: "",
+		},
+		{
+			name:     "kubepods - no ID extraction",
+			content:  "0::/kubepods/besteffort/pod123",
+			expected: "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := extractContainerIDFromContent(tt.content)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
+
 func TestIsRunningInContainer_ConcurrentAccess(t *testing.T) {
 	// Test thread safety with concurrent calls.
 	t.Setenv("RUNNING_IN_CONTAINER", "true")