Port the CLI changes that landed in the rickub monorepo since the splitUnverified
The standalone repo was cut from rickub/rickub at 4a9633d (2026-08-23). Six commits touched cli/ there afterwards; this brings them over with a three-way merge against that ancestor, adapted to what diverged here (host-bound config, --draft/pr ready, label group, clean() output filtering, handled Flush errors): - 7c1e879 feat(review): line-anchored MR comments — pr comment --file/--line/--side/--intent/--commit/--reply-to, pr line-comments, pr resolve [--reopen]; internal/api/inline.go. - 8f56224 feat(review): comment deletion and ONE --body parser — cmd/body.go (resolveBody/requireBody: '-' reads stdin, blank bodies refused), pr delete-comment, issue delete-comment, comment ids and [edited]/[deleted] marks in views. - b62120e feat(api): pr edit -b replaces a merge request's description. - a108664 fix(cli): trim CRLF from stdin bodies; hardened comment tests. - 757d65a fix(ci): cache list / cache delete <key> [--scope] for poisoned CI cache entries. - a901eae fix(cli): token file 0600 on rewrite — already guaranteed here by the atomic 0600 save; its tests are ported and pass against it. Verified live against rickub.com: cache list, comment ids in issue view, a CRLF heredoc comment posted clean and retracted with delete-comment. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FgkcT3UPxwxukBKojB5eg4
8aaf8d0 parent: 59285c9 modified
README.md +24 -2 | @@ -135,6 +135,9 @@ hosts: | ||
| 135 | 135 | ## Command reference |
| 136 | 136 | |
| 137 | 137 | Every command supports `--help`, and `--json` for raw JSON output instead of a table. |
| 138 | +Every `--body`/`-b` takes literal text or `-` to read the body from stdin (a | |
| 139 | +heredoc works; trailing newlines, CRLF included, are trimmed). A body that is | |
| 140 | +empty after resolution is refused where one is required, never posted. | |
| 138 | 141 | |
| 139 | 142 | ``` |
| 140 | 143 | rickub auth login|status|logout |
| @@ -159,7 +162,22 @@ rickub pr create [-R owner/repo] --base B --head H --title T [-b body] [--draft] | ||
| 159 | 162 | rickub pr ready <number> [-R owner/repo] [--undo] # draft → ready for review (or back) |
| 160 | 163 | rickub pr merge <number> [-R owner/repo] [--method merge|squash|ff-only] |
| 161 | 164 | rickub pr close <number> [-R owner/repo] |
| 165 | +rickub pr reopen <number> [-R owner/repo] | |
| 166 | +rickub pr edit <number> [-R owner/repo] -b "text" | -b - | |
| 167 | + # replace the description (author or repo writer) | |
| 162 | 168 | rickub pr comment <number> [-R owner/repo] -b "text" |
| 169 | + # whole-MR conversation comment (any reader) | |
| 170 | +rickub pr comment <number> [-R owner/repo] -b "text" --file PATH --line N | |
| 171 | + [--side new|old] [--intent info|request_change|kudos] [--commit SHA] | |
| 172 | + # line-anchored review comment on the diff (needs WRITE access) | |
| 173 | +rickub pr comment <number> [-R owner/repo] -b "text" --reply-to <comment-id> | |
| 174 | + # reply in an existing line-comment thread | |
| 175 | +rickub pr line-comments <number> [-R owner/repo] [--page N] [--per-page N] | |
| 176 | + # list the MR's line comments with their ids, anchors and state | |
| 177 | +rickub pr resolve <number> <comment-id> [-R owner/repo] [--reopen] | |
| 178 | +rickub pr delete-comment <number> <comment-id> [-R owner/repo] | |
| 179 | + # retract a conversation comment (soft delete: a tombstone stays); | |
| 180 | + # ids are shown by `rickub pr view <number>` | |
| 163 | 181 | |
| 164 | 182 | rickub run list [-R owner/repo] |
| 165 | 183 | rickub run view <number> [-R owner/repo] |
| @@ -176,6 +194,7 @@ rickub issue create [-R owner/repo] -t "title" [-b "body" | -b - < file] | ||
| 176 | 194 | rickub issue close <number> [-R owner/repo] |
| 177 | 195 | rickub issue reopen <number> [-R owner/repo] |
| 178 | 196 | rickub issue comment <number> [-R owner/repo] -b "text" |
| 197 | +rickub issue delete-comment <number> <comment-id> [-R owner/repo] | |
| 179 | 198 | rickub issue label <number> [-R owner/repo] --labels "bug,help wanted" | --clear |
| 180 | 199 | rickub issue milestone <number> [-R owner/repo] --milestone "v1.0" | --clear |
| 181 | 200 | rickub issue assign <number> [-R owner/repo] --user H [--remove] |
| @@ -187,6 +206,9 @@ rickub label edit <name> [-R owner/repo] [--name new] [--color hex] | ||
| 187 | 206 | rickub label delete <name> [-R owner/repo] |
| 188 | 207 | rickub label palette |
| 189 | 208 | |
| 209 | +rickub cache list [-R owner/repo] [--page N] [--per-page N] # CI dependency-cache entries | |
| 210 | +rickub cache delete <key> [-R owner/repo] [--scope refs/heads/main] # purge a poisoned key (repo admin) | |
| 211 | + | |
| 190 | 212 | rickub milestone list [-R owner/repo] [--state open|closed|all] |
| 191 | 213 | rickub milestone create [-R owner/repo] -t "v1.0" [-d desc] [--due YYYY-MM-DD] |
| 192 | 214 | rickub milestone close <id> [-R owner/repo] |
| @@ -215,7 +237,7 @@ open ready for review; pass `--draft` to open a draft, and `pr ready` flips it. | ||
| 215 | 237 | |
| 216 | 238 | ### Repo selector |
| 217 | 239 | |
| 218 | -`pr`, `run`, `issue`, `label`, and `milestone` subcommands take `-R/--repo owner/repo`. When omitted, the repo is | |
| 240 | +`pr`, `run`, `issue`, `label`, `milestone`, and `cache` subcommands take `-R/--repo owner/repo`. When omitted, the repo is | |
| 219 | 241 | inferred from the current directory's git `origin` remote (any of `https://`, |
| 220 | 242 | `ssh://`, or `git@host:owner/repo` forms). |
| 221 | 243 | |
| @@ -266,7 +288,7 @@ Layout: | ||
| 266 | 288 | |
| 267 | 289 | ``` |
| 268 | 290 | main.go # entrypoint; maps API errors to exit codes |
| 269 | -cmd/ # Cobra command tree (auth, repo, pr, issue, label, milestone, run, org, search, api, browse) | |
| 291 | +cmd/ # Cobra command tree (auth, repo, pr, issue, label, milestone, run, cache, org, search, api, browse) | |
| 270 | 292 | internal/api/ # typed HTTP client for the rickub JSON API |
| 271 | 293 | internal/config/ # config load/save + host/token resolution and binding |
| 272 | 294 | ``` |
| @@ -135,6 +135,9 @@ hosts: | |||
| 135 | ## Command reference | 135 | ## Command reference |
| 136 | 136 | ||
| 137 | Every command supports `--help`, and `--json` for raw JSON output instead of a table. | 137 | Every command supports `--help`, and `--json` for raw JSON output instead of a table. |
| 138 | +Every `--body`/`-b` takes literal text or `-` to read the body from stdin (a | ||
| 139 | +heredoc works; trailing newlines, CRLF included, are trimmed). A body that is | ||
| 140 | +empty after resolution is refused where one is required, never posted. | ||
| 138 | 141 | ||
| 139 | ``` | 142 | ``` |
| 140 | rickub auth login|status|logout | 143 | rickub auth login|status|logout |
| @@ -159,7 +162,22 @@ rickub pr create [-R owner/repo] --base B --head H --title T [-b body] [--draft] | |||
| 159 | rickub pr ready <number> [-R owner/repo] [--undo] # draft → ready for review (or back) | 162 | rickub pr ready <number> [-R owner/repo] [--undo] # draft → ready for review (or back) |
| 160 | rickub pr merge <number> [-R owner/repo] [--method merge|squash|ff-only] | 163 | rickub pr merge <number> [-R owner/repo] [--method merge|squash|ff-only] |
| 161 | rickub pr close <number> [-R owner/repo] | 164 | rickub pr close <number> [-R owner/repo] |
| 165 | +rickub pr reopen <number> [-R owner/repo] | ||
| 166 | +rickub pr edit <number> [-R owner/repo] -b "text" | -b - | ||
| 167 | + # replace the description (author or repo writer) | ||
| 162 | rickub pr comment <number> [-R owner/repo] -b "text" | 168 | rickub pr comment <number> [-R owner/repo] -b "text" |
| 169 | + # whole-MR conversation comment (any reader) | ||
| 170 | +rickub pr comment <number> [-R owner/repo] -b "text" --file PATH --line N | ||
| 171 | + [--side new|old] [--intent info|request_change|kudos] [--commit SHA] | ||
| 172 | + # line-anchored review comment on the diff (needs WRITE access) | ||
| 173 | +rickub pr comment <number> [-R owner/repo] -b "text" --reply-to <comment-id> | ||
| 174 | + # reply in an existing line-comment thread | ||
| 175 | +rickub pr line-comments <number> [-R owner/repo] [--page N] [--per-page N] | ||
| 176 | + # list the MR's line comments with their ids, anchors and state | ||
| 177 | +rickub pr resolve <number> <comment-id> [-R owner/repo] [--reopen] | ||
| 178 | +rickub pr delete-comment <number> <comment-id> [-R owner/repo] | ||
| 179 | + # retract a conversation comment (soft delete: a tombstone stays); | ||
| 180 | + # ids are shown by `rickub pr view <number>` | ||
| 163 | 181 | ||
| 164 | rickub run list [-R owner/repo] | 182 | rickub run list [-R owner/repo] |
| 165 | rickub run view <number> [-R owner/repo] | 183 | rickub run view <number> [-R owner/repo] |
| @@ -176,6 +194,7 @@ rickub issue create [-R owner/repo] -t "title" [-b "body" | -b - < file] | |||
| 176 | rickub issue close <number> [-R owner/repo] | 194 | rickub issue close <number> [-R owner/repo] |
| 177 | rickub issue reopen <number> [-R owner/repo] | 195 | rickub issue reopen <number> [-R owner/repo] |
| 178 | rickub issue comment <number> [-R owner/repo] -b "text" | 196 | rickub issue comment <number> [-R owner/repo] -b "text" |
| 197 | +rickub issue delete-comment <number> <comment-id> [-R owner/repo] | ||
| 179 | rickub issue label <number> [-R owner/repo] --labels "bug,help wanted" | --clear | 198 | rickub issue label <number> [-R owner/repo] --labels "bug,help wanted" | --clear |
| 180 | rickub issue milestone <number> [-R owner/repo] --milestone "v1.0" | --clear | 199 | rickub issue milestone <number> [-R owner/repo] --milestone "v1.0" | --clear |
| 181 | rickub issue assign <number> [-R owner/repo] --user H [--remove] | 200 | rickub issue assign <number> [-R owner/repo] --user H [--remove] |
| @@ -187,6 +206,9 @@ rickub label edit <name> [-R owner/repo] [--name new] [--color hex] | |||
| 187 | rickub label delete <name> [-R owner/repo] | 206 | rickub label delete <name> [-R owner/repo] |
| 188 | rickub label palette | 207 | rickub label palette |
| 189 | 208 | ||
| 209 | +rickub cache list [-R owner/repo] [--page N] [--per-page N] # CI dependency-cache entries | ||
| 210 | +rickub cache delete <key> [-R owner/repo] [--scope refs/heads/main] # purge a poisoned key (repo admin) | ||
| 211 | + | ||
| 190 | rickub milestone list [-R owner/repo] [--state open|closed|all] | 212 | rickub milestone list [-R owner/repo] [--state open|closed|all] |
| 191 | rickub milestone create [-R owner/repo] -t "v1.0" [-d desc] [--due YYYY-MM-DD] | 213 | rickub milestone create [-R owner/repo] -t "v1.0" [-d desc] [--due YYYY-MM-DD] |
| 192 | rickub milestone close <id> [-R owner/repo] | 214 | rickub milestone close <id> [-R owner/repo] |
| @@ -215,7 +237,7 @@ open ready for review; pass `--draft` to open a draft, and `pr ready` flips it. | |||
| 215 | 237 | ||
| 216 | ### Repo selector | 238 | ### Repo selector |
| 217 | 239 | ||
| 218 | -`pr`, `run`, `issue`, `label`, and `milestone` subcommands take `-R/--repo owner/repo`. When omitted, the repo is | 240 | +`pr`, `run`, `issue`, `label`, `milestone`, and `cache` subcommands take `-R/--repo owner/repo`. When omitted, the repo is |
| 219 | inferred from the current directory's git `origin` remote (any of `https://`, | 241 | inferred from the current directory's git `origin` remote (any of `https://`, |
| 220 | `ssh://`, or `git@host:owner/repo` forms). | 242 | `ssh://`, or `git@host:owner/repo` forms). |
| 221 | 243 | ||
| @@ -266,7 +288,7 @@ Layout: | |||
| 266 | 288 | ||
| 267 | ``` | 289 | ``` |
| 268 | main.go # entrypoint; maps API errors to exit codes | 290 | main.go # entrypoint; maps API errors to exit codes |
| 269 | -cmd/ # Cobra command tree (auth, repo, pr, issue, label, milestone, run, org, search, api, browse) | 291 | +cmd/ # Cobra command tree (auth, repo, pr, issue, label, milestone, run, cache, org, search, api, browse) |
| 270 | internal/api/ # typed HTTP client for the rickub JSON API | 292 | internal/api/ # typed HTTP client for the rickub JSON API |
| 271 | internal/config/ # config load/save + host/token resolution and binding | 293 | internal/config/ # config load/save + host/token resolution and binding |
| 272 | ``` | 294 | ``` |
added
cmd/body.go +68 -0 | new file mode 100644 | ||
| @@ -0,0 +1,68 @@ | ||
| 1 | +package cmd | |
| 2 | + | |
| 3 | +import ( | |
| 4 | + "fmt" | |
| 5 | + "io" | |
| 6 | + "strings" | |
| 7 | + | |
| 8 | + "github.com/spf13/cobra" | |
| 9 | +) | |
| 10 | + | |
| 11 | +// --- --body resolution: ONE parser for every command that takes one ----------- | |
| 12 | +// | |
| 13 | +// This file exists because there were two. | |
| 14 | +// | |
| 15 | +// `issue comment` resolved `-b -` from stdin; `mr comment` used its flag value | |
| 16 | +// verbatim while accepting the same `-` without complaint. So | |
| 17 | +// `rickub mr comment 11 -b - <<'EOF' … EOF` posted the literal one-character body | |
| 18 | +// "-", threw the heredoc away, and printed "Commented on #11" — a success line | |
| 19 | +// indistinguishable from a real post. Five substantial code-review comments were | |
| 20 | +// destroyed that way on MR #11 (issue #94), and because comment deletion was also | |
| 21 | +// browser-only at the time, they could not even be cleaned up. | |
| 22 | +// | |
| 23 | +// The root cause was not the missing stdin branch, it was the second parser. Per | |
| 24 | +// CLAUDE.md's DRY ladder this is now rung 1: one definition, called by every | |
| 25 | +// command with a --body, and a fourth command cannot be added with a third | |
| 26 | +// interpretation of "-". | |
| 27 | + | |
| 28 | +// bodyFlagHelp is the --body flag's help text. Shared so every command documents | |
| 29 | +// the stdin behaviour in the same words — issue #94's acceptance criterion 4. | |
| 30 | +const bodyFlagHelp = "body text ('-' reads stdin)" | |
| 31 | + | |
| 32 | +// resolveBody resolves a --body flag value: "-" means "read stdin to EOF", | |
| 33 | +// anything else is the literal value. Trailing newlines are trimmed so a heredoc | |
| 34 | +// does not append blank lines to the rendered markdown — both "\n" and the "\r\n" | |
| 35 | +// a Windows heredoc produces, so a cross-platform body never carries a stray | |
| 36 | +// trailing carriage return into the posted markdown. | |
| 37 | +// | |
| 38 | +// It does NOT enforce non-emptiness: some bodies are genuinely optional (an | |
| 39 | +// issue's description). Callers whose body is required use requireBody. | |
| 40 | +func resolveBody(cmd *cobra.Command, body string) (string, error) { | |
| 41 | + if body != "-" { | |
| 42 | + return body, nil | |
| 43 | + } | |
| 44 | + b, err := io.ReadAll(cmd.InOrStdin()) | |
| 45 | + if err != nil { | |
| 46 | + return "", fmt.Errorf("read body from stdin: %w", err) | |
| 47 | + } | |
| 48 | + return strings.TrimRight(string(b), "\r\n"), nil | |
| 49 | +} | |
| 50 | + | |
| 51 | +// requireBody is resolveBody for a body that must not be empty. A blank result — | |
| 52 | +// no --body at all, an empty string, whitespace only, or an empty stdin — is an | |
| 53 | +// ERROR, so the command exits non-zero and nothing is sent. That is the guard | |
| 54 | +// that makes issue #94's failure mode impossible to repeat: whatever goes wrong | |
| 55 | +// upstream, the user sees a refusal instead of a success line over a lost body. | |
| 56 | +func requireBody(cmd *cobra.Command, body string) (string, error) { | |
| 57 | + resolved, err := resolveBody(cmd, body) | |
| 58 | + if err != nil { | |
| 59 | + return "", err | |
| 60 | + } | |
| 61 | + if strings.TrimSpace(resolved) == "" { | |
| 62 | + if body == "-" { | |
| 63 | + return "", fmt.Errorf("--body - read nothing from stdin: refusing to post an empty comment") | |
| 64 | + } | |
| 65 | + return "", fmt.Errorf("--body is required and must not be blank (use -b - to read it from stdin)") | |
| 66 | + } | |
| 67 | + return resolved, nil | |
| 68 | +} | |
| new file mode 100644 | |||
| @@ -0,0 +1,68 @@ | |||
| 1 | +package cmd | ||
| 2 | + | ||
| 3 | +import ( | ||
| 4 | + "fmt" | ||
| 5 | + "io" | ||
| 6 | + "strings" | ||
| 7 | + | ||
| 8 | + "github.com/spf13/cobra" | ||
| 9 | +) | ||
| 10 | + | ||
| 11 | +// --- --body resolution: ONE parser for every command that takes one ----------- | ||
| 12 | +// | ||
| 13 | +// This file exists because there were two. | ||
| 14 | +// | ||
| 15 | +// `issue comment` resolved `-b -` from stdin; `mr comment` used its flag value | ||
| 16 | +// verbatim while accepting the same `-` without complaint. So | ||
| 17 | +// `rickub mr comment 11 -b - <<'EOF' … EOF` posted the literal one-character body | ||
| 18 | +// "-", threw the heredoc away, and printed "Commented on #11" — a success line | ||
| 19 | +// indistinguishable from a real post. Five substantial code-review comments were | ||
| 20 | +// destroyed that way on MR #11 (issue #94), and because comment deletion was also | ||
| 21 | +// browser-only at the time, they could not even be cleaned up. | ||
| 22 | +// | ||
| 23 | +// The root cause was not the missing stdin branch, it was the second parser. Per | ||
| 24 | +// CLAUDE.md's DRY ladder this is now rung 1: one definition, called by every | ||
| 25 | +// command with a --body, and a fourth command cannot be added with a third | ||
| 26 | +// interpretation of "-". | ||
| 27 | + | ||
| 28 | +// bodyFlagHelp is the --body flag's help text. Shared so every command documents | ||
| 29 | +// the stdin behaviour in the same words — issue #94's acceptance criterion 4. | ||
| 30 | +const bodyFlagHelp = "body text ('-' reads stdin)" | ||
| 31 | + | ||
| 32 | +// resolveBody resolves a --body flag value: "-" means "read stdin to EOF", | ||
| 33 | +// anything else is the literal value. Trailing newlines are trimmed so a heredoc | ||
| 34 | +// does not append blank lines to the rendered markdown — both "\n" and the "\r\n" | ||
| 35 | +// a Windows heredoc produces, so a cross-platform body never carries a stray | ||
| 36 | +// trailing carriage return into the posted markdown. | ||
| 37 | +// | ||
| 38 | +// It does NOT enforce non-emptiness: some bodies are genuinely optional (an | ||
| 39 | +// issue's description). Callers whose body is required use requireBody. | ||
| 40 | +func resolveBody(cmd *cobra.Command, body string) (string, error) { | ||
| 41 | + if body != "-" { | ||
| 42 | + return body, nil | ||
| 43 | + } | ||
| 44 | + b, err := io.ReadAll(cmd.InOrStdin()) | ||
| 45 | + if err != nil { | ||
| 46 | + return "", fmt.Errorf("read body from stdin: %w", err) | ||
| 47 | + } | ||
| 48 | + return strings.TrimRight(string(b), "\r\n"), nil | ||
| 49 | +} | ||
| 50 | + | ||
| 51 | +// requireBody is resolveBody for a body that must not be empty. A blank result — | ||
| 52 | +// no --body at all, an empty string, whitespace only, or an empty stdin — is an | ||
| 53 | +// ERROR, so the command exits non-zero and nothing is sent. That is the guard | ||
| 54 | +// that makes issue #94's failure mode impossible to repeat: whatever goes wrong | ||
| 55 | +// upstream, the user sees a refusal instead of a success line over a lost body. | ||
| 56 | +func requireBody(cmd *cobra.Command, body string) (string, error) { | ||
| 57 | + resolved, err := resolveBody(cmd, body) | ||
| 58 | + if err != nil { | ||
| 59 | + return "", err | ||
| 60 | + } | ||
| 61 | + if strings.TrimSpace(resolved) == "" { | ||
| 62 | + if body == "-" { | ||
| 63 | + return "", fmt.Errorf("--body - read nothing from stdin: refusing to post an empty comment") | ||
| 64 | + } | ||
| 65 | + return "", fmt.Errorf("--body is required and must not be blank (use -b - to read it from stdin)") | ||
| 66 | + } | ||
| 67 | + return resolved, nil | ||
| 68 | +} | ||
added
cmd/body_test.go +264 -0 | new file mode 100644 | ||
| @@ -0,0 +1,264 @@ | ||
| 1 | +package cmd | |
| 2 | + | |
| 3 | +import ( | |
| 4 | + "encoding/json" | |
| 5 | + "io" | |
| 6 | + "net/http" | |
| 7 | + "net/http/httptest" | |
| 8 | + "strings" | |
| 9 | + "testing" | |
| 10 | + | |
| 11 | + "github.com/spf13/cobra" | |
| 12 | +) | |
| 13 | + | |
| 14 | +// Regression tests for issue #94. | |
| 15 | +// | |
| 16 | +// `rickub mr comment 11 -b - <<'EOF' … EOF` used to post the LITERAL one-character | |
| 17 | +// body "-", discard the heredoc, and print "Commented on #11". Five substantial | |
| 18 | +// code-review comments were destroyed that way on MR #11. Each test below fails | |
| 19 | +// on the pre-fix code: the first because the body posted was "-", the second | |
| 20 | +// because an empty body was accepted and reported as success. | |
| 21 | + | |
| 22 | +// commentBodyServer records the body the CLI actually sent. It asserts the | |
| 23 | +// request is the POST a comment-create is (a mis-routed request must fail the | |
| 24 | +// test, not pass silently as sent=="") and surfaces a malformed JSON body via | |
| 25 | +// t.Errorf rather than swallowing it — the whole point of these tests is that a | |
| 26 | +// lost or mangled body is loud, so the harness that proves it must be loud too. | |
| 27 | +func commentBodyServer(t *testing.T, got *string) *httptest.Server { | |
| 28 | + t.Helper() | |
| 29 | + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 30 | + if r.Method != http.MethodPost { | |
| 31 | + t.Errorf("comment request used %s %s, want POST", r.Method, r.URL.Path) | |
| 32 | + } | |
| 33 | + var body struct { | |
| 34 | + Body string `json:"body"` | |
| 35 | + } | |
| 36 | + b, err := io.ReadAll(r.Body) | |
| 37 | + if err != nil { | |
| 38 | + t.Errorf("reading request body: %v", err) | |
| 39 | + } | |
| 40 | + if err := json.Unmarshal(b, &body); err != nil { | |
| 41 | + t.Errorf("request body %q is not the JSON the CLI should send: %v", b, err) | |
| 42 | + } | |
| 43 | + *got = body.Body | |
| 44 | + w.WriteHeader(http.StatusCreated) | |
| 45 | + _ = json.NewEncoder(w).Encode(map[string]any{ | |
| 46 | + "id": "c-1", "author": "rick", "body": body.Body, "created_at": "2026-08-26T10:00:00Z", | |
| 47 | + }) | |
| 48 | + })) | |
| 49 | +} | |
| 50 | + | |
| 51 | +func withBodyServer(t *testing.T, got *string) func() { | |
| 52 | + t.Helper() | |
| 53 | + srv := commentBodyServer(t, got) | |
| 54 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | |
| 55 | + t.Setenv("RICKUB_HOST", srv.URL) | |
| 56 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | |
| 57 | + return srv.Close | |
| 58 | +} | |
| 59 | + | |
| 60 | +// executeStdin is execute() with stdin wired to a string, for the `-b -` path. | |
| 61 | +func executeStdin(t *testing.T, stdin string, args ...string) (string, string, error) { | |
| 62 | + t.Helper() | |
| 63 | + rootCmd.SetIn(strings.NewReader(stdin)) | |
| 64 | + defer rootCmd.SetIn(nil) | |
| 65 | + return execute(t, args...) | |
| 66 | +} | |
| 67 | + | |
| 68 | +// TestMRCommentBodyDashReadsStdin: `-b -` sends what was piped in, byte for byte | |
| 69 | +// — not the literal "-". This is issue #94's acceptance criterion 1. | |
| 70 | +func TestMRCommentBodyDashReadsStdin(t *testing.T) { | |
| 71 | + var sent string | |
| 72 | + defer withBodyServer(t, &sent)() | |
| 73 | + | |
| 74 | + const review = "## Review\n\nLine 128 needs a nil guard.\n\n- point one\n- point two" | |
| 75 | + if _, _, err := executeStdin(t, review+"\n", "mr", "comment", "11", "-R", "rick/portal", "-b", "-"); err != nil { | |
| 76 | + t.Fatalf("execute: %v", err) | |
| 77 | + } | |
| 78 | + if sent == "-" { | |
| 79 | + t.Fatal(`the literal "-" was posted — issue #94 has regressed`) | |
| 80 | + } | |
| 81 | + if sent != review { | |
| 82 | + t.Fatalf("posted body = %q, want the stdin text %q", sent, review) | |
| 83 | + } | |
| 84 | + // The length check is the habit the incident taught: a success line proves | |
| 85 | + // nothing, the byte count does. | |
| 86 | + if len(sent) != len(review) { | |
| 87 | + t.Fatalf("posted %d bytes, piped %d", len(sent), len(review)) | |
| 88 | + } | |
| 89 | +} | |
| 90 | + | |
| 91 | +// TestMRCommentBodyStdinTrimsCRLF: a heredoc piped in with Windows CRLF line | |
| 92 | +// endings must not leave a stray trailing '\r' on the posted body. resolveBody | |
| 93 | +// trims "\r\n", not just "\n", so the body a cross-platform user pipes in is the | |
| 94 | +// body that lands — byte for byte, no carriage return smuggled onto the last | |
| 95 | +// line. (Athena review, MR !1: TrimRight "\n" left the '\r'.) | |
| 96 | +func TestMRCommentBodyStdinTrimsCRLF(t *testing.T) { | |
| 97 | + var sent string | |
| 98 | + defer withBodyServer(t, &sent)() | |
| 99 | + | |
| 100 | + // Internal CRLFs are preserved; only the trailing run is trimmed. | |
| 101 | + const want = "first line\r\nsecond line" | |
| 102 | + stdin := want + "\r\n" // the heredoc's own terminating CRLF | |
| 103 | + if _, _, err := executeStdin(t, stdin, "mr", "comment", "11", "-R", "rick/portal", "-b", "-"); err != nil { | |
| 104 | + t.Fatalf("execute: %v", err) | |
| 105 | + } | |
| 106 | + if strings.HasSuffix(sent, "\r") || strings.HasSuffix(sent, "\n") { | |
| 107 | + t.Fatalf("posted body still ends with a carriage return/newline: %q", sent) | |
| 108 | + } | |
| 109 | + if sent != want { | |
| 110 | + t.Fatalf("posted body = %q, want %q (only the trailing CRLF trimmed)", sent, want) | |
| 111 | + } | |
| 112 | +} | |
| 113 | + | |
| 114 | +// TestIssueCommentBodyDashReadsStdin: the sibling command keeps working through | |
| 115 | +// the now-shared helper. | |
| 116 | +func TestIssueCommentBodyDashReadsStdin(t *testing.T) { | |
| 117 | + var sent string | |
| 118 | + defer withBodyServer(t, &sent)() | |
| 119 | + | |
| 120 | + const text = "a multi-line\nissue comment" | |
| 121 | + if _, _, err := executeStdin(t, text+"\n", "issue", "comment", "7", "-R", "rick/portal", "-b", "-"); err != nil { | |
| 122 | + t.Fatalf("execute: %v", err) | |
| 123 | + } | |
| 124 | + if sent != text { | |
| 125 | + t.Fatalf("posted body = %q, want %q", sent, text) | |
| 126 | + } | |
| 127 | +} | |
| 128 | + | |
| 129 | +// TestCommentEmptyBodyIsRefused: an empty or blank resolved body exits non-zero | |
| 130 | +// and sends NOTHING, on both commands and both input paths. Issue #94's | |
| 131 | +// acceptance criterion 2 — this is what makes a lost body loud instead of silent. | |
| 132 | +func TestCommentEmptyBodyIsRefused(t *testing.T) { | |
| 133 | + cases := []struct { | |
| 134 | + name string | |
| 135 | + stdin string | |
| 136 | + args []string | |
| 137 | + }{ | |
| 138 | + {"mr, no --body", "", []string{"mr", "comment", "11", "-R", "rick/portal"}}, | |
| 139 | + {"mr, empty --body", "", []string{"mr", "comment", "11", "-R", "rick/portal", "-b", ""}}, | |
| 140 | + {"mr, whitespace --body", "", []string{"mr", "comment", "11", "-R", "rick/portal", "-b", " \n "}}, | |
| 141 | + {"mr, empty stdin", "", []string{"mr", "comment", "11", "-R", "rick/portal", "-b", "-"}}, | |
| 142 | + {"mr, blank stdin", "\n\n \n", []string{"mr", "comment", "11", "-R", "rick/portal", "-b", "-"}}, | |
| 143 | + {"issue, no --body", "", []string{"issue", "comment", "7", "-R", "rick/portal"}}, | |
| 144 | + {"issue, empty stdin", "", []string{"issue", "comment", "7", "-R", "rick/portal", "-b", "-"}}, | |
| 145 | + } | |
| 146 | + for _, tc := range cases { | |
| 147 | + t.Run(tc.name, func(t *testing.T) { | |
| 148 | + sent := "<nothing sent>" | |
| 149 | + defer withBodyServer(t, &sent)() | |
| 150 | + _, _, err := executeStdin(t, tc.stdin, tc.args...) | |
| 151 | + if err == nil { | |
| 152 | + t.Fatalf("%s was accepted; an empty body must exit non-zero", tc.name) | |
| 153 | + } | |
| 154 | + if sent != "<nothing sent>" { | |
| 155 | + t.Fatalf("%s reached the server with body %q; nothing must be sent", tc.name, sent) | |
| 156 | + } | |
| 157 | + }) | |
| 158 | + } | |
| 159 | +} | |
| 160 | + | |
| 161 | +// TestMRCommentAnchoredBodyAlsoResolvesStdin: the line-comment path shares the | |
| 162 | +// same resolution, so `--file … --line … -b -` is not a second trap. | |
| 163 | +func TestMRCommentAnchoredBodyAlsoResolvesStdin(t *testing.T) { | |
| 164 | + var seen captured | |
| 165 | + defer withServer(t, &seen)() | |
| 166 | + | |
| 167 | + const text = "guard the nil case here" | |
| 168 | + rootCmd.SetIn(strings.NewReader(text + "\n")) | |
| 169 | + defer rootCmd.SetIn(nil) | |
| 170 | + if _, _, err := execute(t, "mr", "comment", "11", "-R", "rick/portal", | |
| 171 | + "--file", "web/api.go", "--line", "128", "-b", "-"); err != nil { | |
| 172 | + t.Fatalf("execute: %v", err) | |
| 173 | + } | |
| 174 | + if seen.Body["body"] != text { | |
| 175 | + t.Fatalf("anchored body = %v, want %q", seen.Body["body"], text) | |
| 176 | + } | |
| 177 | +} | |
| 178 | + | |
| 179 | +// TestMRCommentAnchoredBlankBodyIsRefused closes issue #94's criterion 2 on the | |
| 180 | +// SECOND code path: a blank body on the anchored --file/--line route is refused | |
| 181 | +// non-zero and NOTHING reaches the server — the /line-comments endpoint is never | |
| 182 | +// called. TestCommentEmptyBodyIsRefused already proves this for the whole-MR | |
| 183 | +// conversation route; the anchored route shares resolveBody but takes a different | |
| 184 | +// branch of runPRComment, so it needs its own guard or a regression could slip | |
| 185 | +// through the path the empty-body test does not walk. | |
| 186 | +func TestMRCommentAnchoredBlankBodyIsRefused(t *testing.T) { | |
| 187 | + cases := []struct { | |
| 188 | + name string | |
| 189 | + stdin string | |
| 190 | + args []string | |
| 191 | + }{ | |
| 192 | + {"empty --body", "", []string{"mr", "comment", "11", "-R", "rick/portal", "--file", "web/api.go", "--line", "128", "-b", ""}}, | |
| 193 | + {"whitespace --body", "", []string{"mr", "comment", "11", "-R", "rick/portal", "--file", "web/api.go", "--line", "128", "-b", " \n "}}, | |
| 194 | + {"blank stdin", "\n \n", []string{"mr", "comment", "11", "-R", "rick/portal", "--file", "web/api.go", "--line", "128", "-b", "-"}}, | |
| 195 | + } | |
| 196 | + for _, tc := range cases { | |
| 197 | + t.Run(tc.name, func(t *testing.T) { | |
| 198 | + var seen captured | |
| 199 | + defer withServer(t, &seen)() | |
| 200 | + rootCmd.SetIn(strings.NewReader(tc.stdin)) | |
| 201 | + defer rootCmd.SetIn(nil) | |
| 202 | + if _, _, err := execute(t, tc.args...); err == nil { | |
| 203 | + t.Fatalf("%s was accepted; a blank anchored body must exit non-zero", tc.name) | |
| 204 | + } | |
| 205 | + if seen.Method != "" { | |
| 206 | + t.Fatalf("%s reached the server (%s %s); nothing must be sent", tc.name, seen.Method, seen.Path) | |
| 207 | + } | |
| 208 | + }) | |
| 209 | + } | |
| 210 | +} | |
| 211 | + | |
| 212 | +// TestBodyFlagHelpIsIdentical: both commands document the stdin behaviour in the | |
| 213 | +// same words, because they read the same constant. Issue #94's criterion 4. | |
| 214 | +func TestBodyFlagHelpIsIdentical(t *testing.T) { | |
| 215 | + find := func(path ...string) string { | |
| 216 | + c, _, err := rootCmd.Find(path) | |
| 217 | + if err != nil { | |
| 218 | + t.Fatalf("find %v: %v", path, err) | |
| 219 | + } | |
| 220 | + f := c.Flags().Lookup("body") | |
| 221 | + if f == nil { | |
| 222 | + t.Fatalf("%v has no --body flag", path) | |
| 223 | + } | |
| 224 | + return f.Usage | |
| 225 | + } | |
| 226 | + mr, issue := find("mr", "comment"), find("issue", "comment") | |
| 227 | + if !strings.Contains(mr, bodyFlagHelp) { | |
| 228 | + t.Errorf("mr comment --body help %q does not use the shared text %q", mr, bodyFlagHelp) | |
| 229 | + } | |
| 230 | + if !strings.Contains(issue, bodyFlagHelp) { | |
| 231 | + t.Errorf("issue comment --body help %q does not use the shared text %q", issue, bodyFlagHelp) | |
| 232 | + } | |
| 233 | + if mr != issue { | |
| 234 | + t.Errorf("the two --body help strings differ:\n mr: %q\n issue: %q", mr, issue) | |
| 235 | + } | |
| 236 | +} | |
| 237 | + | |
| 238 | +// TestEveryBodyFlagUsesTheSharedResolver is the structural guard behind issue | |
| 239 | +// #94: the bug was not a missing stdin branch, it was a SECOND parser. This walks | |
| 240 | +// every command in the tree, finds every `--body` flag, and asserts each one's | |
| 241 | +// help text is built from the shared constant — which is only true if it went | |
| 242 | +// through resolveBody/requireBody. A fifth command added with its own | |
| 243 | +// interpretation of "-" fails here, naming itself. | |
| 244 | +func TestEveryBodyFlagUsesTheSharedResolver(t *testing.T) { | |
| 245 | + var checked int | |
| 246 | + var walk func(c *cobra.Command) | |
| 247 | + walk = func(c *cobra.Command) { | |
| 248 | + if f := c.Flags().Lookup("body"); f != nil { | |
| 249 | + checked++ | |
| 250 | + if !strings.Contains(f.Usage, bodyFlagHelp) { | |
| 251 | + t.Errorf("`%s` has a --body flag whose help %q does not use bodyFlagHelp — "+ | |
| 252 | + "it is probably parsing --body itself; route it through resolveBody/requireBody "+ | |
| 253 | + "in cmd/body.go instead (issue #94)", c.CommandPath(), f.Usage) | |
| 254 | + } | |
| 255 | + } | |
| 256 | + for _, sub := range c.Commands() { | |
| 257 | + walk(sub) | |
| 258 | + } | |
| 259 | + } | |
| 260 | + walk(rootCmd) | |
| 261 | + if checked == 0 { | |
| 262 | + t.Fatal("found no --body flags at all — this test has stopped checking anything") | |
| 263 | + } | |
| 264 | +} | |
| new file mode 100644 | |||
| @@ -0,0 +1,264 @@ | |||
| 1 | +package cmd | ||
| 2 | + | ||
| 3 | +import ( | ||
| 4 | + "encoding/json" | ||
| 5 | + "io" | ||
| 6 | + "net/http" | ||
| 7 | + "net/http/httptest" | ||
| 8 | + "strings" | ||
| 9 | + "testing" | ||
| 10 | + | ||
| 11 | + "github.com/spf13/cobra" | ||
| 12 | +) | ||
| 13 | + | ||
| 14 | +// Regression tests for issue #94. | ||
| 15 | +// | ||
| 16 | +// `rickub mr comment 11 -b - <<'EOF' … EOF` used to post the LITERAL one-character | ||
| 17 | +// body "-", discard the heredoc, and print "Commented on #11". Five substantial | ||
| 18 | +// code-review comments were destroyed that way on MR #11. Each test below fails | ||
| 19 | +// on the pre-fix code: the first because the body posted was "-", the second | ||
| 20 | +// because an empty body was accepted and reported as success. | ||
| 21 | + | ||
| 22 | +// commentBodyServer records the body the CLI actually sent. It asserts the | ||
| 23 | +// request is the POST a comment-create is (a mis-routed request must fail the | ||
| 24 | +// test, not pass silently as sent=="") and surfaces a malformed JSON body via | ||
| 25 | +// t.Errorf rather than swallowing it — the whole point of these tests is that a | ||
| 26 | +// lost or mangled body is loud, so the harness that proves it must be loud too. | ||
| 27 | +func commentBodyServer(t *testing.T, got *string) *httptest.Server { | ||
| 28 | + t.Helper() | ||
| 29 | + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 30 | + if r.Method != http.MethodPost { | ||
| 31 | + t.Errorf("comment request used %s %s, want POST", r.Method, r.URL.Path) | ||
| 32 | + } | ||
| 33 | + var body struct { | ||
| 34 | + Body string `json:"body"` | ||
| 35 | + } | ||
| 36 | + b, err := io.ReadAll(r.Body) | ||
| 37 | + if err != nil { | ||
| 38 | + t.Errorf("reading request body: %v", err) | ||
| 39 | + } | ||
| 40 | + if err := json.Unmarshal(b, &body); err != nil { | ||
| 41 | + t.Errorf("request body %q is not the JSON the CLI should send: %v", b, err) | ||
| 42 | + } | ||
| 43 | + *got = body.Body | ||
| 44 | + w.WriteHeader(http.StatusCreated) | ||
| 45 | + _ = json.NewEncoder(w).Encode(map[string]any{ | ||
| 46 | + "id": "c-1", "author": "rick", "body": body.Body, "created_at": "2026-08-26T10:00:00Z", | ||
| 47 | + }) | ||
| 48 | + })) | ||
| 49 | +} | ||
| 50 | + | ||
| 51 | +func withBodyServer(t *testing.T, got *string) func() { | ||
| 52 | + t.Helper() | ||
| 53 | + srv := commentBodyServer(t, got) | ||
| 54 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | ||
| 55 | + t.Setenv("RICKUB_HOST", srv.URL) | ||
| 56 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | ||
| 57 | + return srv.Close | ||
| 58 | +} | ||
| 59 | + | ||
| 60 | +// executeStdin is execute() with stdin wired to a string, for the `-b -` path. | ||
| 61 | +func executeStdin(t *testing.T, stdin string, args ...string) (string, string, error) { | ||
| 62 | + t.Helper() | ||
| 63 | + rootCmd.SetIn(strings.NewReader(stdin)) | ||
| 64 | + defer rootCmd.SetIn(nil) | ||
| 65 | + return execute(t, args...) | ||
| 66 | +} | ||
| 67 | + | ||
| 68 | +// TestMRCommentBodyDashReadsStdin: `-b -` sends what was piped in, byte for byte | ||
| 69 | +// — not the literal "-". This is issue #94's acceptance criterion 1. | ||
| 70 | +func TestMRCommentBodyDashReadsStdin(t *testing.T) { | ||
| 71 | + var sent string | ||
| 72 | + defer withBodyServer(t, &sent)() | ||
| 73 | + | ||
| 74 | + const review = "## Review\n\nLine 128 needs a nil guard.\n\n- point one\n- point two" | ||
| 75 | + if _, _, err := executeStdin(t, review+"\n", "mr", "comment", "11", "-R", "rick/portal", "-b", "-"); err != nil { | ||
| 76 | + t.Fatalf("execute: %v", err) | ||
| 77 | + } | ||
| 78 | + if sent == "-" { | ||
| 79 | + t.Fatal(`the literal "-" was posted — issue #94 has regressed`) | ||
| 80 | + } | ||
| 81 | + if sent != review { | ||
| 82 | + t.Fatalf("posted body = %q, want the stdin text %q", sent, review) | ||
| 83 | + } | ||
| 84 | + // The length check is the habit the incident taught: a success line proves | ||
| 85 | + // nothing, the byte count does. | ||
| 86 | + if len(sent) != len(review) { | ||
| 87 | + t.Fatalf("posted %d bytes, piped %d", len(sent), len(review)) | ||
| 88 | + } | ||
| 89 | +} | ||
| 90 | + | ||
| 91 | +// TestMRCommentBodyStdinTrimsCRLF: a heredoc piped in with Windows CRLF line | ||
| 92 | +// endings must not leave a stray trailing '\r' on the posted body. resolveBody | ||
| 93 | +// trims "\r\n", not just "\n", so the body a cross-platform user pipes in is the | ||
| 94 | +// body that lands — byte for byte, no carriage return smuggled onto the last | ||
| 95 | +// line. (Athena review, MR !1: TrimRight "\n" left the '\r'.) | ||
| 96 | +func TestMRCommentBodyStdinTrimsCRLF(t *testing.T) { | ||
| 97 | + var sent string | ||
| 98 | + defer withBodyServer(t, &sent)() | ||
| 99 | + | ||
| 100 | + // Internal CRLFs are preserved; only the trailing run is trimmed. | ||
| 101 | + const want = "first line\r\nsecond line" | ||
| 102 | + stdin := want + "\r\n" // the heredoc's own terminating CRLF | ||
| 103 | + if _, _, err := executeStdin(t, stdin, "mr", "comment", "11", "-R", "rick/portal", "-b", "-"); err != nil { | ||
| 104 | + t.Fatalf("execute: %v", err) | ||
| 105 | + } | ||
| 106 | + if strings.HasSuffix(sent, "\r") || strings.HasSuffix(sent, "\n") { | ||
| 107 | + t.Fatalf("posted body still ends with a carriage return/newline: %q", sent) | ||
| 108 | + } | ||
| 109 | + if sent != want { | ||
| 110 | + t.Fatalf("posted body = %q, want %q (only the trailing CRLF trimmed)", sent, want) | ||
| 111 | + } | ||
| 112 | +} | ||
| 113 | + | ||
| 114 | +// TestIssueCommentBodyDashReadsStdin: the sibling command keeps working through | ||
| 115 | +// the now-shared helper. | ||
| 116 | +func TestIssueCommentBodyDashReadsStdin(t *testing.T) { | ||
| 117 | + var sent string | ||
| 118 | + defer withBodyServer(t, &sent)() | ||
| 119 | + | ||
| 120 | + const text = "a multi-line\nissue comment" | ||
| 121 | + if _, _, err := executeStdin(t, text+"\n", "issue", "comment", "7", "-R", "rick/portal", "-b", "-"); err != nil { | ||
| 122 | + t.Fatalf("execute: %v", err) | ||
| 123 | + } | ||
| 124 | + if sent != text { | ||
| 125 | + t.Fatalf("posted body = %q, want %q", sent, text) | ||
| 126 | + } | ||
| 127 | +} | ||
| 128 | + | ||
| 129 | +// TestCommentEmptyBodyIsRefused: an empty or blank resolved body exits non-zero | ||
| 130 | +// and sends NOTHING, on both commands and both input paths. Issue #94's | ||
| 131 | +// acceptance criterion 2 — this is what makes a lost body loud instead of silent. | ||
| 132 | +func TestCommentEmptyBodyIsRefused(t *testing.T) { | ||
| 133 | + cases := []struct { | ||
| 134 | + name string | ||
| 135 | + stdin string | ||
| 136 | + args []string | ||
| 137 | + }{ | ||
| 138 | + {"mr, no --body", "", []string{"mr", "comment", "11", "-R", "rick/portal"}}, | ||
| 139 | + {"mr, empty --body", "", []string{"mr", "comment", "11", "-R", "rick/portal", "-b", ""}}, | ||
| 140 | + {"mr, whitespace --body", "", []string{"mr", "comment", "11", "-R", "rick/portal", "-b", " \n "}}, | ||
| 141 | + {"mr, empty stdin", "", []string{"mr", "comment", "11", "-R", "rick/portal", "-b", "-"}}, | ||
| 142 | + {"mr, blank stdin", "\n\n \n", []string{"mr", "comment", "11", "-R", "rick/portal", "-b", "-"}}, | ||
| 143 | + {"issue, no --body", "", []string{"issue", "comment", "7", "-R", "rick/portal"}}, | ||
| 144 | + {"issue, empty stdin", "", []string{"issue", "comment", "7", "-R", "rick/portal", "-b", "-"}}, | ||
| 145 | + } | ||
| 146 | + for _, tc := range cases { | ||
| 147 | + t.Run(tc.name, func(t *testing.T) { | ||
| 148 | + sent := "<nothing sent>" | ||
| 149 | + defer withBodyServer(t, &sent)() | ||
| 150 | + _, _, err := executeStdin(t, tc.stdin, tc.args...) | ||
| 151 | + if err == nil { | ||
| 152 | + t.Fatalf("%s was accepted; an empty body must exit non-zero", tc.name) | ||
| 153 | + } | ||
| 154 | + if sent != "<nothing sent>" { | ||
| 155 | + t.Fatalf("%s reached the server with body %q; nothing must be sent", tc.name, sent) | ||
| 156 | + } | ||
| 157 | + }) | ||
| 158 | + } | ||
| 159 | +} | ||
| 160 | + | ||
| 161 | +// TestMRCommentAnchoredBodyAlsoResolvesStdin: the line-comment path shares the | ||
| 162 | +// same resolution, so `--file … --line … -b -` is not a second trap. | ||
| 163 | +func TestMRCommentAnchoredBodyAlsoResolvesStdin(t *testing.T) { | ||
| 164 | + var seen captured | ||
| 165 | + defer withServer(t, &seen)() | ||
| 166 | + | ||
| 167 | + const text = "guard the nil case here" | ||
| 168 | + rootCmd.SetIn(strings.NewReader(text + "\n")) | ||
| 169 | + defer rootCmd.SetIn(nil) | ||
| 170 | + if _, _, err := execute(t, "mr", "comment", "11", "-R", "rick/portal", | ||
| 171 | + "--file", "web/api.go", "--line", "128", "-b", "-"); err != nil { | ||
| 172 | + t.Fatalf("execute: %v", err) | ||
| 173 | + } | ||
| 174 | + if seen.Body["body"] != text { | ||
| 175 | + t.Fatalf("anchored body = %v, want %q", seen.Body["body"], text) | ||
| 176 | + } | ||
| 177 | +} | ||
| 178 | + | ||
| 179 | +// TestMRCommentAnchoredBlankBodyIsRefused closes issue #94's criterion 2 on the | ||
| 180 | +// SECOND code path: a blank body on the anchored --file/--line route is refused | ||
| 181 | +// non-zero and NOTHING reaches the server — the /line-comments endpoint is never | ||
| 182 | +// called. TestCommentEmptyBodyIsRefused already proves this for the whole-MR | ||
| 183 | +// conversation route; the anchored route shares resolveBody but takes a different | ||
| 184 | +// branch of runPRComment, so it needs its own guard or a regression could slip | ||
| 185 | +// through the path the empty-body test does not walk. | ||
| 186 | +func TestMRCommentAnchoredBlankBodyIsRefused(t *testing.T) { | ||
| 187 | + cases := []struct { | ||
| 188 | + name string | ||
| 189 | + stdin string | ||
| 190 | + args []string | ||
| 191 | + }{ | ||
| 192 | + {"empty --body", "", []string{"mr", "comment", "11", "-R", "rick/portal", "--file", "web/api.go", "--line", "128", "-b", ""}}, | ||
| 193 | + {"whitespace --body", "", []string{"mr", "comment", "11", "-R", "rick/portal", "--file", "web/api.go", "--line", "128", "-b", " \n "}}, | ||
| 194 | + {"blank stdin", "\n \n", []string{"mr", "comment", "11", "-R", "rick/portal", "--file", "web/api.go", "--line", "128", "-b", "-"}}, | ||
| 195 | + } | ||
| 196 | + for _, tc := range cases { | ||
| 197 | + t.Run(tc.name, func(t *testing.T) { | ||
| 198 | + var seen captured | ||
| 199 | + defer withServer(t, &seen)() | ||
| 200 | + rootCmd.SetIn(strings.NewReader(tc.stdin)) | ||
| 201 | + defer rootCmd.SetIn(nil) | ||
| 202 | + if _, _, err := execute(t, tc.args...); err == nil { | ||
| 203 | + t.Fatalf("%s was accepted; a blank anchored body must exit non-zero", tc.name) | ||
| 204 | + } | ||
| 205 | + if seen.Method != "" { | ||
| 206 | + t.Fatalf("%s reached the server (%s %s); nothing must be sent", tc.name, seen.Method, seen.Path) | ||
| 207 | + } | ||
| 208 | + }) | ||
| 209 | + } | ||
| 210 | +} | ||
| 211 | + | ||
| 212 | +// TestBodyFlagHelpIsIdentical: both commands document the stdin behaviour in the | ||
| 213 | +// same words, because they read the same constant. Issue #94's criterion 4. | ||
| 214 | +func TestBodyFlagHelpIsIdentical(t *testing.T) { | ||
| 215 | + find := func(path ...string) string { | ||
| 216 | + c, _, err := rootCmd.Find(path) | ||
| 217 | + if err != nil { | ||
| 218 | + t.Fatalf("find %v: %v", path, err) | ||
| 219 | + } | ||
| 220 | + f := c.Flags().Lookup("body") | ||
| 221 | + if f == nil { | ||
| 222 | + t.Fatalf("%v has no --body flag", path) | ||
| 223 | + } | ||
| 224 | + return f.Usage | ||
| 225 | + } | ||
| 226 | + mr, issue := find("mr", "comment"), find("issue", "comment") | ||
| 227 | + if !strings.Contains(mr, bodyFlagHelp) { | ||
| 228 | + t.Errorf("mr comment --body help %q does not use the shared text %q", mr, bodyFlagHelp) | ||
| 229 | + } | ||
| 230 | + if !strings.Contains(issue, bodyFlagHelp) { | ||
| 231 | + t.Errorf("issue comment --body help %q does not use the shared text %q", issue, bodyFlagHelp) | ||
| 232 | + } | ||
| 233 | + if mr != issue { | ||
| 234 | + t.Errorf("the two --body help strings differ:\n mr: %q\n issue: %q", mr, issue) | ||
| 235 | + } | ||
| 236 | +} | ||
| 237 | + | ||
| 238 | +// TestEveryBodyFlagUsesTheSharedResolver is the structural guard behind issue | ||
| 239 | +// #94: the bug was not a missing stdin branch, it was a SECOND parser. This walks | ||
| 240 | +// every command in the tree, finds every `--body` flag, and asserts each one's | ||
| 241 | +// help text is built from the shared constant — which is only true if it went | ||
| 242 | +// through resolveBody/requireBody. A fifth command added with its own | ||
| 243 | +// interpretation of "-" fails here, naming itself. | ||
| 244 | +func TestEveryBodyFlagUsesTheSharedResolver(t *testing.T) { | ||
| 245 | + var checked int | ||
| 246 | + var walk func(c *cobra.Command) | ||
| 247 | + walk = func(c *cobra.Command) { | ||
| 248 | + if f := c.Flags().Lookup("body"); f != nil { | ||
| 249 | + checked++ | ||
| 250 | + if !strings.Contains(f.Usage, bodyFlagHelp) { | ||
| 251 | + t.Errorf("`%s` has a --body flag whose help %q does not use bodyFlagHelp — "+ | ||
| 252 | + "it is probably parsing --body itself; route it through resolveBody/requireBody "+ | ||
| 253 | + "in cmd/body.go instead (issue #94)", c.CommandPath(), f.Usage) | ||
| 254 | + } | ||
| 255 | + } | ||
| 256 | + for _, sub := range c.Commands() { | ||
| 257 | + walk(sub) | ||
| 258 | + } | ||
| 259 | + } | ||
| 260 | + walk(rootCmd) | ||
| 261 | + if checked == 0 { | ||
| 262 | + t.Fatal("found no --body flags at all — this test has stopped checking anything") | ||
| 263 | + } | ||
| 264 | +} | ||
added
cmd/cache.go +132 -0 | new file mode 100644 | ||
| @@ -0,0 +1,132 @@ | ||
| 1 | +package cmd | |
| 2 | + | |
| 3 | +import ( | |
| 4 | + "fmt" | |
| 5 | + | |
| 6 | + "github.com/spf13/cobra" | |
| 7 | +) | |
| 8 | + | |
| 9 | +// `rickub cache` — the operator surface for CI dependency-cache entries (issue | |
| 10 | +// #219). | |
| 11 | +// | |
| 12 | +// WHY IT EXISTS. A finalized cache entry is immutable: two jobs saving the same | |
| 13 | +// (key, version) must resolve to one writer, so re-reservation is refused. Correct | |
| 14 | +// for a healthy entry, fatal for a broken one — the platform had no way to say "this | |
| 15 | +// archive is bad, drop it". rickub's own truncated Go module cache was served to | |
| 16 | +// every run for two days, the failure surfaced inside the user's toolchain | |
| 17 | +// (`zip: not a valid zip file`), and the only workarounds were changing the lockfile | |
| 18 | +// (a new key) or bolting a repair step onto the workflow. The restore path now drops | |
| 19 | +// an unusable entry by itself; this is the button a human can press without SQL and | |
| 20 | +// object-store access. | |
| 21 | + | |
| 22 | +var ( | |
| 23 | + cacheRepo string | |
| 24 | + cacheScope string | |
| 25 | +) | |
| 26 | + | |
| 27 | +func init() { | |
| 28 | + cacheCmd := &cobra.Command{ | |
| 29 | + Use: "cache", | |
| 30 | + Short: "Work with CI dependency caches (actions/cache)", | |
| 31 | + } | |
| 32 | + cacheCmd.PersistentFlags().StringVarP(&cacheRepo, "repo", "R", "", "target repo as owner/repo (default: current git remote)") | |
| 33 | + | |
| 34 | + listCmd := &cobra.Command{ | |
| 35 | + Use: "list", | |
| 36 | + Short: "List the repo's cache entries (most recently restored first)", | |
| 37 | + Args: cobra.NoArgs, | |
| 38 | + RunE: runCacheList, | |
| 39 | + } | |
| 40 | + addPaging(listCmd) | |
| 41 | + | |
| 42 | + deleteCmd := &cobra.Command{ | |
| 43 | + Use: "delete <key>", | |
| 44 | + Short: "Delete every cache entry for a key (all versions)", | |
| 45 | + Long: "Purge a cache key's entries and their stored archives.\n\n" + | |
| 46 | + "Use this when a restore keeps failing on an entry that no job can replace —\n" + | |
| 47 | + "cache entries are immutable, so a poisoned one is served forever until it is\n" + | |
| 48 | + "deleted. Requires admin access to the repository.", | |
| 49 | + Args: cobra.ExactArgs(1), | |
| 50 | + RunE: runCacheDelete, | |
| 51 | + } | |
| 52 | + deleteCmd.Flags().StringVar(&cacheScope, "scope", "", "restrict to one git ref scope (e.g. refs/heads/main); default: every scope") | |
| 53 | + | |
| 54 | + cacheCmd.AddCommand(listCmd, deleteCmd) | |
| 55 | + rootCmd.AddCommand(cacheCmd) | |
| 56 | +} | |
| 57 | + | |
| 58 | +func runCacheList(cmd *cobra.Command, _ []string) error { | |
| 59 | + client, err := newClient() | |
| 60 | + if err != nil { | |
| 61 | + return err | |
| 62 | + } | |
| 63 | + owner, repo, err := resolveRepo(cacheRepo) | |
| 64 | + if err != nil { | |
| 65 | + return err | |
| 66 | + } | |
| 67 | + page, err := client.ListCaches(cmd.Context(), owner, repo, pageFlag, perPageFlag) | |
| 68 | + if err != nil { | |
| 69 | + return err | |
| 70 | + } | |
| 71 | + if flagJSON { | |
| 72 | + return printJSON(cmd.OutOrStdout(), page) | |
| 73 | + } | |
| 74 | + if len(page.Items) == 0 { | |
| 75 | + fmt.Fprintln(cmd.OutOrStdout(), "No cache entries.") | |
| 76 | + return nil | |
| 77 | + } | |
| 78 | + tw := newTabw(cmd.OutOrStdout()) | |
| 79 | + fmt.Fprintln(tw, "KEY\tSCOPE\tSIZE\tLAST RESTORED") | |
| 80 | + for _, e := range page.Items { | |
| 81 | + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\n", dash(e.Key), dash(e.Scope), humanBytes(e.SizeBytes), dash(e.LastAccessedAt)) | |
| 82 | + } | |
| 83 | + if err := tw.Flush(); err != nil { | |
| 84 | + return err | |
| 85 | + } | |
| 86 | + printPageFooter(cmd, page.Page) | |
| 87 | + return nil | |
| 88 | +} | |
| 89 | + | |
| 90 | +func runCacheDelete(cmd *cobra.Command, args []string) error { | |
| 91 | + client, err := newClient() | |
| 92 | + if err != nil { | |
| 93 | + return err | |
| 94 | + } | |
| 95 | + owner, repo, err := resolveRepo(cacheRepo) | |
| 96 | + if err != nil { | |
| 97 | + return err | |
| 98 | + } | |
| 99 | + res, err := client.DeleteCache(cmd.Context(), owner, repo, args[0], cacheScope) | |
| 100 | + if err != nil { | |
| 101 | + return err | |
| 102 | + } | |
| 103 | + if flagJSON { | |
| 104 | + return printJSON(cmd.OutOrStdout(), res) | |
| 105 | + } | |
| 106 | + fmt.Fprintf(cmd.OutOrStdout(), "Deleted %d cache entr%s for %q.\n", | |
| 107 | + res.Deleted, plural(res.Deleted, "y", "ies"), args[0]) | |
| 108 | + return nil | |
| 109 | +} | |
| 110 | + | |
| 111 | +func plural(n int, one, many string) string { | |
| 112 | + if n == 1 { | |
| 113 | + return one | |
| 114 | + } | |
| 115 | + return many | |
| 116 | +} | |
| 117 | + | |
| 118 | +// humanBytes renders a byte count the way an operator reads it. Cache sizes are the | |
| 119 | +// diagnostic here: the poisoned entry that started all this was recognisable at a | |
| 120 | +// glance as "64 MB" — exactly one upload block, which no real tarball ever is. | |
| 121 | +func humanBytes(n int64) string { | |
| 122 | + const unit = 1024 | |
| 123 | + if n < unit { | |
| 124 | + return fmt.Sprintf("%d B", n) | |
| 125 | + } | |
| 126 | + div, exp := int64(unit), 0 | |
| 127 | + for m := n / unit; m >= unit && exp < 3; m /= unit { | |
| 128 | + div *= unit | |
| 129 | + exp++ | |
| 130 | + } | |
| 131 | + return fmt.Sprintf("%.1f %cB", float64(n)/float64(div), "KMGT"[exp]) | |
| 132 | +} | |
| new file mode 100644 | |||
| @@ -0,0 +1,132 @@ | |||
| 1 | +package cmd | ||
| 2 | + | ||
| 3 | +import ( | ||
| 4 | + "fmt" | ||
| 5 | + | ||
| 6 | + "github.com/spf13/cobra" | ||
| 7 | +) | ||
| 8 | + | ||
| 9 | +// `rickub cache` — the operator surface for CI dependency-cache entries (issue | ||
| 10 | +// #219). | ||
| 11 | +// | ||
| 12 | +// WHY IT EXISTS. A finalized cache entry is immutable: two jobs saving the same | ||
| 13 | +// (key, version) must resolve to one writer, so re-reservation is refused. Correct | ||
| 14 | +// for a healthy entry, fatal for a broken one — the platform had no way to say "this | ||
| 15 | +// archive is bad, drop it". rickub's own truncated Go module cache was served to | ||
| 16 | +// every run for two days, the failure surfaced inside the user's toolchain | ||
| 17 | +// (`zip: not a valid zip file`), and the only workarounds were changing the lockfile | ||
| 18 | +// (a new key) or bolting a repair step onto the workflow. The restore path now drops | ||
| 19 | +// an unusable entry by itself; this is the button a human can press without SQL and | ||
| 20 | +// object-store access. | ||
| 21 | + | ||
| 22 | +var ( | ||
| 23 | + cacheRepo string | ||
| 24 | + cacheScope string | ||
| 25 | +) | ||
| 26 | + | ||
| 27 | +func init() { | ||
| 28 | + cacheCmd := &cobra.Command{ | ||
| 29 | + Use: "cache", | ||
| 30 | + Short: "Work with CI dependency caches (actions/cache)", | ||
| 31 | + } | ||
| 32 | + cacheCmd.PersistentFlags().StringVarP(&cacheRepo, "repo", "R", "", "target repo as owner/repo (default: current git remote)") | ||
| 33 | + | ||
| 34 | + listCmd := &cobra.Command{ | ||
| 35 | + Use: "list", | ||
| 36 | + Short: "List the repo's cache entries (most recently restored first)", | ||
| 37 | + Args: cobra.NoArgs, | ||
| 38 | + RunE: runCacheList, | ||
| 39 | + } | ||
| 40 | + addPaging(listCmd) | ||
| 41 | + | ||
| 42 | + deleteCmd := &cobra.Command{ | ||
| 43 | + Use: "delete <key>", | ||
| 44 | + Short: "Delete every cache entry for a key (all versions)", | ||
| 45 | + Long: "Purge a cache key's entries and their stored archives.\n\n" + | ||
| 46 | + "Use this when a restore keeps failing on an entry that no job can replace —\n" + | ||
| 47 | + "cache entries are immutable, so a poisoned one is served forever until it is\n" + | ||
| 48 | + "deleted. Requires admin access to the repository.", | ||
| 49 | + Args: cobra.ExactArgs(1), | ||
| 50 | + RunE: runCacheDelete, | ||
| 51 | + } | ||
| 52 | + deleteCmd.Flags().StringVar(&cacheScope, "scope", "", "restrict to one git ref scope (e.g. refs/heads/main); default: every scope") | ||
| 53 | + | ||
| 54 | + cacheCmd.AddCommand(listCmd, deleteCmd) | ||
| 55 | + rootCmd.AddCommand(cacheCmd) | ||
| 56 | +} | ||
| 57 | + | ||
| 58 | +func runCacheList(cmd *cobra.Command, _ []string) error { | ||
| 59 | + client, err := newClient() | ||
| 60 | + if err != nil { | ||
| 61 | + return err | ||
| 62 | + } | ||
| 63 | + owner, repo, err := resolveRepo(cacheRepo) | ||
| 64 | + if err != nil { | ||
| 65 | + return err | ||
| 66 | + } | ||
| 67 | + page, err := client.ListCaches(cmd.Context(), owner, repo, pageFlag, perPageFlag) | ||
| 68 | + if err != nil { | ||
| 69 | + return err | ||
| 70 | + } | ||
| 71 | + if flagJSON { | ||
| 72 | + return printJSON(cmd.OutOrStdout(), page) | ||
| 73 | + } | ||
| 74 | + if len(page.Items) == 0 { | ||
| 75 | + fmt.Fprintln(cmd.OutOrStdout(), "No cache entries.") | ||
| 76 | + return nil | ||
| 77 | + } | ||
| 78 | + tw := newTabw(cmd.OutOrStdout()) | ||
| 79 | + fmt.Fprintln(tw, "KEY\tSCOPE\tSIZE\tLAST RESTORED") | ||
| 80 | + for _, e := range page.Items { | ||
| 81 | + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\n", dash(e.Key), dash(e.Scope), humanBytes(e.SizeBytes), dash(e.LastAccessedAt)) | ||
| 82 | + } | ||
| 83 | + if err := tw.Flush(); err != nil { | ||
| 84 | + return err | ||
| 85 | + } | ||
| 86 | + printPageFooter(cmd, page.Page) | ||
| 87 | + return nil | ||
| 88 | +} | ||
| 89 | + | ||
| 90 | +func runCacheDelete(cmd *cobra.Command, args []string) error { | ||
| 91 | + client, err := newClient() | ||
| 92 | + if err != nil { | ||
| 93 | + return err | ||
| 94 | + } | ||
| 95 | + owner, repo, err := resolveRepo(cacheRepo) | ||
| 96 | + if err != nil { | ||
| 97 | + return err | ||
| 98 | + } | ||
| 99 | + res, err := client.DeleteCache(cmd.Context(), owner, repo, args[0], cacheScope) | ||
| 100 | + if err != nil { | ||
| 101 | + return err | ||
| 102 | + } | ||
| 103 | + if flagJSON { | ||
| 104 | + return printJSON(cmd.OutOrStdout(), res) | ||
| 105 | + } | ||
| 106 | + fmt.Fprintf(cmd.OutOrStdout(), "Deleted %d cache entr%s for %q.\n", | ||
| 107 | + res.Deleted, plural(res.Deleted, "y", "ies"), args[0]) | ||
| 108 | + return nil | ||
| 109 | +} | ||
| 110 | + | ||
| 111 | +func plural(n int, one, many string) string { | ||
| 112 | + if n == 1 { | ||
| 113 | + return one | ||
| 114 | + } | ||
| 115 | + return many | ||
| 116 | +} | ||
| 117 | + | ||
| 118 | +// humanBytes renders a byte count the way an operator reads it. Cache sizes are the | ||
| 119 | +// diagnostic here: the poisoned entry that started all this was recognisable at a | ||
| 120 | +// glance as "64 MB" — exactly one upload block, which no real tarball ever is. | ||
| 121 | +func humanBytes(n int64) string { | ||
| 122 | + const unit = 1024 | ||
| 123 | + if n < unit { | ||
| 124 | + return fmt.Sprintf("%d B", n) | ||
| 125 | + } | ||
| 126 | + div, exp := int64(unit), 0 | ||
| 127 | + for m := n / unit; m >= unit && exp < 3; m /= unit { | ||
| 128 | + div *= unit | ||
| 129 | + exp++ | ||
| 130 | + } | ||
| 131 | + return fmt.Sprintf("%.1f %cB", float64(n)/float64(div), "KMGT"[exp]) | ||
| 132 | +} | ||
added
cmd/cache_test.go +145 -0 | new file mode 100644 | ||
| @@ -0,0 +1,145 @@ | ||
| 1 | +package cmd | |
| 2 | + | |
| 3 | +import ( | |
| 4 | + "encoding/json" | |
| 5 | + "net/http" | |
| 6 | + "net/http/httptest" | |
| 7 | + "net/url" | |
| 8 | + "strings" | |
| 9 | + "testing" | |
| 10 | +) | |
| 11 | + | |
| 12 | +// `rickub cache` is the operator exit from a poisoned CI cache entry (issue #219): | |
| 13 | +// a finalized entry is immutable, so before this existed the only way to clear one | |
| 14 | +// was direct SQL plus object-store access. These tests pin that the command reaches | |
| 15 | +// the right route with the right query, and renders an answer a human can act on. | |
| 16 | + | |
| 17 | +func cacheServer(t *testing.T, deleted int) *httptest.Server { | |
| 18 | + t.Helper() | |
| 19 | + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 20 | + if r.URL.Path != "/api/v1/repos/rick/portal/actions/caches" { | |
| 21 | + w.WriteHeader(http.StatusNotFound) | |
| 22 | + _, _ = w.Write([]byte(`{"error":{"code":"not_found","message":"no"}}`)) | |
| 23 | + return | |
| 24 | + } | |
| 25 | + switch r.Method { | |
| 26 | + case http.MethodGet: | |
| 27 | + _ = json.NewEncoder(w).Encode(map[string]any{ | |
| 28 | + "page": 1, "per_page": 30, "has_next": false, | |
| 29 | + "items": []map[string]any{ | |
| 30 | + { | |
| 31 | + "id": "11111111-1111-1111-1111-111111111111", | |
| 32 | + "key": "setup-go-Linux-x64", "scope": "refs/heads/main", "version": "v1", | |
| 33 | + // The tell that started issue #217: exactly one 64 MiB upload block. | |
| 34 | + "size_bytes": 67108864, "last_accessed_at": "2026-09-04T22:46:00Z", | |
| 35 | + }, | |
| 36 | + }, | |
| 37 | + }) | |
| 38 | + case http.MethodDelete: | |
| 39 | + // The command must pass the key (and scope, when given) as query params — | |
| 40 | + // a purge that silently addressed the wrong key would be worse than none. | |
| 41 | + if got := r.URL.Query().Get("key"); got != "setup-go-Linux-x64" { | |
| 42 | + w.WriteHeader(http.StatusBadRequest) | |
| 43 | + _, _ = w.Write([]byte(`{"error":{"code":"invalid_request","message":"key=` + got + `"}}`)) | |
| 44 | + return | |
| 45 | + } | |
| 46 | + _ = json.NewEncoder(w).Encode(map[string]any{ | |
| 47 | + "deleted": deleted, "key": r.URL.Query().Get("key"), | |
| 48 | + "scope_seen": r.URL.Query().Get("scope"), | |
| 49 | + }) | |
| 50 | + default: | |
| 51 | + w.WriteHeader(http.StatusMethodNotAllowed) | |
| 52 | + } | |
| 53 | + })) | |
| 54 | +} | |
| 55 | + | |
| 56 | +func TestCacheListTable(t *testing.T) { | |
| 57 | + srv := cacheServer(t, 0) | |
| 58 | + defer srv.Close() | |
| 59 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | |
| 60 | + t.Setenv("RICKUB_HOST", srv.URL) | |
| 61 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | |
| 62 | + | |
| 63 | + out, _, err := execute(t, "cache", "list", "--repo", "rick/portal") | |
| 64 | + if err != nil { | |
| 65 | + t.Fatalf("execute: %v", err) | |
| 66 | + } | |
| 67 | + for _, want := range []string{"KEY", "SCOPE", "SIZE", "LAST RESTORED", "setup-go-Linux-x64", "refs/heads/main"} { | |
| 68 | + if !strings.Contains(out, want) { | |
| 69 | + t.Errorf("cache list output missing %q:\n%s", want, out) | |
| 70 | + } | |
| 71 | + } | |
| 72 | + // The size is what an operator diagnoses with; "64.0 MB" is legible where | |
| 73 | + // 67108864 is not. | |
| 74 | + if !strings.Contains(out, "64.0 MB") { | |
| 75 | + t.Errorf("cache list must render a human-readable size:\n%s", out) | |
| 76 | + } | |
| 77 | +} | |
| 78 | + | |
| 79 | +func TestCacheDeleteReportsWhatItRemoved(t *testing.T) { | |
| 80 | + srv := cacheServer(t, 2) | |
| 81 | + defer srv.Close() | |
| 82 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | |
| 83 | + t.Setenv("RICKUB_HOST", srv.URL) | |
| 84 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | |
| 85 | + | |
| 86 | + out, _, err := execute(t, "cache", "delete", "setup-go-Linux-x64", "--repo", "rick/portal") | |
| 87 | + if err != nil { | |
| 88 | + t.Fatalf("execute: %v", err) | |
| 89 | + } | |
| 90 | + if !strings.Contains(out, "Deleted 2 cache entries") { | |
| 91 | + t.Errorf("delete must report the count (one key can hold several versions):\n%s", out) | |
| 92 | + } | |
| 93 | + | |
| 94 | + // --scope is forwarded, so an operator can clear one branch without touching the | |
| 95 | + // default branch's cache. | |
| 96 | + var seen url.Values | |
| 97 | + scoped := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 98 | + seen = r.URL.Query() | |
| 99 | + _ = json.NewEncoder(w).Encode(map[string]any{"deleted": 1, "key": seen.Get("key")}) | |
| 100 | + })) | |
| 101 | + defer scoped.Close() | |
| 102 | + t.Setenv("RICKUB_HOST", scoped.URL) | |
| 103 | + if _, _, err := execute(t, "cache", "delete", "k", "--repo", "rick/portal", "--scope", "refs/heads/topic"); err != nil { | |
| 104 | + t.Fatalf("scoped delete: %v", err) | |
| 105 | + } | |
| 106 | + if seen.Get("scope") != "refs/heads/topic" || seen.Get("key") != "k" { | |
| 107 | + t.Errorf("scoped delete sent %v, want key=k scope=refs/heads/topic", seen) | |
| 108 | + } | |
| 109 | +} | |
| 110 | + | |
| 111 | +func TestCacheDeleteSurfacesTheAPIError(t *testing.T) { | |
| 112 | + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 113 | + w.WriteHeader(http.StatusForbidden) | |
| 114 | + _, _ = w.Write([]byte(`{"error":{"code":"admin_required","message":"admin access to this repository is required"}}`)) | |
| 115 | + })) | |
| 116 | + defer srv.Close() | |
| 117 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | |
| 118 | + t.Setenv("RICKUB_HOST", srv.URL) | |
| 119 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | |
| 120 | + | |
| 121 | + _, _, err := execute(t, "cache", "delete", "k", "--repo", "rick/portal") | |
| 122 | + if err == nil { | |
| 123 | + t.Fatal("a 403 must be an error, not a silent no-op — an operator has to know the purge did not happen") | |
| 124 | + } | |
| 125 | + if !strings.Contains(err.Error(), "admin_required") { | |
| 126 | + t.Errorf("error should surface the API code: %v", err) | |
| 127 | + } | |
| 128 | +} | |
| 129 | + | |
| 130 | +func TestHumanBytes(t *testing.T) { | |
| 131 | + for _, tc := range []struct { | |
| 132 | + in int64 | |
| 133 | + want string | |
| 134 | + }{ | |
| 135 | + {0, "0 B"}, | |
| 136 | + {999, "999 B"}, | |
| 137 | + {67108864, "64.0 MB"}, | |
| 138 | + {130704377, "124.6 MB"}, | |
| 139 | + {5 << 30, "5.0 GB"}, | |
| 140 | + } { | |
| 141 | + if got := humanBytes(tc.in); got != tc.want { | |
| 142 | + t.Errorf("humanBytes(%d) = %q, want %q", tc.in, got, tc.want) | |
| 143 | + } | |
| 144 | + } | |
| 145 | +} | |
| new file mode 100644 | |||
| @@ -0,0 +1,145 @@ | |||
| 1 | +package cmd | ||
| 2 | + | ||
| 3 | +import ( | ||
| 4 | + "encoding/json" | ||
| 5 | + "net/http" | ||
| 6 | + "net/http/httptest" | ||
| 7 | + "net/url" | ||
| 8 | + "strings" | ||
| 9 | + "testing" | ||
| 10 | +) | ||
| 11 | + | ||
| 12 | +// `rickub cache` is the operator exit from a poisoned CI cache entry (issue #219): | ||
| 13 | +// a finalized entry is immutable, so before this existed the only way to clear one | ||
| 14 | +// was direct SQL plus object-store access. These tests pin that the command reaches | ||
| 15 | +// the right route with the right query, and renders an answer a human can act on. | ||
| 16 | + | ||
| 17 | +func cacheServer(t *testing.T, deleted int) *httptest.Server { | ||
| 18 | + t.Helper() | ||
| 19 | + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 20 | + if r.URL.Path != "/api/v1/repos/rick/portal/actions/caches" { | ||
| 21 | + w.WriteHeader(http.StatusNotFound) | ||
| 22 | + _, _ = w.Write([]byte(`{"error":{"code":"not_found","message":"no"}}`)) | ||
| 23 | + return | ||
| 24 | + } | ||
| 25 | + switch r.Method { | ||
| 26 | + case http.MethodGet: | ||
| 27 | + _ = json.NewEncoder(w).Encode(map[string]any{ | ||
| 28 | + "page": 1, "per_page": 30, "has_next": false, | ||
| 29 | + "items": []map[string]any{ | ||
| 30 | + { | ||
| 31 | + "id": "11111111-1111-1111-1111-111111111111", | ||
| 32 | + "key": "setup-go-Linux-x64", "scope": "refs/heads/main", "version": "v1", | ||
| 33 | + // The tell that started issue #217: exactly one 64 MiB upload block. | ||
| 34 | + "size_bytes": 67108864, "last_accessed_at": "2026-09-04T22:46:00Z", | ||
| 35 | + }, | ||
| 36 | + }, | ||
| 37 | + }) | ||
| 38 | + case http.MethodDelete: | ||
| 39 | + // The command must pass the key (and scope, when given) as query params — | ||
| 40 | + // a purge that silently addressed the wrong key would be worse than none. | ||
| 41 | + if got := r.URL.Query().Get("key"); got != "setup-go-Linux-x64" { | ||
| 42 | + w.WriteHeader(http.StatusBadRequest) | ||
| 43 | + _, _ = w.Write([]byte(`{"error":{"code":"invalid_request","message":"key=` + got + `"}}`)) | ||
| 44 | + return | ||
| 45 | + } | ||
| 46 | + _ = json.NewEncoder(w).Encode(map[string]any{ | ||
| 47 | + "deleted": deleted, "key": r.URL.Query().Get("key"), | ||
| 48 | + "scope_seen": r.URL.Query().Get("scope"), | ||
| 49 | + }) | ||
| 50 | + default: | ||
| 51 | + w.WriteHeader(http.StatusMethodNotAllowed) | ||
| 52 | + } | ||
| 53 | + })) | ||
| 54 | +} | ||
| 55 | + | ||
| 56 | +func TestCacheListTable(t *testing.T) { | ||
| 57 | + srv := cacheServer(t, 0) | ||
| 58 | + defer srv.Close() | ||
| 59 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | ||
| 60 | + t.Setenv("RICKUB_HOST", srv.URL) | ||
| 61 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | ||
| 62 | + | ||
| 63 | + out, _, err := execute(t, "cache", "list", "--repo", "rick/portal") | ||
| 64 | + if err != nil { | ||
| 65 | + t.Fatalf("execute: %v", err) | ||
| 66 | + } | ||
| 67 | + for _, want := range []string{"KEY", "SCOPE", "SIZE", "LAST RESTORED", "setup-go-Linux-x64", "refs/heads/main"} { | ||
| 68 | + if !strings.Contains(out, want) { | ||
| 69 | + t.Errorf("cache list output missing %q:\n%s", want, out) | ||
| 70 | + } | ||
| 71 | + } | ||
| 72 | + // The size is what an operator diagnoses with; "64.0 MB" is legible where | ||
| 73 | + // 67108864 is not. | ||
| 74 | + if !strings.Contains(out, "64.0 MB") { | ||
| 75 | + t.Errorf("cache list must render a human-readable size:\n%s", out) | ||
| 76 | + } | ||
| 77 | +} | ||
| 78 | + | ||
| 79 | +func TestCacheDeleteReportsWhatItRemoved(t *testing.T) { | ||
| 80 | + srv := cacheServer(t, 2) | ||
| 81 | + defer srv.Close() | ||
| 82 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | ||
| 83 | + t.Setenv("RICKUB_HOST", srv.URL) | ||
| 84 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | ||
| 85 | + | ||
| 86 | + out, _, err := execute(t, "cache", "delete", "setup-go-Linux-x64", "--repo", "rick/portal") | ||
| 87 | + if err != nil { | ||
| 88 | + t.Fatalf("execute: %v", err) | ||
| 89 | + } | ||
| 90 | + if !strings.Contains(out, "Deleted 2 cache entries") { | ||
| 91 | + t.Errorf("delete must report the count (one key can hold several versions):\n%s", out) | ||
| 92 | + } | ||
| 93 | + | ||
| 94 | + // --scope is forwarded, so an operator can clear one branch without touching the | ||
| 95 | + // default branch's cache. | ||
| 96 | + var seen url.Values | ||
| 97 | + scoped := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 98 | + seen = r.URL.Query() | ||
| 99 | + _ = json.NewEncoder(w).Encode(map[string]any{"deleted": 1, "key": seen.Get("key")}) | ||
| 100 | + })) | ||
| 101 | + defer scoped.Close() | ||
| 102 | + t.Setenv("RICKUB_HOST", scoped.URL) | ||
| 103 | + if _, _, err := execute(t, "cache", "delete", "k", "--repo", "rick/portal", "--scope", "refs/heads/topic"); err != nil { | ||
| 104 | + t.Fatalf("scoped delete: %v", err) | ||
| 105 | + } | ||
| 106 | + if seen.Get("scope") != "refs/heads/topic" || seen.Get("key") != "k" { | ||
| 107 | + t.Errorf("scoped delete sent %v, want key=k scope=refs/heads/topic", seen) | ||
| 108 | + } | ||
| 109 | +} | ||
| 110 | + | ||
| 111 | +func TestCacheDeleteSurfacesTheAPIError(t *testing.T) { | ||
| 112 | + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 113 | + w.WriteHeader(http.StatusForbidden) | ||
| 114 | + _, _ = w.Write([]byte(`{"error":{"code":"admin_required","message":"admin access to this repository is required"}}`)) | ||
| 115 | + })) | ||
| 116 | + defer srv.Close() | ||
| 117 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | ||
| 118 | + t.Setenv("RICKUB_HOST", srv.URL) | ||
| 119 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | ||
| 120 | + | ||
| 121 | + _, _, err := execute(t, "cache", "delete", "k", "--repo", "rick/portal") | ||
| 122 | + if err == nil { | ||
| 123 | + t.Fatal("a 403 must be an error, not a silent no-op — an operator has to know the purge did not happen") | ||
| 124 | + } | ||
| 125 | + if !strings.Contains(err.Error(), "admin_required") { | ||
| 126 | + t.Errorf("error should surface the API code: %v", err) | ||
| 127 | + } | ||
| 128 | +} | ||
| 129 | + | ||
| 130 | +func TestHumanBytes(t *testing.T) { | ||
| 131 | + for _, tc := range []struct { | ||
| 132 | + in int64 | ||
| 133 | + want string | ||
| 134 | + }{ | ||
| 135 | + {0, "0 B"}, | ||
| 136 | + {999, "999 B"}, | ||
| 137 | + {67108864, "64.0 MB"}, | ||
| 138 | + {130704377, "124.6 MB"}, | ||
| 139 | + {5 << 30, "5.0 GB"}, | ||
| 140 | + } { | ||
| 141 | + if got := humanBytes(tc.in); got != tc.want { | ||
| 142 | + t.Errorf("humanBytes(%d) = %q, want %q", tc.in, got, tc.want) | ||
| 143 | + } | ||
| 144 | + } | ||
| 145 | +} | ||
added
cmd/comment_marks_test.go +34 -0 | new file mode 100644 | ||
| @@ -0,0 +1,34 @@ | ||
| 1 | +package cmd | |
| 2 | + | |
| 3 | +import ( | |
| 4 | + "testing" | |
| 5 | + | |
| 6 | + "rickub.com/rickub/cli/internal/api" | |
| 7 | +) | |
| 8 | + | |
| 9 | +// commentMarks renders a comment's non-body state (retracted / edited) for the | |
| 10 | +// merge-request and issue listings. It is the CLI half of issue #94's soft-delete | |
| 11 | +// (a tombstone is a comment with Deleted set and its Body cleared), so it must be | |
| 12 | +// covered directly: the value it returns is the only signal a reader gets that a | |
| 13 | +// comment they see with an empty body was retracted rather than blank. | |
| 14 | +func TestCommentMarks(t *testing.T) { | |
| 15 | + cases := []struct { | |
| 16 | + name string | |
| 17 | + c api.Comment | |
| 18 | + want string | |
| 19 | + }{ | |
| 20 | + {"plain", api.Comment{ID: "c1", Body: "hi"}, ""}, | |
| 21 | + {"edited", api.Comment{ID: "c1", Body: "hi", Edited: true}, " [edited]"}, | |
| 22 | + {"deleted tombstone", api.Comment{ID: "c1", Body: "", Deleted: true}, " [deleted]"}, | |
| 23 | + // A tombstone can also carry Edited (it was edited, then retracted); | |
| 24 | + // "deleted" is the state that matters, so it must win. | |
| 25 | + {"deleted wins over edited", api.Comment{ID: "c1", Deleted: true, Edited: true}, " [deleted]"}, | |
| 26 | + } | |
| 27 | + for _, tc := range cases { | |
| 28 | + t.Run(tc.name, func(t *testing.T) { | |
| 29 | + if got := commentMarks(tc.c); got != tc.want { | |
| 30 | + t.Fatalf("commentMarks(%+v) = %q, want %q", tc.c, got, tc.want) | |
| 31 | + } | |
| 32 | + }) | |
| 33 | + } | |
| 34 | +} | |
| new file mode 100644 | |||
| @@ -0,0 +1,34 @@ | |||
| 1 | +package cmd | ||
| 2 | + | ||
| 3 | +import ( | ||
| 4 | + "testing" | ||
| 5 | + | ||
| 6 | + "rickub.com/rickub/cli/internal/api" | ||
| 7 | +) | ||
| 8 | + | ||
| 9 | +// commentMarks renders a comment's non-body state (retracted / edited) for the | ||
| 10 | +// merge-request and issue listings. It is the CLI half of issue #94's soft-delete | ||
| 11 | +// (a tombstone is a comment with Deleted set and its Body cleared), so it must be | ||
| 12 | +// covered directly: the value it returns is the only signal a reader gets that a | ||
| 13 | +// comment they see with an empty body was retracted rather than blank. | ||
| 14 | +func TestCommentMarks(t *testing.T) { | ||
| 15 | + cases := []struct { | ||
| 16 | + name string | ||
| 17 | + c api.Comment | ||
| 18 | + want string | ||
| 19 | + }{ | ||
| 20 | + {"plain", api.Comment{ID: "c1", Body: "hi"}, ""}, | ||
| 21 | + {"edited", api.Comment{ID: "c1", Body: "hi", Edited: true}, " [edited]"}, | ||
| 22 | + {"deleted tombstone", api.Comment{ID: "c1", Body: "", Deleted: true}, " [deleted]"}, | ||
| 23 | + // A tombstone can also carry Edited (it was edited, then retracted); | ||
| 24 | + // "deleted" is the state that matters, so it must win. | ||
| 25 | + {"deleted wins over edited", api.Comment{ID: "c1", Deleted: true, Edited: true}, " [deleted]"}, | ||
| 26 | + } | ||
| 27 | + for _, tc := range cases { | ||
| 28 | + t.Run(tc.name, func(t *testing.T) { | ||
| 29 | + if got := commentMarks(tc.c); got != tc.want { | ||
| 30 | + t.Fatalf("commentMarks(%+v) = %q, want %q", tc.c, got, tc.want) | ||
| 31 | + } | ||
| 32 | + }) | ||
| 33 | + } | ||
| 34 | +} | ||
modified
cmd/issue.go +44 -27 | @@ -53,7 +53,7 @@ func init() { | ||
| 53 | 53 | RunE: runIssueCreate, |
| 54 | 54 | } |
| 55 | 55 | createCmd.Flags().StringVarP(&issueTitle, "title", "t", "", "title (required)") |
| 56 | - createCmd.Flags().StringVarP(&issueBody, "body", "b", "", "description body ('-' reads stdin)") | |
| 56 | + createCmd.Flags().StringVarP(&issueBody, "body", "b", "", "description "+bodyFlagHelp) | |
| 57 | 57 | |
| 58 | 58 | closeCmd := &cobra.Command{ |
| 59 | 59 | Use: "close <number>", |
| @@ -75,7 +75,7 @@ func init() { | ||
| 75 | 75 | Args: cobra.ExactArgs(1), |
| 76 | 76 | RunE: runIssueComment, |
| 77 | 77 | } |
| 78 | - commentCmd.Flags().StringVarP(&issueBody, "body", "b", "", "comment body ('-' reads stdin)") | |
| 78 | + commentCmd.Flags().StringVarP(&issueBody, "body", "b", "", "comment "+bodyFlagHelp+"; required") | |
| 79 | 79 | |
| 80 | 80 | labelCmd := &cobra.Command{ |
| 81 | 81 | Use: "label <number>", |
| @@ -111,7 +111,20 @@ func init() { | ||
| 111 | 111 | RunE: runIssueLabelsList, |
| 112 | 112 | } |
| 113 | 113 | |
| 114 | - issueCmd.AddCommand(listCmd, viewCmd, createCmd, closeCmd, reopenCmd, commentCmd, labelCmd, milestoneCmd, assignCmd, labelsCmd) | |
| 114 | + deleteCommentCmd := &cobra.Command{ | |
| 115 | + Use: "delete-comment <number> <comment-id>", | |
| 116 | + Aliases: []string{"retract"}, | |
| 117 | + Short: "Retract a comment on an issue", | |
| 118 | + Long: "Retract one of an issue's comments.\n\n" + | |
| 119 | + "Comment ids are shown by `rickub issue view <number>` (and in its --json output).\n" + | |
| 120 | + "The comment's author, or anyone with write access to the repository, may retract it.\n" + | |
| 121 | + "The delete is SOFT: the comment survives as a tombstone with its body cleared.", | |
| 122 | + Args: cobra.ExactArgs(2), | |
| 123 | + RunE: runIssueDeleteComment, | |
| 124 | + } | |
| 125 | + | |
| 126 | + issueCmd.AddCommand(listCmd, viewCmd, createCmd, closeCmd, reopenCmd, commentCmd, labelCmd, | |
| 127 | + milestoneCmd, assignCmd, labelsCmd, deleteCommentCmd) | |
| 115 | 128 | rootCmd.AddCommand(issueCmd) |
| 116 | 129 | } |
| 117 | 130 | |
| @@ -228,34 +241,17 @@ func runIssueView(cmd *cobra.Command, args []string) error { | ||
| 228 | 241 | fmt.Fprintln(out, clean(i.Body)) |
| 229 | 242 | } |
| 230 | 243 | for _, c := range i.Comments { |
| 231 | - fmt.Fprintf(out, "\n--- %s (%s) ---\n%s\n", clean(c.Author), humanTime(c.CreatedAt), clean(c.Body)) | |
| 244 | + // The id is the handle `issue delete-comment` takes; print it. | |
| 245 | + fmt.Fprintf(out, "\n--- [%s] %s (%s)%s ---\n%s\n", c.ID, clean(c.Author), humanTime(c.CreatedAt), commentMarks(c), clean(c.Body)) | |
| 232 | 246 | } |
| 233 | 247 | return nil |
| 234 | 248 | } |
| 235 | 249 | |
| 236 | -// bodyOrStdin resolves a --body flag value, reading stdin when it is "-". | |
| 237 | -func bodyOrStdin(cmd *cobra.Command, body string) (string, error) { | |
| 238 | - if body != "-" { | |
| 239 | - return body, nil | |
| 240 | - } | |
| 241 | - var sb strings.Builder | |
| 242 | - buf := make([]byte, 32*1024) | |
| 243 | - in := cmd.InOrStdin() | |
| 244 | - for { | |
| 245 | - n, err := in.Read(buf) | |
| 246 | - sb.Write(buf[:n]) | |
| 247 | - if err != nil { | |
| 248 | - break | |
| 249 | - } | |
| 250 | - } | |
| 251 | - return strings.TrimRight(sb.String(), "\n"), nil | |
| 252 | -} | |
| 253 | - | |
| 254 | 250 | func runIssueCreate(cmd *cobra.Command, _ []string) error { |
| 255 | 251 | if issueTitle == "" { |
| 256 | 252 | return fmt.Errorf("--title is required") |
| 257 | 253 | } |
| 258 | - body, err := bodyOrStdin(cmd, issueBody) | |
| 254 | + body, err := resolveBody(cmd, issueBody) | |
| 259 | 255 | if err != nil { |
| 260 | 256 | return err |
| 261 | 257 | } |
| @@ -302,13 +298,34 @@ func runIssueState(cmd *cobra.Command, args []string, state string) error { | ||
| 302 | 298 | return nil |
| 303 | 299 | } |
| 304 | 300 | |
| 305 | -func runIssueComment(cmd *cobra.Command, args []string) error { | |
| 306 | - body, err := bodyOrStdin(cmd, issueBody) | |
| 301 | +func runIssueDeleteComment(cmd *cobra.Command, args []string) error { | |
| 302 | + client, err := newClient() | |
| 303 | + if err != nil { | |
| 304 | + return err | |
| 305 | + } | |
| 306 | + owner, repo, err := resolveRepo(issueRepo) | |
| 307 | 307 | if err != nil { |
| 308 | 308 | return err |
| 309 | 309 | } |
| 310 | - if strings.TrimSpace(body) == "" { | |
| 311 | - return fmt.Errorf("--body is required") | |
| 310 | + n, err := issueNumber(args[0]) | |
| 311 | + if err != nil { | |
| 312 | + return err | |
| 313 | + } | |
| 314 | + id := strings.TrimSpace(args[1]) | |
| 315 | + if id == "" { | |
| 316 | + return fmt.Errorf("a comment id is required (see `rickub issue view %d`)", n) | |
| 317 | + } | |
| 318 | + if err := client.DeleteIssueComment(cmd.Context(), owner, repo, n, id); err != nil { | |
| 319 | + return err | |
| 320 | + } | |
| 321 | + fmt.Fprintf(cmd.OutOrStdout(), "Retracted comment %s on issue #%d\n", id, n) | |
| 322 | + return nil | |
| 323 | +} | |
| 324 | + | |
| 325 | +func runIssueComment(cmd *cobra.Command, args []string) error { | |
| 326 | + body, err := requireBody(cmd, issueBody) | |
| 327 | + if err != nil { | |
| 328 | + return err | |
| 312 | 329 | } |
| 313 | 330 | client, err := newClient() |
| 314 | 331 | if err != nil { |
| @@ -53,7 +53,7 @@ func init() { | |||
| 53 | RunE: runIssueCreate, | 53 | RunE: runIssueCreate, |
| 54 | } | 54 | } |
| 55 | createCmd.Flags().StringVarP(&issueTitle, "title", "t", "", "title (required)") | 55 | createCmd.Flags().StringVarP(&issueTitle, "title", "t", "", "title (required)") |
| 56 | - createCmd.Flags().StringVarP(&issueBody, "body", "b", "", "description body ('-' reads stdin)") | 56 | + createCmd.Flags().StringVarP(&issueBody, "body", "b", "", "description "+bodyFlagHelp) |
| 57 | 57 | ||
| 58 | closeCmd := &cobra.Command{ | 58 | closeCmd := &cobra.Command{ |
| 59 | Use: "close <number>", | 59 | Use: "close <number>", |
| @@ -75,7 +75,7 @@ func init() { | |||
| 75 | Args: cobra.ExactArgs(1), | 75 | Args: cobra.ExactArgs(1), |
| 76 | RunE: runIssueComment, | 76 | RunE: runIssueComment, |
| 77 | } | 77 | } |
| 78 | - commentCmd.Flags().StringVarP(&issueBody, "body", "b", "", "comment body ('-' reads stdin)") | 78 | + commentCmd.Flags().StringVarP(&issueBody, "body", "b", "", "comment "+bodyFlagHelp+"; required") |
| 79 | 79 | ||
| 80 | labelCmd := &cobra.Command{ | 80 | labelCmd := &cobra.Command{ |
| 81 | Use: "label <number>", | 81 | Use: "label <number>", |
| @@ -111,7 +111,20 @@ func init() { | |||
| 111 | RunE: runIssueLabelsList, | 111 | RunE: runIssueLabelsList, |
| 112 | } | 112 | } |
| 113 | 113 | ||
| 114 | - issueCmd.AddCommand(listCmd, viewCmd, createCmd, closeCmd, reopenCmd, commentCmd, labelCmd, milestoneCmd, assignCmd, labelsCmd) | 114 | + deleteCommentCmd := &cobra.Command{ |
| 115 | + Use: "delete-comment <number> <comment-id>", | ||
| 116 | + Aliases: []string{"retract"}, | ||
| 117 | + Short: "Retract a comment on an issue", | ||
| 118 | + Long: "Retract one of an issue's comments.\n\n" + | ||
| 119 | + "Comment ids are shown by `rickub issue view <number>` (and in its --json output).\n" + | ||
| 120 | + "The comment's author, or anyone with write access to the repository, may retract it.\n" + | ||
| 121 | + "The delete is SOFT: the comment survives as a tombstone with its body cleared.", | ||
| 122 | + Args: cobra.ExactArgs(2), | ||
| 123 | + RunE: runIssueDeleteComment, | ||
| 124 | + } | ||
| 125 | + | ||
| 126 | + issueCmd.AddCommand(listCmd, viewCmd, createCmd, closeCmd, reopenCmd, commentCmd, labelCmd, | ||
| 127 | + milestoneCmd, assignCmd, labelsCmd, deleteCommentCmd) | ||
| 115 | rootCmd.AddCommand(issueCmd) | 128 | rootCmd.AddCommand(issueCmd) |
| 116 | } | 129 | } |
| 117 | 130 | ||
| @@ -228,34 +241,17 @@ func runIssueView(cmd *cobra.Command, args []string) error { | |||
| 228 | fmt.Fprintln(out, clean(i.Body)) | 241 | fmt.Fprintln(out, clean(i.Body)) |
| 229 | } | 242 | } |
| 230 | for _, c := range i.Comments { | 243 | for _, c := range i.Comments { |
| 231 | - fmt.Fprintf(out, "\n--- %s (%s) ---\n%s\n", clean(c.Author), humanTime(c.CreatedAt), clean(c.Body)) | 244 | + // The id is the handle `issue delete-comment` takes; print it. |
| 245 | + fmt.Fprintf(out, "\n--- [%s] %s (%s)%s ---\n%s\n", c.ID, clean(c.Author), humanTime(c.CreatedAt), commentMarks(c), clean(c.Body)) | ||
| 232 | } | 246 | } |
| 233 | return nil | 247 | return nil |
| 234 | } | 248 | } |
| 235 | 249 | ||
| 236 | -// bodyOrStdin resolves a --body flag value, reading stdin when it is "-". | ||
| 237 | -func bodyOrStdin(cmd *cobra.Command, body string) (string, error) { | ||
| 238 | - if body != "-" { | ||
| 239 | - return body, nil | ||
| 240 | - } | ||
| 241 | - var sb strings.Builder | ||
| 242 | - buf := make([]byte, 32*1024) | ||
| 243 | - in := cmd.InOrStdin() | ||
| 244 | - for { | ||
| 245 | - n, err := in.Read(buf) | ||
| 246 | - sb.Write(buf[:n]) | ||
| 247 | - if err != nil { | ||
| 248 | - break | ||
| 249 | - } | ||
| 250 | - } | ||
| 251 | - return strings.TrimRight(sb.String(), "\n"), nil | ||
| 252 | -} | ||
| 253 | - | ||
| 254 | func runIssueCreate(cmd *cobra.Command, _ []string) error { | 250 | func runIssueCreate(cmd *cobra.Command, _ []string) error { |
| 255 | if issueTitle == "" { | 251 | if issueTitle == "" { |
| 256 | return fmt.Errorf("--title is required") | 252 | return fmt.Errorf("--title is required") |
| 257 | } | 253 | } |
| 258 | - body, err := bodyOrStdin(cmd, issueBody) | 254 | + body, err := resolveBody(cmd, issueBody) |
| 259 | if err != nil { | 255 | if err != nil { |
| 260 | return err | 256 | return err |
| 261 | } | 257 | } |
| @@ -302,13 +298,34 @@ func runIssueState(cmd *cobra.Command, args []string, state string) error { | |||
| 302 | return nil | 298 | return nil |
| 303 | } | 299 | } |
| 304 | 300 | ||
| 305 | -func runIssueComment(cmd *cobra.Command, args []string) error { | 301 | +func runIssueDeleteComment(cmd *cobra.Command, args []string) error { |
| 306 | - body, err := bodyOrStdin(cmd, issueBody) | 302 | + client, err := newClient() |
| 303 | + if err != nil { | ||
| 304 | + return err | ||
| 305 | + } | ||
| 306 | + owner, repo, err := resolveRepo(issueRepo) | ||
| 307 | if err != nil { | 307 | if err != nil { |
| 308 | return err | 308 | return err |
| 309 | } | 309 | } |
| 310 | - if strings.TrimSpace(body) == "" { | 310 | + n, err := issueNumber(args[0]) |
| 311 | - return fmt.Errorf("--body is required") | 311 | + if err != nil { |
| 312 | + return err | ||
| 313 | + } | ||
| 314 | + id := strings.TrimSpace(args[1]) | ||
| 315 | + if id == "" { | ||
| 316 | + return fmt.Errorf("a comment id is required (see `rickub issue view %d`)", n) | ||
| 317 | + } | ||
| 318 | + if err := client.DeleteIssueComment(cmd.Context(), owner, repo, n, id); err != nil { | ||
| 319 | + return err | ||
| 320 | + } | ||
| 321 | + fmt.Fprintf(cmd.OutOrStdout(), "Retracted comment %s on issue #%d\n", id, n) | ||
| 322 | + return nil | ||
| 323 | +} | ||
| 324 | + | ||
| 325 | +func runIssueComment(cmd *cobra.Command, args []string) error { | ||
| 326 | + body, err := requireBody(cmd, issueBody) | ||
| 327 | + if err != nil { | ||
| 328 | + return err | ||
| 312 | } | 329 | } |
| 313 | client, err := newClient() | 330 | client, err := newClient() |
| 314 | if err != nil { | 331 | if err != nil { |
modified
cmd/pr.go +301 -13 | @@ -3,6 +3,7 @@ package cmd | ||
| 3 | 3 | import ( |
| 4 | 4 | "fmt" |
| 5 | 5 | "strconv" |
| 6 | + "strings" | |
| 6 | 7 | |
| 7 | 8 | "rickub.com/rickub/cli/internal/api" |
| 8 | 9 | |
| @@ -21,6 +22,13 @@ var ( | ||
| 21 | 22 | prMergeMethod string |
| 22 | 23 | prDraft bool |
| 23 | 24 | prReadyUndo bool |
| 25 | + prFile string | |
| 26 | + prLine int | |
| 27 | + prSide string | |
| 28 | + prIntent string | |
| 29 | + prCommit string | |
| 30 | + prReplyTo string | |
| 31 | + prReopen bool | |
| 24 | 32 | ) |
| 25 | 33 | |
| 26 | 34 | func init() { |
| @@ -57,7 +65,7 @@ func init() { | ||
| 57 | 65 | createCmd.Flags().StringVar(&prBase, "base", "", "base branch (required)") |
| 58 | 66 | createCmd.Flags().StringVar(&prHead, "head", "", "head branch (required)") |
| 59 | 67 | createCmd.Flags().StringVarP(&prTitle, "title", "t", "", "title (required)") |
| 60 | - createCmd.Flags().StringVarP(&prBody, "body", "b", "", "description body") | |
| 68 | + createCmd.Flags().StringVarP(&prBody, "body", "b", "", "description "+bodyFlagHelp) | |
| 61 | 69 | createCmd.Flags().StringVar(&prHeadOwner, "head-owner", "", "fork owner for a cross-repo MR") |
| 62 | 70 | createCmd.Flags().StringVar(&prHeadRepo, "head-repo", "", "fork name for a cross-repo MR") |
| 63 | 71 | createCmd.Flags().BoolVar(&prDraft, "draft", false, "open as a draft (not mergeable until `pr ready`)") |
| @@ -92,15 +100,74 @@ func init() { | ||
| 92 | 100 | RunE: runPRReopen, |
| 93 | 101 | } |
| 94 | 102 | |
| 103 | + // `comment` posts a whole-MR conversation comment by default. Adding --file | |
| 104 | + // (and normally --line) anchors it to the diff instead — the no-flag behaviour | |
| 105 | + // is unchanged, so every existing script keeps working. | |
| 95 | 106 | commentCmd := &cobra.Command{ |
| 96 | 107 | Use: "comment <number>", |
| 97 | - Short: "Comment on a merge request", | |
| 98 | - Args: cobra.ExactArgs(1), | |
| 99 | - RunE: runPRComment, | |
| 108 | + Short: "Comment on a merge request, optionally anchored to a file and line", | |
| 109 | + Long: "Comment on a merge request.\n\n" + | |
| 110 | + "With no anchoring flags this posts a comment on the merge request's conversation.\n" + | |
| 111 | + "Pass --file (and --line) to anchor the comment to a line of the diff instead — the\n" + | |
| 112 | + "same line comments the web UI shows beside the code. Line comments require WRITE\n" + | |
| 113 | + "access to the repository; a conversation comment only needs read access.", | |
| 114 | + Example: " rickub mr comment 11 -b \"looks good\"\n" + | |
| 115 | + " rickub mr comment 11 --file web/api.go --line 128 --intent request_change -b \"guard the nil case\"\n" + | |
| 116 | + " rickub mr comment 11 --file web/api.go --line 40 --side old -b \"why was this dropped?\"\n" + | |
| 117 | + " rickub mr comment 11 --reply-to 6f1c… -b \"agreed, fixed in the next push\"", | |
| 118 | + Args: cobra.ExactArgs(1), | |
| 119 | + RunE: runPRComment, | |
| 120 | + } | |
| 121 | + commentCmd.Flags().StringVarP(&prBody, "body", "b", "", "comment "+bodyFlagHelp+"; required") | |
| 122 | + commentCmd.Flags().StringVar(&prFile, "file", "", "anchor the comment to this file in the diff (repo-relative path)") | |
| 123 | + commentCmd.Flags().IntVar(&prLine, "line", 0, "anchor the comment to this line of --file (requires --file)") | |
| 124 | + commentCmd.Flags().StringVar(&prSide, "side", "", "which side of the diff --line refers to: "+api.InlineSideList()+" (default "+api.DefaultInlineSide+")") | |
| 125 | + commentCmd.Flags().StringVar(&prIntent, "intent", "", "stance of a line comment: "+api.InlineIntentList()+" (default "+api.DefaultInlineIntent+")") | |
| 126 | + commentCmd.Flags().StringVar(&prCommit, "commit", "", "commit sha the diff was read at (line comments only)") | |
| 127 | + commentCmd.Flags().StringVar(&prReplyTo, "reply-to", "", "reply to this line comment id; inherits its file/line/side and intent="+api.DefaultInlineIntent) | |
| 128 | + | |
| 129 | + lineCommentsCmd := &cobra.Command{ | |
| 130 | + Use: "line-comments <number>", | |
| 131 | + Aliases: []string{"comments"}, | |
| 132 | + Short: "List a merge request's line-anchored diff comments", | |
| 133 | + Args: cobra.ExactArgs(1), | |
| 134 | + RunE: runPRLineComments, | |
| 135 | + } | |
| 136 | + addPaging(lineCommentsCmd) | |
| 137 | + | |
| 138 | + resolveCmd := &cobra.Command{ | |
| 139 | + Use: "resolve <number> <comment-id>", | |
| 140 | + Short: "Resolve a line comment thread (or reopen it with --reopen)", | |
| 141 | + Args: cobra.ExactArgs(2), | |
| 142 | + RunE: runPRResolve, | |
| 143 | + } | |
| 144 | + resolveCmd.Flags().BoolVar(&prReopen, "reopen", false, "reopen the thread instead of resolving it") | |
| 145 | + | |
| 146 | + deleteCommentCmd := &cobra.Command{ | |
| 147 | + Use: "delete-comment <number> <comment-id>", | |
| 148 | + Aliases: []string{"retract"}, | |
| 149 | + Short: "Retract a conversation comment on a merge request", | |
| 150 | + Long: "Retract one of a merge request's conversation comments.\n\n" + | |
| 151 | + "Comment ids are shown by `rickub mr view <number>` (and in its --json output).\n" + | |
| 152 | + "The comment's author, or anyone with write access to the repository, may retract it.\n" + | |
| 153 | + "The delete is SOFT: the comment survives as a tombstone with its body cleared, so\n" + | |
| 154 | + "the thread still reads correctly — nothing is erased.", | |
| 155 | + Args: cobra.ExactArgs(2), | |
| 156 | + RunE: runPRDeleteComment, | |
| 100 | 157 | } |
| 101 | - commentCmd.Flags().StringVarP(&prBody, "body", "b", "", "comment body (required)") | |
| 102 | 158 | |
| 103 | - prCmd.AddCommand(listCmd, viewCmd, createCmd, readyCmd, mergeCmd, closeCmd, reopenCmd, commentCmd) | |
| 159 | + editCmd := &cobra.Command{ | |
| 160 | + Use: "edit <number>", | |
| 161 | + Short: "Replace a merge request's description", | |
| 162 | + Long: "Replace a merge request's description with --body (or -b - to read it from stdin).\n\n" + | |
| 163 | + "The merge request's author, or anyone with write access to the repository, may edit it.", | |
| 164 | + Args: cobra.ExactArgs(1), | |
| 165 | + RunE: runPREdit, | |
| 166 | + } | |
| 167 | + editCmd.Flags().StringVarP(&prBody, "body", "b", "", "new description "+bodyFlagHelp+"; required") | |
| 168 | + | |
| 169 | + prCmd.AddCommand(listCmd, viewCmd, createCmd, readyCmd, mergeCmd, closeCmd, reopenCmd, commentCmd, | |
| 170 | + lineCommentsCmd, resolveCmd, deleteCommentCmd, editCmd) | |
| 104 | 171 | rootCmd.AddCommand(prCmd) |
| 105 | 172 | } |
| 106 | 173 | |
| @@ -188,7 +255,10 @@ func runPRView(cmd *cobra.Command, args []string) error { | ||
| 188 | 255 | if len(mr.Comments) > 0 { |
| 189 | 256 | fmt.Fprintln(out, "\nComments:") |
| 190 | 257 | for _, c := range mr.Comments { |
| 191 | - fmt.Fprintf(out, " %s (%s):\n %s\n", clean(c.Author), humanTime(c.CreatedAt), clean(c.Body)) | |
| 258 | + // The id is printed because it is the ONLY handle `mr delete-comment` | |
| 259 | + // accepts; a listing that hides it makes retraction impossible to | |
| 260 | + // discover from the terminal (issue #94). | |
| 261 | + fmt.Fprintf(out, " [%s] %s (%s)%s:\n %s\n", c.ID, clean(c.Author), humanTime(c.CreatedAt), commentMarks(c), clean(c.Body)) | |
| 192 | 262 | } |
| 193 | 263 | } |
| 194 | 264 | return nil |
| @@ -203,6 +273,74 @@ func prStateLabel(mr api.MergeRequest) string { | ||
| 203 | 273 | return mr.State |
| 204 | 274 | } |
| 205 | 275 | |
| 276 | +func runPREdit(cmd *cobra.Command, args []string) error { | |
| 277 | + // Same resolver as every other --body: `-b -` reads stdin, and a blank result | |
| 278 | + // is refused rather than silently blanking the description. | |
| 279 | + body, err := requireBody(cmd, prBody) | |
| 280 | + if err != nil { | |
| 281 | + return err | |
| 282 | + } | |
| 283 | + client, err := newClient() | |
| 284 | + if err != nil { | |
| 285 | + return err | |
| 286 | + } | |
| 287 | + owner, repo, err := resolveRepo(prRepo) | |
| 288 | + if err != nil { | |
| 289 | + return err | |
| 290 | + } | |
| 291 | + n, err := prNumber(args[0]) | |
| 292 | + if err != nil { | |
| 293 | + return err | |
| 294 | + } | |
| 295 | + mr, err := client.UpdateMergeRequestBody(cmd.Context(), owner, repo, n, body) | |
| 296 | + if err != nil { | |
| 297 | + return err | |
| 298 | + } | |
| 299 | + if flagJSON { | |
| 300 | + return printJSON(cmd.OutOrStdout(), mr) | |
| 301 | + } | |
| 302 | + // Report the stored length, not just "ok": a success line that does not prove | |
| 303 | + // the bytes landed is exactly what made issue #94 invisible. | |
| 304 | + fmt.Fprintf(cmd.OutOrStdout(), "Updated the description of #%d (%d bytes)\n", n, len(mr.Body)) | |
| 305 | + return nil | |
| 306 | +} | |
| 307 | + | |
| 308 | +func runPRDeleteComment(cmd *cobra.Command, args []string) error { | |
| 309 | + client, err := newClient() | |
| 310 | + if err != nil { | |
| 311 | + return err | |
| 312 | + } | |
| 313 | + owner, repo, err := resolveRepo(prRepo) | |
| 314 | + if err != nil { | |
| 315 | + return err | |
| 316 | + } | |
| 317 | + n, err := prNumber(args[0]) | |
| 318 | + if err != nil { | |
| 319 | + return err | |
| 320 | + } | |
| 321 | + id := strings.TrimSpace(args[1]) | |
| 322 | + if id == "" { | |
| 323 | + return fmt.Errorf("a comment id is required (see `rickub mr view %d`)", n) | |
| 324 | + } | |
| 325 | + if err := client.DeleteMergeRequestComment(cmd.Context(), owner, repo, n, id); err != nil { | |
| 326 | + return err | |
| 327 | + } | |
| 328 | + fmt.Fprintf(cmd.OutOrStdout(), "Retracted comment %s on #%d\n", id, n) | |
| 329 | + return nil | |
| 330 | +} | |
| 331 | + | |
| 332 | +// commentMarks renders a comment's non-body state (edited / retracted) for a | |
| 333 | +// listing. Shared by the merge-request and issue views so the two read alike. | |
| 334 | +func commentMarks(c api.Comment) string { | |
| 335 | + switch { | |
| 336 | + case c.Deleted: | |
| 337 | + return " [deleted]" | |
| 338 | + case c.Edited: | |
| 339 | + return " [edited]" | |
| 340 | + } | |
| 341 | + return "" | |
| 342 | +} | |
| 343 | + | |
| 206 | 344 | func joinSubjects(subs []api.Subject) string { |
| 207 | 345 | out := "" |
| 208 | 346 | for i, s := range subs { |
| @@ -233,11 +371,18 @@ func runPRCreate(cmd *cobra.Command, _ []string) error { | ||
| 233 | 371 | // The API opens a draft unless told otherwise; like gh, the CLI opens a |
| 234 | 372 | // ready merge request by default and --draft opts in. |
| 235 | 373 | draft := prDraft |
| 374 | + // A merge-request description is optional, so resolveBody (not requireBody): | |
| 375 | + // but `-b -` must still mean stdin here, or the same silent-truncation trap | |
| 376 | + // reopens on a different command. | |
| 377 | + body, err := resolveBody(cmd, prBody) | |
| 378 | + if err != nil { | |
| 379 | + return err | |
| 380 | + } | |
| 236 | 381 | mr, err := client.CreateMergeRequest(cmd.Context(), owner, repo, api.MergeRequestCreate{ |
| 237 | 382 | Base: prBase, |
| 238 | 383 | Head: prHead, |
| 239 | 384 | Title: prTitle, |
| 240 | - Body: prBody, | |
| 385 | + Body: body, | |
| 241 | 386 | HeadOwner: prHeadOwner, |
| 242 | 387 | HeadRepo: prHeadRepo, |
| 243 | 388 | Draft: &draft, |
| @@ -369,16 +514,159 @@ func runPRComment(cmd *cobra.Command, args []string) error { | ||
| 369 | 514 | if err != nil { |
| 370 | 515 | return err |
| 371 | 516 | } |
| 372 | - if prBody == "" { | |
| 373 | - return fmt.Errorf("--body is required") | |
| 517 | + // ONE body parser, shared with `issue comment` (cmd/body.go). Before this, | |
| 518 | + // `mr comment -b -` posted the literal string "-" and reported success, | |
| 519 | + // silently destroying whatever was piped in — issue #94. | |
| 520 | + body, err := requireBody(cmd, prBody) | |
| 521 | + if err != nil { | |
| 522 | + return err | |
| 523 | + } | |
| 524 | + anchored, err := lineCommentInput() | |
| 525 | + if err != nil { | |
| 526 | + return err | |
| 527 | + } | |
| 528 | + if anchored == nil { | |
| 529 | + // No anchoring flags: the original whole-MR conversation comment, unchanged. | |
| 530 | + c, err := client.CommentMergeRequest(cmd.Context(), owner, repo, n, body) | |
| 531 | + if err != nil { | |
| 532 | + return err | |
| 533 | + } | |
| 534 | + if flagJSON { | |
| 535 | + return printJSON(cmd.OutOrStdout(), c) | |
| 536 | + } | |
| 537 | + fmt.Fprintf(cmd.OutOrStdout(), "Commented on #%d\n", n) | |
| 538 | + return nil | |
| 374 | 539 | } |
| 375 | - c, err := client.CommentMergeRequest(cmd.Context(), owner, repo, n, prBody) | |
| 540 | + anchored.Body = body | |
| 541 | + lc, err := client.CreateLineComment(cmd.Context(), owner, repo, n, *anchored) | |
| 376 | 542 | if err != nil { |
| 377 | 543 | return err |
| 378 | 544 | } |
| 379 | 545 | if flagJSON { |
| 380 | - return printJSON(cmd.OutOrStdout(), c) | |
| 546 | + return printJSON(cmd.OutOrStdout(), lc) | |
| 547 | + } | |
| 548 | + fmt.Fprintf(cmd.OutOrStdout(), "Commented on #%d at %s:%d (%s, %s)\n", n, lc.File, lc.Line, lc.Side, lc.Intent) | |
| 549 | + return nil | |
| 550 | +} | |
| 551 | + | |
| 552 | +// lineCommentInput turns the anchoring flags into a create body, or nil when none | |
| 553 | +// were given (meaning: post a plain conversation comment, the pre-existing | |
| 554 | +// behaviour). Every invalid combination is rejected HERE, before a request is | |
| 555 | +// made, so the user gets "--line requires --file" rather than a server 400. | |
| 556 | +func lineCommentInput() (*api.LineCommentCreate, error) { | |
| 557 | + if prReplyTo != "" { | |
| 558 | + // A reply inherits its parent's anchor and intent; accepting anchoring | |
| 559 | + // flags alongside it would silently ignore them. | |
| 560 | + if prFile != "" || prLine != 0 || prSide != "" || prIntent != "" { | |
| 561 | + return nil, fmt.Errorf("--reply-to inherits the parent comment's file, line, side and intent — drop --file/--line/--side/--intent") | |
| 562 | + } | |
| 563 | + return &api.LineCommentCreate{ParentID: prReplyTo, Commit: prCommit}, nil | |
| 564 | + } | |
| 565 | + if prFile == "" { | |
| 566 | + if prLine != 0 { | |
| 567 | + return nil, fmt.Errorf("--line requires --file: a line number needs a file to anchor to") | |
| 568 | + } | |
| 569 | + if prSide != "" || prIntent != "" || prCommit != "" { | |
| 570 | + return nil, fmt.Errorf("--side, --intent and --commit only apply to a line comment — add --file and --line") | |
| 571 | + } | |
| 572 | + return nil, nil // plain conversation comment | |
| 573 | + } | |
| 574 | + if prLine <= 0 { | |
| 575 | + return nil, fmt.Errorf("--file requires --line (a positive line number in the diff)") | |
| 576 | + } | |
| 577 | + if prSide != "" && !api.ValidInlineSide(prSide) { | |
| 578 | + return nil, fmt.Errorf("invalid --side %q: want %s", prSide, api.InlineSideList()) | |
| 579 | + } | |
| 580 | + if prIntent != "" && !api.ValidInlineIntent(prIntent) { | |
| 581 | + return nil, fmt.Errorf("invalid --intent %q: want %s", prIntent, api.InlineIntentList()) | |
| 582 | + } | |
| 583 | + return &api.LineCommentCreate{ | |
| 584 | + File: prFile, Line: prLine, Side: prSide, Intent: prIntent, Commit: prCommit, | |
| 585 | + }, nil | |
| 586 | +} | |
| 587 | + | |
| 588 | +func runPRLineComments(cmd *cobra.Command, args []string) error { | |
| 589 | + client, err := newClient() | |
| 590 | + if err != nil { | |
| 591 | + return err | |
| 592 | + } | |
| 593 | + owner, repo, err := resolveRepo(prRepo) | |
| 594 | + if err != nil { | |
| 595 | + return err | |
| 596 | + } | |
| 597 | + n, err := prNumber(args[0]) | |
| 598 | + if err != nil { | |
| 599 | + return err | |
| 600 | + } | |
| 601 | + page, err := client.ListLineComments(cmd.Context(), owner, repo, n, pageFlag, perPageFlag) | |
| 602 | + if err != nil { | |
| 603 | + return err | |
| 604 | + } | |
| 605 | + if flagJSON { | |
| 606 | + return printJSON(cmd.OutOrStdout(), page) | |
| 607 | + } | |
| 608 | + if len(page.Items) == 0 { | |
| 609 | + fmt.Fprintln(cmd.OutOrStdout(), "No line comments.") | |
| 610 | + return nil | |
| 611 | + } | |
| 612 | + tw := newTabw(cmd.OutOrStdout()) | |
| 613 | + fmt.Fprintln(tw, "ID\tANCHOR\tINTENT\tSTATE\tAUTHOR\tBODY") | |
| 614 | + for _, lc := range page.Items { | |
| 615 | + state := "open" | |
| 616 | + if lc.Resolved { | |
| 617 | + state = "resolved" | |
| 618 | + } | |
| 619 | + anchor := fmt.Sprintf("%s:%d (%s)", lc.File, lc.Line, lc.Side) | |
| 620 | + kind := lc.Intent | |
| 621 | + if lc.ParentID != "" { | |
| 622 | + kind = "reply" | |
| 623 | + } | |
| 624 | + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\n", lc.ID, anchor, kind, state, lc.Author, firstLine(lc.Body)) | |
| 625 | + } | |
| 626 | + if err := tw.Flush(); err != nil { | |
| 627 | + return err | |
| 628 | + } | |
| 629 | + printPageFooter(cmd, page.Page) | |
| 630 | + return nil | |
| 631 | +} | |
| 632 | + | |
| 633 | +// firstLine keeps a table row one row tall: a multi-line comment body shows its | |
| 634 | +// first line with an ellipsis rather than shredding the column alignment. | |
| 635 | +func firstLine(s string) string { | |
| 636 | + if i := strings.IndexAny(s, "\r\n"); i >= 0 { | |
| 637 | + return strings.TrimSpace(s[:i]) + " …" | |
| 638 | + } | |
| 639 | + return s | |
| 640 | +} | |
| 641 | + | |
| 642 | +func runPRResolve(cmd *cobra.Command, args []string) error { | |
| 643 | + client, err := newClient() | |
| 644 | + if err != nil { | |
| 645 | + return err | |
| 646 | + } | |
| 647 | + owner, repo, err := resolveRepo(prRepo) | |
| 648 | + if err != nil { | |
| 649 | + return err | |
| 650 | + } | |
| 651 | + n, err := prNumber(args[0]) | |
| 652 | + if err != nil { | |
| 653 | + return err | |
| 654 | + } | |
| 655 | + id := strings.TrimSpace(args[1]) | |
| 656 | + if id == "" { | |
| 657 | + return fmt.Errorf("a line comment id is required (see `rickub mr line-comments %d`)", n) | |
| 658 | + } | |
| 659 | + lc, err := client.ResolveLineComment(cmd.Context(), owner, repo, n, id, !prReopen) | |
| 660 | + if err != nil { | |
| 661 | + return err | |
| 662 | + } | |
| 663 | + if flagJSON { | |
| 664 | + return printJSON(cmd.OutOrStdout(), lc) | |
| 665 | + } | |
| 666 | + verb := "Resolved" | |
| 667 | + if prReopen { | |
| 668 | + verb = "Reopened" | |
| 381 | 669 | } |
| 382 | - fmt.Fprintf(cmd.OutOrStdout(), "Commented on #%d\n", n) | |
| 670 | + fmt.Fprintf(cmd.OutOrStdout(), "%s thread on %s:%d in #%d\n", verb, lc.File, lc.Line, n) | |
| 383 | 671 | return nil |
| 384 | 672 | } |
| @@ -3,6 +3,7 @@ package cmd | |||
| 3 | import ( | 3 | import ( |
| 4 | "fmt" | 4 | "fmt" |
| 5 | "strconv" | 5 | "strconv" |
| 6 | + "strings" | ||
| 6 | 7 | ||
| 7 | "rickub.com/rickub/cli/internal/api" | 8 | "rickub.com/rickub/cli/internal/api" |
| 8 | 9 | ||
| @@ -21,6 +22,13 @@ var ( | |||
| 21 | prMergeMethod string | 22 | prMergeMethod string |
| 22 | prDraft bool | 23 | prDraft bool |
| 23 | prReadyUndo bool | 24 | prReadyUndo bool |
| 25 | + prFile string | ||
| 26 | + prLine int | ||
| 27 | + prSide string | ||
| 28 | + prIntent string | ||
| 29 | + prCommit string | ||
| 30 | + prReplyTo string | ||
| 31 | + prReopen bool | ||
| 24 | ) | 32 | ) |
| 25 | 33 | ||
| 26 | func init() { | 34 | func init() { |
| @@ -57,7 +65,7 @@ func init() { | |||
| 57 | createCmd.Flags().StringVar(&prBase, "base", "", "base branch (required)") | 65 | createCmd.Flags().StringVar(&prBase, "base", "", "base branch (required)") |
| 58 | createCmd.Flags().StringVar(&prHead, "head", "", "head branch (required)") | 66 | createCmd.Flags().StringVar(&prHead, "head", "", "head branch (required)") |
| 59 | createCmd.Flags().StringVarP(&prTitle, "title", "t", "", "title (required)") | 67 | createCmd.Flags().StringVarP(&prTitle, "title", "t", "", "title (required)") |
| 60 | - createCmd.Flags().StringVarP(&prBody, "body", "b", "", "description body") | 68 | + createCmd.Flags().StringVarP(&prBody, "body", "b", "", "description "+bodyFlagHelp) |
| 61 | createCmd.Flags().StringVar(&prHeadOwner, "head-owner", "", "fork owner for a cross-repo MR") | 69 | createCmd.Flags().StringVar(&prHeadOwner, "head-owner", "", "fork owner for a cross-repo MR") |
| 62 | createCmd.Flags().StringVar(&prHeadRepo, "head-repo", "", "fork name for a cross-repo MR") | 70 | createCmd.Flags().StringVar(&prHeadRepo, "head-repo", "", "fork name for a cross-repo MR") |
| 63 | createCmd.Flags().BoolVar(&prDraft, "draft", false, "open as a draft (not mergeable until `pr ready`)") | 71 | createCmd.Flags().BoolVar(&prDraft, "draft", false, "open as a draft (not mergeable until `pr ready`)") |
| @@ -92,15 +100,74 @@ func init() { | |||
| 92 | RunE: runPRReopen, | 100 | RunE: runPRReopen, |
| 93 | } | 101 | } |
| 94 | 102 | ||
| 103 | + // `comment` posts a whole-MR conversation comment by default. Adding --file | ||
| 104 | + // (and normally --line) anchors it to the diff instead — the no-flag behaviour | ||
| 105 | + // is unchanged, so every existing script keeps working. | ||
| 95 | commentCmd := &cobra.Command{ | 106 | commentCmd := &cobra.Command{ |
| 96 | Use: "comment <number>", | 107 | Use: "comment <number>", |
| 97 | - Short: "Comment on a merge request", | 108 | + Short: "Comment on a merge request, optionally anchored to a file and line", |
| 98 | - Args: cobra.ExactArgs(1), | 109 | + Long: "Comment on a merge request.\n\n" + |
| 99 | - RunE: runPRComment, | 110 | + "With no anchoring flags this posts a comment on the merge request's conversation.\n" + |
| 111 | + "Pass --file (and --line) to anchor the comment to a line of the diff instead — the\n" + | ||
| 112 | + "same line comments the web UI shows beside the code. Line comments require WRITE\n" + | ||
| 113 | + "access to the repository; a conversation comment only needs read access.", | ||
| 114 | + Example: " rickub mr comment 11 -b \"looks good\"\n" + | ||
| 115 | + " rickub mr comment 11 --file web/api.go --line 128 --intent request_change -b \"guard the nil case\"\n" + | ||
| 116 | + " rickub mr comment 11 --file web/api.go --line 40 --side old -b \"why was this dropped?\"\n" + | ||
| 117 | + " rickub mr comment 11 --reply-to 6f1c… -b \"agreed, fixed in the next push\"", | ||
| 118 | + Args: cobra.ExactArgs(1), | ||
| 119 | + RunE: runPRComment, | ||
| 120 | + } | ||
| 121 | + commentCmd.Flags().StringVarP(&prBody, "body", "b", "", "comment "+bodyFlagHelp+"; required") | ||
| 122 | + commentCmd.Flags().StringVar(&prFile, "file", "", "anchor the comment to this file in the diff (repo-relative path)") | ||
| 123 | + commentCmd.Flags().IntVar(&prLine, "line", 0, "anchor the comment to this line of --file (requires --file)") | ||
| 124 | + commentCmd.Flags().StringVar(&prSide, "side", "", "which side of the diff --line refers to: "+api.InlineSideList()+" (default "+api.DefaultInlineSide+")") | ||
| 125 | + commentCmd.Flags().StringVar(&prIntent, "intent", "", "stance of a line comment: "+api.InlineIntentList()+" (default "+api.DefaultInlineIntent+")") | ||
| 126 | + commentCmd.Flags().StringVar(&prCommit, "commit", "", "commit sha the diff was read at (line comments only)") | ||
| 127 | + commentCmd.Flags().StringVar(&prReplyTo, "reply-to", "", "reply to this line comment id; inherits its file/line/side and intent="+api.DefaultInlineIntent) | ||
| 128 | + | ||
| 129 | + lineCommentsCmd := &cobra.Command{ | ||
| 130 | + Use: "line-comments <number>", | ||
| 131 | + Aliases: []string{"comments"}, | ||
| 132 | + Short: "List a merge request's line-anchored diff comments", | ||
| 133 | + Args: cobra.ExactArgs(1), | ||
| 134 | + RunE: runPRLineComments, | ||
| 135 | + } | ||
| 136 | + addPaging(lineCommentsCmd) | ||
| 137 | + | ||
| 138 | + resolveCmd := &cobra.Command{ | ||
| 139 | + Use: "resolve <number> <comment-id>", | ||
| 140 | + Short: "Resolve a line comment thread (or reopen it with --reopen)", | ||
| 141 | + Args: cobra.ExactArgs(2), | ||
| 142 | + RunE: runPRResolve, | ||
| 143 | + } | ||
| 144 | + resolveCmd.Flags().BoolVar(&prReopen, "reopen", false, "reopen the thread instead of resolving it") | ||
| 145 | + | ||
| 146 | + deleteCommentCmd := &cobra.Command{ | ||
| 147 | + Use: "delete-comment <number> <comment-id>", | ||
| 148 | + Aliases: []string{"retract"}, | ||
| 149 | + Short: "Retract a conversation comment on a merge request", | ||
| 150 | + Long: "Retract one of a merge request's conversation comments.\n\n" + | ||
| 151 | + "Comment ids are shown by `rickub mr view <number>` (and in its --json output).\n" + | ||
| 152 | + "The comment's author, or anyone with write access to the repository, may retract it.\n" + | ||
| 153 | + "The delete is SOFT: the comment survives as a tombstone with its body cleared, so\n" + | ||
| 154 | + "the thread still reads correctly — nothing is erased.", | ||
| 155 | + Args: cobra.ExactArgs(2), | ||
| 156 | + RunE: runPRDeleteComment, | ||
| 100 | } | 157 | } |
| 101 | - commentCmd.Flags().StringVarP(&prBody, "body", "b", "", "comment body (required)") | ||
| 102 | 158 | ||
| 103 | - prCmd.AddCommand(listCmd, viewCmd, createCmd, readyCmd, mergeCmd, closeCmd, reopenCmd, commentCmd) | 159 | + editCmd := &cobra.Command{ |
| 160 | + Use: "edit <number>", | ||
| 161 | + Short: "Replace a merge request's description", | ||
| 162 | + Long: "Replace a merge request's description with --body (or -b - to read it from stdin).\n\n" + | ||
| 163 | + "The merge request's author, or anyone with write access to the repository, may edit it.", | ||
| 164 | + Args: cobra.ExactArgs(1), | ||
| 165 | + RunE: runPREdit, | ||
| 166 | + } | ||
| 167 | + editCmd.Flags().StringVarP(&prBody, "body", "b", "", "new description "+bodyFlagHelp+"; required") | ||
| 168 | + | ||
| 169 | + prCmd.AddCommand(listCmd, viewCmd, createCmd, readyCmd, mergeCmd, closeCmd, reopenCmd, commentCmd, | ||
| 170 | + lineCommentsCmd, resolveCmd, deleteCommentCmd, editCmd) | ||
| 104 | rootCmd.AddCommand(prCmd) | 171 | rootCmd.AddCommand(prCmd) |
| 105 | } | 172 | } |
| 106 | 173 | ||
| @@ -188,7 +255,10 @@ func runPRView(cmd *cobra.Command, args []string) error { | |||
| 188 | if len(mr.Comments) > 0 { | 255 | if len(mr.Comments) > 0 { |
| 189 | fmt.Fprintln(out, "\nComments:") | 256 | fmt.Fprintln(out, "\nComments:") |
| 190 | for _, c := range mr.Comments { | 257 | for _, c := range mr.Comments { |
| 191 | - fmt.Fprintf(out, " %s (%s):\n %s\n", clean(c.Author), humanTime(c.CreatedAt), clean(c.Body)) | 258 | + // The id is printed because it is the ONLY handle `mr delete-comment` |
| 259 | + // accepts; a listing that hides it makes retraction impossible to | ||
| 260 | + // discover from the terminal (issue #94). | ||
| 261 | + fmt.Fprintf(out, " [%s] %s (%s)%s:\n %s\n", c.ID, clean(c.Author), humanTime(c.CreatedAt), commentMarks(c), clean(c.Body)) | ||
| 192 | } | 262 | } |
| 193 | } | 263 | } |
| 194 | return nil | 264 | return nil |
| @@ -203,6 +273,74 @@ func prStateLabel(mr api.MergeRequest) string { | |||
| 203 | return mr.State | 273 | return mr.State |
| 204 | } | 274 | } |
| 205 | 275 | ||
| 276 | +func runPREdit(cmd *cobra.Command, args []string) error { | ||
| 277 | + // Same resolver as every other --body: `-b -` reads stdin, and a blank result | ||
| 278 | + // is refused rather than silently blanking the description. | ||
| 279 | + body, err := requireBody(cmd, prBody) | ||
| 280 | + if err != nil { | ||
| 281 | + return err | ||
| 282 | + } | ||
| 283 | + client, err := newClient() | ||
| 284 | + if err != nil { | ||
| 285 | + return err | ||
| 286 | + } | ||
| 287 | + owner, repo, err := resolveRepo(prRepo) | ||
| 288 | + if err != nil { | ||
| 289 | + return err | ||
| 290 | + } | ||
| 291 | + n, err := prNumber(args[0]) | ||
| 292 | + if err != nil { | ||
| 293 | + return err | ||
| 294 | + } | ||
| 295 | + mr, err := client.UpdateMergeRequestBody(cmd.Context(), owner, repo, n, body) | ||
| 296 | + if err != nil { | ||
| 297 | + return err | ||
| 298 | + } | ||
| 299 | + if flagJSON { | ||
| 300 | + return printJSON(cmd.OutOrStdout(), mr) | ||
| 301 | + } | ||
| 302 | + // Report the stored length, not just "ok": a success line that does not prove | ||
| 303 | + // the bytes landed is exactly what made issue #94 invisible. | ||
| 304 | + fmt.Fprintf(cmd.OutOrStdout(), "Updated the description of #%d (%d bytes)\n", n, len(mr.Body)) | ||
| 305 | + return nil | ||
| 306 | +} | ||
| 307 | + | ||
| 308 | +func runPRDeleteComment(cmd *cobra.Command, args []string) error { | ||
| 309 | + client, err := newClient() | ||
| 310 | + if err != nil { | ||
| 311 | + return err | ||
| 312 | + } | ||
| 313 | + owner, repo, err := resolveRepo(prRepo) | ||
| 314 | + if err != nil { | ||
| 315 | + return err | ||
| 316 | + } | ||
| 317 | + n, err := prNumber(args[0]) | ||
| 318 | + if err != nil { | ||
| 319 | + return err | ||
| 320 | + } | ||
| 321 | + id := strings.TrimSpace(args[1]) | ||
| 322 | + if id == "" { | ||
| 323 | + return fmt.Errorf("a comment id is required (see `rickub mr view %d`)", n) | ||
| 324 | + } | ||
| 325 | + if err := client.DeleteMergeRequestComment(cmd.Context(), owner, repo, n, id); err != nil { | ||
| 326 | + return err | ||
| 327 | + } | ||
| 328 | + fmt.Fprintf(cmd.OutOrStdout(), "Retracted comment %s on #%d\n", id, n) | ||
| 329 | + return nil | ||
| 330 | +} | ||
| 331 | + | ||
| 332 | +// commentMarks renders a comment's non-body state (edited / retracted) for a | ||
| 333 | +// listing. Shared by the merge-request and issue views so the two read alike. | ||
| 334 | +func commentMarks(c api.Comment) string { | ||
| 335 | + switch { | ||
| 336 | + case c.Deleted: | ||
| 337 | + return " [deleted]" | ||
| 338 | + case c.Edited: | ||
| 339 | + return " [edited]" | ||
| 340 | + } | ||
| 341 | + return "" | ||
| 342 | +} | ||
| 343 | + | ||
| 206 | func joinSubjects(subs []api.Subject) string { | 344 | func joinSubjects(subs []api.Subject) string { |
| 207 | out := "" | 345 | out := "" |
| 208 | for i, s := range subs { | 346 | for i, s := range subs { |
| @@ -233,11 +371,18 @@ func runPRCreate(cmd *cobra.Command, _ []string) error { | |||
| 233 | // The API opens a draft unless told otherwise; like gh, the CLI opens a | 371 | // The API opens a draft unless told otherwise; like gh, the CLI opens a |
| 234 | // ready merge request by default and --draft opts in. | 372 | // ready merge request by default and --draft opts in. |
| 235 | draft := prDraft | 373 | draft := prDraft |
| 374 | + // A merge-request description is optional, so resolveBody (not requireBody): | ||
| 375 | + // but `-b -` must still mean stdin here, or the same silent-truncation trap | ||
| 376 | + // reopens on a different command. | ||
| 377 | + body, err := resolveBody(cmd, prBody) | ||
| 378 | + if err != nil { | ||
| 379 | + return err | ||
| 380 | + } | ||
| 236 | mr, err := client.CreateMergeRequest(cmd.Context(), owner, repo, api.MergeRequestCreate{ | 381 | mr, err := client.CreateMergeRequest(cmd.Context(), owner, repo, api.MergeRequestCreate{ |
| 237 | Base: prBase, | 382 | Base: prBase, |
| 238 | Head: prHead, | 383 | Head: prHead, |
| 239 | Title: prTitle, | 384 | Title: prTitle, |
| 240 | - Body: prBody, | 385 | + Body: body, |
| 241 | HeadOwner: prHeadOwner, | 386 | HeadOwner: prHeadOwner, |
| 242 | HeadRepo: prHeadRepo, | 387 | HeadRepo: prHeadRepo, |
| 243 | Draft: &draft, | 388 | Draft: &draft, |
| @@ -369,16 +514,159 @@ func runPRComment(cmd *cobra.Command, args []string) error { | |||
| 369 | if err != nil { | 514 | if err != nil { |
| 370 | return err | 515 | return err |
| 371 | } | 516 | } |
| 372 | - if prBody == "" { | 517 | + // ONE body parser, shared with `issue comment` (cmd/body.go). Before this, |
| 373 | - return fmt.Errorf("--body is required") | 518 | + // `mr comment -b -` posted the literal string "-" and reported success, |
| 519 | + // silently destroying whatever was piped in — issue #94. | ||
| 520 | + body, err := requireBody(cmd, prBody) | ||
| 521 | + if err != nil { | ||
| 522 | + return err | ||
| 523 | + } | ||
| 524 | + anchored, err := lineCommentInput() | ||
| 525 | + if err != nil { | ||
| 526 | + return err | ||
| 527 | + } | ||
| 528 | + if anchored == nil { | ||
| 529 | + // No anchoring flags: the original whole-MR conversation comment, unchanged. | ||
| 530 | + c, err := client.CommentMergeRequest(cmd.Context(), owner, repo, n, body) | ||
| 531 | + if err != nil { | ||
| 532 | + return err | ||
| 533 | + } | ||
| 534 | + if flagJSON { | ||
| 535 | + return printJSON(cmd.OutOrStdout(), c) | ||
| 536 | + } | ||
| 537 | + fmt.Fprintf(cmd.OutOrStdout(), "Commented on #%d\n", n) | ||
| 538 | + return nil | ||
| 374 | } | 539 | } |
| 375 | - c, err := client.CommentMergeRequest(cmd.Context(), owner, repo, n, prBody) | 540 | + anchored.Body = body |
| 541 | + lc, err := client.CreateLineComment(cmd.Context(), owner, repo, n, *anchored) | ||
| 376 | if err != nil { | 542 | if err != nil { |
| 377 | return err | 543 | return err |
| 378 | } | 544 | } |
| 379 | if flagJSON { | 545 | if flagJSON { |
| 380 | - return printJSON(cmd.OutOrStdout(), c) | 546 | + return printJSON(cmd.OutOrStdout(), lc) |
| 547 | + } | ||
| 548 | + fmt.Fprintf(cmd.OutOrStdout(), "Commented on #%d at %s:%d (%s, %s)\n", n, lc.File, lc.Line, lc.Side, lc.Intent) | ||
| 549 | + return nil | ||
| 550 | +} | ||
| 551 | + | ||
| 552 | +// lineCommentInput turns the anchoring flags into a create body, or nil when none | ||
| 553 | +// were given (meaning: post a plain conversation comment, the pre-existing | ||
| 554 | +// behaviour). Every invalid combination is rejected HERE, before a request is | ||
| 555 | +// made, so the user gets "--line requires --file" rather than a server 400. | ||
| 556 | +func lineCommentInput() (*api.LineCommentCreate, error) { | ||
| 557 | + if prReplyTo != "" { | ||
| 558 | + // A reply inherits its parent's anchor and intent; accepting anchoring | ||
| 559 | + // flags alongside it would silently ignore them. | ||
| 560 | + if prFile != "" || prLine != 0 || prSide != "" || prIntent != "" { | ||
| 561 | + return nil, fmt.Errorf("--reply-to inherits the parent comment's file, line, side and intent — drop --file/--line/--side/--intent") | ||
| 562 | + } | ||
| 563 | + return &api.LineCommentCreate{ParentID: prReplyTo, Commit: prCommit}, nil | ||
| 564 | + } | ||
| 565 | + if prFile == "" { | ||
| 566 | + if prLine != 0 { | ||
| 567 | + return nil, fmt.Errorf("--line requires --file: a line number needs a file to anchor to") | ||
| 568 | + } | ||
| 569 | + if prSide != "" || prIntent != "" || prCommit != "" { | ||
| 570 | + return nil, fmt.Errorf("--side, --intent and --commit only apply to a line comment — add --file and --line") | ||
| 571 | + } | ||
| 572 | + return nil, nil // plain conversation comment | ||
| 573 | + } | ||
| 574 | + if prLine <= 0 { | ||
| 575 | + return nil, fmt.Errorf("--file requires --line (a positive line number in the diff)") | ||
| 576 | + } | ||
| 577 | + if prSide != "" && !api.ValidInlineSide(prSide) { | ||
| 578 | + return nil, fmt.Errorf("invalid --side %q: want %s", prSide, api.InlineSideList()) | ||
| 579 | + } | ||
| 580 | + if prIntent != "" && !api.ValidInlineIntent(prIntent) { | ||
| 581 | + return nil, fmt.Errorf("invalid --intent %q: want %s", prIntent, api.InlineIntentList()) | ||
| 582 | + } | ||
| 583 | + return &api.LineCommentCreate{ | ||
| 584 | + File: prFile, Line: prLine, Side: prSide, Intent: prIntent, Commit: prCommit, | ||
| 585 | + }, nil | ||
| 586 | +} | ||
| 587 | + | ||
| 588 | +func runPRLineComments(cmd *cobra.Command, args []string) error { | ||
| 589 | + client, err := newClient() | ||
| 590 | + if err != nil { | ||
| 591 | + return err | ||
| 592 | + } | ||
| 593 | + owner, repo, err := resolveRepo(prRepo) | ||
| 594 | + if err != nil { | ||
| 595 | + return err | ||
| 596 | + } | ||
| 597 | + n, err := prNumber(args[0]) | ||
| 598 | + if err != nil { | ||
| 599 | + return err | ||
| 600 | + } | ||
| 601 | + page, err := client.ListLineComments(cmd.Context(), owner, repo, n, pageFlag, perPageFlag) | ||
| 602 | + if err != nil { | ||
| 603 | + return err | ||
| 604 | + } | ||
| 605 | + if flagJSON { | ||
| 606 | + return printJSON(cmd.OutOrStdout(), page) | ||
| 607 | + } | ||
| 608 | + if len(page.Items) == 0 { | ||
| 609 | + fmt.Fprintln(cmd.OutOrStdout(), "No line comments.") | ||
| 610 | + return nil | ||
| 611 | + } | ||
| 612 | + tw := newTabw(cmd.OutOrStdout()) | ||
| 613 | + fmt.Fprintln(tw, "ID\tANCHOR\tINTENT\tSTATE\tAUTHOR\tBODY") | ||
| 614 | + for _, lc := range page.Items { | ||
| 615 | + state := "open" | ||
| 616 | + if lc.Resolved { | ||
| 617 | + state = "resolved" | ||
| 618 | + } | ||
| 619 | + anchor := fmt.Sprintf("%s:%d (%s)", lc.File, lc.Line, lc.Side) | ||
| 620 | + kind := lc.Intent | ||
| 621 | + if lc.ParentID != "" { | ||
| 622 | + kind = "reply" | ||
| 623 | + } | ||
| 624 | + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\n", lc.ID, anchor, kind, state, lc.Author, firstLine(lc.Body)) | ||
| 625 | + } | ||
| 626 | + if err := tw.Flush(); err != nil { | ||
| 627 | + return err | ||
| 628 | + } | ||
| 629 | + printPageFooter(cmd, page.Page) | ||
| 630 | + return nil | ||
| 631 | +} | ||
| 632 | + | ||
| 633 | +// firstLine keeps a table row one row tall: a multi-line comment body shows its | ||
| 634 | +// first line with an ellipsis rather than shredding the column alignment. | ||
| 635 | +func firstLine(s string) string { | ||
| 636 | + if i := strings.IndexAny(s, "\r\n"); i >= 0 { | ||
| 637 | + return strings.TrimSpace(s[:i]) + " …" | ||
| 638 | + } | ||
| 639 | + return s | ||
| 640 | +} | ||
| 641 | + | ||
| 642 | +func runPRResolve(cmd *cobra.Command, args []string) error { | ||
| 643 | + client, err := newClient() | ||
| 644 | + if err != nil { | ||
| 645 | + return err | ||
| 646 | + } | ||
| 647 | + owner, repo, err := resolveRepo(prRepo) | ||
| 648 | + if err != nil { | ||
| 649 | + return err | ||
| 650 | + } | ||
| 651 | + n, err := prNumber(args[0]) | ||
| 652 | + if err != nil { | ||
| 653 | + return err | ||
| 654 | + } | ||
| 655 | + id := strings.TrimSpace(args[1]) | ||
| 656 | + if id == "" { | ||
| 657 | + return fmt.Errorf("a line comment id is required (see `rickub mr line-comments %d`)", n) | ||
| 658 | + } | ||
| 659 | + lc, err := client.ResolveLineComment(cmd.Context(), owner, repo, n, id, !prReopen) | ||
| 660 | + if err != nil { | ||
| 661 | + return err | ||
| 662 | + } | ||
| 663 | + if flagJSON { | ||
| 664 | + return printJSON(cmd.OutOrStdout(), lc) | ||
| 665 | + } | ||
| 666 | + verb := "Resolved" | ||
| 667 | + if prReopen { | ||
| 668 | + verb = "Reopened" | ||
| 381 | } | 669 | } |
| 382 | - fmt.Fprintf(cmd.OutOrStdout(), "Commented on #%d\n", n) | 670 | + fmt.Fprintf(cmd.OutOrStdout(), "%s thread on %s:%d in #%d\n", verb, lc.File, lc.Line, n) |
| 383 | return nil | 671 | return nil |
| 384 | } | 672 | } |
added
cmd/pr_line_comments_test.go +350 -0 | new file mode 100644 | ||
| @@ -0,0 +1,350 @@ | ||
| 1 | +package cmd | |
| 2 | + | |
| 3 | +import ( | |
| 4 | + "encoding/json" | |
| 5 | + "fmt" | |
| 6 | + "io" | |
| 7 | + "net/http" | |
| 8 | + "net/http/httptest" | |
| 9 | + "strings" | |
| 10 | + "testing" | |
| 11 | +) | |
| 12 | + | |
| 13 | +// captured is what a fake server recorded about the request the CLI made. | |
| 14 | +type captured struct { | |
| 15 | + Method string | |
| 16 | + Path string | |
| 17 | + Body map[string]any | |
| 18 | +} | |
| 19 | + | |
| 20 | +// lineCommentServer answers the merge-request comment endpoints and records what | |
| 21 | +// was called, so a test can assert WHICH surface the CLI chose (the whole-MR | |
| 22 | +// /comments endpoint or the anchored /line-comments one). | |
| 23 | +func lineCommentServer(t *testing.T, seen *captured) *httptest.Server { | |
| 24 | + t.Helper() | |
| 25 | + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 26 | + seen.Method, seen.Path = r.Method, r.URL.Path | |
| 27 | + if b, _ := io.ReadAll(r.Body); len(b) > 0 { | |
| 28 | + seen.Body = map[string]any{} | |
| 29 | + _ = json.Unmarshal(b, &seen.Body) | |
| 30 | + } | |
| 31 | + switch { | |
| 32 | + case strings.HasSuffix(r.URL.Path, "/line-comments") && r.Method == http.MethodGet: | |
| 33 | + _ = json.NewEncoder(w).Encode(map[string]any{ | |
| 34 | + "page": 1, "per_page": 30, "has_next": false, | |
| 35 | + "items": []map[string]any{ | |
| 36 | + {"id": "abc-1", "author": "rick", "file": "web/api.go", "line": 128, | |
| 37 | + "side": "new", "intent": "request_change", "body": "guard the nil case", | |
| 38 | + "resolved": false, "created_at": "2026-08-25T10:00:00Z"}, | |
| 39 | + {"id": "abc-2", "parent_id": "abc-1", "author": "morty", "file": "web/api.go", | |
| 40 | + "line": 128, "side": "new", "intent": "info", "body": "on it\nsecond line", | |
| 41 | + "resolved": true, "created_at": "2026-08-25T11:00:00Z"}, | |
| 42 | + }, | |
| 43 | + }) | |
| 44 | + case strings.Contains(r.URL.Path, "/line-comments/"): | |
| 45 | + _ = json.NewEncoder(w).Encode(map[string]any{ | |
| 46 | + "id": "abc-1", "author": "rick", "file": "web/api.go", "line": 128, | |
| 47 | + "side": "new", "intent": "info", "body": "x", "resolved": true, | |
| 48 | + }) | |
| 49 | + case strings.HasSuffix(r.URL.Path, "/line-comments"): | |
| 50 | + w.WriteHeader(http.StatusCreated) | |
| 51 | + _ = json.NewEncoder(w).Encode(map[string]any{ | |
| 52 | + "id": "abc-9", "author": "rick", "file": "web/api.go", "line": 128, | |
| 53 | + "side": "new", "intent": "request_change", "body": "guard the nil case", | |
| 54 | + }) | |
| 55 | + default: // whole-MR conversation comment | |
| 56 | + w.WriteHeader(http.StatusCreated) | |
| 57 | + _ = json.NewEncoder(w).Encode(map[string]any{ | |
| 58 | + "author": "rick", "body": "looks good", "created_at": "2026-08-25T10:00:00Z", | |
| 59 | + }) | |
| 60 | + } | |
| 61 | + })) | |
| 62 | +} | |
| 63 | + | |
| 64 | +func withServer(t *testing.T, seen *captured) func() { | |
| 65 | + t.Helper() | |
| 66 | + srv := lineCommentServer(t, seen) | |
| 67 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | |
| 68 | + t.Setenv("RICKUB_HOST", srv.URL) | |
| 69 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | |
| 70 | + return srv.Close | |
| 71 | +} | |
| 72 | + | |
| 73 | +// TestPRCommentWithoutFlagsStaysAWholeMRComment is the COMPATIBILITY guard: the | |
| 74 | +// pre-existing `rickub mr comment N -b …` must keep hitting the conversation | |
| 75 | +// endpoint, unchanged, now that anchoring flags exist beside it. | |
| 76 | +func TestPRCommentWithoutFlagsStaysAWholeMRComment(t *testing.T) { | |
| 77 | + var seen captured | |
| 78 | + defer withServer(t, &seen)() | |
| 79 | + | |
| 80 | + out, _, err := execute(t, "mr", "comment", "11", "-R", "rick/portal", "-b", "looks good") | |
| 81 | + if err != nil { | |
| 82 | + t.Fatalf("execute: %v", err) | |
| 83 | + } | |
| 84 | + if seen.Path != "/api/v1/repos/rick/portal/merge-requests/11/comments" { | |
| 85 | + t.Fatalf("path = %q, want the whole-MR comments endpoint", seen.Path) | |
| 86 | + } | |
| 87 | + if !strings.Contains(out, "Commented on #11") { | |
| 88 | + t.Errorf("unexpected output: %q", out) | |
| 89 | + } | |
| 90 | +} | |
| 91 | + | |
| 92 | +// TestPRCommentAnchoredHitsLineCommentEndpoint: --file/--line switch it to the | |
| 93 | +// anchored endpoint, carrying side/intent/commit through. | |
| 94 | +func TestPRCommentAnchoredHitsLineCommentEndpoint(t *testing.T) { | |
| 95 | + var seen captured | |
| 96 | + defer withServer(t, &seen)() | |
| 97 | + | |
| 98 | + out, _, err := execute(t, "mr", "comment", "11", "-R", "rick/portal", | |
| 99 | + "--file", "web/api.go", "--line", "128", "--intent", "request_change", | |
| 100 | + "--side", "new", "--commit", "deadbeef", "-b", "guard the nil case") | |
| 101 | + if err != nil { | |
| 102 | + t.Fatalf("execute: %v", err) | |
| 103 | + } | |
| 104 | + if seen.Path != "/api/v1/repos/rick/portal/merge-requests/11/line-comments" { | |
| 105 | + t.Fatalf("path = %q, want the line-comments endpoint", seen.Path) | |
| 106 | + } | |
| 107 | + if seen.Body["file"] != "web/api.go" || seen.Body["line"].(float64) != 128 { | |
| 108 | + t.Fatalf("body anchor = %+v", seen.Body) | |
| 109 | + } | |
| 110 | + if seen.Body["intent"] != "request_change" || seen.Body["side"] != "new" || seen.Body["commit"] != "deadbeef" { | |
| 111 | + t.Fatalf("body options = %+v", seen.Body) | |
| 112 | + } | |
| 113 | + if !strings.Contains(out, "web/api.go:128") { | |
| 114 | + t.Errorf("output should name the anchor: %q", out) | |
| 115 | + } | |
| 116 | +} | |
| 117 | + | |
| 118 | +// TestPRCommentReplyToSendsParentOnly: --reply-to posts a parent_id and no anchor. | |
| 119 | +func TestPRCommentReplyToSendsParentOnly(t *testing.T) { | |
| 120 | + var seen captured | |
| 121 | + defer withServer(t, &seen)() | |
| 122 | + | |
| 123 | + if _, _, err := execute(t, "mr", "comment", "11", "-R", "rick/portal", | |
| 124 | + "--reply-to", "abc-1", "-b", "agreed"); err != nil { | |
| 125 | + t.Fatalf("execute: %v", err) | |
| 126 | + } | |
| 127 | + if seen.Body["parent_id"] != "abc-1" { | |
| 128 | + t.Fatalf("body = %+v, want parent_id", seen.Body) | |
| 129 | + } | |
| 130 | + if _, ok := seen.Body["file"]; ok { | |
| 131 | + t.Errorf("a reply must not send an anchor: %+v", seen.Body) | |
| 132 | + } | |
| 133 | +} | |
| 134 | + | |
| 135 | +// TestPRCommentFlagValidation: every bad flag combination fails FAST, locally, | |
| 136 | +// with a message naming the flags — no request is made, so the user never sees a | |
| 137 | +// server 400 for something the CLI could tell them itself. | |
| 138 | +func TestPRCommentFlagValidation(t *testing.T) { | |
| 139 | + cases := []struct { | |
| 140 | + name string | |
| 141 | + args []string | |
| 142 | + want string | |
| 143 | + }{ | |
| 144 | + {"line without file", []string{"--line", "12"}, "--line requires --file"}, | |
| 145 | + {"file without line", []string{"--file", "a.go"}, "--file requires --line"}, | |
| 146 | + {"zero line with file", []string{"--file", "a.go", "--line", "0"}, "--file requires --line"}, | |
| 147 | + {"negative line", []string{"--file", "a.go", "--line", "-4"}, "--file requires --line"}, | |
| 148 | + {"bad intent", []string{"--file", "a.go", "--line", "3", "--intent", "nitpick"}, `invalid --intent "nitpick"`}, | |
| 149 | + {"bad side", []string{"--file", "a.go", "--line", "3", "--side", "middle"}, `invalid --side "middle"`}, | |
| 150 | + {"intent without anchor", []string{"--intent", "kudos"}, "only apply to a line comment"}, | |
| 151 | + {"reply plus anchor", []string{"--reply-to", "abc-1", "--file", "a.go", "--line", "2"}, "--reply-to inherits"}, | |
| 152 | + } | |
| 153 | + for _, tc := range cases { | |
| 154 | + t.Run(tc.name, func(t *testing.T) { | |
| 155 | + var seen captured | |
| 156 | + defer withServer(t, &seen)() | |
| 157 | + args := append([]string{"mr", "comment", "11", "-R", "rick/portal", "-b", "x"}, tc.args...) | |
| 158 | + _, _, err := execute(t, args...) | |
| 159 | + if err == nil { | |
| 160 | + t.Fatalf("%s was accepted; want a local validation error", tc.name) | |
| 161 | + } | |
| 162 | + if !strings.Contains(err.Error(), tc.want) { | |
| 163 | + t.Fatalf("%s error = %q, want it to contain %q", tc.name, err, tc.want) | |
| 164 | + } | |
| 165 | + if seen.Path != "" { | |
| 166 | + t.Fatalf("%s reached the server at %q; validation must happen before the request", tc.name, seen.Path) | |
| 167 | + } | |
| 168 | + }) | |
| 169 | + } | |
| 170 | +} | |
| 171 | + | |
| 172 | +// TestPRCommentStillRequiresBody: --body stays mandatory on both paths. | |
| 173 | +func TestPRCommentStillRequiresBody(t *testing.T) { | |
| 174 | + var seen captured | |
| 175 | + defer withServer(t, &seen)() | |
| 176 | + if _, _, err := execute(t, "mr", "comment", "11", "-R", "rick/portal", "--file", "a.go", "--line", "1"); err == nil { | |
| 177 | + t.Fatal("missing --body was accepted") | |
| 178 | + } | |
| 179 | + if seen.Path != "" { | |
| 180 | + t.Fatalf("missing --body reached the server at %q", seen.Path) | |
| 181 | + } | |
| 182 | +} | |
| 183 | + | |
| 184 | +// TestPRLineCommentsTable: the listing renders one row per comment, flattens a | |
| 185 | +// multi-line body, and marks replies and resolved threads. | |
| 186 | +func TestPRLineCommentsTable(t *testing.T) { | |
| 187 | + var seen captured | |
| 188 | + defer withServer(t, &seen)() | |
| 189 | + | |
| 190 | + out, _, err := execute(t, "mr", "line-comments", "11", "-R", "rick/portal") | |
| 191 | + if err != nil { | |
| 192 | + t.Fatalf("execute: %v", err) | |
| 193 | + } | |
| 194 | + if seen.Method != http.MethodGet || !strings.HasSuffix(seen.Path, "/merge-requests/11/line-comments") { | |
| 195 | + t.Fatalf("called %s %s", seen.Method, seen.Path) | |
| 196 | + } | |
| 197 | + for _, want := range []string{"ID", "ANCHOR", "abc-1", "web/api.go:128", "request_change", "resolved", "reply"} { | |
| 198 | + if !strings.Contains(out, want) { | |
| 199 | + t.Errorf("listing missing %q:\n%s", want, out) | |
| 200 | + } | |
| 201 | + } | |
| 202 | + if strings.Contains(out, "second line") { | |
| 203 | + t.Errorf("a multi-line body must be flattened to one row:\n%s", out) | |
| 204 | + } | |
| 205 | +} | |
| 206 | + | |
| 207 | +// TestPRResolveAndReopen: `mr resolve` posts resolved=true, `--reopen` false. | |
| 208 | +func TestPRResolveAndReopen(t *testing.T) { | |
| 209 | + var seen captured | |
| 210 | + defer withServer(t, &seen)() | |
| 211 | + | |
| 212 | + if _, _, err := execute(t, "mr", "resolve", "11", "abc-1", "-R", "rick/portal"); err != nil { | |
| 213 | + t.Fatalf("resolve: %v", err) | |
| 214 | + } | |
| 215 | + if !strings.HasSuffix(seen.Path, "/line-comments/abc-1/resolve") { | |
| 216 | + t.Fatalf("resolve path = %q", seen.Path) | |
| 217 | + } | |
| 218 | + if seen.Body["resolved"] != true { | |
| 219 | + t.Fatalf("resolve body = %+v, want resolved=true", seen.Body) | |
| 220 | + } | |
| 221 | + | |
| 222 | + if _, _, err := execute(t, "mr", "resolve", "11", "abc-1", "-R", "rick/portal", "--reopen"); err != nil { | |
| 223 | + t.Fatalf("reopen: %v", err) | |
| 224 | + } | |
| 225 | + if seen.Body["resolved"] != false { | |
| 226 | + t.Fatalf("reopen body = %+v, want resolved=false", seen.Body) | |
| 227 | + } | |
| 228 | +} | |
| 229 | + | |
| 230 | +// --- conversation-comment deletion -------------------------------------------- | |
| 231 | + | |
| 232 | +// TestPRDeleteCommentCallsDeleteEndpoint: `mr delete-comment` issues a DELETE at | |
| 233 | +// the comment's own URL. Before this change there was no such command and no such | |
| 234 | +// route — a ruined comment was permanent from the terminal (issue #94). | |
| 235 | +func TestPRDeleteCommentCallsDeleteEndpoint(t *testing.T) { | |
| 236 | + var seen captured | |
| 237 | + defer withServer(t, &seen)() | |
| 238 | + | |
| 239 | + out, _, err := execute(t, "mr", "delete-comment", "11", "cmt-7", "-R", "rick/portal") | |
| 240 | + if err != nil { | |
| 241 | + t.Fatalf("execute: %v", err) | |
| 242 | + } | |
| 243 | + if seen.Method != http.MethodDelete { | |
| 244 | + t.Fatalf("method = %s, want DELETE", seen.Method) | |
| 245 | + } | |
| 246 | + if seen.Path != "/api/v1/repos/rick/portal/merge-requests/11/comments/cmt-7" { | |
| 247 | + t.Fatalf("path = %q", seen.Path) | |
| 248 | + } | |
| 249 | + if !strings.Contains(out, "cmt-7") { | |
| 250 | + t.Errorf("output should name the retracted comment: %q", out) | |
| 251 | + } | |
| 252 | +} | |
| 253 | + | |
| 254 | +// TestIssueDeleteCommentCallsDeleteEndpoint: the issue twin. | |
| 255 | +func TestIssueDeleteCommentCallsDeleteEndpoint(t *testing.T) { | |
| 256 | + var seen captured | |
| 257 | + defer withServer(t, &seen)() | |
| 258 | + | |
| 259 | + if _, _, err := execute(t, "issue", "delete-comment", "7", "cmt-3", "-R", "rick/portal"); err != nil { | |
| 260 | + t.Fatalf("execute: %v", err) | |
| 261 | + } | |
| 262 | + if seen.Method != http.MethodDelete || seen.Path != "/api/v1/repos/rick/portal/issues/7/comments/cmt-3" { | |
| 263 | + t.Fatalf("called %s %s", seen.Method, seen.Path) | |
| 264 | + } | |
| 265 | +} | |
| 266 | + | |
| 267 | +// TestPRViewShowsCommentIDs: `mr view` prints each comment's id and marks a | |
| 268 | +// tombstone. Without the id printed, `delete-comment` has nothing discoverable to | |
| 269 | +// take — the listing is the only place a terminal user learns it. | |
| 270 | +func TestPRViewShowsCommentIDs(t *testing.T) { | |
| 271 | + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 272 | + _ = json.NewEncoder(w).Encode(map[string]any{ | |
| 273 | + "number": 11, "title": "a change", "state": "open", "author": "rick", | |
| 274 | + "base_branch": "main", "head_branch": "dev", "created_at": "2026-08-26T10:00:00Z", | |
| 275 | + "comments": []map[string]any{ | |
| 276 | + {"id": "cmt-a", "author": "rick", "body": "a real one", "created_at": "2026-08-26T10:00:00Z"}, | |
| 277 | + {"id": "cmt-b", "author": "morty", "body": "", "created_at": "2026-08-26T11:00:00Z", "deleted": true}, | |
| 278 | + }, | |
| 279 | + }) | |
| 280 | + })) | |
| 281 | + defer srv.Close() | |
| 282 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | |
| 283 | + t.Setenv("RICKUB_HOST", srv.URL) | |
| 284 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | |
| 285 | + | |
| 286 | + out, _, err := execute(t, "mr", "view", "11", "-R", "rick/portal") | |
| 287 | + if err != nil { | |
| 288 | + t.Fatalf("execute: %v", err) | |
| 289 | + } | |
| 290 | + for _, want := range []string{"cmt-a", "cmt-b", "[deleted]"} { | |
| 291 | + if !strings.Contains(out, want) { | |
| 292 | + t.Errorf("mr view output missing %q:\n%s", want, out) | |
| 293 | + } | |
| 294 | + } | |
| 295 | +} | |
| 296 | + | |
| 297 | +// TestPREditSendsBodyAndReportsLength: `mr edit` posts the description to the | |
| 298 | +// merge-request body route (which did not exist on /api/v1 before this branch), | |
| 299 | +// resolves `-b -` from stdin through the shared resolver, and reports the STORED | |
| 300 | +// byte count rather than a bare success line. | |
| 301 | +func TestPREditSendsBodyAndReportsLength(t *testing.T) { | |
| 302 | + var seen captured | |
| 303 | + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 304 | + seen.Method, seen.Path = r.Method, r.URL.Path | |
| 305 | + b, _ := io.ReadAll(r.Body) | |
| 306 | + seen.Body = map[string]any{} | |
| 307 | + _ = json.Unmarshal(b, &seen.Body) | |
| 308 | + // Echo the body back the way the server does, so the printed length is | |
| 309 | + // the STORED one. | |
| 310 | + _ = json.NewEncoder(w).Encode(map[string]any{ | |
| 311 | + "number": 12, "title": "t", "state": "open", "author": "rick", | |
| 312 | + "base_branch": "main", "head_branch": "dev", "created_at": "2026-08-26T10:00:00Z", | |
| 313 | + "body": seen.Body["body"], | |
| 314 | + }) | |
| 315 | + })) | |
| 316 | + defer srv.Close() | |
| 317 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | |
| 318 | + t.Setenv("RICKUB_HOST", srv.URL) | |
| 319 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | |
| 320 | + | |
| 321 | + const desc = "## What this adds\n\nA longer description read from stdin." | |
| 322 | + rootCmd.SetIn(strings.NewReader(desc + "\n")) | |
| 323 | + defer rootCmd.SetIn(nil) | |
| 324 | + out, _, err := execute(t, "mr", "edit", "12", "-R", "rick/portal", "-b", "-") | |
| 325 | + if err != nil { | |
| 326 | + t.Fatalf("execute: %v", err) | |
| 327 | + } | |
| 328 | + if seen.Path != "/api/v1/repos/rick/portal/merge-requests/12/body" { | |
| 329 | + t.Fatalf("path = %q", seen.Path) | |
| 330 | + } | |
| 331 | + if seen.Body["body"] != desc { | |
| 332 | + t.Fatalf("sent body = %q, want the stdin text", seen.Body["body"]) | |
| 333 | + } | |
| 334 | + if !strings.Contains(out, fmt.Sprintf("%d bytes", len(desc))) { | |
| 335 | + t.Errorf("output should report the stored byte count (%d): %q", len(desc), out) | |
| 336 | + } | |
| 337 | +} | |
| 338 | + | |
| 339 | +// TestPREditRefusesEmptyBody: a blank description is refused locally, so `mr edit` | |
| 340 | +// can never silently blank a merge request's description. | |
| 341 | +func TestPREditRefusesEmptyBody(t *testing.T) { | |
| 342 | + var seen captured | |
| 343 | + defer withServer(t, &seen)() | |
| 344 | + if _, _, err := execute(t, "mr", "edit", "12", "-R", "rick/portal", "-b", " "); err == nil { | |
| 345 | + t.Fatal("a blank description was accepted") | |
| 346 | + } | |
| 347 | + if seen.Path != "" { | |
| 348 | + t.Fatalf("a blank description reached the server at %q", seen.Path) | |
| 349 | + } | |
| 350 | +} | |
| new file mode 100644 | |||
| @@ -0,0 +1,350 @@ | |||
| 1 | +package cmd | ||
| 2 | + | ||
| 3 | +import ( | ||
| 4 | + "encoding/json" | ||
| 5 | + "fmt" | ||
| 6 | + "io" | ||
| 7 | + "net/http" | ||
| 8 | + "net/http/httptest" | ||
| 9 | + "strings" | ||
| 10 | + "testing" | ||
| 11 | +) | ||
| 12 | + | ||
| 13 | +// captured is what a fake server recorded about the request the CLI made. | ||
| 14 | +type captured struct { | ||
| 15 | + Method string | ||
| 16 | + Path string | ||
| 17 | + Body map[string]any | ||
| 18 | +} | ||
| 19 | + | ||
| 20 | +// lineCommentServer answers the merge-request comment endpoints and records what | ||
| 21 | +// was called, so a test can assert WHICH surface the CLI chose (the whole-MR | ||
| 22 | +// /comments endpoint or the anchored /line-comments one). | ||
| 23 | +func lineCommentServer(t *testing.T, seen *captured) *httptest.Server { | ||
| 24 | + t.Helper() | ||
| 25 | + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 26 | + seen.Method, seen.Path = r.Method, r.URL.Path | ||
| 27 | + if b, _ := io.ReadAll(r.Body); len(b) > 0 { | ||
| 28 | + seen.Body = map[string]any{} | ||
| 29 | + _ = json.Unmarshal(b, &seen.Body) | ||
| 30 | + } | ||
| 31 | + switch { | ||
| 32 | + case strings.HasSuffix(r.URL.Path, "/line-comments") && r.Method == http.MethodGet: | ||
| 33 | + _ = json.NewEncoder(w).Encode(map[string]any{ | ||
| 34 | + "page": 1, "per_page": 30, "has_next": false, | ||
| 35 | + "items": []map[string]any{ | ||
| 36 | + {"id": "abc-1", "author": "rick", "file": "web/api.go", "line": 128, | ||
| 37 | + "side": "new", "intent": "request_change", "body": "guard the nil case", | ||
| 38 | + "resolved": false, "created_at": "2026-08-25T10:00:00Z"}, | ||
| 39 | + {"id": "abc-2", "parent_id": "abc-1", "author": "morty", "file": "web/api.go", | ||
| 40 | + "line": 128, "side": "new", "intent": "info", "body": "on it\nsecond line", | ||
| 41 | + "resolved": true, "created_at": "2026-08-25T11:00:00Z"}, | ||
| 42 | + }, | ||
| 43 | + }) | ||
| 44 | + case strings.Contains(r.URL.Path, "/line-comments/"): | ||
| 45 | + _ = json.NewEncoder(w).Encode(map[string]any{ | ||
| 46 | + "id": "abc-1", "author": "rick", "file": "web/api.go", "line": 128, | ||
| 47 | + "side": "new", "intent": "info", "body": "x", "resolved": true, | ||
| 48 | + }) | ||
| 49 | + case strings.HasSuffix(r.URL.Path, "/line-comments"): | ||
| 50 | + w.WriteHeader(http.StatusCreated) | ||
| 51 | + _ = json.NewEncoder(w).Encode(map[string]any{ | ||
| 52 | + "id": "abc-9", "author": "rick", "file": "web/api.go", "line": 128, | ||
| 53 | + "side": "new", "intent": "request_change", "body": "guard the nil case", | ||
| 54 | + }) | ||
| 55 | + default: // whole-MR conversation comment | ||
| 56 | + w.WriteHeader(http.StatusCreated) | ||
| 57 | + _ = json.NewEncoder(w).Encode(map[string]any{ | ||
| 58 | + "author": "rick", "body": "looks good", "created_at": "2026-08-25T10:00:00Z", | ||
| 59 | + }) | ||
| 60 | + } | ||
| 61 | + })) | ||
| 62 | +} | ||
| 63 | + | ||
| 64 | +func withServer(t *testing.T, seen *captured) func() { | ||
| 65 | + t.Helper() | ||
| 66 | + srv := lineCommentServer(t, seen) | ||
| 67 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | ||
| 68 | + t.Setenv("RICKUB_HOST", srv.URL) | ||
| 69 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | ||
| 70 | + return srv.Close | ||
| 71 | +} | ||
| 72 | + | ||
| 73 | +// TestPRCommentWithoutFlagsStaysAWholeMRComment is the COMPATIBILITY guard: the | ||
| 74 | +// pre-existing `rickub mr comment N -b …` must keep hitting the conversation | ||
| 75 | +// endpoint, unchanged, now that anchoring flags exist beside it. | ||
| 76 | +func TestPRCommentWithoutFlagsStaysAWholeMRComment(t *testing.T) { | ||
| 77 | + var seen captured | ||
| 78 | + defer withServer(t, &seen)() | ||
| 79 | + | ||
| 80 | + out, _, err := execute(t, "mr", "comment", "11", "-R", "rick/portal", "-b", "looks good") | ||
| 81 | + if err != nil { | ||
| 82 | + t.Fatalf("execute: %v", err) | ||
| 83 | + } | ||
| 84 | + if seen.Path != "/api/v1/repos/rick/portal/merge-requests/11/comments" { | ||
| 85 | + t.Fatalf("path = %q, want the whole-MR comments endpoint", seen.Path) | ||
| 86 | + } | ||
| 87 | + if !strings.Contains(out, "Commented on #11") { | ||
| 88 | + t.Errorf("unexpected output: %q", out) | ||
| 89 | + } | ||
| 90 | +} | ||
| 91 | + | ||
| 92 | +// TestPRCommentAnchoredHitsLineCommentEndpoint: --file/--line switch it to the | ||
| 93 | +// anchored endpoint, carrying side/intent/commit through. | ||
| 94 | +func TestPRCommentAnchoredHitsLineCommentEndpoint(t *testing.T) { | ||
| 95 | + var seen captured | ||
| 96 | + defer withServer(t, &seen)() | ||
| 97 | + | ||
| 98 | + out, _, err := execute(t, "mr", "comment", "11", "-R", "rick/portal", | ||
| 99 | + "--file", "web/api.go", "--line", "128", "--intent", "request_change", | ||
| 100 | + "--side", "new", "--commit", "deadbeef", "-b", "guard the nil case") | ||
| 101 | + if err != nil { | ||
| 102 | + t.Fatalf("execute: %v", err) | ||
| 103 | + } | ||
| 104 | + if seen.Path != "/api/v1/repos/rick/portal/merge-requests/11/line-comments" { | ||
| 105 | + t.Fatalf("path = %q, want the line-comments endpoint", seen.Path) | ||
| 106 | + } | ||
| 107 | + if seen.Body["file"] != "web/api.go" || seen.Body["line"].(float64) != 128 { | ||
| 108 | + t.Fatalf("body anchor = %+v", seen.Body) | ||
| 109 | + } | ||
| 110 | + if seen.Body["intent"] != "request_change" || seen.Body["side"] != "new" || seen.Body["commit"] != "deadbeef" { | ||
| 111 | + t.Fatalf("body options = %+v", seen.Body) | ||
| 112 | + } | ||
| 113 | + if !strings.Contains(out, "web/api.go:128") { | ||
| 114 | + t.Errorf("output should name the anchor: %q", out) | ||
| 115 | + } | ||
| 116 | +} | ||
| 117 | + | ||
| 118 | +// TestPRCommentReplyToSendsParentOnly: --reply-to posts a parent_id and no anchor. | ||
| 119 | +func TestPRCommentReplyToSendsParentOnly(t *testing.T) { | ||
| 120 | + var seen captured | ||
| 121 | + defer withServer(t, &seen)() | ||
| 122 | + | ||
| 123 | + if _, _, err := execute(t, "mr", "comment", "11", "-R", "rick/portal", | ||
| 124 | + "--reply-to", "abc-1", "-b", "agreed"); err != nil { | ||
| 125 | + t.Fatalf("execute: %v", err) | ||
| 126 | + } | ||
| 127 | + if seen.Body["parent_id"] != "abc-1" { | ||
| 128 | + t.Fatalf("body = %+v, want parent_id", seen.Body) | ||
| 129 | + } | ||
| 130 | + if _, ok := seen.Body["file"]; ok { | ||
| 131 | + t.Errorf("a reply must not send an anchor: %+v", seen.Body) | ||
| 132 | + } | ||
| 133 | +} | ||
| 134 | + | ||
| 135 | +// TestPRCommentFlagValidation: every bad flag combination fails FAST, locally, | ||
| 136 | +// with a message naming the flags — no request is made, so the user never sees a | ||
| 137 | +// server 400 for something the CLI could tell them itself. | ||
| 138 | +func TestPRCommentFlagValidation(t *testing.T) { | ||
| 139 | + cases := []struct { | ||
| 140 | + name string | ||
| 141 | + args []string | ||
| 142 | + want string | ||
| 143 | + }{ | ||
| 144 | + {"line without file", []string{"--line", "12"}, "--line requires --file"}, | ||
| 145 | + {"file without line", []string{"--file", "a.go"}, "--file requires --line"}, | ||
| 146 | + {"zero line with file", []string{"--file", "a.go", "--line", "0"}, "--file requires --line"}, | ||
| 147 | + {"negative line", []string{"--file", "a.go", "--line", "-4"}, "--file requires --line"}, | ||
| 148 | + {"bad intent", []string{"--file", "a.go", "--line", "3", "--intent", "nitpick"}, `invalid --intent "nitpick"`}, | ||
| 149 | + {"bad side", []string{"--file", "a.go", "--line", "3", "--side", "middle"}, `invalid --side "middle"`}, | ||
| 150 | + {"intent without anchor", []string{"--intent", "kudos"}, "only apply to a line comment"}, | ||
| 151 | + {"reply plus anchor", []string{"--reply-to", "abc-1", "--file", "a.go", "--line", "2"}, "--reply-to inherits"}, | ||
| 152 | + } | ||
| 153 | + for _, tc := range cases { | ||
| 154 | + t.Run(tc.name, func(t *testing.T) { | ||
| 155 | + var seen captured | ||
| 156 | + defer withServer(t, &seen)() | ||
| 157 | + args := append([]string{"mr", "comment", "11", "-R", "rick/portal", "-b", "x"}, tc.args...) | ||
| 158 | + _, _, err := execute(t, args...) | ||
| 159 | + if err == nil { | ||
| 160 | + t.Fatalf("%s was accepted; want a local validation error", tc.name) | ||
| 161 | + } | ||
| 162 | + if !strings.Contains(err.Error(), tc.want) { | ||
| 163 | + t.Fatalf("%s error = %q, want it to contain %q", tc.name, err, tc.want) | ||
| 164 | + } | ||
| 165 | + if seen.Path != "" { | ||
| 166 | + t.Fatalf("%s reached the server at %q; validation must happen before the request", tc.name, seen.Path) | ||
| 167 | + } | ||
| 168 | + }) | ||
| 169 | + } | ||
| 170 | +} | ||
| 171 | + | ||
| 172 | +// TestPRCommentStillRequiresBody: --body stays mandatory on both paths. | ||
| 173 | +func TestPRCommentStillRequiresBody(t *testing.T) { | ||
| 174 | + var seen captured | ||
| 175 | + defer withServer(t, &seen)() | ||
| 176 | + if _, _, err := execute(t, "mr", "comment", "11", "-R", "rick/portal", "--file", "a.go", "--line", "1"); err == nil { | ||
| 177 | + t.Fatal("missing --body was accepted") | ||
| 178 | + } | ||
| 179 | + if seen.Path != "" { | ||
| 180 | + t.Fatalf("missing --body reached the server at %q", seen.Path) | ||
| 181 | + } | ||
| 182 | +} | ||
| 183 | + | ||
| 184 | +// TestPRLineCommentsTable: the listing renders one row per comment, flattens a | ||
| 185 | +// multi-line body, and marks replies and resolved threads. | ||
| 186 | +func TestPRLineCommentsTable(t *testing.T) { | ||
| 187 | + var seen captured | ||
| 188 | + defer withServer(t, &seen)() | ||
| 189 | + | ||
| 190 | + out, _, err := execute(t, "mr", "line-comments", "11", "-R", "rick/portal") | ||
| 191 | + if err != nil { | ||
| 192 | + t.Fatalf("execute: %v", err) | ||
| 193 | + } | ||
| 194 | + if seen.Method != http.MethodGet || !strings.HasSuffix(seen.Path, "/merge-requests/11/line-comments") { | ||
| 195 | + t.Fatalf("called %s %s", seen.Method, seen.Path) | ||
| 196 | + } | ||
| 197 | + for _, want := range []string{"ID", "ANCHOR", "abc-1", "web/api.go:128", "request_change", "resolved", "reply"} { | ||
| 198 | + if !strings.Contains(out, want) { | ||
| 199 | + t.Errorf("listing missing %q:\n%s", want, out) | ||
| 200 | + } | ||
| 201 | + } | ||
| 202 | + if strings.Contains(out, "second line") { | ||
| 203 | + t.Errorf("a multi-line body must be flattened to one row:\n%s", out) | ||
| 204 | + } | ||
| 205 | +} | ||
| 206 | + | ||
| 207 | +// TestPRResolveAndReopen: `mr resolve` posts resolved=true, `--reopen` false. | ||
| 208 | +func TestPRResolveAndReopen(t *testing.T) { | ||
| 209 | + var seen captured | ||
| 210 | + defer withServer(t, &seen)() | ||
| 211 | + | ||
| 212 | + if _, _, err := execute(t, "mr", "resolve", "11", "abc-1", "-R", "rick/portal"); err != nil { | ||
| 213 | + t.Fatalf("resolve: %v", err) | ||
| 214 | + } | ||
| 215 | + if !strings.HasSuffix(seen.Path, "/line-comments/abc-1/resolve") { | ||
| 216 | + t.Fatalf("resolve path = %q", seen.Path) | ||
| 217 | + } | ||
| 218 | + if seen.Body["resolved"] != true { | ||
| 219 | + t.Fatalf("resolve body = %+v, want resolved=true", seen.Body) | ||
| 220 | + } | ||
| 221 | + | ||
| 222 | + if _, _, err := execute(t, "mr", "resolve", "11", "abc-1", "-R", "rick/portal", "--reopen"); err != nil { | ||
| 223 | + t.Fatalf("reopen: %v", err) | ||
| 224 | + } | ||
| 225 | + if seen.Body["resolved"] != false { | ||
| 226 | + t.Fatalf("reopen body = %+v, want resolved=false", seen.Body) | ||
| 227 | + } | ||
| 228 | +} | ||
| 229 | + | ||
| 230 | +// --- conversation-comment deletion -------------------------------------------- | ||
| 231 | + | ||
| 232 | +// TestPRDeleteCommentCallsDeleteEndpoint: `mr delete-comment` issues a DELETE at | ||
| 233 | +// the comment's own URL. Before this change there was no such command and no such | ||
| 234 | +// route — a ruined comment was permanent from the terminal (issue #94). | ||
| 235 | +func TestPRDeleteCommentCallsDeleteEndpoint(t *testing.T) { | ||
| 236 | + var seen captured | ||
| 237 | + defer withServer(t, &seen)() | ||
| 238 | + | ||
| 239 | + out, _, err := execute(t, "mr", "delete-comment", "11", "cmt-7", "-R", "rick/portal") | ||
| 240 | + if err != nil { | ||
| 241 | + t.Fatalf("execute: %v", err) | ||
| 242 | + } | ||
| 243 | + if seen.Method != http.MethodDelete { | ||
| 244 | + t.Fatalf("method = %s, want DELETE", seen.Method) | ||
| 245 | + } | ||
| 246 | + if seen.Path != "/api/v1/repos/rick/portal/merge-requests/11/comments/cmt-7" { | ||
| 247 | + t.Fatalf("path = %q", seen.Path) | ||
| 248 | + } | ||
| 249 | + if !strings.Contains(out, "cmt-7") { | ||
| 250 | + t.Errorf("output should name the retracted comment: %q", out) | ||
| 251 | + } | ||
| 252 | +} | ||
| 253 | + | ||
| 254 | +// TestIssueDeleteCommentCallsDeleteEndpoint: the issue twin. | ||
| 255 | +func TestIssueDeleteCommentCallsDeleteEndpoint(t *testing.T) { | ||
| 256 | + var seen captured | ||
| 257 | + defer withServer(t, &seen)() | ||
| 258 | + | ||
| 259 | + if _, _, err := execute(t, "issue", "delete-comment", "7", "cmt-3", "-R", "rick/portal"); err != nil { | ||
| 260 | + t.Fatalf("execute: %v", err) | ||
| 261 | + } | ||
| 262 | + if seen.Method != http.MethodDelete || seen.Path != "/api/v1/repos/rick/portal/issues/7/comments/cmt-3" { | ||
| 263 | + t.Fatalf("called %s %s", seen.Method, seen.Path) | ||
| 264 | + } | ||
| 265 | +} | ||
| 266 | + | ||
| 267 | +// TestPRViewShowsCommentIDs: `mr view` prints each comment's id and marks a | ||
| 268 | +// tombstone. Without the id printed, `delete-comment` has nothing discoverable to | ||
| 269 | +// take — the listing is the only place a terminal user learns it. | ||
| 270 | +func TestPRViewShowsCommentIDs(t *testing.T) { | ||
| 271 | + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 272 | + _ = json.NewEncoder(w).Encode(map[string]any{ | ||
| 273 | + "number": 11, "title": "a change", "state": "open", "author": "rick", | ||
| 274 | + "base_branch": "main", "head_branch": "dev", "created_at": "2026-08-26T10:00:00Z", | ||
| 275 | + "comments": []map[string]any{ | ||
| 276 | + {"id": "cmt-a", "author": "rick", "body": "a real one", "created_at": "2026-08-26T10:00:00Z"}, | ||
| 277 | + {"id": "cmt-b", "author": "morty", "body": "", "created_at": "2026-08-26T11:00:00Z", "deleted": true}, | ||
| 278 | + }, | ||
| 279 | + }) | ||
| 280 | + })) | ||
| 281 | + defer srv.Close() | ||
| 282 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | ||
| 283 | + t.Setenv("RICKUB_HOST", srv.URL) | ||
| 284 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | ||
| 285 | + | ||
| 286 | + out, _, err := execute(t, "mr", "view", "11", "-R", "rick/portal") | ||
| 287 | + if err != nil { | ||
| 288 | + t.Fatalf("execute: %v", err) | ||
| 289 | + } | ||
| 290 | + for _, want := range []string{"cmt-a", "cmt-b", "[deleted]"} { | ||
| 291 | + if !strings.Contains(out, want) { | ||
| 292 | + t.Errorf("mr view output missing %q:\n%s", want, out) | ||
| 293 | + } | ||
| 294 | + } | ||
| 295 | +} | ||
| 296 | + | ||
| 297 | +// TestPREditSendsBodyAndReportsLength: `mr edit` posts the description to the | ||
| 298 | +// merge-request body route (which did not exist on /api/v1 before this branch), | ||
| 299 | +// resolves `-b -` from stdin through the shared resolver, and reports the STORED | ||
| 300 | +// byte count rather than a bare success line. | ||
| 301 | +func TestPREditSendsBodyAndReportsLength(t *testing.T) { | ||
| 302 | + var seen captured | ||
| 303 | + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 304 | + seen.Method, seen.Path = r.Method, r.URL.Path | ||
| 305 | + b, _ := io.ReadAll(r.Body) | ||
| 306 | + seen.Body = map[string]any{} | ||
| 307 | + _ = json.Unmarshal(b, &seen.Body) | ||
| 308 | + // Echo the body back the way the server does, so the printed length is | ||
| 309 | + // the STORED one. | ||
| 310 | + _ = json.NewEncoder(w).Encode(map[string]any{ | ||
| 311 | + "number": 12, "title": "t", "state": "open", "author": "rick", | ||
| 312 | + "base_branch": "main", "head_branch": "dev", "created_at": "2026-08-26T10:00:00Z", | ||
| 313 | + "body": seen.Body["body"], | ||
| 314 | + }) | ||
| 315 | + })) | ||
| 316 | + defer srv.Close() | ||
| 317 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | ||
| 318 | + t.Setenv("RICKUB_HOST", srv.URL) | ||
| 319 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | ||
| 320 | + | ||
| 321 | + const desc = "## What this adds\n\nA longer description read from stdin." | ||
| 322 | + rootCmd.SetIn(strings.NewReader(desc + "\n")) | ||
| 323 | + defer rootCmd.SetIn(nil) | ||
| 324 | + out, _, err := execute(t, "mr", "edit", "12", "-R", "rick/portal", "-b", "-") | ||
| 325 | + if err != nil { | ||
| 326 | + t.Fatalf("execute: %v", err) | ||
| 327 | + } | ||
| 328 | + if seen.Path != "/api/v1/repos/rick/portal/merge-requests/12/body" { | ||
| 329 | + t.Fatalf("path = %q", seen.Path) | ||
| 330 | + } | ||
| 331 | + if seen.Body["body"] != desc { | ||
| 332 | + t.Fatalf("sent body = %q, want the stdin text", seen.Body["body"]) | ||
| 333 | + } | ||
| 334 | + if !strings.Contains(out, fmt.Sprintf("%d bytes", len(desc))) { | ||
| 335 | + t.Errorf("output should report the stored byte count (%d): %q", len(desc), out) | ||
| 336 | + } | ||
| 337 | +} | ||
| 338 | + | ||
| 339 | +// TestPREditRefusesEmptyBody: a blank description is refused locally, so `mr edit` | ||
| 340 | +// can never silently blank a merge request's description. | ||
| 341 | +func TestPREditRefusesEmptyBody(t *testing.T) { | ||
| 342 | + var seen captured | ||
| 343 | + defer withServer(t, &seen)() | ||
| 344 | + if _, _, err := execute(t, "mr", "edit", "12", "-R", "rick/portal", "-b", " "); err == nil { | ||
| 345 | + t.Fatal("a blank description was accepted") | ||
| 346 | + } | ||
| 347 | + if seen.Path != "" { | ||
| 348 | + t.Fatalf("a blank description reached the server at %q", seen.Path) | ||
| 349 | + } | ||
| 350 | +} | ||
modified
cmd/repo_test.go +11 -0 | @@ -31,11 +31,22 @@ func execute(t *testing.T, args ...string) (string, string, error) { | ||
| 31 | 31 | milestoneDue = "" |
| 32 | 32 | milestoneDescription = "" |
| 33 | 33 | runRepo = "" |
| 34 | + cacheRepo = "" | |
| 35 | + cacheScope = "" | |
| 34 | 36 | runWatchInterval = "2s" |
| 35 | 37 | runWatchTimeout = "30m" |
| 36 | 38 | runWatchLogs = false |
| 37 | 39 | authLoginScope = "all" |
| 38 | 40 | authLoginNoBrowser = false |
| 41 | + prRepo = "" | |
| 42 | + prBody = "" | |
| 43 | + prFile = "" | |
| 44 | + prLine = 0 | |
| 45 | + prSide = "" | |
| 46 | + prIntent = "" | |
| 47 | + prCommit = "" | |
| 48 | + prReplyTo = "" | |
| 49 | + prReopen = false | |
| 39 | 50 | |
| 40 | 51 | var out, errBuf bytes.Buffer |
| 41 | 52 | rootCmd.SetOut(&out) |
| @@ -31,11 +31,22 @@ func execute(t *testing.T, args ...string) (string, string, error) { | |||
| 31 | milestoneDue = "" | 31 | milestoneDue = "" |
| 32 | milestoneDescription = "" | 32 | milestoneDescription = "" |
| 33 | runRepo = "" | 33 | runRepo = "" |
| 34 | + cacheRepo = "" | ||
| 35 | + cacheScope = "" | ||
| 34 | runWatchInterval = "2s" | 36 | runWatchInterval = "2s" |
| 35 | runWatchTimeout = "30m" | 37 | runWatchTimeout = "30m" |
| 36 | runWatchLogs = false | 38 | runWatchLogs = false |
| 37 | authLoginScope = "all" | 39 | authLoginScope = "all" |
| 38 | authLoginNoBrowser = false | 40 | authLoginNoBrowser = false |
| 41 | + prRepo = "" | ||
| 42 | + prBody = "" | ||
| 43 | + prFile = "" | ||
| 44 | + prLine = 0 | ||
| 45 | + prSide = "" | ||
| 46 | + prIntent = "" | ||
| 47 | + prCommit = "" | ||
| 48 | + prReplyTo = "" | ||
| 49 | + prReopen = false | ||
| 39 | 50 | ||
| 40 | var out, errBuf bytes.Buffer | 51 | var out, errBuf bytes.Buffer |
| 41 | rootCmd.SetOut(&out) | 52 | rootCmd.SetOut(&out) |
modified
internal/api/endpoints.go +84 -0 | @@ -177,6 +177,65 @@ func (c *Client) CommentMergeRequest(ctx context.Context, owner, repo string, nu | ||
| 177 | 177 | return &cm, err |
| 178 | 178 | } |
| 179 | 179 | |
| 180 | +// ListLineComments lists a merge request's line-anchored diff comments. | |
| 181 | +func (c *Client) ListLineComments(ctx context.Context, owner, repo string, number, page, perPage int) (*LineCommentPage, error) { | |
| 182 | + var p LineCommentPage | |
| 183 | + err := c.do(ctx, Request{Method: http.MethodGet, Path: lineCommentsPath(owner, repo, number), Query: pageQuery(page, perPage, nil)}, &p) | |
| 184 | + return &p, err | |
| 185 | +} | |
| 186 | + | |
| 187 | +// CreateLineComment anchors a comment to one line of a merge request's diff (or | |
| 188 | +// replies to an existing thread when in.ParentID is set). Requires write access. | |
| 189 | +func (c *Client) CreateLineComment(ctx context.Context, owner, repo string, number int, in LineCommentCreate) (*LineComment, error) { | |
| 190 | + var lc LineComment | |
| 191 | + err := c.do(ctx, Request{Method: http.MethodPost, Path: lineCommentsPath(owner, repo, number), Body: in}, &lc) | |
| 192 | + return &lc, err | |
| 193 | +} | |
| 194 | + | |
| 195 | +// ResolveLineComment marks a line comment thread resolved, or reopens it. | |
| 196 | +func (c *Client) ResolveLineComment(ctx context.Context, owner, repo string, number int, id string, resolved bool) (*LineComment, error) { | |
| 197 | + var lc LineComment | |
| 198 | + err := c.do(ctx, Request{ | |
| 199 | + Method: http.MethodPost, | |
| 200 | + Path: lineCommentsPath(owner, repo, number) + "/" + seg(id) + "/resolve", | |
| 201 | + Body: map[string]bool{"resolved": resolved}, | |
| 202 | + }, &lc) | |
| 203 | + return &lc, err | |
| 204 | +} | |
| 205 | + | |
| 206 | +func lineCommentsPath(owner, repo string, number int) string { | |
| 207 | + return "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/line-comments" | |
| 208 | +} | |
| 209 | + | |
| 210 | +// UpdateMergeRequestBody replaces a merge request's description. The MR's author | |
| 211 | +// or any write-access user may do it. | |
| 212 | +func (c *Client) UpdateMergeRequestBody(ctx context.Context, owner, repo string, number int, body string) (*MergeRequest, error) { | |
| 213 | + var mr MergeRequest | |
| 214 | + err := c.do(ctx, Request{ | |
| 215 | + Method: http.MethodPost, | |
| 216 | + Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/body", | |
| 217 | + Body: map[string]string{"body": body}, | |
| 218 | + }, &mr) | |
| 219 | + return &mr, err | |
| 220 | +} | |
| 221 | + | |
| 222 | +// DeleteMergeRequestComment retracts one merge-request conversation comment. | |
| 223 | +// The comment's author or a repo writer may do it; the delete is soft. | |
| 224 | +func (c *Client) DeleteMergeRequestComment(ctx context.Context, owner, repo string, number int, id string) error { | |
| 225 | + return c.do(ctx, Request{ | |
| 226 | + Method: http.MethodDelete, | |
| 227 | + Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/comments/" + seg(id), | |
| 228 | + }, nil) | |
| 229 | +} | |
| 230 | + | |
| 231 | +// DeleteIssueComment retracts one issue comment (same rules). | |
| 232 | +func (c *Client) DeleteIssueComment(ctx context.Context, owner, repo string, number int, id string) error { | |
| 233 | + return c.do(ctx, Request{ | |
| 234 | + Method: http.MethodDelete, | |
| 235 | + Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/issues/" + itoa(number) + "/comments/" + seg(id), | |
| 236 | + }, nil) | |
| 237 | +} | |
| 238 | + | |
| 180 | 239 | // ReviewMergeRequest records a review verdict. |
| 181 | 240 | func (c *Client) ReviewMergeRequest(ctx context.Context, owner, repo string, number int, verdict string) error { |
| 182 | 241 | return c.do(ctx, Request{Method: http.MethodPost, Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/reviews", Body: map[string]string{"verdict": verdict}}, nil) |
| @@ -230,6 +289,31 @@ func (c *Client) Dispatch(ctx context.Context, owner, repo, ref string) (*Dispat | ||
| 230 | 289 | return &d, err |
| 231 | 290 | } |
| 232 | 291 | |
| 292 | +// ListCaches lists a repo's CI dependency-cache entries (repo admin). | |
| 293 | +func (c *Client) ListCaches(ctx context.Context, owner, repo string, page, perPage int) (*CacheEntryPage, error) { | |
| 294 | + var p CacheEntryPage | |
| 295 | + err := c.do(ctx, Request{Method: http.MethodGet, Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/actions/caches", Query: pageQuery(page, perPage, nil)}, &p) | |
| 296 | + return &p, err | |
| 297 | +} | |
| 298 | + | |
| 299 | +// DeleteCache purges every entry for one cache key — all versions, or one ref | |
| 300 | +// scope's worth when scope is non-empty (repo admin). It is the operator exit from | |
| 301 | +// a cache entry that cannot be restored: entries are immutable once finalized, so a | |
| 302 | +// poisoned one can never be replaced by a job (issue #219). | |
| 303 | +func (c *Client) DeleteCache(ctx context.Context, owner, repo, key, scope string) (*CacheDeleteResult, error) { | |
| 304 | + q := url.Values{"key": {key}} | |
| 305 | + if scope != "" { | |
| 306 | + q.Set("scope", scope) | |
| 307 | + } | |
| 308 | + var res CacheDeleteResult | |
| 309 | + err := c.do(ctx, Request{ | |
| 310 | + Method: http.MethodDelete, | |
| 311 | + Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/actions/caches", | |
| 312 | + Query: q, | |
| 313 | + }, &res) | |
| 314 | + return &res, err | |
| 315 | +} | |
| 316 | + | |
| 233 | 317 | // ---- device flow (pre-auth: the device code is the credential) ---- |
| 234 | 318 | |
| 235 | 319 | // StartDeviceLogin requests a device code the user approves in their browser |
| @@ -177,6 +177,65 @@ func (c *Client) CommentMergeRequest(ctx context.Context, owner, repo string, nu | |||
| 177 | return &cm, err | 177 | return &cm, err |
| 178 | } | 178 | } |
| 179 | 179 | ||
| 180 | +// ListLineComments lists a merge request's line-anchored diff comments. | ||
| 181 | +func (c *Client) ListLineComments(ctx context.Context, owner, repo string, number, page, perPage int) (*LineCommentPage, error) { | ||
| 182 | + var p LineCommentPage | ||
| 183 | + err := c.do(ctx, Request{Method: http.MethodGet, Path: lineCommentsPath(owner, repo, number), Query: pageQuery(page, perPage, nil)}, &p) | ||
| 184 | + return &p, err | ||
| 185 | +} | ||
| 186 | + | ||
| 187 | +// CreateLineComment anchors a comment to one line of a merge request's diff (or | ||
| 188 | +// replies to an existing thread when in.ParentID is set). Requires write access. | ||
| 189 | +func (c *Client) CreateLineComment(ctx context.Context, owner, repo string, number int, in LineCommentCreate) (*LineComment, error) { | ||
| 190 | + var lc LineComment | ||
| 191 | + err := c.do(ctx, Request{Method: http.MethodPost, Path: lineCommentsPath(owner, repo, number), Body: in}, &lc) | ||
| 192 | + return &lc, err | ||
| 193 | +} | ||
| 194 | + | ||
| 195 | +// ResolveLineComment marks a line comment thread resolved, or reopens it. | ||
| 196 | +func (c *Client) ResolveLineComment(ctx context.Context, owner, repo string, number int, id string, resolved bool) (*LineComment, error) { | ||
| 197 | + var lc LineComment | ||
| 198 | + err := c.do(ctx, Request{ | ||
| 199 | + Method: http.MethodPost, | ||
| 200 | + Path: lineCommentsPath(owner, repo, number) + "/" + seg(id) + "/resolve", | ||
| 201 | + Body: map[string]bool{"resolved": resolved}, | ||
| 202 | + }, &lc) | ||
| 203 | + return &lc, err | ||
| 204 | +} | ||
| 205 | + | ||
| 206 | +func lineCommentsPath(owner, repo string, number int) string { | ||
| 207 | + return "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/line-comments" | ||
| 208 | +} | ||
| 209 | + | ||
| 210 | +// UpdateMergeRequestBody replaces a merge request's description. The MR's author | ||
| 211 | +// or any write-access user may do it. | ||
| 212 | +func (c *Client) UpdateMergeRequestBody(ctx context.Context, owner, repo string, number int, body string) (*MergeRequest, error) { | ||
| 213 | + var mr MergeRequest | ||
| 214 | + err := c.do(ctx, Request{ | ||
| 215 | + Method: http.MethodPost, | ||
| 216 | + Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/body", | ||
| 217 | + Body: map[string]string{"body": body}, | ||
| 218 | + }, &mr) | ||
| 219 | + return &mr, err | ||
| 220 | +} | ||
| 221 | + | ||
| 222 | +// DeleteMergeRequestComment retracts one merge-request conversation comment. | ||
| 223 | +// The comment's author or a repo writer may do it; the delete is soft. | ||
| 224 | +func (c *Client) DeleteMergeRequestComment(ctx context.Context, owner, repo string, number int, id string) error { | ||
| 225 | + return c.do(ctx, Request{ | ||
| 226 | + Method: http.MethodDelete, | ||
| 227 | + Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/comments/" + seg(id), | ||
| 228 | + }, nil) | ||
| 229 | +} | ||
| 230 | + | ||
| 231 | +// DeleteIssueComment retracts one issue comment (same rules). | ||
| 232 | +func (c *Client) DeleteIssueComment(ctx context.Context, owner, repo string, number int, id string) error { | ||
| 233 | + return c.do(ctx, Request{ | ||
| 234 | + Method: http.MethodDelete, | ||
| 235 | + Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/issues/" + itoa(number) + "/comments/" + seg(id), | ||
| 236 | + }, nil) | ||
| 237 | +} | ||
| 238 | + | ||
| 180 | // ReviewMergeRequest records a review verdict. | 239 | // ReviewMergeRequest records a review verdict. |
| 181 | func (c *Client) ReviewMergeRequest(ctx context.Context, owner, repo string, number int, verdict string) error { | 240 | func (c *Client) ReviewMergeRequest(ctx context.Context, owner, repo string, number int, verdict string) error { |
| 182 | return c.do(ctx, Request{Method: http.MethodPost, Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/reviews", Body: map[string]string{"verdict": verdict}}, nil) | 241 | return c.do(ctx, Request{Method: http.MethodPost, Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/reviews", Body: map[string]string{"verdict": verdict}}, nil) |
| @@ -230,6 +289,31 @@ func (c *Client) Dispatch(ctx context.Context, owner, repo, ref string) (*Dispat | |||
| 230 | return &d, err | 289 | return &d, err |
| 231 | } | 290 | } |
| 232 | 291 | ||
| 292 | +// ListCaches lists a repo's CI dependency-cache entries (repo admin). | ||
| 293 | +func (c *Client) ListCaches(ctx context.Context, owner, repo string, page, perPage int) (*CacheEntryPage, error) { | ||
| 294 | + var p CacheEntryPage | ||
| 295 | + err := c.do(ctx, Request{Method: http.MethodGet, Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/actions/caches", Query: pageQuery(page, perPage, nil)}, &p) | ||
| 296 | + return &p, err | ||
| 297 | +} | ||
| 298 | + | ||
| 299 | +// DeleteCache purges every entry for one cache key — all versions, or one ref | ||
| 300 | +// scope's worth when scope is non-empty (repo admin). It is the operator exit from | ||
| 301 | +// a cache entry that cannot be restored: entries are immutable once finalized, so a | ||
| 302 | +// poisoned one can never be replaced by a job (issue #219). | ||
| 303 | +func (c *Client) DeleteCache(ctx context.Context, owner, repo, key, scope string) (*CacheDeleteResult, error) { | ||
| 304 | + q := url.Values{"key": {key}} | ||
| 305 | + if scope != "" { | ||
| 306 | + q.Set("scope", scope) | ||
| 307 | + } | ||
| 308 | + var res CacheDeleteResult | ||
| 309 | + err := c.do(ctx, Request{ | ||
| 310 | + Method: http.MethodDelete, | ||
| 311 | + Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/actions/caches", | ||
| 312 | + Query: q, | ||
| 313 | + }, &res) | ||
| 314 | + return &res, err | ||
| 315 | +} | ||
| 316 | + | ||
| 233 | // ---- device flow (pre-auth: the device code is the credential) ---- | 317 | // ---- device flow (pre-auth: the device code is the credential) ---- |
| 234 | 318 | ||
| 235 | // StartDeviceLogin requests a device code the user approves in their browser | 319 | // StartDeviceLogin requests a device code the user approves in their browser |
added
internal/api/inline.go +51 -0 | new file mode 100644 | ||
| @@ -0,0 +1,51 @@ | ||
| 1 | +package api | |
| 2 | + | |
| 3 | +import "strings" | |
| 4 | + | |
| 5 | +// Line-anchored merge-request comment vocabulary. | |
| 6 | +// | |
| 7 | +// DRY (CLAUDE.md ladder, rung 3 — conformance-tested copy). The authority for | |
| 8 | +// these strings is web/store/pr_inline_comments.go: they are the values the | |
| 9 | +// `intent` / `side` columns accept, pinned by the CHECK constraints in migration | |
| 10 | +// 0045. The CLI is a SEPARATE Go module (rickub/cli) and cannot import | |
| 11 | +// rickub/web/store, so this is an unavoidable second copy — it is NOT held | |
| 12 | +// together by this comment. `TestCLIInlineIntentsMatchStore` and | |
| 13 | +// `TestCLIInlineSidesMatchStore` in web/cli_intent_conformance_test.go parse THIS | |
| 14 | +// file and the store constants and fail (in the `web` CI job, which is the one | |
| 15 | +// that runs) naming the file to edit if the two ever disagree. | |
| 16 | +// | |
| 17 | +// Keep the declarations below as plain string literals in a single `var … = []string{…}` | |
| 18 | +// block: the conformance test reads them with a regexp, not a Go parser. | |
| 19 | + | |
| 20 | +// InlineIntents is the stance a line comment can declare, in canonical order. | |
| 21 | +var InlineIntents = []string{"info", "request_change", "kudos"} | |
| 22 | + | |
| 23 | +// InlineSides is which column of the diff a line number refers to. | |
| 24 | +var InlineSides = []string{"old", "new"} | |
| 25 | + | |
| 26 | +// DefaultInlineIntent / DefaultInlineSide are what the server assumes when the | |
| 27 | +// field is omitted; the CLI states them in --help so the default is not a secret. | |
| 28 | +const ( | |
| 29 | + DefaultInlineIntent = "info" | |
| 30 | + DefaultInlineSide = "new" | |
| 31 | +) | |
| 32 | + | |
| 33 | +// ValidInlineIntent reports whether s is a storable intent. | |
| 34 | +func ValidInlineIntent(s string) bool { return contains(InlineIntents, s) } | |
| 35 | + | |
| 36 | +// ValidInlineSide reports whether s is a storable diff side. | |
| 37 | +func ValidInlineSide(s string) bool { return contains(InlineSides, s) } | |
| 38 | + | |
| 39 | +// InlineIntentList / InlineSideList render the vocabulary for an error or help | |
| 40 | +// string, so no message hand-types the words. | |
| 41 | +func InlineIntentList() string { return strings.Join(InlineIntents, " | ") } | |
| 42 | +func InlineSideList() string { return strings.Join(InlineSides, " | ") } | |
| 43 | + | |
| 44 | +func contains(list []string, s string) bool { | |
| 45 | + for _, v := range list { | |
| 46 | + if v == s { | |
| 47 | + return true | |
| 48 | + } | |
| 49 | + } | |
| 50 | + return false | |
| 51 | +} | |
| new file mode 100644 | |||
| @@ -0,0 +1,51 @@ | |||
| 1 | +package api | ||
| 2 | + | ||
| 3 | +import "strings" | ||
| 4 | + | ||
| 5 | +// Line-anchored merge-request comment vocabulary. | ||
| 6 | +// | ||
| 7 | +// DRY (CLAUDE.md ladder, rung 3 — conformance-tested copy). The authority for | ||
| 8 | +// these strings is web/store/pr_inline_comments.go: they are the values the | ||
| 9 | +// `intent` / `side` columns accept, pinned by the CHECK constraints in migration | ||
| 10 | +// 0045. The CLI is a SEPARATE Go module (rickub/cli) and cannot import | ||
| 11 | +// rickub/web/store, so this is an unavoidable second copy — it is NOT held | ||
| 12 | +// together by this comment. `TestCLIInlineIntentsMatchStore` and | ||
| 13 | +// `TestCLIInlineSidesMatchStore` in web/cli_intent_conformance_test.go parse THIS | ||
| 14 | +// file and the store constants and fail (in the `web` CI job, which is the one | ||
| 15 | +// that runs) naming the file to edit if the two ever disagree. | ||
| 16 | +// | ||
| 17 | +// Keep the declarations below as plain string literals in a single `var … = []string{…}` | ||
| 18 | +// block: the conformance test reads them with a regexp, not a Go parser. | ||
| 19 | + | ||
| 20 | +// InlineIntents is the stance a line comment can declare, in canonical order. | ||
| 21 | +var InlineIntents = []string{"info", "request_change", "kudos"} | ||
| 22 | + | ||
| 23 | +// InlineSides is which column of the diff a line number refers to. | ||
| 24 | +var InlineSides = []string{"old", "new"} | ||
| 25 | + | ||
| 26 | +// DefaultInlineIntent / DefaultInlineSide are what the server assumes when the | ||
| 27 | +// field is omitted; the CLI states them in --help so the default is not a secret. | ||
| 28 | +const ( | ||
| 29 | + DefaultInlineIntent = "info" | ||
| 30 | + DefaultInlineSide = "new" | ||
| 31 | +) | ||
| 32 | + | ||
| 33 | +// ValidInlineIntent reports whether s is a storable intent. | ||
| 34 | +func ValidInlineIntent(s string) bool { return contains(InlineIntents, s) } | ||
| 35 | + | ||
| 36 | +// ValidInlineSide reports whether s is a storable diff side. | ||
| 37 | +func ValidInlineSide(s string) bool { return contains(InlineSides, s) } | ||
| 38 | + | ||
| 39 | +// InlineIntentList / InlineSideList render the vocabulary for an error or help | ||
| 40 | +// string, so no message hand-types the words. | ||
| 41 | +func InlineIntentList() string { return strings.Join(InlineIntents, " | ") } | ||
| 42 | +func InlineSideList() string { return strings.Join(InlineSides, " | ") } | ||
| 43 | + | ||
| 44 | +func contains(list []string, s string) bool { | ||
| 45 | + for _, v := range list { | ||
| 46 | + if v == s { | ||
| 47 | + return true | ||
| 48 | + } | ||
| 49 | + } | ||
| 50 | + return false | ||
| 51 | +} | ||
modified
internal/api/models.go +69 -1 | @@ -91,11 +91,56 @@ type MergeRequestCreate struct { | ||
| 91 | 91 | Draft *bool `json:"draft,omitempty"` |
| 92 | 92 | } |
| 93 | 93 | |
| 94 | -// Comment is an MR comment. | |
| 94 | +// Comment is a conversation comment on an MR or an issue. | |
| 95 | +// | |
| 96 | +// ID is what makes it addressable — `mr delete-comment` / `issue delete-comment` | |
| 97 | +// have nothing to target without it, which is why five ruined review comments on | |
| 98 | +// MR #11 were unrecoverable from the CLI (issue #94). Deleted marks a tombstone: | |
| 99 | +// the row survives a retraction with its body cleared, so a listing still shows | |
| 100 | +// the thread's shape. | |
| 95 | 101 | type Comment struct { |
| 102 | + ID string `json:"id"` | |
| 96 | 103 | Author string `json:"author"` |
| 97 | 104 | Body string `json:"body"` |
| 98 | 105 | CreatedAt string `json:"created_at"` |
| 106 | + Edited bool `json:"edited,omitempty"` | |
| 107 | + Deleted bool `json:"deleted,omitempty"` | |
| 108 | +} | |
| 109 | + | |
| 110 | +// LineComment is a review comment anchored to one line of an MR's diff. | |
| 111 | +// ParentID is set on a reply and names the root comment of its thread. | |
| 112 | +type LineComment struct { | |
| 113 | + ID string `json:"id"` | |
| 114 | + ParentID string `json:"parent_id,omitempty"` | |
| 115 | + Author string `json:"author"` | |
| 116 | + File string `json:"file"` | |
| 117 | + Line int `json:"line"` | |
| 118 | + Side string `json:"side"` | |
| 119 | + Commit string `json:"commit,omitempty"` | |
| 120 | + Intent string `json:"intent"` | |
| 121 | + Body string `json:"body"` | |
| 122 | + Resolved bool `json:"resolved"` | |
| 123 | + CreatedAt string `json:"created_at"` | |
| 124 | + ResolvedAt string `json:"resolved_at,omitempty"` | |
| 125 | +} | |
| 126 | + | |
| 127 | +// LineCommentCreate is the create-a-line-comment request body. Side and Intent | |
| 128 | +// are omitted when empty so the server applies its documented defaults; setting | |
| 129 | +// ParentID makes it a reply, which inherits its parent's anchor. | |
| 130 | +type LineCommentCreate struct { | |
| 131 | + File string `json:"file,omitempty"` | |
| 132 | + Line int `json:"line,omitempty"` | |
| 133 | + Side string `json:"side,omitempty"` | |
| 134 | + Commit string `json:"commit,omitempty"` | |
| 135 | + Intent string `json:"intent,omitempty"` | |
| 136 | + Body string `json:"body"` | |
| 137 | + ParentID string `json:"parent_id,omitempty"` | |
| 138 | +} | |
| 139 | + | |
| 140 | +// LineCommentPage is a page of line comments. | |
| 141 | +type LineCommentPage struct { | |
| 142 | + Page | |
| 143 | + Items []LineComment `json:"items"` | |
| 99 | 144 | } |
| 100 | 145 | |
| 101 | 146 | // Subject is a reviewer/assignee (user or team). |
| @@ -296,6 +341,29 @@ type CommitPage struct { | ||
| 296 | 341 | Items []Commit `json:"items"` |
| 297 | 342 | } |
| 298 | 343 | |
| 344 | +// CacheEntry is one CI dependency-cache entry (actions/cache). | |
| 345 | +type CacheEntry struct { | |
| 346 | + ID string `json:"id"` | |
| 347 | + Key string `json:"key"` | |
| 348 | + Scope string `json:"scope"` | |
| 349 | + Version string `json:"version"` | |
| 350 | + SizeBytes int64 `json:"size_bytes"` | |
| 351 | + CreatedAt string `json:"created_at"` | |
| 352 | + LastAccessedAt string `json:"last_accessed_at"` | |
| 353 | +} | |
| 354 | + | |
| 355 | +// CacheEntryPage is a page of cache entries. | |
| 356 | +type CacheEntryPage struct { | |
| 357 | + Page | |
| 358 | + Items []CacheEntry `json:"items"` | |
| 359 | +} | |
| 360 | + | |
| 361 | +// CacheDeleteResult reports how many entries a purge removed. | |
| 362 | +type CacheDeleteResult struct { | |
| 363 | + Deleted int `json:"deleted"` | |
| 364 | + Key string `json:"key"` | |
| 365 | +} | |
| 366 | + | |
| 299 | 367 | // --- issues / labels / milestones --------------------------------------------- |
| 300 | 368 | |
| 301 | 369 | // Label is a repo label. |
| @@ -91,11 +91,56 @@ type MergeRequestCreate struct { | |||
| 91 | Draft *bool `json:"draft,omitempty"` | 91 | Draft *bool `json:"draft,omitempty"` |
| 92 | } | 92 | } |
| 93 | 93 | ||
| 94 | -// Comment is an MR comment. | 94 | +// Comment is a conversation comment on an MR or an issue. |
| 95 | +// | ||
| 96 | +// ID is what makes it addressable — `mr delete-comment` / `issue delete-comment` | ||
| 97 | +// have nothing to target without it, which is why five ruined review comments on | ||
| 98 | +// MR #11 were unrecoverable from the CLI (issue #94). Deleted marks a tombstone: | ||
| 99 | +// the row survives a retraction with its body cleared, so a listing still shows | ||
| 100 | +// the thread's shape. | ||
| 95 | type Comment struct { | 101 | type Comment struct { |
| 102 | + ID string `json:"id"` | ||
| 96 | Author string `json:"author"` | 103 | Author string `json:"author"` |
| 97 | Body string `json:"body"` | 104 | Body string `json:"body"` |
| 98 | CreatedAt string `json:"created_at"` | 105 | CreatedAt string `json:"created_at"` |
| 106 | + Edited bool `json:"edited,omitempty"` | ||
| 107 | + Deleted bool `json:"deleted,omitempty"` | ||
| 108 | +} | ||
| 109 | + | ||
| 110 | +// LineComment is a review comment anchored to one line of an MR's diff. | ||
| 111 | +// ParentID is set on a reply and names the root comment of its thread. | ||
| 112 | +type LineComment struct { | ||
| 113 | + ID string `json:"id"` | ||
| 114 | + ParentID string `json:"parent_id,omitempty"` | ||
| 115 | + Author string `json:"author"` | ||
| 116 | + File string `json:"file"` | ||
| 117 | + Line int `json:"line"` | ||
| 118 | + Side string `json:"side"` | ||
| 119 | + Commit string `json:"commit,omitempty"` | ||
| 120 | + Intent string `json:"intent"` | ||
| 121 | + Body string `json:"body"` | ||
| 122 | + Resolved bool `json:"resolved"` | ||
| 123 | + CreatedAt string `json:"created_at"` | ||
| 124 | + ResolvedAt string `json:"resolved_at,omitempty"` | ||
| 125 | +} | ||
| 126 | + | ||
| 127 | +// LineCommentCreate is the create-a-line-comment request body. Side and Intent | ||
| 128 | +// are omitted when empty so the server applies its documented defaults; setting | ||
| 129 | +// ParentID makes it a reply, which inherits its parent's anchor. | ||
| 130 | +type LineCommentCreate struct { | ||
| 131 | + File string `json:"file,omitempty"` | ||
| 132 | + Line int `json:"line,omitempty"` | ||
| 133 | + Side string `json:"side,omitempty"` | ||
| 134 | + Commit string `json:"commit,omitempty"` | ||
| 135 | + Intent string `json:"intent,omitempty"` | ||
| 136 | + Body string `json:"body"` | ||
| 137 | + ParentID string `json:"parent_id,omitempty"` | ||
| 138 | +} | ||
| 139 | + | ||
| 140 | +// LineCommentPage is a page of line comments. | ||
| 141 | +type LineCommentPage struct { | ||
| 142 | + Page | ||
| 143 | + Items []LineComment `json:"items"` | ||
| 99 | } | 144 | } |
| 100 | 145 | ||
| 101 | // Subject is a reviewer/assignee (user or team). | 146 | // Subject is a reviewer/assignee (user or team). |
| @@ -296,6 +341,29 @@ type CommitPage struct { | |||
| 296 | Items []Commit `json:"items"` | 341 | Items []Commit `json:"items"` |
| 297 | } | 342 | } |
| 298 | 343 | ||
| 344 | +// CacheEntry is one CI dependency-cache entry (actions/cache). | ||
| 345 | +type CacheEntry struct { | ||
| 346 | + ID string `json:"id"` | ||
| 347 | + Key string `json:"key"` | ||
| 348 | + Scope string `json:"scope"` | ||
| 349 | + Version string `json:"version"` | ||
| 350 | + SizeBytes int64 `json:"size_bytes"` | ||
| 351 | + CreatedAt string `json:"created_at"` | ||
| 352 | + LastAccessedAt string `json:"last_accessed_at"` | ||
| 353 | +} | ||
| 354 | + | ||
| 355 | +// CacheEntryPage is a page of cache entries. | ||
| 356 | +type CacheEntryPage struct { | ||
| 357 | + Page | ||
| 358 | + Items []CacheEntry `json:"items"` | ||
| 359 | +} | ||
| 360 | + | ||
| 361 | +// CacheDeleteResult reports how many entries a purge removed. | ||
| 362 | +type CacheDeleteResult struct { | ||
| 363 | + Deleted int `json:"deleted"` | ||
| 364 | + Key string `json:"key"` | ||
| 365 | +} | ||
| 366 | + | ||
| 299 | // --- issues / labels / milestones --------------------------------------------- | 367 | // --- issues / labels / milestones --------------------------------------------- |
| 300 | 368 | ||
| 301 | // Label is a repo label. | 369 | // Label is a repo label. |
modified
internal/config/config_test.go +53 -0 | @@ -39,6 +39,59 @@ func TestSaveLoadRoundTrip(t *testing.T) { | ||
| 39 | 39 | } |
| 40 | 40 | } |
| 41 | 41 | |
| 42 | +// TestSaveTightensLegacyLooseFile pins the #277 repair: os.WriteFile applies | |
| 43 | +// 0600 only on create, so a config file that already exists with looser | |
| 44 | +// permissions (user-created, restored from a tarball) must be tightened to | |
| 45 | +// 0600 by the next save. | |
| 46 | +func TestSaveTightensLegacyLooseFile(t *testing.T) { | |
| 47 | + for _, mode := range []os.FileMode{0o644, 0o664, 0o666} { | |
| 48 | + dir := t.TempDir() | |
| 49 | + path := filepath.Join(dir, "config.yaml") | |
| 50 | + if err := os.WriteFile(path, []byte("host: http://localhost:3998\n"), 0o600); err != nil { | |
| 51 | + t.Fatalf("seed file: %v", err) | |
| 52 | + } | |
| 53 | + // Chmod (not a WriteFile mode) so the seed is exact under any umask. | |
| 54 | + if err := os.Chmod(path, mode); err != nil { | |
| 55 | + t.Fatalf("seed %o mode: %v", mode, err) | |
| 56 | + } | |
| 57 | + | |
| 58 | + in := &Config{} | |
| 59 | + in.SetToken("http://localhost:3998", "rickub_pat_abc") | |
| 60 | + if err := in.SaveTo(path); err != nil { | |
| 61 | + t.Fatalf("SaveTo over %o file: %v", mode, err) | |
| 62 | + } | |
| 63 | + | |
| 64 | + info, err := os.Stat(path) | |
| 65 | + if err != nil { | |
| 66 | + t.Fatalf("stat: %v", err) | |
| 67 | + } | |
| 68 | + if perm := info.Mode().Perm(); perm != 0o600 { | |
| 69 | + t.Errorf("rewrite of a %o file left perm = %o, want 600", mode, perm) | |
| 70 | + } | |
| 71 | + } | |
| 72 | +} | |
| 73 | + | |
| 74 | +// TestSaveCreatesPrivateDir pins the directory half of the exposure: the | |
| 75 | +// config dir is created 0700 so the token file's name and metadata stay | |
| 76 | +// private too. | |
| 77 | +func TestSaveCreatesPrivateDir(t *testing.T) { | |
| 78 | + path := filepath.Join(t.TempDir(), "rickub", "config.yaml") | |
| 79 | + | |
| 80 | + c := &Config{} | |
| 81 | + c.SetToken("https://rickub.com", "t") | |
| 82 | + if err := c.SaveTo(path); err != nil { | |
| 83 | + t.Fatalf("SaveTo: %v", err) | |
| 84 | + } | |
| 85 | + | |
| 86 | + info, err := os.Stat(filepath.Dir(path)) | |
| 87 | + if err != nil { | |
| 88 | + t.Fatalf("stat dir: %v", err) | |
| 89 | + } | |
| 90 | + if perm := info.Mode().Perm(); perm != 0o700 { | |
| 91 | + t.Errorf("dir perm = %o, want 700", perm) | |
| 92 | + } | |
| 93 | +} | |
| 94 | + | |
| 42 | 95 | func TestLoadMissingIsEmpty(t *testing.T) { |
| 43 | 96 | out, err := LoadFrom(filepath.Join(t.TempDir(), "nope.yaml")) |
| 44 | 97 | if err != nil { |
| @@ -39,6 +39,59 @@ func TestSaveLoadRoundTrip(t *testing.T) { | |||
| 39 | } | 39 | } |
| 40 | } | 40 | } |
| 41 | 41 | ||
| 42 | +// TestSaveTightensLegacyLooseFile pins the #277 repair: os.WriteFile applies | ||
| 43 | +// 0600 only on create, so a config file that already exists with looser | ||
| 44 | +// permissions (user-created, restored from a tarball) must be tightened to | ||
| 45 | +// 0600 by the next save. | ||
| 46 | +func TestSaveTightensLegacyLooseFile(t *testing.T) { | ||
| 47 | + for _, mode := range []os.FileMode{0o644, 0o664, 0o666} { | ||
| 48 | + dir := t.TempDir() | ||
| 49 | + path := filepath.Join(dir, "config.yaml") | ||
| 50 | + if err := os.WriteFile(path, []byte("host: http://localhost:3998\n"), 0o600); err != nil { | ||
| 51 | + t.Fatalf("seed file: %v", err) | ||
| 52 | + } | ||
| 53 | + // Chmod (not a WriteFile mode) so the seed is exact under any umask. | ||
| 54 | + if err := os.Chmod(path, mode); err != nil { | ||
| 55 | + t.Fatalf("seed %o mode: %v", mode, err) | ||
| 56 | + } | ||
| 57 | + | ||
| 58 | + in := &Config{} | ||
| 59 | + in.SetToken("http://localhost:3998", "rickub_pat_abc") | ||
| 60 | + if err := in.SaveTo(path); err != nil { | ||
| 61 | + t.Fatalf("SaveTo over %o file: %v", mode, err) | ||
| 62 | + } | ||
| 63 | + | ||
| 64 | + info, err := os.Stat(path) | ||
| 65 | + if err != nil { | ||
| 66 | + t.Fatalf("stat: %v", err) | ||
| 67 | + } | ||
| 68 | + if perm := info.Mode().Perm(); perm != 0o600 { | ||
| 69 | + t.Errorf("rewrite of a %o file left perm = %o, want 600", mode, perm) | ||
| 70 | + } | ||
| 71 | + } | ||
| 72 | +} | ||
| 73 | + | ||
| 74 | +// TestSaveCreatesPrivateDir pins the directory half of the exposure: the | ||
| 75 | +// config dir is created 0700 so the token file's name and metadata stay | ||
| 76 | +// private too. | ||
| 77 | +func TestSaveCreatesPrivateDir(t *testing.T) { | ||
| 78 | + path := filepath.Join(t.TempDir(), "rickub", "config.yaml") | ||
| 79 | + | ||
| 80 | + c := &Config{} | ||
| 81 | + c.SetToken("https://rickub.com", "t") | ||
| 82 | + if err := c.SaveTo(path); err != nil { | ||
| 83 | + t.Fatalf("SaveTo: %v", err) | ||
| 84 | + } | ||
| 85 | + | ||
| 86 | + info, err := os.Stat(filepath.Dir(path)) | ||
| 87 | + if err != nil { | ||
| 88 | + t.Fatalf("stat dir: %v", err) | ||
| 89 | + } | ||
| 90 | + if perm := info.Mode().Perm(); perm != 0o700 { | ||
| 91 | + t.Errorf("dir perm = %o, want 700", perm) | ||
| 92 | + } | ||
| 93 | +} | ||
| 94 | + | ||
| 42 | func TestLoadMissingIsEmpty(t *testing.T) { | 95 | func TestLoadMissingIsEmpty(t *testing.T) { |
| 43 | out, err := LoadFrom(filepath.Join(t.TempDir(), "nope.yaml")) | 96 | out, err := LoadFrom(filepath.Join(t.TempDir(), "nope.yaml")) |
| 44 | if err != nil { | 97 | if err != nil { |