From cbc53b0e504020a0270677741ed0c82e001fe490 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 29 Oct 2026 00:07:40 +0100 Subject: [PATCH] Name every group member that does not merge in one verdict, not only the first After a member's check fails, the checker asks each other member its merge alone (mesh-sdk CheckJob.MergeOnly), so a hidden conflict no longer costs a whole check round and an Approve (issue 1921's kind; no open issue covers it; not issue 1276). --- modules/mesh-delivery-checker/go.mod | 2 +- modules/mesh-delivery-checker/go.sum | 4 +- .../internal/check/examiner_test.go | 34 ++++++ .../internal/check/judge.go | 108 +++++++++++++++++- 4 files changed, 139 insertions(+), 9 deletions(-) diff --git a/modules/mesh-delivery-checker/go.mod b/modules/mesh-delivery-checker/go.mod index 0eb617e..d51a07a 100644 --- a/modules/mesh-delivery-checker/go.mod +++ b/modules/mesh-delivery-checker/go.mod @@ -3,7 +3,7 @@ module mesh-delivery-checker go 1.26.0 require ( - git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261028193502-f91340fe82ef + git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261028230718-261f41aec1ae github.com/nats-io/nats-server/v2 v2.11.17 github.com/nats-io/nats.go v1.54.0 ) diff --git a/modules/mesh-delivery-checker/go.sum b/modules/mesh-delivery-checker/go.sum index cbc4905..dfe83ed 100644 --- a/modules/mesh-delivery-checker/go.sum +++ b/modules/mesh-delivery-checker/go.sum @@ -1,5 +1,5 @@ -git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261028193502-f91340fe82ef h1:ggwHXCvAOhBCKAY+ErehKEhu/IQTS4poJBRDokGYnDc= -git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261028193502-f91340fe82ef/go.mod h1:ouERNpZizzwZ0HLEtAypsY9FR20FyUgUYRni6J7kVCY= +git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261028230718-261f41aec1ae h1:bwcj9NnfGk/DP0sbTRInPAA4aMUnvr8+CmQLhnDvV9w= +git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261028230718-261f41aec1ae/go.mod h1:ouERNpZizzwZ0HLEtAypsY9FR20FyUgUYRni6J7kVCY= github.com/antithesishq/antithesis-sdk-go v0.7.0-default-no-op h1:Z/MZK75wC/NSrkgqeNIa7jexam9uWzhLmFTSCPI/kn0= github.com/antithesishq/antithesis-sdk-go v0.7.0-default-no-op/go.mod h1:FQyySiasQQM8735Ddel3MRojmy4dA1IqCeyJ5jmPMbI= github.com/google/go-tpm v0.9.8 h1:slArAR9Ft+1ybZu0lBwpSmpwhRXaa85hWtMinMyRAWo= diff --git a/modules/mesh-delivery-checker/internal/check/examiner_test.go b/modules/mesh-delivery-checker/internal/check/examiner_test.go index 5d708df..9e8e29b 100644 --- a/modules/mesh-delivery-checker/internal/check/examiner_test.go +++ b/modules/mesh-delivery-checker/internal/check/examiner_test.go @@ -484,6 +484,40 @@ func TestAHeadThatDoesNotMergeEndsFailedNamingTheFilesAndRunsNoTest(t *testing.T if n := len(b.steps.tool("digest")) + len(b.steps.tool("test")); n != 0 { t.Errorf("%d digest and test steps ran after a conflict", n) } + // A group: after the first member's failure every other member is asked its merge alone, so one verdict names each + // member that does not merge, with its files, and runs no test of the others. + for name, c := range map[string]struct { + conflicts map[string]bool // by repository + want []string // the members the verdict names in conflict + listed int // the verdict's Failures: one each when more than one failed, none for one alone + }{ + "both conflict": {map[string]bool{repoSDK: true, repoA: true}, []string{repoSDK + "#12 ", repoA + "#7 "}, 2}, + "the second merges": {map[string]bool{repoSDK: true}, []string{repoSDK + "#12 "}, 0}, + } { + b := newBench(t) + conflict := captured[Fetched](t, "go-fetch-conflict") + b.steps.handlers["build_go_fetch"] = func(sc stepCall) (any, error) { + if c.conflicts[sc.args["repository"].(string)] { + return conflict, nil + } + return b.fetch, nil + } + v := b.judge(t, caseOf(t, pullOf(repoA, 7, head1), pullOf(repoSDK, 12, head2))).Verdict + if v.State != delivery.VerdictFail || !IsConflictFailure(v) || len(v.Failures) != c.listed { + t.Fatalf("%s: verdict = %+v", name, v) + } + for n, w := range c.want { + if !strings.Contains(v.Failed.Words, w+ConflictWords+"a.go, sub/b.go") || len(v.Failures) > 0 && !strings.HasPrefix(v.Failures[n].Words, w) { + t.Errorf("%s: the verdict does not name %s in conflict: %+v", name, w, v) + } + } + if n := len(b.steps.tool("fetch")); n != 2 { + t.Errorf("%s: %d fetches, want each member's merge tried", name, n) + } + if n := len(b.steps.tool("digest")) + len(b.steps.tool("test")); n != 0 { + t.Errorf("%s: %d digest and test steps ran after a conflict", name, n) + } + } } func TestALevelWhoseDigestIsNotNamedRunsWholeAndKeepsNothing(t *testing.T) { // 1287/AC4 diff --git a/modules/mesh-delivery-checker/internal/check/judge.go b/modules/mesh-delivery-checker/internal/check/judge.go index c0fa364..b9a36a1 100644 --- a/modules/mesh-delivery-checker/internal/check/judge.go +++ b/modules/mesh-delivery-checker/internal/check/judge.go @@ -333,9 +333,104 @@ func (st *state) levelsOf() []string { } // judgeAll puts one check ask per pull request and build module the repository needs, the mesh-sdk member first, and -// judges each answer. A head that does not merge, a pin that is not allowed or a level that fails ends the check at once -// (ADR 0484 clause 13). +// judges each answer. A head that does not merge, a pin that is not allowed or a level that fails ends the check (ADR 0484 +// clause 13); the members not yet judged are then asked their merge alone, so the verdict names every member that does not +// merge into its trunk, each with its files, and not only the first (a hidden conflict costs a whole check round). func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { + pulls, v := st.judgeUntilFailed(ctx) + if v == nil { + return nil + } + var more []delivery.FailedFirst + for _, p := range pulls { + if f, ok := st.mergeOnly(ctx, p); ok { + more = append(more, f) + } + } + return withMergeFailures(v, more) +} + +// withMergeFailures adds to the verdict v the members asked their merge alone after it failed: what failed first stays +// first; a conflict's words name every member in conflict on one line, so the status line names them all, and the +// verdict's Failures list each. +func withMergeFailures(v *delivery.Verdict, more []delivery.FailedFirst) *delivery.Verdict { + if len(more) == 0 { + return v + } + if IsConflictFailure(*v) { + words := []string{v.Failed.Words} + for _, f := range more { + if strings.Contains(f.Words, ConflictWords) { + words = append(words, f.Words) + } + } + first := v.Failed + v.Failed.Words = strings.Join(words, "; ") + v.Failures = []delivery.FailedFirst{first} + } else if len(v.Failures) == 0 { + v.Failures = []delivery.FailedFirst{v.Failed} + } + all := append(slices.Clone(v.Failures), more...) + if n := len(all); n > delivery.MaxFailures+1 { + all = append(all[:delivery.MaxFailures], delivery.FailedFirst{Test: fmt.Sprintf("and %d more", n-delivery.MaxFailures)}) + } + v.Failures = all + return v +} + +// mergeOnly asks the merge of a member's head onto its trunk alone, of its repository's first build module, and answers the +// failure it names when the head does not merge or the ask could not be answered. A build module from before +// CheckJob.MergeOnly runs the levels too; only the fetch of its answer is read. +func (st *state) mergeOnly(ctx context.Context, p Pull) (delivery.FailedFirst, bool) { + bms := st.j.Modules.BuildModules(p.Repository) + if len(bms) == 0 || ctx.Err() != nil { + return delivery.FailedFirst{}, false + } + bm := bms[0] + args := map[string]any{"repository": p.Repository, "commit": p.Head, "ref": refOf(p.Number), "base": p.Trunk} + a := delivery.StepAsk{ID: buildstep.NewID(), Kind: delivery.AskCheck, Language: askLanguage[bm], Asker: delivery.CheckerModule, + At: st.j.now().UTC(), Check: &delivery.CheckJob{Fetch: args, Levels: st.levelsOf(), MergeOnly: true}} + st.c.Progress.Set(fmt.Sprintf("the merge alone of %s#%d (ask %s), after the check failed", p.Repository, p.Number, a.ID), "") + st.c.Asked(a.ID) + failed := func(machine, words string) (delivery.FailedFirst, bool) { + return delivery.FailedFirst{Machine: machine, Words: words}, true + } + if err := st.j.Asks.Put(ctx, a); err != nil { + return failed(bm, fmt.Sprintf("the merge of %s#%d onto its trunk could not be asked: %v", p.Repository, p.Number, err)) + } + waiting := ctx + if watch := st.c.Watch; watch != nil { + id := a.ID + watch(id, nil) + waiting = buildrun.WithStepSeen(ctx, func(s delivery.Step) { watch(id, &s) }) + } + step, err := st.j.Asks.Wait(waiting, a) + if err != nil { + if ctx.Err() != nil { + return delivery.FailedFirst{}, false // the check was ended: what failed first is its verdict + } + return failed(bm, fmt.Sprintf("the merge of %s#%d onto its trunk: %v", p.Repository, p.Number, err)) + } + st.nodes[step.Node] = true + if step.State != delivery.StepPassed { + return failed(step.Node, fmt.Sprintf("the merge of %s#%d onto its trunk %s: %s", p.Repository, p.Number, step.State, step.Reason)) + } + var answer delivery.CheckAnswer + var f Fetched + if err := json.Unmarshal(step.Result, &answer); err != nil || json.Unmarshal(answer.Fetch, &f) != nil { + return failed(step.Node, fmt.Sprintf("the merge of %s#%d onto its trunk answered what is not a check: %s", p.Repository, p.Number, step.Result)) + } + if len(f.Conflicts) == 0 { + return delivery.FailedFirst{}, false + } + if st.c.Try == nil { + st.j.writePreview(ctx, p, []Fetched{f}) + } + return failed(step.Node, fmt.Sprintf("%s#%d %s%s", p.Repository, p.Number, ConflictWords, strings.Join(f.Conflicts, ", "))) +} + +// judgeUntilFailed is judgeAll up to the first failure, answering it and the members after the one that failed. +func (st *state) judgeUntilFailed(ctx context.Context) ([]Pull, *delivery.Verdict) { // The mesh-sdk member first: whether its merge changes the TypeScript package's paths decides whether the other members' // Node fetches take the group's package (ADR 0500 ยง5). pulls := slices.Clone(st.c.Pulls) @@ -349,7 +444,8 @@ func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { return 0 }) var groupPackage map[string]any - for _, p := range pulls { + for n, p := range pulls { + rest := pulls[n+1:] var merged []Fetched for _, bm := range st.j.Modules.BuildModules(p.Repository) { t := &target{pull: p, module: bm} @@ -363,7 +459,7 @@ func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { if bm == buildrun.GoBuildModule { beside, err := st.j.besideFor(ctx, st.c, p) if err != nil { - return fail(forgeMachine(err, bm), err.Error()+unheardWords(err)) + return rest, fail(forgeMachine(err, bm), err.Error()+unheardWords(err)) } if len(beside) > 0 { args["beside"] = beside @@ -377,7 +473,7 @@ func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { t.fetchArgs = args st.targets = append(st.targets, t) if v := st.ask(ctx, t, &merged); v != nil { - return v + return rest, v } } if st.c.Try == nil { // a try owes the indexer no preview: no pull request holds its head @@ -387,7 +483,7 @@ func (st *state) judgeAll(ctx context.Context) *delivery.Verdict { groupPackage = map[string]any{"repository": p.Repository, "commit": p.Head, "ref": refOf(p.Number)} } } - return nil + return nil, nil } // DefaultDeclineBound is an Examiner's DeclineBound when it names none: longer than a check runs, so a node with the