fix: address verified review findings (issues-1)

Signed-off-by: bircni <bircni@icloud.com>

internal/services/issue_attachments.go

@@ -13,18 +13,21 @@ import (
 )
 
 // Attachments paginates the server's unpaginated attachment collection locally.
-func (s Issues) Attachments(ctx context.Context, number int64, limit, page int) ([]IssueAttachment, error) {
+func (s Issues) Attachments(ctx context.Context, number int64, limit, page int) (Page[IssueAttachment], error) {
 	attachments, err := repoCall(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, repo string) ([]*gitea.Attachment, *gitea.Response, error) {
 		return c.Issues.ListIssueAttachments(ctx, owner, repo, number)
 	})
 	if err != nil {
-		return nil, err
+		return Page[IssueAttachment]{}, err
 	}
 	items, err := pageOf(attachments, limit, page)
 	if err != nil {
-		return nil, err
+		return Page[IssueAttachment]{}, err
 	}
-	return convertAll(items, func(item *gitea.Attachment) IssueAttachment { return IssueAttachment(assetFrom(item)) }), nil
+	return Page[IssueAttachment]{
+		Items: convertAll(items, func(item *gitea.Attachment) IssueAttachment { return IssueAttachment(assetFrom(item)) }),
+		More:  len(attachments) > page*limit, Total: len(attachments), Counted: true,
+	}, nil
 }
 
 // Attachment retrieves metadata for one attachment in its owning issue.
@@ -73,13 +76,15 @@ func (s Issues) UploadAttachment(ctx context.Context, number int64, name string,
 	return IssueAttachment(asset), err
 }
 
-// DownloadAttachment opens an attachment stream scoped to its instance and issue, and returns the
-// attachment's metadata.
-func (s Issues) DownloadAttachment(ctx context.Context, number, id int64) (IssueAttachment, io.ReadCloser, error) {
+// DownloadAttachment retrieves an attachment's metadata and returns a function that opens its
+// content, scoped to its instance and issue. Nothing is downloaded until that function is called,
+// so callers can refuse or skip the download first.
+func (s Issues) DownloadAttachment(ctx context.Context, number, id int64) (IssueAttachment, func() (io.ReadCloser, error), error) {
 	attachment, err := s.attachment(ctx, number, id)
 	if err != nil {
 		return IssueAttachment{}, nil, err
 	}
-	body, err := downloadAttachment(ctx, s.Client, attachment)
-	return IssueAttachment(assetFrom(attachment)), body, err
+	return IssueAttachment(assetFrom(attachment)), func() (io.ReadCloser, error) {
+		return downloadAttachment(ctx, s.Client, attachment)
+	}, nil
 }

internal/services/issue_labels_test.go

@@ -106,3 +106,31 @@ func assertIssueEdit(t *testing.T, want gitea.EditIssueOption, body []byte) {
 		assert.Equal(t, want, got)
 	}
 }
+
+func TestEditAddLabelReplacesTheExclusiveLabelOfTheSameScope(t *testing.T) {
+	client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		body, _ := io.ReadAll(r.Body)
+		switch r.Method + " " + r.URL.Path {
+		case "GET /api/v1/repos/alice/project/issues/7":
+			_, _ = w.Write([]byte(`{"number":7,"labels":[{"id":1,"name":"bug"},{"id":2,"name":"Priority/Low","exclusive":true}]}`))
+		case "GET /api/v1/repos/alice/project/labels":
+			if r.URL.Query().Get("page") != "1" {
+				_, _ = w.Write([]byte(`[]`))
+				return
+			}
+			_, _ = w.Write([]byte(`[{"id":1,"name":"bug"},{"id":2,"name":"Priority/Low","exclusive":true},{"id":3,"name":"Priority/High","exclusive":true},{"id":4,"name":"Kind/x","exclusive":true}]`))
+		case "GET /api/v1/orgs/alice":
+			w.WriteHeader(http.StatusNotFound)
+		case "PUT /api/v1/repos/alice/project/issues/7/labels":
+			assert.JSONEq(t, `{"labels":[1,3]}`, string(body))
+			_, _ = w.Write([]byte(`[]`))
+		case "GET /api/v1/repos/alice/project/pulls/7":
+			_, _ = w.Write([]byte(`{"number":7}`))
+		default:
+			t.Errorf("unexpected request %s %s", r.Method, r.URL.Path)
+			w.WriteHeader(http.StatusTeapot)
+		}
+	}))
+	_, err := (Issues{Client: client, Repository: "alice/project"}).Edit(context.Background(), 7, IssueEdit{Changes: &MetadataChanges{AddLabels: []int64{3}}})
+	require.NoError(t, err)
+}

