Merge pull request 'Refuse a declaring module's mergeable file that names no keys (hq issue 574, ADR 0326 §3)' (#282) from fix/574-a-declaring-module-names-its-files-keys into main

cmd/mesh-controller/check.go

@@ -75,6 +75,16 @@ func moduleCheckFor(paths []string, longestMachine int, out io.Writer) error {
 			failed += len(wrong)
 			faulted[m.Module] = true
 		}
+		// A module that declares a setting names the keys of its mergeable files (novox/hq ADR 0326 §3,
+		// issue 574): judged where a definition is written, as at registration, never where a stored
+		// build is read again.
+		if wrong := catalogue.WrittenSettingsProblems(m); len(wrong) > 0 {
+			for _, p := range wrong {
+				fmt.Fprintf(out, "%s: %s\n", path, p)
+			}
+			failed += len(wrong)
+			faulted[m.Module] = true
+		}
 		if named := catalogue.InstallationProblems(m); len(named) > 0 {
 			for _, p := range named {
 				fmt.Fprintf(out, "%s: %s\n", path, p)

cmd/mesh-controller/modules.go

@@ -1051,6 +1051,11 @@ func namesNoInstallation(m catalogue.Manifest) error {
 	if problems := catalogue.TrustProblems(m); len(problems) > 0 {
 		return fmt.Errorf("%s", strings.Join(problems, "; "))
 	}
+	// And a module that declares a setting names the keys of its mergeable files (novox/hq ADR 0326
+	// §3, issue 574): judged where a definition is written, never where a stored build is read again.
+	if problems := catalogue.WrittenSettingsProblems(m); len(problems) > 0 {
+		return fmt.Errorf("%s", strings.Join(problems, "; "))
+	}
 	named := catalogue.InstallationProblems(m)
 	if len(named) == 0 {
 		return nil

cmd/mesh-controller/written_settings_test.go

@@ -0,0 +1,46 @@
+package main
+
+import (
+	"bytes"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/novox/mesh-controller/internal/catalogue"
+)
+
+// ADR 0326 §3 is judged where a definition is written (novox/hq issue 574): `module check`, and
+// namesNoInstallation, which `module add`, a build taken in and the merge gate's registration run.
+func TestWhereADefinitionIsWrittenADeclaringModuleNamesItsFilesKeys(t *testing.T) {
+	const silent = `{"module":"mail-client","version":"1",
+		"settings":{"prefs":{"kind":"preference","default":"// none","why":"its own defaults until a machine names one"}},
+		"resources":[
+			{"id":"user-js","type":"file","path":"/var/lib/mail-client/user.js","mode":"0644","content":"${setting:prefs}\n"},
+			{"id":"accounts","type":"file","path":"/var/lib/mail-client/accounts.json","mode":"0600","merge":"json",
+			 "content":"{}\n"%s}]}`
+	dir := t.TempDir()
+	bad := filepath.Join(dir, "bad.json")
+	good := filepath.Join(dir, "good.json")
+	os.WriteFile(bad, []byte(strings.Replace(silent, "%s", "", 1)), 0o600)
+	os.WriteFile(good, []byte(strings.Replace(silent, "%s", `,"takes-settings":["accounts"]`, 1)), 0o600)
+
+	var out bytes.Buffer
+	if err := moduleCheck([]string{bad}, &out); err == nil ||
+		!strings.Contains(out.String(), "mergeable accounts names no takes-settings") {
+		t.Fatalf("module check passed a declaring module's silent mergeable file: %v\n%s", err, out.String())
+	}
+	out.Reset()
+	if err := moduleCheck([]string{good}, &out); err != nil {
+		t.Fatalf("the file naming its keys was refused: %v\n%s", err, out.String())
+	}
+
+	raw, _ := os.ReadFile(bad)
+	m, err := catalogue.ParseManifest(raw)
+	if err != nil {
+		t.Fatalf("reading it is not where it is judged: %v", err)
+	}
+	if err := namesNoInstallation(m); err == nil || !strings.Contains(err.Error(), "names no takes-settings") {
+		t.Fatalf("registration passed it: %v", err)
+	}
+}

internal/builder/builder.go

@@ -226,6 +226,13 @@ func build(ctx context.Context, run Runner, publish Publisher,
 		say("manifest", "INVALID: %v", err)
 		return Result{}, err
 	}
+	// What is judged only where a definition is written (novox/hq ADR 0326 §3, issue 574): a build is
+	// where a changed definition is first read, so it is refused here before anything is made of it.
+	if problems := catalogue.WrittenSettingsProblems(manifest); len(problems) > 0 {
+		err := fmt.Errorf("%s", strings.Join(problems, "; "))
+		say("manifest", "INVALID: %v", err)
+		return Result{}, err
+	}
 	say("manifest", "%s v%s — %d artifact(s)", manifest.Module, manifest.Version, artifactCount(manifest))
 
 	// What it is made from, for its source fingerprint: the module's own tree first.

internal/catalogue/setting_takes.go

@@ -70,12 +70,42 @@ func namesItsKeys(m Manifest) bool {
 // reads itself, one the file keeps for itself, or one mergeable file naming its keys beside another
 // that names none.
 func TakenSettingsProblems(m Manifest) []string {
-	// Not yet here: refusing a module that declares a setting and owns a mergeable file naming no keys
-	// (ADR 0326 §3). betterbird is one, and the catalogue cannot name its keys before this controller
-	// runs, so that refusal follows once the catalogue does (novox/hq issue 574).
 	return takenSettingsProblems(m)
 }
 
+// WrittenSettingsProblems is ADR 0326 §3, judged only where a definition is written — the module
+// check, `module add`, a build taken in, the builder and the merge gate's registration — and never
+// where a stored one is read again (ParseManifest, a build's manifest at a commit, a module put back):
+// a module that declares a setting names the keys of every mergeable file it owns (novox/hq issue
+// 574, ADR 0164 §11 and §15). A build stored before the rule, such as betterbird's before its accounts
+// file named its keys, must still compose and still be put back; only its next change is held to it.
+//
+// Said only when none of the module's mergeable files names its keys: when one does, §2 already
+// names every file that does not, and a file is not reported twice.
+func WrittenSettingsProblems(m Manifest) []string {
+	if len(m.Settings) == 0 {
+		return nil
+	}
+	var silent []string
+	for _, r := range m.Resources {
+		how, _ := r["merge"].(string)
+		if how == "" {
+			continue
+		}
+		if _, said := r[TakesSettingsField]; said {
+			return nil
+		}
+		silent = append(silent, fmt.Sprint(r["id"]))
+	}
+	var problems []string
+	for _, id := range silent {
+		problems = append(problems, fmt.Sprintf("%s declares settings, and its mergeable %s names no %s: "+
+			"a module that declares a setting names the keys each mergeable file takes, an empty list "+
+			"for none (novox/hq ADR 0326 §3, ADR 0164 §11)", m.Module, id, TakesSettingsField))
+	}
+	return problems
+}
+
 // takenSettingsProblems is every rule of ADR 0326 §2 and §4, judged where a definition is checked and
 // where a stored one is composed.
 func takenSettingsProblems(m Manifest) []string {

internal/catalogue/setting_takes_test.go

@@ -194,3 +194,50 @@ func containsText(list []string, text string) bool {
 	}
 	return false
 }
+
+func TestAModuleThatDeclaresASettingNamesItsFilesKeys(t *testing.T) {
+	// ADR 0326 §3, ADR 0164 §15, novox/hq issue 574: only a module that declares no settings keeps
+	// today's form. One that declares a preference and owns a silent mergeable file is refused where
+	// a definition is written — and read and composed where a stored one is read again, so a build kept
+	// from before the rule is still sent and still put back.
+	declaring := func(app, sidecar []any) Manifest {
+		m := twoFiles(app, sidecar)
+		m.Resources[2]["content"] = "WORKERS=${setting:workers}\n"
+		m.Settings = map[string]SettingDeclaration{"workers": {Kind: KindPreference, Default: 4.0, Why: "a number"}}
+		return m
+	}
+	m := declaring(nil, nil)
+	problems := WrittenSettingsProblems(m)
+	if len(problems) != 2 || !containsText(problems, "mergeable app names no takes-settings") ||
+		!containsText(problems, "mergeable sidecar names no takes-settings") {
+		t.Errorf("where it is written: %v", problems)
+	}
+	if problems := TakenSettingsProblems(m); len(problems) != 0 {
+		t.Errorf("what a stored definition is read by refuses it: %v", problems)
+	}
+	raw, err := json.Marshal(map[string]any{"module": m.Module, "version": "1", "resources": m.Resources,
+		"settings": map[string]any{"workers": map[string]any{"kind": KindPreference, "default": 4, "why": "a number"}}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	read, err := ParseManifest(raw)
+	if err != nil {
+		t.Fatalf("a stored build from before the rule is not read again: %v", err)
+	}
+	if err := JudgeSettings(read, nil, false); err != nil {
+		t.Errorf("a stored definition is composed as before, and was refused: %v", err)
+	}
+
+	if problems := WrittenSettingsProblems(declaring([]any{"server"}, []any{})); len(problems) != 0 {
+		t.Errorf("every mergeable file names its keys, and was refused: %v", problems)
+	}
+	if problems := WrittenSettingsProblems(twoFiles(nil, nil)); len(problems) != 0 {
+		t.Errorf("a module declaring no setting was refused: %v", problems)
+	}
+	// One naming file and one silent: §2 names the silent one, and §3 does not name it again.
+	mixed := declaring([]any{"server"}, nil)
+	if written, taken := WrittenSettingsProblems(mixed), TakenSettingsProblems(mixed); len(written) != 0 || len(taken) != 1 {
+		t.Errorf("the silent file is reported %d times by §3 and %d by §2; want once, by §2: %v %v",
+			len(written), len(taken), written, taken)
+	}
+}

internal/inventory/stored_silent_file_test.go

@@ -0,0 +1,67 @@
+package inventory
+
+import (
+	"context"
+	"encoding/json"
+	"strings"
+	"testing"
+
+	"github.com/novox/mesh-controller/internal/catalogue"
+)
+
+// A build kept from before ADR 0326 §3 is still read and composed (novox/hq issue 574). betterbird's
+// builds from before mesh-catalog 8f80cd4 declare a preference and own a mergeable file that names no
+// keys: refused where a definition is written, but a machine still runs that build and a rollback may
+// put it back, so reading it again — ManifestAt, which a walk, the gate and a release compose from —
+// must not refuse it.
+func TestAStoredBuildWithASilentMergeableFileIsStillReadAndComposed(t *testing.T) {
+	inv := fresh(t)
+	ctx := context.Background()
+	manifest, err := json.Marshal(map[string]any{
+		"module": "mail-client", "version": "1",
+		"settings": map[string]any{"prefs": map[string]any{
+			"kind": catalogue.KindPreference, "default": "// none", "why": "its own defaults until a machine names one"}},
+		"resources": []any{
+			map[string]any{"id": "user-js", "type": "file", "path": "/var/lib/mail-client/user.js", "mode": "0644",
+				"content": "${setting:prefs}\n"},
+			map[string]any{"id": "accounts", "type": "file", "path": "/var/lib/mail-client/accounts.json",
+				"mode": "0600", "merge": "json", "content": "{\"accounts\": {}}\n"},
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	b := aBuild("before-574", "mail-client", "")
+	b.Manifest = manifest
+	if err := inv.RecordBuild(ctx, b); err != nil {
+		t.Fatal(err)
+	}
+
+	m, found, err := inv.ManifestAt(ctx, "mail-client", b.Commit)
+	if err != nil || !found {
+		t.Fatalf("the stored build is not read again: found %v, %v", found, err)
+	}
+	if written := catalogue.WrittenSettingsProblems(m); len(written) != 1 {
+		t.Fatalf("the rule this build predates should name its accounts file where it is written: %v", written)
+	}
+
+	// And composed, in today's form: the accounts file still takes its layer's keys.
+	with := catalogue.Rendering{Settings: catalogue.SettingsBy{"mail-client": {{From: "workstation",
+		Values: map[string]any{"accounts": map[string]any{"work": map[string]any{"email": "a@example.invalid"}}}}}}}
+	if err := catalogue.JudgeSettings(m, with.Settings["mail-client"], false); err != nil {
+		t.Fatalf("the stored build's settings do not compose: %v", err)
+	}
+	out, err := catalogue.Resolution{Node: "workstation", Modules: []catalogue.Manifest{m}}.Declaration(with)
+	if err != nil {
+		t.Fatalf("the stored build is not composed: %v", err)
+	}
+	for _, r := range out {
+		if r["id"] == "mail-client.accounts" {
+			if content, _ := r["content"].(string); !strings.Contains(content, "a@example.invalid") {
+				t.Fatalf("the accounts file did not take its layer: %v", content)
+			}
+			return
+		}
+	}
+	t.Fatalf("the accounts file was not composed: %v", out)
+}

testdata/store-tests

@@ -37,3 +37,4 @@
 ./cmd/mesh-controller TestReplay401
 ./cmd/mesh-controller TestReplay201ThePlansSend
 ./internal/inventory TestAWalksRestIsReadFromEachMachinesOwnSend
+./internal/inventory TestAStoredBuildWithASilentMergeableFileIsStillReadAndComposed