Imports: split a refused batch of files again, and allow settings while an import runs (#181)
* Imports: a refused batch of files is split again, and settings can change while an import runs
- CI / Typecheck and bundlesuccess
- CI / Unit tests (1/2)success
- CI / Unit tests (2/2)success
- Deploy / Deploy to Cloudflaresuccess
- End-to-end / End-to-end (agent reviews, tasks, Today)Failed
- End-to-end / End-to-end (git, PRs, Actions)success
- End-to-end / End-to-end (large pushes)success
- End-to-end / End-to-end (passkeys, registry, email)Failed
- End-to-end / End-to-end (real histories)success
- End-to-end / End-to-end (security)Failed
10 files changed+105−49
Files (10)
- apps/diffinery/docs/migrate-from-github.md+2−2
- apps/diffinery/src/http/api.ts+8−8
- apps/diffinery/src/http/common.ts+13−2
- apps/diffinery/src/http/deployments.ts+11−11
- apps/diffinery/src/http/ghctx.ts+2−2
- apps/diffinery/src/http/misc.ts+4−4
- apps/diffinery/src/web/repo-settings.tsx+12−12
- apps/diffinery/test/e2e/e2e.test.mjs+7−2
- containers/runner/src/importer.ts+7−6
- containers/runner/test/import.test.ts+39−0
modifiedapps/diffinery/docs/migrate-from-github.md+2−2View file
| @@ -28,7 +28,7 @@ | ||
| 28 | 28 | Things to know: |
| 29 | 29 | |
| 30 | 30 | - **Nothing the import writes fires events.** No workflow runs start, no webhooks are delivered and nobody is notified about imported issues, comments or pushes. Your first push after the import starts workflows as usual. |
| 31 | -- **The repository is locked while the import runs.** Until the import finishes, fails or is cancelled, only the import writes to it. A push from anyone else is refused with `repository is locked by an import in progress`, and any other change through the REST API, the web UI or MCP (an issue, a comment, a label, a setting, a transfer) gets `423 repository is locked by an import in progress; try again when it finishes, or cancel the import`. Reading the repository and watching it work as usual. | |
| 31 | +- **The repository is locked while the import runs.** Until the import finishes, fails or is cancelled, only the import writes to it. A push from anyone else is refused with `repository is locked by an import in progress`, and any other change through the REST API, the web UI or MCP (an issue, a comment, a label, a webhook, a branch protection rule, a rename or transfer) gets `423 repository is locked by an import in progress; try again when it finishes, or cancel the import`. Reading the repository and watching it work as usual, and so do settings that don't touch its Git data, issues or pull requests: Actions secrets and variables, environments with their secrets and variables, and collaborators and their invitations. You can set the imported secrets' values while the history is still arriving. | |
| 32 | 32 | - **Every write is keyed by GitHub's ids**, so a retried step or a retried import never duplicates issues, pull requests, comments, reviews, labels, milestones, branch protection rules or releases (a release is matched by its tag and creation date, and assets already copied are kept). |
| 33 | 33 | - **The import keeps running if you close the page or stop the CLI.** You get an entry in your notifications list when it succeeds (`Imported acme/website into acme/website`) or fails, linking to its page. |
| 34 | 34 | - **Every import is audited:** `import.create`, `import.push`, `import.finish`, `import.fail`, `import.cancel` and `import.retry` appear in the audit log. See [Audit log](/docs/security/audit-log). |
| @@ -591,7 +591,7 @@ | ||
| 591 | 591 | - Finished work isn't repeated or duplicated: steps write by GitHub id, and the Git transfer only sends what the repository doesn't have yet. |
| 592 | 592 | - An agent can't retry once the repository was created (`an agent cannot retry an import after the repository was created; a person who administers acme must retry it`). A person who administers the target retries it with **Retry this import** or the API, giving the GitHub token again if the source is private. |
| 593 | 593 | |
| 594 | -**The repository lock.** From the moment the import creates the repository until it finishes, fails or is cancelled, only the import may write to it. Any other push gets `repository is locked by an import in progress`, and any other write through the REST API, the web UI or MCP gets `423 repository is locked by an import in progress; try again when it finishes, or cancel the import`. Watching the repository still works. The import itself may only create refs or move refs it wrote. A retry claims the repository again only when it holds nothing the import didn't write. If anyone pushed a branch, opened an issue, added a label, a protection rule or a release in the meantime (for example after the import failed and released the lock), the retry fails with `repository already holds content not written by this import; start a new import`. A failed or cancelled import leaves the partly imported repository in place; delete it before importing again under the same name. | |
| 594 | +**The repository lock.** From the moment the import creates the repository until it finishes, fails or is cancelled, only the import may write to it. Any other push gets `repository is locked by an import in progress`, and any other write through the REST API, the web UI or MCP gets `423 repository is locked by an import in progress; try again when it finishes, or cancel the import`. Watching the repository still works, and so do Actions secrets and variables, environments (with their secrets and variables), and collaborators and invitations. The import never writes secret values; its variables step writes GitHub's values, so a variable you set before that step under the same name ends with GitHub's value. The import itself may only create refs or move refs it wrote. A retry claims the repository again only when it holds nothing the import didn't write. If anyone pushed a branch, opened an issue, added a label, a protection rule or a release in the meantime (for example after the import failed and released the lock), the retry fails with `repository already holds content not written by this import; start a new import`. A failed or cancelled import leaves the partly imported repository in place; delete it before importing again under the same name. | |
| 595 | 595 | |
| 596 | 596 | ## Agents and imports |
| 597 | 597 |
modifiedapps/diffinery/src/http/api.ts+8−8View file
| @@ -18,7 +18,7 @@ | ||
| 18 | 18 | import { imports } from "./imports.js"; |
| 19 | 19 | import { schedulerStub } from "../lib/schedules.js"; |
| 20 | 20 | import { agenticApi } from "../agentic/api.js"; |
| 21 | -import { auditRepo, hiddenAuthors, loadRepo, refuseAgent, repoStub } from "./common.js"; | |
| 21 | +import { auditRepo, hiddenAuthors, loadRepo, refuseAgent, repoStub, WHILE_IMPORTING } from "./common.js"; | |
| 22 | 22 | import { repoCreated } from "../mail/notify.js"; |
| 23 | 23 | import { actionsRest } from "./actions-runtime.js"; |
| 24 | 24 | import { compat } from "./compat.js"; |
| @@ -349,7 +349,7 @@ | ||
| 349 | 349 | return c.json((await c.get("dir").invites({ repoId: repo.id, pending: true })).map(inviteJson)); |
| 350 | 350 | }); |
| 351 | 351 | api.delete("/repos/:owner/:repo/invitations/:id", async (c) => { |
| 352 | - const { repo, caller } = await loadRepo(c, P(c).owner, P(c).repo, "admin"); | |
| 352 | + const { repo, caller } = await loadRepo(c, P(c).owner, P(c).repo, "admin", WHILE_IMPORTING); | |
| 353 | 353 | if (!(await c.get("dir").revokeInvite(c.req.param("id"), { repoId: repo.id }))) throw new HttpError(404, "no pending invitation with this id"); |
| 354 | 354 | auditRepo(c, caller, repo, "repo.invite.revoke", { id: c.req.param("id") }); |
| 355 | 355 | return c.body(null, 204); |
| @@ -456,7 +456,7 @@ | ||
| 456 | 456 | return c.json(paginate(c, (await c.get("dir").collaborators(repo.id)).map((x) => ({ ...userJson(c, x.account), permissions: permissionsOf(x.role), role_name: x.role })))); |
| 457 | 457 | }); |
| 458 | 458 | api.put("/repos/:owner/:repo/collaborators/:login", async (c) => { |
| 459 | - const { repo, caller } = await loadRepo(c, P(c).owner, P(c).repo, "admin"); | |
| 459 | + const { repo, caller } = await loadRepo(c, P(c).owner, P(c).repo, "admin", WHILE_IMPORTING); | |
| 460 | 460 | const who = await c.get("dir").accountByLogin(c.req.param("login")); |
| 461 | 461 | if (!who) throw new HttpError(404, "user not found"); |
| 462 | 462 | if (who.kind === "org") throw new HttpError(422, "an organization can't be a collaborator: add its members, or transfer the repository to it"); |
| @@ -553,7 +553,7 @@ | ||
| 553 | 553 | return c.json(membershipJson(c, me, pending.org!, pending.role, "active")); |
| 554 | 554 | }); |
| 555 | 555 | api.delete("/repos/:owner/:repo/collaborators/:login", async (c) => { |
| 556 | - const { repo, caller } = await loadRepo(c, P(c).owner, P(c).repo, "admin"); | |
| 556 | + const { repo, caller } = await loadRepo(c, P(c).owner, P(c).repo, "admin", WHILE_IMPORTING); | |
| 557 | 557 | const who = await c.get("dir").accountByLogin(c.req.param("login")); |
| 558 | 558 | if (who) { |
| 559 | 559 | await c.get("dir").setCollaborator(repo.id, who.id, null); |
| @@ -1140,7 +1140,7 @@ | ||
| 1140 | 1140 | return c.json({ total_count: s.length, secrets: s.map((x) => ({ name: x.name, updated_at: x.updatedAt })) }); |
| 1141 | 1141 | }); |
| 1142 | 1142 | api.put("/repos/:owner/:repo/actions/secrets/:name", async (c) => { |
| 1143 | - const { stub, caller, repo } = await loadRepo(c, P(c).owner, P(c).repo, "admin"); | |
| 1143 | + const { stub, caller, repo } = await loadRepo(c, P(c).owner, P(c).repo, "admin", WHILE_IMPORTING); | |
| 1144 | 1144 | if (caller.kind === "agent") throw new HttpError(403, "agents cannot manage secrets"); |
| 1145 | 1145 | const b = await body<{ value: string; encrypted_value?: string }>(c); |
| 1146 | 1146 | if (typeof b.value !== "string" && typeof b.encrypted_value === "string") throw new HttpError(422, "encrypted_value (sealed box) is not supported yet: send the secret as `value` over HTTPS; it is encrypted at rest"); |
| @@ -1150,7 +1150,7 @@ | ||
| 1150 | 1150 | return c.body(null, 204); |
| 1151 | 1151 | }); |
| 1152 | 1152 | api.delete("/repos/:owner/:repo/actions/secrets/:name", async (c) => { |
| 1153 | - const { stub, caller, repo } = await loadRepo(c, P(c).owner, P(c).repo, "admin"); | |
| 1153 | + const { stub, caller, repo } = await loadRepo(c, P(c).owner, P(c).repo, "admin", WHILE_IMPORTING); | |
| 1154 | 1154 | refuseAgent(caller, "secrets"); |
| 1155 | 1155 | await stub.deleteSecret(c.req.param("name")); |
| 1156 | 1156 | auditRepo(c, caller, repo, "repo.secret.delete", { name: c.req.param("name").toUpperCase() }); |
| @@ -1163,14 +1163,14 @@ | ||
| 1163 | 1163 | return c.json({ total_count: all.length, variables: paginate(c, all) }); |
| 1164 | 1164 | }); |
| 1165 | 1165 | api.delete("/repos/:owner/:repo/actions/variables/:name", async (c) => { |
| 1166 | - const { stub, caller, repo } = await loadRepo(c, P(c).owner, P(c).repo, "admin"); | |
| 1166 | + const { stub, caller, repo } = await loadRepo(c, P(c).owner, P(c).repo, "admin", WHILE_IMPORTING); | |
| 1167 | 1167 | refuseAgent(caller, "variables"); |
| 1168 | 1168 | await stub.setVariable(c.req.param("name"), null); |
| 1169 | 1169 | auditRepo(c, caller, repo, "repo.variable.delete", { name: c.req.param("name").toUpperCase() }); |
| 1170 | 1170 | return c.body(null, 204); |
| 1171 | 1171 | }); |
| 1172 | 1172 | api.put("/repos/:owner/:repo/actions/variables/:name", async (c) => { |
| 1173 | - const { stub, caller, repo } = await loadRepo(c, P(c).owner, P(c).repo, "admin"); | |
| 1173 | + const { stub, caller, repo } = await loadRepo(c, P(c).owner, P(c).repo, "admin", WHILE_IMPORTING); | |
| 1174 | 1174 | refuseAgent(caller, "variables"); |
| 1175 | 1175 | await stub.setVariable(c.req.param("name"), (await body<{ value: string }>(c)).value); |
| 1176 | 1176 | auditRepo(c, caller, repo, "repo.variable.set", { name: c.req.param("name").toUpperCase() }); |
modifiedapps/diffinery/src/http/common.ts+13−2View file
| @@ -22,6 +22,16 @@ | ||
| 22 | 22 | } |
| 23 | 23 | |
| 24 | 24 | /** |
| 25 | + * For `loadRepo`: a settings change that doesn't touch the repository's Git data, issues or pull | |
| 26 | + * requests (secrets, variables, environments, collaborators and invitations) goes through while | |
| 27 | + * an import fills the repository, so people can set it up as it arrives. An import never writes | |
| 28 | + * secret values; its variables and environments step writes GitHub's, so one set here before that | |
| 29 | + * step under the same name ends with GitHub's value. Webhooks stay locked: nothing the import | |
| 30 | + * does may reach one. | |
| 31 | + */ | |
| 32 | +export const WHILE_IMPORTING = { whileImporting: true } as const; | |
| 33 | + | |
| 34 | +/** | |
| 25 | 35 | * Load a repository and the caller's role, enforcing a minimum role (404 hides |
| 26 | 36 | * private repos; signed-out callers get the same answer for private and missing ones). Anonymous callers get read access to public repositories but |
| 27 | 37 | * may only read: any other method needs a signed-in caller, as on GitHub, |
| @@ -31,8 +41,9 @@ | ||
| 31 | 41 | * except the few routes that pass `whileArchived` (unarchiving, deleting, |
| 32 | 42 | * watching), as on GitHub. While a GitHub import fills the repository, every |
| 33 | 43 | * write is refused with 423 except the import's own (its push token) and the |
| 34 | - * routes that pass `whileImporting` (watching, and pushes, which the | |
| 35 | - * repository refuses itself with a message the git client shows). | |
| 44 | + * routes that pass `whileImporting` (watching, settings that don't touch Git | |
| 45 | + * data (see WHILE_IMPORTING), and pushes, which the repository refuses itself | |
| 46 | + * with a message the git client shows). | |
| 36 | 47 | * |
| 37 | 48 | * An owner/name that a repository was transferred or renamed away from leads |
| 38 | 49 | * to it, after the same visibility check, so callers who can't read it still |
modifiedapps/diffinery/src/http/deployments.ts+11−11View file
| @@ -7,14 +7,14 @@ | ||
| 7 | 7 | import type { Caller } from "../repo/types.js"; |
| 8 | 8 | import { baseOf, ghUser, isoSeconds, nodeId, pageOf, paginate, repoApi, repoHtml, setLinks } from "./gh.js"; |
| 9 | 9 | import { intParam, jsonBody, loadTarget, repoParams } from "./ghctx.js"; |
| 10 | -import { refuseAgent } from "./common.js"; | |
| 10 | +import { refuseAgent, WHILE_IMPORTING } from "./common.js"; | |
| 11 | 11 | import { resolveReviewers, reviewDeployments, viewerFor } from "./environment-reviews.js"; |
| 12 | 12 | |
| 13 | 13 | /** Deployments, deployment statuses and environments, with environment secrets and variables (GitHub REST). */ |
| 14 | 14 | export const deployments = new Hono<{ Bindings: Env; Variables: AppVars }>(); |
| 15 | 15 | |
| 16 | 16 | const R = "/repos/:owner/:repo"; |
| 17 | -const load = (c: AppContext, min: "read" | "write" | "admin" = "read") => loadTarget(c, min); | |
| 17 | +const load = (c: AppContext, min: "read" | "write" | "admin" = "read", opts: { whileImporting?: boolean } = {}) => loadTarget(c, min, opts); | |
| 18 | 18 | const api = (c: AppContext) => repoApi(baseOf(c), repoParams(c).owner, repoParams(c).repo); |
| 19 | 19 | const envName = (c: AppContext) => (c.req.param("env") ?? ""); |
| 20 | 20 | |
| @@ -174,7 +174,7 @@ | ||
| 174 | 174 | return c.json(environmentJson(c, await stub.environment(envName(c)))); |
| 175 | 175 | }); |
| 176 | 176 | deployments.put(`${R}/environments/:env`, async (c) => { |
| 177 | - const { stub, caller, repo } = await load(c, "admin"); | |
| 177 | + const { stub, caller, repo } = await load(c, "admin", WHILE_IMPORTING); | |
| 178 | 178 | const b = await jsonBody<{ wait_timer?: unknown; reviewers?: unknown; deployment_branch_policy?: { protected_branches?: boolean; custom_branch_policies?: boolean } | null; prevent_self_review?: unknown }>(c, true); |
| 179 | 179 | if (caller.kind === "agent") throw new HttpError(403, "only repository admins (people, not agents) can manage environments"); |
| 180 | 180 | const reviewers = await resolveReviewers(c, repo, b.reviewers); |
| @@ -189,7 +189,7 @@ | ||
| 189 | 189 | return c.json(environmentJson(c, e)); |
| 190 | 190 | }); |
| 191 | 191 | deployments.delete(`${R}/environments/:env`, async (c) => { |
| 192 | - const { stub, caller } = await load(c, "admin"); | |
| 192 | + const { stub, caller } = await load(c, "admin", WHILE_IMPORTING); | |
| 193 | 193 | await stub.deleteEnvironment(envName(c), caller); |
| 194 | 194 | audit(c, caller, "environment.delete", { environment: envName(c) }); |
| 195 | 195 | return c.body(null, 204); |
| @@ -201,14 +201,14 @@ | ||
| 201 | 201 | return c.json({ total_count: list.length, branch_policies: paginate(c, list).map((p) => ({ ...p, node_id: nodeId("BranchPolicy", `${e.id}/${p.id}`) })) }); |
| 202 | 202 | }); |
| 203 | 203 | deployments.post(`${R}/environments/:env/deployment-branch-policies`, async (c) => { |
| 204 | - const { stub, caller } = await load(c, "admin"); | |
| 204 | + const { stub, caller } = await load(c, "admin", WHILE_IMPORTING); | |
| 205 | 205 | const b = await jsonBody<{ name?: string; type?: string }>(c); |
| 206 | 206 | const p = await stub.addEnvironmentBranchPattern(envName(c), b, caller); |
| 207 | 207 | audit(c, caller, "environment.branch_policy.create", { environment: envName(c), pattern: p.name, type: p.type }); |
| 208 | 208 | return c.json({ ...p, node_id: nodeId("BranchPolicy", p.id) }); |
| 209 | 209 | }); |
| 210 | 210 | deployments.delete(`${R}/environments/:env/deployment-branch-policies/:id`, async (c) => { |
| 211 | - const { stub, caller } = await load(c, "admin"); | |
| 211 | + const { stub, caller } = await load(c, "admin", WHILE_IMPORTING); | |
| 212 | 212 | await stub.removeEnvironmentBranchPattern(envName(c), intParam(c, "id"), caller); |
| 213 | 213 | audit(c, caller, "environment.branch_policy.delete", { environment: envName(c), id: intParam(c, "id") }); |
| 214 | 214 | return c.body(null, 204); |
| @@ -228,7 +228,7 @@ | ||
| 228 | 228 | return c.json({ name: s.name, created_at: isoSeconds(s.updatedAt), updated_at: isoSeconds(s.updatedAt) }); |
| 229 | 229 | }); |
| 230 | 230 | deployments.put(`${R}/environments/:env/secrets/:name`, async (c) => { |
| 231 | - const { stub, caller } = await load(c, "admin"); | |
| 231 | + const { stub, caller } = await load(c, "admin", WHILE_IMPORTING); | |
| 232 | 232 | if (caller.kind === "agent") throw new HttpError(403, "agents cannot manage secrets"); |
| 233 | 233 | const b = await jsonBody<{ value?: string; encrypted_value?: string }>(c); |
| 234 | 234 | if (typeof b.value !== "string" && typeof b.encrypted_value === "string") throw new HttpError(422, "encrypted_value (sealed box) is not supported yet: send the secret as `value` over HTTPS; it is encrypted at rest"); |
| @@ -239,7 +239,7 @@ | ||
| 239 | 239 | return c.body(null, existed ? 204 : 201); |
| 240 | 240 | }); |
| 241 | 241 | deployments.delete(`${R}/environments/:env/secrets/:name`, async (c) => { |
| 242 | - const { stub, caller } = await load(c, "admin"); | |
| 242 | + const { stub, caller } = await load(c, "admin", WHILE_IMPORTING); | |
| 243 | 243 | if (caller.kind === "agent") throw new HttpError(403, "agents cannot manage secrets"); |
| 244 | 244 | await stub.deleteEnvironmentSecret(envName(c), c.req.param("name"), caller); |
| 245 | 245 | audit(c, caller, "environment.secret.delete", { environment: envName(c), name: c.req.param("name").toUpperCase() }); |
| @@ -260,7 +260,7 @@ | ||
| 260 | 260 | return c.json(variableJson(v)); |
| 261 | 261 | }); |
| 262 | 262 | deployments.post(`${R}/environments/:env/variables`, async (c) => { |
| 263 | - const { stub, caller } = await load(c, "admin"); | |
| 263 | + const { stub, caller } = await load(c, "admin", WHILE_IMPORTING); | |
| 264 | 264 | refuseAgent(caller, "variables"); |
| 265 | 265 | const b = await jsonBody<{ name?: string; value?: string }>(c); |
| 266 | 266 | if (typeof b.name !== "string" || typeof b.value !== "string") throw new HttpError(422, "name and value are required"); |
| @@ -270,7 +270,7 @@ | ||
| 270 | 270 | return c.json({}, 201); |
| 271 | 271 | }); |
| 272 | 272 | deployments.patch(`${R}/environments/:env/variables/:name`, async (c) => { |
| 273 | - const { stub, caller } = await load(c, "admin"); | |
| 273 | + const { stub, caller } = await load(c, "admin", WHILE_IMPORTING); | |
| 274 | 274 | refuseAgent(caller, "variables"); |
| 275 | 275 | const b = await jsonBody<{ name?: string; value?: string }>(c); |
| 276 | 276 | const name = c.req.param("name"); |
| @@ -285,7 +285,7 @@ | ||
| 285 | 285 | return c.body(null, 204); |
| 286 | 286 | }); |
| 287 | 287 | deployments.delete(`${R}/environments/:env/variables/:name`, async (c) => { |
| 288 | - const { stub, caller } = await load(c, "admin"); | |
| 288 | + const { stub, caller } = await load(c, "admin", WHILE_IMPORTING); | |
| 289 | 289 | refuseAgent(caller, "variables"); |
| 290 | 290 | await stub.setEnvironmentVariable(envName(c), c.req.param("name"), null, caller); |
| 291 | 291 | audit(c, caller, "environment.variable.delete", { environment: envName(c), name: c.req.param("name").toUpperCase() }); |
modifiedapps/diffinery/src/http/ghctx.ts+2−2View file
| @@ -18,8 +18,8 @@ | ||
| 18 | 18 | * read-only for everyone (comments, reactions and settings included), so any |
| 19 | 19 | * request that isn't a read is refused, whatever role the route needs. |
| 20 | 20 | */ |
| 21 | -export async function loadTarget(c: AppContext, min: Role = "read"): Promise<Awaited<ReturnType<typeof loadRepo>>> { | |
| 22 | - const l = await loadRepo(c, c.req.param("owner") ?? "", c.req.param("repo") ?? "", min); | |
| 21 | +export async function loadTarget(c: AppContext, min: Role = "read", opts: { whileImporting?: boolean } = {}): Promise<Awaited<ReturnType<typeof loadRepo>>> { | |
| 22 | + const l = await loadRepo(c, c.req.param("owner") ?? "", c.req.param("repo") ?? "", min, opts); | |
| 23 | 23 | if (l.repo.archived && !["GET", "HEAD", "OPTIONS"].includes(c.req.method)) throw new HttpError(403, "repository is archived (read-only)"); |
| 24 | 24 | return l; |
| 25 | 25 | } |
modifiedapps/diffinery/src/http/misc.ts+4−4View file
| @@ -4,7 +4,7 @@ | ||
| 4 | 4 | import { HttpError } from "../lib/util.js"; |
| 5 | 5 | import { baseOf, ghRun, isoSeconds, nodeId, repoApi, repoHtml } from "./gh.js"; |
| 6 | 6 | import { intParam, jsonBody, loadTarget, repoParams } from "./ghctx.js"; |
| 7 | -import { auditRepo, refuseAgent } from "./common.js"; | |
| 7 | +import { auditRepo, refuseAgent, WHILE_IMPORTING } from "./common.js"; | |
| 8 | 8 | |
| 9 | 9 | /** |
| 10 | 10 | * Smaller GitHub endpoints scripts and actions call: rate limits, topics, |
| @@ -14,7 +14,7 @@ | ||
| 14 | 14 | export const misc = new Hono<{ Bindings: Env; Variables: AppVars }>(); |
| 15 | 15 | |
| 16 | 16 | const R = "/repos/:owner/:repo"; |
| 17 | -const load = (c: AppContext, min: "read" | "write" | "admin" = "read") => loadTarget(c, min); | |
| 17 | +const load = (c: AppContext, min: "read" | "write" | "admin" = "read", opts: { whileImporting?: boolean } = {}) => loadTarget(c, min, opts); | |
| 18 | 18 | |
| 19 | 19 | // Diffinery doesn't meter API calls; report a generous, constant budget so clients that check it carry on. |
| 20 | 20 | const LIMIT = 15_000; |
| @@ -128,7 +128,7 @@ | ||
| 128 | 128 | return c.json({ name, value: vars[name], created_at: null, updated_at: null }); |
| 129 | 129 | }); |
| 130 | 130 | misc.post(`${R}/actions/variables`, async (c) => { |
| 131 | - const { stub, caller, repo } = await load(c, "admin"); | |
| 131 | + const { stub, caller, repo } = await load(c, "admin", WHILE_IMPORTING); | |
| 132 | 132 | refuseAgent(caller, "variables"); |
| 133 | 133 | const b = await jsonBody<{ name?: string; value?: string }>(c); |
| 134 | 134 | if (typeof b.name !== "string" || typeof b.value !== "string") throw new HttpError(422, "name and value are required", { errors: [{ resource: "ActionsVariable", code: "missing_field", field: typeof b.name !== "string" ? "name" : "value" }] }); |
| @@ -138,7 +138,7 @@ | ||
| 138 | 138 | return c.json({}, 201); |
| 139 | 139 | }); |
| 140 | 140 | misc.patch(`${R}/actions/variables/:name`, async (c) => { |
| 141 | - const { stub, caller, repo } = await load(c, "admin"); | |
| 141 | + const { stub, caller, repo } = await load(c, "admin", WHILE_IMPORTING); | |
| 142 | 142 | refuseAgent(caller, "variables"); |
| 143 | 143 | const name = c.req.param("name"); |
| 144 | 144 | const vars = await stub.variables(false); |
modifiedapps/diffinery/src/web/repo-settings.tsx+12−12View file
| @@ -3,7 +3,7 @@ | ||
| 3 | 3 | import { raw } from "hono/html"; |
| 4 | 4 | import type { Child, FC } from "hono/jsx"; |
| 5 | 5 | import { requireAccount, requireRecentSignIn, type AppContext } from "../auth/principal.js"; |
| 6 | -import { loadRepo } from "../http/common.js"; | |
| 6 | +import { loadRepo, WHILE_IMPORTING } from "../http/common.js"; | |
| 7 | 7 | import { HttpError } from "../lib/util.js"; |
| 8 | 8 | import { MAX_REVIEWERS, MAX_WAIT_TIMER, type Environment } from "../repo/deployments.js"; |
| 9 | 9 | import { resolveReviewers } from "../http/environment-reviews.js"; |
| @@ -20,7 +20,7 @@ | ||
| 20 | 20 | |
| 21 | 21 | const WEBHOOK_EVENTS = ["push", "pull_request", "pull_request_review", "issues", "issue_comment", "workflow_run", "workflow_dispatch"]; |
| 22 | 22 | |
| 23 | -const load = (c: AppContext, opts: { whileArchived?: boolean } = {}) => loadRepo(c, prm(c, "owner"), prm(c, "repo"), "admin", opts); | |
| 23 | +const load = (c: AppContext, opts: { whileArchived?: boolean; whileImporting?: boolean } = {}) => loadRepo(c, prm(c, "owner"), prm(c, "repo"), "admin", opts); | |
| 24 | 24 | |
| 25 | 25 | function audit(c: AppContext, l: Loaded, action: string, detail?: Record<string, unknown>): void { |
| 26 | 26 | const p = c.get("principal"); |
| @@ -818,7 +818,7 @@ | ||
| 818 | 818 | web.get("/:owner/:repo/settings/environments/:env", async (c) => environmentPage(c, await load(c), prm(c, "env"))); |
| 819 | 819 | |
| 820 | 820 | post(web, "/:owner/:repo/settings/environments", async (c, f) => { |
| 821 | - const l = await load(c); | |
| 821 | + const l = await load(c, WHILE_IMPORTING); | |
| 822 | 822 | const name = f.s("name").trim(); |
| 823 | 823 | return act(c, envPath(l), async () => { |
| 824 | 824 | const e = await l.stub.upsertEnvironment(name, {}, l.caller); |
| @@ -828,7 +828,7 @@ | ||
| 828 | 828 | }); |
| 829 | 829 | |
| 830 | 830 | post(web, "/:owner/:repo/settings/environments/:env", async (c, f) => { |
| 831 | - const l = await load(c); | |
| 831 | + const l = await load(c, WHILE_IMPORTING); | |
| 832 | 832 | const name = prm(c, "env"); |
| 833 | 833 | return act( |
| 834 | 834 | c, |
| @@ -850,7 +850,7 @@ | ||
| 850 | 850 | }); |
| 851 | 851 | |
| 852 | 852 | post(web, "/:owner/:repo/settings/environments/:env/delete", async (c) => { |
| 853 | - const l = await load(c); | |
| 853 | + const l = await load(c, WHILE_IMPORTING); | |
| 854 | 854 | const name = prm(c, "env"); |
| 855 | 855 | return act( |
| 856 | 856 | c, |
| @@ -967,7 +967,7 @@ | ||
| 967 | 967 | }); |
| 968 | 968 | |
| 969 | 969 | post(web, "/:owner/:repo/settings/access", async (c, f) => { |
| 970 | - const l = await load(c); | |
| 970 | + const l = await load(c, WHILE_IMPORTING); | |
| 971 | 971 | const dir = c.get("dir"); |
| 972 | 972 | return act(c, `${repoUrl(l.repo)}/settings/access`, async () => { |
| 973 | 973 | const who = await dir.accountByLogin(f.s("login").trim()); |
| @@ -990,7 +990,7 @@ | ||
| 990 | 990 | }); |
| 991 | 991 | |
| 992 | 992 | post(web, "/:owner/:repo/settings/access/invites/:id/revoke", async (c) => { |
| 993 | - const l = await load(c); | |
| 993 | + const l = await load(c, WHILE_IMPORTING); | |
| 994 | 994 | return act(c, `${repoUrl(l.repo)}/settings/access`, async () => { |
| 995 | 995 | if (!(await c.get("dir").revokeInvite(prm(c, "id"), { repoId: l.repo.id }))) throw new HttpError(404, "this invitation was already used, revoked or never existed"); |
| 996 | 996 | audit(c, l, "repo.invite.revoke", { id: prm(c, "id") }); |
| @@ -998,7 +998,7 @@ | ||
| 998 | 998 | }); |
| 999 | 999 | |
| 1000 | 1000 | post(web, "/:owner/:repo/settings/access/people", async (c, f) => { |
| 1001 | - const l = await load(c); | |
| 1001 | + const l = await load(c, WHILE_IMPORTING); | |
| 1002 | 1002 | const me = requireAccount(c); |
| 1003 | 1003 | const role = ROLES.find((r) => r === f.s("role")) ?? "write"; |
| 1004 | 1004 | let invited: { email: string; url: string } | undefined; |
| @@ -1051,7 +1051,7 @@ | ||
| 1051 | 1051 | }); |
| 1052 | 1052 | |
| 1053 | 1053 | post(web, "/:owner/:repo/settings/secrets", async (c, f) => { |
| 1054 | - const l = await load(c); | |
| 1054 | + const l = await load(c, WHILE_IMPORTING); | |
| 1055 | 1055 | return act( |
| 1056 | 1056 | c, |
| 1057 | 1057 | `${repoUrl(l.repo)}/settings/secrets`, |
| @@ -1071,7 +1071,7 @@ | ||
| 1071 | 1071 | }); |
| 1072 | 1072 | |
| 1073 | 1073 | post(web, "/:owner/:repo/settings/secrets/delete", async (c, f) => { |
| 1074 | - const l = await load(c); | |
| 1074 | + const l = await load(c, WHILE_IMPORTING); | |
| 1075 | 1075 | return act(c, `${repoUrl(l.repo)}/settings/secrets`, async () => { |
| 1076 | 1076 | await l.stub.deleteSecret(f.s("name")); |
| 1077 | 1077 | audit(c, l, "repo.secret.delete", { name: f.s("name") }); |
| @@ -1079,7 +1079,7 @@ | ||
| 1079 | 1079 | }); |
| 1080 | 1080 | |
| 1081 | 1081 | post(web, "/:owner/:repo/settings/variables", async (c, f) => { |
| 1082 | - const l = await load(c); | |
| 1082 | + const l = await load(c, WHILE_IMPORTING); | |
| 1083 | 1083 | return act(c, `${repoUrl(l.repo)}/settings/secrets`, async () => { |
| 1084 | 1084 | const name = f.s("name").trim(); |
| 1085 | 1085 | if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(name)) throw new HttpError(422, "invalid variable name"); |
| @@ -1089,7 +1089,7 @@ | ||
| 1089 | 1089 | }); |
| 1090 | 1090 | |
| 1091 | 1091 | post(web, "/:owner/:repo/settings/variables/delete", async (c, f) => { |
| 1092 | - const l = await load(c); | |
| 1092 | + const l = await load(c, WHILE_IMPORTING); | |
| 1093 | 1093 | return act(c, `${repoUrl(l.repo)}/settings/secrets`, async () => { |
| 1094 | 1094 | await l.stub.setVariable(f.s("name"), null); |
| 1095 | 1095 | audit(c, l, "repo.variable.delete", { name: f.s("name") }); |
modifiedapps/diffinery/test/e2e/e2e.test.mjs+7−2View file
| @@ -2498,8 +2498,9 @@ | ||
| 2498 | 2498 | const started = await ok("POST", "/diffinery/imports", { source: "octo/app", owner, name: "imported-app", github_token: GH_TOKEN, github_api_url: apiUrl }); |
| 2499 | 2499 | assert.equal(started.status, "queued"); |
| 2500 | 2500 | assert.ok(!JSON.stringify(started).includes(GH_TOKEN), "the token is never returned"); |
| 2501 | - // While the import holds the repository it created, nothing else writes to it: the REST API, the | |
| 2502 | - // web UI and MCP get 423 Locked (reads still work). The import's own writes go through below. | |
| 2501 | + // While the import holds the repository it created, nothing else writes to it but settings such as | |
| 2502 | + // secrets: the REST API, the web UI and MCP get 423 Locked (reads still work). The import's own | |
| 2503 | + // writes go through below. | |
| 2503 | 2504 | await until(`import ${started.id} to wait for an agent`, async () => (await ok("GET", `/diffinery/imports/${started.id}`)).git?.phase === "waiting", 120_000); |
| 2504 | 2505 | const L = `/repos/${owner}/imported-app`; |
| 2505 | 2506 | for (const [method, path, body] of [ |
| @@ -2514,6 +2515,10 @@ | ||
| 2514 | 2515 | assert.match(r.json.message, /locked by an import/); |
| 2515 | 2516 | } |
| 2516 | 2517 | assert.equal((await api("GET", `${L}/issues`)).status, 200, "reads are not locked"); |
| 2518 | + // Settings that don't touch Git data, issues or pull requests can be set up while it imports. | |
| 2519 | + assert.equal((await api("PUT", `${L}/actions/secrets/EARLY_SECRET`, { value: "set during the import" })).status, 204); | |
| 2520 | + assert.ok((await ok("GET", `${L}/actions/secrets`)).secrets.some((x) => x.name === "EARLY_SECRET")); | |
| 2521 | + assert.equal((await api("DELETE", `${L}/actions/secrets/EARLY_SECRET`)).status, 204); | |
| 2517 | 2522 | const csrf = randomBytes(16).toString("hex"); |
| 2518 | 2523 | const webWrite = await fetch(`${base}/${owner}/imported-app/issues`, { method: "POST", headers: { cookie: `dfy_csrf=${csrf}`, "content-type": "application/x-www-form-urlencoded" }, body: new URLSearchParams({ _csrf: csrf, title: "too early" }), redirect: "manual" }); |
| 2519 | 2524 | assert.equal(webWrite.status, 423, await webWrite.text()); |
modifiedcontainers/runner/src/importer.ts+7−6View file
| @@ -395,14 +395,15 @@ | ||
| 395 | 395 | .map((l) => l.split(" ")) |
| 396 | 396 | .filter((p) => p[1] === "blob") |
| 397 | 397 | .map((p) => ({ oid: p[0]!, size: Number(p[2]) })); |
| 398 | - const batches = batchBySize(blobs, this.budget); | |
| 399 | - for (const [i, batch] of batches.entries()) { | |
| 398 | + // Batched at the budget as it stands: when the server refuses a batch the budget shrinks, and | |
| 399 | + // what is left is batched again at the new one. A lone object is retried as is (it can't be | |
| 400 | + // split further), until the budget can't shrink any more. | |
| 401 | + for (let left = blobs, n = 1; left.length; n++) { | |
| 402 | + const batch = batchBySize(left, this.budget)[0]!; | |
| 400 | 403 | const tree = (await this.git.run(["mktree"], { input: batch.map((b, j) => `100644 blob ${b.oid}\tobject-${j}\n`).join("") })).stdout.trim(); |
| 401 | - const carrier = (await this.git.run(["commit-tree", tree, "-m", `Diffinery import: objects for ${commit.slice(0, 12)} (${i + 1}/${batches.length})`])).stdout.trim(); | |
| 404 | + const carrier = (await this.git.run(["commit-tree", tree, "-m", `Diffinery import: objects for ${commit.slice(0, 12)} (${n})`])).stdout.trim(); | |
| 402 | 405 | const size = batch.reduce((s, b) => s + b.size, 0); |
| 403 | - while (!(await this.pushTemp(carrier, size, label))) { | |
| 404 | - /* budget shrank; the carrier is retried as is (a lone object can't be split further) */ | |
| 405 | - } | |
| 406 | + if (await this.pushTemp(carrier, size, label)) left = left.slice(batch.length); | |
| 406 | 407 | } |
| 407 | 408 | } |
| 408 | 409 |
modifiedcontainers/runner/test/import.test.ts+39−0View file
| @@ -148,4 +148,43 @@ | ||
| 148 | 148 | expect(log.every((l) => l.endsWith("diffinery-import"))).toBe(true); |
| 149 | 149 | expect(log.some((l) => Number(l.split(" ")[0]) > 153600)).toBe(true); |
| 150 | 150 | }); |
| 151 | + it("splits a refused batch of a large commit's files again, rather than retrying it whole", async () => { | |
| 152 | + const root = mkdtempSync(path.join(tmpdir(), "diffinery-import-")); | |
| 153 | + const work = path.join(root, "work"); | |
| 154 | + g(root, "init", "-q", "-b", "main", work); | |
| 155 | + // One commit over the budget, of files that each fit in a push the server takes, but not all together. | |
| 156 | + for (let i = 0; i < 8; i++) writeFileSync(path.join(work, `f${i}.bin`), randomBytes(40 * 1024)); | |
| 157 | + g(work, "add", "-A"); | |
| 158 | + g(work, "commit", "-q", "-m", "many files"); | |
| 159 | + const source = path.join(root, "source.git"); | |
| 160 | + g(root, "clone", "-q", "--bare", work, source); | |
| 161 | + const target = path.join(root, "target.git"); | |
| 162 | + g(root, "init", "-q", "--bare", target); | |
| 163 | + g(target, "config", "receive.advertisePushOptions", "true"); | |
| 164 | + const hook = path.join(target, "hooks", "pre-receive"); | |
| 165 | + writeFileSync( | |
| 166 | + hook, | |
| 167 | + `#!/bin/sh | |
| 168 | +size=$(du -sb "$GIT_QUARANTINE_PATH" 2>/dev/null | cut -f1) | |
| 169 | +if [ "\${size:-0}" -gt 153600 ]; then echo "pack takes more than 500 reads (or 1 bytes) of storage to unpack; split it into smaller pushes"; exit 1; fi | |
| 170 | +`, | |
| 171 | + ); | |
| 172 | + chmodSync(hook, 0o755); | |
| 173 | + const spec: ImportSpec = { | |
| 174 | + importId: "t2", | |
| 175 | + source: { fullName: "octo/many", cloneUrl: `file://${source}`, defaultBranch: "main" }, | |
| 176 | + githubToken: null, | |
| 177 | + target: { fullName: "me/many", pushUrl: `file://${target}` }, | |
| 178 | + pushToken: "dfy_test", | |
| 179 | + lfs: false, | |
| 180 | + secrets: "bypass", | |
| 181 | + bypassReason: "Imported from GitHub octo/many", | |
| 182 | + // The commit is batched at 256 KB (six files), which the server refuses. | |
| 183 | + chunkBytes: 256 * 1024, | |
| 184 | + }; | |
| 185 | + const result = await transfer(spec, { workRoot: path.join(root, "agent"), progress: async () => {}, log: () => {} }); | |
| 186 | + expect(result.ok).toBe(true); | |
| 187 | + expect(g(target, "rev-parse", "refs/heads/main")).toBe(g(work, "rev-parse", "HEAD")); | |
| 188 | + g(target, "fsck", "--no-progress"); | |
| 189 | + }); | |
| 151 | 190 | }); |