mesh-issues: review — the verdict note has its own marker, ready is said once per spell and follows a duplicate to its original, a delivery that cannot be asked is not taken, fixed-by read in any case and as a page; no built binary (hq ADR 0279)

.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

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
 		}

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.

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: `<repository>#<number>`, 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: `<repository>#<number>`, 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 `<repository>#<number>`.
+// pullRequests is the pull requests an issue's fixed-by names, as `<repository>#<number>`, 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
 			}
 		}
 	}

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 {

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)
+	}
+}