Importer fixes from review: a real import lock, idempotent releases, agent import rules, and the campaign's token (#51)
While an import fills a repository every write but the import's own gets 423 (REST, web and MCP, through loadRepo). Release imports are idempotent and count as existing content. An admin of the target owner can retry an agent's import, and agents import where their owner may. A bad IMPORT_MAX_REPO_KB falls back to the default, the CLI prints a working command for environment secrets, and stale texts and README labels match the UI. The import campaign copies IMPORT_GITHUB_TOKEN to Diffinery when it is set on GitHub and says which mode it ran in. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AizKXSsengXZRegJxRATkL
Canberk Demirci authored · GitHub committed signed1 parent cb3e550commit 1e5d220e8ce06a5a7d6463efcd98c85b7e7086cbBrowse files
20 files changed+340−77
Files (20)
- .github/workflows/import-campaign.yml+18−2
- README.md+19−14
- apps/cli/src/commands/migrate.ts+16−5
- apps/cli/test/migrate.test.ts+13−2
- apps/diffinery/src/env.ts+0−2
- apps/diffinery/src/http/common.ts+12−3
- apps/diffinery/src/http/git.ts+2−1
- apps/diffinery/src/imports/service.ts+23−17
- apps/diffinery/src/mail/api.ts+2−2
- apps/diffinery/src/mail/web.tsx+2−2
- apps/diffinery/src/mcp/server.ts+1−1
- apps/diffinery/src/repo/RepoDO.ts+2−8
- apps/diffinery/src/repo/imports.ts+2−2
- apps/diffinery/src/repo/releases.ts+28−9
- apps/diffinery/src/repo/schema.ts+1−0
- apps/diffinery/src/web/account.tsx+1−1
- apps/diffinery/src/web/imports.tsx+1−1
- apps/diffinery/test/e2e/e2e.test.mjs+29−5
- apps/diffinery/test/unit/imports.test.ts+165−0
- scripts/import-campaign.sh+3−0
modified.github/workflows/import-campaign.yml+18−2View file
| @@ -7,7 +7,8 @@ | ||
| 7 | 7 | # Dispatched on GitHub, it starts the same workflow on Diffinery and prints its report; |
| 8 | 8 | # on Diffinery it does the work. Imports use the IMPORT_GITHUB_TOKEN secret when it is |
| 9 | 9 | # set (a read-only token for public repositories), otherwise GitHub's anonymous rate |
| 10 | -# limit applies and large repositories take hours. | |
| 10 | +# limit applies and large repositories take hours. Set on GitHub, it is copied to | |
| 11 | +# Diffinery's repository secrets before the run starts (the main sync doesn't copy it). | |
| 11 | 12 | on: |
| 12 | 13 | workflow_dispatch: |
| 13 | 14 | inputs: |
| @@ -46,6 +47,17 @@ | ||
| 46 | 47 | TOKEN: ${{ secrets.DIFFINERY_LIVE_TOKEN }} |
| 47 | 48 | R: codician-team/diffinery |
| 48 | 49 | steps: |
| 50 | + - name: Copy IMPORT_GITHUB_TOKEN to Diffinery | |
| 51 | + env: | |
| 52 | + IMPORT_GITHUB_TOKEN: ${{ secrets.IMPORT_GITHUB_TOKEN }} | |
| 53 | + run: | | |
| 54 | + set -euo pipefail | |
| 55 | + if [ -n "$IMPORT_GITHUB_TOKEN" ]; then | |
| 56 | + jq -n --arg v "$IMPORT_GITHUB_TOKEN" '{value: $v}' | curl -fsS -o /dev/null -X PUT "$DIFFINERY/api/v3/repos/$R/actions/secrets/IMPORT_GITHUB_TOKEN" -H "Authorization: Bearer $TOKEN" -H "content-type: application/json" --data-binary @- | |
| 57 | + echo "IMPORT_GITHUB_TOKEN is set on GitHub and was copied to Diffinery" | |
| 58 | + else | |
| 59 | + echo "IMPORT_GITHUB_TOKEN is not set on GitHub; Diffinery's own copy is used if it has one" | |
| 60 | + fi | |
| 49 | 61 | - name: Dispatch and follow |
| 50 | 62 | env: |
| 51 | 63 | REPOS: ${{ inputs.repos }} |
| @@ -87,7 +99,11 @@ | ||
| 87 | 99 | MINUTES: ${{ inputs.minutes }} |
| 88 | 100 | RUN_WORKFLOWS: ${{ inputs.run_workflows }} |
| 89 | 101 | run: | |
| 90 | - [ -n "$GITHUB_TOKEN_FOR_IMPORT" ] || echo "::warning::IMPORT_GITHUB_TOKEN is not set, so imports use GitHub's anonymous rate limit" | |
| 102 | + if [ -n "$GITHUB_TOKEN_FOR_IMPORT" ]; then | |
| 103 | + echo "::notice::Mode: imports use the IMPORT_GITHUB_TOKEN secret" | |
| 104 | + else | |
| 105 | + echo "::warning::Mode: token-free. IMPORT_GITHUB_TOKEN is not set, so imports use GitHub's anonymous rate limit" | |
| 106 | + fi | |
| 91 | 107 | # shellcheck disable=SC2086 |
| 92 | 108 | REPORT=report.md scripts/import-campaign.sh $REPOS |
| 93 | 109 | cat report.md >> "$GITHUB_STEP_SUMMARY" |
modifiedREADME.md+19−14View file
| @@ -524,8 +524,8 @@ | ||
| 524 | 524 | |
| 525 | 525 | Start an import three ways: |
| 526 | 526 | |
| 527 | -- **Web** — *Settings → Imports → New*, or the *Import from GitHub* link on *New | |
| 528 | - repository*. Paste `owner/name` (or a GitHub URL), choose where it lands and | |
| 527 | +- **Web** — *Settings → Imports → Import from GitHub*, or the *Import a repository* | |
| 528 | + link on *New repository*. Paste `owner/name` (or a GitHub URL), choose where it lands and | |
| 529 | 529 | whether it stays public or private, and watch each step. For an organization, |
| 530 | 530 | pick the repositories from the list. |
| 531 | 531 | - **CLI** — `diffinery migrate github octo/app` imports one repository and |
| @@ -534,8 +534,8 @@ | ||
| 534 | 534 | client-side path instead (a mirror push plus a best-effort recreation of |
| 535 | 535 | issues and open pull requests). |
| 536 | 536 | - **MCP / agents** — `start_github_import`, then `get_import` to poll and |
| 537 | - `list_imports` to review. An agent can import only into its owner's account or | |
| 538 | - an organization the owner administers. | |
| 537 | + `list_imports` to review. An agent can import only where its owner may: the | |
| 538 | + owner's own account or an organization the owner administers. | |
| 539 | 539 | |
| 540 | 540 | A private repository, Git LFS, Actions variables and branch protection need a |
| 541 | 541 | GitHub token (`GH_TOKEN`/`GITHUB_TOKEN`, or the token field in the form) with |
| @@ -554,8 +554,10 @@ | ||
| 554 | 554 | cleanly. Draft imports left unstarted for seven days are cancelled and their |
| 555 | 555 | stored token is erased. |
| 556 | 556 | |
| 557 | -**Agents.** An agent may import only into its owner's account, or an organization | |
| 558 | -the owner administers, and only when the owner holds at least **write** there. | |
| 557 | +**Agents.** An agent may import only where its owner may import: the owner's own | |
| 558 | +account, or an organization the owner owns or administers (an organization's own | |
| 559 | +agent imports into that organization). The agent's own policy must also allow | |
| 560 | +**write**, the most an agent holds by default. | |
| 559 | 561 | Agents never read or set secrets and never manage **variables**: the repository, |
| 560 | 562 | organization and environment **variable** steps are skipped for an |
| 561 | 563 | agent-started import (and re-checked against a human owner when they do run). An |
| @@ -573,14 +575,15 @@ | ||
| 573 | 575 | - **Labels, milestones, branch protection**, and **Actions variables** (repository |
| 574 | 576 | and organization) and **deployment environments** (names, branch policies and |
| 575 | 577 | variables). |
| 576 | -- **Releases** and their assets, where the server's releases feature is enabled. | |
| 578 | +- **Releases** and their assets. | |
| 577 | 579 | |
| 578 | 580 | What does **not** come across: |
| 579 | 581 | |
| 580 | 582 | - **Secret values.** GitHub never discloses them, and Diffinery never tries to. |
| 581 | 583 | Repository and environment secret **names** are listed so you can re-enter the |
| 582 | - values by hand (`diffinery secret set <NAME> -R owner/repo`), which an admin | |
| 583 | - does — never an agent. | |
| 584 | + values by hand (`diffinery secret set <NAME> -R owner/repo`, or for an | |
| 585 | + environment `diffinery api PUT /repos/owner/repo/environments/<env>/secrets/<NAME> -f value=...`), | |
| 586 | + which an admin does — never an agent. | |
| 584 | 587 | - **People.** No accounts are created. A GitHub user is attributed to a Diffinery |
| 585 | 588 | account only through a verified link, matched by GitHub's **numeric id** so a |
| 586 | 589 | rename can't mis-map and keyed by the full API base so a github.com link and an |
| @@ -589,15 +592,16 @@ | ||
| 589 | 592 | carries the history but can't sign in, be assigned work or cast a vote. Where a |
| 590 | 593 | mannequin authors a squashed merge, the commit uses GitHub's `noreply` address. |
| 591 | 594 | |
| 592 | -Link a GitHub account under *Settings → Linked accounts → GitHub* one of two ways: | |
| 595 | +Link a GitHub account under *Settings → Linked accounts* one of two ways: | |
| 593 | 596 | |
| 594 | 597 | - **Gist proof (no token).** Diffinery shows a one-time code; you put it in a |
| 595 | 598 | **public gist** and confirm your login. Diffinery reads your public profile and |
| 596 | 599 | gists **unauthenticated** on the API base you named and links the account only |
| 597 | 600 | when it finds the code. Nothing but your public data is read. |
| 598 | 601 | - **OAuth (optional).** When `GITHUB_OAUTH_CLIENT_ID` and |
| 599 | - `GITHUB_OAUTH_CLIENT_SECRET` are configured, a *Connect GitHub* button runs the | |
| 600 | - OAuth flow with state bound to your account so a callback can't be replayed. | |
| 602 | + `GITHUB_OAUTH_CLIENT_SECRET` are configured, *Link with GitHub* → | |
| 603 | + *Continue to GitHub* runs the OAuth flow with state bound to your account so a | |
| 604 | + callback can't be replayed. | |
| 601 | 605 | |
| 602 | 606 | A site admin can clear any link (audited). Imported reviews are kept as history |
| 603 | 607 | but **never count as approvals**, even when their author is a linked account — a |
| @@ -606,12 +610,13 @@ | ||
| 606 | 610 | Old history that contains a credential is imported with an audited push-protection |
| 607 | 611 | bypass (the history is already public on GitHub); choose *stop* to fail instead. |
| 608 | 612 | |
| 609 | -**Retrying.** Only the person who started an import may retry it, and a retry | |
| 613 | +**Retrying.** Only whoever started an import may retry it, and a retry | |
| 610 | 614 | needs the GitHub token again (it is never stored past the run). While an import |
| 611 | 615 | is in progress the target repository is **locked** against other writes, and a |
| 612 | 616 | retry refuses to touch a repository that already holds content the import did not |
| 613 | 617 | write — it will not adopt or overwrite an existing repository. An agent may not |
| 614 | -retry once the repository has been created. | |
| 618 | +retry once the repository has been created; a person who administers the target | |
| 619 | +account takes over that retry instead. | |
| 615 | 620 | |
| 616 | 621 | ## Developing |
| 617 | 622 |
modifiedapps/cli/src/commands/migrate.ts+16−5View file
| @@ -271,13 +271,24 @@ | ||
| 271 | 271 | } |
| 272 | 272 | } |
| 273 | 273 | |
| 274 | -/** Secret names across the repository and its environments, with the command to set each. */ | |
| 274 | +/** | |
| 275 | + * Secret names across the repository and its environments, with the command to set each. | |
| 276 | + * Environment secrets (`<environment>/<NAME>`) have no `secret set` option, so they get the | |
| 277 | + * REST call through `diffinery api` instead. | |
| 278 | + */ | |
| 275 | 279 | function secretLines(ctx: Ctx, imp: ImportJson, target: string): void { |
| 276 | - const names = [...(imp.summary.secretNames ?? []), ...(imp.summary.environments?.secretNames ?? [])]; | |
| 277 | - if (!names.length) return; | |
| 280 | + const repoNames = imp.summary.secretNames ?? []; | |
| 281 | + const envNames = imp.summary.environments?.secretNames ?? []; | |
| 282 | + const total = repoNames.length + envNames.length; | |
| 283 | + if (!total) return; | |
| 278 | 284 | ctx.out(""); |
| 279 | - ctx.out(`GitHub does not reveal secret values. Set these ${names.length} secret${names.length === 1 ? "" : "s"} by hand:`); | |
| 280 | - for (const n of names) ctx.out(` diffinery secret set ${n} -R ${target}`); | |
| 285 | + ctx.out(`GitHub does not reveal secret values. Set these ${total} secret${total === 1 ? "" : "s"} by hand:`); | |
| 286 | + for (const n of repoNames) ctx.out(` diffinery secret set ${n} -R ${target}`); | |
| 287 | + for (const n of envNames) { | |
| 288 | + const at = n.lastIndexOf("/"); | |
| 289 | + const [env, name] = [n.slice(0, at), n.slice(at + 1)]; | |
| 290 | + ctx.out(` diffinery api PUT /repos/${target}/environments/${encodeURIComponent(env)}/secrets/${encodeURIComponent(name)} -f value=...`); | |
| 291 | + } | |
| 281 | 292 | } |
| 282 | 293 | |
| 283 | 294 | function renderImport(ctx: Ctx, imp: ImportJson): number { |
modifiedapps/cli/test/migrate.test.ts+13−2View file
| @@ -128,7 +128,7 @@ | ||
| 128 | 128 | source: { full_name: "gh-org/widgets" }, |
| 129 | 129 | target: { full_name: "alice/widgets", html_url: `${srv.url}/alice/widgets` }, |
| 130 | 130 | git: { phase: "done", message: "pushed", bytes: 123, chunks: 1 }, |
| 131 | - summary: { secretNames: ["NPM_TOKEN"], environments: { secretNames: ["prod/DEPLOY_KEY"] } }, | |
| 131 | + summary: { secretNames: ["NPM_TOKEN"], environments: { secretNames: ["prod/DEPLOY_KEY", "Staging EU/API_KEY"] } }, | |
| 132 | 132 | warnings: [] as string[], |
| 133 | 133 | error: null, |
| 134 | 134 | children: [] as unknown[], |
| @@ -154,7 +154,18 @@ | ||
| 154 | 154 | expect(polls).toBeGreaterThanOrEqual(2); |
| 155 | 155 | expect(r.stdout).toContain("Imported gh-org/widgets → alice/widgets"); |
| 156 | 156 | expect(r.stdout).toContain("diffinery secret set NPM_TOKEN -R alice/widgets"); |
| 157 | - expect(r.stdout).toContain("diffinery secret set prod/DEPLOY_KEY -R alice/widgets"); | |
| 157 | + // An environment secret gets a command that works (`secret set` has no environment option). | |
| 158 | + expect(r.stdout).not.toContain("secret set prod/"); | |
| 159 | + const envLines = r.stdout.split("\n").filter((l) => l.includes("/environments/")); | |
| 160 | + expect(envLines.map((l) => l.trim())).toEqual([ | |
| 161 | + "diffinery api PUT /repos/alice/widgets/environments/prod/secrets/DEPLOY_KEY -f value=...", | |
| 162 | + "diffinery api PUT /repos/alice/widgets/environments/Staging%20EU/secrets/API_KEY -f value=...", | |
| 163 | + ]); | |
| 164 | + // Running the printed command (with a value) sets that environment secret. | |
| 165 | + srv.on("PUT", "/api/v3/repos/alice/widgets/environments/Staging%20EU/secrets/API_KEY", { status: 201 }); | |
| 166 | + const set = await run(envLines[1]!.trim().replace(/^diffinery /, "").replace(/\.\.\.$/, "s3cret").split(" "), { env }); | |
| 167 | + expect(set.code).toBe(0); | |
| 168 | + expect(srv.find("PUT", "/api/v3/repos/alice/widgets/environments/Staging%20EU/secrets/API_KEY")[0]!.body).toEqual({ value: "s3cret" }); | |
| 158 | 169 | // The git data was NOT cloned or pushed by the CLI; the server does that. |
| 159 | 170 | expect(srv.find("POST", "/api/v3/user/repos")).toHaveLength(0); |
| 160 | 171 | // The GitHub token never went to Diffinery's own endpoints. |
modifiedapps/diffinery/src/env.ts+0−2View file
| @@ -46,8 +46,6 @@ | ||
| 46 | 46 | MASTER_KEY?: string; |
| 47 | 47 | /** Secret: ES256 private JWK (JSON) used to sign OIDC tokens for jobs. */ |
| 48 | 48 | OIDC_PRIVATE_KEY?: string; |
| 49 | - /** Secret: optional GitHub token used by the importer. */ | |
| 50 | - GITHUB_IMPORT_TOKEN?: string; | |
| 51 | 49 | /** Optional GitHub Enterprise host (e.g. github.example.com) the importer may download release assets from. */ |
| 52 | 50 | IMPORT_GHE_HOST?: string; |
| 53 | 51 | } |
modifiedapps/diffinery/src/http/common.ts+12−3View file
| @@ -27,7 +27,10 @@ | ||
| 27 | 27 | * download) and says so. Writes also need a token with the write scope. |
| 28 | 28 | * Archived repositories refuse every write, whatever role the route needs, |
| 29 | 29 | * except the few routes that pass `whileArchived` (unarchiving, deleting, |
| 30 | - * watching), as on GitHub. | |
| 30 | + * watching), as on GitHub. While a GitHub import fills the repository, every | |
| 31 | + * write is refused with 423 except the import's own (its push token) and the | |
| 32 | + * routes that pass `whileImporting` (watching, and pushes, which the | |
| 33 | + * repository refuses itself with a message the git client shows). | |
| 31 | 34 | * |
| 32 | 35 | * An owner/name that a repository was transferred or renamed away from leads |
| 33 | 36 | * to it, after the same visibility check, so callers who can't read it still |
| @@ -36,7 +39,7 @@ | ||
| 36 | 39 | * `follow` (Git, MCP) serves the repository under the old name instead, and |
| 37 | 40 | * `moved` says it was reached that way. |
| 38 | 41 | */ |
| 39 | -export async function loadRepo(c: AppContext, owner: string, name: string, min: Role = "read", opts: { readOnlyPost?: boolean; whileArchived?: boolean; follow?: boolean } = {}): Promise<{ repo: Repo; caller: Caller; stub: ReturnType<typeof repoStub>; moved: boolean }> { | |
| 42 | +export async function loadRepo(c: AppContext, owner: string, name: string, min: Role = "read", opts: { readOnlyPost?: boolean; whileArchived?: boolean; whileImporting?: boolean; follow?: boolean } = {}): Promise<{ repo: Repo; caller: Caller; stub: ReturnType<typeof repoStub>; moved: boolean }> { | |
| 40 | 43 | const dir = c.get("dir"); |
| 41 | 44 | let repo = await dir.repo(owner, name.replace(/\.git$/, "")); |
| 42 | 45 | const moved = !repo; |
| @@ -66,7 +69,13 @@ | ||
| 66 | 69 | if (reason && !(repo.visibility === "public" && readOnly(req))) throw new HttpError(403, reason); |
| 67 | 70 | } |
| 68 | 71 | if (repo.archived && !reading && !opts.whileArchived) throw new HttpError(403, "repository is archived (read-only)"); |
| 69 | - return { repo, caller, stub: repoStub(c.env, repo.id), moved }; | |
| 72 | + const stub = repoStub(c.env, repo.id); | |
| 73 | + if (!reading && !opts.whileImporting) { | |
| 74 | + const holder = await stub.importLockHolder(); | |
| 75 | + const p = c.get("principal"); | |
| 76 | + if (holder && !(p.type === "account" && p.importId === holder)) throw new HttpError(423, "repository is locked by an import in progress; try again when it finishes, or cancel the import"); | |
| 77 | + } | |
| 78 | + return { repo, caller, stub, moved }; | |
| 70 | 79 | } |
| 71 | 80 | |
| 72 | 81 | /** Record a repository settings change made through the API, like the web settings pages do. */ |
modifiedapps/diffinery/src/http/git.ts+2−1View file
| @@ -40,7 +40,8 @@ | ||
| 40 | 40 | }); |
| 41 | 41 | |
| 42 | 42 | git.post("/:owner/:repo/git-receive-pack", async (c) => { |
| 43 | - const { stub, caller, repo, moved } = await loadRepo(c, c.req.param("owner"), c.req.param("repo"), "write", { follow: true }); | |
| 43 | + // During someone else's import the repository refuses the push itself, with a message the client prints. | |
| 44 | + const { stub, caller, repo, moved } = await loadRepo(c, c.req.param("owner"), c.req.param("repo"), "write", { follow: true, whileImporting: true }); | |
| 44 | 45 | // A GitHub import's own token, while that import is running and filling this repository, may push in import mode. |
| 45 | 46 | const p = c.get("principal"); |
| 46 | 47 | if (p.type === "account" && p.importId && p.tokenId && (await importPushAllowed(c.env, p.importId, repo.id, p.tokenId))) caller.importId = p.importId; |
modifiedapps/diffinery/src/imports/service.ts+23−17View file
| @@ -233,7 +233,7 @@ | ||
| 233 | 233 | * records that releases were not imported. |
| 234 | 234 | */ |
| 235 | 235 | interface ReleaseImporter { |
| 236 | - importRelease(input: { tag_name: string; target_commitish?: string; name?: string | null; body?: string | null; draft?: boolean; prerelease?: boolean; created_at: string; published_at?: string | null; author: { id?: string; login: string; kind?: string }; latest?: boolean }, caller: ImportCaller): Promise<{ id: number }> | { id: number }; | |
| 236 | + importRelease(input: { tag_name: string; target_commitish?: string; name?: string | null; body?: string | null; draft?: boolean; prerelease?: boolean; created_at: string; published_at?: string | null; author: { id?: string; login: string; kind?: string }; latest?: boolean }, caller: ImportCaller): Promise<{ id: number; created?: boolean }> | { id: number; created?: boolean }; | |
| 237 | 237 | importReleaseAsset( |
| 238 | 238 | releaseId: number, |
| 239 | 239 | meta: { name: string; label?: string; content_type?: string; download_count?: number; created_at?: string; updated_at?: string; uploader?: { id?: string; login: string; kind?: string } }, |
| @@ -340,17 +340,17 @@ | ||
| 340 | 340 | |
| 341 | 341 | /** |
| 342 | 342 | * People import into their own account or organizations they own or administer. |
| 343 | - * Agents import only into organizations they belong to, or whose owner or admin owns them. | |
| 343 | + * Agents import only where their owner may: an organization's agent into that | |
| 344 | + * organization, a person's agent wherever that person may (`create` also needs | |
| 345 | + * the agent's own policy to allow write; an agent holds at most write by default). | |
| 344 | 346 | */ |
| 345 | 347 | async canImportInto(actor: Account, owner: Account): Promise<boolean> { |
| 346 | 348 | if (actor.status !== "active" || owner.kind === "agent") return false; |
| 347 | 349 | if (actor.kind === "agent") { |
| 348 | - if (owner.kind !== "org" || !actor.owner_id) return false; | |
| 349 | - if (actor.owner_id === owner.id) return true; | |
| 350 | + if (!actor.owner_id) return false; | |
| 351 | + if (owner.kind === "org" && actor.owner_id === owner.id) return true; | |
| 350 | 352 | const human = await this.dir.account(actor.owner_id); |
| 351 | - if (!human || human.kind !== "user" || human.status !== "active") return false; | |
| 352 | - const role = await this.dir.orgRole(owner.id, human.id); | |
| 353 | - return role === "owner" || role === "admin"; | |
| 353 | + return !!human && human.kind === "user" && (await this.canImportInto(human, owner)); | |
| 354 | 354 | } |
| 355 | 355 | if (actor.kind !== "user") return false; |
| 356 | 356 | if (actor.site_admin) return true; |
| @@ -394,9 +394,10 @@ | ||
| 394 | 394 | const owner = await this.dir.accountByLogin(req.owner); |
| 395 | 395 | if (!owner || owner.kind === "agent") throw new HttpError(422, `no Diffinery user or organization named ${req.owner}`); |
| 396 | 396 | if (!(await this.canImportInto(actor, owner))) { |
| 397 | - throw new HttpError(403, actor.kind === "agent" ? "agents can import only into organizations they or their owner administer" : `you can import only into your own account or organizations you administer, not ${owner.login}`); | |
| 397 | + throw new HttpError(403, actor.kind === "agent" ? `agents can import only where their owner may: the owner's own account or an organization the owner administers, not ${owner.login}` : `you can import only into your own account or organizations you administer, not ${owner.login}`); | |
| 398 | 398 | } |
| 399 | - // An agent may import only if its policy lets it write (H1): an import creates and fills a repository. | |
| 399 | + // An agent may import only if its own policy lets it write (H1): an import creates and fills a repository. | |
| 400 | + // (Its owner may import here, so holds admin; the agent acts with at most its policy's role.) | |
| 400 | 401 | if (actor.kind === "agent" && !atLeast(this.dir.agentMaxRole(actor), "write")) throw new HttpError(403, "this agent's policy does not allow writing; an import needs at least write"); |
| 401 | 402 | // Agents can never bypass push protection: an import they start imports secret-bearing |
| 402 | 403 | // history only if it is clean, never with an admin bypass. |
| @@ -454,7 +455,9 @@ | ||
| 454 | 455 | |
| 455 | 456 | /** Refuse a repository whose GitHub size (KB) is over the configured ceiling (default 10 GB). */ |
| 456 | 457 | private assertRepoSize(sizeKb: number | undefined): void { |
| 457 | - const maxKb = Number(this.env.IMPORT_MAX_REPO_KB) || DEFAULT_MAX_REPO_KB; | |
| 458 | + // A missing, non-numeric, zero or negative setting means the default, never "refuse everything". | |
| 459 | + const configured = Number(this.env.IMPORT_MAX_REPO_KB); | |
| 460 | + const maxKb = Number.isFinite(configured) && configured > 0 ? configured : DEFAULT_MAX_REPO_KB; | |
| 458 | 461 | if (typeof sizeKb === "number" && sizeKb > maxKb) { |
| 459 | 462 | throw new HttpError(422, `the repository is about ${Math.round(sizeKb / 1024 / 1024)} GB, over the ${Math.round(maxKb / 1024 / 1024)} GB import limit; ask an administrator to raise IMPORT_MAX_REPO_KB or split the repository`); |
| 460 | 463 | } |
| @@ -589,15 +592,17 @@ | ||
| 589 | 592 | /** Run a failed or cancelled import again. Everything is an upsert, so finished work is not duplicated. */ |
| 590 | 593 | async retry(id: string, actor: Account, token: string | null): Promise<ImportRow> { |
| 591 | 594 | const row = await this.row(id); |
| 595 | + const owner = row ? await this.dir.account(row.target_owner_id) : null; | |
| 592 | 596 | // Retry is creator-only (Low 7): even an admin of the target cannot resume someone else's import. |
| 593 | - if (!row || row.created_by !== actor.id) throw new HttpError(404, "import not found"); | |
| 597 | + // The one exception is an agent's import, which a person who may import into the target takes over (H2 below). | |
| 598 | + const takeover = async () => !!row && !!owner && actor.kind === "user" && (await this.dir.account(row.created_by))?.kind === "agent" && (await this.canImportInto(actor, owner)); | |
| 599 | + if (!row || (row.created_by !== actor.id && !(await takeover()))) throw new HttpError(404, "import not found"); | |
| 594 | 600 | if (row.parent_id) throw new HttpError(422, "retry the organization import this repository belongs to"); |
| 595 | 601 | if (row.status !== "failed" && row.status !== "cancelled") throw new HttpError(409, "only failed or cancelled imports can be retried"); |
| 596 | 602 | if (token !== null && (token.length > 400 || /\s/.test(token))) throw new HttpError(422, "github_token is not a valid token"); |
| 597 | 603 | // H2: once the repository exists, retry replays writes into it. An agent must not do that on its |
| 598 | - // own; a person has to take over the retry (and provide a token for a private source). | |
| 599 | - if (actor.kind === "agent" && row.repo_id) throw new HttpError(403, "an agent cannot retry an import after the repository was created; a person must retry it"); | |
| 600 | - const owner = await this.dir.account(row.target_owner_id); | |
| 604 | + // own; a person who administers the target has to take over the retry (and provide a token for a private source). | |
| 605 | + if (actor.kind === "agent" && row.repo_id) throw new HttpError(403, `an agent cannot retry an import after the repository was created; a person who administers ${owner?.login ?? "the target"} must retry it`); | |
| 601 | 606 | if (!owner || !(await this.canImportInto(actor, owner))) throw new HttpError(403, "you cannot import into this account"); |
| 602 | 607 | const attempt = row.attempt + 1; |
| 603 | 608 | const t = now(); |
| @@ -993,7 +998,7 @@ | ||
| 993 | 998 | p = await ctx.gh.get<GhProtection>(`repos/${src}/branches/${encodeURIComponent(b.name)}/protection`); |
| 994 | 999 | } catch (e) { |
| 995 | 1000 | if (e instanceof GitHubError && (e.status === 403 || e.status === 404)) { |
| 996 | - await this.warn(ctx.row.id, `Branch protection of ${b.name} could not be read (the GitHub token needs admin access to the repository); set it up under Settings → Branches.`); | |
| 1001 | + await this.warn(ctx.row.id, `Branch protection of ${b.name} could not be read (the GitHub token needs admin access to the repository); set it up under Settings → Branch protection.`); | |
| 997 | 1002 | continue; |
| 998 | 1003 | } |
| 999 | 1004 | throw e; |
| @@ -1114,11 +1119,12 @@ | ||
| 1114 | 1119 | const page = await ctx.gh.page<GhRelease>(next); |
| 1115 | 1120 | for (const r of page.items) { |
| 1116 | 1121 | if (!r.tag_name) continue; |
| 1117 | - const { id: releaseId } = await stub.importRelease( | |
| 1122 | + const { id: releaseId, created } = await stub.importRelease( | |
| 1118 | 1123 | { tag_name: r.tag_name, target_commitish: r.target_commitish, name: r.name ?? r.tag_name, body: r.body ?? "", draft: !!r.draft, prerelease: !!r.prerelease, created_at: r.created_at, published_at: r.published_at, author: this.importedActor(ctx, r.author), latest: false }, |
| 1119 | 1124 | caller, |
| 1120 | 1125 | ); |
| 1121 | - totals.imported++; | |
| 1126 | + // A release an earlier try of this import already wrote is updated, not counted again. | |
| 1127 | + if (created !== false) totals.imported++; | |
| 1122 | 1128 | for (const a of r.assets ?? []) { |
| 1123 | 1129 | const name = safeAssetName(a.name); |
| 1124 | 1130 | if (!name) { |
modifiedapps/diffinery/src/mail/api.ts+2−2View file
| @@ -66,7 +66,7 @@ | ||
| 66 | 66 | |
| 67 | 67 | mailApi.put("/repos/:owner/:repo/subscription", async (c) => { |
| 68 | 68 | const me = writer(c); |
| 69 | - const l = await loadRepo(c, c.req.param("owner"), c.req.param("repo"), "read", { whileArchived: true }); | |
| 69 | + const l = await loadRepo(c, c.req.param("owner"), c.req.param("repo"), "read", { whileArchived: true, whileImporting: true }); | |
| 70 | 70 | const b = await json<{ subscribed?: boolean; ignored?: boolean }>(c); |
| 71 | 71 | const level: WatchLevel = b.ignored ? "ignore" : b.subscribed === false ? "participating" : "all"; |
| 72 | 72 | await new MailStore(c.env.DB).setWatch(me.id, l.repo.id, level); |
| @@ -75,7 +75,7 @@ | ||
| 75 | 75 | |
| 76 | 76 | mailApi.delete("/repos/:owner/:repo/subscription", async (c) => { |
| 77 | 77 | const me = writer(c); |
| 78 | - const l = await loadRepo(c, c.req.param("owner"), c.req.param("repo"), "read", { whileArchived: true }); | |
| 78 | + const l = await loadRepo(c, c.req.param("owner"), c.req.param("repo"), "read", { whileArchived: true, whileImporting: true }); | |
| 79 | 79 | await new MailStore(c.env.DB).setWatch(me.id, l.repo.id, "participating"); |
| 80 | 80 | return c.body(null, 204); |
| 81 | 81 | }); |
modifiedapps/diffinery/src/mail/web.tsx+2−2View file
| @@ -344,7 +344,7 @@ | ||
| 344 | 344 | |
| 345 | 345 | post(web, "/:owner/:repo/subscription", async (c, f) => { |
| 346 | 346 | const me = requireUser(c); |
| 347 | - const l = await loadRepo(c, prm(c, "owner"), prm(c, "repo"), "read", { whileArchived: true }); | |
| 347 | + const l = await loadRepo(c, prm(c, "owner"), prm(c, "repo"), "read", { whileArchived: true, whileImporting: true }); | |
| 348 | 348 | const back = f.s("back") === "/settings/notifications" ? "/settings/notifications" : repoUrl(l.repo); |
| 349 | 349 | const level = WATCH_LEVELS.find((x) => x === f.s("level")) as WatchLevel | undefined; |
| 350 | 350 | if (!level) throw new HttpError(422, "pick participating, all or ignore"); |
| @@ -354,7 +354,7 @@ | ||
| 354 | 354 | |
| 355 | 355 | post(web, "/:owner/:repo/issues/:number/subscription", async (c, f) => { |
| 356 | 356 | const me = requireUser(c); |
| 357 | - const l = await loadRepo(c, prm(c, "owner"), prm(c, "repo"), "read", { whileArchived: true }); | |
| 357 | + const l = await loadRepo(c, prm(c, "owner"), prm(c, "repo"), "read", { whileArchived: true, whileImporting: true }); | |
| 358 | 358 | const n = Number(prm(c, "number")); |
| 359 | 359 | if (!Number.isInteger(n) || n < 1) throw new HttpError(404, "not found"); |
| 360 | 360 | const on = f.s("subscribed") === "1"; |
modifiedapps/diffinery/src/mcp/server.ts+1−1View file
| @@ -441,7 +441,7 @@ | ||
| 441 | 441 | ...DOCS_TOOLS, |
| 442 | 442 | start_github_import: { |
| 443 | 443 | description: |
| 444 | - "Start a server-side import of a GitHub repository into Diffinery: git history (keeping issue and pull-request numbers), issues and pull requests with comments and reviews, labels, milestones, branch protection, Actions variables, releases and deployment environments. Secret VALUES are never read from GitHub (only their names are reported, to set by hand), and push protection is never bypassed on an agent's behalf. You can import only into your owner's account or an organization you or they administer. Returns the import id; poll it with get_import.", | |
| 444 | + "Start a server-side import of a GitHub repository into Diffinery: git history (keeping issue and pull-request numbers), issues and pull requests with comments and reviews, labels, milestones, branch protection, Actions variables, releases and deployment environments. Secret VALUES are never read from GitHub (only their names are reported, to set by hand), and push protection is never bypassed on an agent's behalf. An agent can import only where its owner may: the owner's own account or an organization the owner administers (an organization's agent: that organization), and only when the agent's policy allows write. Returns the import id; poll it with get_import.", | |
| 445 | 445 | input: { |
| 446 | 446 | source: { type: "string", description: "GitHub repository as owner/name or a github.com/Enterprise URL" }, |
| 447 | 447 | owner: { type: "string", description: "Diffinery user or organization to import into" }, |
modifiedapps/diffinery/src/repo/RepoDO.ts+2−8View file
| @@ -1337,14 +1337,14 @@ | ||
| 1337 | 1337 | return (this.importsCache ??= new RepoImports(this.sql)); |
| 1338 | 1338 | } |
| 1339 | 1339 | |
| 1340 | - /** The import that currently holds this repository, or null. */ | |
| 1340 | + /** The import that currently holds this repository, or null. Other writes are refused meanwhile (loadRepo, gitReceivePack). */ | |
| 1341 | 1341 | importLockHolder(): string | null { |
| 1342 | 1342 | return this.sql.exec<{ value: string }>("SELECT value FROM meta WHERE key = 'import_lock'").toArray()[0]?.value ?? null; |
| 1343 | 1343 | } |
| 1344 | 1344 | |
| 1345 | 1345 | /** |
| 1346 | 1346 | * Claim the repository for an import. Refuses if another import holds it, or if the repo |
| 1347 | - * already holds any ref, issue, comment, milestone or protection this import did not write | |
| 1347 | + * already holds any ref, issue, comment, milestone, protection or release this import did not write | |
| 1348 | 1348 | * (so a retry never adopts content people added in the meantime — H2). |
| 1349 | 1349 | */ |
| 1350 | 1350 | importBegin(importId: string): void { |
| @@ -1366,12 +1366,6 @@ | ||
| 1366 | 1366 | if (this.importLockHolder() === importId) this.sql.exec("DELETE FROM meta WHERE key = 'import_lock'"); |
| 1367 | 1367 | } |
| 1368 | 1368 | |
| 1369 | - /** Refuse any non-import write while an import holds the repository. */ | |
| 1370 | - private assertWritable(importId?: string): void { | |
| 1371 | - const holder = this.importLockHolder(); | |
| 1372 | - if (holder && holder !== importId) throw new HttpError(423, "repository is locked by an import in progress"); | |
| 1373 | - } | |
| 1374 | - | |
| 1375 | 1369 | importProtection(pattern: string, rules: ProtectionRules | null, importId: string) { |
| 1376 | 1370 | // What agents may do is a person's decision here, never something an import brings in. |
| 1377 | 1371 | if (rules) { |
modifiedapps/diffinery/src/repo/imports.ts+2−2View file
| @@ -248,11 +248,11 @@ | ||
| 248 | 248 | |
| 249 | 249 | /** |
| 250 | 250 | * True when the repository already holds any issue, pull request, comment, review, label, |
| 251 | - * milestone or protection that this import did not write. A retry refuses to reuse a repo | |
| 251 | + * milestone, protection or release that this import did not write. A retry refuses to reuse a repo | |
| 252 | 252 | * that people have since adopted, so an import never clobbers real content. |
| 253 | 253 | */ |
| 254 | 254 | hasForeignContent(importId: string): boolean { |
| 255 | - const tables = ["issues", "pulls", "comments", "reviews", "labels", "milestones", "protections"]; | |
| 255 | + const tables = ["issues", "pulls", "comments", "reviews", "labels", "milestones", "protections", "releases"]; | |
| 256 | 256 | for (const t of tables) { |
| 257 | 257 | const n = this.sql.exec<{ n: number }>(`SELECT count(*) AS n FROM ${t} WHERE imported IS NULL OR imported <> ?`, importId).one().n; |
| 258 | 258 | if (n > 0) return true; |
modifiedapps/diffinery/src/repo/releases.ts+28−9View file
| @@ -570,9 +570,12 @@ | ||
| 570 | 570 | |
| 571 | 571 | /** |
| 572 | 572 | * Insert a release with its original dates and author, without creating |
| 573 | - * tags, firing webhooks or starting workflows. Returns the new release id. | |
| 573 | + * tags, firing webhooks or starting workflows. Returns the release id, and | |
| 574 | + * whether it is new: the row records the import that wrote it, and the same | |
| 575 | + * release (tag and creation date) from the same import is updated in place, so | |
| 576 | + * a retried step or import never duplicates it. | |
| 574 | 577 | */ |
| 575 | - importRelease(input: ImportedRelease, caller: Caller): { id: number } { | |
| 578 | + importRelease(input: ImportedRelease, caller: Caller): { id: number; created: boolean } { | |
| 576 | 579 | this.requireWrite(caller); |
| 577 | 580 | const tag = input.tag_name?.trim(); |
| 578 | 581 | if (!tag) throw new HttpError(422, "tag_name is required"); |
| @@ -581,9 +584,8 @@ | ||
| 581 | 584 | const draft = !!input.draft; |
| 582 | 585 | const published = draft ? null : (input.published_at ?? input.created_at); |
| 583 | 586 | const author = input.author ?? { login: caller.login }; |
| 584 | - this.sql.exec( | |
| 585 | - "INSERT INTO releases (tag_name, target_commitish, name, body, draft, prerelease, author_id, author_login, author_kind, created_at, published_at, updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", | |
| 586 | - tag, | |
| 587 | + const importId = caller.importId ?? null; | |
| 588 | + const values = [ | |
| 587 | 589 | input.target_commitish || this.repo.meta.defaultBranch, |
| 588 | 590 | input.name ?? "", |
| 589 | 591 | input.body ?? "", |
| @@ -592,13 +594,26 @@ | ||
| 592 | 594 | author.id ?? `imported:${author.login}`, |
| 593 | 595 | author.login, |
| 594 | 596 | author.kind ?? "user", |
| 595 | - created.toISOString(), | |
| 596 | 597 | published, |
| 597 | 598 | published ?? created.toISOString(), |
| 598 | - ); | |
| 599 | - const id = this.sql.exec<{ id: number }>("SELECT last_insert_rowid() AS id").one().id; | |
| 599 | + ]; | |
| 600 | + const existing = importId ? this.sql.exec<{ id: number }>("SELECT id FROM releases WHERE imported = ? AND tag_name = ? AND created_at = ?", importId, tag, created.toISOString()).toArray()[0] : undefined; | |
| 601 | + let id: number; | |
| 602 | + if (existing) { | |
| 603 | + id = existing.id; | |
| 604 | + this.sql.exec("UPDATE releases SET target_commitish = ?, name = ?, body = ?, draft = ?, prerelease = ?, author_id = ?, author_login = ?, author_kind = ?, published_at = ?, updated_at = ? WHERE id = ?", ...values, id); | |
| 605 | + } else { | |
| 606 | + this.sql.exec( | |
| 607 | + "INSERT INTO releases (target_commitish, name, body, draft, prerelease, author_id, author_login, author_kind, published_at, updated_at, tag_name, created_at, imported) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", | |
| 608 | + ...values, | |
| 609 | + tag, | |
| 610 | + created.toISOString(), | |
| 611 | + importId, | |
| 612 | + ); | |
| 613 | + id = this.sql.exec<{ id: number }>("SELECT last_insert_rowid() AS id").one().id; | |
| 614 | + } | |
| 600 | 615 | if (input.latest && !draft && !input.prerelease) this.pin(id); |
| 601 | - return { id }; | |
| 616 | + return { id, created: !existing }; | |
| 602 | 617 | } |
| 603 | 618 | |
| 604 | 619 | /** |
| @@ -615,6 +630,10 @@ | ||
| 615 | 630 | */ |
| 616 | 631 | async importAsset(releaseId: number, meta: ImportedAsset, source: { r2Key: string } | { url: string; headers?: Record<string, string> }, caller: Caller): Promise<ReleaseAsset> { |
| 617 | 632 | const uploader = meta.uploader ? { id: meta.uploader.id ?? `imported:${meta.uploader.login}`, login: meta.uploader.login, kind: meta.uploader.kind ?? "user" } : undefined; |
| 633 | + // A retried import finds the assets it already copied: keep them rather than failing on the name. | |
| 634 | + this.requireWrite(caller); | |
| 635 | + const done = this.sql.exec<AssetRow>("SELECT * FROM release_assets WHERE release_id = ? AND name = ? AND state = 'uploaded'", releaseId, assetName(meta.name ?? "")).toArray()[0]; | |
| 636 | + if (done) return toAsset(done); | |
| 618 | 637 | if ("r2Key" in source) { |
| 619 | 638 | const staging = `repos/${this.repo.meta.repoId}/import/`; |
| 620 | 639 | if (typeof source.r2Key !== "string" || !source.r2Key.startsWith(staging) || source.r2Key.includes("..")) throw new HttpError(422, `r2Key must be under ${staging}`); |
modifiedapps/diffinery/src/repo/schema.ts+1−0View file
| @@ -182,6 +182,7 @@ | ||
| 182 | 182 | ["protections", "imported", "TEXT"], |
| 183 | 183 | // Auto-merge while it is on (repo/auto-merge.ts AutoMergeState as JSON); null when off. |
| 184 | 184 | ["pulls", "auto_merge", "TEXT"], |
| 185 | + ["releases", "imported", "TEXT"], | |
| 185 | 186 | ]; |
| 186 | 187 | |
| 187 | 188 | export function migrateRepoSchema(sql: SqlStorage): void { |
modifiedapps/diffinery/src/web/account.tsx+1−1View file
| @@ -276,7 +276,7 @@ | ||
| 276 | 276 | </li> |
| 277 | 277 | <li>Push your code, open pull requests and add workflows under <code>.github/workflows</code>.</li> |
| 278 | 278 | <li> |
| 279 | - Invite people from an organization's <b>People</b> page or a repository's <b>Settings → Access</b>. | |
| 279 | + Invite people from an organization's <b>People</b> page or a repository's <b>Settings → Collaborators</b>. | |
| 280 | 280 | </li> |
| 281 | 281 | </ol> |
| 282 | 282 | </div> |
modifiedapps/diffinery/src/web/imports.tsx+1−1View file
| @@ -24,7 +24,7 @@ | ||
| 24 | 24 | ["labels", "Labels and milestones", "Even without issues."], |
| 25 | 25 | ["protection", "Branch protection", "Mapped to Diffinery's rules; required checks are matched to this repository's workflows."], |
| 26 | 26 | ["variables", "Actions variables and secret names", "Variable values and the names of secrets (never their values)."], |
| 27 | - ["releases", "Releases", "When Diffinery supports releases; until then their tags are imported with the Git data."], | |
| 27 | + ["releases", "Releases", "Release notes and their assets. The tags come with the Git data either way."], | |
| 28 | 28 | ["lfs", "Git LFS objects", "Large files tracked with Git LFS."], |
| 29 | 29 | ]; |
| 30 | 30 |
modifiedapps/diffinery/test/e2e/e2e.test.mjs+29−5View file
| @@ -2271,21 +2271,45 @@ | ||
| 2271 | 2271 | assert.equal((await api("POST", "/diffinery/imports", { source: "octo/app", owner: "nobody-here", github_api_url: apiUrl })).status, 422); |
| 2272 | 2272 | assert.equal((await api("POST", "/diffinery/imports", { source: "octo/app", owner, github_token: "wrong", github_api_url: apiUrl })).status, 422); |
| 2273 | 2273 | const bot = (await ok("POST", "/diffinery/agents", { login: "import-bot" }, null)).token.token; |
| 2274 | - const botTry = await api("POST", "/diffinery/imports", { source: "octo/app", owner, github_token: GH_TOKEN, github_api_url: apiUrl }, bot); | |
| 2275 | - assert.equal(botTry.status, 403, "agents cannot import into a person's account"); | |
| 2274 | + // An agent may import where its owner may, its owner's own account included: this one gets past | |
| 2275 | + // the permission check and stops only because the GitHub repository does not exist. | |
| 2276 | + const botTry = await api("POST", "/diffinery/imports", { source: "octo/missing", owner, github_token: GH_TOKEN, github_api_url: apiUrl }, bot); | |
| 2277 | + assert.equal(botTry.status, 422, JSON.stringify(botTry.json)); | |
| 2278 | + assert.match(botTry.json.message, /octo\/missing was not found/); | |
| 2276 | 2279 | |
| 2277 | 2280 | // ------------------------------------------------------------ one repository |
| 2278 | 2281 | const started = await ok("POST", "/diffinery/imports", { source: "octo/app", owner, name: "imported-app", github_token: GH_TOKEN, github_api_url: apiUrl }); |
| 2279 | 2282 | assert.equal(started.status, "queued"); |
| 2280 | 2283 | assert.ok(!JSON.stringify(started).includes(GH_TOKEN), "the token is never returned"); |
| 2281 | - // While the Git transfer waits, a webhook is added: nothing the import does may reach it. | |
| 2282 | - await until("the repository to be created", async () => (await api("GET", `/repos/${owner}/imported-app`)).status === 200, 120_000); | |
| 2283 | - const hook = await ok("POST", `/repos/${owner}/imported-app/hooks`, { config: { url: "https://hooks.invalid/import-sink" }, events: ["*"] }); | |
| 2284 | + // While the import holds the repository it created, nothing else writes to it: the REST API, the | |
| 2285 | + // web UI and MCP get 423 Locked (reads still work). The import's own writes go through below. | |
| 2286 | + await until(`import ${started.id} to wait for an agent`, async () => (await ok("GET", `/diffinery/imports/${started.id}`)).git?.phase === "waiting", 120_000); | |
| 2287 | + const L = `/repos/${owner}/imported-app`; | |
| 2288 | + for (const [method, path, body] of [ | |
| 2289 | + ["POST", `${L}/hooks`, { config: { url: "https://hooks.invalid/import-sink" }, events: ["*"] }], | |
| 2290 | + ["POST", `${L}/issues`, { title: "too early" }], | |
| 2291 | + ["PUT", `${L}/contents/early.txt`, { message: "too early", content: Buffer.from("early\n").toString("base64") }], | |
| 2292 | + ["POST", `${L}/labels`, { name: "early", color: "ffffff" }], | |
| 2293 | + ["PATCH", L, { description: "changed during the import" }], | |
| 2294 | + ]) { | |
| 2295 | + const r = await api(method, path, body); | |
| 2296 | + assert.equal(r.status, 423, `${method} ${path} -> ${r.status} ${JSON.stringify(r.json)}`); | |
| 2297 | + assert.match(r.json.message, /locked by an import/); | |
| 2298 | + } | |
| 2299 | + assert.equal((await api("GET", `${L}/issues`)).status, 200, "reads are not locked"); | |
| 2300 | + const csrf = randomBytes(16).toString("hex"); | |
| 2301 | + 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" }); | |
| 2302 | + assert.equal(webWrite.status, 423, await webWrite.text()); | |
| 2303 | + const mcpWrite = await fetch(`${base}/mcp`, { method: "POST", headers: { authorization: `Bearer ${token}`, "content-type": "application/json", accept: "application/json, text/event-stream" }, body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "tools/call", params: { name: "create_issue", arguments: { owner, repo: "imported-app", title: "too early" } } }) }).then((r) => r.json()); | |
| 2304 | + assert.equal(mcpWrite.result.isError, true, JSON.stringify(mcpWrite)); | |
| 2305 | + assert.match(mcpWrite.result.content[0].text, /locked by an import/); | |
| 2284 | 2306 | const agent = await transfer(started.id); |
| 2285 | 2307 | assert.equal(agent.status, 0, agent.out); |
| 2286 | 2308 | const done = await finished(started.id); |
| 2287 | 2309 | assert.equal(done.status, "succeeded", JSON.stringify(done, null, 2)); |
| 2288 | 2310 | // No side effects: no workflow runs (ci.yml runs on push and pull_request), no webhook deliveries. |
| 2311 | + // The lock is released: a webhook can be added now, and nothing the import queued reaches it. | |
| 2312 | + const hook = await ok("POST", `/repos/${owner}/imported-app/hooks`, { config: { url: "https://hooks.invalid/import-sink" }, events: ["*"] }); | |
| 2289 | 2313 | await sleep(2000); |
| 2290 | 2314 | assert.equal((await ok("GET", `/repos/${owner}/imported-app/actions/runs`)).total_count, 0); |
| 2291 | 2315 | const sink = (await ok("GET", `/repos/${owner}/imported-app/hooks`)).find((h) => h.id === hook.id); |
modifiedapps/diffinery/test/unit/imports.test.ts+165−0View file
| @@ -1,9 +1,16 @@ | ||
| 1 | +import { readFileSync } from "node:fs"; | |
| 1 | 2 | import { DatabaseSync } from "node:sqlite"; |
| 2 | 3 | import { describe, expect, it } from "vitest"; |
| 3 | 4 | import { checkPublicUrl, isPrivateAddress, normalizeApiUrl, parseImportRequest, parseOptions, parseRepoSource, safeAssetName, visibilityFor } from "../../src/imports/options.js"; |
| 4 | 5 | import { People, mapIssue, mapPull, mapReview, type GhIssue, type GhPull, type Link } from "../../src/imports/mapping.js"; |
| 5 | 6 | import { RepoImports } from "../../src/repo/imports.js"; |
| 6 | 7 | import { REPO_SCHEMA, migrateRepoSchema } from "../../src/repo/schema.js"; |
| 8 | +import { Releases, type ImportedRelease } from "../../src/repo/releases.js"; | |
| 9 | +import type { RepoDO } from "../../src/repo/RepoDO.js"; | |
| 10 | +import type { Caller } from "../../src/repo/types.js"; | |
| 11 | +import type { Env } from "../../src/env.js"; | |
| 12 | +import type { Account } from "../../src/lib/directory.js"; | |
| 13 | +import { ImportService } from "../../src/imports/service.js"; | |
| 7 | 14 | |
| 8 | 15 | // ---------------------------------------------------------------- request and option validation |
| 9 | 16 | |
| @@ -220,3 +227,161 @@ | ||
| 220 | 227 | expect(imp.pullNumbers(0, 10).map((p) => p.number)).toEqual([2]); |
| 221 | 228 | }); |
| 222 | 229 | }); |
| 230 | + | |
| 231 | +// ---------------------------------------------------------------- releases: idempotent import, provenance | |
| 232 | + | |
| 233 | +/** Releases over the node:sqlite storage, with just the RepoDO fields importRelease reads. */ | |
| 234 | +function releases() { | |
| 235 | + const sql = sqlStorage(); | |
| 236 | + const repo = { sql, meta: { repoId: "r1", defaultBranch: "main" } } as unknown as RepoDO; | |
| 237 | + return { sql, rel: new Releases(repo, {} as Env), imp: new RepoImports(sql) }; | |
| 238 | +} | |
| 239 | +const importer = (importId: string): Caller => ({ id: "u1", login: "olive", kind: "user", role: "admin", importId }); | |
| 240 | +const ghRelease = (over: Partial<ImportedRelease> = {}): ImportedRelease => ({ tag_name: "v1.0.0", name: "1.0.0", body: "first", created_at: "2024-01-01T00:00:00Z", published_at: "2024-01-01T00:00:00Z", author: { login: "github:octocat" }, ...over }); | |
| 241 | + | |
| 242 | +describe("release import", () => { | |
| 243 | + it("is idempotent: importing the same release again updates the one row", () => { | |
| 244 | + const { sql, rel } = releases(); | |
| 245 | + const first = rel.importRelease(ghRelease(), importer("imp-1")); | |
| 246 | + const again = rel.importRelease(ghRelease({ body: "edited" }), importer("imp-1")); | |
| 247 | + expect(again.id).toBe(first.id); | |
| 248 | + expect(sql.exec<{ id: number; body: string }>("SELECT id, body FROM releases").toArray()).toEqual([{ id: first.id, body: "edited" }]); | |
| 249 | + // Another tag is another release. | |
| 250 | + expect(rel.importRelease(ghRelease({ tag_name: "v2.0.0" }), importer("imp-1")).id).not.toBe(first.id); | |
| 251 | + }); | |
| 252 | + | |
| 253 | + it("counts a release the import did not write as foreign content", () => { | |
| 254 | + const { rel, imp, sql } = releases(); | |
| 255 | + rel.importRelease(ghRelease(), importer("imp-1")); | |
| 256 | + expect(imp.hasForeignContent("imp-1")).toBe(false); | |
| 257 | + expect(imp.hasForeignContent("imp-2")).toBe(true); | |
| 258 | + sql.exec("DELETE FROM releases"); | |
| 259 | + // A release people created by hand (no import provenance). | |
| 260 | + sql.exec("INSERT INTO releases (tag_name, target_commitish, author_id, author_login, created_at, updated_at) VALUES ('v9', 'main', 'u1', 'olive', '2024-01-01', '2024-01-01')"); | |
| 261 | + expect(imp.hasForeignContent("imp-1")).toBe(true); | |
| 262 | + }); | |
| 263 | +}); | |
| 264 | + | |
| 265 | +// ---------------------------------------------------------------- who may import, retry, and the size limit | |
| 266 | + | |
| 267 | +type Acc = Partial<Account> & { id: string; login: string; kind: Account["kind"] }; | |
| 268 | +const acc = (a: Acc): Account => ({ status: "active", site_admin: 0, owner_id: null, agent_policy: null, ...a }) as unknown as Account; | |
| 269 | +const people = { | |
| 270 | + olive: acc({ id: "u-olive", login: "olive", kind: "user" }), | |
| 271 | + rita: acc({ id: "u-rita", login: "rita", kind: "user" }), | |
| 272 | + milo: acc({ id: "u-milo", login: "milo", kind: "user" }), | |
| 273 | + gone: acc({ id: "u-gone", login: "gone", kind: "user", status: "disabled" }), | |
| 274 | + guild: acc({ id: "o-guild", login: "guild", kind: "org" }), | |
| 275 | + shop: acc({ id: "o-shop", login: "shop", kind: "org" }), | |
| 276 | + bot: acc({ id: "a-bot", login: "olive-bot", kind: "agent", owner_id: "u-olive" }), | |
| 277 | + reader: acc({ id: "a-reader", login: "olive-reader", kind: "agent", owner_id: "u-olive", agent_policy: JSON.stringify({ maxRole: "read" }) }), | |
| 278 | + orphan: acc({ id: "a-orphan", login: "gone-bot", kind: "agent", owner_id: "u-gone" }), | |
| 279 | + guildBot: acc({ id: "a-guild", login: "guild-bot", kind: "agent", owner_id: "o-guild" }), | |
| 280 | +}; | |
| 281 | +// olive owns guild and is a plain member of shop; rita administers guild; milo is a guild member. | |
| 282 | +const roles: Record<string, string> = { "o-guild/u-olive": "owner", "o-guild/u-rita": "admin", "o-guild/u-milo": "member", "o-shop/u-olive": "member" }; | |
| 283 | + | |
| 284 | +/** An ImportService over fake D1, workflow and directory lookups (enough for the permission checks). */ | |
| 285 | +function service(env: Record<string, unknown> = {}) { | |
| 286 | + const db = { prepare: () => ({ bind: () => ({ run: async () => ({}), first: async () => ({ n: 0 }) }) }) }; | |
| 287 | + const svc = new ImportService({ DB: db, IMPORT_WORKFLOW: { create: async () => ({}) }, ...env } as unknown as Env); | |
| 288 | + const byId = new Map(Object.values(people).map((a) => [a.id, a])); | |
| 289 | + Object.assign(svc.dir, { | |
| 290 | + account: async (id: string) => byId.get(id) ?? null, | |
| 291 | + accountByLogin: async (login: string) => Object.values(people).find((a) => a.login === login) ?? null, | |
| 292 | + orgRole: async (org: string, user: string) => roles[`${org}/${user}`] ?? null, | |
| 293 | + audit: async () => undefined, | |
| 294 | + }); | |
| 295 | + return svc; | |
| 296 | +} | |
| 297 | + | |
| 298 | +describe("where an agent may import", () => { | |
| 299 | + it("imports only where its owner may: the owner's account or an organization the owner administers", async () => { | |
| 300 | + const svc = service(); | |
| 301 | + const may = (a: Account, o: Account) => svc.canImportInto(a, o); | |
| 302 | + expect(await may(people.bot, people.olive)).toBe(true); | |
| 303 | + expect(await may(people.bot, people.guild)).toBe(true); | |
| 304 | + expect(await may(people.bot, people.rita)).toBe(false); | |
| 305 | + expect(await may(people.bot, people.shop)).toBe(false); | |
| 306 | + expect(await may(people.orphan, people.gone)).toBe(false); | |
| 307 | + expect(await may(people.guildBot, people.guild)).toBe(true); | |
| 308 | + expect(await may(people.guildBot, people.shop)).toBe(false); | |
| 309 | + // The same rule as for the owner themselves. | |
| 310 | + expect(await may(people.olive, people.olive)).toBe(true); | |
| 311 | + expect(await may(people.olive, people.shop)).toBe(false); | |
| 312 | + }); | |
| 313 | + | |
| 314 | + it("needs an agent whose policy allows write, even where its owner may import", async () => { | |
| 315 | + const svc = service(); | |
| 316 | + await expect(svc.create(people.reader, ["write"], { source: "octo/app", owner: "olive" })).rejects.toMatchObject({ status: 403, detail: expect.stringMatching(/write/) }); | |
| 317 | + await expect(svc.create(people.bot, ["write"], { source: "octo/app", owner: "rita" })).rejects.toMatchObject({ status: 403, detail: expect.stringMatching(/owner/) }); | |
| 318 | + }); | |
| 319 | +}); | |
| 320 | + | |
| 321 | +describe("retrying an agent's import", () => { | |
| 322 | + const failed = (over: Record<string, unknown> = {}) => ({ id: "imp-1", parent_id: null, kind: "repo", status: "failed", created_by: people.bot.id, target_owner_id: people.guild.id, repo_id: "r1", attempt: 1, token_ciphertext: null, source: "octo/app", ...over }); | |
| 323 | + const withRow = (row: ReturnType<typeof failed>) => { | |
| 324 | + const svc = service(); | |
| 325 | + (svc as unknown as { row: () => Promise<unknown> }).row = async () => row; | |
| 326 | + return svc; | |
| 327 | + }; | |
| 328 | + | |
| 329 | + it("lets a person who administers the target retry it, and tells the agent so", async () => { | |
| 330 | + await expect(withRow(failed()).retry("imp-1", people.bot, null)).rejects.toMatchObject({ status: 403, detail: expect.stringMatching(/a person who administers guild/) }); | |
| 331 | + await expect(withRow(failed()).retry("imp-1", people.rita, null)).resolves.toBeTruthy(); | |
| 332 | + await expect(withRow(failed()).retry("imp-1", people.olive, null)).resolves.toBeTruthy(); | |
| 333 | + }); | |
| 334 | + | |
| 335 | + it("still refuses everyone else", async () => { | |
| 336 | + // A guild member who does not administer it. | |
| 337 | + await expect(withRow(failed()).retry("imp-1", people.milo, null)).rejects.toMatchObject({ status: 404 }); | |
| 338 | + // A person's import stays theirs to retry, even for an admin of the target. | |
| 339 | + await expect(withRow(failed({ created_by: people.olive.id })).retry("imp-1", people.rita, null)).rejects.toMatchObject({ status: 404 }); | |
| 340 | + }); | |
| 341 | +}); | |
| 342 | + | |
| 343 | +describe("IMPORT_MAX_REPO_KB", () => { | |
| 344 | + const check = (value: string | undefined, sizeKb: number) => (service({ IMPORT_MAX_REPO_KB: value }) as unknown as { assertRepoSize(kb: number): void }).assertRepoSize(sizeKb); | |
| 345 | + | |
| 346 | + it("treats a non-positive or invalid value as the default (10 GB)", () => { | |
| 347 | + for (const v of ["-1", "0", "-0.5", "abc", "", undefined]) { | |
| 348 | + expect(() => check(v, 1024), String(v)).not.toThrow(); | |
| 349 | + expect(() => check(v, 11 * 1024 * 1024), String(v)).toThrow(/10 GB import limit/); | |
| 350 | + } | |
| 351 | + }); | |
| 352 | + | |
| 353 | + it("uses a positive value", () => { | |
| 354 | + expect(() => check("2048", 2048)).not.toThrow(); | |
| 355 | + expect(() => check("2048", 2049)).toThrow(/import limit/); | |
| 356 | + }); | |
| 357 | +}); | |
| 358 | + | |
| 359 | +// ---------------------------------------------------------------- texts that point at the web UI | |
| 360 | + | |
| 361 | +describe("import texts name what the web UI shows", () => { | |
| 362 | + const source = (p: string) => readFileSync(new URL(p, import.meta.url), "utf8"); | |
| 363 | + /** Text nodes of a TSX file (`>Label<`, also after an expression like `{icon} Label`). */ | |
| 364 | + const texts = (...files: string[]) => new Set(files.flatMap((f) => [...source(`../../src/web/${f}`).matchAll(/[>}]\s*([^<>{}\n]+?)\s*[<{]/g)].map((m) => m[1]!.trim()))); | |
| 365 | + const repoTabs = [...source("../../src/web/repo-settings.tsx").matchAll(/\["\w+", "([^"]+)", `\$\{base\}/g)].map((m) => m[1]); | |
| 366 | + | |
| 367 | + it("points at repository settings tabs that exist", () => { | |
| 368 | + expect(repoTabs).toContain("Branch protection"); | |
| 369 | + for (const f of ["../../src/imports/service.ts", "../../src/web/imports.tsx"]) { | |
| 370 | + for (const m of source(f).matchAll(/Settings → ([A-Z][A-Za-z ]*[a-z])/g)) expect(repoTabs, `${f}: ${m[0]}`).toContain(m[1]); | |
| 371 | + } | |
| 372 | + }); | |
| 373 | + | |
| 374 | + it("README's import section uses the web UI's labels", () => { | |
| 375 | + const readme = source("../../../../README.md"); | |
| 376 | + const section = readme.slice(readme.indexOf("## Moving from GitHub"), readme.indexOf("## Developing")); | |
| 377 | + const ui = texts("imports.tsx", "account.tsx", "ui.tsx"); | |
| 378 | + const labels = [...section.matchAll(/(?<!\*)\*([A-Z][^*\n]+)\*(?!\*)/g)].map((m) => m[1]!); | |
| 379 | + expect(labels.length).toBeGreaterThan(3); | |
| 380 | + for (const label of labels) for (const part of label.split(" → ")) expect(ui.has(part), `README: *${label}* (${part})`).toBe(true); | |
| 381 | + }); | |
| 382 | + | |
| 383 | + it("does not say releases are unsupported", () => { | |
| 384 | + expect(source("../../src/web/imports.tsx")).not.toMatch(/When Diffinery supports releases/); | |
| 385 | + expect(source("../../../../README.md")).not.toMatch(/where the server's releases feature is enabled/); | |
| 386 | + }); | |
| 387 | +}); |
modifiedscripts/import-campaign.sh+3−0View file
| @@ -101,6 +101,9 @@ | ||
| 101 | 101 | { |
| 102 | 102 | echo "# Import campaign" |
| 103 | 103 | echo |
| 104 | + if [ -n "${GITHUB_TOKEN_FOR_IMPORT:-}" ]; then echo "Imported with a GitHub token (IMPORT_GITHUB_TOKEN)."; else echo "Imported without a GitHub token (GitHub's anonymous rate limit)."; fi | |
| 105 | + echo "Workflows run: ${RUN_WORKFLOWS:-false}." | |
| 106 | + echo | |
| 104 | 107 | echo "| Repository | Import | Took | Issues, PRs and comments | Warnings | Workflows | Runs |" |
| 105 | 108 | echo "|---|---|---|---|---|---|---|" |
| 106 | 109 | } > "$REPORT" |