internal/services/issues.go

@@ -164,7 +164,8 @@ func (s Issues) Create(ctx context.Context, options IssueOptions) (Issue, error)
 			Deadline: options.Deadline, Milestone: options.Milestone, Labels: options.Labels,
 		})
 	})
-	return item, Classify(err, "issue", "", s.Repository)
+	// A 404 here can only mean the repository is missing or invisible.
+	return item, Classify(err, "repository", s.Repository, "")
 }
 
 // SetState updates only the selected issue state.

internal/services/issues_test.go

@@ -107,3 +107,12 @@ func TestIssueTemplatesAcceptCheckboxOptionObjects(t *testing.T) {
 	var sdkTemplates []*gitea.IssueTemplate
 	require.Error(t, json.Unmarshal([]byte(body), &sdkTemplates), "remove the raw request once the SDK decodes checkbox options")
 }
+
+func TestIssueCreateInMissingRepositoryReportsTheRepository(t *testing.T) {
+	client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusNotFound)
+		_, _ = w.Write([]byte(`{"message":"not found"}`))
+	}))
+	_, err := (Issues{Client: client, Repository: "alice/projetc"}).Create(context.Background(), IssueOptions{Title: "x"})
+	require.EqualError(t, err, "repository alice/projetc not found")
+}

internal/services/metadata.go

@@ -388,8 +388,12 @@ func (s Issues) applyChanges(ctx context.Context, n int64, opts IssueEdit) (Issu
 		return opts, err
 	}
 	if len(changes.AddLabels)+len(changes.RemoveLabels) > 0 {
-		ids := make([]int64, len(current.Labels))
-		for i, label := range current.Labels {
+		kept, err := s.withoutReplacedExclusive(ctx, current.Labels, changes.AddLabels)
+		if err != nil {
+			return opts, err
+		}
+		ids := make([]int64, len(kept))
+		for i, label := range kept {
 			ids[i] = label.ID
 		}
 		ids = mergeUnique(ids, changes.AddLabels, changes.RemoveLabels, func(a, b int64) bool { return a == b })
@@ -402,6 +406,44 @@ func (s Issues) applyChanges(ctx context.Context, n int64, opts IssueEdit) (Issu
 	return opts, nil
 }
 
+// withoutReplacedExclusive drops the current labels that an added label of the same exclusive
+// scope replaces. Gitea keeps one label per scope but picks it by name, not by recency, so
+// sending both would keep whichever sorts last.
+func (s Issues) withoutReplacedExclusive(ctx context.Context, current []Label, add []int64) ([]Label, error) {
+	if len(add) == 0 || !slices.ContainsFunc(current, func(l Label) bool { return l.Exclusive }) {
+		return current, nil
+	}
+	all, err := (Labels{Client: s.Client, Repository: s.Repository}).All(ctx)
+	if err != nil {
+		return nil, err
+	}
+	owner, _, _ := strings.Cut(s.Repository, "/")
+	if _, isOrg, err := organization(ctx, s.Client, owner); err != nil {
+		return nil, err
+	} else if isOrg {
+		shared, err := (Labels{Client: s.Client, Organization: owner}).All(ctx)
+		if err != nil {
+			return nil, err
+		}
+		all = append(all, shared...)
+	}
+	replaced := map[string]bool{}
+	for _, label := range all {
+		if scope := exclusiveScope(label); scope != "" && slices.Contains(add, label.ID) {
+			replaced[scope] = true
+		}
+	}
+	return slices.DeleteFunc(slices.Clone(current), func(l Label) bool { return replaced[exclusiveScope(l)] }), nil
+}
+
+// exclusiveScope is the name of an exclusive label up to its last slash, or "" for any other label.
+func exclusiveScope(label Label) string {
+	if i := strings.LastIndex(label.Name, "/"); label.Exclusive && i > 0 {
+		return label.Name[:i]
+	}
+	return ""
+}
+
 // mergeUnique applies additions and removals to current values, preserving order.
 func mergeUnique[T any](current, add, remove []T, equal func(T, T) bool) []T {
 	result := []T{}