Gather the waiting duplicates of a decided original under the oldest of them

docs/setup.md

@@ -108,7 +108,7 @@ At the end of each run, tamis copies the state of every advisory to `State`, rec
 
 An advisory in the `triage` state that tamis commented but that has no item, for example one triaged before the board existed, is triaged again without a new comment. When that triage fails, the item gets `Analysis failed` and the advisory is not retried. Deleting an item by hand triggers that triage again on the next run.
 
-With `issues-repository` set, for example to `your-org/your-project-security`, new items are issues of that repository instead of drafts, and each duplicate becomes a sub-issue of its `Original report`. tamis closes an issue when its advisory is published, closed or withdrawn. Everyone with access to that repository reads the reports, and each new issue notifies its watchers. tamis refuses a public repository and the repository of the advisories, and then leaves the board out of the run.
+With `issues-repository` set, for example to `your-org/your-project-security`, new items are issues of that repository instead of drafts, and each duplicate becomes a sub-issue of its `Original report`. Once that report leaves `triage` and `draft`, its duplicates still waiting move under the oldest of them, so a view filtered on `State` shows them, and that issue gets a comment naming the report it left. tamis closes an issue when its advisory is published, closed or withdrawn. Everyone with access to that repository reads the reports, and each new issue notifies its watchers. tamis refuses a public repository and the repository of the advisories, and then leaves the board out of the run.
 
 tamis writes the issues and adds them to the project with `issues-token`, a second installation token that reaches the issues repository only, and refuses it when it also reaches `repository`. `github-token` updates the fields of the items, so it reads the issues repository too, but keep its "Issues" permission at read, so neither token can open an issue on the project:
 

internal/board/board.go

