refactor(config): add a pure resolver alongside the viper loader

config/resolve_test.go

@@ -0,0 +1,181 @@
+package config
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"gopkg.in/yaml.v3"
+)
+
+// The pure resolver has exactly one job: produce what the viper-based loader
+// produces, for every assignment in every fixture. Running both against the same
+// input and diffing is a stronger check than the goldens — the goldens pin a
+// recorded snapshot, this pins the two implementations to each other, including
+// on inputs nobody thought to record.
+//
+// Once the loader is gone this test goes with it; until then it is what makes
+// the swap provable rather than hopeful.
+
+// rawCourseBody reads a fixture and returns the course name and the raw body
+// under its single top-level key — the input side of the pure resolver.
+func rawCourseBody(t *testing.T, path string) (string, any) {
+	t.Helper()
+
+	data, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatalf("reading fixture: %v", err)
+	}
+	var top map[string]any
+	if err := yaml.Unmarshal(data, &top); err != nil {
+		t.Fatalf("parsing fixture: %v", err)
+	}
+	if len(top) != 1 {
+		t.Fatalf("fixture %s has %d top-level keys, want 1", path, len(top))
+	}
+	for name, body := range top {
+		return name, body
+	}
+	return "", nil
+}
+
+func TestResolveMatchesViperLoader(t *testing.T) {
+	for _, path := range courseFixtures(t) {
+		course := loadCourseFixture(t, path)
+
+		for _, assignment := range concreteAssignments(t, course) {
+			t.Run(course+"/"+assignment, func(t *testing.T) {
+				// viper mutates itself while resolving `extends`, so reload.
+				loadCourseFixture(t, path)
+				want := mustAssignmentConfig(t, course, assignment)
+
+				name, body := rawCourseBody(t, path)
+				got, err := ResolveAssignment(name, body, Globals{GitlabHost: goldenHost}, assignment)
+				if err != nil {
+					t.Fatalf("ResolveAssignment(%q, %q): %v", name, assignment, err)
+				}
+
+				if diff := cmp.Diff(want, got, cmpopts.IgnoreUnexported(openpgpEntityPlaceholder{})); diff != "" {
+					t.Errorf("pure resolver disagrees with the viper loader (-viper +pure):\n%s", diff)
+				}
+			})
+		}
+	}
+}
+
+// openpgpEntityPlaceholder exists only to give cmpopts something to name. No
+// fixture configures a sign key, so Seeder.SignKey is nil throughout and cmp
+// never has to walk an openpgp.Entity.
+type openpgpEntityPlaceholder struct{ _ int }
+
+// The filter arguments are regexps, and they select students and groups the same
+// way in both loaders.
+func TestResolveMatchesViperLoaderWithFilters(t *testing.T) {
+	tests := []struct {
+		course     string
+		assignment string
+		onlyFor    []string
+	}{
+		{"vss", "blatt1", []string{"grp0[1-3]"}},
+		{"vss", "blatt1", []string{"grp01", "grp02"}},
+		{"vss", "blatt0", []string{"s0"}},
+		{"mpd", "blatt10", []string{"s161"}},
+		{"casecourse", "upperkeys", []string{"grp"}},
+		{"vss", "blatt1", []string{"nomatch"}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.course+"/"+tt.assignment, func(t *testing.T) {
+			path := filepath.Join("testdata", "courses", tt.course+".yaml")
+
+			loadCourseFixture(t, path)
+			want := mustAssignmentConfig(t, tt.course, tt.assignment, tt.onlyFor...)
+
+			name, body := rawCourseBody(t, path)
+			got, err := ResolveAssignment(name, body, Globals{GitlabHost: goldenHost}, tt.assignment, tt.onlyFor...)
+			if err != nil {
+				t.Fatalf("ResolveAssignment: %v", err)
+			}
+
+			if diff := cmp.Diff(want, got); diff != "" {
+				t.Errorf("pure resolver disagrees with the viper loader for onlyFor=%v (-viper +pure):\n%s",
+					tt.onlyFor, diff)
+			}
+		})
+	}
+}
+
+// The error paths must agree too, or a config that is rejected today would
+// quietly resolve tomorrow.
+func TestResolveMatchesViperLoaderOnErrors(t *testing.T) {
+	tests := []struct {
+		name       string
+		course     string
+		assignment string
+	}{
+		{"unknown assignment", "edge", "nosuchassignment"},
+		{"abstract assignment", "edge", "base"},
+		{"cyclic extends", "edge", "cyclea"},
+		{"self cycle", "edge", "selfcycle"},
+		{"extends not a string", "edge", "extendsnotstring"},
+		{"extends blank", "edge", "extendsempty"},
+		{"extends missing parent", "edge", "extendsmissing"},
+		{"seeder without cmd", "edge", "seederwithoutcmd"},
+		{"seeder bad signkey", "edge", "seederbadsignkey"},
+		{"invalid whenCommitAdded", "edge", "badwhencommitadded"},
+	}
+
+	const path = "testdata/errors/edge.yaml"
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			loadErrorFixture(t)
+			_, viperErr := GetAssignmentConfig(tt.course, tt.assignment)
+
+			name, body := rawCourseBody(t, path)
+			_, pureErr := ResolveAssignment(name, body, Globals{GitlabHost: goldenHost}, tt.assignment)
+
+			if viperErr == nil {
+				t.Fatalf("viper loader unexpectedly succeeded; fixture or test is wrong")
+			}
+			if pureErr == nil {
+				t.Fatalf("pure resolver accepted %q, which the viper loader rejects with: %v", tt.assignment, viperErr)
+			}
+		})
+	}
+}
+
+// The one place the pure resolver is allowed to differ: it never mutates global
+// state, so resolving in any order gives the same answer. The viper loader
+// writes the merged `extends` result back into the global registry, which is
+// exactly what this replaces.
+func TestResolveIsIndependentOfOrder(t *testing.T) {
+	t.Parallel()
+
+	name, body := rawCourseBody(t, filepath.Join("testdata", "courses", "mpd.yaml"))
+	g := Globals{GitlabHost: goldenHost}
+
+	// blatt12 sits at the end of a five-deep extends chain.
+	first, err := ResolveAssignment(name, body, g, "blatt12")
+	if err != nil {
+		t.Fatalf("resolving blatt12 first: %v", err)
+	}
+
+	// Resolving its ancestors in between must not change the answer.
+	for _, ancestor := range []string{"blatt08", "blatt09", "blatt10", "blatt11"} {
+		if _, err := ResolveAssignment(name, body, g, ancestor); err != nil {
+			t.Fatalf("resolving %s: %v", ancestor, err)
+		}
+	}
+
+	second, err := ResolveAssignment(name, body, g, "blatt12")
+	if err != nil {
+		t.Fatalf("resolving blatt12 again: %v", err)
+	}
+
+	if diff := cmp.Diff(first, second); diff != "" {
+		t.Errorf("resolution depends on order (-first +second):\n%s", diff)
+	}
+}

