diff --git a/modules/mesh-config/cmd/mesh-config/pulls.go b/modules/mesh-config/cmd/mesh-config/pulls.go index 5f7742a5..c0b82ae0 100644 --- a/modules/mesh-config/cmd/mesh-config/pulls.go +++ b/modules/mesh-config/cmd/mesh-config/pulls.go @@ -102,7 +102,7 @@ func declDigest(d Declaration) string { // PullForge is the forge as this module asks it of a pull request: its state, and whether it merged. type PullForge interface { - Pull(owner, repo string, number int) (open, merged bool, err error) + Pull(owner, repo string, number int) (open, merged bool, head string, err error) } // pulls reads what is kept for pull requests, and the held values. @@ -424,16 +424,16 @@ func (c *Config) keep(p Proposal) (string, error) { // PullMerged writes each value held for a pull request as the module's own value on its node, with the approval kept // beside it; it resolves once the trunk declares the key. It answers a line for each. -func (c *Config) PullMerged(pull string) []string { - return c.settlePull(pull, true) +func (c *Config) PullMerged(pull, head string) []string { + return c.settlePull(pull, true, head) } // PullClosed drops the values held for a pull request closed unmerged, and says that nothing was set. func (c *Config) PullClosed(pull string) []string { - return c.settlePull(pull, false) + return c.settlePull(pull, false, "") } -func (c *Config) settlePull(pull string, merged bool) []string { +func (c *Config) settlePull(pull string, merged bool, head string) []string { c.mu.Lock() defer c.mu.Unlock() var s Sources @@ -453,6 +453,24 @@ func (c *Config) settlePull(pull string, merged bool) []string { said = append(said, c.dropKept(kv, kv.Pull+" closed unmerged")...) continue } + // **Only a value held at the merged head, whose key that head declares as it was approved** (ADR 0502 clause 7). + why := "" + pd, known := s.Pulls[pullKey(pull)] + d, declared := pd.Declares[kv.Module][kv.Key] + switch { + case head == "": + why = "the merged head is not known, so the value cannot be held to it" + case !strings.EqualFold(head, kv.Head): + why = fmt.Sprintf("it is held at head %s and the pull request merged at %s", short(kv.Head), short(head)) + case !known || !declared || declDigest(d) != kv.Digest: + why = "the merged head does not declare the key as the operator was shown" + } + if why != "" { + said = append(said, fmt.Sprintf("%s of %s on %s was held for %s, which merged: dropped, nothing was set (%s)", + kv.Key, kv.Module, kv.Node, kv.Pull, why)) + said = append(said, c.dropKept(kv, kv.Pull+" merged, but "+why)...) + continue + } vk := kv.level().ValueKey(kv.Key) v := Value{Value: compact(kv.Value), By: kv.By, Via: kv.Via, Why: kv.Why + " (held for " + kv.Pull + ", merged)", Approved: kv.Approved, At: c.now()} @@ -501,6 +519,8 @@ type PullEvent struct { Repo string `json:"repo"` Number int `json:"number"` Merged bool `json:"merged,omitempty"` + // HeadSHA is the head the pull request merged at (the forge's own word). + HeadSHA string `json:"head_sha,omitempty"` } func (p PullEvent) name() string { return fmt.Sprintf("%s/%s#%d", p.Owner, p.Repo, p.Number) } @@ -511,15 +531,24 @@ func (c *Config) HeardPull(p PullEvent, merged bool) []string { if p.Owner == "" || p.Repo == "" || p.Number <= 0 { return nil } + head := p.HeadSHA if !merged && !p.Merged && c.Forge != nil { - if open, m, err := c.Forge.Pull(p.Owner, p.Repo, p.Number); err == nil { - if open { - return nil - } - merged = m + // A close is told from a merge by the forge; when it cannot be asked nothing is dropped, and the five-minute + // look finds it. + open, m, h, err := c.Forge.Pull(p.Owner, p.Repo, p.Number) + if err != nil { + c.logf("[mesh-config] %s closed, and the forge does not say whether it merged (%v); the next look decides", p.name(), err) + return nil + } + if open { + return nil + } + merged = m + if head == "" { + head = h } } - return c.settlePull(p.name(), merged || p.Merged) + return c.settlePull(p.name(), merged || p.Merged, head) } // SweepPulls looks at the forge for every pull request it keeps something for, so a merge or a close it did not hear @@ -549,13 +578,13 @@ func (c *Config) SweepPulls() []string { if err != nil { continue } - open, merged, err := c.Forge.Pull(owner, repo, n) + open, merged, head, err := c.Forge.Pull(owner, repo, n) if err != nil { said = append(said, fmt.Sprintf("the forge does not say whether %s is open: %v", pull, err)) continue } if !open { - said = append(said, c.settlePull(pull, merged)...) + said = append(said, c.settlePull(pull, merged, head)...) } } return said @@ -573,25 +602,28 @@ type toolForge struct { } // Pull asks the forge's gitea_get_pull_request whether a pull request is open, and whether it merged. -func (f toolForge) Pull(owner, repo string, number int) (bool, bool, error) { +func (f toolForge) Pull(owner, repo string, number int) (bool, bool, string, error) { raw, err := f.ask("gitea.gitea_get_pull_request", map[string]any{"owner": owner, "repo": repo, "number": number}) if err != nil { - return false, false, err + return false, false, "", err } answer, output, ok := unwrapAnswer(raw) if !ok { - return false, false, fmt.Errorf("the forge refused gitea_get_pull_request: %s", output) + return false, false, "", fmt.Errorf("the forge refused gitea_get_pull_request: %s", output) } var a struct { Pull struct { State string `json:"state"` Merged bool `json:"merged"` + Head struct { + SHA string `json:"sha"` + } `json:"head"` } `json:"pull"` } if err := json.Unmarshal(answer, &a); err != nil || a.Pull.State == "" { - return false, false, fmt.Errorf("the forge's gitea_get_pull_request is not readable: %s", output) + return false, false, "", fmt.Errorf("the forge's gitea_get_pull_request is not readable: %s", output) } - return a.Pull.State == "open", a.Pull.Merged, nil + return a.Pull.State == "open", a.Pull.Merged, a.Pull.Head.SHA, nil } // unwrapAnswer reads a tool's answer whatever wraps it: the protocol's content list, or {output, ok, answer}. diff --git a/modules/mesh-config/cmd/mesh-config/pulls_test.go b/modules/mesh-config/cmd/mesh-config/pulls_test.go index e8a514b6..0f664f7d 100644 --- a/modules/mesh-config/cmd/mesh-config/pulls_test.go +++ b/modules/mesh-config/cmd/mesh-config/pulls_test.go @@ -36,18 +36,19 @@ func aHoldingMesh(t *testing.T) (*Config, *memStore, *standIn, *standForge) { type standForge struct { state map[string][2]bool // pull -> open, merged + head string err error } -func (f *standForge) Pull(owner, repo string, n int) (bool, bool, error) { +func (f *standForge) Pull(owner, repo string, n int) (bool, bool, string, error) { if f.err != nil { - return false, false, f.err + return false, false, "", f.err } st, ok := f.state[(PullEvent{Owner: owner, Repo: repo, Number: n}).name()] if !ok { - return true, false, nil + return true, false, f.head, nil } - return st[0], st[1], nil + return st[0], st[1], f.head, nil } func tell(t *testing.T, c *Config, pull, head string, declares map[string]any) *PullDeclaresAnswer { @@ -318,7 +319,7 @@ func TestTheMergeAppliesAHeldValueAndItResolvesOnceTheTrunkDeclaresTheKey(t *tes }() m.events, m.handActs = nil, nil - said := c.HeardPull(PullEvent{Owner: "novox", Repo: "mesh-catalog", Number: 434, Merged: true}, true) + said := c.HeardPull(PullEvent{Owner: "novox", Repo: "mesh-catalog", Number: 434, Merged: true, HeadSHA: "abc1234def"}, true) if len(said) == 0 { t.Fatal("the merge said nothing") } @@ -355,7 +356,7 @@ func TestTheMergeAppliesAHeldValueAndItResolvesOnceTheTrunkDeclaresTheKey(t *tes } // A merge heard twice sets nothing twice. m.events = nil - c.HeardPull(PullEvent{Owner: "novox", Repo: "mesh-catalog", Number: 434, Merged: true}, true) + c.HeardPull(PullEvent{Owner: "novox", Repo: "mesh-catalog", Number: 434, Merged: true, HeadSHA: "abc1234def"}, true) if len(m.events) != 0 { t.Errorf("a second merge raised %+v", m.events) } @@ -390,6 +391,7 @@ func TestACloseEventForAMergedPullRequestIsItsMerge(t *testing.T) { tell(t, c, thePull, "abc1234def", n8nPullDeclares()) holdIt(t, c, m, r) f.state[thePull] = [2]bool{false, true} + f.head = "abc1234def" c.HeardPull(PullEvent{Owner: "novox", Repo: "mesh-catalog", Number: 434}, false) if v, set := storedValue(t, m, "module.ace.n8n.media-gid"); !set || string(v.Value) != "2000" { t.Errorf("the merge of a pull request announced closed set %+v", v) @@ -410,6 +412,7 @@ func TestTheLookAtTheForgeFindsAMissedMergeOrCloseAndLeavesAnOpenPullRequest(t * t.Errorf("a silent forge: said %v", said) } f.err = nil + f.head = "abc1234def" f.state[thePull] = [2]bool{false, true} c.SweepPulls() if v, set := storedValue(t, m, "module.ace.n8n.media-gid"); !set || string(v.Value) != "2000" || keptCount(m) != 0 { @@ -443,3 +446,50 @@ func TestThePullArgumentsAreRead(t *testing.T) { } _ = time.Second } + +// 1826 review r1: the merge applies a value only if it is held at the merged head; otherwise it is dropped and said. +func TestTheMergeAppliesOnlyAValueHeldAtTheMergedHead(t *testing.T) { + for name, head := range map[string]string{"another head": "9999999999", "no head told": ""} { + c, m, r, _ := aHoldingMesh(t) + tell(t, c, thePull, "abc1234def", n8nPullDeclares()) + holdIt(t, c, m, r) + said := c.HeardPull(PullEvent{Owner: "novox", Repo: "mesh-catalog", Number: 434, Merged: true, HeadSHA: head}, true) + if _, set := storedValue(t, m, "module.ace.n8n.media-gid"); set || keptCount(m) != 0 || + !strings.Contains(strings.Join(said, "\n"), "dropped, nothing was set") { + t.Errorf("%s: set=%v held=%d said %v", name, set, keptCount(m), said) + } + if len(m.events) == 0 || m.events[len(m.events)-1].Action != ActionDrop { + t.Errorf("%s: raised %+v", name, m.events) + } + } +} + +// 1826 review r1: a later head without the declaration drops the value, so the merge has nothing to apply. +func TestALaterHeadWithoutTheDeclarationLeavesNothingToApplyAtTheMerge(t *testing.T) { + c, m, r, _ := aHoldingMesh(t) + tell(t, c, thePull, "abc1234def", n8nPullDeclares()) + holdIt(t, c, m, r) + tell(t, c, thePull, "bbbbbbbbbb", map[string]any{}) + c.HeardPull(PullEvent{Owner: "novox", Repo: "mesh-catalog", Number: 434, Merged: true, HeadSHA: "bbbbbbbbbb"}, true) + if _, set := storedValue(t, m, "module.ace.n8n.media-gid"); set { + t.Error("a value held for a declaration the merged head lacks was set") + } +} + +// 1826 review r1: a close the forge cannot be asked about drops nothing; the five-minute look decides. +func TestACloseThatTheForgeCannotBeAskedAboutDropsNothing(t *testing.T) { + c, m, r, f := aHoldingMesh(t) + tell(t, c, thePull, "abc1234def", n8nPullDeclares()) + holdIt(t, c, m, r) + f.err = errors.New("the forge is down") + c.HeardPull(PullEvent{Owner: "novox", Repo: "mesh-catalog", Number: 434}, false) + if keptCount(m) != 1 { + t.Fatal("a close the forge could not confirm dropped the value") + } + f.err = nil + f.state[thePull] = [2]bool{false, false} + c.SweepPulls() + if keptCount(m) != 0 { + t.Error("the look did not drop it") + } +}