Tell mesh-config what a head declares at every head, so a head that reverts a key drops its held value (issue 1826)

cmd/mesh-controller/checks.go

@@ -191,6 +191,16 @@ func (f following) pullUpdated(ctx context.Context, p link.PullUpdated, w io.Wri
 		scope.Modules, scope.Dependents, scope.New, scope.Manifests = nil, nil, nil, nil
 		composedGate(&direct)
 	}
+	// **Mesh-config is told what the head declares at every announced head of a pull request in a repository that holds
+	// modules** (novox/hq ADR 0502 clauses 2 and 6, issue 1826), also when the head touches no module: a head that reverts
+	// the change must drop the value held for it. Asked of nothing else here.
+	this := pullAtOf(p)
+	if this.Truncated {
+		this.Paths, this.Truncated = truncatedPaths(entries, p.Owner, p.Repo, scope.Modules), false
+	}
+	if scope.gated() || holdsModules(entries, p.Owner, p.Repo) {
+		tellPullDeclares(ctx, w, []pullAt{this})
+	}
 	switch {
 	case !scope.gated() && !scope.Mesh:
 		fmt.Fprintf(w, "%s/%s#%d (%.8s): %s, and the repository is not the mesh's: said, nothing run\n",
@@ -209,12 +219,9 @@ func (f following) pullUpdated(ctx context.Context, p link.PullUpdated, w io.Wri
 	}
 	request.Check.Plan = &plan
 	request.Check.Composed = composed
-	// **A value held for a key this pull request introduces is told and carried** (novox/hq ADR 0502, issue 1826).
-	this := pullAtOf(p)
-	if this.Truncated {
-		this.Paths, this.Truncated = truncatedPaths(entries, p.Owner, p.Repo, scope.Modules), false
+	if !composed {
+		carryHeld(ctx, w, &request, []pullAt{this})
 	}
-	withHeldConfig(ctx, w, &request, []pullAt{this}, !composed)
 	// **The group's other open pull requests are read beside it** (novox/hq ADR 0382 §1).
 	siblingsFrom(&request, p, entries, lines, groupsErr)
 	seat := buildSeatHeld(ctx)

cmd/mesh-controller/pull_declares.go

@@ -68,12 +68,11 @@ func pullKey(owner, repo string, number int) string {
 func (p pullAt) key() string { return pullKey(p.Owner, p.Repo, p.Number) }
 
 // pullDeclarations are the config declarations a pull request's head adds or changes over what it merges into, by
-// module, in the shape of the declared-config mirror. The bool says the pull request changes a module.json that exists
-// at its head: mesh-config is told then, even of no declaration, so what it kept for an earlier head is replaced. A
-// pull request whose paths are truncated is read through the manifests of the modules it moves (the caller's paths).
+// module, in the shape of the declared-config mirror. The bool says the head could be read: false for a pull request whose
+// paths are cut (the caller cuts them to the manifests of the modules it moves) or whose base is unknown, which is then
+// told nothing, never an empty set.
 func pullDeclarations(ctx context.Context, p pullAt) ([]inventory.DeclaredConfigMirrored, bool, error) {
 	var out []inventory.DeclaredConfigMirrored
-	touched := false
 	if p.Truncated || p.Old == "" {
 		return nil, false, nil
 	}
@@ -88,7 +87,6 @@ func pullDeclarations(ctx context.Context, p pullAt) ([]inventory.DeclaredConfig
 		if !exists || now.Module == "" {
 			continue
 		}
-		touched = true
 		was, _, err := manifestCached(ctx, p.Owner, p.Repo, file, p.Old)
 		if err != nil {
 			return nil, false, err
@@ -111,19 +109,21 @@ func pullDeclarations(ctx context.Context, p pullAt) ([]inventory.DeclaredConfig
 			out = append(out, inventory.DeclaredConfigMirrored{Module: now.Module, Settings: changed})
 		}
 	}
-	return out, touched, nil
+	return out, true, nil
 }
 
-// tellPullDeclares tells mesh-config, for each pull request that changes a module.json, what its head declares. A
+// tellPullDeclares tells mesh-config, for each pull request at every announced head, what the head declares, an empty set
+// when it declares nothing new: mesh-config re-judges the values held at every head, so a head that reverts the change
+// drops them (ADR 0502 clause 6). A
 // failure is said to w and the check goes on: the held values are then none (ADR 0502 clause 2).
 func tellPullDeclares(ctx context.Context, w io.Writer, pulls []pullAt) {
 	for _, p := range pulls {
-		mods, touched, err := pullDeclarations(ctx, p)
+		mods, readable, err := pullDeclarations(ctx, p)
 		if err != nil {
 			fmt.Fprintf(w, "%s: what its head declares cannot be read, so mesh-config is told nothing: %v\n", p.key(), err)
 			continue
 		}
-		if !touched {
+		if !readable {
 			continue
 		}
 		declares := map[string]map[string]inventory.DeclaredMirrored{}
@@ -191,7 +191,21 @@ func heldFor(ctx context.Context, w io.Writer, pulls []pullAt) []link.HeldConfig
 // declarations are told.
 func withHeldConfig(ctx context.Context, w io.Writer, request *link.BuildRequest, pulls []pullAt, carry bool) {
 	tellPullDeclares(ctx, w, pulls)
-	if carry && request.Check != nil {
+	if carry {
+		carryHeld(ctx, w, request, pulls)
+	}
+}
+
+// carryHeld puts the approved held values of these pull requests on the check request.
+func carryHeld(ctx context.Context, w io.Writer, request *link.BuildRequest, pulls []pullAt) {
+	if request.Check != nil {
 		request.Check.HeldConfig = heldFor(ctx, w, pulls)
 	}
 }
+
+// holdsModules is whether a module of the mesh is built from this repository.
+func holdsModules(entries []inventory.Entry, owner, repo string) bool {
+	return slices.ContainsFunc(entries, func(e inventory.Entry) bool {
+		return sameRepository(e.Source.Repository, link.SourceMoved{Owner: owner, Repo: repo})
+	})
+}

cmd/mesh-controller/pull_declares_test.go

@@ -98,8 +98,9 @@ func TestAnUnchangedKeyIsNotToldAndAChangedOneIs(t *testing.T) {
 	}
 }
 
-// 1826/AC1: a pull request that changes no module.json, or removes it, tells mesh-config nothing.
-func TestAPullRequestThatDeclaresNothingSendsNothing(t *testing.T) {
+// 1826/AC1: a head that changes no module.json, or removes it, is still told, with declares {}: mesh-config re-judges at every
+// head, so a head that reverts the change drops the value held for it (review round 1, finding 1).
+func TestAHeadThatDeclaresNothingIsToldAsEmpty(t *testing.T) {
 	fakeManifests(t, map[string]*catalogue.Manifest{baseSHA: manifest("n8n", nil), headSHA: manifest("n8n", nil)})
 	calls := fakeConfig(t, nil)
 	p := thePull()
@@ -108,9 +109,25 @@ func TestAPullRequestThatDeclaresNothingSendsNothing(t *testing.T) {
 	p = thePull()
 	p.Removed = p.Paths
 	tellPullDeclares(context.Background(), io.Discard, []pullAt{p})
-	if len(*calls) != 0 {
+	if len(*calls) != 2 {
 		t.Fatalf("mesh-config was told %v", *calls)
 	}
+	for _, c := range *calls {
+		if d, _ := json.Marshal(c["declares"]); string(d) != "{}" || c["head"] != headSHA {
+			t.Fatalf("told %v", c)
+		}
+	}
+}
+
+// 1826/AC1: a head whose paths are cut, or whose base is unknown, cannot be read: nothing is told, never an empty set.
+func TestAHeadThatCannotBeReadIsNotToldAsEmpty(t *testing.T) {
+	calls := fakeConfig(t, nil)
+	p := thePull()
+	p.Truncated = true
+	tellPullDeclares(context.Background(), io.Discard, []pullAt{p})
+	if len(*calls) != 0 {
+		t.Fatalf("told %v", *calls)
+	}
 }
 
 // 1826/AC1: a module.json changed without a declaration changing is told as an empty set, so mesh-config re-judges what it holds.
@@ -182,3 +199,11 @@ func TestTheCheckRequestCarriesTheHeldValues(t *testing.T) {
 		t.Fatalf("a composed member carries %+v", request.Check.HeldConfig)
 	}
 }
+
+// 1826/AC1: a repository that builds a module of the mesh holds modules; another does not.
+func TestARepositoryThatBuildsAModuleHoldsModules(t *testing.T) {
+	entries := []inventory.Entry{{Manifest: catalogue.Manifest{Module: "a"}, Source: inventory.Source{Repository: "novox/mesh-catalog", Path: "modules/a"}}}
+	if !holdsModules(entries, "novox", "mesh-catalog") || holdsModules(entries, "novox", "hq") {
+		t.Fatal("wrong")
+	}
+}