From 24d9c6cdf8d9493f2ae475a60d184f91436f29d7 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 12 Nov 2026 15:25:44 +0100 Subject: [PATCH] The checker previews a member whose fetch merged, whatever its later steps ended with A member whose check step errored after its fetch merged was left unpreviewed, so one failing level hid the manifest validator and the composition from the check, and the words "its head does not merge or its fetch did not answer" were wrong for it. The checker now reads the fetch a failed step answers beside its error and previews the merge from it; only a head that does not merge or a fetch that did not answer stays unpreviewed, and the check says which of the two. Builds: design 47 G A-286 (G-22-lead-answers-2.md at hq 41dd30fe; issue 1921; principle 3) Quick: this checker on main; thorough: the same rule in G's checker (join 11, J15). Claude-Session: https://claude.ai/code/session_01NcbgfGKaxw984XyAPfLm4h --- .../internal/check/examiner_test.go | 24 +++++++++++---- .../internal/check/judge.go | 30 +++++++++++++++---- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/modules/mesh-delivery-checker/internal/check/examiner_test.go b/modules/mesh-delivery-checker/internal/check/examiner_test.go index 1c5f9146..be9f44e6 100644 --- a/modules/mesh-delivery-checker/internal/check/examiner_test.go +++ b/modules/mesh-delivery-checker/internal/check/examiner_test.go @@ -245,11 +245,12 @@ func (b *bench) answer(a buildstep.StepAsk) buildstep.Step { } out, err := gobuild.RunAsk(context.Background(), steps, a) step := buildstep.Step{ID: a.ID, Tool: a.Kind + " ask", Node: node, State: buildstep.StepPassed, Started: began, Ended: began.Add(2 * time.Second)} + if out != nil { + step.Result, _ = json.Marshal(out) // a failed step keeps what it answered beside its error, as the step runner does + } if err != nil { step.State, step.Reason = buildstep.StepFailed, err.Error() - return step } - step.Result, _ = json.Marshal(out) return step } @@ -589,9 +590,11 @@ func TestANodePackageIsNeverKeptAndRunsEveryCheck(t *testing.T) { // 1288/AC4, 1 // change's own tests still run first (issue 1289), and the rest of the level after them. A member that only failed its // tests still gets its preview, so the composition is read and its failure named after the tests'. A member whose head // does not merge has no preview of a merge, so the assigner's composition would only wait: the check does not wait for it -// and says why it was not run (review of 650a1d0). +// and says why it was not run (review of 650a1d0). A member whose step failed after its fetch merged is previewed from the +// fetch the failed step answered, and the composition is read (design 47 G A-286). func TestEveryFailureOfEveryMemberLevelAndTheCompositionIsNamed(t *testing.T) { - for _, conflicts := range []bool{false, true} { + for _, kind := range []string{"tests", "conflicts", "errors"} { + conflicts, errs := kind == "conflicts", kind == "errors" b := newBench(t) comps := &fakeCompositions{v: refusedOn("x", "g14", "route-proxy is defined twice"), waits: conflicts} b.ex.Compositions = comps @@ -606,6 +609,9 @@ func TestEveryFailureOfEveryMemberLevelAndTheCompositionIsNamed(t *testing.T) { } failing := captured[Tested](t, "go-test-failing") b.test = func(c stepCall) (any, error) { + if errs && c.args["level"] == "unit" { + return nil, errors.New("the seat definitions in . do not generate") + } if got := packagesOf(c); c.args["level"] == "integration" || (len(got) == 1 && got[0] == "m/internal/b") { r := failing r.Level = c.args["level"].(string) @@ -622,6 +628,14 @@ func TestEveryFailureOfEveryMemberLevelAndTheCompositionIsNamed(t *testing.T) { case <-time.After(5 * time.Second): t.Fatalf("conflicts %v: the check waits on a composition that only waits", conflicts) } + if errs { + keys, _ := b.prev.Keys(context.Background()) + if len(keys) != 2 || len(comps.asked) == 0 || v.Composition == nil || v.Composition.State == meshdeliverycheckerclient.CompositionNotRun { + t.Errorf("a step failed after its fetch merged: previews %v, asked %v, composition %+v, want both previews and the composition read", + keys, comps.asked, v.Composition) + } + continue + } var got []string for _, f := range v.Failures { got = append(got, firstFailed(meshdeliverychecker.VerdictsByGroupHeadsValue{Failed: &FailedFirst{Test: f.Test, Machine: f.Machine, Words: f.Words}})) @@ -644,7 +658,7 @@ func TestEveryFailureOfEveryMemberLevelAndTheCompositionIsNamed(t *testing.T) { case !conflicts && (len(keys) != 2 || v.Composition == nil || v.Composition.State != meshdeliverycheckerclient.CompositionFail): t.Errorf("tests alone failed: previews %v, composition %+v, want both previews and the composition read", keys, v.Composition) case conflicts && (len(comps.asked) != 0 || v.Composition == nil || v.Composition.State != meshdeliverycheckerclient.CompositionNotRun || - !strings.Contains(v.Composition.Words, "novox/mesh-sdk#12")): + !strings.Contains(v.Composition.Words, "novox/mesh-sdk#12 (its head does not merge)")): t.Errorf("a member did not merge: asked %v, composition %+v, want not run naming novox/mesh-sdk#12", comps.asked, v.Composition) } var unit []string diff --git a/modules/mesh-delivery-checker/internal/check/judge.go b/modules/mesh-delivery-checker/internal/check/judge.go index bc6adc0a..5d8c25cc 100644 --- a/modules/mesh-delivery-checker/internal/check/judge.go +++ b/modules/mesh-delivery-checker/internal/check/judge.go @@ -284,11 +284,14 @@ type state struct { judgeHead string // predefinedHead is the catalogue member's head when its merge changes predefined.json: the list is read there. predefinedHead string - // unpreviewed are the members (pullKey) with no preview of a merge: a head that does not merge, or a fetch that did not - // answer. The indexer holds no manifest of them, so the assigner's composition would only wait: it is not awaited. + // unpreviewed are the members (pullKey) with no preview of a merge, each with which of the two it is: a head that does not + // merge, or a fetch that did not answer (design 47 G A-286). The indexer holds no manifest of them, so the assigner's + // composition would only wait: it is not awaited. A member whose fetch merged is previewed whatever its later steps ended with. unpreviewed []string // tags are the group's tagged versions its members are built against (ADR 0607 §3), once the check heard them. tags groupVersions + // conflicted are the members (pullKey) whose head does not merge into its trunk. + conflicted map[string]bool // outOfStep are, by pull request (pullKey), the generated files a level's compare step found out of step (design 26). outOfStep map[string][]string // noCandidate says why the case has no previewed candidate of its heads from the indexer within the bound: for a whole @@ -344,7 +347,7 @@ func (st *state) takeTryCandidate(ctx context.Context) { return } if len(st.unpreviewed) > 0 { - no("no preview of a merge of " + strings.Join(st.unpreviewed, ", ") + " (its head does not merge or its fetch did not answer)") + no("no preview of a merge of " + strings.Join(st.unpreviewed, ", ")) return } if st.c.Wait != nil { @@ -425,7 +428,7 @@ func (st *state) compose(ctx context.Context) { if len(st.unpreviewed) > 0 { // An engine would only say it cannot read a manifest the indexer will not have: the check does not ask it. st.composition = &rules.Composition{NotRun: "no preview of a merge of " + strings.Join(st.unpreviewed, ", ") + - " (its head does not merge or its fetch did not answer), so the change cannot be composed"} + ", so the change cannot be composed"} return } // ADR 0603 §10: the check composes with the candidate's builds; whether it failed already is the running rules' word @@ -593,13 +596,16 @@ func (st *state) judgeAll(ctx context.Context) { st.ask(ctx, t, &merged) } // Every build module's fetch merged (each is in merged once): the preview is written, also when the member's tests - // failed, so the indexer holds its manifests and the assigner can compose the change (review of 650a1d0). + // or a later step failed, so the indexer holds its manifests and the assigner can compose the change (review of + // 650a1d0; design 47 G A-286). switch { case st.narrowTry(): // a try of a level or some tests owes the indexer no preview: it judges no manifest case len(merged) == len(modules): st.j.writePreview(ctx, p, merged) + case st.conflicted[pullKey(p)]: + st.unpreviewed = append(st.unpreviewed, pullKey(p)+" (its head does not merge)") default: - st.unpreviewed = append(st.unpreviewed, pullKey(p)) + st.unpreviewed = append(st.unpreviewed, pullKey(p)+" (its fetch did not answer)") } st.judgedByMember(p, merged) // The old form (ADR 0500 §5): the group's package member packed by the Node fetch, kept only while no tagged @@ -730,6 +736,14 @@ func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, l st.facts = append(st.facts, rules.Step{Repository: p.Repository, Number: p.Number, Module: t.module, Node: step.Node}) st.c.Progress.Set(fmt.Sprintf("judge the check of %s#%d", p.Repository, p.Number), step.Node) if step.State != buildstep.StepPassed { + // A step that failed after its fetch merged answers the fetch beside its error (design 47 G A-286): the merge is + // previewed from it, so its manifests are judged and the change composed whatever the step ended with. + var failed buildstep.CheckAnswer + if !placed && json.Unmarshal(step.Result, &failed) == nil && failed.Fetch.Workspace != "" && len(failed.Fetch.Conflicts) == 0 { + t.fetched, t.changed = failed.Fetch, failed.Fetch.Changed + *merged = append(*merged, t.fetched) + st.checkBinariesOf(t) + } st.fail(step.Node, fmt.Sprintf("the check of %s#%d %s: %s", p.Repository, p.Number, step.State, step.Reason)) return } @@ -743,6 +757,10 @@ func (st *state) askNeeding(ctx context.Context, t *target, merged *[]Fetched, l return } if len(t.fetched.Conflicts) > 0 { + if st.conflicted == nil { + st.conflicted = map[string]bool{} + } + st.conflicted[pullKey(p)] = true if !st.narrowTry() { st.j.writePreview(ctx, p, append(*merged, t.fetched)) }