diff --git a/.gitignore b/.gitignore index dfd78dd4..705acb20 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,4 @@ modules/messenger/messenger modules/claude-code/claude-code modules/mesh-watcher/mesh-watcher modules/mesh-delivery/mesh-delivery +modules/mesh-issues/cmd/mesh-issues/mesh-issues diff --git a/modules/mesh-issues/cmd/mesh-issues/groom.go b/modules/mesh-issues/cmd/mesh-issues/groom.go index dd1c122c..82f04fe1 100644 --- a/modules/mesh-issues/cmd/mesh-issues/groom.go +++ b/modules/mesh-issues/cmd/mesh-issues/groom.go @@ -129,8 +129,11 @@ func unresolved(is *Issue) bool { return is.Status == StatusOpen || is.Status == StatusDiagnosing || is.Status == StatusLocated } -// notePrefix starts the triage's note on the issue it judged. -const notePrefix = "Triage: " +// notePrefix starts every note the triage writes; verdictPrefix the one on the issue it judged. +const ( + notePrefix = "Triage: " + verdictPrefix = "Triage of this issue: " +) var ( legacyLine = regexp.MustCompile(`^Duplicate check: (duplicate of|related to) ([0-9]+)\.`) @@ -455,15 +458,15 @@ func triageNote(is *Issue, v *Verdict) string { switch { case v.closes(): fmt.Fprintf(&b, "%sduplicate of %d. %s This issue is closed as wontfix and its report is kept as a note on %d, "+ - "which stays open.\n", notePrefix, v.Issue, v.Reason, v.Issue) + "which stays open.\n", verdictPrefix, v.Issue, v.Reason, v.Issue) case v.Verdict == VerdictNone: most := "" if v.Older > v.Checked { most = fmt.Sprintf(" (the %d most recent of %d; the titles of the rest did not fit the question)", v.Checked, v.Older) } - fmt.Fprintf(&b, "%snone of %d older issues not resolved%s asks the same question.\n", notePrefix, v.Checked, most) + fmt.Fprintf(&b, "%snone of %d older issues not resolved%s asks the same question.\n", verdictPrefix, v.Checked, most) default: - fmt.Fprintf(&b, "%snot a duplicate; closest is %d.\n", notePrefix, v.Issue) + fmt.Fprintf(&b, "%snot a duplicate; closest is %d.\n", verdictPrefix, v.Issue) } value := func(name, given, judged, why string) { if judged == "" { @@ -551,7 +554,7 @@ func (t *Tracker) apply(newer uint64, v *Verdict) ([]string, error) { } // 1. One note saying the whole judgement. - if !noted(is, notePrefix) { + if !noted(is, verdictPrefix) { if _, err := t.note(GroomBy, newer, triageNote(is, v)); err != nil { return did, err } diff --git a/modules/mesh-issues/cmd/mesh-issues/issue.go b/modules/mesh-issues/cmd/mesh-issues/issue.go index 4e10a3cd..1c40c36d 100644 --- a/modules/mesh-issues/cmd/mesh-issues/issue.go +++ b/modules/mesh-issues/cmd/mesh-issues/issue.go @@ -91,6 +91,12 @@ type Issue struct { SuggestedOwner string `json:"suggested-owner,omitempty"` // Delivered is the pull requests of FixedBy the delivery said are delivered. Delivered []string `json:"delivered,omitempty"` + // Undelivered is the pull requests of FixedBy asked of the delivery at the move to resolved and not yet + // delivered then; their own delivery event marks them. + Undelivered []string `json:"undelivered,omitempty"` + // ReadyFor is the issues it depended on when it was last said ready (ADR 0279 §3), so one spell of + // readiness is said once however many dependencies let it go at one delivery. + ReadyFor []uint64 `json:"ready-for,omitempty"` // Triage is the triage's judgement as it was made, kept before it is applied. Triage *Verdict `json:"triage,omitempty"` // Opening is the id of the call that opened it, so a write whose answer was lost is known as its own. diff --git a/modules/mesh-issues/cmd/mesh-issues/mesh-issues b/modules/mesh-issues/cmd/mesh-issues/mesh-issues deleted file mode 100755 index 6b3ae756..00000000 Binary files a/modules/mesh-issues/cmd/mesh-issues/mesh-issues and /dev/null differ diff --git a/modules/mesh-issues/cmd/mesh-issues/ready.go b/modules/mesh-issues/cmd/mesh-issues/ready.go index c38899b8..46000d6f 100644 --- a/modules/mesh-issues/cmd/mesh-issues/ready.go +++ b/modules/mesh-issues/cmd/mesh-issues/ready.go @@ -118,6 +118,21 @@ func (t *Tracker) set(by string, a SetAsk, onlyEmpty bool) (*SetDone, error) { var errNothing = errors.New("nothing to change") +// MostDeliveryAsks is how many deliveries one handling of a `moved` event asks of mesh-delivery. +const MostDeliveryAsks = 3 + +func equalNumbers(a, b []uint64) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + // dependsOn is the issues an issue depends on. func dependsOn(is *Issue) []uint64 { var out []uint64 @@ -152,14 +167,23 @@ func reaches(all map[uint64]*Issue, from, to uint64) bool { return false } -// pullRequestRef is a fixed-by entry naming a pull request: `#`, optionally with the owner. -var pullRequestRef = regexp.MustCompile(`^(?:[A-Za-z0-9._-]+/)?([a-z0-9][a-z0-9._-]{0,99})#([1-9][0-9]{0,6})$`) +// pullRequestRef is a fixed-by entry naming a pull request: `#`, optionally with the owner; +// pullRequestPage the forge's page of one. +var ( + pullRequestRef = regexp.MustCompile(`^(?:[a-z0-9._-]+/)?([a-z0-9][a-z0-9._-]{0,99})#([1-9][0-9]{0,6})$`) + pullRequestPage = regexp.MustCompile(`^https?://\S+/([a-z0-9][a-z0-9._-]{0,99})/pulls?/([1-9][0-9]{0,6})/?$`) +) -// pullRequests is the pull requests an issue's fixed-by names, as `#`. +// pullRequests is the pull requests an issue's fixed-by names, as `#`, lower-case. func pullRequests(is *Issue) []string { var out []string for _, f := range is.FixedBy { - if m := pullRequestRef.FindStringSubmatch(strings.TrimSpace(f)); m != nil { + f = strings.ToLower(strings.TrimSpace(f)) + m := pullRequestRef.FindStringSubmatch(f) + if m == nil { + m = pullRequestPage.FindStringSubmatch(f) + } + if m != nil && !contains(out, m[1]+"#"+m[2]) { out = append(out, m[1]+"#"+m[2]) } } @@ -175,11 +199,29 @@ func contains(list []string, s string) bool { return false } -// done says whether an issue another depends on lets it go: wontfix, or resolved with every pull request its -// fix names delivered. -func done(dep *Issue) bool { - if dep.Status == StatusWontfix { - return true +// done says whether an issue another depends on lets it go: resolved with every pull request its fix names +// delivered, or wontfix — but a wontfix closed as the duplicate of another lets go only when that one does, as +// the work moved there (ADR 0279 §3). +func done(all map[uint64]*Issue, dep *Issue) bool { + seen := map[uint64]bool{} + for dep != nil && dep.Status == StatusWontfix { + if seen[dep.Number] { + return false + } + seen[dep.Number] = true + original := uint64(0) + for _, l := range dep.Links { + if l.Kind == LinkDuplicates { + original, _ = numberOfKey(l.Target) + } + } + if original == 0 { + return true + } + dep = all[original] + } + if dep == nil { + return false } if dep.Status != StatusResolved { return false @@ -196,7 +238,7 @@ func done(dep *Issue) bool { func waitsFor(all map[uint64]*Issue, is *Issue) []uint64 { var out []uint64 for _, n := range dependsOn(is) { - if dep := all[n]; dep == nil || !done(dep) { + if dep := all[n]; dep == nil || !done(all, dep) { out = append(out, n) } } @@ -278,24 +320,48 @@ func (t *Tracker) letGo(dep uint64) error { return err } d := all[dep] - if d == nil || !done(d) { + if d == nil || !done(all, d) { return nil } - for _, w := range neededBy(all, dep) { + // Those that depend on it, and on anything closed as its duplicate. + waiting := neededBy(all, dep) + for n, is := range all { + for _, l := range is.Links { + if l.Kind == LinkDuplicates && l.Target == keyOf(dep) { + waiting = append(waiting, neededBy(all, n)...) + } + } + } + for _, w := range waiting { is := all[w] if is == nil || !isReady(all, is) { continue } - prefix := fmt.Sprintf("Ready: issue %d", dep) - if noted(is, prefix) { + // Said once per spell of readiness: the dependencies it was said ready for are kept, claimed by + // compare-and-set before the note, so a second dependency letting it go at the same delivery, or the + // same event again, says nothing. + deps := dependsOn(is) + sort.Slice(deps, func(i, j int) bool { return deps[i] < deps[j] }) + claimed := false + if _, err := t.change(w, func(i *Issue) error { + claimed = false + if equalNumbers(i.ReadyFor, deps) { + return nil + } + i.ReadyFor, claimed = deps, true + return nil + }); err != nil { + return err + } + if !claimed { continue } why := "is resolved and its fix is delivered" if d.Status == StatusWontfix { why = "was closed as wontfix" } - if _, err := t.note(GroomBy, w, fmt.Sprintf("%s, which this one depends on, %s; nothing it depends on is left "+ - "waiting, so it is ready. (novox/hq ADR 0279)", prefix, why)); err != nil { + if _, err := t.note(GroomBy, w, fmt.Sprintf("Ready: issue %d, which this one depends on, %s; nothing it depends "+ + "on is left waiting, so it is ready. (novox/hq ADR 0279)", dep, why)); err != nil { return err } t.emit(EventReady, map[string]any{"number": w, "after": dep, "at": t.now()}) @@ -375,21 +441,37 @@ func (t *Tracker) OnMoved(e stdio.Envelope) error { return err } if body.To == StatusResolved && t.Deliveries != nil { + // At most a few asks per handling, each up to the bus's 30 seconds, under the runtime's two minutes; one + // that fails, or more left to ask, has the event offered again, so a delivery not asked is never silent: + // past the last offer the controller raises max-deliveries. + asked := 0 for _, pr := range pullRequests(is) { - if contains(is.Delivered, pr) { + if contains(is.Delivered, pr) || contains(is.Undelivered, pr) { continue } + if asked == MostDeliveryAsks { + return fmt.Errorf("issue %d names more pull requests than are asked at once; the rest at the next offer", body.Number) + } + asked++ repo, number, _ := strings.Cut(pr, "#") state, err := t.Deliveries(repo, number) if err != nil { - // Not known now: the delivery's own event marks it when it comes. - t.Logf("[mesh-issues] the delivery of %s (issue %d) could not be asked: %v", pr, body.Number, err) - continue + return fmt.Errorf("the delivery of %s (issue %d) could not be asked: %v", pr, body.Number, err) } if state == "delivered" { if _, err := t.markDelivered(pr, body.Number); err != nil { return err } + continue + } + // Asked and not delivered yet: the delivery's own event marks it when it comes. + if _, err := t.change(body.Number, func(i *Issue) error { + if !contains(i.Undelivered, pr) { + i.Undelivered = append(i.Undelivered, pr) + } + return nil + }); err != nil { + return err } } } diff --git a/modules/mesh-issues/cmd/mesh-issues/tracker.go b/modules/mesh-issues/cmd/mesh-issues/tracker.go index de050460..638c9f2d 100644 --- a/modules/mesh-issues/cmd/mesh-issues/tracker.go +++ b/modules/mesh-issues/cmd/mesh-issues/tracker.go @@ -620,6 +620,8 @@ type Row struct { Severity string `json:"severity,omitempty"` Needs string `json:"needs,omitempty"` Ready bool `json:"ready"` + // SuggestedOwner is the triage's or a person's suggestion, for an issue located nowhere yet. + SuggestedOwner string `json:"suggested-owner,omitempty"` } // Listed is the answer to a list. @@ -697,12 +699,8 @@ func (t *Tracker) List(a ListAsk) (*Listed, error) { } out.Matched++ if len(out.Rows) < limit { - owner := strings.Join(is.LocatedIn, "; ") - if owner == "" && is.SuggestedOwner != "" { - owner = is.SuggestedOwner + " (suggested)" - } - out.Rows = append(out.Rows, Row{Number: n, Status: is.Status, Owner: owner, Title: is.Title, - Severity: is.Severity, Needs: is.Needs, Ready: ready}) + out.Rows = append(out.Rows, Row{Number: n, Status: is.Status, Owner: strings.Join(is.LocatedIn, "; "), Title: is.Title, + Severity: is.Severity, Needs: is.Needs, Ready: ready, SuggestedOwner: is.SuggestedOwner}) } } switch { diff --git a/modules/mesh-issues/cmd/mesh-issues/triage_test.go b/modules/mesh-issues/cmd/mesh-issues/triage_test.go index 47b5b303..50501335 100644 --- a/modules/mesh-issues/cmd/mesh-issues/triage_test.go +++ b/modules/mesh-issues/cmd/mesh-issues/triage_test.go @@ -116,7 +116,7 @@ func TestAClearDuplicateIsClosedAndItsReportKeptOnTheOlder(t *testing.T) { t.Fatalf("links %+v", n.Links) } notes := triageNotes(n) - if len(notes) != 1 || !strings.HasPrefix(notes[0], "Triage: duplicate of 365.") || + if len(notes) != 1 || !strings.HasPrefix(notes[0], "Triage of this issue: duplicate of 365.") || !strings.Contains(notes[0], "the agent on control (sonnet)") || !strings.Contains(notes[0], "Severity: minor — a contained fix") { t.Fatalf("newer's notes %q", notes) } @@ -160,7 +160,7 @@ func TestRelatedOrNotCertainIsLinkedAndNotedAndNothingMoves(t *testing.T) { if len(n.Links) != 1 || n.Links[0].Kind != LinkRelatesTo || n.Links[0].Target != "365" { t.Fatalf("%v: links %+v", v, n.Links) } - if nn := triageNotes(n); len(nn) != 1 || !strings.HasPrefix(nn[0], "Triage: not a duplicate; closest is 365.") || + if nn := triageNotes(n); len(nn) != 1 || !strings.HasPrefix(nn[0], "Triage of this issue: not a duplicate; closest is 365.") || !strings.Contains(nn[0], "Relates to 365") { t.Fatalf("%v: %q", v, nn) } @@ -187,7 +187,7 @@ func TestAnIssueThatRepeatsNothingGainsOneNoteAndItsValues(t *testing.T) { n.SuggestedOwner != "mesh-catalog modules/records" { t.Fatalf("%+v", n) } - if nn := triageNotes(n); len(nn) != 1 || !strings.HasPrefix(nn[0], "Triage: none of 2 older issues") || + if nn := triageNotes(n); len(nn) != 1 || !strings.HasPrefix(nn[0], "Triage of this issue: none of 2 older issues") || !strings.Contains(nn[0], "Needs: operator — only the operator knows") || !strings.Contains(nn[0], "Suggested owner: mesh-catalog modules/records") { t.Fatalf("%q", nn) } @@ -728,7 +728,83 @@ func TestThePassRunsBesideTheTriageOfNewIssues(t *testing.T) { if p.State != PassDone || p.Judged != 6 { t.Fatalf("%+v", p) } - if nn := triageNotes(get(t, h, newer)); len(nn) != 1 || !strings.HasPrefix(nn[0], "Triage: none of 6 older") { + if nn := triageNotes(get(t, h, newer)); len(nn) != 1 || !strings.HasPrefix(nn[0], "Triage of this issue: none of 6 older") { t.Fatalf("%q", nn) } } + +// After a judgement that failed, the next one still writes its verdict note (review of PR 184). +func TestAFailureNoteDoesNotHideTheVerdictNote(t *testing.T) { + h := newHarness(t) + h.open(t, "the counter skips a number") + n := h.open(t, "a number is skipped") + s := withJudge(h, &stubJudge{err: errors.New("down")}) + if err := h.Check(n); err == nil { + t.Fatal("a failure was taken") + } + s.err = nil + if err := h.Check(n); err != nil { + t.Fatal(err) + } + nn := triageNotes(get(t, h, n)) + if len(nn) != 2 || !strings.HasPrefix(nn[1], "Triage of this issue: none of 1") { + t.Fatalf("%q", nn) + } +} + +// One delivery that lets two dependencies go says ready once; a wontfix duplicate lets go only with its original; +// fixed-by is read in any case and as the forge's page; a delivery that cannot be asked is not taken. +func TestReadyIsSaidOnceFollowsDuplicatesAndAskingFailsLoudly(t *testing.T) { + h := newHarness(t) + a := h.open(t, "a") + b := h.open(t, "b") + w := h.open(t, "waits on both") + _, _ = h.Link(LinkAsk{Number: w, Kind: LinkDependsOn, Target: keyOf(a)}) + _, _ = h.Link(LinkAsk{Number: w, Kind: LinkDependsOn, Target: keyOf(b)}) + h.Deliveries = func(string, string) (string, error) { return "merged", nil } + resolve(t, h, a, "Mesh-Catalog#9") + resolve(t, h, b, "https://forge.example/novox/mesh-catalog/pulls/9") + if got := pullRequests(get(t, h, a)); !reflect.DeepEqual(got, []string{"mesh-catalog#9"}) { + t.Fatalf("%v", got) + } + if got := pullRequests(get(t, h, b)); !reflect.DeepEqual(got, []string{"mesh-catalog#9"}) { + t.Fatalf("%v", got) + } + tr := stdio.Envelope{Body: json.RawMessage(`{"repository":"novox/mesh-catalog","number":9,"to":"delivered"}`)} + if err := h.OnTransition(tr); err != nil { + t.Fatal(err) + } + readies := 0 + for _, e := range h.said() { + if e.Event == EventReady { + readies++ + } + } + if readies != 1 || len(triageNotes(get(t, h, w))) != 1 { + t.Fatalf("%d ready events, notes %q", readies, triageNotes(get(t, h, w))) + } + + h2 := newHarness(t) + orig := h2.open(t, "the original") + dup := h2.open(t, "its duplicate") + w2 := h2.open(t, "waits on the duplicate") + _, _ = h2.Link(LinkAsk{Number: w2, Kind: LinkDependsOn, Target: keyOf(dup)}) + _, _ = h2.Link(LinkAsk{Number: dup, Kind: LinkDuplicates, Target: keyOf(orig)}) + _, _ = h2.Move(MoveAsk{Number: dup, Status: StatusWontfix, Reason: "duplicate of 365"}) + _ = h2.OnMoved(stdio.Envelope{Body: json.RawMessage(`{"number":366,"to":"wontfix"}`)}) + if v, _ := h2.View(w2); v.Ready { + t.Fatal("ready while the original is open") + } + h2.Deliveries = func(string, string) (string, error) { return "", errors.New("mesh-delivery did not answer") } + resolve(t, h2, orig, "mesh-catalog#3") + if err := h2.OnMoved(stdio.Envelope{Body: json.RawMessage(`{"number":365,"to":"resolved"}`)}); err == nil { + t.Fatal("a delivery that could not be asked was taken") + } + h2.Deliveries = func(string, string) (string, error) { return "delivered", nil } + if err := h2.OnMoved(stdio.Envelope{Body: json.RawMessage(`{"number":365,"to":"resolved"}`)}); err != nil { + t.Fatal(err) + } + if v, _ := h2.View(w2); !v.Ready || len(triageNotes(get(t, h2, w2))) != 1 { + t.Fatalf("not let go through the duplicate: %+v", v) + } +}