@@ -126,6 +126,7 @@ type Issues interface {
 	ListIssues(ctx context.Context, repo string) ([]github.Issue, error)
 	CreateIssue(ctx context.Context, repo, title, body string) (github.Issue, error)
 	UpdateIssue(ctx context.Context, repo string, number int, fields map[string]string) error
+	CommentIssue(ctx context.Context, repo string, number int, body string) error
 	AddSubIssue(ctx context.Context, repo string, parent int, child int64) error
 	RemoveSubIssue(ctx context.Context, repo string, parent int, child int64) error
 	// AddProjectIssue needs a token that sees the issue, which the token of the advisories does not.
@@ -569,17 +570,17 @@ func (b *Board) issueItem(id int64) *item {
 	return nil
 }
 
-// nest returns the item the issue of it goes under: the oldest report of its Duplicate of chain that has an issue.
-// When none has, as for the duplicates of an original closed before the board, the reports of the group gather
-// under the oldest of them that has an issue, and nil means it is that one.
-func (b *Board) nest(it *item) *item {
-	if parent := b.ancestor(it); parent != nil || it.duplicateOf == "" {
+// nest returns the item the issue of it goes under: the oldest report of its Duplicate of chain that has an issue
+// and waits for a decision. When none has, as for the duplicates of a closed original, the reports of the group
+// gather under the oldest of them that has an issue and waits, and nil means it is that one.
+func (b *Board) nest(it *item, states map[string]string) *item {
+	if parent := b.ancestor(it, states); parent != nil || it.duplicateOf == "" {
 		return parent
 	}
 	group := strings.ToLower(b.oldest(it.duplicateOf))
 	head := it
 	for _, other := range b.items {
-		if other.issue == 0 || other.duplicateOf == "" || b.ancestor(other) != nil || strings.ToLower(b.oldest(other.duplicateOf)) != group {
+		if other.issue == 0 || other.duplicateOf == "" || !waiting(other, states) || b.ancestor(other, states) != nil || strings.ToLower(b.oldest(other.duplicateOf)) != group {
 			continue
 		}
 		if other.reported < head.reported || other.reported == head.reported && strings.ToLower(other.ghsa) < strings.ToLower(head.ghsa) {
@@ -593,17 +594,19 @@ func (b *Board) nest(it *item) *item {
 	return head
 }
 
-// ancestor returns the oldest report of the Duplicate of chain of it that has an issue, nil when none has.
-func (b *Board) ancestor(it *item) *item {
+// ancestor returns the oldest report of the Duplicate of chain of it that has an issue and waits for a decision,
+// nil when none has. A report that no longer waits itself stays under the oldest that has an issue.
+func (b *Board) ancestor(it *item, states map[string]string) *item {
 	var parent *item
+	decided := !waiting(it, states)
 	seen := map[*item]bool{it: true}
 	for ghsa := it.duplicateOf; ghsa != ""; {
 		next := b.items[strings.ToLower(ghsa)]
 		if next == nil || seen[next] {
 			break
 		}
 		seen[next] = true
-		if next.issue != 0 {
+		if next.issue != 0 && (decided || waiting(next, states)) {
 			parent = next
 		}
 		ghsa = next.duplicateOf
@@ -612,6 +615,22 @@ func (b *Board) ancestor(it *item) *item {
 	return parent
 }
 
+// stateOf returns the state of the advisory of it, the one Sync received when it has one.
+func stateOf(it *item, states map[string]string) string {
+	if state, ok := states[strings.ToLower(it.ghsa)]; ok {
+		return state
+	}
+
+	return it.state
+}
+
+// waiting tells whether the advisory of it waits for a decision.
+func waiting(it *item, states map[string]string) bool {
+	state := stateOf(it, states)
+
+	return state == "triage" || state == "draft"
+}
+
 // option sets a single select field, or clears it when the value has no option.
 func (b *Board) option(field, value string) github.FieldValue {
 	v := github.FieldValue{ID: b.ids[field]}
@@ -637,7 +656,7 @@ func (b *Board) Sync(ctx context.Context, advisories []github.Advisory) error {
 		case b.issues == "" && it.duplicateOf != "":
 			counts[strings.ToLower(it.duplicateOf)]++
 		case b.issues != "" && it.issue != 0:
-			if parent := b.nest(it); parent != nil {
+			if parent := b.nest(it, states); parent != nil {
 				counts[strings.ToLower(parent.ghsa)]++
 			}
 		}
@@ -673,17 +692,26 @@ func (b *Board) Sync(ctx context.Context, advisories []github.Advisory) error {
 		}
 		// The group is only written once the sub-issue is linked, so a failed link is tried again next run.
 		// The group can predate the issue of its original, so the link follows the parent the issue has.
-		if parent := b.nest(it); it.issue != 0 && parent != nil && it.parent != parent.issueID {
+		if parent := b.nest(it, states); it.issue != 0 && parent != nil && it.parent != parent.issueID {
 			if err := b.ig.AddSubIssue(ctx, b.issues, parent.issue, it.issueID); err != nil {
 				errs = append(errs, err)
 				group = it.group
 			} else {
 				it.parent = parent.issueID
 			}
 		} else if it.issue != 0 && parent == nil && it.parent != 0 {
-			// A report that is no longer a duplicate leaves the issue it was nested under, when that one is known.
+			// A report that is no longer a duplicate, or now heads a group whose original left triage, leaves the issue
+			// it was nested under, when that one is known. The note on this issue comes first, so a failed one is
+			// posted again next run.
 			if old := b.issueItem(it.parent); old != nil {
-				if err := b.ig.RemoveSubIssue(ctx, b.issues, old.issue, it.issueID); err != nil {
+				var err error
+				if it.duplicateOf != "" && waiting(it, states) && !waiting(old, states) {
+					err = b.ig.CommentIssue(ctx, b.issues, it.issue, fmt.Sprintf("%s, the report this issue was a sub-issue of, is now %s. The duplicates of its group still in triage or draft gather under the oldest of them, this issue.", old.ghsa, stateOf(old, states)))
+				}
+				if err == nil {
+					err = b.ig.RemoveSubIssue(ctx, b.issues, old.issue, it.issueID)
+				}
+				if err != nil {
 					errs = append(errs, err)
 				} else {
 					it.parent = 0

internal/board/board_test.go

@@ -717,6 +717,7 @@ func TestSyncTakesOutAReportThatIsNoLongerADuplicate(t *testing.T) {
 	p.IssueErr = nil
 	require.NoError(t, b.Sync(t.Context(), nil))
 	require.Zero(t, dup.Parent, "a report cleared by hand stayed nested")
+	require.Empty(t, dup.Comments, "a report that is no longer a duplicate got a note")
 }
 
 func TestSyncGathersTheDuplicatesOfAClosedOriginalUnderTheOldestOfThem(t *testing.T) {
@@ -748,6 +749,52 @@ func TestSyncGathersTheDuplicatesOfAClosedOriginalUnderTheOldestOfThem(t *testin
 	require.Equal(t, writes, p.Writes, "a nested group was written again")
 }
 
+func TestSyncTakesTheDuplicatesOutOfAnOriginalThatLeftTriage(t *testing.T) {
+	p := boardtest.New()
+	b, err := Open(t.Context(), p, p, "php/16", "acme/private-issues", "php/php-src")
+	require.NoError(t, err)
+	put := func(g, state, dup string, day int) {
+		a := github.Advisory{GHSAID: g, Summary: g, State: state, CreatedAt: reported.AddDate(0, 0, day)}
+		require.NoError(t, b.Put(t.Context(), a, Duplicate, &Triage{DuplicateOf: dup}, ""))
+	}
+	put("GHSA-root", "triage", "", -2)
+	put("GHSA-gone", "closed", "GHSA-root", -1)
+	put("GHSA-first", "triage", "GHSA-root", 0)
+	put("GHSA-later", "triage", "GHSA-root", 1)
+	require.NoError(t, b.Sync(t.Context(), nil))
+	root, gone, first, later := p.IssueOf(p.Find("GHSA-root")), p.IssueOf(p.Find("GHSA-gone")), p.IssueOf(p.Find("GHSA-first")), p.IssueOf(p.Find("GHSA-later"))
+	require.Equal(t, int(root.ID-1000), gone.Parent)
+	require.Equal(t, int(root.ID-1000), first.Parent)
+	require.Equal(t, int(root.ID-1000), later.Parent)
+
+	closed := []github.Advisory{{GHSAID: "GHSA-root", State: "published"}, {GHSAID: "GHSA-gone", State: "closed"}, {GHSAID: "GHSA-first", State: "triage"}, {GHSAID: "GHSA-later", State: "draft"}}
+	p.IssueErr = errors.New("HTTP 502")
+	require.Error(t, b.Sync(t.Context(), closed))
+	require.Equal(t, int(root.ID-1000), later.Parent, "a failed link changed the parent")
+	require.Equal(t, int(root.ID-1000), first.Parent, "a failed note let the issue leave its original")
+	require.Empty(t, first.Comments)
+
+	p.IssueErr = nil
+	require.NoError(t, b.Sync(t.Context(), closed))
+	require.Zero(t, first.Parent, "the oldest waiting duplicate stayed under its published original")
+	require.Equal(t, int(first.ID-1000), later.Parent, "the duplicate is not under the oldest waiting one")
+	require.Equal(t, "GHSA-root", p.Value(p.Find("GHSA-later"), "Original report"))
+	require.Equal(t, int(root.ID-1000), gone.Parent, "a closed duplicate left its published original")
+	require.Empty(t, gone.Comments, "a closed duplicate got a note")
+	require.Equal(t, "1", p.Value(p.Find("GHSA-root"), "Duplicates"), "the published original kept the count of its waiting duplicates")
+	require.Equal(t, "1", p.Value(p.Find("GHSA-first"), "Duplicates"))
+	require.Len(t, first.Comments, 1, "the new head of the group got no note")
+	require.Contains(t, first.Comments[0], "GHSA-root")
+	require.Contains(t, first.Comments[0], "published")
+	require.Empty(t, later.Comments, "a duplicate that only changed parent got a note")
+	require.NoError(t, b.Sync(t.Context(), closed))
+	require.Len(t, first.Comments, 1, "the note was posted again")
+
+	require.NoError(t, b.Sync(t.Context(), []github.Advisory{{GHSAID: "GHSA-root", State: "triage"}}))
+	require.Equal(t, int(root.ID-1000), first.Parent, "a duplicate stayed out of its original back in triage")
+	require.Equal(t, int(root.ID-1000), later.Parent)
+}
+
 func TestSyncCountsTheSubIssuesOfEachIssue(t *testing.T) {
 	p := boardtest.New()
 	b, err := Open(t.Context(), p, p, "php/16", "acme/private-issues", "php/php-src")

internal/board/boardtest/boardtest.go

@@ -43,6 +43,7 @@ type Issue struct {
 	ID                                    int64
 	Repo, Title, Body, State, StateReason string
 	Parent                                int
+	Comments                              []string
 }
 
 func New() *Project {
@@ -303,6 +304,19 @@ func (p *Project) AddProjectIssue(_ context.Context, _ string, _ int, issue int6
 	return github.ProjectItem{ID: id, ContentType: "Issue"}, nil
 }
 
+func (p *Project) CommentIssue(_ context.Context, repo string, number int, body string) error {
+	if p.Err != nil || p.IssueErr != nil {
+		return errors.Join(p.Err, p.IssueErr)
+	}
+	issue := p.Issues[number]
+	if issue == nil || issue.Repo != repo {
+		return &github.HTTPError{Status: 404}
+	}
+	issue.Comments = append(issue.Comments, body)
+
+	return nil
+}
+
 func (p *Project) RemoveSubIssue(_ context.Context, repo string, parent int, child int64) error {
 	if p.Err != nil || p.IssueErr != nil {
 		return errors.Join(p.Err, p.IssueErr)

internal/github/client.go

@@ -557,6 +557,12 @@ func (c *Client) UpdateIssue(ctx context.Context, repo string, number int, field
 	return err
 }
 
+func (c *Client) CommentIssue(ctx context.Context, repo string, number int, body string) error {
+	_, err := c.do(ctx, http.MethodPost, fmt.Sprintf("%s/repos/%s/issues/%d/comments", c.BaseURL, repo, number), map[string]string{"body": body}, nil)
+
+	return err
+}
+
 // AddSubIssue makes the issue child a sub-issue of parent, moving it from any other parent.
 func (c *Client) AddSubIssue(ctx context.Context, repo string, parent int, child int64) error {
 	_, err := c.do(ctx, http.MethodPost, fmt.Sprintf("%s/repos/%s/issues/%d/sub_issues", c.BaseURL, repo, parent), map[string]any{"sub_issue_id": child, "replace_parent": true}, nil)

internal/github/client_test.go

@@ -212,6 +212,9 @@ func TestIssues(t *testing.T) {
 			fmt.Fprint(w, `{"id":42,"number":7}`)
 		case "PATCH /repos/acme/private-issues/issues/7":
 			assert.Equal(t, map[string]any{"state": "closed", "state_reason": "completed"}, body)
+		case "POST /repos/acme/private-issues/issues/7/comments":
+			assert.Equal(t, map[string]any{"body": "c"}, body)
+			w.WriteHeader(http.StatusCreated)
 		case "DELETE /repos/acme/private-issues/issues/3/sub_issue":
 			assert.Equal(t, map[string]any{"sub_issue_id": float64(42)}, body)
 		case "POST /repos/acme/private-issues/issues/3/sub_issues":
@@ -240,6 +243,7 @@ func TestIssues(t *testing.T) {
 	require.Equal(t, int64(42), issue.ID)
 	require.Equal(t, 7, issue.Number)
 	require.NoError(t, c.UpdateIssue(context.Background(), "acme/private-issues", 7, map[string]string{"state": "closed", "state_reason": "completed"}))
+	require.NoError(t, c.CommentIssue(context.Background(), "acme/private-issues", 7, "c"))
 	require.NoError(t, c.AddSubIssue(context.Background(), "acme/private-issues", 3, 42))
 	require.NoError(t, c.RemoveSubIssue(context.Background(), "acme/private-issues", 3, 42))
 	item, err := c.AddProjectIssue(context.Background(), "php", 16, 42)