fix: harden template application and parity helpers (#19)

* test(parity): harden matrix helper normalization

* fix(templates): isolate extraction and support JSONC metadata

* ci: remove disabled code quality upload

Author: aledbf

.github/workflows/go-cli.yml

@@ -16,7 +16,6 @@ jobs:
     runs-on: ubuntu-latest
     permissions:
       contents: read
-      code-quality: write
     steps:
       - uses: actions/checkout@v7
       - uses: actions/setup-go@v7
@@ -39,19 +38,6 @@ jobs:
       - run: task build
       - run: ./devcontainer --version
 
-      # Convert the Go coverage profile (coverage.out) to Cobertura XML for
-      # GitHub's native Code Quality feature.
-      - name: Convert coverage to Cobertura XML
-        run: |
-          go install github.com/boumenot/gocover-cobertura@latest
-          gocover-cobertura < coverage.out > coverage.xml
-      - name: Upload coverage report
-        uses: actions/upload-code-coverage@v1
-        with:
-          file: coverage.xml
-          language: Go
-          label: code-coverage/go
-
       # Binary covdata for the cross-lane merge (coverage-report job).
       - uses: actions/upload-artifact@v7
         if: always()

internal/cli/code_quality_workflow_test.go

@@ -0,0 +1,23 @@
+package cli
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+func TestGoCLIWorkflowDoesNotUploadDisabledCodeQualityCoverage(t *testing.T) {
+	workflowPath := filepath.Join("..", "..", ".github", "workflows", "go-cli.yml")
+	data, err := os.ReadFile(workflowPath)
+	if err != nil {
+		t.Fatalf("read %s: %v", workflowPath, err)
+	}
+
+	workflow := string(data)
+	for _, disabledSetting := range []string{"actions/upload-code-coverage", "code-quality: write"} {
+		if strings.Contains(workflow, disabledSetting) {
+			t.Errorf("%s still contains disabled Code Quality setting %q", workflowPath, disabledSetting)
+		}
+	}
+}

internal/cli/parity_matrix_helpers_test.go

@@ -168,6 +168,22 @@ func TestExtractCLIResultEnv_EmbeddedJSON(t *testing.T) {
 	}
 }
 
+func TestNormalizeRequired(t *testing.T) {
+	got := normalizeRequired("One of --workspace-folder or --workspace-folder-data is required.")
+	want := "workspace-folder,workspace-folder-data"
+	if got != want {
+		t.Fatalf("normalizeRequired() = %q, want %q", got, want)
+	}
+}
+
+func TestComposeProjectName(t *testing.T) {
+	got := composeProjectName("Build_Feature-1 / alpine")
+	want := "dcbuild_feature-1alpine"
+	if got != want {
+		t.Fatalf("composeProjectName() = %q, want %q", got, want)
+	}
+}
+
 // TestInShardPartitions proves the shard split is a proper partition: with N
 // shards, every case is claimed by exactly one shard and the union is the whole
 // set (no case dropped, none run twice).

internal/cli/parity_matrix_test.go

@@ -921,6 +921,9 @@ var reChoiceYargs = regexp.MustCompile(`(?m)Argument:\s*([^,]+),\s*Given:\s*"([^
 var reChoiceGo = regexp.MustCompile(`(?m)Invalid value "([^"]+)" for --([^.\s]+)\.\s*Choose from:\s*(.+)$`)
 var reInvalidMode = regexp.MustCompile(`(?m)Invalid mode "([^"]+)".*Choose from:\s*(.+)$`)
 var reSetupEnv = regexp.MustCompile(`^([A-Za-z_][A-Za-z0-9_]*)=(.*)$`)
