From f7d8622deb9e418ac94a00d68fab114f5656f53b Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 17:56:59 +0200 Subject: [PATCH] Keep a core repository's passing package under a digest that covers its siblings' commits, so core checks keep verdicts (issue 1287, C3a/C3b, lead decision L5) The checker took and kept nothing for a workspace given siblings, and every core repository has siblings, so the kept store stayed empty. The digest step now folds the siblings' commits in and echoes them; a verdict is kept or taken only when the echo equals the siblings the fetch was given, so a digest that misses one never poisons another check (ADR 0444, issue 482). --- .../internal/check/checker_test.go | 1 + .../internal/check/examiner_test.go | 68 ++++++++++++++++++- .../internal/check/judge.go | 31 ++++++--- .../internal/check/results.go | 3 + 4 files changed, 92 insertions(+), 11 deletions(-) diff --git a/modules/mesh-delivery-checker/internal/check/checker_test.go b/modules/mesh-delivery-checker/internal/check/checker_test.go index f57ed71..295a9ff 100644 --- a/modules/mesh-delivery-checker/internal/check/checker_test.go +++ b/modules/mesh-delivery-checker/internal/check/checker_test.go @@ -19,6 +19,7 @@ const ( trunkA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" trunkA2 = "abababababababababababababababababababab" trunkS = "5555555555555555555555555555555555555555" + trunkS2 = "5656565656565656565656565656565656565656" ) // candidateOf is a candidate of a group with the given pull requests, as the indexer writes one. diff --git a/modules/mesh-delivery-checker/internal/check/examiner_test.go b/modules/mesh-delivery-checker/internal/check/examiner_test.go index 66edd34..735eea5 100644 --- a/modules/mesh-delivery-checker/internal/check/examiner_test.go +++ b/modules/mesh-delivery-checker/internal/check/examiner_test.go @@ -2,10 +2,13 @@ package check import ( "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "fmt" "os" "path/filepath" + "sort" "strings" "sync" "testing" @@ -565,7 +568,7 @@ func trunksOf(t *testing.T, heads map[string]string) *Trunks { return tr } -func TestNothingIsKeptForAWorkspaceGivenSiblingsBeside(t *testing.T) { // review D1, ADR 0444, issue 1287 +func TestNothingIsKeptForAWorkspaceGivenSiblingsWhenTheDigestDoesNotSayItCoversThem(t *testing.T) { // review D1, ADR 0444, issue 1287 b := newBench(t) b.ex.Trunks = trunksOf(t, map[string]string{repoSDK: trunkS, "novox/mesh-catalog": trunkA}) b.judge(t, caseOf(t, pullOf(repoA, 7, head1))) @@ -573,7 +576,7 @@ func TestNothingIsKeptForAWorkspaceGivenSiblingsBeside(t *testing.T) { // review t.Fatalf("the fetch was not given siblings: %+v", b.steps.tool("fetch")) } if keys, _ := b.kept.Keys(context.Background()); len(keys) != 0 { - t.Errorf("kept %v for a package whose tests may read a sibling the digest does not cover", keys) + t.Errorf("kept %v for a package whose digest does not say it covers the siblings", keys) } // And a kept verdict of the same digest from elsewhere is not taken for it: everything runs. if _, err := (Kept{Store: b.kept}).Keep(context.Background(), "unit", digA, keptValue("fixture/slow", 2)); err != nil { @@ -585,3 +588,64 @@ func TestNothingIsKeptForAWorkspaceGivenSiblingsBeside(t *testing.T) { // review t.Errorf("%d test steps, want both levels run whole", n) } } + +// besideDigest stands for the Go build module's digest over a workspace with siblings (DigestVersion 3, lead decision L5): +// each package's digest folds the siblings' commits in, and the answer echoes the lines it folded. +func besideDigest(b *bench) func(c stepCall) (any, error) { + return func(c stepCall) (any, error) { + level := c.args["level"].(string) + var lines []string + for _, f := range b.steps.tool("fetch") { + refs, _ := f.args["beside"].([]BesideRef) + for _, r := range refs { + lines = append(lines, r.Repository+" "+r.Commit) + } + } + sort.Strings(lines) + sum := sha256.Sum256([]byte(level + "\n" + strings.Join(lines, "\n"))) + return Digested{Level: level, Packages: map[string]string{"fixture/slow": hex.EncodeToString(sum[:])}, Beside: lines}, nil + } +} + +func TestACoreRepositorysPassingPackageIsKeptUnderADigestCoveringItsSiblings(t *testing.T) { // 1287 AC1-AC3, AC7 with L5 + b := newBench(t) + b.steps.handlers["build_go_digest"] = besideDigest(b) + b.ex.Trunks = trunksOf(t, map[string]string{repoSDK: trunkS, "novox/mesh-catalog": trunkA}) + b.judge(t, caseOf(t, pullOf(repoA, 7, head1))) + keys, _ := b.kept.Keys(context.Background()) + if len(keys) != 2 { + t.Fatalf("kept %v, want the passing package at both levels", keys) + } + // The same siblings again: everything is taken, nothing runs. + b.steps.calls = nil + v := b.judge(t, caseOf(t, pullOf(repoA, 7, head1))).Verdict + if n := len(b.steps.tool("test")); n != 0 || v.Levels[delivery.LevelUnit].Run != 0 || v.Levels[delivery.LevelUnit].Kept == 0 { + t.Fatalf("%d test steps, unit %+v, want none run and the kept verdict taken", n, v.Levels[delivery.LevelUnit]) + } + // A sibling's commit moves: a new digest, so the package runs again and is kept under it. + b.ex.Trunks = trunksOf(t, map[string]string{repoSDK: trunkS2, "novox/mesh-catalog": trunkA}) + b.steps.calls = nil + b.judge(t, caseOf(t, pullOf(repoA, 7, head1))) + if n := len(b.steps.tool("test")); n != 2 { + t.Errorf("%d test steps after a sibling moved, want both levels run", n) + } + if keys, _ := b.kept.Keys(context.Background()); len(keys) != 4 { + t.Errorf("kept %v, want a second key per level under the new digest", keys) + } +} + +func TestADigestThatEchoesOtherSiblingsKeepsNothing(t *testing.T) { // 1287 with L5: an echo that differs is not trusted + b := newBench(t) + inner := besideDigest(b) + b.steps.handlers["build_go_digest"] = func(c stepCall) (any, error) { + v, err := inner(c) + d := v.(Digested) + d.Beside = d.Beside[:len(d.Beside)-1] + return d, err + } + b.ex.Trunks = trunksOf(t, map[string]string{repoSDK: trunkS, "novox/mesh-catalog": trunkA}) + b.judge(t, caseOf(t, pullOf(repoA, 7, head1))) + if keys, _ := b.kept.Keys(context.Background()); len(keys) != 0 { + t.Errorf("kept %v under a digest that does not cover every sibling", keys) + } +} diff --git a/modules/mesh-delivery-checker/internal/check/judge.go b/modules/mesh-delivery-checker/internal/check/judge.go index df003a4..7395ee2 100644 --- a/modules/mesh-delivery-checker/internal/check/judge.go +++ b/modules/mesh-delivery-checker/internal/check/judge.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "path" + "slices" "sort" "strings" "time" @@ -87,6 +88,16 @@ func (j *Examiner) logf(format string, args ...any) { } } +// besideLines are the siblings as the digest step echoes them: ` `, sorted. +func besideLines(refs []BesideRef) []string { + out := make([]string, 0, len(refs)) + for _, r := range refs { + out = append(out, r.Repository+" "+r.Commit) + } + sort.Strings(out) + return out +} + // target is a pull request on one build module: the node chosen, the workspace its fetch made and what it said. type target struct { pull Pull @@ -98,8 +109,9 @@ type target struct { fetchArgs map[string]any // priority are the Go packages (import paths) or the Node module directories whose test files the merge changes. changed []string - // beside says the workspace was given siblings: its tests may read what the digest does not cover, so nothing is kept for it. - beside bool + // beside are the lines ` ` of the siblings the workspace was given, sorted: its tests may read them, + // so a verdict is kept or taken only under a digest that says it folded exactly these in (lead decision L5). + beside []string plans map[string]*goPlan // by level } @@ -272,7 +284,7 @@ func (st *state) fetchAll(ctx context.Context) *delivery.Verdict { } if len(beside) > 0 { args["beside"] = beside - t.beside = true + t.beside = besideLines(beside) } } raw, err := st.step(ctx, t, "fetch", fmt.Sprintf("fetch %s#%d on %s", p.Repository, p.Number, node), args) @@ -398,12 +410,6 @@ func (st *state) planGo(ctx context.Context, t *target, level string) *delivery. } plan := &goPlan{digests: map[string]string{}} t.plans[level] = plan - if t.beside { - // The digest covers the package's own files, the toolchain and the header, not the siblings placed beside the tree: a - // package whose tests read one must never be kept, nor taken from a verdict kept for other inputs (ADR 0444). - plan.all = true - return nil - } raw, err := st.step(ctx, t, "digest", fmt.Sprintf("digest the %s level of %s#%d on %s", level, t.pull.Repository, t.pull.Number, t.node), map[string]any{"workspace": t.fetched.Workspace, "level": level}) var d Digested @@ -421,6 +427,13 @@ func (st *state) planGo(ctx context.Context, t *target, level string) *delivery. plan.all = true return nil } + if !slices.Equal(d.Beside, t.beside) { + // The siblings the tests read are not all in the digest (a build module before DigestVersion 3, or another set): a + // package whose tests read one is never kept, nor taken from a verdict kept for other inputs (ADR 0444, L5). + st.j.logf("%s#%d: the %s level's digest covers the siblings %v, not %v, so it runs whole and keeps nothing", t.pull.Repository, t.pull.Number, level, d.Beside, t.beside) + plan.all = true + return nil + } plan.digests = d.Packages a := st.levels[level] pkgs := make([]string, 0, len(d.Packages)) diff --git a/modules/mesh-delivery-checker/internal/check/results.go b/modules/mesh-delivery-checker/internal/check/results.go index 5583592..355185d 100644 --- a/modules/mesh-delivery-checker/internal/check/results.go +++ b/modules/mesh-delivery-checker/internal/check/results.go @@ -89,6 +89,9 @@ type Tested struct { // Digested is the answer of the Go build module's digest step: one digest per package that has tests at the level, by import // path. type Digested struct { + // Beside are the lines ` ` of the siblings the digest folded in (DigestVersion 3, lead decision L5), + // sorted; absent when the workspace has none or the build module folds none in. + Beside []string `json:"beside,omitempty"` Level string `json:"level"` Packages map[string]string `json:"packages"` WithoutTests []string `json:"without_tests,omitempty"`