Merge pull request 'feat: add pr review (task 0013)' (#13) from task-0013-pr-review into main

.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 }`
+- [x] 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
+- [x] 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
+- [x] A server-side 422 for a missing body surfaces as `VALIDATION_ERROR` carrying Gitea's message
-- [ ] `--body-file` works as everywhere else
+- [x] `--body-file` works as everywhere else
-- [ ] Fixture-server tests cover all three actions, the flag-count validations, and the 422 passthrough
+- [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 <n> --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.

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 <command> --help\` for the flags of a command.
@@ -239,6 +241,23 @@ global flags:
   --login <name>              Select a tea login profile by name
 `;
 
+export const PR_REVIEW_HELP = `usage: gitea-axi pr review <number> [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 <text>         Review body
+  --body-file <path>    Read the review body from a file (mutually exclusive with --body)
+  --help                Show this help
+
+global flags:
+  -R, --repo <OWNER/NAME>     Override the repository detected from the git origin remote
+  --login <name>              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<string> {
   });
 }
 
+// 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<string, ReviewAction> = {
+  "--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<string, string | true>): 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<string> {
+  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);
     }

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",
+    });
+  });
+});