feat: add pr review (task 0013)
All checks were successful
CI / test (pull_request) Successful in 38s
All checks were successful
CI / test (pull_request) Successful in 38s
Add `pr review <n>` with the three action switches --approve,
--request-changes, and --comment, plus --body/--body-file. Exactly one
action flag is required; zero or multiple raise VALIDATION_ERROR before
any API call, mirroring the merge shorthand-conflict rule. Body
requirements are left to Gitea: a body-less event it rejects surfaces its
422 as VALIDATION_ERROR carrying the server's message. Output is
`review: { number, action }`.
This commit is contained in:
@@ -12,8 +12,35 @@ Output: `review: { number, action }`.
|
|||||||
|
|
||||||
## Acceptance criteria
|
## 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.
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
import type {
|
import type {
|
||||||
Comment,
|
Comment,
|
||||||
CreatePullRequestOption,
|
CreatePullRequestOption,
|
||||||
|
CreatePullReviewOptions,
|
||||||
EditPullRequestOption,
|
EditPullRequestOption,
|
||||||
MergePullRequestOption,
|
MergePullRequestOption,
|
||||||
PullRequest,
|
PullRequest,
|
||||||
@@ -56,6 +57,7 @@ commands:
|
|||||||
update-branch Merge the base branch into a pull request's head branch
|
update-branch Merge the base branch into a pull request's head branch
|
||||||
close Close a pull request
|
close Close a pull request
|
||||||
reopen Reopen a closed pull request
|
reopen Reopen a closed pull request
|
||||||
|
review Submit a review on a pull request
|
||||||
comment Post a comment on a pull request
|
comment Post a comment on a pull request
|
||||||
|
|
||||||
Run \`gitea-axi pr <command> --help\` for the flags of a command.
|
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
|
--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 = [
|
const PR_CREATE_HELP_SUGGESTION = [
|
||||||
"Run `gitea-axi pr create --help` to see available flags",
|
"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
|
* The assignee list to PATCH: the pull request's current assignees with the
|
||||||
* requested additions applied and removals dropped (fetch-then-patch, ADR 0007).
|
* requested additions applied and removals dropped (fetch-then-patch, ADR 0007).
|
||||||
@@ -1491,6 +1591,9 @@ export function prCommand(deps: CliDeps) {
|
|||||||
if (subcommand === "reopen") {
|
if (subcommand === "reopen") {
|
||||||
return prReopen(deps, rest);
|
return prReopen(deps, rest);
|
||||||
}
|
}
|
||||||
|
if (subcommand === "review") {
|
||||||
|
return prReview(deps, rest);
|
||||||
|
}
|
||||||
if (subcommand === "comment") {
|
if (subcommand === "comment") {
|
||||||
return prComment(deps, rest);
|
return prComment(deps, rest);
|
||||||
}
|
}
|
||||||
|
|||||||
128
test/pr-review.test.ts
Normal file
128
test/pr-review.test.ts
Normal file
@@ -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",
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user