config/seeder.go

@@ -2,14 +2,9 @@ package config
 
 import (
 	"fmt"
-	"strings"
-	"syscall"
 
-	"github.com/ProtonMail/go-crypto/openpgp"
-	"github.com/logrusorgru/aurora"
 	"github.com/rs/zerolog/log"
 	"github.com/spf13/viper"
-	"golang.org/x/term"
 )
 
 func seeder(assignmentKey string) (*Seeder, error) {
@@ -30,27 +25,9 @@ func seeder(assignmentKey string) (*Seeder, error) {
 		toBranch = tB
 	}
 
-	privKeyString := viper.GetString(assignmentKey + ".seeder.signKey")
-	var entity *openpgp.Entity
-	if privKeyString != "" {
-		entities, err := openpgp.ReadArmoredKeyRing(strings.NewReader(privKeyString))
-		if err != nil {
-			return nil, fmt.Errorf("%s: cannot read seeder.signKey as an armored PGP key ring: %w", assignmentKey, err)
-		}
-		if len(entities) == 0 {
-			return nil, fmt.Errorf("%s: seeder.signKey contains no PGP key", assignmentKey)
-		}
-		if entities[0].PrivateKey == nil {
-			return nil, fmt.Errorf("%s: seeder.signKey contains no private key", assignmentKey)
-		}
-		if entities[0].PrivateKey.Encrypted {
-			fmt.Println(aurora.Blue("Passphrase for signing key is required. Please enter it now:"))
-			passphrase, _ := term.ReadPassword(int(syscall.Stdin))
-			if err := entities[0].PrivateKey.Decrypt(passphrase); err != nil {
-				return nil, fmt.Errorf("%s: cannot decrypt seeder.signKey with the given passphrase: %w", assignmentKey, err)
-			}
-		}
-		entity = entities[0]
+	entity, err := parseSignKey(assignmentKey, viper.GetString(assignmentKey+".seeder.signKey"))
+	if err != nil {
+		return nil, err
 	}
 
 	return &Seeder{

config/signkey.go

@@ -0,0 +1,51 @@
+package config
+
+import (
+	"fmt"
+	"strings"
+	"syscall"
+
+	"github.com/ProtonMail/go-crypto/openpgp"
+	"github.com/logrusorgru/aurora"
+	"golang.org/x/term"
+)
+
+// parseSignKey turns an inline armored PGP private key into an entity, asking
+// for the passphrase on the terminal if it is encrypted.
+//
+// The terminal prompt is why this is worth naming: it blocks on stdin from
+// inside what is otherwise a pure config read. That is survivable in a CLI and
+// fatal in a server, which is the one thing standing between config loading and
+// being usable from a request handler. Both callers reach it only for
+// assignments that actually configure seeder.signKey — no real course file does
+// — so the block is latent, not live.
+//
+// The right fix is to load and decrypt the key in the execution path rather than
+// the parse path, and to prefer a `signKeyFile` over a secret pasted into a
+// config file. Deferred: the seeder is deprecated and unused.
+func parseSignKey(assignmentKey, armored string) (*openpgp.Entity, error) {
+	if armored == "" {
+		return nil, nil
+	}
+
+	entities, err := openpgp.ReadArmoredKeyRing(strings.NewReader(armored))
+	if err != nil {
+		return nil, fmt.Errorf("%s: cannot read seeder.signKey as an armored PGP key ring: %w", assignmentKey, err)
+	}
+	if len(entities) == 0 {
+		return nil, fmt.Errorf("%s: seeder.signKey contains no PGP key", assignmentKey)
+	}
+	if entities[0].PrivateKey == nil {
+		return nil, fmt.Errorf("%s: seeder.signKey contains no private key", assignmentKey)
+	}
+
+	if entities[0].PrivateKey.Encrypted {
+		fmt.Println(aurora.Blue("Passphrase for signing key is required. Please enter it now:"))
+		passphrase, _ := term.ReadPassword(int(syscall.Stdin))
+		if err := entities[0].PrivateKey.Decrypt(passphrase); err != nil {
+			return nil, fmt.Errorf("%s: cannot decrypt seeder.signKey with the given passphrase: %w", assignmentKey, err)
+		}
+	}
+
+	return entities[0], nil
+}

config/students.go

@@ -25,6 +25,15 @@ func (cfg *AssignmentConfig) SetAccessLevel(level string) {
 	cfg.AccessLevel = accesslevel
 }
 
+// matchesPattern reports whether value matches the regexp pattern. An invalid
+// pattern matches nothing rather than erroring: these come from the CLI's
+// positional arguments, where a plain name is the common case and a typo should
+// simply select no repositories.
+func matchesPattern(pattern, value string) bool {
+	ok, err := regexp.MatchString(pattern, value)
+	return ok && err == nil
+}
+
 func accessLevel(assignmentKey string) AccessLevel {
 	accesslevelIdentifier := viper.GetString(assignmentKey + ".accesslevel")
 
@@ -58,7 +67,7 @@ func students(per Per, course, assignment string, onlyForStudentsOrGroups ...str
 		onlyForStudents := make([]string, 0, len(onlyForStudentsOrGroups))
 		for _, onlyStudent := range onlyForStudentsOrGroups {
 			for _, student := range studs {
-				if ok, err := regexp.MatchString(onlyStudent, student); ok && err == nil {
+				if matchesPattern(onlyStudent, student) {
 					onlyForStudents = append(onlyForStudents, student)
 				}
 			}

config/testdata/courses/defaults.yaml

@@ -0,0 +1,68 @@
+# Synthetic fixture that omits every optional setting, so the defaults are
+# actually exercised.
+#
+# The real course files fill nearly everything in — every assignment has a
+# description, a fromBranch, a toBranch — which means the default branches of the
+# resolver had no coverage at all. A mutation probe proved it: changing the
+# default description to a nonsense string failed no test.
+#
+# Each assignment below drops one more layer.
+defaults:
+  coursepath: defaults/semester
+  semesterpath: ss2026
+
+  # Nothing but a path. Exercises: description "generated by glabs", per student,
+  # accesslevel developer, mergeMethod merge, squashOption default_off,
+  # clone localpath "." and branch "main", no startercode, no release, no seeder.
+  bare:
+    assignmentpath: bare
+
+  # A startercode block with only the required url. Exercises: fromBranch "main",
+  # toBranch "main", templateMessage "Initial", and the branch rule synthesized
+  # from toBranch (with default forced onto the first rule).
+  starterdefaults:
+    assignmentpath: starter-defaults
+    startercode:
+      url: git@gitlab.lrz.de:defaults/startercode.git
+
+  # An empty-ish release block. Exercises: source "develop", target "main".
+  releasedefaults:
+    assignmentpath: release-defaults
+    release:
+      mergeRequest:
+        pipeline: true
+
+  # A seeder with only cmd. Exercises: toBranch "main", no sign key.
+  seederdefaults:
+    assignmentpath: seeder-defaults
+    seeder:
+      cmd: /bin/true
+
+  # A deferred branch with only fromBranch. Exercises: url falling back to the
+  # startercode url, toBranch falling back to fromBranch, orphan defaulting to
+  # true, and the generated orphan message.
+  deferreddefaults:
+    assignmentpath: deferred-defaults
+    startercode:
+      url: git@gitlab.lrz.de:defaults/startercode.git
+    deferredBranches:
+      solution:
+        fromBranch: solution
+
+  # issues with replication on but no numbers. Exercises: issueNumbers [1].
+  issuedefaults:
+    assignmentpath: issue-defaults
+    issues:
+      replicateFromStartercode: true
+
+  # per: group, so the group path is exercised without any group-level overrides.
+  groupdefaults:
+    assignmentpath: group-defaults
+    per: group
+
+  students:
+    - a@example.edu
+
+  groups:
+    grp01:
+      - a@example.edu