From 767928eb944df9dbc3e612e684fad80f49b38cd6 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 9 Nov 2026 11:29:31 +0100 Subject: [PATCH 1/3] Forge bridge reads gitea_api's {"result": body} wrapper in one place (issue 1966) Both ForgeMade and Tags took the tool's wrapper for the pull request / the tag list, so every forge-made merge read "#0 is not merged" and bridge_tags failed, leaving builds unnumbered. Quick fix: unwrap "result" in forgeAPI (the one Go reader of gitea_api). Thorough fix (chosen, the kind was met twice and hid behind a drifted fake): bridge.API answers what the tool answers, forgeBody is the one reader of that shape and refuses an answer without a result, naming it; ForgeMade refuses number 0 as "no pull request"; the unit fakes answer the tool's real wrapped shape, with rows for the bare body. Claude-Session: https://claude.ai/code/session_01NcbgfFSnn24M3GcqttoU7i --- .../main_test.go | 10 ++++++++- .../internal/bridge/bridge_test.go | 7 ++++-- .../internal/bridge/forgemade.go | 8 ++++++- .../internal/bridge/tags.go | 22 +++++++++++++++++-- 4 files changed, 41 insertions(+), 6 deletions(-) diff --git a/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go b/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go index 98bbd789..0be6d7ad 100644 --- a/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go +++ b/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go @@ -184,6 +184,8 @@ func TestTheBridgeToolsReadTheGroupsAndAskTheEndAgain(t *testing.T) { api: forgeAnswers(t, "/repos/novox/a/pulls/9", true, "fix/x", h2, c2, "dev"), fail: "not the trunk"}, {name: "a forge pull request with no head branch", raws: map[string][]byte{}, repository: "novox/a", merge: c2, number: 9, api: forgeAnswers(t, "/repos/novox/a/pulls/9", true, "refs/pull/9/head", h2, c2, "main"), fail: "no head branch"}, + {name: "a forge answer that is the bare body, no wrapper", raws: map[string][]byte{}, repository: "novox/a", merge: c2, number: 9, + api: func(string) (json.RawMessage, error) { return json.RawMessage(`{"number":9,"merged":true}`), nil }, fail: `{"result"`}, {name: "a forge that does not answer", raws: map[string][]byte{}, repository: "novox/a", merge: c2, api: func(string) (json.RawMessage, error) { return nil, errors.New("the forge is down") }, fail: "the forge is down"}, } { @@ -221,6 +223,12 @@ func forgeAnswers(t *testing.T, path string, merged bool, branch, head, merge, b if p != path { return nil, errors.New("the forge has no " + p) } - return body, nil + return wrapped(body), nil } } + +// wrapped is gitea_api's answer for a body: {"result": } (mesh-catalog modules/gitea/tools/index.ts). +func wrapped(body []byte) json.RawMessage { + b, _ := json.Marshal(map[string]json.RawMessage{"result": body}) + return b +} diff --git a/modules/mesh-delivery-forge-bridge/internal/bridge/bridge_test.go b/modules/mesh-delivery-forge-bridge/internal/bridge/bridge_test.go index 9da7668e..6744636b 100644 --- a/modules/mesh-delivery-forge-bridge/internal/bridge/bridge_test.go +++ b/modules/mesh-delivery-forge-bridge/internal/bridge/bridge_test.go @@ -428,10 +428,10 @@ func TestBridgeTagsAnswersAPackagesTagsByPrefixPageByPage(t *testing.T) { // DLV var tags []map[string]string _ = json.Unmarshal(full, &tags) tags = append(tags, map[string]string{"name": "go/v0.1.13"}, map[string]string{"name": "v0.0.1"}) - b, _ := json.Marshal(tags) + b, _ := json.Marshal(map[string]any{"result": tags}) return b, nil } - return json.RawMessage(`[{"name":"go/v0.1.2"}]`), nil + return json.RawMessage(`{"result":[{"name":"go/v0.1.2"}]}`), nil } a, err := Tags(api, "novox/mesh-sdk", "go/v") if err != nil || !slicesEqual(a.Tags, []string{"go/v0.1.13", "go/v0.1.2"}) || a.More || len(asked) != 2 || @@ -441,6 +441,9 @@ func TestBridgeTagsAnswersAPackagesTagsByPrefixPageByPage(t *testing.T) { // DLV if _, err := Tags(func(string) (json.RawMessage, error) { return nil, errors.New("down") }, "novox/mesh-sdk", ""); err == nil || !strings.Contains(err.Error(), "page 1") { t.Errorf("a forge not answering = %v, want an error naming the page", err) } + if _, err := Tags(func(string) (json.RawMessage, error) { return json.RawMessage(`[{"name":"go/v0.1.2"}]`), nil }, "novox/mesh-sdk", ""); err == nil || !strings.Contains(err.Error(), `{"result"`) { + t.Errorf("a bare list with no wrapper = %v, want the shape named", err) + } if _, err := Tags(api, "mesh-sdk", ""); err == nil { t.Error("a repository with no owner answered") } diff --git a/modules/mesh-delivery-forge-bridge/internal/bridge/forgemade.go b/modules/mesh-delivery-forge-bridge/internal/bridge/forgemade.go index d09e2929..811504cc 100644 --- a/modules/mesh-delivery-forge-bridge/internal/bridge/forgemade.go +++ b/modules/mesh-delivery-forge-bridge/internal/bridge/forgemade.go @@ -29,6 +29,10 @@ func ForgeMade(api API, owner, repo string, number int, merge string) (ForgePull if err != nil { return ForgePull{}, fmt.Errorf("the forge did not answer %s: %w", path, err) } + body, err := forgeBody(raw) + if err != nil { + return ForgePull{}, fmt.Errorf("the forge's answer for %s: %w", path, err) + } var p struct { Number int `json:"number"` Merged bool `json:"merged"` @@ -41,10 +45,12 @@ func ForgeMade(api API, owner, repo string, number int, merge string) (ForgePull Ref string `json:"ref"` } `json:"base"` } - if err := json.Unmarshal(raw, &p); err != nil { + if err := json.Unmarshal(body, &p); err != nil { return ForgePull{}, fmt.Errorf("the forge's answer for %s is no pull request: %w", path, err) } switch { + case p.Number == 0: + return ForgePull{}, fmt.Errorf("the forge's answer for %s is no pull request (no number)", path) case !p.Merged: return ForgePull{}, fmt.Errorf("the forge says the pull request %s/%s#%d is not merged", owner, repo, p.Number) case p.MergeCommitSHA != merge: diff --git a/modules/mesh-delivery-forge-bridge/internal/bridge/tags.go b/modules/mesh-delivery-forge-bridge/internal/bridge/tags.go index 4ae8c146..b4a942ec 100644 --- a/modules/mesh-delivery-forge-bridge/internal/bridge/tags.go +++ b/modules/mesh-delivery-forge-bridge/internal/bridge/tags.go @@ -18,9 +18,23 @@ const ( TagsMaxPages = 40 ) -// API asks the forge one GET of its API (the forge module's gitea_api), answering the body. +// API asks the forge one GET of its API (the forge module's gitea_api), answering what the tool answers: the body +// wrapped as {"result": }. Every reader of an API answer takes the body through forgeBody, the one place that +// knows that shape (issue 1966: two readers took the wrapper for the body). type API func(path string) (json.RawMessage, error) +// forgeBody is the body of the forge's answer: the tool's "result". An answer without one is refused, naming the shape, +// never read as an empty body. +func forgeBody(raw json.RawMessage) (json.RawMessage, error) { + var w struct { + Result json.RawMessage `json:"result"` + } + if err := json.Unmarshal(raw, &w); err != nil || len(w.Result) == 0 || string(w.Result) == "null" { + return nil, fmt.Errorf(`the forge module's answer is not {"result": }`) + } + return w.Result, nil +} + // TagsAnswer is what bridge_tags answers. type TagsAnswer struct { Repository string `json:"repository"` @@ -48,10 +62,14 @@ func Tags(api API, repository, prefix string) (TagsAnswer, error) { if err != nil { return TagsAnswer{}, fmt.Errorf("the forge did not answer the tags of %s (page %d): %w", out.Repository, page, err) } + body, err := forgeBody(raw) + if err != nil { + return TagsAnswer{}, fmt.Errorf("the forge's tags of %s (page %d): %w", out.Repository, page, err) + } var tags []struct { Name string `json:"name"` } - if err := json.Unmarshal(raw, &tags); err != nil { + if err := json.Unmarshal(body, &tags); err != nil { return TagsAnswer{}, fmt.Errorf("the forge's tags of %s (page %d) are no list of tags: %w", out.Repository, page, err) } for _, t := range tags { From 04fd0a4254e626f2c8c56a388ba6fca3678dbeaa Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 9 Nov 2026 11:29:31 +0100 Subject: [PATCH 2/3] Indexer ends a pending merge its repository's taken merge covers, without reading its message (issue 1966) Quick fix: none smaller than this (the bridge fix alone leaves a covered merge needing the bridge). Thorough fix (chosen): loadNewest asks takenAlready (already used for group members) before loading the newest merge of a repository; at or behind taken it is marked Covered, said once in the log, and never read. A merge whose ancestry cannot be asked is read as before. Test: a row pair behind/ahead of taken with a bridge that cannot answer. Claude-Session: https://claude.ai/code/session_01NcbgfFSnn24M3GcqttoU7i --- .../internal/index/pending.go | 8 ++++ .../internal/index/pending_test.go | 39 +++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/modules/mesh-delivery-indexer/internal/index/pending.go b/modules/mesh-delivery-indexer/internal/index/pending.go index 39a6e79f..067e4909 100644 --- a/modules/mesh-delivery-indexer/internal/index/pending.go +++ b/modules/mesh-delivery-indexer/internal/index/pending.go @@ -254,6 +254,14 @@ func (ix *Indexer) loadNewest(ctx context.Context, entries []PendingEntry) (out, continue } newest[repo] = true + // A merge at or behind its repository's taken merge writes nothing: it is ended as covered without reading its + // message, which a bridge that cannot give it would otherwise hold the whole revision for (issue 1966). A merge whose + // ancestry cannot be asked is read as before. + if taken, err := ix.takenAlready(ctx, forgemsg.GroupMerge{Repository: out[i].Forge.Repository, Number: out[i].Forge.Number, Merge: out[i].Forge.Merge}); err == nil && taken { + ix.logf("take-in: %s (%s#%d) is covered by the taken merge of %s, its message is not read", out[i].Key, out[i].Forge.Repository, out[i].Forge.Number, out[i].Forge.Repository) + out[i].Covered = true + continue + } full, err := ix.load(ctx, out[i]) if err != nil { lost = append(lost, out[i]) diff --git a/modules/mesh-delivery-indexer/internal/index/pending_test.go b/modules/mesh-delivery-indexer/internal/index/pending_test.go index 29807aab..e7d52e72 100644 --- a/modules/mesh-delivery-indexer/internal/index/pending_test.go +++ b/modules/mesh-delivery-indexer/internal/index/pending_test.go @@ -710,6 +710,9 @@ func TestTakeIn(t *testing.T) { // 1585/T3-T13 (ADR 0633) if len(b.said) != 1 || !slices.Equal(b.said[0].Lost, []string{takein.PendingKey(3)}) || len(waits) != 1 || !slices.Equal(waits[0].Lost, []string{takein.PendingKey(3)}) || b.commit(t, "avahi") != sha('b') { t.Fatalf("lost: said %+v, waits %+v, avahi at %s: want m.3 lost on the event and in indexer_pending, nothing taken in", b.said, waits, b.commit(t, "avahi")) } + if _, err := takein.EncodeTakeIn(b.said[0]); err != nil { + t.Errorf("the lost hold's event does not pass its check: %v", err) + } if v, _, _, _ := b.pending.Get(ctx, takein.PendingKey(3)); !strings.Contains(string(v), "the bridge does not answer") { t.Errorf("m.3 = %s: want the bridge's words", v) } @@ -753,6 +756,42 @@ func TestTakeIn(t *testing.T) { // 1585/T3-T13 (ADR 0633) t.Errorf("an old value with the message whole: err %v, avahi at %s: want it taken in", err, b.commit(t, "avahi")) } }) + t.Run("1966 a merge at or behind its repository's taken merge is ended without its message being read, one ahead of it is lost as before", func(t *testing.T) { + for _, c := range []struct { + name string + behind bool + }{{"behind taken", true}, {"ahead of taken", false}} { + b := newTakeInBed(t) + f := b.merge(21, sha('c'), "tree-avahi-3", "tree-indexer-2") + f.Words, f.Branch = strings.Repeat("x", 300*1024), "feat/g" + b.arrive(t, 2, f) // held by reference, for an indexer build that is not this one + b.ix.Taken = newMem() + b.ix.Contains = func(context.Context, string, string, string) (bool, error) { return c.behind, nil } + data, _ := takein.EncodeTaken(takein.Taken{Repository: trunk.RepositoryName(catalogueRepo), Merge: sha('d')}) + key, _ := trunk.RepositoryKey(catalogueRepo) + b.ix.Taken.(*memStore).put(key, data) + named := b.fp(t, Module) + asked := 0 + next := &Indexer{Store: b.ix.Store, Modules: b.ix.Modules, Manifests: b.ix.Manifests, Graph: b.ix.Graph, Verdicts: b.ix.Verdicts, + Packages: b.ix.Packages, Builds: b.ix.Builds, Pending: b.pending, Taken: b.ix.Taken, Contains: b.ix.Contains, + Message: func(context.Context, string, string, int) ([]byte, error) { + asked++ + return nil, errors.New("the bridge does not answer") + }, + SayTakeIn: b.ix.SayTakeIn, Logf: b.ix.Logf, GoSum: b.ix.GoSum, OwnBuild: func(context.Context) (string, error) { return named, nil }} + next.toolchains = b.ix.toolchains + next.FixOwnBuild(ctx) + if _, _, err := next.TakeInAtStart(ctx); err != nil { + t.Fatal(err) + } + if c.behind && (asked != 0 || len(b.keys()) != 0 || !strings.Contains(b.logs.String(), "covered by the taken merge")) { + t.Errorf("%s: asked %d, pending %v: want no read and the entry ended", c.name, asked, b.keys()) + } + if !c.behind && (asked == 0 || len(b.keys()) != 1) { + t.Errorf("%s: asked %d, pending %v: want the message asked and the entry held", c.name, asked, b.keys()) + } + } + }) t.Run("the dry run names every field of the indexer", func(t *testing.T) { // dry (pending.go) builds an indexer field by field: a field added to Indexer is named there, or this fails. named := map[string]bool{} From af3b678a92f01e5f3e376101202d5309bd92e8e9 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 9 Nov 2026 11:29:31 +0100 Subject: [PATCH 3/3] A lost take-in hold passes the take-in event's check, so it is said (issue 1966) holdLost builds a pending take-in naming neither WaitsFor nor Unknown, which TakeIn's check refused, so the hold stayed in the journal and the watchman never saw it. Quick fix: set Unknown on holdLost's event. Thorough fix (chosen): the check accepts a pending revision that names Lost, the field made for it; one naming nothing is still refused. Tests: a row accepting Lost, the refusal row kept, and the lost-hold scenario encodes its said event. Claude-Session: https://claude.ai/code/session_01NcbgfFSnn24M3GcqttoU7i --- .../mesh-delivery-forge-bridge-tools/main_test.go | 2 ++ .../internal/index/pending.go | 15 ++++++++++----- modules/mesh-delivery-indexer/takein/takein.go | 4 ++-- .../mesh-delivery-indexer/takein/takein_test.go | 9 ++++++++- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go b/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go index 0be6d7ad..946eece1 100644 --- a/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go +++ b/modules/mesh-delivery-forge-bridge/cmd/mesh-delivery-forge-bridge-tools/main_test.go @@ -186,6 +186,8 @@ func TestTheBridgeToolsReadTheGroupsAndAskTheEndAgain(t *testing.T) { api: forgeAnswers(t, "/repos/novox/a/pulls/9", true, "refs/pull/9/head", h2, c2, "main"), fail: "no head branch"}, {name: "a forge answer that is the bare body, no wrapper", raws: map[string][]byte{}, repository: "novox/a", merge: c2, number: 9, api: func(string) (json.RawMessage, error) { return json.RawMessage(`{"number":9,"merged":true}`), nil }, fail: `{"result"`}, + {name: "a forge answer with no pull request number", raws: map[string][]byte{}, repository: "novox/a", merge: c2, number: 9, + api: func(string) (json.RawMessage, error) { return json.RawMessage(`{"result":{"merged":true}}`), nil }, fail: "no number"}, {name: "a forge that does not answer", raws: map[string][]byte{}, repository: "novox/a", merge: c2, api: func(string) (json.RawMessage, error) { return nil, errors.New("the forge is down") }, fail: "the forge is down"}, } { diff --git a/modules/mesh-delivery-indexer/internal/index/pending.go b/modules/mesh-delivery-indexer/internal/index/pending.go index 067e4909..d92c6956 100644 --- a/modules/mesh-delivery-indexer/internal/index/pending.go +++ b/modules/mesh-delivery-indexer/internal/index/pending.go @@ -99,8 +99,9 @@ type PendingEntry struct { Forge forgemsg.Forge Light bool Holds []string - // Covered says a newer merge of the same repository is in the same revision: its message is not read or applied, since - // applying the newest writes every module whose manifest differs from what is held (modules.go). + // Covered says its message is not read or applied: either a newer merge of the same repository is in the same revision + // (applying the newest writes every module whose manifest differs from what is held, modules.go), or its repository's + // taken merge is at or ahead of it (loadNewest, takenAlready), so applying it would write nothing. Covered bool } @@ -613,7 +614,8 @@ func (ix *Indexer) headTaken(ctx context.Context, m meshdeliveryclient.Head) (bo return in, err } -// takenAlready says whether a group member's merge is at or behind its repository's taken merge. +// takenAlready says whether a merge (a group member's, or the newest pending merge of a repository in loadNewest) is at or +// behind its repository's taken merge. func (ix *Indexer) takenAlready(ctx context.Context, m forgemsg.GroupMerge) (bool, error) { if ix.Taken == nil { return false, nil @@ -978,8 +980,11 @@ func (ix *Indexer) takeInWholeBy(ctx context.Context, entries []PendingEntry, ow } for _, e := range claimed { if e.Covered { - by := newest[strings.ToLower(e.Forge.Repository)] - ix.logf("take-in: %s (%s#%d) is covered by %s, the newest merge of %s in this revision: its message is not read, and it is ended with it", e.Key, e.Forge.Repository, e.Forge.Number, by.Key, e.Forge.Repository) + by := "the taken merge" + if n, ok := newest[strings.ToLower(e.Forge.Repository)]; ok { + by = n.Key + ", the newest merge" + } + ix.logf("take-in: %s (%s#%d) is covered by %s of %s in this revision: its message is not read, and it is ended with it", e.Key, e.Forge.Repository, e.Forge.Number, by, e.Forge.Repository) // Its pull request leaves its group's candidate as a merged message takes it out; nothing else of it is applied. if err := ix.removeCandidate(ctx, e.Forge); err != nil { ix.release(ctx, claimed) diff --git a/modules/mesh-delivery-indexer/takein/takein.go b/modules/mesh-delivery-indexer/takein/takein.go index b809ca5c..645535c2 100644 --- a/modules/mesh-delivery-indexer/takein/takein.go +++ b/modules/mesh-delivery-indexer/takein/takein.go @@ -172,8 +172,8 @@ func (t TakeIn) check() error { if !strings.HasPrefix(t.Key, "m.") { return fmt.Errorf("delivery: a revision's take-in key is its oldest merge's m., not %q", t.Key) } - if t.Kind == TakeInPending && t.WaitsFor == "" && t.Unknown == "" { - return fmt.Errorf("delivery: a pending revision %s names the indexer build it waits for, or why the indexer does not know its own", t.Key) + if t.Kind == TakeInPending && t.WaitsFor == "" && t.Unknown == "" && len(t.Lost) == 0 { + return fmt.Errorf("delivery: a pending revision %s names the indexer build it waits for, why the indexer does not know its own, or the merges it lost", t.Key) } case TakeInGroup: if t.Group == "" { diff --git a/modules/mesh-delivery-indexer/takein/takein_test.go b/modules/mesh-delivery-indexer/takein/takein_test.go index baaa3844..8aef1dbd 100644 --- a/modules/mesh-delivery-indexer/takein/takein_test.go +++ b/modules/mesh-delivery-indexer/takein/takein_test.go @@ -70,7 +70,7 @@ func TestTakeInEvents(t *testing.T) { }{ {"group take-in without its group", "names its group", 1, func(t *TakeIn) { t.Group = "" }}, {"group take-in under another group's key", "g.other", 1, func(t *TakeIn) { t.Key = GroupTakeInKey("other") }}, - {"pending revision waiting for no build", "waits for", 0, func(t *TakeIn) { t.WaitsFor = "" }}, + {"pending revision waiting for no build", "lost", 0, func(t *TakeIn) { t.WaitsFor = "" }}, {"pending with a hand act", "only a taken event", 0, func(t *TakeIn) { t.By = "someone@node" }}, {"hand act without its by", "by", 2, func(t *TakeIn) { t.By = "" }}, {"take-in without merges", "no merge", 0, func(t *TakeIn) { t.Merges = nil }}, @@ -84,6 +84,13 @@ func TestTakeInEvents(t *testing.T) { t.Errorf("%s: %v, want a refusal naming %q", c.name, err, c.want) } } + // A revision held for a merge the forge bridge could not give waits for no build and names what it lost (issue 1966: it was never said). + if held := sampleTakeIns(since)[0]; true { + held.WaitsFor, held.Lost = "", []string{PendingKey(12)} + if _, err := EncodeTakeIn(held); err != nil { + t.Errorf("a lost hold is refused: %v", err) + } + } // indexer_pending's answer reads back; a wait of neither kind is refused. pa := PendingAnswer{Waits: []PendingWait{{Key: PendingKey(12), What: TakeInRevision, Merges: sampleTakeIns(since)[0].Merges, Keys: []string{PendingKey(12)}, WaitsFor: "src2:abc", Since: since, Holds: []string{"mesh-delivery-indexer"}, Said: "take-in.pending." + PendingKey(12), SaidAt: since}}}