From c1d51c386d1d373d711d2f185687386f21f84cc1 Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 11 Oct 2026 23:08:09 +0200 Subject: [PATCH] fix(pr): refuse pr comment and pr attachment on issues Gitea serves pull request comments and attachments through issue endpoints, so tea pr comment create 1 posted on issue #1. pr comment and pr attachment commands now check that the number is a pull request, and comment IDs that the comment is on one, and point at the issue command otherwise: "#1 in alice/project is an issue, not a pull request; use tea issue comment create 1". Issues gain a kind field (issue or pull) in JSON. Signed-off-by: bircni --- internal/app/comments.go | 39 ++++++++++++++++++- internal/app/comments_test.go | 53 ++++++++++++++++++++++++++ internal/app/issue_attachments.go | 27 ++++++++++++- internal/app/issue_attachments_test.go | 6 +++ internal/app/issues.go | 2 +- internal/app/pulls.go | 2 +- internal/services/comments.go | 4 +- internal/services/issues.go | 25 ++++++++++-- internal/services/issues_test.go | 2 +- 9 files changed, 150 insertions(+), 10 deletions(-) diff --git a/internal/app/comments.go b/internal/app/comments.go index b8c37b22..44df1b4b 100644 --- a/internal/app/comments.go +++ b/internal/app/comments.go @@ -19,7 +19,25 @@ func commentRecord(item services.Comment) terminal.Record { return terminal.Record{Heading: terminal.Row(strconv.FormatInt(item.ID, 10), item.Author), Lines: []string{item.Body, item.URL}} } -func (a *application) commentCommand() *cobra.Command { +// requirePull fails unless issue n is a pull request, for pr commands that Gitea serves through +// issue endpoints and would otherwise apply to the issue with that number. +func requirePull(cmd *cobra.Command, t target, n int64) error { + item, err := (services.Issues{Client: t.client, Repository: t.Repo}).View(cmd.Context(), n) + if err != nil { + return err + } + if item.Kind != services.KindPull { + return fmt.Errorf("#%d in %s is an issue, not a pull request; use %s %d", n, t.Repo, issueCommandPath(cmd), n) + } + return nil +} + +// issueCommandPath names the issue command that matches a pr command, such as "tea issue comment". +func issueCommandPath(cmd *cobra.Command) string { + return strings.Replace(cmd.CommandPath(), " pr ", " issue ", 1) +} + +func (a *application) commentCommand(kind refKind) *cobra.Command { group := &cobra.Command{Use: "comment", Short: "Manage discussion comments"} var limit, page int list := &cobra.Command{Use: "list ", Short: "List comments on an issue or pull request", Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { @@ -31,6 +49,11 @@ func (a *application) commentCommand() *cobra.Command { if err != nil { return err } + if kind == pullRef { + if err := requirePull(cmd, target, n); err != nil { + return err + } + } items, err := (services.Comments{Client: target.client, Repository: target.Repo}).Page(cmd.Context(), n, limit, page) if err != nil { return err @@ -77,6 +100,20 @@ func (a *application) commentCommand() *cobra.Command { return err } service := services.Comments{Client: target.client, Repository: target.Repo} + if kind == pullRef && verb == "create" { + if err := requirePull(cmd, target, id); err != nil { + return err + } + } + if kind == pullRef && verb != "create" { + current, err := service.View(cmd.Context(), id) + if err != nil { + return err + } + if !current.Pull { + return fmt.Errorf("comment %d in %s is on an issue, not a pull request; use %s %d", id, target.Repo, issueCommandPath(cmd), id) + } + } if verb == "create" || verb == "edit" { if useEditor { initial := "" diff --git a/internal/app/comments_test.go b/internal/app/comments_test.go index c764475d..72844480 100644 --- a/internal/app/comments_test.go +++ b/internal/app/comments_test.go @@ -32,3 +32,56 @@ func TestCommentConfirmationNamesTheTarget(t *testing.T) { require.NoError(t, Execute(context.Background(), deps, []string{"issue", "comment", "delete", "12", "-R", "alice/project"})) require.Equal(t, []string{"Comment on alice/project#3", "Delete comment 12 on alice/project"}, prompt.confirmations) } + +func TestPullCommentsAndAttachmentsRequirePullRequests(t *testing.T) { + var changed []string + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method + " " + r.URL.Path { + case "GET /api/v1/repos/alice/project/issues/1": + _, _ = w.Write([]byte(`{"number":1,"title":"An issue"}`)) + case "GET /api/v1/repos/alice/project/issues/13": + _, _ = w.Write([]byte(`{"number":13,"title":"A pull request","pull_request":{"merged":false}}`)) + case "GET /api/v1/repos/alice/project/issues/comments/5": + _, _ = w.Write([]byte(`{"id":5,"body":"on an issue","issue_url":"http://x/alice/project/issues/1"}`)) + case "GET /api/v1/repos/alice/project/issues/comments/6": + _, _ = w.Write([]byte(`{"id":6,"body":"on a pull request","pull_request_url":"http://x/alice/project/pulls/13"}`)) + case "GET /api/v1/repos/alice/project/issues/13/comments", "GET /api/v1/repos/alice/project/issues/13/assets": + _, _ = w.Write([]byte(`[]`)) + default: + changed = append(changed, r.Method+" "+r.URL.Path) + _, _ = w.Write([]byte(`{"id":7,"body":"x"}`)) + } + })) + defer server.Close() + deps, out, _ := serverDeps(t, server.URL) + for _, test := range []struct { + args []string + want string + }{ + {[]string{"pr", "comment", "create", "1", "--body", "x"}, "#1 in alice/project is an issue, not a pull request; use tea issue comment create 1"}, + {[]string{"pr", "comment", "list", "1"}, "#1 in alice/project is an issue"}, + {[]string{"pr", "comment", "edit", "5", "--body", "x"}, "comment 5 in alice/project is on an issue, not a pull request; use tea issue comment edit 5"}, + {[]string{"pr", "comment", "delete", "5", "--yes"}, "comment 5 in alice/project is on an issue"}, + {[]string{"pr", "attachment", "list", "1"}, "#1 in alice/project is an issue, not a pull request; use tea issue attachment list 1"}, + {[]string{"pr", "attachment", "delete", "1", "2", "--yes"}, "#1 in alice/project is an issue"}, + } { + err := Execute(context.Background(), deps, append(test.args, "-R", "alice/project")) + require.ErrorContains(t, err, test.want) + } + require.Empty(t, changed) + require.Empty(t, out.String()) + for _, args := range [][]string{ + {"pr", "comment", "create", "13", "--body", "x"}, + {"pr", "comment", "edit", "6", "--body", "x"}, + {"pr", "comment", "list", "13"}, + {"pr", "attachment", "list", "13"}, + {"issue", "comment", "create", "1", "--body", "x"}, + } { + require.NoError(t, Execute(context.Background(), deps, append(args, "-R", "alice/project", "--json"))) + } + require.Equal(t, []string{ + "POST /api/v1/repos/alice/project/issues/13/comments", + "PATCH /api/v1/repos/alice/project/issues/comments/6", + "POST /api/v1/repos/alice/project/issues/1/comments", + }, changed) +} diff --git a/internal/app/issue_attachments.go b/internal/app/issue_attachments.go index 9fe48bb8..4184562a 100644 --- a/internal/app/issue_attachments.go +++ b/internal/app/issue_attachments.go @@ -16,7 +16,7 @@ import ( "github.com/spf13/cobra" ) -func (a *application) issueAttachmentCommand() *cobra.Command { +func (a *application) issueAttachmentCommand(kind refKind) *cobra.Command { group := &cobra.Command{Use: "attachment", Short: "Inspect issue or pull request attachments"} var limit, page int list := &cobra.Command{Use: "list ", Short: "List attachments", Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { @@ -28,6 +28,11 @@ func (a *application) issueAttachmentCommand() *cobra.Command { if err != nil { return err } + if kind == pullRef { + if err := requirePull(cmd, target, n); err != nil { + return err + } + } items, err := (services.Issues{Client: target.client, Repository: target.Repo}).Attachments(cmd.Context(), n, limit, page) if err != nil { return err @@ -48,6 +53,11 @@ func (a *application) issueAttachmentCommand() *cobra.Command { if err != nil { return err } + if kind == pullRef { + if err := requirePull(cmd, target, n); err != nil { + return err + } + } item, err := (services.Issues{Client: target.client, Repository: target.Repo}).Attachment(cmd.Context(), n, id) if err != nil { return err @@ -72,6 +82,11 @@ func (a *application) issueAttachmentCommand() *cobra.Command { if err != nil { return err } + if kind == pullRef { + if err := requirePull(cmd, target, n); err != nil { + return err + } + } service := services.Issues{Client: target.client, Repository: target.Repo} if verb == "delete" { if err := a.confirm(cmd, fmt.Sprintf("Delete attachment %d on %s#%d", id, target.Repo, n)); err != nil { @@ -115,6 +130,11 @@ func (a *application) issueAttachmentCommand() *cobra.Command { if err != nil { return err } + if kind == pullRef { + if err := requirePull(cmd, target, n); err != nil { + return err + } + } item, err := (services.Issues{Client: target.client, Repository: target.Repo}).UploadAttachment(cmd.Context(), n, name, files[0]) if err != nil { return err @@ -139,6 +159,11 @@ func (a *application) issueAttachmentCommand() *cobra.Command { if err != nil { return err } + if kind == pullRef { + if err := requirePull(cmd, target, n); err != nil { + return err + } + } body, err := (services.Issues{Client: target.client, Repository: target.Repo}).DownloadAttachment(cmd.Context(), n, id) if err != nil { return err diff --git a/internal/app/issue_attachments_test.go b/internal/app/issue_attachments_test.go index cc5399e9..c1ff740c 100644 --- a/internal/app/issue_attachments_test.go +++ b/internal/app/issue_attachments_test.go @@ -21,6 +21,8 @@ func TestAttachmentCommandsShareIssueAssetsAndPagination(t *testing.T) { _, _ = w.Write([]byte(`[{"id":1,"name":"one"},{"id":2,"name":"two"}]`)) case "/api/v1/repos/alice/project/issues/1/assets/1": _, _ = w.Write([]byte(`{"id":1,"name":"one"}`)) + case "/api/v1/repos/alice/project/issues/1": + _, _ = w.Write([]byte(`{"number":1,"pull_request":{}}`)) default: t.Fatalf("unexpected path %s", r.URL.Path) } @@ -43,6 +45,10 @@ func TestAttachmentRenameAndConfirmedDeletion(t *testing.T) { for _, group := range []string{"issue", "pr"} { mutations := 0 server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/issues/1" { + _, _ = w.Write([]byte(`{"number":1,"pull_request":{}}`)) + return + } require.Equal(t, "/api/v1/repos/alice/project/issues/1/assets/1", r.URL.Path) mutations++ if r.Method == http.MethodPatch { diff --git a/internal/app/issues.go b/internal/app/issues.go index ab9504ae..7984163f 100644 --- a/internal/app/issues.go +++ b/internal/app/issues.go @@ -445,6 +445,6 @@ func (a *application) issueCommand() *cobra.Command { }, issueRow) }}) } - group.AddCommand(a.issueAttachmentCommand(), a.commentCommand(), a.issueEditCommand()) + group.AddCommand(a.issueAttachmentCommand(issueRef), a.commentCommand(issueRef), a.issueEditCommand()) return group } diff --git a/internal/app/pulls.go b/internal/app/pulls.go index 69a83bb1..d6b331b2 100644 --- a/internal/app/pulls.go +++ b/internal/app/pulls.go @@ -34,7 +34,7 @@ $ tea pr merge --squash --delete-branch` group.AddCommand(a.pullListCommand(), a.pullViewCommand()) a.addPullMutations(group) group.AddCommand(a.pullEditCommand(), a.pullReadyCommand()) - group.AddCommand(a.issueAttachmentCommand(), a.commentCommand()) + group.AddCommand(a.issueAttachmentCommand(pullRef), a.commentCommand(pullRef)) return group } diff --git a/internal/services/comments.go b/internal/services/comments.go index f7339ef1..03f80ef6 100644 --- a/internal/services/comments.go +++ b/internal/services/comments.go @@ -18,12 +18,14 @@ type Comment struct { Author string `json:"author"` CreatedAt string `json:"created_at"` UpdatedAt string `json:"updated_at"` + // Pull reports a comment on a pull request rather than an issue. + Pull bool `json:"-"` } func commentFrom(comment *gitea.Comment) Comment { return Comment{ ID: comment.ID, Body: comment.Body, URL: comment.HTMLURL, Author: login(comment.Poster), - CreatedAt: timestamp(comment.Created), UpdatedAt: timestamp(comment.Updated), + CreatedAt: timestamp(comment.Created), UpdatedAt: timestamp(comment.Updated), Pull: comment.PRURL != "", } } diff --git a/internal/services/issues.go b/internal/services/issues.go index 51126068..f5769567 100644 --- a/internal/services/issues.go +++ b/internal/services/issues.go @@ -20,6 +20,7 @@ type Issue struct { Ref string `json:"ref"` ID int64 `json:"id"` Number int64 `json:"number"` + Kind string `json:"kind"` Title string `json:"title"` Body string `json:"body"` URL string `json:"url"` @@ -32,6 +33,22 @@ type Issue struct { Comments int64 `json:"comments"` } +// Kinds of Issue: Gitea lists pull requests among issues. +const ( + KindIssue = "issue" + KindPull = "pull" +) + +// discussionFrom converts an issue or a pull request listed among issues. +func discussionFrom(issue *gitea.Issue) Issue { + item := issueFrom(issue) + item.Kind = KindIssue + if issue.PullRequest != nil { + item.Kind = KindPull + } + return item +} + // IssueMilestone identifies the milestone associated with an issue or pull request. type IssueMilestone struct { ID int64 `json:"id"` @@ -99,7 +116,7 @@ func (s Issues) List(ctx context.Context, opts ListOptions) ([]Issue, bool, erro if err != nil { return nil, false, err } - return convertAll(issues, issueFrom), more, nil + return convertAll(issues, discussionFrom), more, nil } // filterValues splits a comma-separated filter, which the SDK joins again. @@ -112,14 +129,14 @@ func filterValues(value string) []string { // View retrieves one issue. func (s Issues) View(ctx context.Context, number int64) (Issue, error) { - return repoConvert(ctx, s.Client, s.Repository, issueFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Issue, *gitea.Response, error) { + return repoConvert(ctx, s.Client, s.Repository, discussionFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Issue, *gitea.Response, error) { return c.Issues.GetIssue(ctx, owner, repo, number) }) } // Create opens an issue. func (s Issues) Create(ctx context.Context, options IssueOptions) (Issue, error) { - return repoConvert(ctx, s.Client, s.Repository, issueFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Issue, *gitea.Response, error) { + return repoConvert(ctx, s.Client, s.Repository, discussionFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Issue, *gitea.Response, error) { return c.Issues.CreateIssue(ctx, owner, repo, gitea.CreateIssueOption{ Title: options.Title, Body: options.Body, Ref: options.Ref, Assignees: options.Assignees, Deadline: options.Deadline, Milestone: options.Milestone, Labels: options.Labels, @@ -134,7 +151,7 @@ func (s Issues) SetState(ctx context.Context, number int64, state string) (Issue } func (s Issues) edit(ctx context.Context, number int64, opts gitea.EditIssueOption) (Issue, error) { - return repoConvert(ctx, s.Client, s.Repository, issueFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Issue, *gitea.Response, error) { + return repoConvert(ctx, s.Client, s.Repository, discussionFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Issue, *gitea.Response, error) { return c.Issues.EditIssue(ctx, owner, repo, number, opts) }) } diff --git a/internal/services/issues_test.go b/internal/services/issues_test.go index 34e50ddf..94b5b550 100644 --- a/internal/services/issues_test.go +++ b/internal/services/issues_test.go @@ -27,7 +27,7 @@ func TestIssueListSendsTypePageSizeAndFilters(t *testing.T) { items, more, err := service.List(context.Background(), ListOptions{State: "open", Limit: 1, Page: 3}) require.NoError(t, err) require.False(t, more) - require.Equal(t, []Issue{{Author: "bob", ID: 1, Number: 2, Title: "Bug", Labels: []Label{}, Assignees: []string{}}}, items) + require.Equal(t, []Issue{{Author: "bob", ID: 1, Number: 2, Kind: KindIssue, Title: "Bug", Labels: []Label{}, Assignees: []string{}}}, items) since := time.Date(2026, 10, 1, 8, 0, 0, 0, time.UTC) _, _, err = service.List(context.Background(), ListOptions{ Kind: "all", Limit: 30, Page: 1, Search: "crash", Assignee: "carol", Labels: "bug,ui", Milestones: "v1",