From 55fb32ba252c6b1137af7603bedf6d3c86e90cf0 Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 6 Nov 2026 01:43:40 +0100 Subject: [PATCH 1/3] Every build at a group's heads names every member at its head, also one that publishes no package yet (issue 1921) One shared function (sdk/grouppackages) for the check asks, the judge, the composer and a candidate's builds, so a producer cannot leave a member out. Claude-Session: https://claude.ai/code/session_01NcbgfFSnn24M3GcqttoU7i --- .../cmd/mesh-delivery-checker/main.go | 1 + .../internal/check/engines.go | 9 +- .../internal/check/engines_test.go | 7 +- .../internal/check/examiner_test.go | 9 +- .../internal/check/manifests_judge.go | 2 +- .../internal/check/pins.go | 86 ++++++++----------- .../internal/index/previews.go | 6 +- .../internal/index/previews_test.go | 49 +++++++++++ sdk/grouppackages/grouppackages.go | 67 +++++++++++++++ sdk/grouppackages/grouppackages_test.go | 51 +++++++++++ 10 files changed, 229 insertions(+), 58 deletions(-) create mode 100644 sdk/grouppackages/grouppackages.go create mode 100644 sdk/grouppackages/grouppackages_test.go diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go index d464578d..877f3627 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/main.go @@ -209,6 +209,7 @@ func run(ctx context.Context, getenv func(string) string, logOut io.Writer) erro // The indexer's packages state (ADR 0593 §2): followed once the bus has it, so a Go check builds every mesh package at // its published version; until then a check builds each at the version its go.mod names. published := &publishedPackages{} + compositions.Published = published process.AwaitBucket(ctx, js, conn, delivery.PackagesBucket, "a Go check builds each mesh package at the version its go.mod names", logger.Printf).Ready(func(kv jetstream.KeyValue) { w, err := settle(ctx, kv, func(e jetstream.KeyValueEntry) { published.Put(e.Key(), e.Value(), !opOf(e)) }) diff --git a/modules/mesh-delivery-checker/internal/check/engines.go b/modules/mesh-delivery-checker/internal/check/engines.go index 2e077789..91342266 100644 --- a/modules/mesh-delivery-checker/internal/check/engines.go +++ b/modules/mesh-delivery-checker/internal/check/engines.go @@ -52,7 +52,12 @@ type EngineCompositions struct { // Manifests are the indexer's trunk manifest digests by module, and Verdicts its refusals of them: the catalogue the // sandbox composes with, before the case's own manifests are laid over. Manifests TrunkManifests - Verdicts func() map[string]delivery.ManifestVerdict + // Published names the latest published version of every mesh package (Examiner.Published): the composer is built + // against them and the group's heads, as a check is (issue 1921); nil names the group's heads alone. + Published interface { + LatestPackages() delivery.PackageVersions + } + Verdicts func() map[string]delivery.ManifestVerdict } // composerHead is the node-engine member's head of a case, or empty. @@ -220,7 +225,7 @@ func (e EngineCompositions) inSandbox(ctx context.Context, c *Case, head, node s if err != nil { return a, err } - job := delivery.ComposeJob{Fetch: map[string]any{"repository": nodeEngineRepo, "commit": head}, Dir: delivery.RootPath, + job := delivery.ComposeJob{Fetch: withPackages(map[string]any{"repository": nodeEngineRepo, "commit": head}, packagesFor(e.Published, c, nodeEngineRepo)), Dir: delivery.RootPath, Package: composer.Package, Request: raw, Reader: "the node-engine head of " + c.Group} verdict, err := e.Sandbox.Compose(ctx, job) if err != nil { diff --git a/modules/mesh-delivery-checker/internal/check/engines_test.go b/modules/mesh-delivery-checker/internal/check/engines_test.go index fed2774f..259fd122 100644 --- a/modules/mesh-delivery-checker/internal/check/engines_test.go +++ b/modules/mesh-delivery-checker/internal/check/engines_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "git.novox.be/novox/mesh-sdk/go/buildstep" "git.novox.be/novox/mesh-sdk/go/composer" "git.novox.be/novox/mesh-sdk/go/delivery" ) @@ -150,7 +151,7 @@ func TestTheCompositionIsMadeFromTheEnginesAnswers(t *testing.T) { Machines: map[string]composer.Machine{node: {Composes: false, Reason: "the new composer refuses it"}}}}} }} ec := EngineCompositions{Asker: e, Placed: func() ([]string, error) { return []string{"anchor"}, nil }, Bound: time.Second, - Sandbox: sb, Manifests: digests{"web": "sha256:" + strings.Repeat("c", 64)}} + Sandbox: sb, Manifests: digests{"web": "sha256:" + strings.Repeat("c", 64)}, Published: published{}} v, err := ec.Compose(context.Background(), host) if err != nil { t.Fatal(err) @@ -165,6 +166,10 @@ func TestTheCompositionIsMadeFromTheEnginesAnswers(t *testing.T) { string(req.Input) != string(withInput.Input) || req.Cases[0].Manifests["web"].Digest == "" { t.Errorf("job = %+v, request %+v", job, req) } + // Issue 1921: the composer is built at the group's heads, also a member that publishes no package yet. + if pkgs, _ := buildstep.PackagesArg(job.Fetch["packages"]); len(pkgs) != 1 || pkgs[delivery.ForgeModulePrefix+"mesh-delivery"] != head1 { + t.Errorf("the composer is fetched with the packages %v, want the group's mesh-delivery member at its head %s alone", pkgs, head1) + } if v.State != delivery.CompositionFail || v.SDK != head2 || !strings.Contains(v.Words, "the new composer refuses it") { t.Errorf("verdict = %+v", v) } diff --git a/modules/mesh-delivery-checker/internal/check/examiner_test.go b/modules/mesh-delivery-checker/internal/check/examiner_test.go index 07a468f7..56d6b020 100644 --- a/modules/mesh-delivery-checker/internal/check/examiner_test.go +++ b/modules/mesh-delivery-checker/internal/check/examiner_test.go @@ -20,6 +20,7 @@ import ( gobuild "git.novox.be/novox/mesh-catalog/modules/mesh-build-go/buildstep" "git.novox.be/novox/mesh-delivery/modules/mesh-delivery-builder/buildrun" + "git.novox.be/novox/mesh-sdk/go/buildstep" "git.novox.be/novox/mesh-sdk/go/catalogue" "git.novox.be/novox/mesh-sdk/go/composer" "git.novox.be/novox/mesh-sdk/go/delivery" @@ -1125,7 +1126,8 @@ func TestTheIndexersJudgeJudgesTheChangesManifests(t *testing.T) { } p.Modules = []string{"web"} p.Fps = map[string]delivery.CandidateModule{"web": {FP2: "src2:" + strings.Repeat("f", 64), Manifest: web}} - cs := caseOf(t, p) + // Issue 1921: the judge is built at the group's heads too, also a member that publishes no package yet. + cs := caseOf(t, p, pullOf("novox/mesh-networking", 9, head2)) if row.try { cs = TryCase("try-1", Try{Members: []TryMember{{Repository: repoA, Head: head1, Trunk: trunkA}}}, nil) b.ex.Candidates = &fakeTryCandidates{cand: delivery.Candidate{FPSet: "fps", Pulls: []delivery.CandidatePull{p}}} @@ -1136,6 +1138,11 @@ func TestTheIndexersJudgeJudgesTheChangesManifests(t *testing.T) { }) { t.Errorf("%s: the case says %v, want the new indexer's manifest said accepted by the trunk's manifest validator", row.name, cs.notes) } + for _, j := range judged { + if pkgs, _ := buildstep.PackagesArg(j.Fetch["packages"]); !row.try && pkgs[delivery.ForgeModulePrefix+"mesh-networking"] != head2 { + t.Errorf("%s: the judge at %v is fetched with the packages %v, want the group's mesh-networking member at its head %s", row.name, j.Fetch["commit"], pkgs, head2) + } + } if row.ownJudge && (len(judged) != 2 || judged[0].Fetch["commit"] != head1 || judged[1].Fetch["commit"] != trunk) { t.Fatalf("%s: judged %+v, want the head's judge and then the trunk's", row.name, judged) } diff --git a/modules/mesh-delivery-checker/internal/check/manifests_judge.go b/modules/mesh-delivery-checker/internal/check/manifests_judge.go index 878fb438..31e8944c 100644 --- a/modules/mesh-delivery-checker/internal/check/manifests_judge.go +++ b/modules/mesh-delivery-checker/internal/check/manifests_judge.go @@ -182,7 +182,7 @@ func (st *state) runJudge(ctx context.Context, req composer.Request, touched []s if err != nil { return change, trunk, fail(goModule, err.Error()) } - job := delivery.ComposeJob{Fetch: map[string]any{"repository": DeliveryRepository, "commit": commit}, Dir: JudgeDir, + job := delivery.ComposeJob{Fetch: withPackages(map[string]any{"repository": DeliveryRepository, "commit": commit}, st.packagesOf(DeliveryRepository)), Dir: JudgeDir, Package: JudgePackage, Request: raw, Reader: "the indexer's judge at " + at} a := delivery.StepAsk{ID: buildstep.NewID(), Kind: delivery.AskCheck, Language: delivery.AskGo, Asker: delivery.CheckerModule, At: st.j.now().UTC(), Compose: &job} diff --git a/modules/mesh-delivery-checker/internal/check/pins.go b/modules/mesh-delivery-checker/internal/check/pins.go index 368bea05..3e418bd3 100644 --- a/modules/mesh-delivery-checker/internal/check/pins.go +++ b/modules/mesh-delivery-checker/internal/check/pins.go @@ -3,9 +3,8 @@ package check import ( "context" "errors" - "maps" - "strings" + "git.novox.be/novox/mesh-delivery/sdk/grouppackages" "git.novox.be/novox/mesh-sdk/go/delivery" ) @@ -32,59 +31,28 @@ type Forge interface { File(ctx context.Context, repository, path, ref string) (string, error) } -// packagesOf is the version of each mesh package a Go check of repository builds against (ADR 0573 §2, §4): the latest -// published version of each package the mesh's modules use (Published), and for a package one of the case's pull -// requests publishes, that pull request's head (mesh-sdk's Go module whenever mesh-sdk is a member). A package of -// repository itself comes with the tree the check fetched, and is left out. Nil for none. +// packagesOf is the version of each mesh package a Go build of repository (a check ask, the judge, the composer) builds +// against (ADR 0573 §2, §4): grouppackages.Heads over the latest published version of each package the mesh's modules use +// (Published) and the heads of the case's pull requests, so a member that publishes no package yet is named too (issue +// 1921). Nil for none. func (st *state) packagesOf(repository string) delivery.PackageVersions { - versions := delivery.PackageVersions{} - if st.j.Published != nil { - maps.Copy(versions, st.j.Published.LatestPackages()) + return packagesFor(st.j.Published, st.c, repository) +} + +// packagesFor is packagesOf for a case and the published versions: every producer of a build at a group's heads takes its +// versions from here, so none leaves a member out. +func packagesFor(published interface { + LatestPackages() delivery.PackageVersions +}, c *Case, repository string) delivery.PackageVersions { + var latest delivery.PackageVersions + if published != nil { + latest = published.LatestPackages() } - if st.c.member(SDKRepository) != nil { - if _, named := versions[SDKModulePath]; !named { - versions[SDKModulePath] = "" - } + heads := map[string]string{} + for _, m := range c.Pulls { + heads[m.Repository] = m.Head } - for pkg := range versions { - repo, _, ok := delivery.PackageRepository(pkg) - if !ok || delivery.SameRepository(repo, repository) { - delete(versions, pkg) - continue - } - if m := st.c.member(repo); m != nil && len(m.Head) == 40 { - versions[pkg] = m.Head - } - if versions[pkg] == "" { - delete(versions, pkg) - } - } - // A package of a member that no indexer state publishes yet (a repository whose packages are all new) is in no list above, - // and the build would look for it as the trunk's latest version, which does not exist (issue 1921). The member's root - // module path stands for its repository: the build module takes the head of a repository from any package of it. - for _, m := range st.c.Pulls { - if delivery.SameRepository(m.Repository, repository) || len(m.Head) != 40 { - continue - } - name := strings.TrimPrefix(m.Repository, "novox/") - if m.Repository == name { - continue - } - held := false - for pkg := range versions { - if repo, _, ok := delivery.PackageRepository(pkg); ok && repo == m.Repository { - held = true - break - } - } - if !held { - versions[delivery.ForgeModulePrefix+name] = m.Head - } - } - if len(versions) == 0 { - return nil - } - return versions + return grouppackages.Heads(latest, heads, repository) } // unheardWords ends the words of a failed read of the forge that reached nobody: the forge was not heard (UnheardWords). @@ -103,3 +71,17 @@ func forgeMachine(err error, node string) string { } return node } + +// withPackages puts the versions into a fetch's arguments (the fetch tool's packages argument, which a compose ask has no +// field for: a compose ask names none, StepAsk.Packages), so the composer or the judge is built at the group's heads. +func withPackages(fetch map[string]any, versions delivery.PackageVersions) map[string]any { + if len(versions) == 0 { + return fetch + } + packages := make(map[string]any, len(versions)) + for pkg, version := range versions { + packages[pkg] = version + } + fetch["packages"] = packages + return fetch +} diff --git a/modules/mesh-delivery-indexer/internal/index/previews.go b/modules/mesh-delivery-indexer/internal/index/previews.go index 18bf869d..ac9fecc6 100644 --- a/modules/mesh-delivery-indexer/internal/index/previews.go +++ b/modules/mesh-delivery-indexer/internal/index/previews.go @@ -8,6 +8,7 @@ import ( "slices" "strings" + "git.novox.be/novox/mesh-delivery/sdk/grouppackages" "git.novox.be/novox/mesh-sdk/go/delivery" "mesh-delivery-indexer/internal/fingerprint" @@ -318,7 +319,10 @@ func (ix *Indexer) candidateModule(ctx context.Context, m, dir string, pv delive out.None2 = none return out } - out.FP2, out.Packages = fp, versions + // The ask of the candidate's build names every member of the group at its head, also one the module names no package of + // (a member that publishes none yet, or one the module reaches through mesh-sdk): the fingerprint stays the trunk's, so + // the build is the one the merge keeps (grouppackages.Heads, issue 1921). + out.FP2, out.Packages = fp, grouppackages.Heads(versions, members, pv.Repository) if len(inputs.Paths) > 0 { out.Paths = inputs.Paths // the builder's fetch for the candidate names them (ADR 0499 clause 4) } diff --git a/modules/mesh-delivery-indexer/internal/index/previews_test.go b/modules/mesh-delivery-indexer/internal/index/previews_test.go index 1d95b041..1b8c850d 100644 --- a/modules/mesh-delivery-indexer/internal/index/previews_test.go +++ b/modules/mesh-delivery-indexer/internal/index/previews_test.go @@ -138,6 +138,55 @@ func TestACandidatesModuleGetsTheFingerprintATrunkModuleOfTheSameTreeHas(t *test } } +// Issue 1921: a candidate's build is asked every member of the group at its head, also one that publishes no package yet +// and one the module names no package of; the fingerprint stays the trunk's, so the build is the one the merge keeps. +func TestACandidatesBuildNamesEveryMemberOfItsGroupAtItsHead(t *testing.T) { + for _, tc := range []struct { + name string + members map[string]string // the other pull requests of the group: repository, head + want delivery.PackageVersions + }{ + {"a group of one names nothing", nil, nil}, + {"a member that publishes no package yet", map[string]string{"novox/mesh-contracts": h2}, + delivery.PackageVersions{delivery.ForgeModulePrefix + "mesh-contracts": h2}}, + {"mesh-sdk by its go module", map[string]string{"novox/mesh-sdk": h2, "novox/mesh-contracts": h3}, + delivery.PackageVersions{delivery.ForgeModulePrefix + "mesh-sdk/go": h2, delivery.ForgeModulePrefix + "mesh-contracts": h3}}, + } { + t.Run(tc.name, func(t *testing.T) { + ix, mods, cands := previewBed(t, true) + trunk, _ := moduleOf(t, mods, "avahi") + if err := ix.Apply(context.Background(), avahiPull(t, h1)); err != nil { + t.Fatal(err) + } + for repository, head := range tc.members { + other := forgemsg.Forge{V: delivery.Version, Kind: forgemsg.ForgePull, Repository: repository, Branch: "feat/x", Number: 7, Head: head, + Action: forgemsg.PullUpdated} + if err := ix.Apply(context.Background(), other); err != nil { + t.Fatal(err) + } + } + applyPreview(t, ix, avahiPreview(t, trunkNow, h1)) + var got delivery.CandidateModule + for _, p := range candidateOf(t, cands).Pulls { + if p.Repository == catalogueRepo { + got = p.Fps["avahi"] + } + } + if got.FP2 != trunk.FP2 { + t.Fatalf("candidate avahi = %+v, want the trunk's fingerprint %s", got, trunk.FP2) + } + if len(got.Packages) != len(tc.want) { + t.Fatalf("packages = %v, want %v", got.Packages, tc.want) + } + for pkg, head := range tc.want { + if got.Packages[pkg] != head { + t.Errorf("%s = %q, want %q", pkg, got.Packages[pkg], head) + } + } + }) + } +} + func TestAnUnknownToolchainLeavesTheCandidatesModuleNamingIt(t *testing.T) { // 1445/AC2 ix, _, cands := previewBed(t, false) if err := ix.Apply(context.Background(), avahiPull(t, h1)); err != nil { diff --git a/sdk/grouppackages/grouppackages.go b/sdk/grouppackages/grouppackages.go new file mode 100644 index 00000000..82f706fa --- /dev/null +++ b/sdk/grouppackages/grouppackages.go @@ -0,0 +1,67 @@ +// Package grouppackages is the one place that names the versions a build at a delivery group's heads uses (novox/hq +// ADR 0573 §2, §4; issue 1921). A build writes into its workspace the version of every mesh package it is asked, whatever +// its go.mod names; a requirement at the zero version that no ask names a commit for cannot be fetched. So every part +// that asks a build for a check, a try or a candidate (the checker's check, judge and compose asks, the indexer's +// candidate modules) takes the versions from Heads, and a member of the group that publishes no package yet is named too. +package grouppackages + +import ( + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +// RootModules are the repositories whose Go module is not at their root, by repository: the module a member of that +// repository stands for when no package of it is known yet. +var RootModules = map[string]string{ + "novox/mesh-sdk": delivery.ForgeModulePrefix + "mesh-sdk/go", +} + +// Heads is the version of each mesh package a build of own builds against: latest (the latest published version of each +// package the mesh's modules use, nil for none) with every package of a group member at that member's head (heads, by +// repository /, a full commit each), and for a member of which latest holds no package, the member's root +// module path (RootModules where the module is not at the root) at its head: the build module takes the head of a +// repository from any package of it. A package of own comes with the tree the build fetched, and is left out, as is a +// package no version is known of. Nil for none. latest is not changed. +func Heads(latest delivery.PackageVersions, heads map[string]string, own string) delivery.PackageVersions { + versions := delivery.PackageVersions{} + held := map[string]bool{} + for pkg, version := range latest { + repo, _, ok := delivery.PackageRepository(pkg) + if !ok || delivery.SameRepository(repo, own) { + continue + } + if head := heads[repo]; len(head) == 40 { + version = head + } + if version == "" { + continue + } + versions[pkg] = version + held[repo] = true + } + for repo, head := range heads { + if delivery.SameRepository(repo, own) || len(head) != 40 || held[repo] { + continue + } + root, ok := RootModules[repo] + if !ok { + name, found := cutOwner(repo) + if !found { + continue + } + root = delivery.ForgeModulePrefix + name + } + versions[root] = head + } + if len(versions) == 0 { + return nil + } + return versions +} + +func cutOwner(repo string) (string, bool) { + const owner = "novox/" + if len(repo) > len(owner) && repo[:len(owner)] == owner { + return repo[len(owner):], true + } + return "", false +} diff --git a/sdk/grouppackages/grouppackages_test.go b/sdk/grouppackages/grouppackages_test.go new file mode 100644 index 00000000..0727e9ab --- /dev/null +++ b/sdk/grouppackages/grouppackages_test.go @@ -0,0 +1,51 @@ +package grouppackages + +import ( + "strings" + "testing" + + "git.novox.be/novox/mesh-sdk/go/delivery" +) + +func TestHeads(t *testing.T) { + h := func(c string) string { return strings.Repeat(c, 40) } + const ( + contracts = "git.novox.be/novox/mesh-contracts/go/clients/assignerclient" + sdk = "git.novox.be/novox/mesh-sdk/go" + tools = "git.novox.be/novox/mesh-tools/go/x" + ) + for _, tc := range []struct { + name string + latest delivery.PackageVersions + heads map[string]string + own string + want delivery.PackageVersions + }{ + {"none", nil, nil, "novox/mesh-host", nil}, + {"trunk latest alone", delivery.PackageVersions{tools: h("a")}, nil, "novox/mesh-host", delivery.PackageVersions{tools: h("a")}}, + {"a member at its head replaces the latest", delivery.PackageVersions{sdk: h("a")}, map[string]string{"novox/mesh-sdk": h("b")}, "novox/mesh-host", + delivery.PackageVersions{sdk: h("b")}}, + {"a member that publishes no package yet is named by its root module", delivery.PackageVersions{sdk: h("a")}, + map[string]string{"novox/mesh-contracts": h("c")}, "novox/mesh-host", + delivery.PackageVersions{sdk: h("a"), "git.novox.be/novox/mesh-contracts": h("c")}}, + {"mesh-sdk named by its go module before any package of it is known", nil, map[string]string{"novox/mesh-sdk": h("b")}, "novox/mesh-host", + delivery.PackageVersions{sdk: h("b")}}, + {"a member whose packages are known gets no root", delivery.PackageVersions{contracts: h("a")}, + map[string]string{"novox/mesh-contracts": h("c")}, "novox/mesh-host", delivery.PackageVersions{contracts: h("c")}}, + {"the build's own repository is left out", delivery.PackageVersions{contracts: h("a")}, + map[string]string{"novox/mesh-contracts": h("c")}, "novox/mesh-contracts", nil}, + {"a head that is no full commit names nothing", nil, map[string]string{"novox/mesh-contracts": "abc"}, "novox/mesh-host", nil}, + } { + t.Run(tc.name, func(t *testing.T) { + got := Heads(tc.latest, tc.heads, tc.own) + if len(got) != len(tc.want) { + t.Fatalf("got %v, want %v", got, tc.want) + } + for k, v := range tc.want { + if got[k] != v { + t.Errorf("%s = %q, want %q (all: %v)", k, got[k], v, got) + } + } + }) + } +} From 4907470e90d153d129703d96d422a320d871170f Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 6 Nov 2026 01:44:57 +0100 Subject: [PATCH 2/3] A group that holds the node-engine is judged with it beside at the member's head, not the running commit (issue 1921) Claude-Session: https://claude.ai/code/session_01NcbgfFSnn24M3GcqttoU7i --- .../mesh-delivery-checker/internal/check/beside.go | 11 +++++++++-- .../internal/check/pins_test.go | 12 ++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/modules/mesh-delivery-checker/internal/check/beside.go b/modules/mesh-delivery-checker/internal/check/beside.go index d0fa103c..0b8a584d 100644 --- a/modules/mesh-delivery-checker/internal/check/beside.go +++ b/modules/mesh-delivery-checker/internal/check/beside.go @@ -50,8 +50,9 @@ var besideRules = map[string][]string{ "novox/mesh-delivery": {SDKRepository, catalogueRepository}, } -// besideFor are the siblings a pull request's tests read (ADR 0484 clause 4): the node-engine at the commit the mesh runs -// (the commit of the indexer's module mesh-host, its trunk head when none is known); any other sibling, the catalogue +// besideFor are the siblings a pull request's tests read (ADR 0484 clause 4): the node-engine at the head of the group's pull +// request for it when the group holds one, else at the commit the mesh runs (the commit of the indexer's module mesh-host, +// its trunk head when none is known); any other sibling, the catalogue // included, at the head of the group's pull request for it when the group holds one, else at its trunk, since a group's // check judges each member against the group's heads (ADR 0482 §2, ADR 0573 §4; issue 1126: a judge a group changed was // judged against the catalogue's trunk). And first, the change's own repository at the trunk it is merged onto, so a test @@ -68,6 +69,12 @@ func (j *Examiner) besideFor(ctx context.Context, c *Case, p Pull) ([]BesideRef, } for _, repo := range besideRules[p.Repository] { switch { + case repo == nodeEngineRepo && c.member(repo) != nil: + // A group's check judges each member against the group's heads (ADR 0573 §4, ADR 0482 §2), and the group's + // node-engine is what runs after its merge: it is placed at the member's head, as any member is. Neither atTrunk + // nor runs, so no move of the trunk or of the running commit puts the verdict out of date (besideAtTrunk). + member := c.member(repo) + out = append(out, BesideRef{Repository: repo, Commit: member.Head, Ref: refOf(member.Number)}) case repo == nodeEngineRepo: commit := "" if j.Runs != nil { diff --git a/modules/mesh-delivery-checker/internal/check/pins_test.go b/modules/mesh-delivery-checker/internal/check/pins_test.go index 31dc4577..d0ad069f 100644 --- a/modules/mesh-delivery-checker/internal/check/pins_test.go +++ b/modules/mesh-delivery-checker/internal/check/pins_test.go @@ -183,6 +183,18 @@ func TestACatalogueChangeHasTheCatalogueBesideItAtItsTrunk(t *testing.T) { if h := got[nodeEngineRepo]; h.Commit != hostRuns { t.Errorf("the node-engine beside a catalogue change = %+v, want the commit the mesh runs %s", h, hostRuns) } + // A group that holds the node-engine is judged against its head, which is what runs after its merge, never the commit + // the mesh runs now; the verdict names it in no carry (it is a member, not read at a trunk or a running commit). + b = newBench(t) + b.ex.Trunks = trunksOf(t, map[string]string{catalogueRepository: trunkA, SDKRepository: trunkS, nodeEngineRepo: trunkA2}) + b.ex.Runs = runsAt{nodeEngineModule: hostRuns} + out = b.judge(t, caseOf(t, pullOf(catalogueRepository, 12, head1), pullOf(nodeEngineRepo, 8, head2))) + if h := besideOf(t, b, catalogueRepository)[nodeEngineRepo]; h.Commit != head2 || h.Ref != "refs/pull/8/head" { + t.Errorf("the node-engine beside a catalogue change in a group holding it = %+v, want the member's head %s", h, head2) + } + if out.Beside[nodeEngineRepo] != "" { + t.Errorf("the verdict names the member node-engine in its carry: %v", out.Beside) + } } // 1887/A12: a mesh-lab head gets a check: with its trunk known it is judged like any repository, its Go fetch finding From b1094a926592b32e3e01f70d1da71add4a42e19f Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 6 Nov 2026 01:51:23 +0100 Subject: [PATCH 3/3] The trunk's manifest validator is built without a group's heads; the package function's tests fold into existing ones (issue 1921) Claude-Session: https://claude.ai/code/session_01NcbgfFSnn24M3GcqttoU7i --- .../internal/check/examiner_test.go | 22 ++++- .../internal/check/manifests_judge.go | 19 ++-- .../internal/check/pins_test.go | 34 ++++++++ .../internal/index/previews_test.go | 86 ++++++++----------- sdk/grouppackages/grouppackages.go | 38 ++++---- sdk/grouppackages/grouppackages_test.go | 51 ----------- 6 files changed, 122 insertions(+), 128 deletions(-) delete mode 100644 sdk/grouppackages/grouppackages_test.go diff --git a/modules/mesh-delivery-checker/internal/check/examiner_test.go b/modules/mesh-delivery-checker/internal/check/examiner_test.go index 56d6b020..e0c03e66 100644 --- a/modules/mesh-delivery-checker/internal/check/examiner_test.go +++ b/modules/mesh-delivery-checker/internal/check/examiner_test.go @@ -1138,8 +1138,24 @@ func TestTheIndexersJudgeJudgesTheChangesManifests(t *testing.T) { }) { t.Errorf("%s: the case says %v, want the new indexer's manifest said accepted by the trunk's manifest validator", row.name, cs.notes) } - for _, j := range judged { - if pkgs, _ := buildstep.PackagesArg(j.Fetch["packages"]); !row.try && pkgs[delivery.ForgeModulePrefix+"mesh-networking"] != head2 { + // Issue 1921: the judge of the change is built at the group's heads (also a member that publishes no package yet), but + // the trunk's manifest validator that rules on a new indexer is built at the latest published versions alone, never + // at a group's head (ADR 0633 clause 6): a group with a member of its own then judges the indexer a second time. + trunkRuns := 0 + if row.indexer != "" && row.indexer != "heldplain" && (row.ownJudge || !row.try) { + trunkRuns = 1 + } + if want := 1 + trunkRuns; len(judged) != want { + t.Fatalf("%s: %d judge asks %+v, want %d", row.name, len(judged), judged, want) + } + for i, j := range judged { + pkgs, _ := buildstep.PackagesArg(j.Fetch["packages"]) + atGroup := pkgs[delivery.ForgeModulePrefix+"mesh-networking"] == head2 + switch trunkRun := trunkRuns == 1 && i == len(judged)-1; { + case row.try: + case trunkRun && atGroup: + t.Errorf("%s: the trunk's manifest validator is built with the group's head: %v", row.name, pkgs) + case !trunkRun && !atGroup: t.Errorf("%s: the judge at %v is fetched with the packages %v, want the group's mesh-networking member at its head %s", row.name, j.Fetch["commit"], pkgs, head2) } } @@ -1148,6 +1164,8 @@ func TestTheIndexersJudgeJudgesTheChangesManifests(t *testing.T) { } if row.ownJudge { judged = judged[1:] + } else if trunkRuns == 1 { + judged = judged[1:] // the group's run; the trunk's is the one read below } if len(judged) != 1 || judged[0].Fetch["repository"] != DeliveryRepository || judged[0].Fetch["commit"] != trunk || judged[0].Package != JudgePackage || judged[0].Dir != JudgeDir { diff --git a/modules/mesh-delivery-checker/internal/check/manifests_judge.go b/modules/mesh-delivery-checker/internal/check/manifests_judge.go index 31e8944c..3b192271 100644 --- a/modules/mesh-delivery-checker/internal/check/manifests_judge.go +++ b/modules/mesh-delivery-checker/internal/check/manifests_judge.go @@ -120,7 +120,7 @@ func (st *state) judgeManifests(ctx context.Context) *delivery.Verdict { req.Predefined = list st.c.note("the manifests are judged against the predefined keys of " + from) } - cv, trunkCase, v := st.runJudge(ctx, req, touched, commit, at) + cv, trunkCase, v := st.runJudge(ctx, req, touched, commit, at, st.packagesOf(DeliveryRepository)) if v != nil { return v } @@ -177,12 +177,12 @@ func (st *state) judgeManifests(ctx context.Context) *delivery.Verdict { // runJudge asks the Go build seat to build the judge at the commit and run it over the request, and reads its change and // trunk cases; a failed verdict when it cannot be asked or does not answer. -func (st *state) runJudge(ctx context.Context, req composer.Request, touched []string, commit, at string) (change, trunk composer.CaseVerdict, v *delivery.Verdict) { +func (st *state) runJudge(ctx context.Context, req composer.Request, touched []string, commit, at string, versions delivery.PackageVersions) (change, trunk composer.CaseVerdict, v *delivery.Verdict) { raw, err := json.Marshal(req) if err != nil { return change, trunk, fail(goModule, err.Error()) } - job := delivery.ComposeJob{Fetch: withPackages(map[string]any{"repository": DeliveryRepository, "commit": commit}, st.packagesOf(DeliveryRepository)), Dir: JudgeDir, + job := delivery.ComposeJob{Fetch: withPackages(map[string]any{"repository": DeliveryRepository, "commit": commit}, versions), Dir: JudgeDir, Package: JudgePackage, Request: raw, Reader: "the indexer's judge at " + at} a := delivery.StepAsk{ID: buildstep.NewID(), Kind: delivery.AskCheck, Language: delivery.AskGo, Asker: delivery.CheckerModule, At: st.j.now().UTC(), Compose: &job} @@ -232,7 +232,10 @@ func (st *state) refuseUnmovableIndexer(ctx context.Context, req composer.Reques if indexer == "" { return nil } - if st.judgeHead != "" { + // The validator the running parts hold is the trunk's, built at the latest published versions alone, never at the group's + // heads (ADR 0633 clause 6): a group's mesh-sdk or mesh-contracts head must not change the validator that judges the new + // indexer. The head's run is reused when it was that validator already. + if trunkVersions := st.trunkJudgeVersions(); st.judgeHead != "" || !maps.Equal(st.packagesOf(DeliveryRepository), trunkVersions) { trunk := "" if st.j.Trunks != nil { trunk = st.j.Trunks.Heads()[DeliveryRepository] @@ -242,7 +245,7 @@ func (st *state) refuseUnmovableIndexer(ctx context.Context, req composer.Reques } var v *delivery.Verdict judgedAt = "mesh-delivery's trunk " + trunk - if cv, _, v = st.runJudge(ctx, req, touched, trunk, judgedAt); v != nil { + if cv, _, v = st.runJudge(ctx, req, touched, trunk, judgedAt, trunkVersions); v != nil { return v } } @@ -340,3 +343,9 @@ func (st *state) judgedByMember(p Pull, merged []Fetched) { } } } + +// trunkJudgeVersions are the versions the trunk's manifest validator is built at: the latest published version of each +// package, with no group member's head (ADR 0633 clause 6). +func (st *state) trunkJudgeVersions() delivery.PackageVersions { + return packagesFor(st.j.Published, &Case{}, DeliveryRepository) +} diff --git a/modules/mesh-delivery-checker/internal/check/pins_test.go b/modules/mesh-delivery-checker/internal/check/pins_test.go index d0ad069f..a61b018f 100644 --- a/modules/mesh-delivery-checker/internal/check/pins_test.go +++ b/modules/mesh-delivery-checker/internal/check/pins_test.go @@ -6,6 +6,7 @@ import ( "strings" "testing" + "git.novox.be/novox/mesh-delivery/sdk/grouppackages" "git.novox.be/novox/mesh-sdk/go/buildstep" "git.novox.be/novox/mesh-sdk/go/delivery" ) @@ -69,6 +70,39 @@ func TestAGroupMemberIsBuiltAgainstTheSDKMembersHeadWhateverItsGoModNames(t *tes if got := packagesOfFetch(t, b, repoA); len(got) != 1 || got["git.novox.be/novox/mesh-host"] != head2 { t.Errorf("the member's packages = %v, want the unpublished member's root at its head %s alone", got, head2) } + // The function every producer takes the versions from (issue 1921): rows for what a build at a group's heads is asked. + h := func(c string) string { return strings.Repeat(c, 40) } + const ( + contracts = "git.novox.be/novox/mesh-contracts/go/clients/assignerclient" + tools = "git.novox.be/novox/mesh-tools/go/x" + ) + for _, tc := range []struct { + name string + latest delivery.PackageVersions + heads map[string]string + own string + want delivery.PackageVersions + }{ + {"none", nil, nil, "novox/mesh-host", nil}, + {"trunk latest alone", delivery.PackageVersions{tools: h("a")}, nil, "novox/mesh-host", delivery.PackageVersions{tools: h("a")}}, + {"a member at its head replaces the latest", delivery.PackageVersions{SDKModulePath: h("a")}, map[string]string{SDKRepository: h("b")}, "novox/mesh-host", + delivery.PackageVersions{SDKModulePath: h("b")}}, + {"a member that publishes no package yet is named by its root module", delivery.PackageVersions{SDKModulePath: h("a")}, + map[string]string{"novox/mesh-contracts": h("c")}, "novox/mesh-host", + delivery.PackageVersions{SDKModulePath: h("a"), "git.novox.be/novox/mesh-contracts": h("c")}}, + {"mesh-sdk is named by its go module before any package of it is known", nil, map[string]string{SDKRepository: h("b")}, "novox/mesh-host", + delivery.PackageVersions{SDKModulePath: h("b")}}, + {"a member whose packages are known gets no root", delivery.PackageVersions{contracts: h("a")}, + map[string]string{"novox/mesh-contracts": h("c")}, "novox/mesh-host", delivery.PackageVersions{contracts: h("c")}}, + {"the build's own repository is left out", delivery.PackageVersions{contracts: h("a")}, + map[string]string{"novox/mesh-contracts": h("c")}, "novox/mesh-contracts", nil}, + {"a head that is no full commit names nothing", nil, map[string]string{"novox/mesh-contracts": "abc"}, "novox/mesh-host", nil}, + } { + got := grouppackages.Heads(tc.latest, tc.heads, tc.own) + if !maps.Equal(got, tc.want) { + t.Errorf("%s: got %v, want %v", tc.name, got, tc.want) + } + } } type runsAt map[string]string diff --git a/modules/mesh-delivery-indexer/internal/index/previews_test.go b/modules/mesh-delivery-indexer/internal/index/previews_test.go index 1b8c850d..1fed3046 100644 --- a/modules/mesh-delivery-indexer/internal/index/previews_test.go +++ b/modules/mesh-delivery-indexer/internal/index/previews_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "maps" "os" "runtime" "slices" @@ -108,6 +109,42 @@ func TestACandidatesModuleGetsTheFingerprintATrunkModuleOfTheSameTreeHas(t *test t.Errorf("candidate base = %d, want the source revision 5 its preview was read onto", c.Base) } written(t, writtenCandidate{Source: sourceRev{Rev: 5, Heads: map[string]string{catalogueRepo: trunkNow}}, Candidate: c}) + // Issue 1921: a candidate's build is asked every member of the group at its head, also one that publishes no package yet + // and one the module names no package of; the fingerprint stays the trunk's, so the build is the one the merge keeps. + for _, tc := range []struct { + name string + members map[string]string // the other pull requests of the group: repository, head + want delivery.PackageVersions + }{ + {"a group of one names nothing", nil, nil}, + {"a member that publishes no package yet", map[string]string{"novox/mesh-contracts": h2}, + delivery.PackageVersions{delivery.ForgeModulePrefix + "mesh-contracts": h2}}, + {"mesh-sdk by its go module", map[string]string{"novox/mesh-sdk": h2, "novox/mesh-contracts": h3}, + delivery.PackageVersions{delivery.ForgeModulePrefix + "mesh-sdk/go": h2, delivery.ForgeModulePrefix + "mesh-contracts": h3}}, + } { + ix, mods, cands := previewBed(t, true) + trunk, _ := moduleOf(t, mods, "avahi") + if err := ix.Apply(context.Background(), avahiPull(t, h1)); err != nil { + t.Fatal(err) + } + for repository, head := range tc.members { + other := forgemsg.Forge{V: delivery.Version, Kind: forgemsg.ForgePull, Repository: repository, Branch: "feat/x", Number: 7, Head: head, + Action: forgemsg.PullUpdated} + if err := ix.Apply(context.Background(), other); err != nil { + t.Fatal(err) + } + } + applyPreview(t, ix, avahiPreview(t, trunkNow, h1)) + var got delivery.CandidateModule + for _, p := range candidateOf(t, cands).Pulls { + if p.Repository == catalogueRepo { + got = p.Fps["avahi"] + } + } + if got.FP2 != trunk.FP2 || !maps.Equal(got.Packages, tc.want) { + t.Errorf("%s: candidate avahi = %+v, want the trunk's fingerprint %s and the packages %v", tc.name, got, trunk.FP2, tc.want) + } + } // Another tree gives another fingerprint. pv := avahiPreview(t, trunkNow, h1) m := pv.Modules["modules/avahi"] @@ -138,55 +175,6 @@ func TestACandidatesModuleGetsTheFingerprintATrunkModuleOfTheSameTreeHas(t *test } } -// Issue 1921: a candidate's build is asked every member of the group at its head, also one that publishes no package yet -// and one the module names no package of; the fingerprint stays the trunk's, so the build is the one the merge keeps. -func TestACandidatesBuildNamesEveryMemberOfItsGroupAtItsHead(t *testing.T) { - for _, tc := range []struct { - name string - members map[string]string // the other pull requests of the group: repository, head - want delivery.PackageVersions - }{ - {"a group of one names nothing", nil, nil}, - {"a member that publishes no package yet", map[string]string{"novox/mesh-contracts": h2}, - delivery.PackageVersions{delivery.ForgeModulePrefix + "mesh-contracts": h2}}, - {"mesh-sdk by its go module", map[string]string{"novox/mesh-sdk": h2, "novox/mesh-contracts": h3}, - delivery.PackageVersions{delivery.ForgeModulePrefix + "mesh-sdk/go": h2, delivery.ForgeModulePrefix + "mesh-contracts": h3}}, - } { - t.Run(tc.name, func(t *testing.T) { - ix, mods, cands := previewBed(t, true) - trunk, _ := moduleOf(t, mods, "avahi") - if err := ix.Apply(context.Background(), avahiPull(t, h1)); err != nil { - t.Fatal(err) - } - for repository, head := range tc.members { - other := forgemsg.Forge{V: delivery.Version, Kind: forgemsg.ForgePull, Repository: repository, Branch: "feat/x", Number: 7, Head: head, - Action: forgemsg.PullUpdated} - if err := ix.Apply(context.Background(), other); err != nil { - t.Fatal(err) - } - } - applyPreview(t, ix, avahiPreview(t, trunkNow, h1)) - var got delivery.CandidateModule - for _, p := range candidateOf(t, cands).Pulls { - if p.Repository == catalogueRepo { - got = p.Fps["avahi"] - } - } - if got.FP2 != trunk.FP2 { - t.Fatalf("candidate avahi = %+v, want the trunk's fingerprint %s", got, trunk.FP2) - } - if len(got.Packages) != len(tc.want) { - t.Fatalf("packages = %v, want %v", got.Packages, tc.want) - } - for pkg, head := range tc.want { - if got.Packages[pkg] != head { - t.Errorf("%s = %q, want %q", pkg, got.Packages[pkg], head) - } - } - }) - } -} - func TestAnUnknownToolchainLeavesTheCandidatesModuleNamingIt(t *testing.T) { // 1445/AC2 ix, _, cands := previewBed(t, false) if err := ix.Apply(context.Background(), avahiPull(t, h1)); err != nil { diff --git a/sdk/grouppackages/grouppackages.go b/sdk/grouppackages/grouppackages.go index 82f706fa..b5b35922 100644 --- a/sdk/grouppackages/grouppackages.go +++ b/sdk/grouppackages/grouppackages.go @@ -6,19 +6,29 @@ package grouppackages import ( + "strings" + "git.novox.be/novox/mesh-sdk/go/delivery" + "git.novox.be/novox/mesh-sdk/go/sdkversion" ) -// RootModules are the repositories whose Go module is not at their root, by repository: the module a member of that -// repository stands for when no package of it is known yet. -var RootModules = map[string]string{ - "novox/mesh-sdk": delivery.ForgeModulePrefix + "mesh-sdk/go", +// rootOf is the module a member of repo stands for when no package of it is known: its root module path, or for the +// repository of mesh-sdk its Go module (sdkversion.Module, which is not at the root). +func rootOf(repo string) (string, bool) { + if sdkRepo, _, ok := delivery.PackageRepository(sdkversion.Module); ok && delivery.SameRepository(repo, sdkRepo) { + return sdkversion.Module, true + } + name, found := strings.CutPrefix(repo, "novox/") + if !found || name == "" { + return "", false + } + return delivery.ForgeModulePrefix + name, true } // Heads is the version of each mesh package a build of own builds against: latest (the latest published version of each // package the mesh's modules use, nil for none) with every package of a group member at that member's head (heads, by // repository /, a full commit each), and for a member of which latest holds no package, the member's root -// module path (RootModules where the module is not at the root) at its head: the build module takes the head of a +// module path (mesh-sdk's Go module for mesh-sdk) at its head: the build module takes the head of a // repository from any package of it. A package of own comes with the tree the build fetched, and is left out, as is a // package no version is known of. Nil for none. latest is not changed. func Heads(latest delivery.PackageVersions, heads map[string]string, own string) delivery.PackageVersions { @@ -42,26 +52,12 @@ func Heads(latest delivery.PackageVersions, heads map[string]string, own string) if delivery.SameRepository(repo, own) || len(head) != 40 || held[repo] { continue } - root, ok := RootModules[repo] - if !ok { - name, found := cutOwner(repo) - if !found { - continue - } - root = delivery.ForgeModulePrefix + name + if root, ok := rootOf(repo); ok { + versions[root] = head } - versions[root] = head } if len(versions) == 0 { return nil } return versions } - -func cutOwner(repo string) (string, bool) { - const owner = "novox/" - if len(repo) > len(owner) && repo[:len(owner)] == owner { - return repo[len(owner):], true - } - return "", false -} diff --git a/sdk/grouppackages/grouppackages_test.go b/sdk/grouppackages/grouppackages_test.go deleted file mode 100644 index 0727e9ab..00000000 --- a/sdk/grouppackages/grouppackages_test.go +++ /dev/null @@ -1,51 +0,0 @@ -package grouppackages - -import ( - "strings" - "testing" - - "git.novox.be/novox/mesh-sdk/go/delivery" -) - -func TestHeads(t *testing.T) { - h := func(c string) string { return strings.Repeat(c, 40) } - const ( - contracts = "git.novox.be/novox/mesh-contracts/go/clients/assignerclient" - sdk = "git.novox.be/novox/mesh-sdk/go" - tools = "git.novox.be/novox/mesh-tools/go/x" - ) - for _, tc := range []struct { - name string - latest delivery.PackageVersions - heads map[string]string - own string - want delivery.PackageVersions - }{ - {"none", nil, nil, "novox/mesh-host", nil}, - {"trunk latest alone", delivery.PackageVersions{tools: h("a")}, nil, "novox/mesh-host", delivery.PackageVersions{tools: h("a")}}, - {"a member at its head replaces the latest", delivery.PackageVersions{sdk: h("a")}, map[string]string{"novox/mesh-sdk": h("b")}, "novox/mesh-host", - delivery.PackageVersions{sdk: h("b")}}, - {"a member that publishes no package yet is named by its root module", delivery.PackageVersions{sdk: h("a")}, - map[string]string{"novox/mesh-contracts": h("c")}, "novox/mesh-host", - delivery.PackageVersions{sdk: h("a"), "git.novox.be/novox/mesh-contracts": h("c")}}, - {"mesh-sdk named by its go module before any package of it is known", nil, map[string]string{"novox/mesh-sdk": h("b")}, "novox/mesh-host", - delivery.PackageVersions{sdk: h("b")}}, - {"a member whose packages are known gets no root", delivery.PackageVersions{contracts: h("a")}, - map[string]string{"novox/mesh-contracts": h("c")}, "novox/mesh-host", delivery.PackageVersions{contracts: h("c")}}, - {"the build's own repository is left out", delivery.PackageVersions{contracts: h("a")}, - map[string]string{"novox/mesh-contracts": h("c")}, "novox/mesh-contracts", nil}, - {"a head that is no full commit names nothing", nil, map[string]string{"novox/mesh-contracts": "abc"}, "novox/mesh-host", nil}, - } { - t.Run(tc.name, func(t *testing.T) { - got := Heads(tc.latest, tc.heads, tc.own) - if len(got) != len(tc.want) { - t.Fatalf("got %v, want %v", got, tc.want) - } - for k, v := range tc.want { - if got[k] != v { - t.Errorf("%s = %q, want %q (all: %v)", k, got[k], v, got) - } - } - }) - } -}