Merge pull request 'Resolve a group's order rules by precedence, not as a cycle (hq issue 309)' (#134) from fix/309-an-order-rule-yields-to-a-stronger-one into main

cmd/mesh-controller/delivery.go

@@ -168,11 +168,22 @@ func (m orderMember) pull() link.PullUpdated {
 		ModuleDirs: m.ModuleDirs, ModuleDirsSaid: m.ModuleDirsSaid}
 }
 
-// orderPair is one "before" among a group's members, and why.
+// orderPair is one "before" among a group's members, why, and the rules it won over.
 type orderPair struct {
 	Before string `json:"before"`
 	After  string `json:"after"`
 	Why    string `json:"why"`
+	// Over is the rules that ordered the pair the other way and lost to Why, by precedence (novox/hq ADR
+	// 0249): said, so a person reading the order sees which rule won.
+	Over []string `json:"over,omitempty"`
+}
+
+// orderContradiction is two members that rules of one rank order both ways: no precedence resolves it, so
+// the group is refused, the rules named, and Said says how a person declares the order.
+type orderContradiction struct {
+	Members []string    `json:"members"`
+	Rules   []orderPair `json:"rules"`
+	Said    string      `json:"said"`
 }
 
 // orderReach is what a member moves, as the planner says.
@@ -183,12 +194,14 @@ type orderReach struct {
 	Manifests  []string `json:"manifests,omitempty"`
 }
 
-// groupOrder is a group's order: the members in it, every pair and why, and the cycle when there is one.
+// groupOrder is a group's order: the members in it, every pair and why, and the cycle when there is one —
+// with each contradiction no precedence resolves.
 type groupOrder struct {
-	Order []string              `json:"order"`
-	Pairs []orderPair           `json:"pairs,omitempty"`
-	Cycle []string              `json:"cycle,omitempty"`
-	Reach map[string]orderReach `json:"reach,omitempty"`
+	Order          []string              `json:"order"`
+	Pairs          []orderPair           `json:"pairs,omitempty"`
+	Cycle          []string              `json:"cycle,omitempty"`
+	Contradictions []orderContradiction  `json:"contradictions,omitempty"`
+	Reach          map[string]orderReach `json:"reach,omitempty"`
 }
 
 // The reasons a pair is ordered, in the words the delivery plan shows.
@@ -199,7 +212,23 @@ const (
 	orderEngineFirst = "engine before controller"
 )
 
-// orderOf is a group's order (novox/hq ADR 0239 decision 4), pure. A member goes before another when:
+// orderRank is a rule's precedence when two rules order one pair both ways (novox/hq ADR 0249), the lower
+// winning: what a person declared; then what the graph and the change say (built by, version skew); then
+// the rollout default, which infers a direction from the modules' names alone.
+func orderRank(why string) int {
+	switch why {
+	case orderDeclared:
+		return 0
+	case orderBuiltBy, orderVersionSkew:
+		return 1
+	case orderEngineFirst:
+		return 2
+	}
+	return 3
+}
+
+// orderOf is a group's order (novox/hq ADR 0239 decision 4, ADR 0249), pure. A member goes before another
+// when:
 //
 //   - its pull request is named in the other's `after:` lines (declared);
 //   - it moves a module the other's moved modules are built by, stand on, package or declare (built by);
