From 14c78e485f663343f08c57b743ae4859ded28e21 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 15:00:58 +0200 Subject: [PATCH 1/2] Read a Go module's go.mod at the trunk when the head does not hold it, so a branch cut before the module was added passes the pin check (issue 1845) --- .../cmd/mesh-delivery-checker/forge.go | 6 +++ .../cmd/mesh-delivery-checker/forge_test.go | 16 ++++++ .../internal/check/pins.go | 14 ++++- .../internal/check/pins_test.go | 53 ++++++++++++++++++- 4 files changed, 86 insertions(+), 3 deletions(-) diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge.go index 9cf6715..f0d0c07 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge.go @@ -5,6 +5,8 @@ import ( "encoding/json" "fmt" "strings" + + "mesh-delivery-checker/internal/check" ) // The forge's tools the checker reads with (mesh-catalog modules/gitea): gitea_get_file answers {file: {content}}, the text @@ -31,6 +33,10 @@ func (f forgeReads) File(_ context.Context, repository, path, ref string) (strin owner, repo := ownerRepo(repository) raw, err := f.call(getFileTool, map[string]string{"owner": owner, "repo": repo, "path": path, "ref": ref}) if err != nil { + if strings.Contains(err.Error(), "404") { + // The forge answers 404 for a file the ref does not hold (issue 1845). + return "", fmt.Errorf("%w: %v", check.ErrNotFound, err) + } return "", err } var a struct { diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge_test.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge_test.go index 2b1f2b1..b337053 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge_test.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge_test.go @@ -3,10 +3,13 @@ package main import ( "context" "encoding/json" + "errors" "os" "path/filepath" "strings" "testing" + + "mesh-delivery-checker/internal/check" ) // capturedAnswer is what the forge's tool answered (internal/check/testdata/captured/README). @@ -68,3 +71,16 @@ func TestTheForgeNamesTheWholeShaOfAShortCommit(t *testing.T) { // review D2 t.Error("a commit that does not start with the pin was taken") } } + +func TestAFileTheRefDoesNotHoldIsNotFoundAndOtherFailuresAreNot(t *testing.T) { // 1845/AC1, AC4 + f := forgeReads{call: func(string, any) (json.RawMessage, error) { + return nil, errors.New("Gitea API contents/checks/particulars/go.mod?ref=98ca97fc: 404 object does not exist") + }} + if _, err := f.File(context.Background(), "novox/mesh-catalog", "checks/particulars/go.mod", "98ca97fc"); !errors.Is(err, check.ErrNotFound) { + t.Errorf("a 404 is %v, want ErrNotFound", err) + } + f = forgeReads{call: func(string, any) (json.RawMessage, error) { return nil, errors.New("Gitea API: 500 internal error") }} + if _, err := f.File(context.Background(), "novox/mesh-catalog", "go.mod", "main"); err == nil || errors.Is(err, check.ErrNotFound) { + t.Errorf("a 500 is %v, want a failure that is not ErrNotFound", err) + } +} diff --git a/modules/mesh-delivery-checker/internal/check/pins.go b/modules/mesh-delivery-checker/internal/check/pins.go index cbedcaa..326d606 100644 --- a/modules/mesh-delivery-checker/internal/check/pins.go +++ b/modules/mesh-delivery-checker/internal/check/pins.go @@ -2,6 +2,7 @@ package check import ( "context" + "errors" "fmt" "path" "regexp" @@ -22,6 +23,10 @@ const ( // Issue1148 is why a pin that only a head carries fails the check. const Issue1148 = "issue 1148" +// ErrNotFound is what Forge.File wraps when the ref holds no such file (the forge answered 404): a file the ref does not hold, +// told from a forge that could not be read (issue 1845). +var ErrNotFound = errors.New("the ref holds no such file") + // Forge reads a file of a repository at a ref, and says whether a commit is on a repository's trunk. type Forge interface { // File is the text of a file at a ref (a branch, a tag or a commit). @@ -51,8 +56,9 @@ func SDKPin(goMod string) (commit string, ok bool) { } // checkPins refuses a Go member whose go.mod names a mesh-sdk commit that is neither on mesh-sdk's trunk nor the head of the -// group's mesh-sdk pull request. It reads each go.mod the fetch named at the member's head from the forge. It answers nil -// when every pin is the group's. +// group's mesh-sdk pull request. It reads each go.mod the fetch named from the forge: the one the check builds, which is the +// head's when the head holds the file and the trunk's when it does not, since a module added on the trunk after the branch was +// cut is in the merge and not at the head (issue 1845). It answers nil when every pin is the group's. func (st *state) checkPins(ctx context.Context) verdictPtr { if st.j.Forge == nil { return nil @@ -66,6 +72,10 @@ func (st *state) checkPins(ctx context.Context) verdictPtr { file := path.Join(dir, "go.mod") st.c.Progress.Set(fmt.Sprintf("read %s of %s#%d", file, t.pull.Repository, t.pull.Number), t.node) text, err := st.j.Forge.File(ctx, t.pull.Repository, file, t.pull.Head) + if errors.Is(err, ErrNotFound) && t.pull.Trunk != "" { + // Only a file the head does not hold is read at the trunk; any other failure fails the check as it did. + text, err = st.j.Forge.File(ctx, t.pull.Repository, file, t.pull.Trunk) + } if err != nil { return failed(forgeMachine(err, t.node), fmt.Sprintf("%s#%d: %s cannot be read to check its mesh-sdk pin: %v", t.pull.Repository, t.pull.Number, file, err)) } diff --git a/modules/mesh-delivery-checker/internal/check/pins_test.go b/modules/mesh-delivery-checker/internal/check/pins_test.go index 065dc56..9198b17 100644 --- a/modules/mesh-delivery-checker/internal/check/pins_test.go +++ b/modules/mesh-delivery-checker/internal/check/pins_test.go @@ -17,16 +17,20 @@ type fakeForge struct { goMods map[string]string // ` ` -> text onTrunk map[string]bool // by commit prefix err error + errAt map[string]error // by ref: a read at that ref fails with it } func (f *fakeForge) File(_ context.Context, repository, path, ref string) (string, error) { if f.err != nil { return "", f.err } + if e := f.errAt[ref]; e != nil { + return "", e + } if s, ok := f.goMods[repository+" "+path+" "+ref]; ok { return s, nil } - return "", fmt.Errorf("no file %s of %s at %s", path, repository, ref) + return "", fmt.Errorf("no file %s of %s at %s: %w", path, repository, ref, ErrNotFound) } func (f *fakeForge) FullSHA(_ context.Context, _ string, commit string) (string, error) { @@ -153,6 +157,53 @@ func TestAPinThatCannotBeCheckedFailsAndNeverPasses(t *testing.T) { // ADR 0453 } } +// trunkOnlyBench is a group of mesh-delivery#7 whose fetch holds two Go modules: modules/m, which the head holds, and +// modules/new, which only the trunk holds (added on the trunk after the branch was cut). The SDK pin of each go.mod is given. +func trunkOnlyBench(t *testing.T, headPin, trunkPin string) *bench { + b := newBench(t) + b.fetch.Modules = []string{"modules/m", "modules/new"} + forge := &fakeForge{goMods: map[string]string{ + repoA + " modules/m/go.mod " + head1: capturedGoMod(t, headPin), + repoA + " modules/m/go.mod " + trunkA: capturedGoMod(t, headPin), + repoA + " modules/new/go.mod " + trunkA: capturedGoMod(t, trunkPin), + }, onTrunk: map[string]bool{"cb54756c3865": true}} + b.ex.Forge = forge + return b +} + +func TestAModuleOnlyTheTrunkHoldsIsReadAtTheTrunkAndPasses(t *testing.T) { // 1845/AC1 + b := trunkOnlyBench(t, "cb54756c3865", "cb54756c3865") + if v := b.judge(t, caseOf(t, pullOf(repoA, 7, head1))).Verdict; v.State != delivery.VerdictPass { + t.Fatalf("verdict = %+v", v) + } +} + +func TestAStalePinOfAModuleOnlyTheTrunkHoldsStillFailsNamingTheFileAndTheCommit(t *testing.T) { // 1845/AC2 + b := trunkOnlyBench(t, "cb54756c3865", "9aaa5c80ac5c") + v := b.judge(t, caseOf(t, pullOf(repoA, 7, head1))).Verdict + if v.State != delivery.VerdictFail || !strings.Contains(v.Failed.Words, repoA+"#7") || !strings.Contains(v.Failed.Words, "modules/new/go.mod") || !strings.Contains(v.Failed.Words, "9aaa5c80ac5c") { + t.Fatalf("verdict = %+v", v) + } +} + +func TestAModuleTheHeadHoldsIsJudgedByTheHeadsPin(t *testing.T) { // 1845/AC3 + b := trunkOnlyBench(t, "9aaa5c80ac5c", "cb54756c3865") // the head's pin is stale, the trunk's go.mod of the same module is fine + b.ex.Forge.(*fakeForge).goMods[repoA+" modules/m/go.mod "+trunkA] = capturedGoMod(t, "cb54756c3865") + v := b.judge(t, caseOf(t, pullOf(repoA, 7, head1))).Verdict + if v.State != delivery.VerdictFail || !strings.Contains(v.Failed.Words, "modules/m/go.mod") || !strings.Contains(v.Failed.Words, "9aaa5c80ac5c") { + t.Fatalf("verdict = %+v", v) + } +} + +func TestAReadThatFailsForAnotherReasonStillFailsAndIsNotTriedAtTheTrunk(t *testing.T) { // 1845/AC4 + b := trunkOnlyBench(t, "cb54756c3865", "cb54756c3865") + b.ex.Forge.(*fakeForge).errAt = map[string]error{head1: fmt.Errorf("the forge answered 500")} + v := b.judge(t, caseOf(t, pullOf(repoA, 7, head1))).Verdict + if v.State != delivery.VerdictFail || !strings.Contains(v.Failed.Words, "modules/m/go.mod") || !strings.Contains(v.Failed.Words, "the forge answered 500") { + t.Fatalf("verdict = %+v", v) + } +} + type runsAt map[string]string func (r runsAt) CommitOf(module string) string { return r[module] } -- 2.54.0 From 4a0c77105943a1d39a6350667a66e1c3c7ba77de Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 15:09:25 +0200 Subject: [PATCH 2/2] Match the forge's 404 by its status, not by digits a path's sha may hold (issue 1845) --- .../cmd/mesh-delivery-checker/forge.go | 9 +++++++-- .../cmd/mesh-delivery-checker/forge_test.go | 4 +++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge.go index f0d0c07..31702bf 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "regexp" "strings" "mesh-delivery-checker/internal/check" @@ -18,6 +19,9 @@ const ( commitsTool = "gitea.gitea_commits" ) +// notFoundStatus is the 404 of the forge's error, `Gitea API : `. +var notFoundStatus = regexp.MustCompile(`: 404( |$)`) + // forgeReads reads the forge through call (mesh-sdk process.Conn.Call as the checker's own account). type forgeReads struct { call func(key string, body any) (json.RawMessage, error) @@ -33,8 +37,9 @@ func (f forgeReads) File(_ context.Context, repository, path, ref string) (strin owner, repo := ownerRepo(repository) raw, err := f.call(getFileTool, map[string]string{"owner": owner, "repo": repo, "path": path, "ref": ref}) if err != nil { - if strings.Contains(err.Error(), "404") { - // The forge answers 404 for a file the ref does not hold (issue 1845). + if notFoundStatus.MatchString(err.Error()) { + // The forge answers 404 for a file the ref does not hold (issue 1845). The status is matched in its own place, + // after the path's colon: the path carries a sha that may hold the digits 404. return "", fmt.Errorf("%w: %v", check.ErrNotFound, err) } return "", err diff --git a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge_test.go b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge_test.go index b337053..e7ecc85 100644 --- a/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge_test.go +++ b/modules/mesh-delivery-checker/cmd/mesh-delivery-checker/forge_test.go @@ -79,7 +79,9 @@ func TestAFileTheRefDoesNotHoldIsNotFoundAndOtherFailuresAreNot(t *testing.T) { if _, err := f.File(context.Background(), "novox/mesh-catalog", "checks/particulars/go.mod", "98ca97fc"); !errors.Is(err, check.ErrNotFound) { t.Errorf("a 404 is %v, want ErrNotFound", err) } - f = forgeReads{call: func(string, any) (json.RawMessage, error) { return nil, errors.New("Gitea API: 500 internal error") }} + f = forgeReads{call: func(string, any) (json.RawMessage, error) { + return nil, errors.New("Gitea API contents/go.mod?ref=1111111111111111111111111111111404abcde: 500 internal error") + }} if _, err := f.File(context.Background(), "novox/mesh-catalog", "go.mod", "main"); err == nil || errors.Is(err, check.ErrNotFound) { t.Errorf("a 500 is %v, want a failure that is not ErrNotFound", err) } -- 2.54.0