Find a bare or aliased mirror reader, and say what the reader test misses

Review 257 of mesh-controller#315 (hq issue 652): the test that only the audit
and a mirror's writer read a mirror (ADR 0289 §19, ADR 0356 §2) matched only
names qualified as broker. or recordmirror., so a reader inside package broker,
or one importing broker under another name, passed. It now matches by import
path and bare names in the defining packages, proves it on planted readers,
and names what it cannot see.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

internal/recordmirror/readers_test.go

@@ -5,8 +5,10 @@ import (
 	"go/parser"
 	"go/token"
 	"io/fs"
+	"os"
 	"path/filepath"
 	"slices"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -24,17 +26,87 @@ var mayNameAMirror = []string{
 	"internal/catalogue/state.go",
 }
 
+// The packages whose names stand for a mirror, by import path, and the names in each.
+const (
+	brokerPath       = "github.com/novox/mesh-controller/internal/broker"
+	recordmirrorPath = "github.com/novox/mesh-controller/internal/recordmirror"
+)
+
+var mirrorNames = map[string][]string{
+	brokerPath: {"AssignmentsBucket", "DeclaredConfigBucket", "MirrorAssignments", "MirrorDeclaredConfig",
+		"ControllerMirrors"},
+	recordmirrorPath: {"Assignments", "DeclaredConfig", "Mirrors"},
+}
+
 // **Nothing in the controller reads a mirror to decide anything** (ADR 0289 §19 as ADR 0356 §2 changes it): no
-// file of the controller's packages outside its tests names a mirror's bucket, by its Go name or its bus name,
-// but the audit and a mirror's own writer. A reader elsewhere fails here, naming the file and what it named.
+// file of the controller's packages outside its tests names a mirror's bucket but the audit and a mirror's own
+// writer. A reader elsewhere fails here, naming the file and what it named.
+//
+// **What it catches**: a name that stands for a mirror, qualified by the package it comes from under any
+// local name the file imports it as (`broker.AssignmentsBucket`, `b.AssignmentsBucket`); the same names bare
+// in a file of package broker or package recordmirror; and a string literal holding a mirror's bucket name.
+// **What it does not catch**: a bucket name built at run time, such as `BucketName(ControllerSeat,
+// "assignments")` or literals concatenated into one, and a mirror reached through a value passed in from an
+// allowed file. Those are deliberate evasions rather than ordinary code; a reviewer judges them.
 func TestOnlyTheAuditAndAMirrorsWriterReadAMirror(t *testing.T) {
-	names := []string{"AssignmentsBucket", "DeclaredConfigBucket", "MirrorAssignments", "MirrorDeclaredConfig",
-		"ControllerMirrors", "Assignments", "DeclaredConfig", "Mirrors"}
 	var buckets []string
 	for _, m := range broker.ControllerMirrors() {
 		buckets = append(buckets, broker.BucketName(broker.ControllerSeat, m))
 	}
 	root := filepath.Join("..", "..")
+	found := mirrorReaders(t, root, buckets)
+	if len(found) > 0 {
+		t.Fatalf("a package of the controller reads a record mirror, which only the audit and the mirror's "+
+			"writer may (ADR 0289 §19, ADR 0356 §2):\n  %s", strings.Join(found, "\n  "))
+	}
+}
+
+// **The reader test sees what it says it sees**: in a planted tree, each planted reader is named, and an allowed
+// file and a selector on a value are not.
+func TestTheReaderTestFindsPlantedReaders(t *testing.T) {
+	root := t.TempDir()
+	plant := func(path, src string) {
+		full := filepath.Join(root, path)
+		if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(full, []byte(src), 0o644); err != nil {
+			t.Fatal(err)
+		}
+	}
+	plant("cmd/mesh-controller/qualified.go", "package main\n\nimport \""+brokerPath+"\"\n\nvar a = broker.AssignmentsBucket\n")
+	plant("internal/x/aliased.go", "package x\n\nimport b \""+brokerPath+"\"\n\nvar a = b.DeclaredConfigBucket\n")
+	plant("internal/broker/zz_reader.go", "package broker\n\nfunc zzR() string { return AssignmentsBucket }\n")
+	plant("internal/inventory/literal.go", "package inventory\n\nvar a = \"mesh-controller_declared-config\"\n")
+	plant("internal/y/writer.go", "package y\n\nimport \""+recordmirrorPath+"\"\n\nvar a = recordmirror.Assignments\n")
+	// Allowed: a mirror's definition and its writer.
+	plant("internal/broker/controller_buckets.go", "package broker\n\nvar AssignmentsBucket = \"x\"\n")
+	plant("internal/broker/other.go", "package broker\n\nfunc other() string { return \"nothing\" }\n")
+	// A selector on a value (v.AssignmentsBucket) is no use of the mirror, but declaring a field of a mirror's
+	// name in package broker is named, on the safe side; and a bare use inside a call under a selector is found.
+	plant("internal/broker/field.go", "package broker\n\ntype s struct{ AssignmentsBucket string }\n\nfunc f(v s) string { return v.AssignmentsBucket }\n")
+	plant("internal/broker/nested.go", "package broker\n\nfunc g() int { return len(h(DeclaredConfigBucket).x) }\n")
+	found := mirrorReaders(t, root, []string{"mesh-controller_assignments", "mesh-controller_declared-config"})
+	want := []string{
+		"cmd/mesh-controller/qualified.go: broker.AssignmentsBucket",
+		"internal/broker/field.go: AssignmentsBucket",
+		"internal/broker/nested.go: DeclaredConfigBucket",
+		"internal/broker/zz_reader.go: AssignmentsBucket",
+		"internal/inventory/literal.go: \"mesh-controller_declared-config\"",
+		"internal/x/aliased.go: b.DeclaredConfigBucket",
+		"internal/y/writer.go: recordmirror.Assignments",
+	}
+	slices.Sort(found)
+	if !slices.Equal(found, want) {
+		t.Errorf("the planted readers were found as\n  %s\nwant\n  %s", strings.Join(found, "\n  "),
+			strings.Join(want, "\n  "))
+	}
+}
+
+// mirrorReaders is every place under root's cmd and internal, outside tests and the allowed files, that names a
+// mirror.
+func mirrorReaders(t *testing.T, root string, buckets []string) []string {
+	t.Helper()
 	var found []string
 	for _, dir := range []string{"cmd", "internal"} {
 		err := filepath.WalkDir(filepath.Join(root, dir), func(path string, e fs.DirEntry, err error) error {
@@ -53,13 +125,43 @@ func TestOnlyTheAuditAndAMirrorsWriterReadAMirror(t *testing.T) {
 			if err != nil {
 				return err
 			}
+			// Each import's local name, mapped to the package it stands for.
+			imported := map[string]string{}
+			for _, imp := range f.Imports {
+				p, _ := strconv.Unquote(imp.Path.Value)
+				if _, ok := mirrorNames[p]; !ok {
+					continue
+				}
+				local := filepath.Base(p)
+				if imp.Name != nil {
+					local = imp.Name.Name
+				}
+				imported[local] = p
+			}
+			// A file of the package that defines the names may use them bare.
+			var bare []string
+			switch f.Name.Name {
+			case "broker":
+				bare = mirrorNames[brokerPath]
+			case "recordmirror":
+				bare = mirrorNames[recordmirrorPath]
+			}
 			ast.Inspect(f, func(n ast.Node) bool {
 				switch n := n.(type) {
 				case *ast.SelectorExpr:
 					pkg, ok := n.X.(*ast.Ident)
-					if ok && (pkg.Name == "broker" || pkg.Name == "recordmirror") && slices.Contains(names, n.Sel.Name) {
+					if !ok {
+						return true
+					}
+					if p, imp := imported[pkg.Name]; imp && slices.Contains(mirrorNames[p], n.Sel.Name) {
 						found = append(found, rel+": "+pkg.Name+"."+n.Sel.Name)
 					}
+					// Neither a selector's own name nor its package is a bare use.
+					return false
+				case *ast.Ident:
+					if slices.Contains(bare, n.Name) {
+						found = append(found, rel+": "+n.Name)
+					}
 				case *ast.BasicLit:
 					for _, b := range buckets {
 						if n.Kind == token.STRING && strings.Contains(n.Value, b) {
@@ -75,8 +177,5 @@ func TestOnlyTheAuditAndAMirrorsWriterReadAMirror(t *testing.T) {
 			t.Fatal(err)
 		}
 	}
-	if len(found) > 0 {
-		t.Fatalf("a package of the controller reads a record mirror, which only the audit and the mirror's "+
-			"writer may (ADR 0289 §19, ADR 0356 §2):\n  %s", strings.Join(found, "\n  "))
-	}
+	return found
 }