+var reRequiredPrefix = regexp.MustCompile(`(?i)^One of\s+`)
+var reRequiredSuffix = regexp.MustCompile(`(?i)\s+is required\.?$`)
+var reRequiredSplit = regexp.MustCompile(`\s+or\s+|,\s*`)
 
 func matchChoiceYargs(text string) string {
 	match := reChoiceYargs.FindStringSubmatch(text)
@@ -968,9 +971,9 @@ func normalizeChoices(raw string) string {
 }
 
 func normalizeRequired(raw string) string {
-	raw = regexp.MustCompile(`(?i)^One of\s+`).ReplaceAllString(raw, "")
-	raw = regexp.MustCompile(`(?i)\s+is required\.?$`).ReplaceAllString(raw, "")
-	parts := regexp.MustCompile(`\s+or\s+|,\s*`).Split(raw, -1)
+	raw = reRequiredPrefix.ReplaceAllString(raw, "")
+	raw = reRequiredSuffix.ReplaceAllString(raw, "")
+	parts := reRequiredSplit.Split(raw, -1)
 	var clean []string
 	for _, p := range parts {
 		p = strings.TrimSpace(strings.TrimPrefix(p, "--"))
@@ -1089,13 +1092,17 @@ func composeProjectName(caseID string) string {
 	var b strings.Builder
 	b.WriteString("dc")
 	for _, r := range strings.ToLower(caseID) {
-		if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') {
+		if isComposeProjectNameRune(r) {
 			b.WriteRune(r)
 		}
 	}
 	return b.String()
 }
 
+func isComposeProjectNameRune(r rune) bool {
+	return (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' || r == '_'
+}
+
 func sanitizeEnvValue(value string) string {
 	replacer := strings.NewReplacer("/", "-", " ", "-", ":", "-", "\t", "-", "\n", "-")
 	return replacer.Replace(value)

internal/templates/apply.go

@@ -60,12 +60,19 @@ func FetchAndApply(params ApplyParams, selected SelectedTemplate) ([]string, err
 		return nil, fmt.Errorf("fetch template blob: %w", err)
 	}
 
-	// Extract to temp dir
-	tmpDir := params.TmpDir
-	if tmpDir == "" {
-		tmpDir = os.TempDir()
+	// Extract to a unique temporary directory when the caller did not provide
+	// one. Caller-provided directories retain the stable per-template layout and
+	// remain caller-owned.
+	var extractDir string
+	if params.TmpDir == "" {
+		extractDir, err = os.MkdirTemp("", "devcontainer-template-")
+		if err != nil {
+			return nil, fmt.Errorf("create extract dir: %w", err)
+		}
+		defer os.RemoveAll(extractDir)
+	} else {
+		extractDir = filepath.Join(params.TmpDir, "template-"+ref.ID)
 	}
-	extractDir := filepath.Join(tmpDir, "template-"+ref.ID)
 	if err := fsys.MkdirAll(extractDir); err != nil {
 		return nil, fmt.Errorf("create extract dir: %w", err)
 	}
@@ -191,7 +198,9 @@ func mergeFeatures(fsys pfs.FS, workspaceFolder string, featureOpts []TemplateFe
 		return fmt.Errorf("parse %s: %w", configPath, stdErr)
 	}
 	var config map[string]json.RawMessage
-	json.Unmarshal(stdData, &config)
+	if err := json.Unmarshal(stdData, &config); err != nil {
+		return fmt.Errorf("unmarshal %s: %w", configPath, err)
+	}
 	existing := map[string]bool{}
 	_, hasFeatures := config["features"]
 	if hasFeatures {
@@ -262,8 +271,12 @@ func applyOptionDefaults(fsys pfs.FS, extractDir string, userOptions map[string]
 	if err != nil {
 		return merged
 	}
+	standardized, err := hujson.Standardize(data)
+	if err != nil {
+		return merged
+	}
 	var meta TemplateMetadata
-	if err := json.Unmarshal(data, &meta); err != nil {
+	if err := json.Unmarshal(standardized, &meta); err != nil {
 		return merged
 	}
 	for key, raw := range meta.Options {

internal/templates/apply_fetch_test.go

@@ -194,6 +194,76 @@ func TestFetchAndApply_Success(t *testing.T) {
 	}
 }
 
+func TestFetchAndApply_DefaultTempDirIsRemoved(t *testing.T) {
+	tests := []struct {
+		name    string
+		blob    func(*testing.T) []byte
+		wantErr bool
+	}{
+		{
+			name: "success",
+			blob: func(t *testing.T) []byte {
+				return buildTemplateTarGz(t, templateEntries())
+			},
+		},
+		{
+			name: "extraction error",
+			blob: func(*testing.T) []byte {
+				return []byte("not a valid gzip tarball")
+			},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tempRoot := t.TempDir()
+			t.Setenv("TMPDIR", tempRoot)
+			params := ApplyParams{
+				OCIClient:       &fakeTemplateRegistry{blob: tt.blob(t)},
+				FS:              pfs.OSFS{},
+				Logger:          log.Null,
+				WorkspaceFolder: t.TempDir(),
+			}
+
+			_, err := FetchAndApply(params, SelectedTemplate{ID: "ghcr.io/devcontainers/templates/sample:1"})
+			if (err != nil) != tt.wantErr {
+				t.Fatalf("FetchAndApply() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			entries, readErr := os.ReadDir(tempRoot)
+			if readErr != nil {
+				t.Fatal(readErr)
+			}
+			if len(entries) != 0 {
+				t.Fatalf("default temp root contains %v after FetchAndApply", entries)
+			}
+		})
+	}
+}
+
+func TestFetchAndApply_CallerTempDirIsPreserved(t *testing.T) {
+	tempRoot := t.TempDir()
+	params := ApplyParams{
+		OCIClient:       &fakeTemplateRegistry{blob: buildTemplateTarGz(t, templateEntries())},
+		FS:              pfs.OSFS{},
+		Logger:          log.Null,
+		WorkspaceFolder: t.TempDir(),
+		TmpDir:          tempRoot,
+	}
+
+	_, err := FetchAndApply(params, SelectedTemplate{ID: "ghcr.io/devcontainers/templates/sample:1"})
+	if err != nil {
+		t.Fatalf("FetchAndApply: %v", err)
+	}
+	entries, err := os.ReadDir(tempRoot)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(entries) != 1 || entries[0].Name() != "template-sample" {
+		t.Fatalf("caller temp root contains %v, want preserved template-sample directory", entries)
+	}
+}
+
 // TestFetchAndApply_PartialWorkspaceWrite covers the partial-write risk: a WriteFile
 // that fails mid-Walk. The first workspace file is written, the second fails,
 // and the error must propagate (wrapped) instead of silently leaving a partial

internal/templates/apply_pure_test.go

@@ -60,6 +60,25 @@ func TestApplyOptionDefaults(t *testing.T) {
 	}
 }
 
+func TestApplyOptionDefaults_JSONCMetadata(t *testing.T) {
+	dir := t.TempDir()
+	metadata := []byte(`{
+		// Template metadata permits JSON with comments and trailing commas.
+		"id": "x",
+		"options": {
+			"imageVariant": { "type": "string", "default": "bookworm", },
+		},
+	}`)
+	if err := os.WriteFile(filepath.Join(dir, "devcontainer-template.json"), metadata, 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	got := applyOptionDefaults(pfs.OSFS{}, dir, nil, log.Null)
+	if got["imageVariant"] != "bookworm" {
+		t.Fatalf("imageVariant = %q, want JSONC default %q", got["imageVariant"], "bookworm")
+	}
+}
+
 func TestApplyOptionDefaults_NoMetadata(t *testing.T) {
 	// Missing devcontainer-template.json → returns the user options unchanged.
 	dir := t.TempDir()