refactor(config): add a pure resolver alongside the viper loader#78
Merged
Conversation
Resolution currently runs on global viper state and writes back to it while resolving `extends`, so a read path mutates a global registry and two courses cannot be resolved concurrently. Add ResolveAssignment as a pure function of its inputs, to be swapped in once it is proven equivalent. Nothing calls it yet. The viper loader is untouched and no existing golden moves. Inheritance stays on the raw map, and that is the load-bearing decision here. A decoded child cannot express "I did not mention pipeline" — its field is false either way, and a struct merge would let that false overwrite the parent's true. Absence only survives as a missing map key, so the merge has to happen before the struct decode. That is why ResolveAssignment takes a course body rather than a *CourseSource: the typed source and the resolution path are two branches out of the same raw map, not a chain. Two places lowercase map keys explicitly, with a comment saying why: viper did it implicitly for every key it loaded, and group and deferred-branch names are data, not field names. They end up in repository names and in `glabs push mpd blatt07 devcontainer`, so keeping the written case would silently target a different project. TestResolveMatchesViperLoader runs both implementations over every assignment in every fixture and diffs the results — 60 cases, plus filter arguments and error paths. That is a stronger check than the goldens: goldens pin a recorded snapshot, this pins the two implementations to each other. It is what makes the swap provable rather than hopeful, and it gets deleted along with the loader. Mutation-probing that test exposed a real gap: changing the default description to a nonsense string failed nothing, because every assignment in every fixture sets one. The same held for most defaults — the real course files fill nearly everything in, so the resolver's default branches had no coverage at all. Adding testdata/courses/defaults.yaml, which omits every optional setting, closes it: all nine defaults are now caught by at least one test. Also lifts the sign-key parsing into config/signkey.go, unchanged. It is the one place config loading blocks on stdin, which is what stands between it and a request handler — worth having under its own name and comment rather than buried in a seeder field read. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Schritt 2b-1. Rein additiv — niemand ruft den neuen Resolver auf, der viper-Pfad ist unangetastet, kein bestehender Golden bewegt sich.
Warum geteilt
2b war der einzige Hochrisiko-Schritt des Plans. Statt Resolver bauen und umschalten in einem PR:
Resolve()neben dem viper-Pfad, plus ein differentieller Test, der beide gegeneinander hält. Null Risiko, und der Beweis entsteht vor dem Umschalten.Die tragende Entscheidung: Vererbung bleibt auf der rohen Map
Beim Entwerfen der Signatur bin ich auf eine Gabelung gestoßen, die der Plan überspringt. Ein dekodiertes Kind kann nicht ausdrücken „ich habe
pipelinenicht erwähnt" — sein Feld ist so oder sofalse, und ein Struct-Merge ließe diesesfalsedastruedes Parents überschreiben. Abwesenheit überlebt nur als fehlender Map-Key, also muss der Merge vor dem Struct-Decode passieren.Deshalb nimmt
ResolveAssignmenteinen Course-Body, kein*CourseSource: das typisierte Quellschema und der Auflösungspfad sind zwei Zweige aus derselben rohen Map, keine Kette. (Genau so zeigt es auch das Plandiagramm — mir war beim Planen nicht klar, wie sehr das die Signatur bestimmt.)Zwei Stellen schreiben Map-Keys explizit klein, mit Begründung im Code: viper tat das implizit für jeden geladenen Key, und Gruppen- und DeferredBranch-Namen sind Daten, keine Feldnamen. Sie landen in Repo-Namen und in
glabs push mpd blatt07 devcontainer— die geschriebene Schreibweise zu behalten würde still ein anderes Projekt treffen.Der Beweis
TestResolveMatchesViperLoaderlässt beide Implementierungen über jedes Assignment jeder Fixture laufen und diffed die Ergebnisse — 60 Fälle, dazu Filter-Argumente und Fehlerpfade.Das ist stärker als die Goldens: die pinnen einen aufgezeichneten Snapshot, dieser Test pinnt die zwei Implementierungen aneinander. Er wird zusammen mit dem alten Loader gelöscht.
Eine echte Lücke, gefunden durch Mutationsproben
Ich habe dem Test nicht geglaubt und ihn mutationsgetestet — den Resolver absichtlich kaputtgemacht und geschaut, ob es auffällt. Ergebnis:
"generated by glabs"→"SABOTAGE"fiel niemandem auf.Der Grund: jedes Assignment in jeder Fixture setzt eine
description. Dasselbe galt für fast alle Defaults — Deine echten Kursdateien füllen praktisch alles aus, die Default-Zweige des Resolvers hatten null Abdeckung.testdata/courses/defaults.yamllässt konsequent jede optionale Einstellung weg und schließt die Lücke. Danach:generated by glabsmainInitial.developmain[1]developertrueNebenbei
config/signkey.goholt das Sign-Key-Parsen unter eigenen Namen — unverändert, nur verschoben. Es ist die einzige Stelle, an der Config-Laden auf stdin blockiert, und genau das steht zwischenconfig/und der Benutzbarkeit aus einem Request-Handler. Das verdient einen eigenen Namen und Kommentar statt in einem Feld-Read vergraben zu sein.Verifikation
go test ./...grün;gofmt,go vet,golangci-lintsauber🤖 Generated with Claude Code