@@ -209,24 +238,28 @@ const (
 //     does not yet grant, and a controller sends nothing an older engine would refuse — ADR 0236's rollout
 //     order).
 //
-// Otherwise, by repository then id, so every reading gives one order. A pair both ways is a cycle: the
-// members in it are named, and none of them is ordered.
+// When rules order one pair both ways, the one of higher precedence wins (orderRank) and the pair says
+// what it won over. Rules of one rank both ways are a contradiction: both are kept as pairs, the members
+// are the cycle, and the contradiction says how to declare the order. Otherwise, by repository then id, so
+// every reading gives one order. A cycle through three members or more is named by its members, and none
+// of them is ordered.
 func orderOf(members []orderMember, reach map[string]orderReach, edges []inventory.Edge) groupOrder {
 	out := groupOrder{Reach: reach}
 	byID := map[string]orderMember{}
 	for _, m := range members {
 		byID[m.ID] = m
 	}
+	var claims []orderPair
 	add := func(before, after, why string) {
 		if before == after {
 			return
 		}
-		for _, p := range out.Pairs {
-			if p.Before == before && p.After == after {
+		for _, p := range claims {
+			if p.Before == before && p.After == after && p.Why == why {
 				return
 			}
 		}
-		out.Pairs = append(out.Pairs, orderPair{Before: before, After: after, Why: why})
+		claims = append(claims, orderPair{Before: before, After: after, Why: why})
 	}
 	named := func(said, repository string) bool {
 		said = strings.TrimSuffix(strings.TrimSpace(said), ".git")
@@ -263,6 +296,7 @@ func orderOf(members []orderMember, reach map[string]orderReach, edges []invento
 			}
 		}
 	}
+	out.Pairs, out.Contradictions = resolveClaims(claims, byID)
 	sort.Slice(out.Pairs, func(i, j int) bool {
 		if out.Pairs[i].Before != out.Pairs[j].Before {
 			return out.Pairs[i].Before < out.Pairs[j].Before
@@ -312,6 +346,89 @@ func orderOf(members []orderMember, reach map[string]orderReach, edges []invento
 	return out
 }
 
+// orderRules is the rules in the order a pair names them when more than one says the same.
+var orderRules = []string{orderDeclared, orderBuiltBy, orderVersionSkew, orderEngineFirst}
+
+// resolveClaims is the pairs the rules' claims come to (novox/hq ADR 0249): for each two members, the claims
+// of the highest precedence decide, and the claims they overrule are said on the pair. Claims of one rank
+// both ways are a contradiction: both pairs are kept, so the walk finds the cycle, and the contradiction
+// says how a person declares the order.
+func resolveClaims(claims []orderPair, byID map[string]orderMember) ([]orderPair, []orderContradiction) {
+	byTwo := map[[2]string][]orderPair{}
+	var keys [][2]string
+	for _, c := range claims {
+		k := [2]string{min(c.Before, c.After), max(c.Before, c.After)}
+		if _, ok := byTwo[k]; !ok {
+			keys = append(keys, k)
+		}
+		byTwo[k] = append(byTwo[k], c)
+	}
+	slices.SortFunc(keys, func(a, b [2]string) int {
+		if c := strings.Compare(a[0], b[0]); c != 0 {
+			return c
+		}
+		return strings.Compare(a[1], b[1])
+	})
+	ruleIndex := func(why string) int { return slices.Index(orderRules, why) }
+	var pairs []orderPair
+	var contradictions []orderContradiction
+	for _, k := range keys {
+		cs := byTwo[k]
+		slices.SortStableFunc(cs, func(a, b orderPair) int { return ruleIndex(a.Why) - ruleIndex(b.Why) })
+		top := orderRank(cs[0].Why)
+		var won []orderPair
+		ways := map[string]bool{}
+		for _, c := range cs {
+			if orderRank(c.Why) == top {
+				won = append(won, c)
+				ways[c.Before] = true
+			}
+		}
+		if len(ways) == 1 {
+			p := orderPair{Before: won[0].Before, After: won[0].After, Why: won[0].Why}
+			for _, c := range cs {
+				if c.Before != p.Before && !slices.Contains(p.Over, c.Why) {
+					p.Over = append(p.Over, c.Why)
+				}
+			}
+			pairs = append(pairs, p)
+			continue
+		}
+		// One rank both ways: no precedence resolves it.
+		var first, second *orderPair
+		var said []string
+		for i := range won {
+			c := won[i]
+			if c.Before == k[0] && first == nil {
+				first = &won[i]
+			}
+			if c.Before == k[1] && second == nil {
+				second = &won[i]
+			}
+			said = append(said, fmt.Sprintf("%s puts %s first", c.Why, c.Before))
+		}
+		pairs = append(pairs, orderPair{Before: first.Before, After: first.After, Why: first.Why},
+			orderPair{Before: second.Before, After: second.After, Why: second.Why})
+		a, b := byID[k[0]], byID[k[1]]
+		how := fmt.Sprintf("neither outranks the other: declare the order with a line `after: %s` in %s's "+
+			"description, or `after: %s` in %s's", a.Repository, pullName(b), b.Repository, pullName(a))
+		if top == orderRank(orderDeclared) {
+			how = "each pull request declares it goes after the other: remove one of the `after:` lines"
+		}
+		contradictions = append(contradictions, orderContradiction{Members: []string{k[0], k[1]}, Rules: won,
+			Said: fmt.Sprintf("%s and %s: %s; %s", k[0], k[1], strings.Join(said, ", "), how)})
+	}
+	return pairs, contradictions
+}
+
+// pullName is a member as a person finds it: its pull request, or its id when no number was said.
+func pullName(m orderMember) string {
+	if m.Number > 0 {
+		return fmt.Sprintf("%s#%d", m.Repository, m.Number)
+	}
+	return m.ID
+}
+
 // reachOfMembers is each member's reach, as the planner says it.
 func reachOfMembers(members []orderMember, entries []inventory.Entry, read map[string][]inventory.ReadRepository,
 	edges []inventory.Edge) map[string]orderReach {

cmd/mesh-controller/delivery_test.go

@@ -3,6 +3,7 @@ package main
 import (
 	"encoding/json"
 	"reflect"
+	"slices"
 	"strings"
 	"testing"
 	"time"
@@ -63,11 +64,82 @@ func TestAGroupIsOrderedByTheGraphAndWhatItsPullRequestsSay(t *testing.T) {
 		t.Fatalf("another reading ordered %v", again.Order)
 	}
 
-	// A declared order against an inferred one is a cycle: named, and nothing ordered.
+	// A declared order against an inferred one wins (novox/hq ADR 0249), and the pair says what it won over.
 	members[1].After = []string{"mesh-catalog"}
 	got = orderOf(members, reach, edges)
-	if !reflect.DeepEqual(got.Cycle, []string{"cat", "ctl"}) {
-		t.Fatalf("the cycle is %v, ordered %v", got.Cycle, got.Order)
+	if len(got.Cycle) > 0 || len(got.Contradictions) > 0 {
+		t.Fatalf("a declared order did not win: cycle %v, %+v", got.Cycle, got.Contradictions)
+	}
+	if want := []string{"agent", "lab", "app", "cat", "host", "ctl"}; !reflect.DeepEqual(got.Order, want) {
+		t.Fatalf("ordered %v, wanted %v", got.Order, want)
+	}
+	if !slices.ContainsFunc(got.Pairs, func(p orderPair) bool {
+		return p.Before == "cat" && p.After == "ctl" && p.Why == orderDeclared && reflect.DeepEqual(p.Over, []string{orderVersionSkew})
+	}) || slices.ContainsFunc(got.Pairs, func(p orderPair) bool { return p.Before == "ctl" && p.After == "cat" }) {
+		t.Fatalf("the declared pair does not say it won over version skew: %+v", got.Pairs)
+	}
+
+	// Declared both ways is a contradiction no precedence resolves: a cycle, and how to end it.
+	members[0].After = []string{"novox/mesh-controller"}
+	got = orderOf(members, reach, edges)
+	if !reflect.DeepEqual(got.Cycle, []string{"cat", "ctl"}) || len(got.Contradictions) != 1 {
+		t.Fatalf("the cycle is %v, ordered %v, contradictions %+v", got.Cycle, got.Order, got.Contradictions)
+	}
+	if c := got.Contradictions[0]; !reflect.DeepEqual(c.Members, []string{"cat", "ctl"}) ||
+		!strings.Contains(c.Said, "remove one of the `after:` lines") {
+		t.Fatalf("the contradiction is said %+v", c)
+	}
+}
+
+// novox/hq ADR 0249: when two rules order one pair both ways, the rule of higher precedence wins and the
+// pair says what it overruled; rules of one rank both ways are refused, naming both and how to declare.
+func TestAnOrderRuleYieldsToAStrongerOne(t *testing.T) {
+	members := []orderMember{
+		{ID: "ctl", Repository: "novox/mesh-controller", Number: 7},
+		{ID: "host", Repository: "novox/mesh-host", Number: 9},
+	}
+	reach := map[string]orderReach{
+		"ctl":  {Moved: []string{"mesh-controller", "build-agent"}},
+		"host": {Moved: []string{"mesh-host"}},
+	}
+	builtBy := []inventory.Edge{{From: "mesh-host", To: "build-agent", Kind: inventory.EdgeBuiltBy}}
+
+	// Built by outranks engine before controller.
+	got := orderOf(members, reach, builtBy)
+	if !reflect.DeepEqual(got.Order, []string{"ctl", "host"}) || len(got.Cycle) > 0 {
+		t.Fatalf("ordered %v, cycle %v", got.Order, got.Cycle)
+	}
+	if want := []orderPair{{Before: "ctl", After: "host", Why: orderBuiltBy, Over: []string{orderEngineFirst}}}; !reflect.DeepEqual(got.Pairs, want) {
+		t.Fatalf("pairs %+v, wanted %+v", got.Pairs, want)
+	}
+
+	// A declared line outranks both.
+	members[0].After = []string{"mesh-host"}
+	got = orderOf(members, reach, builtBy)
+	if !reflect.DeepEqual(got.Order, []string{"host", "ctl"}) || len(got.Cycle) > 0 ||
+		!reflect.DeepEqual(got.Pairs[0].Over, []string{orderBuiltBy}) || got.Pairs[0].Why != orderDeclared {
+		t.Fatalf("a declared order did not win: %v %+v", got.Order, got.Pairs)
+	}
+
+	// Without the build agent, engine before controller stands alone.
+	members[0].After = nil
+	got = orderOf(members, map[string]orderReach{"ctl": {Moved: []string{"mesh-controller"}}, "host": reach["host"]}, builtBy)
+	if !reflect.DeepEqual(got.Order, []string{"host", "ctl"}) || got.Pairs[0].Why != orderEngineFirst || len(got.Pairs[0].Over) > 0 {
+		t.Fatalf("engine before controller alone: %v %+v", got.Order, got.Pairs)
+	}
+
+	// Built by both ways is one rank against itself: refused, both rules named, and how to declare.
+	both := append(builtBy, inventory.Edge{From: "build-agent", To: "mesh-host", Kind: inventory.EdgeStandsOn})
+	got = orderOf(members, reach, both)
+	if !reflect.DeepEqual(got.Cycle, []string{"ctl", "host"}) || len(got.Order) > 0 || len(got.Contradictions) != 1 {
+		t.Fatalf("a contradiction was ordered: %v, cycle %v, %+v", got.Order, got.Cycle, got.Contradictions)
+	}
+	said := got.Contradictions[0].Said
+	for _, w := range []string{"built by puts ctl first", "built by puts host first",
+		"`after: novox/mesh-controller` in novox/mesh-host#9's description", "`after: novox/mesh-host` in novox/mesh-controller#7's"} {
+		if !strings.Contains(said, w) {
+			t.Errorf("the contradiction does not say %q: %s", w, said)
+		}
 	}
 }
 

cmd/mesh-controller/replay309_test.go

@@ -0,0 +1,50 @@
+package main
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/novox/mesh-controller/internal/inventory"
+)
+
+// novox/hq issue 309, replayed with only what orderOf had before its fix, so it can be laid over the older
+// commit. The group `feat/a-machine-joins-through-the-tunnel` (2026-10-08): the controller's pull request
+// moved the controller and the build agent, the node-engine's moved the node-engine, the lab's moved
+// nothing. The build agent builds the node-engine (built by: the controller's member first), and the
+// node-engine goes before the controller (engine before controller: the node-engine's member first). Both
+// rules applied to one pair in opposite directions, and the group was rejected as a cycle; the person merged
+// the controller first by hand. A rule read from the graph outranks the rollout default, so the group is
+// ordered, the controller's member first.
+func TestReplay309(t *testing.T) {
+	members := []orderMember{
+		{ID: "novox/mesh-controller@8bbfdb53db2b", Repository: "novox/mesh-controller", Number: 132},
+		{ID: "novox/mesh-host@86cbebd10f8a", Repository: "novox/mesh-host", Number: 52},
+		{ID: "novox/mesh-lab@e80a4b1642bf", Repository: "novox/mesh-lab", Number: 61},
+	}
+	reach := map[string]orderReach{
+		"novox/mesh-controller@8bbfdb53db2b": {Moved: []string{"build-agent", "mesh-controller", "route-proxy"},
+			Manifests: []string{"module.json"}},
+		"novox/mesh-host@86cbebd10f8a": {Moved: []string{"mesh-host"}},
+		"novox/mesh-lab@e80a4b1642bf":  {},
+	}
+	// Every source-built module is built by the build seat's holder; the holder follows the controller.
+	edges := []inventory.Edge{
+		{From: "build-agent", To: "mesh-controller", Kind: inventory.EdgeWorkerOf},
+		{From: "mesh-controller", To: "build-agent", Kind: inventory.EdgeBuiltBy},
+		{From: "mesh-host", To: "build-agent", Kind: inventory.EdgeBuiltBy},
+		{From: "route-proxy", To: "build-agent", Kind: inventory.EdgeBuiltBy},
+	}
+	got := orderOf(members, reach, edges)
+	if len(got.Cycle) > 0 {
+		t.Fatalf("the group is refused as a cycle %v; pairs %+v", got.Cycle, got.Pairs)
+	}
+	want := []string{"novox/mesh-controller@8bbfdb53db2b", "novox/mesh-host@86cbebd10f8a", "novox/mesh-lab@e80a4b1642bf"}
+	if !reflect.DeepEqual(got.Order, want) {
+		t.Fatalf("ordered %v, wanted %v; pairs %+v", got.Order, want, got.Pairs)
+	}
+	for _, p := range got.Pairs {
+		if p.Before == want[1] && p.After == want[0] {
+			t.Fatalf("a pair that lost still stands: %+v", p)
+		}
+	}
+}