diff --git a/README.md b/README.md index e5e4318..af25329 100644 --- a/README.md +++ b/README.md @@ -432,6 +432,16 @@ gitlink-cli pr +reviews --owner Gitlink --repo forgeplus -i 42 # Create a PR review (with dry-run preview) gitlink-cli pr +review --owner Gitlink --repo forgeplus -i 42 --status approved -c "LGTM" --dry-run gitlink-cli pr +review --owner Gitlink --repo forgeplus -i 42 --status approved -c "LGTM" + +# List review comments and unresolved discussion threads +gitlink-cli pr +review-comments --owner Gitlink --repo forgeplus -i 42 --state opened --need-respond true --full + +# Create a line-level review comment or reply +gitlink-cli pr +review-comment --owner Gitlink --repo forgeplus -i 42 -b "Please handle this edge case" --type problem --review-id 7 --line-code abc_1_2 --commit deadbeef --path main.go --dry-run + +# Resolve, edit, or delete a review comment +gitlink-cli pr +review-comment-update --owner Gitlink --repo forgeplus -i 42 --comment-id 99 --state resolved +gitlink-cli pr +review-comment-delete --owner Gitlink --repo forgeplus -i 42 --comment-id 99 ``` ### Branch Management diff --git a/README.zh-CN.md b/README.zh-CN.md index 6a8879d..3a199c3 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -442,6 +442,16 @@ gitlink-cli pr +reviews --owner Gitlink --repo forgeplus -i 42 # 创建 PR 审查（支持 dry-run 预览） gitlink-cli pr +review --owner Gitlink --repo forgeplus -i 42 --status approved -c "LGTM" --dry-run gitlink-cli pr +review --owner Gitlink --repo forgeplus -i 42 --status approved -c "LGTM" + +# 查看行级审查评论和未解决讨论 +gitlink-cli pr +review-comments --owner Gitlink --repo forgeplus -i 42 --state opened --need-respond true --full + +# 创建行级审查评论或回复 +gitlink-cli pr +review-comment --owner Gitlink --repo forgeplus -i 42 -b "请处理这个边界情况" --type problem --review-id 7 --line-code abc_1_2 --commit deadbeef --path main.go --dry-run + +# 解决、编辑或删除审查评论 +gitlink-cli pr +review-comment-update --owner Gitlink --repo forgeplus -i 42 --comment-id 99 --state resolved +gitlink-cli pr +review-comment-delete --owner Gitlink --repo forgeplus -i 42 --comment-id 99 ``` ### 发布管理 diff --git a/doc/changes/pr-review-comment-management.md b/doc/changes/pr-review-comment-management.md new file mode 100644 index 0000000..b4f504a --- /dev/null +++ b/doc/changes/pr-review-comment-management.md @@ -0,0 +1,25 @@ +# Pull request review comment management shortcuts + +This change adds first-class shortcuts for GitLink pull request review comment +threads. It complements `pr +review`, which creates an overall review decision, +with commands for line-level and threaded discussion records. + +- `pr +review-comments` lists review comments with keyword, review ID, + need-response, state, parent, path, full-thread, and sorting filters. +- `pr +review-comment` creates review comments or replies and supports + `comment`/`problem` types, review IDs, line codes, commit IDs, paths, + parent IDs, raw diff JSON, and dry-run previews. +- `pr +review-comment-update` edits the note, commit, or state (`opened`, + `resolved`, `disabled`) with dry-run support. +- `pr +review-comment-delete` deletes a review comment by ID. + +The implementation validates numeric IDs, boolean filters, comment type, state, +and diff JSON before sending API requests. + +Verification: + +- `go test ./shortcuts/pr` +- `go test ./shortcuts` +- `go test ./...` +- `go build ./...` +- `git diff --check` diff --git a/shortcuts/pr/pr.go b/shortcuts/pr/pr.go index 03f537f..9e893af 100644 --- a/shortcuts/pr/pr.go +++ b/shortcuts/pr/pr.go @@ -1,8 +1,10 @@ package pr import ( + "encoding/json" "fmt" "net/url" + "strconv" "strings" "github.com/gitlink-org/gitlink-cli/internal/i18n" @@ -431,6 +433,62 @@ func Shortcuts(translators ...*i18n.Translator) []*common.Shortcut { return ctx.Output(env) }, }, + { + Name: "review-comments", + Description: "List pull request review comments and discussion threads", + Flags: []common.Flag{ + {Name: "id", Short: "i", Usage: tr.T("flag.pr.id"), Required: true}, + {Name: "keyword", Short: "k", Usage: "Search review comment content"}, + {Name: "review-id", Usage: "Filter by review ID"}, + {Name: "need-respond", Usage: "Filter comments that need response: true or false"}, + {Name: "state", Short: "s", Usage: "Filter by opened, resolved, or disabled"}, + {Name: "parent-id", Usage: "Filter replies under a parent comment ID"}, + {Name: "path", Usage: "Filter by file path"}, + {Name: "full", Usage: "Include replies in the response", Bool: true, Default: "false"}, + {Name: "sort-by", Usage: "Sort field: created_on or updated_on"}, + {Name: "sort-direction", Usage: "Sort direction: asc or desc"}, + }, + Run: runPRReviewComments, + }, + { + Name: "review-comment", + Description: "Create a pull request review comment or reply", + Flags: []common.Flag{ + {Name: "id", Short: "i", Usage: tr.T("flag.pr.id"), Required: true}, + {Name: "body", Short: "b", Usage: tr.T("flag.comment.body"), Required: true}, + {Name: "type", Usage: "Comment type: comment or problem", Default: "comment"}, + {Name: "review-id", Usage: "Review ID"}, + {Name: "line-code", Usage: "GitLink diff line code"}, + {Name: "commit", Usage: "Commit SHA for the commented diff"}, + {Name: "path", Usage: "Commented file path"}, + {Name: "parent-id", Usage: "Parent review comment ID for a reply"}, + {Name: "diff-json", Usage: "Raw diff JSON object for line comments"}, + {Name: "dry-run", Usage: tr.T("flag.dry_run"), Bool: true, Default: "false"}, + }, + Run: runPRReviewCommentCreate, + }, + { + Name: "review-comment-update", + Description: "Update a pull request review comment note, commit, or state", + Flags: []common.Flag{ + {Name: "id", Short: "i", Usage: tr.T("flag.pr.id"), Required: true}, + {Name: "comment-id", Usage: "Review comment ID", Required: true}, + {Name: "body", Short: "b", Usage: tr.T("flag.comment.body")}, + {Name: "commit", Usage: "Commit SHA"}, + {Name: "state", Short: "s", Usage: "New state: opened, resolved, or disabled"}, + {Name: "dry-run", Usage: tr.T("flag.dry_run"), Bool: true, Default: "false"}, + }, + Run: runPRReviewCommentUpdate, + }, + { + Name: "review-comment-delete", + Description: "Delete a pull request review comment", + Flags: []common.Flag{ + {Name: "id", Short: "i", Usage: tr.T("flag.pr.id"), Required: true}, + {Name: "comment-id", Usage: "Review comment ID", Required: true}, + }, + Run: runPRReviewCommentDelete, + }, } } @@ -445,6 +503,265 @@ func prV1Path(ctx *common.RuntimeContext, id string) string { return fmt.Sprintf("/v1/%s/%s/pulls/%s", ctx.Owner, ctx.Repo, id) } +func prReviewCommentPath(ctx *common.RuntimeContext, prID string) string { + return fmt.Sprintf("%s/journals", prV1Path(ctx, url.PathEscape(prID))) +} + +func prReviewCommentItemPath(ctx *common.RuntimeContext, prID, commentID string) string { + return fmt.Sprintf("%s/%s", prReviewCommentPath(ctx, prID), url.PathEscape(commentID)) +} + +func runPRReviewComments(ctx *common.RuntimeContext) error { + if err := ctx.ResolveOwnerRepo(); err != nil { + return err + } + id, err := ctx.RequireArg("id") + if err != nil { + return err + } + q := url.Values{} + setPRQueryIfPresent(q, "keyword", ctx.Arg("keyword")) + if reviewID := ctx.Arg("review-id"); reviewID != "" { + if _, err := parsePRPositiveID(reviewID, "review-id"); err != nil { + return err + } + q.Set("review_id", strings.TrimSpace(reviewID)) + } + if needRespond := ctx.Arg("need-respond"); needRespond != "" { + if err := validatePRBoolString("need-respond", needRespond); err != nil { + return err + } + q.Set("need_respond", strings.ToLower(strings.TrimSpace(needRespond))) + } + if state := ctx.Arg("state"); state != "" { + if err := validatePRReviewCommentState(state); err != nil { + return err + } + q.Set("state", strings.TrimSpace(state)) + } + if parentID := ctx.Arg("parent-id"); parentID != "" { + if _, err := parsePRPositiveID(parentID, "parent-id"); err != nil { + return err + } + q.Set("parent_id", strings.TrimSpace(parentID)) + } + setPRQueryIfPresent(q, "path", ctx.Arg("path")) + if ctx.Arg("full") == "true" { + q.Set("is_full", "true") + } + setPRQueryIfPresent(q, "sort_by", ctx.Arg("sort-by")) + setPRQueryIfPresent(q, "sort_direction", ctx.Arg("sort-direction")) + env, err := ctx.CallAPIWithQuery("GET", prReviewCommentPath(ctx, id), q) + if err != nil { + return err + } + return ctx.Output(env) +} + +func runPRReviewCommentCreate(ctx *common.RuntimeContext) error { + if err := ctx.ResolveOwnerRepo(); err != nil { + return err + } + id, err := ctx.RequireArg("id") + if err != nil { + return err + } + body, err := ctx.RequireArg("body") + if err != nil { + return err + } + payload, err := prReviewCommentCreatePayload(ctx, body) + if err != nil { + return err + } + if ctx.Arg("dry-run") == "true" { + return ctx.OutputData(map[string]interface{}{ + "repository": fmt.Sprintf("%s/%s", ctx.Owner, ctx.Repo), + "pull_request": id, + "dry_run": true, + "action": "create_review_comment", + "payload": payload, + }) + } + env, err := ctx.CallAPI("POST", prReviewCommentPath(ctx, id), payload) + if err != nil { + return err + } + return ctx.Output(env) +} + +func runPRReviewCommentUpdate(ctx *common.RuntimeContext) error { + if err := ctx.ResolveOwnerRepo(); err != nil { + return err + } + prID, commentID, err := prReviewCommentTarget(ctx) + if err != nil { + return err + } + payload, err := prReviewCommentUpdatePayload(ctx) + if err != nil { + return err + } + if ctx.Arg("dry-run") == "true" { + return ctx.OutputData(map[string]interface{}{ + "repository": fmt.Sprintf("%s/%s", ctx.Owner, ctx.Repo), + "pull_request": prID, + "review_comment": commentID, + "dry_run": true, + "action": "update_review_comment", + "payload": payload, + }) + } + env, err := ctx.CallAPI("PUT", prReviewCommentItemPath(ctx, prID, commentID), payload) + if err != nil { + return err + } + return ctx.Output(env) +} + +func runPRReviewCommentDelete(ctx *common.RuntimeContext) error { + if err := ctx.ResolveOwnerRepo(); err != nil { + return err + } + prID, commentID, err := prReviewCommentTarget(ctx) + if err != nil { + return err + } + env, err := ctx.CallAPI("DELETE", prReviewCommentItemPath(ctx, prID, commentID), nil) + if err != nil { + return err + } + return ctx.Output(env) +} + +func prReviewCommentCreatePayload(ctx *common.RuntimeContext, body string) (map[string]interface{}, error) { + commentType := firstPRNonEmpty(ctx.Arg("type"), "comment") + if err := validatePRReviewCommentType(commentType); err != nil { + return nil, err + } + payload := map[string]interface{}{ + "type": commentType, + "note": body, + } + for _, field := range []struct { + flag string + key string + }{ + {"review-id", "review_id"}, + {"parent-id", "parent_id"}, + } { + if value := ctx.Arg(field.flag); value != "" { + id, err := parsePRPositiveID(value, field.flag) + if err != nil { + return nil, err + } + payload[field.key] = id + } + } + setPayloadStringIfPresent(payload, "line_code", ctx.Arg("line-code")) + setPayloadStringIfPresent(payload, "commit_id", ctx.Arg("commit")) + setPayloadStringIfPresent(payload, "path", ctx.Arg("path")) + if rawDiff := strings.TrimSpace(ctx.Arg("diff-json")); rawDiff != "" { + var diff map[string]interface{} + if err := json.Unmarshal([]byte(rawDiff), &diff); err != nil { + return nil, fmt.Errorf("invalid --diff-json: %w", err) + } + payload["diff"] = diff + } + return payload, nil +} + +func prReviewCommentUpdatePayload(ctx *common.RuntimeContext) (map[string]interface{}, error) { + payload := map[string]interface{}{} + setPayloadStringIfPresent(payload, "note", ctx.Arg("body")) + setPayloadStringIfPresent(payload, "commit_id", ctx.Arg("commit")) + if state := ctx.Arg("state"); state != "" { + if err := validatePRReviewCommentState(state); err != nil { + return nil, err + } + payload["state"] = strings.TrimSpace(state) + } + if len(payload) == 0 { + return nil, fmt.Errorf("at least one of --body, --commit, or --state is required") + } + return payload, nil +} + +func prReviewCommentTarget(ctx *common.RuntimeContext) (string, string, error) { + prID, err := ctx.RequireArg("id") + if err != nil { + return "", "", err + } + commentID, err := ctx.RequireArg("comment-id") + if err != nil { + return "", "", err + } + if _, err := parsePRPositiveID(commentID, "comment-id"); err != nil { + return "", "", err + } + return strings.TrimSpace(prID), strings.TrimSpace(commentID), nil +} + +func validatePRReviewCommentType(value string) error { + switch strings.TrimSpace(value) { + case "comment", "problem": + return nil + default: + return fmt.Errorf("invalid --type value %q: use comment or problem", value) + } +} + +func validatePRReviewCommentState(value string) error { + switch strings.TrimSpace(value) { + case "opened", "resolved", "disabled": + return nil + default: + return fmt.Errorf("invalid --state value %q: use opened, resolved, or disabled", value) + } +} + +func validatePRBoolString(flagName, value string) error { + switch strings.ToLower(strings.TrimSpace(value)) { + case "true", "false": + return nil + default: + return fmt.Errorf("invalid --%s value %q: use true or false", flagName, value) + } +} + +func parsePRPositiveID(value, flagName string) (int, error) { + trimmed := strings.TrimSpace(value) + if trimmed == "" { + return 0, fmt.Errorf("--%s contains an empty ID", flagName) + } + id, err := strconv.Atoi(trimmed) + if err != nil || id <= 0 { + return 0, fmt.Errorf("--%s must be a positive numeric ID", flagName) + } + return id, nil +} + +func setPRQueryIfPresent(q url.Values, key, value string) { + if strings.TrimSpace(value) != "" { + q.Set(key, strings.TrimSpace(value)) + } +} + +func setPayloadStringIfPresent(payload map[string]interface{}, key, value string) { + if strings.TrimSpace(value) != "" { + payload[key] = strings.TrimSpace(value) + } +} + +func firstPRNonEmpty(values ...string) string { + for _, value := range values { + if strings.TrimSpace(value) != "" { + return strings.TrimSpace(value) + } + } + return "" +} + func validatePRReviewStatus(status string) error { switch status { case "common", "approved", "rejected": diff --git a/shortcuts/pr/pr_test.go b/shortcuts/pr/pr_test.go index eece6d9..fd172f9 100644 --- a/shortcuts/pr/pr_test.go +++ b/shortcuts/pr/pr_test.go @@ -92,6 +92,155 @@ func TestPRCommentFailsWhenIssueFieldMissing(t *testing.T) { } } +func TestPRReviewCommentsListSendsFilters(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "GET" || r.URL.Path != "/v1/owner/repo/pulls/13/journals.json" { + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + q := r.URL.Query() + assertEqual(t, q.Get("keyword"), "race") + assertEqual(t, q.Get("review_id"), "5") + assertEqual(t, q.Get("need_respond"), "true") + assertEqual(t, q.Get("state"), "opened") + assertEqual(t, q.Get("parent_id"), "7") + assertEqual(t, q.Get("path"), "main.go") + assertEqual(t, q.Get("is_full"), "true") + assertEqual(t, q.Get("sort_by"), "updated_on") + assertEqual(t, q.Get("sort_direction"), "desc") + writeJSON(t, w, map[string]interface{}{ + "total_count": float64(1), + "journals": []interface{}{ + map[string]interface{}{"id": float64(9), "note": "race"}, + }, + }) + })) + defer server.Close() + + err := runPRShortcut(t, server, "review-comments", map[string]string{ + "id": "13", + "keyword": "race", + "review-id": "5", + "need-respond": "true", + "state": "opened", + "parent-id": "7", + "path": "main.go", + "full": "true", + "sort-by": "updated_on", + "sort-direction": "desc", + }) + if err != nil { + t.Fatalf("review-comments failed: %v", err) + } +} + +func TestPRReviewCommentCreateSendsPayload(t *testing.T) { + var payload map[string]interface{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "POST" || r.URL.Path != "/v1/owner/repo/pulls/13/journals.json" { + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + payload = decodeJSON(t, r) + writeJSON(t, w, payload) + })) + defer server.Close() + + err := runPRShortcut(t, server, "review-comment", map[string]string{ + "id": "13", + "body": "Please handle this edge case", + "type": "problem", + "review-id": "5", + "line-code": "abc_1_2", + "commit": "deadbeef", + "path": "main.go", + "parent-id": "7", + "diff-json": `{"name":"main.go","addition":1}`, + }) + if err != nil { + t.Fatalf("review-comment failed: %v", err) + } + assertEqual(t, payload["type"], "problem") + assertEqual(t, payload["note"], "Please handle this edge case") + assertEqual(t, payload["review_id"], float64(5)) + assertEqual(t, payload["line_code"], "abc_1_2") + assertEqual(t, payload["commit_id"], "deadbeef") + assertEqual(t, payload["path"], "main.go") + assertEqual(t, payload["parent_id"], float64(7)) + diff, ok := payload["diff"].(map[string]interface{}) + if !ok { + t.Fatalf("diff = %v, want object", payload["diff"]) + } + assertEqual(t, diff["name"], "main.go") + assertEqual(t, diff["addition"], float64(1)) +} + +func TestPRReviewCommentUpdateSendsPayload(t *testing.T) { + var payload map[string]interface{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "PUT" || r.URL.Path != "/v1/owner/repo/pulls/13/journals/9.json" { + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + payload = decodeJSON(t, r) + writeJSON(t, w, payload) + })) + defer server.Close() + + err := runPRShortcut(t, server, "review-comment-update", map[string]string{ + "id": "13", + "comment-id": "9", + "body": "Resolved after follow-up", + "commit": "cafebabe", + "state": "resolved", + }) + if err != nil { + t.Fatalf("review-comment-update failed: %v", err) + } + assertEqual(t, payload["note"], "Resolved after follow-up") + assertEqual(t, payload["commit_id"], "cafebabe") + assertEqual(t, payload["state"], "resolved") +} + +func TestPRReviewCommentDelete(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "DELETE" || r.URL.Path != "/v1/owner/repo/pulls/13/journals/9.json" { + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + writeJSON(t, w, map[string]interface{}{"status": float64(0), "message": "success"}) + })) + defer server.Close() + + err := runPRShortcut(t, server, "review-comment-delete", map[string]string{"id": "13", "comment-id": "9"}) + if err != nil { + t.Fatalf("review-comment-delete failed: %v", err) + } +} + +func TestPRReviewCommentRejectsInvalidArgs(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Fatalf("invalid args should not call API, got %s %s", r.Method, r.URL.Path) + })) + defer server.Close() + + cases := []struct { + name string + cmd string + args map[string]string + }{ + {name: "bad list state", cmd: "review-comments", args: map[string]string{"id": "13", "state": "done"}}, + {name: "bad bool", cmd: "review-comments", args: map[string]string{"id": "13", "need-respond": "maybe"}}, + {name: "bad type", cmd: "review-comment", args: map[string]string{"id": "13", "body": "x", "type": "note"}}, + {name: "bad diff json", cmd: "review-comment", args: map[string]string{"id": "13", "body": "x", "diff-json": "{"}}, + {name: "missing update fields", cmd: "review-comment-update", args: map[string]string{"id": "13", "comment-id": "9"}}, + {name: "bad comment id", cmd: "review-comment-delete", args: map[string]string{"id": "13", "comment-id": "abc"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if err := runPRShortcut(t, server, tc.cmd, tc.args); err == nil { + t.Fatal("expected validation error") + } + }) + } +} + // --- list --- func TestPRList(t *testing.T) {