Commit
Keep snapshots working when issues and pull requests are not permitted
commit
4bc062b
2 changed files with +86 and −6
Jump to a changed file
- src/github.js +26 −5
- tests/github-client.test.js +60 −1
modified src/github.js +26 −5
| @@ -26,14 +26,35 @@async function request(path, token, options = {}) { | ||
| 26 | 26 | } |
| 27 | 27 | if (response.status === 403) { |
| 28 | 28 | const error = new Error(`GitHub request failed (${response.status}): ${detail}`); |
| 29 | - error.publicMessage = "GitHub denied access. Grant the GitHub App read-only Contents, Issues, Metadata, and Pull requests permissions, approve the updated installation, then try again."; | |
| 29 | + error.publicMessage = "GitHub denied access. Grant the GitHub App at least read-only Contents and Metadata, approve the updated installation, then try again."; | |
| 30 | + error.forbidden = true; | |
| 30 | 31 | throw error; |
| 31 | 32 | } |
| 32 | - throw new Error(`GitHub request failed (${response.status}): ${detail}`); | |
| 33 | + const error = new Error(`GitHub request failed (${response.status}): ${detail}`); | |
| 34 | + if (response.status === 404) error.missing = true; | |
| 35 | + throw error; | |
| 33 | 36 | } |
| 34 | 37 | return response.json(); |
| 35 | 38 | } |
| 36 | 39 | |
| 40 | +/** | |
| 41 | + * For the parts of a snapshot that are nice to have rather than required. | |
| 42 | + * | |
| 43 | + * Issues and pull requests need their own read permissions, and a repository | |
| 44 | + * can have issues switched off entirely. Neither is a reason to fail the whole | |
| 45 | + * snapshot: the code is what the snapshot is for, and every view already | |
| 46 | + * renders an empty list as "none". A rate-limit 403 still throws, because that | |
| 47 | + * one is temporary and silently producing a half-empty snapshot would hide it. | |
| 48 | + */ | |
| 49 | +async function optional(promise, fallback) { | |
| 50 | + try { | |
| 51 | + return await promise; | |
| 52 | + } catch (error) { | |
| 53 | + if (error.forbidden || error.missing) return fallback; | |
| 54 | + throw error; | |
| 55 | + } | |
| 56 | +} | |
| 57 | + | |
| 37 | 58 | async function allPages(path, token) { |
| 38 | 59 | const rows = []; |
| 39 | 60 | for (let page = 1; ; page += 1) { |
| @@ -252,9 +273,9 @@export function createGitHubClient(config) { | ||
| 252 | 273 | request(`${root}/commits/${encodeURIComponent(repo.default_branch)}`, token), |
| 253 | 274 | allPages(`${root}/branches`, token), |
| 254 | 275 | allPages(`${root}/tags`, token), |
| 255 | - allPages(`${root}/issues?state=all&sort=updated&direction=desc`, token), | |
| 256 | - allPages(`${root}/pulls?state=all&sort=updated&direction=desc`, token), | |
| 257 | - allPages(`${root}/releases`, token), | |
| 276 | + optional(allPages(`${root}/issues?state=all&sort=updated&direction=desc`, token), []), | |
| 277 | + optional(allPages(`${root}/pulls?state=all&sort=updated&direction=desc`, token), []), | |
| 278 | + optional(allPages(`${root}/releases`, token), []), | |
| 258 | 279 | request(`${root}/languages`, token), |
| 259 | 280 | ]); |
| 260 | 281 |
modified tests/github-client.test.js +60 −1
| @@ -27,10 +27,69 @@describe("production GitHub client", () => { | ||
| 27 | 27 | { status: 403 }, |
| 28 | 28 | ))); |
| 29 | 29 | await expect(client().getViewer("token")).rejects.toMatchObject({ |
| 30 | - publicMessage: expect.stringContaining("read-only Contents, Issues, Metadata, and Pull requests"), | |
| 30 | + publicMessage: expect.stringContaining("read-only Contents and Metadata"), | |
| 31 | 31 | }); |
| 32 | 32 | }); |
| 33 | 33 | |
| 34 | + it("snapshots the code even when issues and pull requests are forbidden", async () => { | |
| 35 | + // Contents and Metadata only: the installation can read the repository and | |
| 36 | + // its files, but every issue and pull request endpoint answers 403. The | |
| 37 | + // snapshot has to survive that, because the code is the point of it. | |
| 38 | + vi.stubGlobal("fetch", vi.fn(async (url) => { | |
| 39 | + const path = String(url); | |
| 40 | + if (path.includes("/issues") || path.includes("/pulls") || path.includes("/releases")) { | |
| 41 | + return json({ message: "Resource not accessible by integration" }, { status: 403 }); | |
| 42 | + } | |
| 43 | + if (path.endsWith("/repos/rasmusjy/demo")) { | |
| 44 | + return json({ id: 7, name: "demo", full_name: "rasmusjy/demo", default_branch: "main", private: true }); | |
| 45 | + } | |
| 46 | + if (path.includes("/commits/main")) { | |
| 47 | + return json({ sha: "abc", commit: { message: "init", tree: { sha: "t1" }, author: { date: "2026-01-01T00:00:00Z" } } }); | |
| 48 | + } | |
| 49 | + if (path.includes("/branches")) return json([{ name: "main", commit: { sha: "abc" } }]); | |
| 50 | + if (path.includes("/languages")) return json({ TypeScript: 100 }); | |
| 51 | + if (path.includes("/git/trees")) return json({ tree: [] }); | |
| 52 | + return json([]); | |
| 53 | + })); | |
| 54 | + | |
| 55 | + const [repo] = await client().snapshotRepositories("token", [ | |
| 56 | + { id: 7, name: "demo", fullName: "rasmusjy/demo" }, | |
| 57 | + ]); | |
| 58 | + | |
| 59 | + expect(repo.name).toBe("demo"); | |
| 60 | + expect(repo.issues).toEqual([]); | |
| 61 | + expect(repo.pullRequests).toEqual([]); | |
| 62 | + expect(repo.releases).toEqual([]); | |
| 63 | + }); | |
| 64 | + | |
| 65 | + it("still fails the snapshot when the rate limit is the reason for the 403", async () => { | |
| 66 | + // A rate-limit 403 is temporary. Swallowing it would quietly produce a | |
| 67 | + // snapshot with no issues in it and no sign that anything went wrong. | |
| 68 | + vi.stubGlobal("fetch", vi.fn(async (url) => { | |
| 69 | + const path = String(url); | |
| 70 | + if (path.includes("/issues")) { | |
| 71 | + return new Response(JSON.stringify({ message: "rate limited" }), { | |
| 72 | + status: 403, | |
| 73 | + headers: { "Content-Type": "application/json", "x-ratelimit-remaining": "0" }, | |
| 74 | + }); | |
| 75 | + } | |
| 76 | + if (path.endsWith("/repos/rasmusjy/demo")) { | |
| 77 | + return json({ id: 7, name: "demo", full_name: "rasmusjy/demo", default_branch: "main" }); | |
| 78 | + } | |
| 79 | + if (path.includes("/commits/main")) { | |
| 80 | + return json({ sha: "abc", commit: { message: "init", tree: { sha: "t1" }, author: { date: "2026-01-01T00:00:00Z" } } }); | |
| 81 | + } | |
| 82 | + if (path.includes("/branches")) return json([{ name: "main", commit: { sha: "abc" } }]); | |
| 83 | + if (path.includes("/languages")) return json({}); | |
| 84 | + if (path.includes("/git/trees")) return json({ tree: [] }); | |
| 85 | + return json([]); | |
| 86 | + })); | |
| 87 | + | |
| 88 | + await expect( | |
| 89 | + client().snapshotRepositories("token", [{ id: 7, name: "demo", fullName: "rasmusjy/demo" }]), | |
| 90 | + ).rejects.toThrow(/rate limit/i); | |
| 91 | + }); | |
| 92 | + | |
| 34 | 93 | it("snapshots an empty repository without requesting a branch, tree, or commits", async () => { |
| 35 | 94 | const calls = []; |
| 36 | 95 | vi.stubGlobal("fetch", vi.fn(async (url) => { |