From 4375ed8b2cf0148e4aa9eb0bc67fedd35b45e27e Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 19:18:11 +0200 Subject: [PATCH 1/8] Move the builder's shared build-run parts into go/delivery/buildrun, so the checker shares them and no module keeps a copy (issue 1671) --- go/delivery/buildrun/choose.go | 88 ++++++++++++ go/delivery/buildrun/choose_test.go | 99 ++++++++++++++ go/delivery/buildrun/doc.go | 5 + go/delivery/buildrun/have.go | 56 ++++++++ go/delivery/buildrun/modules.go | 7 + go/delivery/buildrun/places.go | 171 ++++++++++++++++++++++++ go/delivery/buildrun/places_test.go | 126 +++++++++++++++++ go/delivery/buildrun/presence.go | 53 ++++++++ go/delivery/buildrun/presence_test.go | 43 ++++++ go/delivery/buildrun/steps.go | 67 ++++++++++ go/delivery/buildrun/steps_test.go | 116 ++++++++++++++++ go/delivery/buildrun/toolchains.go | 64 +++++++++ go/delivery/buildrun/toolchains_test.go | 51 +++++++ 13 files changed, 946 insertions(+) create mode 100644 go/delivery/buildrun/choose.go create mode 100644 go/delivery/buildrun/choose_test.go create mode 100644 go/delivery/buildrun/doc.go create mode 100644 go/delivery/buildrun/have.go create mode 100644 go/delivery/buildrun/modules.go create mode 100644 go/delivery/buildrun/places.go create mode 100644 go/delivery/buildrun/places_test.go create mode 100644 go/delivery/buildrun/presence.go create mode 100644 go/delivery/buildrun/presence_test.go create mode 100644 go/delivery/buildrun/steps.go create mode 100644 go/delivery/buildrun/steps_test.go create mode 100644 go/delivery/buildrun/toolchains.go create mode 100644 go/delivery/buildrun/toolchains_test.go diff --git a/go/delivery/buildrun/choose.go b/go/delivery/buildrun/choose.go new file mode 100644 index 0000000..06aea83 --- /dev/null +++ b/go/delivery/buildrun/choose.go @@ -0,0 +1,88 @@ +package buildrun + +import ( + "fmt" + "sort" + "strings" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +// ChooseNodes picks up to want build nodes for a language (ADR 0478 clause 3): the nodes whose toolchains key reports the +// version the nodes agree on, the ones with room under their build capacity first (places), then the fewest lanes the +// part has running or chosen there, a tie broken by the node's name. A place is reserved on each node chosen, in the +// same step as the choice, so builds choosing at once do not choose the same full node while another has room; the +// caller takes the place (Places.Acquire) or gives it up (Places.Cancel). A node at its capacity is still chosen when +// the others are too (fewer than want have room): its lane waits for a place. A node qualifies only when qualifies is true +// of it (online and its build module healthy; nil means every node does). It answers the toolchain all the reporting nodes +// agree on, qualifying or not, as the indexer computes it. When nothing is reported, or the nodes report different +// versions (so the indexer agreed none), it fails naming the language and why; when no reporting node qualifies it fails +// naming the nodes. A node whose build module refuses on its toolchain refuses the step, and the build fails naming it. +func ChooseNodes(language string, entries []delivery.NodeToolchain, places *Places, qualifies func(node string) bool, want int) ([]string, delivery.Toolchain2, error) { + tc, none := delivery.AgreedToolchain(entries) + if none != "" { + return nil, delivery.Toolchain2{}, fmt.Errorf("no %s build node qualifies: %s", language, none) + } + nodes, err := choose(language, entries, places, qualifies, want) + return nodes, tc, err +} + +// ChooseAnyNodes is ChooseNodes for artifacts that need no toolchain, such as an archive (ADR 0478 clause 1): the +// build module's nodes agree on nothing, so any node of the build module that qualifies is chosen, by capacity and load only. +func ChooseAnyNodes(language string, entries []delivery.NodeToolchain, places *Places, qualifies func(node string) bool, want int) ([]string, error) { + return choose(language, entries, places, qualifies, want) +} + +func choose(language string, entries []delivery.NodeToolchain, places *Places, qualifies func(node string) bool, want int) ([]string, error) { + if places == nil { + places = &Places{} + } + nodes := make([]string, 0, len(entries)) + var reporting []string + seen := map[string]bool{} + for _, e := range entries { + if seen[e.Node] { + continue + } + seen[e.Node] = true + reporting = append(reporting, e.Node) + if places.CapacityOf(e.Node) > 0 && (qualifies == nil || qualifies(e.Node)) { // a capacity of 0 keeps the part off a node + nodes = append(nodes, e.Node) + } + } + if len(nodes) == 0 { + sort.Strings(reporting) + return nil, fmt.Errorf("no %s build node qualifies: %s report the toolchain, and none is online with a healthy build module and a build capacity", language, strings.Join(reporting, ", ")) + } + return places.choose(nodes, want), nil +} + +// DigestsDifferPrefix opens the error of a build whose nodes built an artifact to different digests (ADR 0478 clause 4): +// such a key is not built again as a new revision, since the same source on the same toolchains gave two answers and +// building again settles nothing; it waits for another fingerprint. +const DigestsDifferPrefix = "digests differ:" + +// Agree is the one digest every node built an artifact to (ADR 0478 clause 4), or an error naming each node and its +// digest when they differ. One node's digest agrees with itself. +func Agree(artifact string, byNode map[string]string) (string, error) { + nodes := make([]string, 0, len(byNode)) + for n := range byNode { + nodes = append(nodes, n) + } + sort.Strings(nodes) + var first string + for i, n := range nodes { + if i == 0 { + first = byNode[n] + continue + } + if byNode[n] != first { + msg := fmt.Sprintf("%s the nodes built %s to different digests:", DigestsDifferPrefix, artifact) + for _, n := range nodes { + msg += fmt.Sprintf(" %s %s;", n, byNode[n]) + } + return "", fmt.Errorf("%s", msg[:len(msg)-1]) + } + } + return first, nil +} diff --git a/go/delivery/buildrun/choose_test.go b/go/delivery/buildrun/choose_test.go new file mode 100644 index 0000000..6e9c2bb --- /dev/null +++ b/go/delivery/buildrun/choose_test.go @@ -0,0 +1,99 @@ +package buildrun + +import ( + "strings" + "testing" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +const digest = "sha256:2222222222222222222222222222222222222222222222222222222222222222" + +func hex64(c byte) string { return strings.Repeat(string(c), 64) } + +func tcs(language, version string, nodes ...string) []delivery.NodeToolchain { + var out []delivery.NodeToolchain + for _, n := range nodes { + out = append(out, delivery.NodeToolchain{Node: n, Toolchain: delivery.Toolchain{Language: language, Version: version, Recipe: "1"}}) + } + return out +} + +func TestChooseTakesTheAgreedNodesWithFewestRunningStepsTiesByName(t *testing.T) { // 1607/AC4, ADR 0478 clause 3 + entries := tcs("go", "go1.27.1", "novox", "ace", "g14", "shanks") + nodes, tc, err := ChooseNodes("go", entries, placesRunning(map[string]int{"novox": 3, "ace": 1, "g14": 1, "shanks": 0}), nil, 2) + if err != nil { + t.Fatal(err) + } + if len(nodes) != 2 || nodes[0] != "shanks" || nodes[1] != "ace" { + t.Errorf("nodes = %v, want [shanks ace]: fewest steps first, a tie by name", nodes) + } + if tc.Version != "go1.27.1" || tc.Recipe != "1" || tc.Language != "go" { + t.Errorf("toolchain = %+v", tc) + } +} + +func TestChooseWithOneNodeAnswersOneNode(t *testing.T) { // 1607/AC4 + nodes, _, err := ChooseNodes("go", tcs("go", "go1.27.1", "novox"), nil, nil, 2) + if err != nil || len(nodes) != 1 || nodes[0] != "novox" { + t.Errorf("nodes = %v, err %v, want [novox]", nodes, err) + } +} + +func TestChooseFailsNamingTheLanguageAndTheVersionsWhenNoNodeQualifies(t *testing.T) { // 1607/AC4 + _, _, err := ChooseNodes("typescript", nil, nil, nil, 2) + if err == nil || !strings.Contains(err.Error(), "typescript") || !strings.Contains(err.Error(), delivery.ToolchainNoReport) { + t.Errorf("no report: error = %v, want it to name typescript and %q", err, delivery.ToolchainNoReport) + } + entries := append(tcs("go", "go1.27.1", "novox"), tcs("go", "go1.26.0", "ace")...) + _, _, err = ChooseNodes("go", entries, nil, nil, 2) + if err == nil || !strings.Contains(err.Error(), "go1.27.1") || !strings.Contains(err.Error(), "go1.26.0") || !strings.Contains(err.Error(), "go") { + t.Errorf("disagreeing nodes: error = %v, want both versions named", err) + } +} + +func TestAgreeNeedsEqualDigestsAndNamesBothNodesWhenTheyDiffer(t *testing.T) { // 1607/AC4, ADR 0478 clause 4 + d, err := Agree("tool", map[string]string{"ace": digest, "novox": digest}) + if err != nil || d != digest { + t.Errorf("Agree = %q, %v, want %q", d, err, digest) + } + _, err = Agree("tool", map[string]string{"ace": digest, "novox": "sha256:" + hex64('9')}) + if err == nil || !strings.Contains(err.Error(), "ace") || !strings.Contains(err.Error(), "novox") || !strings.Contains(err.Error(), digest) || !strings.Contains(err.Error(), hex64('9')) || !strings.Contains(err.Error(), "tool") { + t.Errorf("differing digests: error = %v, want the artifact, both nodes and both digests", err) + } + if d, err := Agree("tool", map[string]string{"ace": digest}); err != nil || d != digest { + t.Errorf("one node: %q, %v", d, err) + } +} + +func TestChooseSkipsNodesThatAreNotOnlineAndFailsNamingThemWhenNoneIs(t *testing.T) { // 1607 review 3, ADR 0478 clause 3 + entries := tcs("go", "go1.27.1", "ace", "g14", "novox") + online := func(n string) bool { return n != "g14" } // a sleeping laptop + nodes, tc, err := ChooseNodes("go", entries, placesRunning(map[string]int{"novox": 5}), online, 2) + if err != nil || len(nodes) != 2 || nodes[0] != "ace" || nodes[1] != "novox" || tc.Version != "go1.27.1" { + t.Errorf("nodes = %v, toolchain %+v, err %v, want [ace novox] with g14 left out although it runs nothing", nodes, tc, err) + } + _, tc, err = ChooseNodes("go", entries, nil, func(string) bool { return false }, 2) + if err == nil || !strings.Contains(err.Error(), "online") || !strings.Contains(err.Error(), "g14") || !strings.Contains(err.Error(), "go") { + t.Errorf("none online: error = %v, want it to name the nodes that report and that none is online", err) + } + if tc.Version != "go1.27.1" { + t.Errorf("none online: the agreed toolchain %+v is still answered", tc) + } +} + +// placesRunning is Places with these steps counted as running, for a test of the choice. +func placesRunning(running map[string]int) *Places { + return &Places{running: running, changed: make(chan struct{})} +} + +func TestChoosePrefersNodesBelowTheirCapacity(t *testing.T) { // round 3 + entries := tcs("go", "go1.27.1", "ace", "novox", "shanks") + // shanks has capacity 3 with 2 running: room; ace has the default 1 with 1 running: none; novox has none running. + p := placesRunning(map[string]int{"shanks": 2, "ace": 1}) + p.Capacity = map[string]int{"shanks": 3} + nodes, _, err := ChooseNodes("go", entries, p, nil, 2) + if err != nil || len(nodes) != 2 || nodes[0] != "novox" || nodes[1] != "shanks" { + t.Errorf("nodes = %v, err %v, want [novox shanks]: the nodes with room, ace at capacity left out", nodes, err) + } +} diff --git a/go/delivery/buildrun/doc.go b/go/delivery/buildrun/doc.go new file mode 100644 index 0000000..b4a943d --- /dev/null +++ b/go/delivery/buildrun/doc.go @@ -0,0 +1,5 @@ +// Package buildrun holds the parts every module that runs steps on the build modules shares: the caller, the steps +// watch and the runner of a long step (ADR 0469), the sets of presence, toolchains and healthy build modules, the +// places a node offers by capacity, and the choice of build nodes (ADR 0478). The builder and the checker of the +// delivery use them; they moved here from the builder's own package so neither keeps a copy (issue 1671, ADR 0484). +package buildrun diff --git a/go/delivery/buildrun/have.go b/go/delivery/buildrun/have.go new file mode 100644 index 0000000..f3c079c --- /dev/null +++ b/go/delivery/buildrun/have.go @@ -0,0 +1,56 @@ +package buildrun + +import ( + "sync" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +// HaveSet holds what each node's node-engine says of each module on it (node-engine_have, key `.`): the +// builder reads it to know whether a node's build module is running and healthy, since a build module whose installed +// toolchain is not the one its recipe builds with is unhealthy (mesh-build-go build_go_health) and its steps would +// refuse (ADR 0478 clause 3). A module the node-engine has written no key of is not healthy. Safe for use from the watch +// and from a build at once. +type HaveSet struct { + mu sync.Mutex + healthy map[string]bool // by `.` +} + +// Put takes a change of the have bucket in and answers whether a module's healthy state changed. A node's own key +// (`.node`) and a value that is not a have are left out. +func (s *HaveSet) Put(key string, value []byte, deleted bool) bool { + node, module, isNode, err := delivery.ParseHaveKey(key) + if err != nil || isNode { + return false + } + ok := false + if !deleted { + h, err := delivery.DecodeHave(value) + if err != nil { + return false + } + ok = h.Phase == delivery.PhaseRunning && h.Health == delivery.HealthOK + } + k := node + "." + module + s.mu.Lock() + defer s.mu.Unlock() + if s.healthy[k] == ok { + return false + } + if s.healthy == nil { + s.healthy = map[string]bool{} + } + if ok { + s.healthy[k] = true + } else { + delete(s.healthy, k) + } + return true +} + +// Healthy is whether the module on the node was last seen running and healthy. +func (s *HaveSet) Healthy(node, module string) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.healthy[node+"."+module] +} diff --git a/go/delivery/buildrun/modules.go b/go/delivery/buildrun/modules.go new file mode 100644 index 0000000..db15aed --- /dev/null +++ b/go/delivery/buildrun/modules.go @@ -0,0 +1,7 @@ +package buildrun + +// The build modules the mesh runs, by the language each builds (ADR 0469). +const ( + GoBuildModule = "mesh-build-go" + NodeBuildModule = "mesh-build-node" +) diff --git a/go/delivery/buildrun/places.go b/go/delivery/buildrun/places.go new file mode 100644 index 0000000..4b189ba --- /dev/null +++ b/go/delivery/buildrun/places.go @@ -0,0 +1,171 @@ +package buildrun + +import ( + "context" + "fmt" + "sort" + "strconv" + "strings" + "sync" +) + +// DefaultCapacity is the build capacity of a node that has no setting: one build step at a time. +const DefaultCapacity = 1 + +// ParseCapacity reads a capacity setting: `node=steps` pairs separated by commas, such as +// `shanks=3,ace=2`. A node without a pair has DefaultCapacity. A pair with no node, a count that is not a whole number of +// at least 0, and a node named twice are refused. A count of 0 keeps the part off the node: it is never chosen. +func ParseCapacity(s string) (map[string]int, error) { + out := map[string]int{} + for _, pair := range strings.Split(s, ",") { + pair = strings.TrimSpace(pair) + if pair == "" { + continue + } + node, count, ok := strings.Cut(pair, "=") + node, count = strings.TrimSpace(node), strings.TrimSpace(count) + n, err := strconv.Atoi(count) + if !ok || node == "" || err != nil || n < 0 { + return nil, fmt.Errorf("the capacity %q is not =", pair) + } + if _, dup := out[node]; dup { + return nil, fmt.Errorf("the capacity names %s twice", node) + } + out[node] = n + } + return out, nil +} + +// Places counts the build steps a part runs on each node and holds a lane back while its node is at its +// capacity (the operator's decision of 2026-10-21; ADR 0478 clause 3). A lane is the fetch and the builds of one build +// module on one node, run one step at a time, so a lane holds one place while it runs. A node is chosen for a lane by +// reserving a place on it at the moment of the choice (ChooseNodes), so that builds choosing at once see each other's +// choices and none waits on a full node while another has room. A lane waits for its place by watching the ends of the +// other lanes, not by a timer: ending a lane wakes the waiting ones. +type Places struct { + // Capacity is the number of steps at once by node; a node not in it has DefaultCapacity. + Capacity map[string]int + + mu sync.Mutex + running map[string]int // lanes holding a place + reserved map[string]int // lanes chosen for the node that do not hold a place yet + changed chan struct{} // closed, and replaced, each time a lane ends +} + +// CapacityOf is how many steps the part runs on the node at once: DefaultCapacity for a node the setting does not +// name, and 0 for one it names so, where the part never builds. +func (p *Places) CapacityOf(node string) int { + if n, ok := p.Capacity[node]; ok && n >= 0 { + return n + } + return DefaultCapacity +} + +// Running is the steps in flight now, by node. +func (p *Places) Running() map[string]int { + p.mu.Lock() + defer p.mu.Unlock() + out := make(map[string]int, len(p.running)) + for n, c := range p.running { + if c > 0 { + out[n] = c + } + } + return out +} + +// Reserved is the lanes chosen for a node that hold no place yet, by node. +func (p *Places) Reserved() map[string]int { + p.mu.Lock() + defer p.mu.Unlock() + out := make(map[string]int, len(p.reserved)) + for n, c := range p.reserved { + if c > 0 { + out[n] = c + } + } + return out +} + +func (p *Places) init() { + if p.running == nil { + p.running = map[string]int{} + } + if p.reserved == nil { + p.reserved = map[string]int{} + } + if p.changed == nil { + p.changed = make(chan struct{}) + } +} + +// Acquire takes a place on the node, waiting while the node is at its capacity, and answers the function that gives it +// back. A place reserved for the node by a choice is the one taken. It returns an error when ctx ends first, and then +// gives up the reservation too. +func (p *Places) Acquire(ctx context.Context, node string) (release func(), err error) { + for { + p.mu.Lock() + p.init() + if p.running[node] < p.CapacityOf(node) { + p.running[node]++ + if p.reserved[node] > 0 { + p.reserved[node]-- + } + p.mu.Unlock() + var once sync.Once + return func() { + once.Do(func() { + p.mu.Lock() + p.running[node]-- + close(p.changed) + p.changed = make(chan struct{}) + p.mu.Unlock() + }) + }, nil + } + wait := p.changed + p.mu.Unlock() + select { + case <-wait: + case <-ctx.Done(): + p.Cancel(node) + return nil, fmt.Errorf("waiting for a place on %s: %w", node, ctx.Err()) + } + } +} + +// Cancel gives up a place reserved for the node that will not be taken. +func (p *Places) Cancel(node string) { + p.mu.Lock() + defer p.mu.Unlock() + p.init() + if p.reserved[node] > 0 { + p.reserved[node]-- + } +} + +// choose orders the nodes (those with room first, counting the lanes running and the lanes reserved, then the fewest of +// them, then by name), keeps the first want and reserves a place on each, all at once. +func (p *Places) choose(nodes []string, want int) []string { + p.mu.Lock() + defer p.mu.Unlock() + p.init() + load := func(n string) int { return p.running[n] + p.reserved[n] } + sort.SliceStable(nodes, func(i, j int) bool { + ri, rj := load(nodes[i]) < p.CapacityOf(nodes[i]), load(nodes[j]) < p.CapacityOf(nodes[j]) + if ri != rj { + return ri + } + if load(nodes[i]) != load(nodes[j]) { + return load(nodes[i]) < load(nodes[j]) + } + return nodes[i] < nodes[j] + }) + if len(nodes) > want { + nodes = nodes[:want] + } + for _, n := range nodes { + p.reserved[n]++ + } + return nodes +} diff --git a/go/delivery/buildrun/places_test.go b/go/delivery/buildrun/places_test.go new file mode 100644 index 0000000..384c537 --- /dev/null +++ b/go/delivery/buildrun/places_test.go @@ -0,0 +1,126 @@ +package buildrun + +import ( + "context" + "strings" + "testing" + "time" +) + +func TestParseCapacityReadsNodeEqualsCountPairs(t *testing.T) { + c, err := ParseCapacity(" shanks=3, ace=2 ") + if err != nil || c["shanks"] != 3 || c["ace"] != 2 || len(c) != 2 { + t.Errorf("capacity = %v, %v", c, err) + } + if c, err := ParseCapacity(""); err != nil || len(c) != 0 { + t.Errorf("empty: %v, %v", c, err) + } + for _, bad := range []string{"shanks", "shanks=x", "=2", "a=1,a=2", "shanks=-1"} { + if _, err := ParseCapacity(bad); err == nil { + t.Errorf("%q accepted", bad) + } + } +} + +func acquireAsync(p *Places, node string) chan func() { + got := make(chan func(), 1) + go func() { + release, err := p.Acquire(context.Background(), node) + if err != nil { + close(got) + return + } + got <- release + }() + return got +} + +func TestThreeStepsOnANodeOfCapacityTwoRunTwoAtOnceAndTheThirdStartsWhenOneEnds(t *testing.T) { // round 3 + p := &Places{Capacity: map[string]int{"ace": 2}} + a, b := acquireAsync(p, "ace"), acquireAsync(p, "ace") + ra, rb := <-a, <-b + third := acquireAsync(p, "ace") + select { + case <-third: + t.Fatal("a third step started on a node of capacity 2 with two running") + case <-time.After(50 * time.Millisecond): + } + if p.Running()["ace"] != 2 { + t.Errorf("running = %v, want 2 on ace", p.Running()) + } + ra() + select { + case r := <-third: + r() + case <-time.After(2 * time.Second): + t.Fatal("the third step did not start when one ended") + } + rb() +} + +func TestANodeWithNoSettingRunsOneStepAtATime(t *testing.T) { // round 3 + p := &Places{Capacity: map[string]int{"shanks": 3}} + first := <-acquireAsync(p, "g14") + second := acquireAsync(p, "g14") + select { + case <-second: + t.Fatal("two steps on a node without a setting") + case <-time.After(50 * time.Millisecond): + } + first() + r := <-second + r() + if p.CapacityOf("g14") != 1 || p.CapacityOf("shanks") != 3 { + t.Errorf("capacities = %d, %d", p.CapacityOf("g14"), p.CapacityOf("shanks")) + } +} + +func TestAWaitingStepStopsWhenItsContextEnds(t *testing.T) { // round 3 + p := &Places{} + r := <-acquireAsync(p, "ace") + defer r() + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { _, err := p.Acquire(ctx, "ace"); done <- err }() + cancel() + select { + case err := <-done: + if err == nil { + t.Error("Acquire returned no error for a cancelled wait") + } + case <-time.After(2 * time.Second): + t.Fatal("a cancelled wait did not return") + } +} + +// The operator's decision of 2026-10-21 13:40: the builder builds on shanks and ace only; a capacity of 0 keeps it off a node. +func TestACapacityOfZeroKeepsThePartOffTheNodeAndANodeNotNamedKeepsOne(t *testing.T) { // issue 1661 + c, err := ParseCapacity("shanks=3,ace=2,g14=0,novox=0") + if err != nil || c["g14"] != 0 || c["novox"] != 0 || c["shanks"] != 3 || len(c) != 4 { + t.Fatalf("capacity = %v, %v", c, err) + } + p := &Places{Capacity: c} + if p.CapacityOf("g14") != 0 || p.CapacityOf("novox") != 0 || p.CapacityOf("ace") != 2 || p.CapacityOf("other") != DefaultCapacity { + t.Errorf("capacities = %d %d %d %d, want 0 0 2 and the default for a node not named", p.CapacityOf("g14"), p.CapacityOf("novox"), p.CapacityOf("ace"), p.CapacityOf("other")) + } + if _, err := ParseCapacity("shanks=-1"); err == nil { + t.Error("a negative capacity accepted") + } + entries := tcs("go", "go1.27.1", "ace", "g14", "novox", "shanks", "other") + for i := 0; i < 6; i++ { + nodes, _, err := ChooseNodes("go", entries, p, nil, 2) + if err != nil { + t.Fatal(err) + } + for _, n := range nodes { + if n == "g14" || n == "novox" { + t.Fatalf("choice %d: %v includes a node of capacity 0", i, nodes) + } + } + } + // Only nodes of capacity 0 report: nothing qualifies, and the build says why. + _, _, err = ChooseNodes("go", tcs("go", "go1.27.1", "g14", "novox"), p, nil, 2) + if err == nil || !strings.Contains(err.Error(), "capacity") { + t.Errorf("only capacity-0 nodes: error = %v, want it to name the capacity", err) + } +} diff --git a/go/delivery/buildrun/presence.go b/go/delivery/buildrun/presence.go new file mode 100644 index 0000000..63f8243 --- /dev/null +++ b/go/delivery/buildrun/presence.go @@ -0,0 +1,53 @@ +package buildrun + +import ( + "sync" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +// PresenceSet holds which nodes are online, as the watchman writes it (mesh-delivery-watchman_presence, key ``, the +// node-engine's connection): a node asleep or off the bus is offline and its build module cannot answer. A node the +// watchman has no key of is not online. Safe for use from the watch and from a build at once. +type PresenceSet struct { + mu sync.Mutex + online map[string]bool +} + +// Put takes a change of the presence bucket in and answers whether a node's online state changed. A part's key and a +// value that is not a presence are left out. +func (s *PresenceSet) Put(key string, value []byte, deleted bool) bool { + node, part, _, err := delivery.ParsePresenceKey(key) + if err != nil || part != "" { + return false + } + on := false + if !deleted { + p, err := delivery.DecodePresence(value) + if err != nil { + return false + } + on = p.Online + } + s.mu.Lock() + defer s.mu.Unlock() + if s.online[node] == on { + return false + } + if s.online == nil { + s.online = map[string]bool{} + } + if on { + s.online[node] = true + } else { + delete(s.online, node) + } + return true +} + +// Online is whether the watchman last saw the node online. +func (s *PresenceSet) Online(node string) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.online[node] +} diff --git a/go/delivery/buildrun/presence_test.go b/go/delivery/buildrun/presence_test.go new file mode 100644 index 0000000..5157609 --- /dev/null +++ b/go/delivery/buildrun/presence_test.go @@ -0,0 +1,43 @@ +package buildrun + +import ( + "testing" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +func presenceValue(t *testing.T, online bool) []byte { + t.Helper() + b, err := delivery.EncodePresence(delivery.Presence{Online: online}) + if err != nil { + t.Fatal(err) + } + return b +} + +func TestPresenceSetKnowsOnlyNodesTheWatchmanSawOnline(t *testing.T) { // 1607 review 3 + var s PresenceSet + if s.Online("ace") { + t.Error("a node the watchman never wrote is online") + } + if !s.Put("ace", presenceValue(t, true), false) || !s.Online("ace") { + t.Error("ace online was not taken in") + } + if s.Put("ace", presenceValue(t, true), false) { + t.Error("the same presence again counted as a change") + } + if !s.Put("ace", presenceValue(t, false), false) || s.Online("ace") { + t.Error("ace going offline was not taken in") + } + s.Put("g14", presenceValue(t, true), false) + if !s.Put("g14", nil, true) || s.Online("g14") { + t.Error("a deleted key is still online") + } + // A part's instance (part..) is not a node. + if s.Put("part.mesh-delivery-builder.novox", presenceValue(t, true), false) || s.Online("part") { + t.Error("a part's key was taken for a node") + } + if s.Put("novox", []byte("junk"), false) { + t.Error("junk counted") + } +} diff --git a/go/delivery/buildrun/steps.go b/go/delivery/buildrun/steps.go new file mode 100644 index 0000000..f08e002 --- /dev/null +++ b/go/delivery/buildrun/steps.go @@ -0,0 +1,67 @@ +package buildrun + +import ( + "context" + "encoding/json" + "fmt" + "time" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +// Caller calls a tool of a module on one named machine (mesh-sdk process.Conn.CallOn; ADR 0469 clause 3). +type Caller interface { + CallOn(node, key string, body any) (json.RawMessage, error) +} + +// Steps waits for the end of a build module's long step: the value of its key `.` in the module's steps state +// (ADR 0469 clause 2) once its state is not running. It is a watch of that key, not a poll; it returns when the step +// ends or ctx does. +type Steps interface { + Wait(ctx context.Context, module, key string) (delivery.Step, error) +} + +// Runner runs a build module's long steps: the call answers at once with the step's key, and the result is read from that +// key when the step ends (ADR 0469 clause 2). +type Runner struct { + Caller Caller + Steps Steps + // Bound is how long a step may run before the builder stops waiting for it. + Bound time.Duration +} + +// Run calls tool of the build module on node with args, waits for the step's end and answers its result. A step that +// failed or was refused is an error naming the node, the state and the reason. +func (r Runner) Run(ctx context.Context, module, node, tool string, args map[string]any) (json.RawMessage, error) { + raw, err := r.Caller.CallOn(node, module+"."+tool, args) + if err != nil { + return nil, fmt.Errorf("%s on %s: %w", tool, node, err) + } + var answer struct { + Step string `json:"step"` + Key string `json:"key"` + State string `json:"state"` + } + if err := json.Unmarshal(raw, &answer); err != nil { + return nil, fmt.Errorf("%s on %s answered what is not a step: %w", tool, node, err) + } + if answer.Key == "" { + return nil, fmt.Errorf("%s on %s answered no step key: %s", tool, node, raw) + } + if n, _, err := delivery.ParseStepKey(answer.Key); err != nil || n != node { + return nil, fmt.Errorf("%s on %s answered the step key %q, which is not a key of that node", tool, node, answer.Key) + } + wait, cancel := context.WithTimeoutCause(ctx, r.Bound, fmt.Errorf("the step %s did not end within %s", answer.Key, r.Bound)) + defer cancel() + step, err := r.Steps.Wait(wait, module, answer.Key) + if err != nil { + if cause := context.Cause(wait); cause != nil && ctx.Err() == nil { + err = cause + } + return nil, fmt.Errorf("%s on %s: %w", tool, node, err) + } + if step.State != delivery.StepPassed { + return nil, fmt.Errorf("%s on %s: the step %s %s: %s", tool, node, answer.Key, step.State, step.Reason) + } + return step.Result, nil +} diff --git a/go/delivery/buildrun/steps_test.go b/go/delivery/buildrun/steps_test.go new file mode 100644 index 0000000..9d6d024 --- /dev/null +++ b/go/delivery/buildrun/steps_test.go @@ -0,0 +1,116 @@ +package buildrun + +import ( + "context" + "encoding/json" + "errors" + "strings" + "sync" + "testing" + "time" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +type call struct { + node, key string + body any +} + +// fakeCaller answers a long tool as the build modules do: at once, with the step and its key. +type fakeCaller struct { + calls []call + answer string + err error +} + +func (f *fakeCaller) CallOn(node, key string, body any) (json.RawMessage, error) { + f.calls = append(f.calls, call{node, key, body}) + if f.err != nil { + return nil, f.err + } + return json.RawMessage(f.answer), nil +} + +// fakeSteps holds the steps values by `/`; Wait on a key it does not hold blocks until the context ends. +type fakeSteps struct { + mu sync.Mutex + steps map[string]delivery.Step + // during, when set, is called as a wait starts and the function it answers as the wait ends. + during func(key string) func() +} + +func (f *fakeSteps) Wait(ctx context.Context, module, key string) (delivery.Step, error) { + f.mu.Lock() + s, ok := f.steps[module+"/"+key] + during := f.during + f.mu.Unlock() + if ok { + if during != nil { + defer during(key)() + } + return s, nil + } + <-ctx.Done() + return delivery.Step{}, context.Cause(ctx) +} + +func stepAt(node, id, state, reason string, result string) delivery.Step { + s := delivery.Step{ID: id, Tool: "build_go_fetch", Node: node, State: state, Reason: reason} + if result != "" { + s.Result = json.RawMessage(result) + } + return s +} + +func TestRunCallsTheToolOnTheNodeAndReadsTheStepsKey(t *testing.T) { // 1607/AC5 + caller := &fakeCaller{answer: `{"step":"abc1","key":"ace.abc1","state":"running"}`} + steps := &fakeSteps{steps: map[string]delivery.Step{"mesh-build-go/ace.abc1": stepAt("ace", "abc1", delivery.StepPassed, "", `{"workspace":"w1","commit":"c"}`)}} + res, err := Runner{Caller: caller, Steps: steps, Bound: time.Second}.Run(context.Background(), "mesh-build-go", "ace", "build_go_fetch", map[string]any{"repository": "novox/r", "commit": "c"}) + if err != nil { + t.Fatal(err) + } + if len(caller.calls) != 1 || caller.calls[0].node != "ace" || caller.calls[0].key != "mesh-build-go.build_go_fetch" { + t.Errorf("calls = %+v, want one on ace for mesh-build-go.build_go_fetch", caller.calls) + } + var fr struct{ Workspace string } + if err := json.Unmarshal(res, &fr); err != nil || fr.Workspace != "w1" { + t.Errorf("result = %s (%v), want the step's result", res, err) + } +} + +func TestRunFailsOnAFailedOrRefusedStepNamingItsReason(t *testing.T) { // 1607/AC5 + for _, state := range []string{delivery.StepFailed, delivery.StepRefused} { + caller := &fakeCaller{answer: `{"step":"abc1","key":"ace.abc1","state":"running"}`} + steps := &fakeSteps{steps: map[string]delivery.Step{"mesh-build-go/ace.abc1": stepAt("ace", "abc1", state, "go is go1.26.0, the recipe builds with go1.27.1", "")}} + _, err := Runner{Caller: caller, Steps: steps, Bound: time.Second}.Run(context.Background(), "mesh-build-go", "ace", "build_go_build", nil) + if err == nil || !strings.Contains(err.Error(), "go1.27.1") || !strings.Contains(err.Error(), state) || !strings.Contains(err.Error(), "ace") { + t.Errorf("%s: error = %v, want the node, the state and the reason", state, err) + } + } +} + +func TestRunStopsWaitingAtTheBound(t *testing.T) { // 1607/AC5 + caller := &fakeCaller{answer: `{"step":"abc1","key":"ace.abc1","state":"running"}`} + _, err := Runner{Caller: caller, Steps: &fakeSteps{}, Bound: 20 * time.Millisecond}.Run(context.Background(), "mesh-build-go", "ace", "build_go_build", nil) + if err == nil || !strings.Contains(err.Error(), "ace.abc1") || !strings.Contains(err.Error(), "did not end") { + t.Errorf("error = %v, want the step key and that it did not end within its bound", err) + } +} + +func TestRunRefusesAnAnswerWithNoKeyAndAKeyOfAnotherNode(t *testing.T) { // 1607/AC5 + for _, answer := range []string{`{"state":"running"}`, `{"step":"abc1","key":"novox.abc1","state":"running"}`, `not json`} { + _, err := Runner{Caller: &fakeCaller{answer: answer}, Steps: &fakeSteps{}, Bound: time.Second}.Run(context.Background(), "mesh-build-go", "ace", "build_go_fetch", nil) + if err == nil { + t.Errorf("answer %s: no error", answer) + } + } +} + +func TestRunReturnsTheCallsOwnError(t *testing.T) { // 1607/AC5 + boom := errors.New("no responders") + _, err := Runner{Caller: &fakeCaller{err: boom}, Steps: &fakeSteps{}, Bound: time.Second}.Run(context.Background(), "mesh-build-go", "ace", "build_go_fetch", nil) + if !errors.Is(err, boom) || !strings.Contains(err.Error(), "ace") { + t.Errorf("error = %v, want the call's error naming the node", err) + } +} diff --git a/go/delivery/buildrun/toolchains.go b/go/delivery/buildrun/toolchains.go new file mode 100644 index 0000000..6170425 --- /dev/null +++ b/go/delivery/buildrun/toolchains.go @@ -0,0 +1,64 @@ +package buildrun + +import ( + "sync" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +// LanguageOf is the language each build module's toolchains state holds (the indexer's ToolchainModules, reversed). +var LanguageOf = map[string]string{GoBuildModule: "go", NodeBuildModule: "typescript"} + +// ToolchainSet holds what the build modules' toolchains buckets hold: each node's entry of a language, as the watches +// put them in. Safe for use from the watches and from a build at once. +type ToolchainSet struct { + mu sync.Mutex + by map[string]map[string]delivery.Toolchain // language -> node -> entry +} + +// Put takes a change of a build module's toolchains bucket in: a put of a node's entry (key .) or its +// delete. It answers whether the language's entries changed (a node's version or recipe, or a node appearing or going). +// A key or a value that is not an entry of the module's language is left out. +func (s *ToolchainSet) Put(module, key string, value []byte, deleted bool) bool { + language, ok := LanguageOf[module] + if !ok { + return false + } + node, keyLanguage, err := delivery.ParseToolchainKey(key) + if err != nil || keyLanguage != language { + return false + } + s.mu.Lock() + defer s.mu.Unlock() + if deleted { + if _, had := s.by[language][node]; !had { + return false + } + delete(s.by[language], node) + return true + } + tc, err := delivery.DecodeToolchain(value) + if err != nil || tc.Language != language { + return false + } + if s.by == nil { + s.by = map[string]map[string]delivery.Toolchain{} + } + if s.by[language] == nil { + s.by[language] = map[string]delivery.Toolchain{} + } + cur, had := s.by[language][node] + s.by[language][node] = tc + return !had || cur.Version != tc.Version || cur.Recipe != tc.Recipe +} + +// Entries are the nodes' entries of a language, in no particular order. +func (s *ToolchainSet) Entries(language string) []delivery.NodeToolchain { + s.mu.Lock() + defer s.mu.Unlock() + var out []delivery.NodeToolchain + for node, tc := range s.by[language] { + out = append(out, delivery.NodeToolchain{Node: node, Toolchain: tc}) + } + return out +} diff --git a/go/delivery/buildrun/toolchains_test.go b/go/delivery/buildrun/toolchains_test.go new file mode 100644 index 0000000..e434b8a --- /dev/null +++ b/go/delivery/buildrun/toolchains_test.go @@ -0,0 +1,51 @@ +package buildrun + +import ( + "testing" + "time" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +func tcValue(t *testing.T, language, version string) []byte { + t.Helper() + b, err := delivery.EncodeToolchain(delivery.Toolchain{Language: language, Version: version, Recipe: "1", At: time.Now()}) + if err != nil { + t.Fatal(err) + } + return b +} + +func TestToolchainSetKeepsEachNodesEntryByLanguageAndSaysWhenOneChanged(t *testing.T) { // 1607/AC6 + var s ToolchainSet + if !s.Put(GoBuildModule, "novox.go", tcValue(t, "go", "go1.27.1"), false) { + t.Error("a first entry did not count as a change") + } + if s.Put(GoBuildModule, "novox.go", tcValue(t, "go", "go1.27.1"), false) { + t.Error("the same entry again counted as a change") + } + s.Put(GoBuildModule, "ace.go", tcValue(t, "go", "go1.27.1"), false) + s.Put(NodeBuildModule, "ace.typescript", tcValue(t, "typescript", "tsc5"), false) + if got := s.Entries("go"); len(got) != 2 { + t.Errorf("go entries = %+v, want two", got) + } + if got := s.Entries("typescript"); len(got) != 1 || got[0].Node != "ace" || got[0].Version != "tsc5" { + t.Errorf("typescript entries = %+v", got) + } + if !s.Put(GoBuildModule, "ace.go", nil, true) || len(s.Entries("go")) != 1 { + t.Error("a delete did not remove the node's entry") + } +} + +func TestToolchainSetLeavesOutWhatIsNotAToolchainOfTheModulesLanguage(t *testing.T) { // 1607/AC6 + var s ToolchainSet + if s.Put(GoBuildModule, "novox.typescript", tcValue(t, "typescript", "x"), false) { + t.Error("a typescript toolchain in the go build module's bucket counted") + } + if s.Put(GoBuildModule, "novox.go", []byte("not json"), false) || s.Put("other-module", "novox.go", tcValue(t, "go", "x"), false) { + t.Error("garbage or an unknown module counted") + } + if len(s.Entries("go")) != 0 { + t.Error("an entry was kept") + } +} -- 2.54.0 From 6d0c2076a3ba930a8e1960b6301d93cf1e6e86bd Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 19:19:14 +0200 Subject: [PATCH 2/8] Give the checker's verdict a skipped count, a composition and the over-bound levels, so the line never hides skips or claims a composition not run (issue 1672) --- go/delivery/checker.go | 61 +++++++++++++++++++- go/delivery/checker_test.go | 108 ++++++++++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+), 1 deletion(-) diff --git a/go/delivery/checker.go b/go/delivery/checker.go index bdb57e9..acc287b 100644 --- a/go/delivery/checker.go +++ b/go/delivery/checker.go @@ -88,6 +88,25 @@ func ParseCheckerVerdictKey(key string) (group, headsHash string, err error) { type LevelResult struct { Run int `json:"run"` Kept int `json:"kept"` + // Skipped is the tests the run skipped, so a pass never hides them (issue 1251, ADR 0484 clause 7). + Skipped int `json:"skipped,omitempty"` +} + +// The states of a Composition (ADR 0484 clause 6): the composition check is the assigner's, so until it delivers a +// verdict says CompositionNotRun, and a line never says the composition passed on that. +const ( + CompositionNotRun = "not-run" + CompositionPass = "pass" + CompositionFail = "fail" +) + +// Composition is what the check of the machines' composition made of a verdict: its State (not-run, pass or fail), the +// Words of a refusal, the mesh-sdk commit it composed with and the machines composed (ADR 0482 clause 9, ADR 0484 clause 6). +type Composition struct { + State string `json:"state"` + Words string `json:"words,omitempty"` + SDK string `json:"sdk,omitempty"` + Machines []string `json:"machines,omitempty"` } // Took is the time of each part of a check in integer milliseconds (ADR 0447 clause 3): waiting in the queue, cloning, @@ -123,6 +142,33 @@ type Verdict struct { Took Took `json:"took,omitzero"` Machines []string `json:"machines,omitempty"` SDK string `json:"sdk,omitempty"` + // Composition is the composition check's result; absent in a value written before it (ADR 0484 clause 6). + Composition *Composition `json:"composition,omitempty"` + // OverBound names the levels whose running went over their bound; it does not change the State (ADR 0453 clause 9). + OverBound []string `json:"over-bound,omitempty"` +} + +func (c *Composition) check() error { + if c == nil { + return nil + } + switch c.State { + case CompositionNotRun: + if c.Words != "" || c.SDK != "" || len(c.Machines) > 0 { + return fmt.Errorf("delivery: the verdict's composition is not run and says nothing of words, the mesh-sdk commit or machines") + } + case CompositionPass: + if c.SDK == "" { + return fmt.Errorf("delivery: the verdict's composition passed and names no mesh-sdk commit") + } + case CompositionFail: + if c.Words == "" { + return fmt.Errorf("delivery: the verdict's composition failed and gives no words") + } + default: + return fmt.Errorf("delivery: the verdict's composition state %q is not %s, %s or %s", c.State, CompositionNotRun, CompositionPass, CompositionFail) + } + return nil } func (v Verdict) check() error { @@ -141,10 +187,23 @@ func (v Verdict) check() error { if err := checkLevel(level); err != nil { return err } - if r.Run < 0 || r.Kept < 0 { + if r.Run < 0 || r.Kept < 0 || r.Skipped < 0 { return fmt.Errorf("delivery: the verdict's %s level counts a negative number of tests", level) } } + if err := v.Composition.check(); err != nil { + return err + } + seen := map[string]bool{} + for _, level := range v.OverBound { + if err := checkLevel(level); err != nil { + return fmt.Errorf("delivery: the verdict's over-bound names: %w", err) + } + if seen[level] { + return fmt.Errorf("delivery: the verdict's over-bound names the level %s twice", level) + } + seen[level] = true + } for part, ms := range map[string]int64{"queue": v.Took.Queue, "clone": v.Took.Clone, "unit": v.Took.Unit, "integration-setup": v.Took.IntegrationSetup, "integration-run": v.Took.IntegrationRun, "composition": v.Took.Composition} { if ms < 0 { diff --git a/go/delivery/checker_test.go b/go/delivery/checker_test.go index 2dd939f..f8542c4 100644 --- a/go/delivery/checker_test.go +++ b/go/delivery/checker_test.go @@ -1,6 +1,7 @@ package delivery import ( + "encoding/json" "errors" "strings" "testing" @@ -220,3 +221,110 @@ func TestCheckerVersions(t *testing.T) { } } } + +// oldVerdictJSON is a verdict as the SDK before issue 1672 wrote it (copied from EncodeVerdict of passVerdict's shape +// at the commit before this change): no skipped count, no composition, no over-bound names. +const oldVerdictJSON = `{"v":1,"group":"feat/x","heads":"4b5491557853","trunk":{"novox/mesh-control":"bbbb","novox/mesh-sdk":"aaaa"},"state":"pass","levels":{"integration":{"run":3,"kept":40},"unit":{"run":12,"kept":301}},"took":{"queue":1200,"clone":800,"unit":950,"integration-setup":2100,"integration-run":7400,"composition":3300},"machines":["ace","g14","novox","shanks"],"sdk":"v0.1.14-0.20261019091011-aa68b67389c9"}` + +// oldVerdict is the reader of today's shape: the struct as it stood before the new fields. +type oldVerdict struct { + V int `json:"v"` + Group string `json:"group"` + Heads string `json:"heads"` + State string `json:"state"` + Levels map[string]struct{ Run, Kept int } + Machines []string + SDK string +} + +// 1672/AC1: the skipped count, the composition and the over-bound names round trip and appear in the bytes. +func TestVerdictCarriesSkippedCompositionAndOverBound(t *testing.T) { + v := passVerdict() + v.Levels[LevelUnit] = LevelResult{Run: 12, Kept: 301, Skipped: 4} + v.Composition = &Composition{State: CompositionPass, SDK: v.SDK, Machines: []string{"ace", "novox"}} + v.OverBound = []string{LevelIntegration} + data, err := EncodeVerdict(v) + if err != nil { + t.Fatal(err) + } + for _, field := range []string{`"skipped":4`, `"composition":{"state":"pass"`, `"over-bound":["integration"]`} { + if !strings.Contains(string(data), field) { + t.Errorf("the value lacks %s: %s", field, data) + } + } + got, err := DecodeVerdict(data) + v.V = Version + if err != nil || !SameVerdict(got, v) || got.Levels[LevelUnit].Skipped != 4 || got.Composition == nil || got.Composition.State != CompositionPass { + t.Errorf("round trip = %+v, %v", got, err) + } + notRun := passVerdict() + notRun.Composition = &Composition{State: CompositionNotRun} + if _, err := EncodeVerdict(notRun); err != nil { + t.Errorf("a composition not yet run refused: %v", err) + } + failed := passVerdict() + failed.State, failed.Failed = VerdictFail, FailedFirst{Machine: "ace", Words: "x"} + failed.Composition = &Composition{State: CompositionFail, Words: "path /etc/x is owned by two modules", SDK: failed.SDK, Machines: []string{"ace"}} + if _, err := EncodeVerdict(failed); err != nil { + t.Errorf("a failed composition refused: %v", err) + } +} + +// 1672/AC1: an invalid new value is refused, naming the field. +func TestVerdictNewFieldsChecked(t *testing.T) { + cases := map[string]struct { + edit func(*Verdict) + words []string + }{ + "a negative skipped count": {func(v *Verdict) { v.Levels[LevelUnit] = LevelResult{Run: 1, Skipped: -1} }, []string{"unit", "negative"}}, + "an unknown composition": {func(v *Verdict) { v.Composition = &Composition{State: "maybe"} }, []string{"composition", "maybe"}}, + "a composition with no state": {func(v *Verdict) { v.Composition = &Composition{} }, []string{"composition"}}, + "a failed composition without words": {func(v *Verdict) { v.Composition = &Composition{State: CompositionFail} }, []string{"composition", "words"}}, + "a passed composition without the sdk": {func(v *Verdict) { v.Composition = &Composition{State: CompositionPass, Machines: []string{"ace"}} }, []string{"composition", "sdk"}}, + "a not-run composition naming machines": {func(v *Verdict) { v.Composition = &Composition{State: CompositionNotRun, Machines: []string{"ace"}} }, []string{"composition", "not run"}}, + "an unknown over-bound level": {func(v *Verdict) { v.OverBound = []string{"e2e"} }, []string{"e2e"}}, + "an over-bound level named twice": {func(v *Verdict) { v.OverBound = []string{LevelUnit, LevelUnit} }, []string{"over-bound", "unit"}}, + } + for name, c := range cases { + v := passVerdict() + c.edit(&v) + _, err := EncodeVerdict(v) + if err == nil { + t.Errorf("%s accepted", name) + continue + } + for _, w := range c.words { + if !strings.Contains(err.Error(), w) { + t.Errorf("%s: error %q does not name %q", name, err, w) + } + } + } +} + +// 1672/AC2: a value written by the SDK before this change decodes with the new reader, with the new fields empty, and +// the new SDK writes the same bytes for it; a value with the new fields decodes under a reader of today's shape. +func TestVerdictOldAndNewReadersAgree(t *testing.T) { + v, err := DecodeVerdict([]byte(oldVerdictJSON)) + if err != nil { + t.Fatalf("the new reader of an old value: %v", err) + } + if v.Composition != nil || v.OverBound != nil || v.Levels[LevelUnit].Skipped != 0 || v.Levels[LevelUnit].Run != 12 { + t.Errorf("an old value grew fields: %+v", v) + } + again, err := EncodeVerdict(v) + if err != nil || string(again) != oldVerdictJSON { + t.Errorf("the new SDK writes %s, %v for an old value, want today's bytes", again, err) + } + n := passVerdict() + n.Levels[LevelUnit] = LevelResult{Run: 12, Kept: 301, Skipped: 4} + n.Composition = &Composition{State: CompositionNotRun} + n.OverBound = []string{LevelUnit} + data, err := EncodeVerdict(n) + if err != nil { + t.Fatal(err) + } + var ov oldVerdict + if err := json.Unmarshal(data, &ov); err != nil || ov.Group != "feat/x" || ov.State != VerdictPass || ov.Levels[LevelUnit].Run != 12 || ov.SDK == "" { + t.Errorf("the old reader of a new value: %+v, %v", ov, err) + } +} -- 2.54.0 From b944d07158a3727d2b5f1ffccd9866d1596fce98 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 19:21:22 +0200 Subject: [PATCH 3/8] Add the checker's owed bucket, the mesh/check status context and the previews layout, and correct the checker header: the checker decides the status, the bridge sets it (issue 1673) --- go/delivery/checker.go | 8 +- go/delivery/checker_test.go | 188 ++++++++++++++++++++++++++++++++++++ go/delivery/gate.go | 26 ++++- go/delivery/preview.go | 168 ++++++++++++++++++++++++++++++++ 4 files changed, 386 insertions(+), 4 deletions(-) create mode 100644 go/delivery/preview.go diff --git a/go/delivery/checker.go b/go/delivery/checker.go index acc287b..81c0dc9 100644 --- a/go/delivery/checker.go +++ b/go/delivery/checker.go @@ -9,9 +9,11 @@ import ( ) // The checker's keys (novox/hq ADR 0447 clauses 1 to 3, ADR 0444, ADR 0446 clause 5, ADR 0453 clause 4): the verdicts of -// its checks and the passing verdicts it keeps per package and level. The checker writes them from R4c, dark; the -// forge bridge writes the status `mesh/check` from a verdict (unrequired until R4f), the gate reads it from R4f, and -// the view and the watchman read them (a running check carries a pulse). +// its checks and the passing verdicts it keeps per package and level. The checker writes them from R4c, dark, and +// writes the status `mesh/check` it decides as an owed act in its own bucket (CheckerOwedBucket, the gate's owed shape); +// the forge bridge, the only part that calls the forge, performs that act (unrequired until R4f), once per revision of +// the key (ADR 0484 clause 2). The gate reads the status from R4f, and the view and the watchman read the verdicts (a +// running check carries a pulse). // // **The verdict's key is the group and the heads, not the fingerprints** (the lead's limit for R4a). Research 063 // section 01 drew `verdicts..check.`; ADR 0447 clause 1 judges a pull request's head merged onto its diff --git a/go/delivery/checker_test.go b/go/delivery/checker_test.go index f8542c4..eb79299 100644 --- a/go/delivery/checker_test.go +++ b/go/delivery/checker_test.go @@ -328,3 +328,191 @@ func TestVerdictOldAndNewReadersAgree(t *testing.T) { t.Errorf("the old reader of a new value: %+v, %v", ov, err) } } + +func sampleCheckerOwed() Owed { + return Owed{Act: OwedStatus, Context: StatusContextCheck, State: "success", Target: "https://mesh.example/delivery/check/36", + Text: "pass 4b5491557853 on aaaa: unit 12 run 301 kept, integration 3 run 40 kept; composition: not yet run"} +} + +// 1673/AC1: the checker's owed bucket is the checker module plus _owed, and its key is the gate's owed key with the act +// status=mesh/check. +func TestCheckerOwedKeyAndBucket(t *testing.T) { + if CheckerOwedBucket != "mesh-delivery-checker_owed" || CheckerOwedHistory != 5 || PreviewsBucket != "mesh-delivery-checker_previews" || PreviewsHistory != 5 { + t.Errorf("buckets %s %d %s %d", CheckerOwedBucket, CheckerOwedHistory, PreviewsBucket, PreviewsHistory) + } + key, err := CheckerOwedKey("novox/mesh-sdk", 36, "aaaa") + if err != nil || key != "novox/mesh-sdk.36.aaaa.status=mesh/check" { + t.Fatalf("key = %q, %v", key, err) + } + repo, number, head, err := ParseCheckerOwedKey(key) + if err != nil || repo != "novox/mesh-sdk" || number != 36 || head != "aaaa" { + t.Errorf("parse = %q %d %q %v", repo, number, head, err) + } + other, _ := OwedKey("novox/mesh-sdk", 36, "aaaa", "status=mesh/merge-gate") + for _, bad := range []string{other, "novox/mesh-sdk.36.aaaa.view", "novox/mesh-sdk.36.aaaa.note", "novox/mesh-sdk.36.aaaa.comment.1", ""} { + if _, _, _, err := ParseCheckerOwedKey(bad); err == nil { + t.Errorf("checker owed key %q accepted", bad) + } + } + if _, err := CheckerOwedKey("nope", 36, "aaaa"); err == nil { + t.Error("a bad repository accepted") + } +} + +// 1673/AC2: an owed value with the context mesh/check is accepted and an unknown context is refused, in the act and in the value. +func TestOwedAcceptsMeshCheckAndRefusesAnUnknownContext(t *testing.T) { + for _, ctx := range []string{StatusContextCheck, StatusContextMergeGate} { + if kind, got, err := ParseOwedAct("status=" + ctx); err != nil || kind != OwedStatus || got != ctx { + t.Errorf("ParseOwedAct(status=%s) = %q %q %v", ctx, kind, got, err) + } + } + for _, bad := range []string{"status=mesh/other", "status=check", "status=mesh/Check"} { + if _, err := OwedKey("novox/mesh-sdk", 36, "aaaa", bad); err == nil || !strings.Contains(err.Error(), bad[len("status="):]) { + t.Errorf("owed act %q: %v, want a refusal naming the context", bad, err) + } + } + data, err := EncodeCheckerOwed(sampleCheckerOwed()) + if err != nil { + t.Fatal(err) + } + if got, err := DecodeCheckerOwed(data); err != nil || !SameOwed(got, sampleCheckerOwed()) && got.Context != StatusContextCheck { + t.Errorf("round trip = %+v, %v", got, err) + } + unknown := sampleCheckerOwed() + unknown.Context = "mesh/other" + if _, err := EncodeOwed(unknown); err == nil || !strings.Contains(err.Error(), "mesh/other") { + t.Errorf("an owed status of the context mesh/other: %v", err) + } + // The checker's bucket holds only its own status. + gate := sampleCheckerOwed() + gate.Context = StatusContextMergeGate + for name, o := range map[string]Owed{"the gate's context": gate, "a view": {Act: OwedView, Text: "x"}, "a note": {Act: OwedNote, Text: "x", Ref: "mesh-plan"}} { + if _, err := EncodeCheckerOwed(o); err == nil { + t.Errorf("the checker's bucket takes %s", name) + } + } + for _, state := range []string{"pending", "success", "failure"} { + o := sampleCheckerOwed() + o.State = state + if _, err := EncodeCheckerOwed(o); err != nil { + t.Errorf("state %s: %v", state, err) + } + } +} + +func samplePreview() Preview { + return Preview{Repository: "novox/mesh-delivery", Head: "aaaa", Trunk: "bbbb", Merge: "cccc", + Modules: map[string]PreviewModule{ + "modules/mesh-delivery-checker": {Tree: "t1", Manifest: strings.Repeat("1", 64), Locks: map[string]string{"go.sum": strings.Repeat("2", 64)}}, + "modules/mesh-delivery-view": {Tree: "t2", Manifest: strings.Repeat("3", 64)}, + }} +} + +// 1673/AC3: the previews key is .. and parses back. +func TestPreviewKey(t *testing.T) { + key, err := PreviewKey("novox/mesh-delivery", "aaaa", "bbbb") + if err != nil || key != "novox/mesh-delivery.aaaa.bbbb" { + t.Fatalf("key = %q, %v", key, err) + } + repo, head, trunk, err := ParsePreviewKey(key) + if err != nil || repo != "novox/mesh-delivery" || head != "aaaa" || trunk != "bbbb" { + t.Errorf("parse = %q %q %q %v", repo, head, trunk, err) + } + for _, bad := range [][3]string{{"nope", "aaaa", "bbbb"}, {"novox/x", "", "bbbb"}, {"novox/x", "aaaa", ""}, {"novox/x", "a.b", "bbbb"}, {"novox/x", "aaaa", "b b"}} { + if _, err := PreviewKey(bad[0], bad[1], bad[2]); err == nil { + t.Errorf("PreviewKey%v accepted", bad) + } + } + for _, bad := range []string{"", "novox/x.aaaa", "novox/x.aaaa.bbbb.cccc", "nope.aaaa.bbbb", "novox/x..bbbb", "novox/x.aaaa."} { + if _, _, _, err := ParsePreviewKey(bad); err == nil || !strings.Contains(err.Error(), "\""+bad+"\"") { + t.Errorf("ParsePreviewKey(%q) = %v, want a refusal naming it", bad, err) + } + } +} + +// 1673/AC4: a preview round trips, and an invalid one is refused naming the field. +func TestPreviewRoundTripAndRefusals(t *testing.T) { + data, err := EncodePreview(samplePreview()) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(string(data), `{"v":1,`) { + t.Errorf("at %s", data) + } + got, err := DecodePreview(data) + want := samplePreview() + want.V = Version + if err != nil || !SamePreview(got, want) { + t.Errorf("round trip = %+v, %v", got, err) + } + if _, err := EncodePreview(Preview{Repository: "novox/x", Head: "aaaa", Trunk: "bbbb", Merge: "cccc"}); err != nil { + t.Errorf("a preview of a head that changes no module refused: %v", err) + } + cases := map[string]struct { + edit func(*Preview) + words []string + }{ + "no repository": {func(p *Preview) { p.Repository = "" }, []string{"repository"}}, + "a bad repository": {func(p *Preview) { p.Repository = "nope" }, []string{"nope"}}, + "no head": {func(p *Preview) { p.Head = "" }, []string{"head"}}, + "no trunk": {func(p *Preview) { p.Trunk = "" }, []string{"trunk"}}, + "no merge": {func(p *Preview) { p.Merge = "" }, []string{"merge"}}, + "a module with no tree": {func(p *Preview) { + p.Modules["modules/mesh-delivery-view"] = PreviewModule{Manifest: strings.Repeat("3", 64)} + }, []string{"modules/mesh-delivery-view", "tree"}}, + "a bad manifest digest": {func(p *Preview) { + p.Modules["modules/mesh-delivery-view"] = PreviewModule{Tree: "t2", Manifest: "xyz"} + }, []string{"modules/mesh-delivery-view", "xyz"}}, + "an empty directory": {func(p *Preview) { p.Modules[""] = PreviewModule{Tree: "t", Manifest: strings.Repeat("3", 64)} }, []string{"directory"}}, + "a lock with no hash": {func(p *Preview) { + p.Modules["modules/mesh-delivery-view"] = PreviewModule{Tree: "t2", Manifest: strings.Repeat("3", 64), Locks: map[string]string{"go.sum": ""}} + }, []string{"modules/mesh-delivery-view", "go.sum"}}, + } + for name, c := range cases { + p := samplePreview() + c.edit(&p) + _, err := EncodePreview(p) + if err == nil { + t.Errorf("%s accepted", name) + continue + } + for _, w := range c.words { + if !strings.Contains(err.Error(), w) { + t.Errorf("%s: error %q does not name %q", name, err, w) + } + } + } + if _, err := DecodePreview([]byte(`{"v":2,"repository":"novox/x"}`)); !errors.Is(err, ErrUnknownVersion) { + t.Errorf("a preview of version 2: %v", err) + } + if _, err := DecodePreview([]byte(`{"v":1,"repository":"novox/x","head":"a","trunk":"b"}`)); err == nil { + t.Error("decoded a preview with no merge") + } +} + +// 1673/AC5: a reader of today's checker shapes ignores a field it does not know, and the checker's owed value is the +// gate's shape, so the gate's reader reads it (as indexer2_test.go does for the indexer's values). +func TestCheckerOwedIsReadableByTheGatesReaderAndAnOldOwedReaderIgnoresNewFields(t *testing.T) { + data, err := EncodeCheckerOwed(sampleCheckerOwed()) + if err != nil { + t.Fatal(err) + } + var old struct { + V int `json:"v"` + Act string `json:"act"` + Context string `json:"context"` + State string `json:"state"` + } + if err := json.Unmarshal(data, &old); err != nil || old.Act != OwedStatus || old.Context != "mesh/check" { + t.Errorf("the old reader: %+v %v", old, err) + } + withNew := strings.Replace(string(data), `{"v":1,`, `{"v":1,"a-field-of-tomorrow":7,`, 1) + if _, err := DecodeOwed([]byte(withNew)); err != nil { + t.Errorf("an owed value with a field of tomorrow: %v", err) + } + p, _ := EncodePreview(samplePreview()) + withNew = strings.Replace(string(p), `{"v":1,`, `{"v":1,"a-field-of-tomorrow":7,`, 1) + if _, err := DecodePreview([]byte(withNew)); err != nil { + t.Errorf("a preview with a field of tomorrow: %v", err) + } +} diff --git a/go/delivery/gate.go b/go/delivery/gate.go index 61ec74d..281ab13 100644 --- a/go/delivery/gate.go +++ b/go/delivery/gate.go @@ -28,6 +28,17 @@ const ( AnswersHistory = 5 ) +// The contexts of the statuses the mesh owes the forge: the gate's, the delivery's view of a head and of a group +// (design 47), and the checker's, which is not required until R4f (ADR 0484 clause 2, ADR 0453 clause 4). +const ( + StatusContextMergeGate = "mesh/merge-gate" + StatusContextDelivery = "mesh/delivery" + StatusContextDeliveryGroup = "mesh/delivery-group" + StatusContextCheck = "mesh/check" +) + +var statusContexts = []string{StatusContextMergeGate, StatusContextDelivery, StatusContextDeliveryGroup, StatusContextCheck} + // AskKindMerge is the kind of the gate's ask for a group's merge, the first part of its key. const AskKindMerge = "merge" @@ -131,6 +142,16 @@ func AskingID(askKey string, asking int) string { return AskID(askKey + "." + strconv.Itoa(asking)) } +// checkStatusContext refuses a status context the mesh does not owe the forge. +func checkStatusContext(context string) error { + for _, c := range statusContexts { + if context == c { + return nil + } + } + return fmt.Errorf("delivery: the status context %q is not one of %s", context, strings.Join(statusContexts, ", ")) +} + func checkOwedAct(act string) (kind, context string, err error) { kind, context, hasContext := strings.Cut(act, "=") switch kind { @@ -138,7 +159,7 @@ func checkOwedAct(act string) (kind, context string, err error) { if !hasContext { return "", "", fmt.Errorf("delivery: the owed act %q is a status without its context, status=", act) } - if err := checkName("status context", context); err != nil { + if err := checkStatusContext(context); err != nil { return "", "", err } return kind, context, nil @@ -345,6 +366,9 @@ func (o Owed) check() error { if o.Context == "" { return fmt.Errorf("delivery: an owed status has no context") } + if err := checkStatusContext(o.Context); err != nil { + return err + } if o.Target == "" { return fmt.Errorf("delivery: an owed status has no target") } diff --git a/go/delivery/preview.go b/go/delivery/preview.go new file mode 100644 index 0000000..67ce56c --- /dev/null +++ b/go/delivery/preview.go @@ -0,0 +1,168 @@ +package delivery + +import ( + "encoding/json" + "fmt" + "reflect" + "strings" +) + +// The checker's other buckets (ADR 0484 clauses 2 and 3): owed, the acts it owes the forge, which the forge bridge +// performs once per revision of a key, and previews, what the fetch step answered for a head merged onto a trunk commit. +const ( + // CheckerOwedBucket is the checker's bucket of owed acts, in the gate's owed shape: a status with the context + // StatusContextCheck for each head it judged. One part writes each key: the checker. + CheckerOwedBucket = CheckerModule + "_owed" + // CheckerOwedHistory is the revisions the checker's owed bucket keeps per key. + CheckerOwedHistory = 5 + // PreviewsBucket is the checker's bucket of merge previews; the indexer reads it for the candidates' fingerprints. + PreviewsBucket = CheckerModule + "_previews" + // PreviewsHistory is the revisions the previews bucket keeps per key. + PreviewsHistory = 5 +) + +// CheckerOwedKey is the key, in CheckerOwedBucket, of the status the checker owes on a pull request at a head: the +// gate's OwedKey with the act `status=mesh/check`, the only act the checker owes. +func CheckerOwedKey(repository string, number int, head string) (string, error) { + return OwedKey(repository, number, head, OwedStatus+"="+StatusContextCheck) +} + +// ParseCheckerOwedKey is the inverse of CheckerOwedKey; it refuses an owed key of any other act. +func ParseCheckerOwedKey(key string) (repository string, number int, head string, err error) { + repository, number, head, act, err := ParseOwedKey(key) + if err != nil { + return "", 0, "", err + } + if act != OwedStatus+"="+StatusContextCheck { + return "", 0, "", fmt.Errorf("delivery: %q is not a checker owed key: the checker owes only status=%s, not %q", key, StatusContextCheck, act) + } + return repository, number, head, nil +} + +// EncodeCheckerOwed is o as JSON at Version; it refuses what EncodeOwed refuses and any act but a status with the +// context StatusContextCheck. +func EncodeCheckerOwed(o Owed) ([]byte, error) { + if err := checkCheckerOwed(o); err != nil { + return nil, err + } + return EncodeOwed(o) +} + +func checkCheckerOwed(o Owed) error { + if o.Act != OwedStatus || o.Context != StatusContextCheck { + return fmt.Errorf("delivery: the checker owes only a status with the context %s, not the act %q with the context %q", StatusContextCheck, o.Act, o.Context) + } + return nil +} + +// DecodeCheckerOwed reads an owed act of the checker's bucket. +func DecodeCheckerOwed(data []byte) (Owed, error) { + o, err := DecodeOwed(data) + if err != nil { + return Owed{}, err + } + if err := checkCheckerOwed(o); err != nil { + return Owed{}, err + } + return o, nil +} + +// PreviewKey is the key, in PreviewsBucket, of a head merged onto a trunk commit: `..`. +func PreviewKey(repository, head, trunk string) (string, error) { + repo, err := escapeRepository(repository) + if err != nil { + return "", err + } + if err := checkName("head", head); err != nil { + return "", err + } + if err := checkName("trunk commit", trunk); err != nil { + return "", err + } + return repo + "." + head + "." + trunk, nil +} + +// ParsePreviewKey is the inverse of PreviewKey. +func ParsePreviewKey(key string) (repository, head, trunk string, err error) { + parts := strings.Split(key, ".") + if len(parts) != 3 { + return "", "", "", fmt.Errorf("delivery: %q is not a preview key, ..", key) + } + repository, err = unescapeRepository(parts[0]) + if err != nil { + return "", "", "", fmt.Errorf("delivery: %q is not a preview key: %w", key, err) + } + if _, err := PreviewKey(repository, parts[1], parts[2]); err != nil { + return "", "", "", fmt.Errorf("delivery: %q is not a preview key: %w", key, err) + } + return repository, parts[1], parts[2], nil +} + +// PreviewModule is what the preview holds of one module directory: the tree sha of the directory, the digest of its +// manifest (the key in the manifests bucket) and the hex sha256 of its lock files by path relative to it. +type PreviewModule struct { + Tree string `json:"tree"` + Manifest string `json:"manifest"` + Locks map[string]string `json:"locks,omitempty"` +} + +// Preview is the value of a preview key: the repository, the head and the trunk commit it was merged onto, the merge +// commit and, for each module directory of the merge, a PreviewModule. +type Preview struct { + V int `json:"v"` + Repository string `json:"repository"` + Head string `json:"head"` + Trunk string `json:"trunk"` + Merge string `json:"merge"` + Modules map[string]PreviewModule `json:"modules,omitempty"` +} + +func (p Preview) check() error { + if _, err := PreviewKey(p.Repository, p.Head, p.Trunk); err != nil { + return err + } + if p.Merge == "" { + return fmt.Errorf("delivery: a preview names no merge commit") + } + for dir, m := range p.Modules { + if dir == "" { + return fmt.Errorf("delivery: a preview names a module with an empty directory") + } + if m.Tree == "" { + return fmt.Errorf("delivery: the preview's module %q has no tree", dir) + } + if _, err := ManifestsKey(m.Manifest); err != nil { + return fmt.Errorf("delivery: the preview's module %q: %w", dir, err) + } + for path, hash := range m.Locks { + if path == "" || hash == "" { + return fmt.Errorf("delivery: the lock %q of the preview's module %q has an empty path or hash", path, dir) + } + } + } + return nil +} + +// EncodePreview is p as JSON at Version. +func EncodePreview(p Preview) ([]byte, error) { + if err := p.check(); err != nil { + return nil, err + } + p.V = Version + return json.Marshal(p) +} + +// DecodePreview reads a preview. +func DecodePreview(data []byte) (Preview, error) { + var p Preview + if err := decode(data, &p); err != nil { + return Preview{}, err + } + if err := p.check(); err != nil { + return Preview{}, err + } + return p, nil +} + +// SamePreview says whether two previews are the same value. +func SamePreview(a, b Preview) bool { return reflect.DeepEqual(a, b) } -- 2.54.0 From 4495a1660814d8494aa2e19d5ea8677b98295b91 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 19:22:33 +0200 Subject: [PATCH 4/8] Put the merge of a head onto a trunk commit into go/buildstep, so both build modules' fetch steps call one function and none copies the controller's (issue 1674) --- go/buildstep/merge.go | 112 ++++++++++++++++++ go/buildstep/merge_test.go | 232 +++++++++++++++++++++++++++++++++++++ 2 files changed, 344 insertions(+) create mode 100644 go/buildstep/merge.go create mode 100644 go/buildstep/merge_test.go diff --git a/go/buildstep/merge.go b/go/buildstep/merge.go new file mode 100644 index 0000000..6bc8df9 --- /dev/null +++ b/go/buildstep/merge.go @@ -0,0 +1,112 @@ +package buildstep + +import ( + "context" + "fmt" + "strings" + + "git.novox.be/novox/mesh-sdk/go/forgekey" +) + +// MergeResult is what MergeInto made of a head and a trunk commit: the trunk commit (Base) and the head, whole, and the +// merge commit and the paths the merge changes against the trunk commit; or, when the head does not merge cleanly, the +// files in conflict, with Merge and Changed empty. +type MergeResult struct { + Base, Head, Merge string + Conflicts []string + Changed []string +} + +// MergeInto makes, in the workspace dir that FetchCommit left at commit, the merge of commit into the trunk commit +// base, as the forge's default merge would (a merge commit, never a fast-forward), and leaves the workspace at the merge +// (novox/hq ADR 0484 clause 3, ADR 0323). It is the one merge the build modules' fetch steps make, with the identity +// and flags of the controller's check merge (mesh-controller internal/builder/check_merge.go): a name nobody owns, no +// signature, no hook, nothing of the machine's git configuration that changes a merge. Nothing is pushed. +// +// A merge needs both histories, so it makes the fetch whole (the shallow clone FetchCommit made is deepened) and +// fetches base by its sha. A head that does not merge cleanly is an answer, not an error: the files in conflict, the +// merge undone and the workspace left at base. A head already in base answers a Merge equal to Base and no changed +// paths. An error is that the merge could not be tried: a base or commit that is not a full sha, no base, a workspace +// not at commit, or a fetch or git command that failed. remote and the options are those of FetchCommit; the runner's +// bound must allow a whole fetch. +func MergeInto(ctx context.Context, run forgekey.Runner, dir, remote, base, commit string, opts ...Option) (MergeResult, error) { + var o fetchOptions + for _, opt := range opts { + opt(&o) + } + res := MergeResult{Base: base, Head: commit} + if base == "" { + return res, fmt.Errorf("buildstep: no base: a merge needs the trunk commit it is made onto") + } + if err := checkFetch(o, dir, remote, base, ""); err != nil { + return res, err + } + if err := checkFetch(o, dir, remote, commit, ""); err != nil { + return res, err + } + env := []string{"GIT_TERMINAL_PROMPT=0"} + git := func(args ...string) (string, error) { + full := append([]string{"-c", "credential.helper=", "-C", dir}, args...) + r := run(ctx, env, "git", full...) + if r.Status != 0 || r.Err != "" { + return "", fmt.Errorf("git %s: %s", args[0], firstLine(r.Err+" "+r.Stderr)) + } + return r.Stdout, nil + } + at, err := git("rev-parse", "HEAD") + if err != nil { + return res, err + } + if head := strings.TrimSpace(at); head != commit { + return res, fmt.Errorf("buildstep: the workspace is at %s, not at the commit %s", head, commit) + } + shallow, err := git("rev-parse", "--is-shallow-repository") + if err != nil { + return res, err + } + fetch := []string{"fetch", "--no-tags", "origin", base} + if strings.TrimSpace(shallow) == "true" { + fetch = []string{"fetch", "--no-tags", "--unshallow", "origin", base} + } + if _, err := git(fetch...); err != nil { + return res, fmt.Errorf("the trunk commit %s: %w", base, err) + } + if _, err := git("checkout", "-q", "--detach", base); err != nil { + return res, fmt.Errorf("the trunk commit %s cannot be checked out: %w", base, err) + } + message := fmt.Sprintf("Check the merge of %s into %s (made for a check, never pushed)", short(commit), short(base)) + args := append(append([]string{}, mergeIdentity...), "merge", "--no-ff", "--no-edit", "--no-verify", "--quiet", "-m", message, commit) + if _, err := git(args...); err != nil { + out, _ := git("diff", "--name-only", "--diff-filter=U") + res.Conflicts = strings.Fields(out) + _, _ = git("merge", "--abort") + if len(res.Conflicts) == 0 { + return res, fmt.Errorf("the head %s could not be merged into the trunk commit %s: %w", short(commit), short(base), err) + } + return res, nil + } + merge, err := git("rev-parse", "HEAD") + if err != nil { + return res, fmt.Errorf("the merge cannot be read: %w", err) + } + res.Merge = strings.TrimSpace(merge) + changed, err := git("diff", "--name-only", "--no-renames", base, res.Merge) + if err != nil { + return res, fmt.Errorf("the paths the merge changes cannot be read: %w", err) + } + res.Changed = strings.Fields(changed) + return res, nil +} + +// mergeIdentity is who a check's merge is made as: it is never pushed, so it names nobody's account. No hook of the +// clone's or the build machine's git configuration runs, and no setting there changes the merge (the controller's +// check merge, internal/builder/check_merge.go). +var mergeIdentity = []string{"-c", "user.name=mesh merge check", "-c", "user.email=merge-check@mesh.invalid", + "-c", "commit.gpgsign=false", "-c", "core.hooksPath=/dev/null", "-c", "merge.verifySignatures=false"} + +func short(commit string) string { + if len(commit) > 12 { + return commit[:12] + } + return commit +} diff --git a/go/buildstep/merge_test.go b/go/buildstep/merge_test.go new file mode 100644 index 0000000..4c59610 --- /dev/null +++ b/go/buildstep/merge_test.go @@ -0,0 +1,232 @@ +package buildstep + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "reflect" + "strings" + "testing" + + "git.novox.be/novox/mesh-sdk/go/forgekey" +) + +func realGit(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir, "-c", "user.name=t", "-c", "user.email=t@example.invalid", "-c", "commit.gpgsign=false"}, args...)...) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + return strings.TrimSpace(string(out)) +} + +func writeFile(t *testing.T, dir, name, content string) { + t.Helper() + p := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +// mergeRig is a local bare repository standing in for the forge, a work repository that pushes to it, and an empty +// workspace: the fetch step's shape (FetchCommit of the head into the workspace, then MergeInto). +type mergeRig struct { + t *testing.T + bare, work, ws string + fork, trunk string // the commit both histories share, and the trunk's head + remote string +} + +func newMergeRig(t *testing.T) *mergeRig { + root := t.TempDir() + r := &mergeRig{t: t, bare: filepath.Join(root, "r.git"), work: filepath.Join(root, "work"), ws: filepath.Join(root, "ws")} + for _, d := range []string{r.bare, r.work, r.ws} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + r.remote = "file://" + r.bare + realGit(t, r.bare, "init", "-q", "--bare") + realGit(t, r.work, "init", "-q", "-b", "main") + writeFile(t, r.work, "a.txt", "one\ntwo\nthree\n") + writeFile(t, r.work, "modules/m/module.json", "{}\n") + realGit(t, r.work, "add", ".") + realGit(t, r.work, "commit", "-q", "-m", "fork point") + r.fork = realGit(t, r.work, "rev-parse", "HEAD") + return r +} + +// head makes a commit on a branch off the fork point, pushes it as the pull request's head ref and answers its sha. +func (r *mergeRig) head(files map[string]string) string { + realGit(r.t, r.work, "checkout", "-q", "-B", "pr", r.fork) + for n, c := range files { + writeFile(r.t, r.work, n, c) + } + realGit(r.t, r.work, "add", ".") + realGit(r.t, r.work, "commit", "-q", "-m", "head") + h := realGit(r.t, r.work, "rev-parse", "HEAD") + realGit(r.t, r.work, "push", "-q", r.bare, "HEAD:refs/pull/1/head") + return h +} + +// advance moves the trunk past the fork point with a commit changing files, pushes it and answers its sha. +func (r *mergeRig) advance(files map[string]string) string { + realGit(r.t, r.work, "checkout", "-q", "-B", "main", r.fork) + for n, c := range files { + writeFile(r.t, r.work, n, c) + } + realGit(r.t, r.work, "add", ".") + realGit(r.t, r.work, "commit", "-q", "-m", "trunk moves") + r.trunk = realGit(r.t, r.work, "rev-parse", "HEAD") + realGit(r.t, r.work, "push", "-q", "-f", r.bare, "HEAD:refs/heads/main") + return r.trunk +} + +func (r *mergeRig) fetch(head string) { + if err := FetchCommit(context.Background(), forgekey.ExecRunner, r.ws, r.remote, head, "refs/pull/1/head", AllowFileRemote()); err != nil { + r.t.Fatal(err) + } +} + +// 1674/AC1: a clean merge answers the merge commit and the changed files, and leaves the workspace at the merge, which +// holds both sides' changes. +func TestMergeIntoACleanMergeAnswersTheMergeCommitAndTheChangedFiles(t *testing.T) { + r := newMergeRig(t) + head := r.head(map[string]string{"b.txt": "head\n"}) + base := r.advance(map[string]string{"a.txt": "one\ntwo\nthree\nfour\n"}) + r.fetch(head) + res, err := MergeInto(context.Background(), forgekey.ExecRunner, r.ws, r.remote, base, head, AllowFileRemote()) + if err != nil { + t.Fatal(err) + } + if res.Base != base || res.Head != head || len(res.Merge) != 40 || res.Merge == base || res.Merge == head || len(res.Conflicts) != 0 { + t.Fatalf("result = %+v", res) + } + if !reflect.DeepEqual(res.Changed, []string{"b.txt"}) { + t.Errorf("changed = %v, want only what the head adds to the trunk: b.txt", res.Changed) + } + if at := realGit(t, r.ws, "rev-parse", "HEAD"); at != res.Merge { + t.Errorf("the workspace is at %s, not at the merge %s", at, res.Merge) + } + for f, want := range map[string]string{"b.txt": "head\n", "a.txt": "one\ntwo\nthree\nfour\n"} { + if got, _ := os.ReadFile(filepath.Join(r.ws, f)); string(got) != want { + t.Errorf("%s = %q, want %q", f, got, want) + } + } + if parents := strings.Fields(realGit(t, r.ws, "rev-list", "--parents", "-n", "1", res.Merge)); len(parents) != 3 || parents[1] != base || parents[2] != head { + t.Errorf("the merge commit's parents = %v, want the trunk commit and the head", parents) + } + if author := realGit(t, r.ws, "log", "-1", "--format=%an <%ae>", res.Merge); author != "mesh merge check " { + t.Errorf("the merge is made as %q", author) + } +} + +// 1674/AC2: a conflict is an answer: the files in conflict, no merge commit, and the workspace left at the trunk commit. +func TestMergeIntoAConflictAnswersTheFilesAndLeavesTheTreeAtTheTrunk(t *testing.T) { + r := newMergeRig(t) + head := r.head(map[string]string{"a.txt": "one\nHEAD\nthree\n", "c.txt": "c\n"}) + base := r.advance(map[string]string{"a.txt": "one\nTRUNK\nthree\n"}) + r.fetch(head) + res, err := MergeInto(context.Background(), forgekey.ExecRunner, r.ws, r.remote, base, head, AllowFileRemote()) + if err != nil { + t.Fatalf("a conflict is an answer, not an error: %v", err) + } + if !reflect.DeepEqual(res.Conflicts, []string{"a.txt"}) || res.Merge != "" || len(res.Changed) != 0 || res.Base != base || res.Head != head { + t.Fatalf("result = %+v", res) + } + if at := realGit(t, r.ws, "rev-parse", "HEAD"); at != base { + t.Errorf("the workspace is at %s, not at the trunk commit %s", at, base) + } + if got, _ := os.ReadFile(filepath.Join(r.ws, "a.txt")); string(got) != "one\nTRUNK\nthree\n" { + t.Errorf("a.txt = %q, want the trunk's", got) + } + if st := realGit(t, r.ws, "status", "--porcelain"); st != "" { + t.Errorf("the tree is not clean after the aborted merge: %q", st) + } + if _, err := os.Stat(filepath.Join(r.ws, "c.txt")); err == nil { + t.Error("a file of the head is in a tree left at the trunk") + } +} + +// 1674/AC3: no base is refused before anything runs; so is a base or a commit that is not a full sha, a workspace that +// is not at the commit, and a remote that is not ssh://. +func TestMergeIntoRefusesWhatIsNotAMerge(t *testing.T) { + f := &fakeGit{} + for name, c := range map[string]struct{ remote, base, commit string }{ + "no base": {"ssh://mesh-forge/o/r.git", "", sha}, + "a short base": {"ssh://mesh-forge/o/r.git", "abc123", sha}, + "a branch as base": {"ssh://mesh-forge/o/r.git", "main", sha}, + "a short commit": {"ssh://mesh-forge/o/r.git", sha, "abc"}, + "an https remote": {"https://forge.example/o/r.git", sha, sha}, + } { + _, err := MergeInto(context.Background(), f.run, "/w", c.remote, c.base, c.commit) + if err == nil { + t.Errorf("%s accepted", name) + } + } + if len(f.calls) != 0 { + t.Errorf("git ran for a refused merge: %v", f.calls) + } + g := &fakeGit{rule: func(l string) forgekey.Ran { + if strings.Contains(l, "rev-parse HEAD") { + return forgekey.Ran{Stdout: strings.Repeat("f", 40) + "\n"} + } + return forgekey.Ran{} + }} + _, err := MergeInto(context.Background(), g.run, "/w", "ssh://mesh-forge/o/r.git", strings.Repeat("a", 40), sha) + if err == nil || !strings.Contains(err.Error(), "not at the commit") || len(g.calls) != 1 { + t.Errorf("a workspace at another commit: %v after %d commands", err, len(g.calls)) + } +} + +// 1674/AC4: the merge is made with the check merge's identity and flags, hooks off, and fetches with no credential. +func TestMergeIntoRunsTheCheckMergesCommands(t *testing.T) { + base := strings.Repeat("b", 40) + f := &fakeGit{rule: func(l string) forgekey.Ran { + switch { + case strings.HasSuffix(l, "rev-parse HEAD"): + return forgekey.Ran{Stdout: sha + "\n"} + case strings.Contains(l, "--is-shallow-repository"): + return forgekey.Ran{Stdout: "true\n"} + } + return forgekey.Ran{} + }} + if _, err := MergeInto(context.Background(), f.run, "/w", "ssh://mesh-forge/o/r.git", base, sha); err != nil { + t.Fatal(err) + } + all := strings.Join(f.calls, "\n") + for _, want := range []string{ + pre + "fetch --no-tags --unshallow origin " + base, + pre + "checkout -q --detach " + base, + "user.name=mesh merge check", "user.email=merge-check@mesh.invalid", "commit.gpgsign=false", "core.hooksPath=/dev/null", + "merge --no-ff --no-edit --no-verify --quiet -m", + pre + "diff --name-only --no-renames " + base, + } { + if !strings.Contains(all, want) { + t.Errorf("the commands lack %q:\n%s", want, all) + } + } + for _, c := range f.calls { + if strings.Contains(c, "https://") || !strings.Contains(c, "credential.helper= ") { + t.Errorf("command %q carries a credential or https", c) + } + } +} + +// A head already in the trunk merges into nothing: the merge is the trunk commit and no path changes. +func TestMergeIntoAHeadAlreadyInTheTrunkChangesNothing(t *testing.T) { + r := newMergeRig(t) + head := r.head(map[string]string{"b.txt": "head\n"}) + r.fork = head // the trunk continues from the head + base := r.advance(map[string]string{"a.txt": "one\ntwo\nthree\nfour\n"}) + r.fetch(head) + res, err := MergeInto(context.Background(), forgekey.ExecRunner, r.ws, r.remote, base, head, AllowFileRemote()) + if err != nil || res.Merge != base || len(res.Changed) != 0 || len(res.Conflicts) != 0 { + t.Errorf("result = %+v, %v", res, err) + } +} -- 2.54.0 From 0730ef609216798e787ee8b6542912f5ec613a77 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 19:26:02 +0200 Subject: [PATCH 5/8] Keep the unit level cheap: the merge's real-git tests start processes, so they are integration tests; the places tests wait 10 ms, not 50 (issue 895) --- go/buildstep/merge_integration_test.go | 172 +++++++++++++++++++++++++ go/buildstep/merge_test.go | 158 ----------------------- go/delivery/buildrun/places_test.go | 4 +- 3 files changed, 174 insertions(+), 160 deletions(-) create mode 100644 go/buildstep/merge_integration_test.go diff --git a/go/buildstep/merge_integration_test.go b/go/buildstep/merge_integration_test.go new file mode 100644 index 0000000..88b6d11 --- /dev/null +++ b/go/buildstep/merge_integration_test.go @@ -0,0 +1,172 @@ +//go:build integration + +package buildstep + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "reflect" + "strings" + "testing" + + "git.novox.be/novox/mesh-sdk/go/forgekey" +) + +// The merge on real git repositories starts git processes, so it is an integration test (the unit level keeps its bound, +// issue 895); the unit tests of merge_test.go prove the commands and the refusals with a fake runner. + +func realGit(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir, "-c", "user.name=t", "-c", "user.email=t@example.invalid", "-c", "commit.gpgsign=false"}, args...)...) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + return strings.TrimSpace(string(out)) +} + +func writeFile(t *testing.T, dir, name, content string) { + t.Helper() + p := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +// mergeRig is a local bare repository standing in for the forge, a work repository that pushes to it, and an empty +// workspace: the fetch step's shape (FetchCommit of the head into the workspace, then MergeInto). +type mergeRig struct { + t *testing.T + bare, work, ws string + fork, trunk string // the commit both histories share, and the trunk's head + remote string +} + +func newMergeRig(t *testing.T) *mergeRig { + root := t.TempDir() + r := &mergeRig{t: t, bare: filepath.Join(root, "r.git"), work: filepath.Join(root, "work"), ws: filepath.Join(root, "ws")} + for _, d := range []string{r.bare, r.work, r.ws} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + r.remote = "file://" + r.bare + realGit(t, r.bare, "init", "-q", "--bare") + realGit(t, r.work, "init", "-q", "-b", "main") + writeFile(t, r.work, "a.txt", "one\ntwo\nthree\n") + writeFile(t, r.work, "modules/m/module.json", "{}\n") + realGit(t, r.work, "add", ".") + realGit(t, r.work, "commit", "-q", "-m", "fork point") + r.fork = realGit(t, r.work, "rev-parse", "HEAD") + return r +} + +// head makes a commit on a branch off the fork point, pushes it as the pull request's head ref and answers its sha. +func (r *mergeRig) head(files map[string]string) string { + realGit(r.t, r.work, "checkout", "-q", "-B", "pr", r.fork) + for n, c := range files { + writeFile(r.t, r.work, n, c) + } + realGit(r.t, r.work, "add", ".") + realGit(r.t, r.work, "commit", "-q", "-m", "head") + h := realGit(r.t, r.work, "rev-parse", "HEAD") + realGit(r.t, r.work, "push", "-q", r.bare, "HEAD:refs/pull/1/head") + return h +} + +// advance moves the trunk past the fork point with a commit changing files, pushes it and answers its sha. +func (r *mergeRig) advance(files map[string]string) string { + realGit(r.t, r.work, "checkout", "-q", "-B", "main", r.fork) + for n, c := range files { + writeFile(r.t, r.work, n, c) + } + realGit(r.t, r.work, "add", ".") + realGit(r.t, r.work, "commit", "-q", "-m", "trunk moves") + r.trunk = realGit(r.t, r.work, "rev-parse", "HEAD") + realGit(r.t, r.work, "push", "-q", "-f", r.bare, "HEAD:refs/heads/main") + return r.trunk +} + +func (r *mergeRig) fetch(head string) { + if err := FetchCommit(context.Background(), forgekey.ExecRunner, r.ws, r.remote, head, "refs/pull/1/head", AllowFileRemote()); err != nil { + r.t.Fatal(err) + } +} + +// 1674/AC1: a clean merge answers the merge commit and the changed files, and leaves the workspace at the merge, which +// holds both sides' changes. +func TestMergeIntoACleanMergeAnswersTheMergeCommitAndTheChangedFiles(t *testing.T) { + r := newMergeRig(t) + head := r.head(map[string]string{"b.txt": "head\n"}) + base := r.advance(map[string]string{"a.txt": "one\ntwo\nthree\nfour\n"}) + r.fetch(head) + res, err := MergeInto(context.Background(), forgekey.ExecRunner, r.ws, r.remote, base, head, AllowFileRemote()) + if err != nil { + t.Fatal(err) + } + if res.Base != base || res.Head != head || len(res.Merge) != 40 || res.Merge == base || res.Merge == head || len(res.Conflicts) != 0 { + t.Fatalf("result = %+v", res) + } + if !reflect.DeepEqual(res.Changed, []string{"b.txt"}) { + t.Errorf("changed = %v, want only what the head adds to the trunk: b.txt", res.Changed) + } + if at := realGit(t, r.ws, "rev-parse", "HEAD"); at != res.Merge { + t.Errorf("the workspace is at %s, not at the merge %s", at, res.Merge) + } + for f, want := range map[string]string{"b.txt": "head\n", "a.txt": "one\ntwo\nthree\nfour\n"} { + if got, _ := os.ReadFile(filepath.Join(r.ws, f)); string(got) != want { + t.Errorf("%s = %q, want %q", f, got, want) + } + } + if parents := strings.Fields(realGit(t, r.ws, "rev-list", "--parents", "-n", "1", res.Merge)); len(parents) != 3 || parents[1] != base || parents[2] != head { + t.Errorf("the merge commit's parents = %v, want the trunk commit and the head", parents) + } + if author := realGit(t, r.ws, "log", "-1", "--format=%an <%ae>", res.Merge); author != "mesh merge check " { + t.Errorf("the merge is made as %q", author) + } +} + +// 1674/AC2: a conflict is an answer: the files in conflict, no merge commit, and the workspace left at the trunk commit. +func TestMergeIntoAConflictAnswersTheFilesAndLeavesTheTreeAtTheTrunk(t *testing.T) { + r := newMergeRig(t) + head := r.head(map[string]string{"a.txt": "one\nHEAD\nthree\n", "c.txt": "c\n"}) + base := r.advance(map[string]string{"a.txt": "one\nTRUNK\nthree\n"}) + r.fetch(head) + res, err := MergeInto(context.Background(), forgekey.ExecRunner, r.ws, r.remote, base, head, AllowFileRemote()) + if err != nil { + t.Fatalf("a conflict is an answer, not an error: %v", err) + } + if !reflect.DeepEqual(res.Conflicts, []string{"a.txt"}) || res.Merge != "" || len(res.Changed) != 0 || res.Base != base || res.Head != head { + t.Fatalf("result = %+v", res) + } + if at := realGit(t, r.ws, "rev-parse", "HEAD"); at != base { + t.Errorf("the workspace is at %s, not at the trunk commit %s", at, base) + } + if got, _ := os.ReadFile(filepath.Join(r.ws, "a.txt")); string(got) != "one\nTRUNK\nthree\n" { + t.Errorf("a.txt = %q, want the trunk's", got) + } + if st := realGit(t, r.ws, "status", "--porcelain"); st != "" { + t.Errorf("the tree is not clean after the aborted merge: %q", st) + } + if _, err := os.Stat(filepath.Join(r.ws, "c.txt")); err == nil { + t.Error("a file of the head is in a tree left at the trunk") + } +} + +// A head already in the trunk merges into nothing: the merge is the trunk commit and no path changes. +func TestMergeIntoAHeadAlreadyInTheTrunkChangesNothing(t *testing.T) { + r := newMergeRig(t) + head := r.head(map[string]string{"b.txt": "head\n"}) + r.fork = head // the trunk continues from the head + base := r.advance(map[string]string{"a.txt": "one\ntwo\nthree\nfour\n"}) + r.fetch(head) + res, err := MergeInto(context.Background(), forgekey.ExecRunner, r.ws, r.remote, base, head, AllowFileRemote()) + if err != nil || res.Merge != base || len(res.Changed) != 0 || len(res.Conflicts) != 0 { + t.Errorf("result = %+v, %v", res, err) + } +} diff --git a/go/buildstep/merge_test.go b/go/buildstep/merge_test.go index 4c59610..4eb0a88 100644 --- a/go/buildstep/merge_test.go +++ b/go/buildstep/merge_test.go @@ -2,157 +2,12 @@ package buildstep import ( "context" - "os" - "os/exec" - "path/filepath" - "reflect" "strings" "testing" "git.novox.be/novox/mesh-sdk/go/forgekey" ) -func realGit(t *testing.T, dir string, args ...string) string { - t.Helper() - cmd := exec.Command("git", append([]string{"-C", dir, "-c", "user.name=t", "-c", "user.email=t@example.invalid", "-c", "commit.gpgsign=false"}, args...)...) - out, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("git %v: %v\n%s", args, err, out) - } - return strings.TrimSpace(string(out)) -} - -func writeFile(t *testing.T, dir, name, content string) { - t.Helper() - p := filepath.Join(dir, name) - if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(p, []byte(content), 0o644); err != nil { - t.Fatal(err) - } -} - -// mergeRig is a local bare repository standing in for the forge, a work repository that pushes to it, and an empty -// workspace: the fetch step's shape (FetchCommit of the head into the workspace, then MergeInto). -type mergeRig struct { - t *testing.T - bare, work, ws string - fork, trunk string // the commit both histories share, and the trunk's head - remote string -} - -func newMergeRig(t *testing.T) *mergeRig { - root := t.TempDir() - r := &mergeRig{t: t, bare: filepath.Join(root, "r.git"), work: filepath.Join(root, "work"), ws: filepath.Join(root, "ws")} - for _, d := range []string{r.bare, r.work, r.ws} { - if err := os.MkdirAll(d, 0o755); err != nil { - t.Fatal(err) - } - } - r.remote = "file://" + r.bare - realGit(t, r.bare, "init", "-q", "--bare") - realGit(t, r.work, "init", "-q", "-b", "main") - writeFile(t, r.work, "a.txt", "one\ntwo\nthree\n") - writeFile(t, r.work, "modules/m/module.json", "{}\n") - realGit(t, r.work, "add", ".") - realGit(t, r.work, "commit", "-q", "-m", "fork point") - r.fork = realGit(t, r.work, "rev-parse", "HEAD") - return r -} - -// head makes a commit on a branch off the fork point, pushes it as the pull request's head ref and answers its sha. -func (r *mergeRig) head(files map[string]string) string { - realGit(r.t, r.work, "checkout", "-q", "-B", "pr", r.fork) - for n, c := range files { - writeFile(r.t, r.work, n, c) - } - realGit(r.t, r.work, "add", ".") - realGit(r.t, r.work, "commit", "-q", "-m", "head") - h := realGit(r.t, r.work, "rev-parse", "HEAD") - realGit(r.t, r.work, "push", "-q", r.bare, "HEAD:refs/pull/1/head") - return h -} - -// advance moves the trunk past the fork point with a commit changing files, pushes it and answers its sha. -func (r *mergeRig) advance(files map[string]string) string { - realGit(r.t, r.work, "checkout", "-q", "-B", "main", r.fork) - for n, c := range files { - writeFile(r.t, r.work, n, c) - } - realGit(r.t, r.work, "add", ".") - realGit(r.t, r.work, "commit", "-q", "-m", "trunk moves") - r.trunk = realGit(r.t, r.work, "rev-parse", "HEAD") - realGit(r.t, r.work, "push", "-q", "-f", r.bare, "HEAD:refs/heads/main") - return r.trunk -} - -func (r *mergeRig) fetch(head string) { - if err := FetchCommit(context.Background(), forgekey.ExecRunner, r.ws, r.remote, head, "refs/pull/1/head", AllowFileRemote()); err != nil { - r.t.Fatal(err) - } -} - -// 1674/AC1: a clean merge answers the merge commit and the changed files, and leaves the workspace at the merge, which -// holds both sides' changes. -func TestMergeIntoACleanMergeAnswersTheMergeCommitAndTheChangedFiles(t *testing.T) { - r := newMergeRig(t) - head := r.head(map[string]string{"b.txt": "head\n"}) - base := r.advance(map[string]string{"a.txt": "one\ntwo\nthree\nfour\n"}) - r.fetch(head) - res, err := MergeInto(context.Background(), forgekey.ExecRunner, r.ws, r.remote, base, head, AllowFileRemote()) - if err != nil { - t.Fatal(err) - } - if res.Base != base || res.Head != head || len(res.Merge) != 40 || res.Merge == base || res.Merge == head || len(res.Conflicts) != 0 { - t.Fatalf("result = %+v", res) - } - if !reflect.DeepEqual(res.Changed, []string{"b.txt"}) { - t.Errorf("changed = %v, want only what the head adds to the trunk: b.txt", res.Changed) - } - if at := realGit(t, r.ws, "rev-parse", "HEAD"); at != res.Merge { - t.Errorf("the workspace is at %s, not at the merge %s", at, res.Merge) - } - for f, want := range map[string]string{"b.txt": "head\n", "a.txt": "one\ntwo\nthree\nfour\n"} { - if got, _ := os.ReadFile(filepath.Join(r.ws, f)); string(got) != want { - t.Errorf("%s = %q, want %q", f, got, want) - } - } - if parents := strings.Fields(realGit(t, r.ws, "rev-list", "--parents", "-n", "1", res.Merge)); len(parents) != 3 || parents[1] != base || parents[2] != head { - t.Errorf("the merge commit's parents = %v, want the trunk commit and the head", parents) - } - if author := realGit(t, r.ws, "log", "-1", "--format=%an <%ae>", res.Merge); author != "mesh merge check " { - t.Errorf("the merge is made as %q", author) - } -} - -// 1674/AC2: a conflict is an answer: the files in conflict, no merge commit, and the workspace left at the trunk commit. -func TestMergeIntoAConflictAnswersTheFilesAndLeavesTheTreeAtTheTrunk(t *testing.T) { - r := newMergeRig(t) - head := r.head(map[string]string{"a.txt": "one\nHEAD\nthree\n", "c.txt": "c\n"}) - base := r.advance(map[string]string{"a.txt": "one\nTRUNK\nthree\n"}) - r.fetch(head) - res, err := MergeInto(context.Background(), forgekey.ExecRunner, r.ws, r.remote, base, head, AllowFileRemote()) - if err != nil { - t.Fatalf("a conflict is an answer, not an error: %v", err) - } - if !reflect.DeepEqual(res.Conflicts, []string{"a.txt"}) || res.Merge != "" || len(res.Changed) != 0 || res.Base != base || res.Head != head { - t.Fatalf("result = %+v", res) - } - if at := realGit(t, r.ws, "rev-parse", "HEAD"); at != base { - t.Errorf("the workspace is at %s, not at the trunk commit %s", at, base) - } - if got, _ := os.ReadFile(filepath.Join(r.ws, "a.txt")); string(got) != "one\nTRUNK\nthree\n" { - t.Errorf("a.txt = %q, want the trunk's", got) - } - if st := realGit(t, r.ws, "status", "--porcelain"); st != "" { - t.Errorf("the tree is not clean after the aborted merge: %q", st) - } - if _, err := os.Stat(filepath.Join(r.ws, "c.txt")); err == nil { - t.Error("a file of the head is in a tree left at the trunk") - } -} - // 1674/AC3: no base is refused before anything runs; so is a base or a commit that is not a full sha, a workspace that // is not at the commit, and a remote that is not ssh://. func TestMergeIntoRefusesWhatIsNotAMerge(t *testing.T) { @@ -217,16 +72,3 @@ func TestMergeIntoRunsTheCheckMergesCommands(t *testing.T) { } } } - -// A head already in the trunk merges into nothing: the merge is the trunk commit and no path changes. -func TestMergeIntoAHeadAlreadyInTheTrunkChangesNothing(t *testing.T) { - r := newMergeRig(t) - head := r.head(map[string]string{"b.txt": "head\n"}) - r.fork = head // the trunk continues from the head - base := r.advance(map[string]string{"a.txt": "one\ntwo\nthree\nfour\n"}) - r.fetch(head) - res, err := MergeInto(context.Background(), forgekey.ExecRunner, r.ws, r.remote, base, head, AllowFileRemote()) - if err != nil || res.Merge != base || len(res.Changed) != 0 || len(res.Conflicts) != 0 { - t.Errorf("result = %+v, %v", res, err) - } -} diff --git a/go/delivery/buildrun/places_test.go b/go/delivery/buildrun/places_test.go index 384c537..f7ff940 100644 --- a/go/delivery/buildrun/places_test.go +++ b/go/delivery/buildrun/places_test.go @@ -43,7 +43,7 @@ func TestThreeStepsOnANodeOfCapacityTwoRunTwoAtOnceAndTheThirdStartsWhenOneEnds( select { case <-third: t.Fatal("a third step started on a node of capacity 2 with two running") - case <-time.After(50 * time.Millisecond): + case <-time.After(10 * time.Millisecond): } if p.Running()["ace"] != 2 { t.Errorf("running = %v, want 2 on ace", p.Running()) @@ -65,7 +65,7 @@ func TestANodeWithNoSettingRunsOneStepAtATime(t *testing.T) { // round 3 select { case <-second: t.Fatal("two steps on a node without a setting") - case <-time.After(50 * time.Millisecond): + case <-time.After(10 * time.Millisecond): } first() r := <-second -- 2.54.0 From 9aaa5c80ac5c4820c4eddf488fbb90ac6981a624 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 19:35:14 +0200 Subject: [PATCH 6/8] Accept the gate's recheck status mesh/repo-check among the owed contexts, so a pinned recheck is not refused (issue 1673) --- go/delivery/checker_test.go | 26 ++++++++++++++++++++++++++ go/delivery/gate.go | 3 ++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/go/delivery/checker_test.go b/go/delivery/checker_test.go index eb79299..6b2f438 100644 --- a/go/delivery/checker_test.go +++ b/go/delivery/checker_test.go @@ -516,3 +516,29 @@ func TestCheckerOwedIsReadableByTheGatesReaderAndAnOldOwedReaderIgnoresNewFields t.Errorf("a preview with a field of tomorrow: %v", err) } } + +// 1673/AC2 (review): the gate's recheck owes a pending status=mesh/repo-check (mesh-catalog mesh-delivery-gate owed.go, +// checks.go ctxRepo); its key and value round trip, and the checker's bucket still refuses it. +func TestTheGatesRecheckStatusRoundTripsAndTheCheckersBucketRefusesIt(t *testing.T) { + key, err := OwedKey("novox/mesh-sdk", 36, "aaaa", "status=mesh/repo-check") + if err != nil || key != "novox/mesh-sdk.36.aaaa.status=mesh/repo-check" { + t.Fatalf("key = %q, %v", key, err) + } + if _, _, _, act, err := ParseOwedKey(key); err != nil || act != "status=mesh/repo-check" { + t.Errorf("parse = %q, %v", act, err) + } + o := Owed{Act: OwedStatus, Context: StatusContextRepoCheck, State: "pending", Target: "https://mesh.example/delivery/36", Text: "checking again"} + data, err := EncodeOwed(o) + if err != nil { + t.Fatal(err) + } + if got, err := DecodeOwed(data); err != nil || !SameOwed(got, o) && got.Context != o.Context { + t.Errorf("round trip = %+v, %v", got, err) + } + if _, err := EncodeCheckerOwed(o); err == nil { + t.Error("the checker's bucket takes the gate's recheck status") + } + if _, _, _, err := ParseCheckerOwedKey(key); err == nil { + t.Error("a checker owed key of the recheck status accepted") + } +} diff --git a/go/delivery/gate.go b/go/delivery/gate.go index 281ab13..18444c5 100644 --- a/go/delivery/gate.go +++ b/go/delivery/gate.go @@ -34,10 +34,11 @@ const ( StatusContextMergeGate = "mesh/merge-gate" StatusContextDelivery = "mesh/delivery" StatusContextDeliveryGroup = "mesh/delivery-group" + StatusContextRepoCheck = "mesh/repo-check" // the gate's pending status of a recheck (mesh-delivery.recheck) StatusContextCheck = "mesh/check" ) -var statusContexts = []string{StatusContextMergeGate, StatusContextDelivery, StatusContextDeliveryGroup, StatusContextCheck} +var statusContexts = []string{StatusContextMergeGate, StatusContextDelivery, StatusContextDeliveryGroup, StatusContextRepoCheck, StatusContextCheck} // AskKindMerge is the kind of the gate's ask for a group's merge, the first part of its key. const AskKindMerge = "merge" -- 2.54.0 From 78364e318bdcc0573405147291433410e652fb43 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 21:21:48 +0200 Subject: [PATCH 7/8] Put the module trees of a merge into go/buildstep for both fetch steps, and let a preview name the conflicts of a head that does not merge (issues 1675, 1676, 1445) --- go/buildstep/trees.go | 67 ++++++++++++++++++++++++++ go/buildstep/trees_test.go | 93 +++++++++++++++++++++++++++++++++++++ go/delivery/checker_test.go | 48 +++++++++++++++++++ go/delivery/preview.go | 17 ++++++- 4 files changed, 223 insertions(+), 2 deletions(-) create mode 100644 go/buildstep/trees.go create mode 100644 go/buildstep/trees_test.go diff --git a/go/buildstep/trees.go b/go/buildstep/trees.go new file mode 100644 index 0000000..58b2ccd --- /dev/null +++ b/go/buildstep/trees.go @@ -0,0 +1,67 @@ +package buildstep + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "fmt" + "os" + "path/filepath" + "strings" + + "git.novox.be/novox/mesh-sdk/go/forgekey" +) + +// ModuleTree is what a merge holds of one module directory: its tree id, the sha256 of its module.json and the sha256 +// of each lock file it has (go.sum, package-lock.json), from which the checker and the indexer take the module's inputs +// (novox/hq ADR 0484 clause 3). +type ModuleTree struct { + Tree string `json:"tree"` + Manifest string `json:"manifest"` + Locks map[string]string `json:"locks,omitempty"` +} + +var lockFiles = []string{"go.sum", "package-lock.json"} + +// ModuleTrees are the trees, in commit, of every directory of the workspace dir that holds a module.json, by directory +// (the repository's root is "."). The workspace is at commit, so the manifests and the lock files are read from it. env is +// the environment git runs with (nil for none). Both build modules' fetch steps answer it for a merge. +func ModuleTrees(ctx context.Context, run forgekey.Runner, env []string, dir, commit string) (map[string]ModuleTree, error) { + r := run(ctx, env, "git", "-C", dir, "ls-files", "--", "module.json", "*/module.json") + if r.Status != 0 || r.Err != "" { + return nil, fmt.Errorf("git ls-files: %s", firstLine(r.Err+" "+r.Stderr)) + } + trees := map[string]ModuleTree{} + for _, l := range strings.Split(r.Stdout, "\n") { + if l = strings.TrimSpace(l); l == "" { + continue + } + mdir := filepath.ToSlash(filepath.Dir(l)) + spec := commit + ":" + mdir + if mdir == "." { + spec = commit + "^{tree}" + } + t := run(ctx, env, "git", "-C", dir, "rev-parse", spec) + if t.Status != 0 || t.Err != "" { + return nil, fmt.Errorf("git rev-parse %s: %s", spec, firstLine(t.Err+" "+t.Stderr)) + } + mt := ModuleTree{Tree: strings.TrimSpace(t.Stdout)} + raw, err := os.ReadFile(filepath.Join(dir, filepath.FromSlash(l))) + if err != nil { + return nil, err + } + mt.Manifest = sha256Hex(raw) + for _, lock := range lockFiles { + if raw, err := os.ReadFile(filepath.Join(dir, filepath.FromSlash(mdir), lock)); err == nil { + if mt.Locks == nil { + mt.Locks = map[string]string{} + } + mt.Locks[lock] = sha256Hex(raw) + } + } + trees[mdir] = mt + } + return trees, nil +} + +func sha256Hex(b []byte) string { h := sha256.Sum256(b); return hex.EncodeToString(h[:]) } diff --git a/go/buildstep/trees_test.go b/go/buildstep/trees_test.go new file mode 100644 index 0000000..9cbbf0b --- /dev/null +++ b/go/buildstep/trees_test.go @@ -0,0 +1,93 @@ +package buildstep + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + + "git.novox.be/novox/mesh-sdk/go/forgekey" +) + +func hexOf(s string) string { h := sha256.Sum256([]byte(s)); return hex.EncodeToString(h[:]) } + +func writeTreeFile(t *testing.T, root, name, content string) { + t.Helper() + p := filepath.Join(root, filepath.FromSlash(name)) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +// ModuleTrees answers, for each directory holding a module.json (the root as "."), the tree id git gives, the sha256 of +// module.json and of each lock file present, and reads them from the workspace. +func TestModuleTreesAnswersTreeManifestAndLocksPerModuleDirectory(t *testing.T) { + root := t.TempDir() + writeTreeFile(t, root, "module.json", `{"root":1}`) + writeTreeFile(t, root, "go.sum", "rootsum") + writeTreeFile(t, root, "modules/a/module.json", `{"a":1}`) + writeTreeFile(t, root, "modules/a/go.sum", "asum") + writeTreeFile(t, root, "modules/a/package-lock.json", "alock") + writeTreeFile(t, root, "modules/b/module.json", `{"b":1}`) + merge := strings.Repeat("c", 40) + var calls []string + var envs [][]string + run := func(ctx context.Context, env []string, name string, args ...string) forgekey.Ran { + calls = append(calls, name+" "+strings.Join(args[2:], " ")) + envs = append(envs, env) + switch args[2] { + case "ls-files": + return forgekey.Ran{Stdout: "module.json\nmodules/a/module.json\nmodules/b/module.json\n"} + case "rev-parse": + return forgekey.Ran{Stdout: "tree-of-" + args[3] + "\n"} + } + return forgekey.Ran{Status: 1, Stderr: "unexpected"} + } + got, err := ModuleTrees(context.Background(), run, []string{"X=1"}, root, merge) + if err != nil { + t.Fatal(err) + } + want := map[string]ModuleTree{ + ".": {Tree: "tree-of-" + merge + "^{tree}", Manifest: hexOf(`{"root":1}`), Locks: map[string]string{"go.sum": hexOf("rootsum")}}, + "modules/a": {Tree: "tree-of-" + merge + ":modules/a", Manifest: hexOf(`{"a":1}`), Locks: map[string]string{"go.sum": hexOf("asum"), "package-lock.json": hexOf("alock")}}, + "modules/b": {Tree: "tree-of-" + merge + ":modules/b", Manifest: hexOf(`{"b":1}`)}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("trees = %+v\nwant %+v", got, want) + } + if got["modules/b"].Locks != nil { + t.Error("a module with no lock file carries a lock map") + } + for _, e := range envs { + if len(e) != 1 || e[0] != "X=1" { + t.Errorf("git ran with env %v, want the one given", e) + } + } +} + +func TestModuleTreesFailsNamingTheGitCommandOrTheUnreadableManifest(t *testing.T) { + root := t.TempDir() + fail := func(on string) forgekey.Runner { + return func(ctx context.Context, env []string, name string, args ...string) forgekey.Ran { + if args[2] == on { + return forgekey.Ran{Status: 128, Stderr: "fatal: no\n"} + } + return forgekey.Ran{Stdout: "modules/a/module.json\n"} + } + } + for _, on := range []string{"ls-files", "rev-parse"} { + if _, err := ModuleTrees(context.Background(), fail(on), nil, root, sha); err == nil || !strings.Contains(err.Error(), on) || !strings.Contains(err.Error(), "fatal: no") { + t.Errorf("%s failing: %v", on, err) + } + } + if _, err := ModuleTrees(context.Background(), fail("none"), nil, root, sha); err == nil { + t.Error("a module.json that is not in the workspace was read") + } +} diff --git a/go/delivery/checker_test.go b/go/delivery/checker_test.go index 6b2f438..296da52 100644 --- a/go/delivery/checker_test.go +++ b/go/delivery/checker_test.go @@ -542,3 +542,51 @@ func TestTheGatesRecheckStatusRoundTripsAndTheCheckersBucketRefusesIt(t *testing t.Error("a checker owed key of the recheck status accepted") } } + +// 1445/AC5 (layout): a head that does not merge is a preview naming its conflicting files, with no merge commit and no +// modules; a preview with conflicts and a merge or modules is refused; a preview of today's shape (no conflicts) +// decodes with none, and a reader of today's shape reads one with conflicts. +func TestPreviewOfAHeadInConflict(t *testing.T) { + c := Preview{Repository: "novox/mesh-delivery", Head: "aaaa", Trunk: "bbbb", Conflicts: []string{"a.txt", "modules/m/module.json"}} + data, err := EncodePreview(c) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(data), `"conflicts":["a.txt","modules/m/module.json"]`) || strings.Contains(string(data), `"merge"`) { + t.Errorf("at %s", data) + } + got, err := DecodePreview(data) + c.V = Version + if err != nil || !SamePreview(got, c) { + t.Errorf("round trip = %+v, %v", got, err) + } + withMerge := c + withMerge.Merge = "cccc" + withModules := c + withModules.Modules = samplePreview().Modules + emptyPath := c + emptyPath.Conflicts = []string{""} + for name, p := range map[string]Preview{"a merge commit": withMerge, "modules": withModules, "an empty conflicting path": emptyPath} { + if _, err := EncodePreview(p); err == nil || !strings.Contains(err.Error(), "onflict") { + t.Errorf("a preview in conflict with %s: %v", name, err) + } + } + // today's bytes: a clean preview has no conflicts and decodes with none. + clean, _ := EncodePreview(samplePreview()) + if strings.Contains(string(clean), "conflicts") { + t.Errorf("a clean preview carries conflicts: %s", clean) + } + if p, err := DecodePreview(clean); err != nil || p.Conflicts != nil { + t.Errorf("a clean preview decodes with conflicts %v, %v", p.Conflicts, err) + } + var old struct { + V int `json:"v"` + Repository string `json:"repository"` + Head string `json:"head"` + Trunk string `json:"trunk"` + Merge string `json:"merge"` + } + if err := json.Unmarshal(data, &old); err != nil || old.Head != "aaaa" || old.Merge != "" { + t.Errorf("the old reader: %+v %v", old, err) + } +} diff --git a/go/delivery/preview.go b/go/delivery/preview.go index 67ce56c..dd90cb7 100644 --- a/go/delivery/preview.go +++ b/go/delivery/preview.go @@ -107,13 +107,15 @@ type PreviewModule struct { } // Preview is the value of a preview key: the repository, the head and the trunk commit it was merged onto, the merge -// commit and, for each module directory of the merge, a PreviewModule. +// commit and, for each module directory of the merge, a PreviewModule. A head that does not merge is a preview too: it +// names the files in Conflicts and has no merge commit and no modules (issue 1445). type Preview struct { V int `json:"v"` Repository string `json:"repository"` Head string `json:"head"` Trunk string `json:"trunk"` - Merge string `json:"merge"` + Merge string `json:"merge,omitempty"` + Conflicts []string `json:"conflicts,omitempty"` Modules map[string]PreviewModule `json:"modules,omitempty"` } @@ -121,6 +123,17 @@ func (p Preview) check() error { if _, err := PreviewKey(p.Repository, p.Head, p.Trunk); err != nil { return err } + for _, f := range p.Conflicts { + if f == "" { + return fmt.Errorf("delivery: a preview names a conflicting file with an empty path") + } + } + if len(p.Conflicts) > 0 { + if p.Merge != "" || len(p.Modules) > 0 { + return fmt.Errorf("delivery: a preview of a head in conflict has no merge commit and no modules") + } + return nil + } if p.Merge == "" { return fmt.Errorf("delivery: a preview names no merge commit") } -- 2.54.0 From 580da61a9107f74fbef33388ece982e1b9a2e483 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 22:44:52 +0200 Subject: [PATCH 8/8] Name plans-building among the controller's readable states, for the checker's read (issue 1687, ADR 0483 clause 2) --- go/catalogue/state.go | 4 +++- go/catalogue/state_test.go | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/go/catalogue/state.go b/go/catalogue/state.go index 14e3432..87e402f 100644 --- a/go/catalogue/state.go +++ b/go/catalogue/state.go @@ -133,7 +133,9 @@ func StateProblems(m Manifest) []string { // controller's account alone (the writers table); declared as a module's state, they would be asserted a // second way and granted to the module's own principal as well. Named here so a read of one is a read of // kept state. The bus's list is broker.ControllerMirrors; a test keeps the two agreeing. -var ControllerMirrors = []string{"assignments", "declared-config", "predefined", "builds", "rollouts", "checks-running"} +// plans-building is the plans in their build phase, read by the checker to hold checks while a delivery builds (ADR 0483 +// clause 2, issue 1687). +var ControllerMirrors = []string{"assignments", "declared-config", "predefined", "builds", "rollouts", "checks-running", "plans-building"} // StateReadsNothingDeclares is every read across a catalogue whose owner is present and declares no // such state. An absent owner says nothing — a module may be installed long before the one whose diff --git a/go/catalogue/state_test.go b/go/catalogue/state_test.go index d4cf687..475ea2e 100644 --- a/go/catalogue/state_test.go +++ b/go/catalogue/state_test.go @@ -82,6 +82,16 @@ func TestAReadOfTheControllersMirrorsIsAReadOfKeptState(t *testing.T) { } } +// A module may read `mesh-controller.plans-building` (novox/hq ADR 0483 clause 2, issue 1687): the checker holds a check +// while a delivery builds, and the composed check refused its read as state the controller does not keep. +func TestAReadOfThePlansBuildingIsAReadOfKeptState(t *testing.T) { + controller := Manifest{Module: ControllerSeatName} + checker := Manifest{Module: "mesh-delivery-checker", Reads: []string{"mesh-controller.plans-building"}} + if p := StateReadsNothingDeclares([]Manifest{controller, checker}); len(p) != 0 { + t.Fatalf("a read of the controller's plans-building was refused: %v", p) + } +} + // **Across the whole catalogue**: every state name is local, and every read whose owner is present // names state that owner keeps. func TestEveryManifestsStateIsLocalAndEveryReadIsKept(t *testing.T) { -- 2.54.0