From ac93d6461cc2325dfabeaf8758976d8c2595c23f Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Tue, 18 Aug 2026 19:51:47 -0400 Subject: [PATCH] feat(uploads): local-gh fallback parity for link adoption (#708) --- .changeset/gh-fallback-link-adoption.md | 5 + packages/uploads/src/commands.ts | 67 +++++- packages/uploads/src/github-gh.ts | 68 ++++++ .../uploads/test/commands-comment.test.ts | 210 ++++++++++++++++++ 4 files changed, 349 insertions(+), 1 deletion(-) create mode 100644 .changeset/gh-fallback-link-adoption.md diff --git a/.changeset/gh-fallback-link-adoption.md b/.changeset/gh-fallback-link-adoption.md new file mode 100644 index 00000000..44b238d3 --- /dev/null +++ b/.changeset/gh-fallback-link-adoption.md @@ -0,0 +1,5 @@ +--- +"@buildinternet/uploads": minor +--- + +Link adoption (`adoptLinkedFiles`, issue #701) now also runs on the local-`gh` fallback comment path, not just the bot-managed one. `uploads comment --pr`/`--issue` (and the attach-time comment sync) scans the PR/issue body and comments for pasted uploads.sh URLs via local `gh` and adopts any that resolve to a file in the current workspace, before rendering the comment. Same semantics as the bot path: copy never move, additive metadata, idempotent re-adoption, no migration into private prefixes, and a lone adopted image with nothing else to consolidate doesn't trigger a fresh comment on its own (it still heals an existing one). diff --git a/packages/uploads/src/commands.ts b/packages/uploads/src/commands.ts index ea27e569..99f5a8f2 100644 --- a/packages/uploads/src/commands.ts +++ b/packages/uploads/src/commands.ts @@ -69,6 +69,9 @@ import { timedExecRunner, ghMetadataFromTargetWithTitle, upsertAttachmentsComment, + hasLinkCandidate, + extractCandidateUrls, + fetchAdoptionCandidateText, type CommandRunner, } from "./github-gh.js"; import { deriveRepoFromGit, deriveRepoSlugFromGit } from "./keys.js"; @@ -833,6 +836,58 @@ export async function syncAttachmentsComment( } } + // Link adoption (issue #708): local-gh fallback parity with the bot's own + // adoption (issue #701, apps/api/src/github-link-adopt.ts). When the bot + // already handled this target the server already adopted for us, so this + // only runs once we've fallen through to gh. Scans the PR/issue body and + // every comment for pasted uploads.sh URLs and adopts each one that + // resolves (server-side, inside `POST .../github/attach`) to a file in + // THIS workspace's own bound-repo attachment prefix — copy, never move, + // same as the bot path. Best-effort end to end: any failure (not a git + // repo, `gh` unavailable/unauthenticated, config unreadable) degrades to + // "adopt nothing" rather than blocking the comment sync it rides along + // with. + let adoptedCount = 0; + let preAdoptionAttachmentCount: number | undefined; + try { + const root = run("git", ["rev-parse", "--show-toplevel"]).trim(); + const { config: adoptConfig } = readLocalRepoCommentConfig(root); + const { options: adoptOptions } = resolveCommentOptions(adoptConfig, null); + if (adoptOptions.adoptLinkedFiles) { + const text = fetchAdoptionCandidateText(target, run); + if (hasLinkCandidate(text)) { + const urls = extractCandidateUrls(text); + if (urls.length > 0) { + // Baseline BEFORE this pass's adoptions land (mirrors the bot + // path's `gatherCommentBody` call before its own copies) — feeds + // the noise guard below without a lone adoption inflating its own + // count. Plain prefix only (not the private-prefix listing done + // for the final render below) — good enough for a guard decision. + preAdoptionAttachmentCount = (await client.listAll({ prefix: ghKeyPrefix(target) })) + .length; + for (const url of urls) { + try { + await client.attachExisting({ + source: url, + repo: target.repo, + ...(target.kind === "pull" ? { pr: target.num } : { issue: target.num }), + }); + adoptedCount++; + } catch { + // Not a resolvable uploads.sh URL, belongs to a different + // workspace, or the source was deleted — silently dropped, + // matching the bot path's contract (a throw from + // `resolveAttachSourceKey` is caught per-URL there too). + } + } + } + } + } + } catch { + // Not a git repo, `.uploads.yml` unreadable, or `gh` unavailable for the + // PR/comments fetch — degrade to no adoption this pass. + } + // gh fallback: gather from this workspace's own data and post via local `gh`. // Note (issues #304, #365): this CLI process has no server-side // WorkspaceRecord in scope, so it cannot honor a workspace's @@ -939,11 +994,21 @@ export async function syncAttachmentsComment( // bot identity, so this note would be wrong there. const body = `${attachmentsCommentBody(items, previewGalleries, marker, renderOptions, target)}\n${GH_FALLBACK_AUTHOR_NOTE}`; const count = items.length + previewGalleries.length; + // Noise guard (issue #708, mirrors the bot's `shouldSyncAfterAdopt`): a + // lone adopted link with nothing else already attached is already fully + // visible inline in the PR/comment — don't create a brand-new comment just + // to repeat it. `upsertAttachmentsComment` still PATCHes an existing + // managed comment unconditionally (its own `if (existing)` branch runs + // regardless of `createIfMissing`), so "a managed comment already exists" + // and "other attachments are already present" both heal/sync for free + // here without any extra condition — this only ever suppresses a fresh + // create. + const skipLoneAdoptionCreate = adoptedCount === 1 && (preAdoptionAttachmentCount ?? 0) === 0; // Empty (count 0) renders the neutral empty-state body but must not create a // comment — it only rewrites one that already exists (`action: "skipped"` // when none does). const { action } = upsertAttachmentsComment(target, body, run, marker, { - createIfMissing: count > 0, + createIfMissing: count > 0 && !skipLoneAdoptionCreate, }); return { action, count, via: "gh" }; } diff --git a/packages/uploads/src/github-gh.ts b/packages/uploads/src/github-gh.ts index 34a5cf1b..eb749b1f 100644 --- a/packages/uploads/src/github-gh.ts +++ b/packages/uploads/src/github-gh.ts @@ -402,6 +402,74 @@ function reconcileAfterCreate( } } +// --- link adoption (issue #708, local-gh fallback parity with the bot's +// #701/apps/api/src/github-link-adopt.ts) --- + +/** Cheap, regex-only reject: no http(s) URL at all means nothing to scan for. + * Mirrors `hasLinkCandidate` in apps/api/src/github-link-adopt.ts. */ +export function hasLinkCandidate(text: string): boolean { + return /https?:\/\//i.test(text); +} + +const URL_RE = /https?:\/\/[^\s)"'<>\]]+/gi; + +/** + * Distinct http(s) URLs found in `text`, trailing prose punctuation + * stripped, order preserved, first occurrence wins on duplicates. Ported + * verbatim from apps/api/src/github-link-adopt.ts's `extractCandidateUrls` + * so the two paths recognize the same URL spellings — resolution itself + * (storage host / embed host / `/f/` page → key, and the bound-workspace + * check) happens server-side inside `POST .../github/attach`, so this file + * doesn't need its own copy of that logic. + */ +export function extractCandidateUrls(text: string): string[] { + const seen = new Set(); + const out: string[] = []; + for (const m of text.matchAll(URL_RE)) { + const url = m[0].replace(/[.,;:]+$/, ""); + if (seen.has(url)) continue; + seen.add(url); + out.push(url); + } + return out; +} + +/** + * Best-effort concatenation of a PR/issue's current body plus every comment's + * current body, for link-adoption scanning. Returns "" on any `gh` failure + * (not authenticated, network, repo/number not found) — adoption degrades to + * a no-op rather than blocking the comment sync it rides along with. + * + * Unlike the webhook path (which re-scans one specific body/comment ref per + * event), the CLI has no per-event ref to key off of — `uploads comment` is + * one ad-hoc invocation — so this scans the PR/issue body and every comment + * on the thread in one pass every time it runs. That's a documented + * divergence: harmless (adoption is idempotent) but does mean a link posted + * in comment #1 gets rescanned on every later `uploads comment` run too. + */ +export function fetchAdoptionCandidateText(target: GhTarget, run: CommandRunner): string { + let body = ""; + try { + body = run("gh", ["api", `repos/${target.repo}/issues/${target.num}`, "--jq", '.body // ""']); + } catch { + // Not found / no access — fall through with an empty body; comments may + // still be readable. + } + let comments = ""; + try { + comments = run("gh", [ + "api", + `repos/${target.repo}/issues/${target.num}/comments?per_page=100`, + "--paginate", + "--jq", + '[.[].body] | join("\\n")', + ]); + } catch { + // Same degrade — an empty comments blob just means nothing more to scan. + } + return `${body}\n${comments}`; +} + /** PATCH one comment's body via stdin, so the body is never shell-interpolated. */ function patchComment(target: GhTarget, run: CommandRunner, id: number, body: string): void { run( diff --git a/packages/uploads/test/commands-comment.test.ts b/packages/uploads/test/commands-comment.test.ts index d55ff632..e2a9e8cc 100644 --- a/packages/uploads/test/commands-comment.test.ts +++ b/packages/uploads/test/commands-comment.test.ts @@ -468,6 +468,216 @@ describe("syncAttachmentsComment", () => { expect(calls.some((c) => c.args.includes("repos/acme/web/issues/12/comments"))).toBe(false); }); + describe("link adoption (issue #708, local-gh fallback parity)", () => { + /** + * gh+git runner for adoption tests: `git rev-parse --show-toplevel` + * resolves to a scratch dir (no `.uploads.yml`, so the knob's default + * "on" applies), the issue/PR body call returns `prBody`, the comments + * call returns `[]` (no comments to scan), the marker hunt finds no + * existing comment, and any create/patch just records its body. + */ + function adoptionRunner(prBody: string): { run: CommandRunner; calls: string[][] } { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "uploads-commands-comment-adopt-")); + const calls: string[][] = []; + const run: CommandRunner = (cmd, args) => { + calls.push([cmd, ...args]); + if (cmd === "git" && args.includes("--show-toplevel")) return `${dir}\n`; + if (cmd !== "gh") throw new Error(`unexpected command: ${cmd}`); + if (args[0] === "api" && args[1]?.startsWith("repos/") && args[1]?.includes("/comments?")) { + return "[]"; + } + if (args[0] === "api" && args[1]?.match(/^repos\/[^/]+\/[^/]+\/issues\/\d+$/)) { + return prBody; + } + if (args[1]?.includes("per_page=100")) return "[]"; // marker hunt: no existing comment + return JSON.stringify({ id: 9 }); // create + }; + return { run, calls }; + } + + it("adopts a pasted uploads.sh URL and syncs when other attachments already exist", async () => { + const { run } = adoptionRunner("see https://uploads.sh/f/acme/other-workspace-image.png"); + const attachExisting = vi.fn(async () => ({ + key: "gh/acme/web/pull/12/other-workspace-image.png", + url: "u", + embedUrl: null, + moved: false, + source: { key: "f/acme/other-workspace-image.png" }, + comment: { posted: false, reason: "not_installed" }, + })); + const client = fakeClient({ + attachExisting, + listAll: async () => [{ key: "gh/acme/web/pull/12/a.png", url: "u", embedUrl: null }], + findGalleriesByReference: async () => ({ galleries: [], nextCursor: null }), + } as never); + const res = await syncAttachmentsComment( + client, + { repo: "acme/web", num: 12, kind: "pull" }, + run, + ); + expect(attachExisting).toHaveBeenCalledWith({ + source: "https://uploads.sh/f/acme/other-workspace-image.png", + repo: "acme/web", + pr: 12, + }); + expect(res.via).toBe("gh"); + expect(res.action).toBe("created"); + }); + + it("does not create a comment for a lone adoption with nothing else attached (noise guard)", async () => { + const { run } = adoptionRunner("see https://uploads.sh/f/acme/solo.png"); + const attachExisting = vi.fn(async () => ({ + key: "gh/acme/web/pull/12/solo.png", + url: "u", + embedUrl: null, + moved: false, + source: { key: "f/acme/solo.png" }, + comment: { posted: false, reason: "not_installed" }, + })); + // First listAll() (baseline, before adoption) reports empty; the second + // listAll() (post-adoption gather for rendering) reports the newly + // adopted file — mirroring how a real workspace would look either side + // of the copy this pass makes. + let listCalls = 0; + const client = fakeClient({ + attachExisting, + listAll: async () => { + listCalls++; + return listCalls === 1 + ? [] + : [{ key: "gh/acme/web/pull/12/solo.png", url: "u", embedUrl: null }]; + }, + findGalleriesByReference: async () => ({ galleries: [], nextCursor: null }), + } as never); + const res = await syncAttachmentsComment( + client, + { repo: "acme/web", num: 12, kind: "pull" }, + run, + ); + expect(attachExisting).toHaveBeenCalledTimes(1); + // Copy still happened, but no NEW comment was created for it. + expect(res.action).toBe("skipped"); + }); + + it("heals an existing managed comment even for a lone adoption", async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "uploads-commands-comment-adopt-")); + const calls: string[][] = []; + const run: CommandRunner = (cmd, args) => { + calls.push([cmd, ...args]); + if (cmd === "git" && args.includes("--show-toplevel")) return `${dir}\n`; + if (cmd !== "gh") throw new Error(`unexpected command: ${cmd}`); + // The adoption comments-scan and the marker hunt share the same URL + // shape (`.../comments?per_page=100`) — the adoption call is the one + // carrying `--jq`, so check that first. + if (args[0] === "api" && args[1]?.includes("/comments?") && args.includes("--jq")) { + return "[]"; // no comments to scan for links + } + if (args[0] === "api" && args[1]?.match(/^repos\/[^/]+\/[^/]+\/issues\/\d+$/)) { + return "see https://uploads.sh/f/acme/solo.png"; + } + if (args[1]?.includes("per_page=100")) { + // The marker hunt (no --jq) finds an existing comment. + return JSON.stringify([{ id: 7, body: `${ATTACHMENTS_MARKER}\nold` }]); + } + return JSON.stringify({ id: 7 }); + }; + const attachExisting = vi.fn(async () => ({ + key: "gh/acme/web/pull/12/solo.png", + url: "u", + embedUrl: null, + moved: false, + source: { key: "f/acme/solo.png" }, + comment: { posted: false, reason: "not_installed" }, + })); + // Baseline (before adoption) is empty — the noise guard alone would + // suppress a fresh create — but a managed comment already exists, so + // it must still heal via PATCH regardless of `createIfMissing`. + let listCalls = 0; + const client = fakeClient({ + attachExisting, + listAll: async () => { + listCalls++; + return listCalls === 1 + ? [] + : [{ key: "gh/acme/web/pull/12/solo.png", url: "u", embedUrl: null }]; + }, + findGalleriesByReference: async () => ({ galleries: [], nextCursor: null }), + } as never); + const res = await syncAttachmentsComment( + client, + { repo: "acme/web", num: 12, kind: "pull" }, + run, + ); + expect(attachExisting).toHaveBeenCalledTimes(1); + expect(res.action).toBe("updated"); + expect(calls.some((c) => c[0] === "gh" && c.includes("PATCH"))).toBe(true); + }); + + it("skips an unresolvable candidate URL without failing the sync", async () => { + const { run } = adoptionRunner("see https://example.com/not-uploads.png"); + const attachExisting = vi.fn(async () => { + throw new Error("source_not_found"); + }); + const client = fakeClient({ + attachExisting, + listAll: async () => [], + findGalleriesByReference: async () => ({ galleries: [], nextCursor: null }), + } as never); + const res = await syncAttachmentsComment( + client, + { repo: "acme/web", num: 12, kind: "pull" }, + run, + ); + // A non-uploads.sh URL is still a "candidate" by the cheap regex, so + // attachExisting is called and this asserts the per-URL catch: the + // failure is swallowed, not thrown. + expect(attachExisting).toHaveBeenCalled(); + expect(res.action).toBe("skipped"); + }); + + it("honors adoptLinkedFiles: false in .uploads.yml", async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "uploads-commands-comment-adopt-off-")); + fs.writeFileSync(path.join(dir, ".uploads.yml"), "comment:\n adoptLinkedFiles: false\n"); + const run: CommandRunner = (cmd, args) => { + if (cmd === "git" && args.includes("--show-toplevel")) return `${dir}\n`; + if (cmd !== "gh") throw new Error(`unexpected command: ${cmd}`); + if (args[1]?.includes("per_page=100")) return "[]"; + return JSON.stringify({ id: 9 }); + }; + const attachExisting = vi.fn(); + const client = fakeClient({ + attachExisting, + listAll: async () => [], + findGalleriesByReference: async () => ({ galleries: [], nextCursor: null }), + } as never); + const res = await syncAttachmentsComment( + client, + { repo: "acme/web", num: 12, kind: "pull" }, + run, + ); + expect(attachExisting).not.toHaveBeenCalled(); + expect(res.action).toBe("skipped"); + }); + + it("does not scan for adoption when the bot posts successfully", async () => { + const attachExisting = vi.fn(); + const client = fakeClient({ + attachExisting, + upsertGithubComment: async () => ({ posted: true, action: "created", count: 1 }), + } as never); + const run: CommandRunner = () => { + throw new Error("gh must not be called on the bot success path"); + }; + const res = await syncAttachmentsComment( + client, + { repo: "acme/web", num: 12, kind: "pull" }, + run, + ); + expect(res.via).toBe("bot"); + expect(attachExisting).not.toHaveBeenCalled(); + }); + }); + describe("repo comment config (.uploads.yml)", () => { let dirs: string[] = [];