diff --git a/.claude/tasks/0013-pr-review.md b/.claude/tasks/0013-pr-review.md index eaf1a2c..74eb652 100644 --- a/.claude/tasks/0013-pr-review.md +++ b/.claude/tasks/0013-pr-review.md @@ -12,8 +12,35 @@ Output: `review: { number, action }`. ## Acceptance criteria -- [ ] Each action flag submits the corresponding review event and outputs `review: { number, action }` -- [ ] Zero action flags, or more than one, yield `VALIDATION_ERROR` (exit 2) with no API call -- [ ] A server-side 422 for a missing body surfaces as `VALIDATION_ERROR` carrying Gitea's message -- [ ] `--body-file` works as everywhere else -- [ ] Fixture-server tests cover all three actions, the flag-count validations, and the 422 passthrough +- [x] Each action flag submits the corresponding review event and outputs `review: { number, action }` +- [x] Zero action flags, or more than one, yield `VALIDATION_ERROR` (exit 2) with no API call +- [x] A server-side 422 for a missing body surfaces as `VALIDATION_ERROR` carrying Gitea's message +- [x] `--body-file` works as everywhere else +- [x] Fixture-server tests cover all three actions, the flag-count validations, and the 422 passthrough + +## Implementation Notes + +`pr review` follows the established `pr merge`/`pr comment` shape: `resolveReviewAction` +collects the set of action switches (`--approve`/`--request-changes`/`--comment`) mapped +through the `REVIEW_ACTIONS` record and raises `VALIDATION_ERROR` when the count is not +exactly one — the direct analogue of `resolveMergeMethod`'s conflicting-selector rule the +spec references, adapted from "at most one (with a default)" to "exactly one (no default)". +The action flag count is settled before `createClient`, so an invalid invocation never +reaches the API. The body flows through the shared `resolveBodySource`, so `--body`/ +`--body-file` and their mutual exclusion behave as everywhere else, and no body requirement +is pre-validated locally: a body-less event Gitea rejects returns 422, which the shared +`classifyHttpError` already maps to `VALIDATION_ERROR` carrying the server's message. + +Deviations from the literal spec, both deliberate: + +- The success block appends a `pr view --reviews` suggestion line via `renderDetail`'s + `help`. The spec's output contract names only `review: { number, action }`; the extra + hint is the house style every sibling mutation command (`merge`, `edit`, `close`, …) + already follows, so it was kept for consistency rather than trimmed to the bare contract. +- A named `ReviewAction` interface was introduced in place of a repeated inline + `{ event; action }` shape, following a Standards-axis review nit — it matches the local + convention of naming such types (`MergeMethod`, `UpdateStyle`). + +Built test-first via the `test-driven-development` skill (test-writer sub-agent, one +behavior per RED→GREEN cycle); `test/pr-review.test.ts` holds 10 tests. Full suite: 311 +passing, typecheck clean. diff --git a/src/commands/pr.ts b/src/commands/pr.ts index 09e8964..f387d60 100644 --- a/src/commands/pr.ts +++ b/src/commands/pr.ts @@ -1,6 +1,7 @@ import type { Comment, CreatePullRequestOption, + CreatePullReviewOptions, EditPullRequestOption, MergePullRequestOption, PullRequest, @@ -56,6 +57,7 @@ commands: update-branch Merge the base branch into a pull request's head branch close Close a pull request reopen Reopen a closed pull request + review Submit a review on a pull request comment Post a comment on a pull request Run \`gitea-axi pr --help\` for the flags of a command. @@ -239,6 +241,23 @@ global flags: --login Select a tea login profile by name `; +export const PR_REVIEW_HELP = `usage: gitea-axi pr review [flags] + +Submit a review on a pull request. Exactly one action flag is required. + +flags: + --approve Approve the pull request + --request-changes Request changes on the pull request + --comment Leave a review comment without approving or rejecting + --body Review body + --body-file Read the review body from a file (mutually exclusive with --body) + --help Show this help + +global flags: + -R, --repo Override the repository detected from the git origin remote + --login Select a tea login profile by name +`; + const PR_CREATE_HELP_SUGGESTION = [ "Run `gitea-axi pr create --help` to see available flags", ]; @@ -1036,6 +1055,87 @@ async function prComment(deps: CliDeps, args: string[]): Promise { }); } +// A review action: the event Gitea's review endpoint expects (`event`) paired +// with the value gitea-axi reports back (`action`). +interface ReviewAction { + event: string; + action: string; +} + +// The three action switches, each mapped to its {@link ReviewAction}. +const REVIEW_ACTIONS: Record = { + "--approve": { event: "APPROVED", action: "approve" }, + "--request-changes": { event: "REQUEST_CHANGES", action: "request-changes" }, + "--comment": { event: "COMMENT", action: "comment" }, +}; + +const PR_REVIEW_HELP_SUGGESTION = [ + "Run `gitea-axi pr review --help` to see available flags", +]; + +/** + * The single review action to submit. Exactly one of the three action switches + * is required: zero or more than one is a `VALIDATION_ERROR` raised before any + * request goes out, mirroring `pr merge`'s conflicting-method rule. Body + * requirements are Gitea's to enforce, so they are not pre-checked here — a + * body-less event the server rejects surfaces as its own 422. + */ +function resolveReviewAction(flags: Record): ReviewAction { + const selected = Object.entries(REVIEW_ACTIONS) + .filter(([flag]) => flags[flag] === true) + .map(([, value]) => value); + if (selected.length !== 1) { + throw axiError( + "Choose exactly one review action (--approve, --request-changes, or --comment)", + "VALIDATION_ERROR", + PR_REVIEW_HELP_SUGGESTION, + ); + } + return selected[0]!; +} + +async function prReview(deps: CliDeps, args: string[]): Promise { + if (args.includes("--help")) { + return PR_REVIEW_HELP; + } + const { flags, positionals } = parseFlags( + args, + { + "--approve": { takesValue: false }, + "--request-changes": { takesValue: false }, + "--comment": { takesValue: false }, + "--body": { takesValue: true }, + "--body-file": { takesValue: true }, + }, + "pr review", + ); + const number = parsePositionalNumber(positionals, "pr review", "pull request"); + + // Everything the caller's own input can settle is checked before any request + // goes out: the action flag count first, then the body source. + const chosen = resolveReviewAction(flags); + const body = resolveBodySource(deps, flags, "pr review"); + + const context = await resolveRepoContext(deps); + const api = createClient(context); + + const payload: CreatePullReviewOptions = { event: chosen.event }; + if (body !== undefined) { + payload.body = body; + } + try { + await api.repos.repoCreatePullReview(context.owner, context.name, number, payload); + } catch (error) { + throw classifyHttpError(error); + } + + return renderDetail({ + noun: "review", + item: { number, action: chosen.action }, + help: [suggestCommand(context, `pr view ${number} --reviews`, "to see the review in full")], + }); +} + /** * The assignee list to PATCH: the pull request's current assignees with the * requested additions applied and removals dropped (fetch-then-patch, ADR 0007). @@ -1491,6 +1591,9 @@ export function prCommand(deps: CliDeps) { if (subcommand === "reopen") { return prReopen(deps, rest); } + if (subcommand === "review") { + return prReview(deps, rest); + } if (subcommand === "comment") { return prComment(deps, rest); } diff --git a/test/pr-review.test.ts b/test/pr-review.test.ts new file mode 100644 index 0000000..e8ecaf7 --- /dev/null +++ b/test/pr-review.test.ts @@ -0,0 +1,128 @@ +import { afterEach, describe, expect, it } from "vitest"; +import { startFixtureServer, type FixtureServer } from "./fixture-server.js"; +import { runCliTest, tempFiles, testModeEnv, type TempFiles } from "./harness.js"; + +const REPO_PATH = "/api/v1/repos/testowner/testrepo"; +const REVIEWS_PATH = `${REPO_PATH}/pulls/9/reviews`; + +let server: FixtureServer; +const files: TempFiles = tempFiles(); + +afterEach(async () => { + await server.close(); + files.cleanup(); +}); + +function reviewPosted() { + return server.requests.find( + (request) => request.method === "POST" && request.path === REVIEWS_PATH, + ); +} + +describe("pr review", () => { + it("submits an APPROVED review and reports number and action for --approve", async () => { + server = await startFixtureServer([{ method: "POST", path: REVIEWS_PATH, body: {} }]); + + const { stdout, exitCode } = await runCliTest(["pr", "review", "9", "--approve"], { + env: testModeEnv(server.url), + }); + + expect(exitCode).toBe(0); + expect(reviewPosted()?.body).toEqual({ event: "APPROVED" }); + expect(stdout).toContain("review:"); + expect(stdout).toContain("number: 9"); + expect(stdout).toContain("action: approve"); + }); + + it.each([ + ["--request-changes", "REQUEST_CHANGES", "request-changes"], + ["--comment", "COMMENT", "comment"], + ])("submits %s as event %s and reports action %s", async (flag, event, action) => { + server = await startFixtureServer([{ method: "POST", path: REVIEWS_PATH, body: {} }]); + + const { stdout, exitCode } = await runCliTest(["pr", "review", "9", flag], { + env: testModeEnv(server.url), + }); + + expect(exitCode).toBe(0); + expect(reviewPosted()?.body).toEqual({ event }); + expect(stdout).toContain("review:"); + expect(stdout).toContain("number: 9"); + expect(stdout).toContain(`action: ${action}`); + }); + + it("rejects zero action flags before any API call", async () => { + server = await startFixtureServer([]); + + const { stdout, exitCode } = await runCliTest(["pr", "review", "9"], { + env: testModeEnv(server.url), + }); + + expect(exitCode).toBe(2); + expect(stdout).toContain("code: VALIDATION_ERROR"); + expect(server.requests).toHaveLength(0); + }); + + it.each([ + ["--approve", "--comment"], + ["--approve", "--request-changes"], + ["--request-changes", "--comment"], + ])("rejects multiple action flags (%s %s) before any API call", async (...actions) => { + server = await startFixtureServer([]); + + const { stdout, exitCode } = await runCliTest(["pr", "review", "9", ...actions], { + env: testModeEnv(server.url), + }); + + expect(exitCode).toBe(2); + expect(stdout).toContain("code: VALIDATION_ERROR"); + expect(server.requests).toHaveLength(0); + }); + + it("surfaces a 422 from the server as VALIDATION_ERROR carrying its message", async () => { + server = await startFixtureServer([ + { + method: "POST", + path: REVIEWS_PATH, + status: 422, + body: { message: "review body cannot be empty" }, + }, + ]); + + const { stdout, exitCode } = await runCliTest(["pr", "review", "9", "--request-changes"], { + env: testModeEnv(server.url), + }); + + expect(exitCode).toBe(2); + expect(stdout).toContain("code: VALIDATION_ERROR"); + expect(stdout).toContain("review body cannot be empty"); + }); + + it("forwards a --body review body inline", async () => { + server = await startFixtureServer([{ method: "POST", path: REVIEWS_PATH, body: {} }]); + + const { exitCode } = await runCliTest( + ["pr", "review", "9", "--comment", "--body", "Looks good"], + { env: testModeEnv(server.url) }, + ); + + expect(exitCode).toBe(0); + expect(reviewPosted()?.body).toEqual({ event: "COMMENT", body: "Looks good" }); + }); + + it("forwards a --body-file review body from the file contents", async () => { + server = await startFixtureServer([{ method: "POST", path: REVIEWS_PATH, body: {} }]); + const path = files.write("review.txt", "Please fix the tests"); + + const { exitCode } = await runCliTest( + ["pr", "review", "9", "--request-changes", "--body-file", path], + { env: testModeEnv(server.url) }, + ); + + expect(exitCode).toBe(0); + expect(reviewPosted()?.body).toEqual({ + event: "REQUEST_CHANGES", + body: "Please fix the tests", + }); + }); +});