From 3a1937914e7099d7e363c52f81e305dd1f5087cc Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 18:23:23 +0200 Subject: [PATCH] =?UTF-8?q?Ask=20build-agent=20no=20composed=20check=20of?= =?UTF-8?q?=20a=20delivery=20group,=20and=20answer=20that=20the=20checker?= =?UTF-8?q?=20judges=20it=20(issue=201299,=20C2,=20ADR=200503=20=C2=A73)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../checks_stop_integration_test.go | 47 ++++++ cmd/mesh-controller/composed_verdict_test.go | 49 ------ cmd/mesh-controller/delivery.go | 143 +----------------- cmd/mesh-controller/delivery_edges_test.go | 29 ---- cmd/mesh-controller/pull_declares.go | 66 -------- cmd/mesh-controller/pull_declares_test.go | 47 +----- 6 files changed, 53 insertions(+), 328 deletions(-) diff --git a/cmd/mesh-controller/checks_stop_integration_test.go b/cmd/mesh-controller/checks_stop_integration_test.go index f37ac855..d6f32d1d 100644 --- a/cmd/mesh-controller/checks_stop_integration_test.go +++ b/cmd/mesh-controller/checks_stop_integration_test.go @@ -6,6 +6,9 @@ import ( "context" "encoding/json" "errors" + "io" + "os" + "strings" "testing" "git.novox.be/novox/mesh-sdk/go/catalogue" @@ -92,3 +95,47 @@ func TestThePullRequestChecksAreAskedOnlyForARepositoryTheCheckerDoesNotJudge(t t.Fatalf("0503 §5: the verdict said names a gate layer, or no repo-check layer: %+v", c) } } + +// C2 (ADR 0503 §3; issue 1299): a delivery group's composed check asks build-agent nothing and answers that the checker +// judges the group. +func TestTheGroupCheckAsksNothingAndNamesTheChecker(t *testing.T) { + aMesh(t) + ctx := t.Context() + js, _ := aBuildQueue(t) + servingBus.Store(js) + t.Cleanup(func() { servingBus.Store(nil) }) + members := `[{"id":"novox/a@111111111111","repository":"novox/a","base":"main","head":"1111111111111111111111111111111111111111","number":1},` + + `{"id":"novox/b@222222222222","repository":"novox/b","base":"main","head":"2222222222222222222222222222222222222222","number":2}]` + out := capturedStdout(t, func() error { + return deliveryCommand(ctx, []string{"check", "--group", "feat/x", "--members", members}) + }) + if !strings.Contains(out, `"judged-by": "mesh-delivery-checker"`) || !strings.Contains(out, `"asked": ""`) { + t.Fatalf("0503 §3: the group check answered %s", out) + } + q, err := link.ReadQueue(ctx, js, link.TheBuildMachine) + if err != nil { + t.Fatal(err) + } + if len(q.Asks) != 0 { + t.Fatalf("0503 §3: the group check asked %+v", q.Asks) + } +} + +// capturedStdout is what run writes to standard output. +func capturedStdout(t *testing.T, run func() error) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + was := os.Stdout + os.Stdout = w + runErr := run() + os.Stdout = was + w.Close() + out, _ := io.ReadAll(r) + if runErr != nil { + t.Fatal(runErr) + } + return string(out) +} diff --git a/cmd/mesh-controller/composed_verdict_test.go b/cmd/mesh-controller/composed_verdict_test.go index 5fcd49f7..58e4ee36 100644 --- a/cmd/mesh-controller/composed_verdict_test.go +++ b/cmd/mesh-controller/composed_verdict_test.go @@ -1,13 +1,10 @@ package main import ( - "context" "encoding/json" "strings" "testing" - "git.novox.be/novox/mesh-sdk/go/catalogue" - "github.com/novox/mesh-controller/internal/inventory" "github.com/novox/mesh-controller/internal/link" ) @@ -62,49 +59,3 @@ func TestTheComposedVerdictCarriesEveryMember(t *testing.T) { } } } - -// Members whose manifests order their modules in a cycle fail the composed check, naming every module and edge, and no -// ask is made of the build seat. -func TestAGroupInACycleFailsItsComposedCheckWithoutABuild(t *testing.T) { - members := []orderMember{ - {ID: "novox/x@" + headHost[:12], Repository: "novox/x", Base: "main", Head: headHost, Number: 7, Paths: []string{"module.json"}}, - {ID: "novox/y@" + headController[:12], Repository: "novox/y", Base: "main", Head: headController, Number: 9, Paths: []string{"module.json"}}, - } - reach := map[string]orderReach{members[0].ID: {Moved: []string{"mx"}}, members[1].ID: {Moved: []string{"my"}}} - was, wasParent := manifestAt, parentOf - t.Cleanup(func() { manifestAt, parentOf = was, wasParent }) - parentOf = func(context.Context, string, string, string) (string, error) { return "", nil } - manifestAt = func(_ context.Context, _, repo, _, ref string) (catalogue.Manifest, bool, error) { - if ref == "main" { - return catalogue.Manifest{}, false, nil // what the pull requests merge into holds no such manifest - } - if repo == "x" { - return catalogue.Manifest{Module: "mx", DefinesSeats: []catalogue.SeatDeclaration{{Name: "s", Serves: []catalogue.Verb{{Name: "a"}}}}, - Invokes: []string{"seat:s.b"}}, true, nil - } - return catalogue.Manifest{Module: "my", DefinesSeats: []catalogue.SeatDeclaration{{Name: "s", Serves: []catalogue.Verb{{Name: "b"}}}}, - Invokes: []string{"seat:s.a"}}, true, nil - } - c, cyclic, err := cycleVerdict(context.Background(), "feat/cycle", members, reach, []inventory.Edge(nil), nil) - if err != nil || !cyclic { - t.Fatalf("no cycle found: %v", err) - } - if c.Verdict != "fail" || c.Group != "feat/cycle" || len(c.Members) != 2 || c.Gate == nil || c.Gate.Verdict != "fail" { - t.Fatalf("the verdict is %+v", c) - } - for _, w := range []string{"mx", "my", "s.a", "s.b"} { - if !strings.Contains(c.Summary, w) { - t.Errorf("the cycle does not name %s: %s", w, c.Summary) - } - } - // Members with no edge between them are no cycle. - manifestsReadMu.Lock() - clear(manifestsRead) - manifestsReadMu.Unlock() - manifestAt = func(context.Context, string, string, string, string) (catalogue.Manifest, bool, error) { - return catalogue.Manifest{}, false, nil - } - if _, cyclic, err := cycleVerdict(context.Background(), "feat/cycle", members, reach, nil, nil); err != nil || cyclic { - t.Fatalf("a cycle where there is none: %v %v", cyclic, err) - } -} diff --git a/cmd/mesh-controller/delivery.go b/cmd/mesh-controller/delivery.go index e8af7c27..4e102431 100644 --- a/cmd/mesh-controller/delivery.go +++ b/cmd/mesh-controller/delivery.go @@ -914,11 +914,10 @@ func deliveryCommand(ctx context.Context, args []string) error { if *group == "" || len(members) < 2 { return errors.New("delivery check --group --members (two heads or more), or --members ") } - id, err := askGroupCheck(ctx, open, *group, members) - if err != nil { - return err - } - return answer(map[string]any{"asked": id, "group": *group}) + // **The group's composed check is the checker's** (novox/hq ADR 0503 §3; issue 1299, C2): nothing is asked of + // build-agent, and the answer says who judges it. + return answer(map[string]any{"asked": "", "group": *group, "judged-by": checkerModule, + "said": "the checker judges a delivery group's heads merged together (ADR 0503 §3): nothing was asked"}) case "go": if len(positionals) != 1 { return errors.New("delivery go [--by ] [--why ]") @@ -1093,38 +1092,6 @@ func theGraph(ctx context.Context, inv *inventory.Inventory) ([]inventory.Entry, return entries, read, edges, nil } -// cycleVerdict is the failing verdict of a group whose modules are ordered in a cycle, and whether there is one: the -// group's composed check, said without a build. The manifests' change is read from the forge; an answer it cannot give -// is an error, never read as no cycle. -func cycleVerdict(ctx context.Context, group string, members []orderMember, reach map[string]orderReach, - edges []inventory.Edge, entries []inventory.Entry) (link.Checked, bool, error) { - changes, err := changesOf(ctx, changeSourcesOf(members, entries, reach)) - if err != nil { - return link.Checked{}, false, fmt.Errorf("what the group's members say of their manifests cannot be read: %w", err) - } - var moved []string - for _, m := range members { - moved = appendNew(moved, reach[m.ID].Moved...) - } - said := orderEdges(moved, changes, edges).CycleSaid() - if said == "" { - return link.Checked{}, false, nil - } - first := members[0].pull() - c := link.Checked{ID: link.NewBuildID(time.Now()), Owner: first.Owner, Repo: first.Repo, Number: first.Number, - Commit: first.Commit, Verdict: "fail", Summary: said, Report: said, Group: group, - Gate: &link.CheckLayer{Verdict: "fail", Summary: said}} - for _, m := range members { - if group == "" { - break // a lone pull request's verdict names no group and no member - } - p := m.pull() - c.Members = append(c.Members, link.CheckedMember{Owner: p.Owner, Repo: p.Repo, Number: p.Number, Commit: p.Commit, - Trunk: twelve(p.BaseSHA)}) - } - return c, true, nil -} - // twelve is a commit as twelve characters. func twelve(c string) string { if len(c) > 12 { @@ -1133,99 +1100,6 @@ func twelve(c string) string { return c } -// groupPrimary is the head a group's composed check is run from: the one moving the controller, which then -// judges the group by itself; else the one moving the node-engine, whose validator judges; else the first. -func groupPrimary(members []orderMember, reach map[string]orderReach) int { - for _, want := range []string{"mesh-controller", "mesh-host"} { - for i, m := range members { - if slices.Contains(reach[m.ID].Moved, want) { - return i - } - } - } - return 0 -} - -// askGroupCheck asks the build seat for a group's composed check: every head laid over the mesh in turn, -// judged as one future state (novox/hq ADR 0239). The verdict comes back as `checked` with the group's id, -// for mesh-delivery; the forge's holder sets no status from it. -func askGroupCheck(ctx context.Context, open *stores, group string, members []orderMember) (string, error) { - entries, read, edges, err := theGraph(ctx, open.inventory) - if err != nil { - return "", err - } - reach := reachOfMembers(members, entries, read, edges) - // **A cycle of edges fails the group's composed check, naming every module and edge** (novox/hq ADR 0419 clause 6), - // before the build seat is asked: no composition of the heads has an order to be sent in. - if failing, cyclic, err := cycleVerdict(ctx, group, members, reach, edges, entries); err != nil { - return "", err - } else if cyclic { - sayChecked(ctx, os.Stderr, failing) - return failing.ID, nil - } - primary := groupPrimary(members, reach) - p := members[primary].pull() - scope := pullScope(p, entries, read, edges) - // The gate runs over what any member moves; a judge from the primary alone. - for _, m := range members { - r := reach[m.ID] - scope.Modules = appendNew(scope.Modules, r.Moved...) - scope.Dependents = appendNew(scope.Dependents, r.Dependents...) - } - request, err := checkRequestFor(ctx, open, p, scope, entries) - if err != nil { - return "", err - } - request.Check.Group = group - // **The values held for the keys the members introduce are told and carried** (novox/hq ADR 0502, issue 1826). - var pulls []pullAt - for i, s := range changeSourcesOf(members, entries, reach) { - pulls = append(pulls, pullAt{changeSource: s, Number: members[i].pull().Number}) - } - withHeldConfig(ctx, os.Stderr, &request, pulls, true) - // A group's check is queued as ready when any member's pull request is marked ready. - request.Check.Ready = slices.ContainsFunc(members, func(m orderMember) bool { return m.Ready }) - world, err := restOfTheMesh(ctx, open, shelfOf(entries), "") - if err != nil { - return "", err - } - for i, m := range members { - if i == primary { - continue - } - mp := m.pull() - // As the mesh clones a module built from it; a repository no module is built from, from the forge. - source := inventory.Source{Seat: gitSeat, Repository: mp.Owner + "/" + mp.Repo} - for _, e := range entries { - if !e.Provided && sameRepository(e.Source.Repository, link.SourceMoved{Owner: mp.Owner, Repo: mp.Repo}) { - source = e.Source - break - } - } - url := source.Repository - if source.Seat != "" { - url, err = clonedFromSeat(world, source.Seat, source.Repository) - } - if err != nil { - return "", fmt.Errorf("%s cannot be cloned for the group's check: %w", m.ID, err) - } - request.Check.Members = append(request.Check.Members, link.GroupMember{Owner: mp.Owner, Repo: mp.Repo, - Number: mp.Number, Repository: url, Ref: mp.Commit, Paths: mp.Paths, Base: mp.Base, Trunk: mp.BaseSHA}) - } - seat := buildSeatHeld(ctx) - ask, err := askOverOn(seat) - if err != nil { - return "", err - } - defer ask.Close() - if err := ask.Ask(ctx, request); err != nil { - return "", err - } - fmt.Fprintf(os.Stderr, "group %s: asked %s to check %d head(s) composed together, as %s\n", group, seat, - len(members), request.ID) - return request.ID, nil -} - // appendNew appends what is not there yet. func appendNew(to []string, items ...string) []string { for _, i := range items { @@ -1236,15 +1110,6 @@ func appendNew(to []string, items ...string) []string { return to } -// shelfOf is the catalogue's manifests by name. -func shelfOf(entries []inventory.Entry) map[string]catalogue.Manifest { - shelf := map[string]catalogue.Manifest{} - for _, e := range entries { - shelf[e.Manifest.Module] = e.Manifest - } - return shelf -} - // sayPlanMoved says a walk kept in a new state as the controller's `plan-moved`, the plan whole: what the // delivery it walks reads its steps from. Never in the way of the walk: said beside it, and a save that could // not be said is logged — the delivery's owner, which also asks for the walks it follows, finds it by diff --git a/cmd/mesh-controller/delivery_edges_test.go b/cmd/mesh-controller/delivery_edges_test.go index b858720c..96da775d 100644 --- a/cmd/mesh-controller/delivery_edges_test.go +++ b/cmd/mesh-controller/delivery_edges_test.go @@ -181,32 +181,3 @@ func TestATruncatedChangeIsReadAsChangingTheManifestsOfWhatItMoves(t *testing.T) t.Fatalf("the member's change is read as %+v", src) } } - -// A lone pull request's cycle fails its check, naming every module and edge, with no group and no members. -func TestALonePullRequestInACycleFailsWithNoGroup(t *testing.T) { - was, wasParent := manifestAt, parentOf - t.Cleanup(func() { manifestAt, parentOf = was, wasParent }) - manifestsReadMu.Lock() - clear(manifestsRead) - manifestsReadMu.Unlock() - const head = "3333333333333333333333333333333333333333" - parentOf = func(context.Context, string, string, string) (string, error) { return "", nil } - manifestAt = func(_ context.Context, _, _, file, ref string) (catalogue.Manifest, bool, error) { - if ref == "main" { - return catalogue.Manifest{}, false, nil - } - if file == "modules/x/module.json" { - return catalogue.Manifest{Module: "mx", DefinesSeats: []catalogue.SeatDeclaration{{Name: "s", Serves: []catalogue.Verb{{Name: "a"}}}}, - Invokes: []string{"seat:s.b"}}, true, nil - } - return catalogue.Manifest{Module: "my", DefinesSeats: []catalogue.SeatDeclaration{{Name: "s", Serves: []catalogue.Verb{{Name: "b"}}}}, - Invokes: []string{"seat:s.a"}}, true, nil - } - m := orderMember{ID: "novox/r@" + head[:12], Repository: "novox/r", Base: "main", Head: head, Number: 5, - Paths: []string{"modules/x/module.json", "modules/y/module.json"}} - c, cyclic, err := cycleVerdict(context.Background(), "", []orderMember{m}, map[string]orderReach{m.ID: {Moved: []string{"mx", "my"}}}, nil, nil) - if err != nil || !cyclic || c.Verdict != "fail" || c.Group != "" || len(c.Members) != 0 || !strings.Contains(c.Summary, "s.a") || - !strings.Contains(c.Summary, "my") { - t.Fatalf("the verdict is %+v (cyclic %v, %v)", c, cyclic, err) - } -} diff --git a/cmd/mesh-controller/pull_declares.go b/cmd/mesh-controller/pull_declares.go index b639a6b3..726be0ae 100644 --- a/cmd/mesh-controller/pull_declares.go +++ b/cmd/mesh-controller/pull_declares.go @@ -2,7 +2,6 @@ package main import ( "context" - "encoding/json" "fmt" "io" "path" @@ -138,71 +137,6 @@ func tellPullDeclares(ctx context.Context, w io.Writer, pulls []pullAt) { } } -// heldFor asks mesh-config for the approved held values of these pull requests at their heads (ADR 0502 clause 4). Only -// those of the pull requests asked about are carried. A failure is said to w and answers none. -func heldFor(ctx context.Context, w io.Writer, pulls []pullAt) []link.HeldConfig { - if len(pulls) == 0 { - return nil - } - type pullHead struct { - Pull string `json:"pull"` - Head string `json:"head"` - } - asked := make([]pullHead, 0, len(pulls)) - for _, p := range pulls { - asked = append(asked, pullHead{p.key(), p.New}) - } - raw, err := meshConfigAsk(ctx, map[string]any{"action": "held", "pulls": asked}) - if err != nil { - fmt.Fprintf(w, "mesh-config cannot say what it holds for these pull requests (%v); the check goes on without held values\n", err) - return nil // empty-on-error: said above; a check without held values fails for want of the value and says so, it never passes on a guess - } - var answer struct { - Held []link.HeldConfig `json:"held"` - Skipped []struct { - Pull string `json:"pull"` - Why string `json:"why"` - } `json:"skipped"` - } - if len(raw) == 0 { - return nil - } - if err := json.Unmarshal(raw, &answer); err != nil { - fmt.Fprintf(w, "mesh-config's held values cannot be read (%v); the check goes on without them\n", err) - return nil // empty-on-error: said above; a check without held values fails for want of the value and says so, it never passes on a guess - } - for _, s := range answer.Skipped { - fmt.Fprintf(w, "%s: mesh-config holds a value it does not carry: %s\n", s.Pull, s.Why) - } - var out []link.HeldConfig - for _, h := range answer.Held { - for _, p := range pulls { - if h.Pull == p.key() { - out = append(out, h) - break - } - } - } - return out -} - -// withHeldConfig tells mesh-config what these pull requests declare and puts the approved held values of them on the -// check request (ADR 0502 clause 4). With carry false, a member of a group whose composed check composes, only the -// declarations are told. -func withHeldConfig(ctx context.Context, w io.Writer, request *link.BuildRequest, pulls []pullAt, carry bool) { - tellPullDeclares(ctx, w, pulls) - if carry { - carryHeld(ctx, w, request, pulls) - } -} - -// carryHeld puts the approved held values of these pull requests on the check request. -func carryHeld(ctx context.Context, w io.Writer, request *link.BuildRequest, pulls []pullAt) { - if request.Check != nil { - request.Check.HeldConfig = heldFor(ctx, w, pulls) - } -} - // holdsModules is whether a module of the mesh is built from this repository. func holdsModules(entries []inventory.Entry, owner, repo string) bool { return slices.ContainsFunc(entries, func(e inventory.Entry) bool { diff --git a/cmd/mesh-controller/pull_declares_test.go b/cmd/mesh-controller/pull_declares_test.go index c325e79a..c136842e 100644 --- a/cmd/mesh-controller/pull_declares_test.go +++ b/cmd/mesh-controller/pull_declares_test.go @@ -9,7 +9,6 @@ import ( "git.novox.be/novox/mesh-sdk/go/catalogue" "github.com/novox/mesh-controller/internal/inventory" - "github.com/novox/mesh-controller/internal/link" ) const ( @@ -153,50 +152,8 @@ func TestMeshConfigNotAnsweringIsSaidAndGoneOn(t *testing.T) { meshConfigAsk = func(context.Context, map[string]any) ([]byte, error) { return nil, io.ErrUnexpectedEOF } var said strings.Builder tellPullDeclares(context.Background(), &said, []pullAt{thePull()}) - got := heldFor(context.Background(), &said, []pullAt{thePull()}) - if len(got) != 0 || !strings.Contains(said.String(), "mesh-config") { - t.Fatalf("held %v, said %q", got, said.String()) - } -} - -// 1826/AC2: the held values mesh-config answers for the check's pull requests are what the check request carries. -func TestTheHeldValuesOfTheChecksPullRequestsAreAsked(t *testing.T) { - calls := fakeConfig(t, map[string]string{"held": `{"held":[{"pull":"novox/mesh-catalog#434","head":"` + headSHA + - `","node":"ace","module":"n8n","key":"media-gid","value":2000,"approved":"the operator"}]}`}) - other := pullAt{Number: 7, changeSource: changeSource{Owner: "novox", Repo: "mesh-controller", New: baseSHA}} - got := heldFor(context.Background(), io.Discard, []pullAt{thePull(), other}) - if len(got) != 1 || got[0].Node != "ace" || got[0].Key != "media-gid" || string(got[0].Value) != "2000" { - t.Fatalf("got %+v", got) - } - c := (*calls)[0] - pulls, _ := json.Marshal(c["pulls"]) - if c["action"] != "held" || string(pulls) != `[{"pull":"novox/mesh-catalog#434","head":"`+headSHA+ - `"},{"pull":"novox/mesh-controller#7","head":"`+baseSHA+`"}]` { - t.Fatalf("asked %v %s", c, pulls) - } -} - -// 1826/AC2: a value held for a pull request the check is not of is never carried. -func TestAHeldValueOfAnotherPullRequestIsNotCarried(t *testing.T) { - fakeConfig(t, map[string]string{"held": `{"held":[{"pull":"novox/mesh-catalog#999","head":"x","node":"ace","module":"n8n","key":"k","value":1}]}`}) - if got := heldFor(context.Background(), io.Discard, []pullAt{thePull()}); len(got) != 0 { - t.Fatalf("carried %+v", got) - } -} - -// 1826/AC2: the check request carries what mesh-config answered; a member of a composed group asks none. -func TestTheCheckRequestCarriesTheHeldValues(t *testing.T) { - fakeManifests(t, map[string]*catalogue.Manifest{baseSHA: manifest("n8n", nil), headSHA: manifest("n8n", nil)}) - fakeConfig(t, map[string]string{"held": `{"held":[{"pull":"novox/mesh-catalog#434","node":"ace","module":"n8n","key":"media-gid","value":2000}]}`}) - request := link.BuildRequest{Check: &link.CheckRequest{}} - withHeldConfig(context.Background(), io.Discard, &request, []pullAt{thePull()}, true) - if len(request.Check.HeldConfig) != 1 || request.Check.HeldConfig[0].Key != "media-gid" { - t.Fatalf("the request carries %+v", request.Check.HeldConfig) - } - request = link.BuildRequest{Check: &link.CheckRequest{}} - withHeldConfig(context.Background(), io.Discard, &request, []pullAt{thePull()}, false) - if len(request.Check.HeldConfig) != 0 { - t.Fatalf("a composed member carries %+v", request.Check.HeldConfig) + if !strings.Contains(said.String(), "mesh-config") { + t.Fatalf("said %q", said.String()) } }