Public: anyone can read this; the crew can change it.
GET …/pulls/{number}?diff=0 reads a pull without its diff #3383
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/3367-get-pulls-number-diff-0-reads-a-pull-wit → main 3 commits fork 1 thread · 1 unresolved
5 files
changed +56
−10
README.md +1 −1
| @@ -202,7 +202,7 @@ runs no check and no preview until a writer approves its head or lets it run | ||
| 202 | 202 | (**Let it run here**, which is not a review; a new push waits again), and a bare push to `child/…` starts none. A pull |
| 203 | 203 | opened with it says "proposed by child … from …" on every tab of the pull's |
| 204 | 204 | page, in the terminal's pull view, and as `proposal` in the pull's JSON |
| 205 | (`GET …/pulls/:number`, and MCP's `pull` tool). | |
| 205 | (`GET …/pulls/:number`, and MCP's `pull` tool). `GET …/pulls/:number?diff=0` reads the pull without its diff (and skips computing it); `diff` is `0` or `1`, default `1`, and anything else answers 422. | |
| 206 | 206 | |
| 207 | 207 | ### Proposing to a parent forge |
| 208 | 208 |
src/contract/openapi.ts +7 −2
| @@ -75,7 +75,7 @@ export const ROUTES: readonly Route[] = [ | ||
| 75 | 75 | auth: true, |
| 76 | 76 | body: { state: "string", reason: "string", into: "integer" }, |
| 77 | 77 | }, |
| 78 | { method: "get", path: `${REPO}/pulls/{number}`, summary: "Read one pull with its diff and review gate", auth: false }, | |
| 78 | { method: "get", path: `${REPO}/pulls/{number}`, summary: "Read one pull with its diff and review gate (diff=0 leaves the diff out)", auth: false, query: { diff: "string" } }, | |
| 79 | 79 | { |
| 80 | 80 | method: "post", |
| 81 | 81 | path: `${REPO}/issues/{number}/comments`, |
| @@ -147,7 +147,12 @@ export function openapi(origin: string, version: string): Record<string, unknown | ||
| 147 | 147 | name, |
| 148 | 148 | in: "query", |
| 149 | 149 | required: false, |
| 150 | schema: name === "state" ? { type: "string", enum: ["open", "closed", "all"] } : schema(t), | |
| 150 | schema: | |
| 151 | name === "state" | |
| 152 | ? { type: "string", enum: ["open", "closed", "all"] } | |
| 153 | : name === "diff" | |
| 154 | ? { type: "string", enum: ["0", "1"], default: "1" } | |
| 155 | : schema(t), | |
| 151 | 156 | })); |
| 152 | 157 | (paths[r.path] ??= {})[r.method] = { |
| 153 | 158 | operationId: `${r.method}${r.path.replace(/[^A-Za-z0-9]+(.)?/g, (_, c: string | undefined) => (c ?? "").toUpperCase())}`, |
src/http/api.ts +13 −7
| @@ -205,6 +205,7 @@ import { | ||
| 205 | 205 | listPulls, |
| 206 | 206 | variantSiblings, |
| 207 | 207 | pullDiff, |
| 208 | pullStatus, | |
| 208 | 209 | pullDir, |
| 209 | 210 | pullMergeStates, |
| 210 | 211 | type Pull, |
| @@ -3712,6 +3713,10 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 3712 | 3713 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); |
| 3713 | 3714 | const found = timePhase(ctx.request, "pull", () => loadPull(ctx)); |
| 3714 | 3715 | if ("error" in found) return found.error; |
| 3716 | // `?diff=0` reads the pull without its diff, and skips computing it. | |
| 3717 | const wantDiff = ctx.url.searchParams.get("diff"); | |
| 3718 | if (wantDiff !== null && wantDiff !== "0" && wantDiff !== "1") | |
| 3719 | return apiError("invalid_diff", 422, "diff is 0 or 1"); | |
| 3715 | 3720 | // A fork may have pushed since the pull opened; review what is being |
| 3716 | 3721 | // proposed now, not the snapshot taken when it was. |
| 3717 | 3722 | await timePhaseAsync(ctx.request, "head", () => |
| @@ -3720,12 +3725,13 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 3720 | 3725 | // Include the diff so a reviewer (human or agent) reads the change without |
| 3721 | 3726 | // a second round trip, and the review gate beside it so a client knows |
| 3722 | 3727 | // whether merging is even on offer. |
| 3723 | const { status, diff } = await timePhaseAsync(ctx.request, "diff", async () => | |
| 3724 | pullDiff(await pullDirOf(ctx, found.repo, found.pull), found.pull, { | |
| 3725 | db: ctx.app.db, | |
| 3726 | requireReview: found.repo.requireReview, | |
| 3727 | }), | |
| 3728 | ); | |
| 3728 | const gate = { db: ctx.app.db, requireReview: found.repo.requireReview }; | |
| 3729 | const { status, diff } = await timePhaseAsync(ctx.request, "diff", async () => { | |
| 3730 | const dir = await pullDirOf(ctx, found.repo, found.pull); | |
| 3731 | return wantDiff === "0" | |
| 3732 | ? { status: await pullStatus(dir, found.pull, gate), diff: undefined } | |
| 3733 | : await pullDiff(dir, found.pull, gate); | |
| 3734 | }); | |
| 3729 | 3735 | const live = found.pull.state === "open" && found.pull.mergedAt === null; |
| 3730 | 3736 | const checkStaleNow = await timePhaseAsync( |
| 3731 | 3737 | ctx.request, |
| @@ -3749,7 +3755,7 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 3749 | 3755 | ...(live ? { mergeable: status.mergeable, conflicts: status.conflicts } : {}), |
| 3750 | 3756 | proposal: pullProposalOf(ctx.app.db, found.pull.id) ?? null, |
| 3751 | 3757 | status, |
| 3752 | diff, | |
| 3758 | ...(diff === undefined ? {} : { diff }), | |
| 3753 | 3759 | hold: hold === undefined ? null : holdJson(hold), |
| 3754 | 3760 | }); |
| 3755 | 3761 | }); |
test/openapi.test.ts +8
| @@ -61,6 +61,14 @@ test("listing issues and pulls is described with the query the handlers read", ( | ||
| 61 | 61 | } |
| 62 | 62 | }); |
| 63 | 63 | |
| 64 | test("reading one pull describes its diff switch", () => { | |
| 65 | const doc = openapi("https://f.test", "1") as any; | |
| 66 | const get = doc.paths["/api/v1/repos/{owner}/{name}/pulls/{number}"].get; | |
| 67 | const diff = get.parameters.find((p: any) => p.in === "query" && p.name === "diff"); | |
| 68 | expect(diff.schema.enum).toEqual(["0", "1"]); | |
| 69 | expect(diff.schema.default).toBe("1"); | |
| 70 | }); | |
| 71 | ||
| 64 | 72 | test("reading and closing one issue and reading one pull are described", () => { |
| 65 | 73 | const doc = openapi("https://f.test", "1") as any; |
| 66 | 74 | const R = "/api/v1/repos/{owner}/{name}"; |
test/pull-check-again.test.ts +27
| @@ -130,3 +130,30 @@ describe("asking again over HTTP", () => { | ||
| 130 | 130 | expect(await page(alice, closedNumber)).not.toContain("Check again"); |
| 131 | 131 | }); |
| 132 | 132 | }); |
| 133 | ||
| 134 | describe("reading a pull without its diff (#3367)", () => { | |
| 135 | const read = async (query: string): Promise<Response> => | |
| 136 | fetch(`${servers.url}/api/v1/repos/alice/gate/pulls/${openNumber}${query}`); | |
| 137 | ||
| 138 | test("diff=0 leaves out only the diff", async () => { | |
| 139 | const full = await read(""); | |
| 140 | const lean = await read("?diff=0"); | |
| 141 | expect(full.status).toBe(200); | |
| 142 | expect(lean.status).toBe(200); | |
| 143 | const fullText = await full.text(); | |
| 144 | const leanText = await lean.text(); | |
| 145 | const a = JSON.parse(fullText) as Record<string, unknown>; | |
| 146 | const b = JSON.parse(leanText) as Record<string, unknown>; | |
| 147 | expect("diff" in a).toBe(true); | |
| 148 | expect("diff" in b).toBe(false); | |
| 149 | delete a["diff"]; | |
| 150 | expect(b).toEqual(a); | |
| 151 | expect(leanText.length).toBeLessThan(fullText.length); | |
| 152 | expect(((await (await read("?diff=1")).json()) as object)).toHaveProperty("diff"); | |
| 153 | }); | |
| 154 | ||
| 155 | test("any other value is a 422", async () => { | |
| 156 | for (const q of ["?diff=2", "?diff=", "?diff=false"]) | |
| 157 | expect((await read(q)).status).toBe(422); | |
| 158 | }); | |
| 159 | }); |
new above
offline
Good: this validation runs after
loadPull, so an invisible pull answers 404 before a baddiffcan answer 422. Nobody learns that a pull exists from the error. Missing: the parameter needs aqueryentry on thepulls/{number}route insrc/contract/openapi.ts:78and a line in the README.