diff --git a/docs/plugins/github.md b/docs/plugins/github.md index d6236b8c031a..1c16ec4f7dc2 100644 --- a/docs/plugins/github.md +++ b/docs/plugins/github.md @@ -106,6 +106,9 @@ hovercard, and GitHub links open externally. up to 100 results. Incomplete or unavailable checks retain a link to GitHub. - Long text and patches are bounded. Incomplete content is labeled rather than presented as a complete conversation or diff. +- Uncached hover previews share a two-second upstream request budget. Slow avatars + or co-author lookups are omitted; slow required metadata returns a retryable + unavailable error. The full reader keeps its longer request timeout. - **Refresh** requests the current item again. Rate limits, deleted items, and unavailable services show their specific explanation in the reader and hovercards, including GitHub's retry delay when available. Cached preview details stay visible diff --git a/extensions/github/src/preview.test.ts b/extensions/github/src/preview.test.ts index f813293fc527..dee1c5967f9f 100644 --- a/extensions/github/src/preview.test.ts +++ b/extensions/github/src/preview.test.ts @@ -1,6 +1,6 @@ import { createDeferred } from "openclaw/plugin-sdk/extension-shared"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { ControlUiGitHubError } from "./github-api.js"; +import { ControlUiGitHubError, formatControlUiGitHubPreviewError } from "./github-api.js"; import { loadControlUiGitHubPreview as loadPluginPreview, parseControlUiGitHubPreviewTarget, @@ -131,10 +131,104 @@ describe("loadControlUiGitHubPreview", () => { vi.stubEnv("GITHUB_TOKEN", ""); }); - afterEach(() => { + afterEach(async () => { + if (vi.isFakeTimers()) { + await vi.runAllTimersAsync(); + vi.useRealTimers(); + } + vi.restoreAllMocks(); vi.unstubAllEnvs(); }); + it.each(["repository", "item", "body", "commits", "avatar", "co-author avatar"])( + "bounds slow %s reads with the preview deadline and reuses the settled cache", + async (stage) => { + vi.useFakeTimers(); + // Native AbortSignal timers do not use Vitest's clock. + vi.spyOn(AbortSignal, "timeout").mockImplementation((ms) => { + const controller = new AbortController(); + setTimeout(() => controller.abort(new DOMException("Timed out", "TimeoutError")), ms); + return controller.signal; + }); + const identity = managedIdentity(`deadline-${stage}`); + const fetchMock = vi.fn().mockImplementation(async (input, init) => { + const url = requestUrl(input); + const signal = init?.signal; + if (!signal) { + throw new Error("Expected cancellable GitHub transport"); + } + const currentStage = url.endsWith("/repos/openclaw/openclaw") + ? "repository" + : url.includes("/commits") + ? "commits" + : url.includes("/u/20") + ? "co-author avatar" + : url.includes("avatars.") + ? "avatar" + : "item"; + if (currentStage === stage || (stage === "body" && currentStage === "item")) { + if (stage === "body") { + return new Response( + new ReadableStream({ + start(controller) { + signal.addEventListener("abort", () => controller.error(signal.reason), { + once: true, + }); + }, + }), + ); + } + return new Promise((_resolve, reject) => { + signal.addEventListener( + "abort", + () => reject(new DOMException("Aborted", "AbortError")), + { + once: true, + }, + ); + }); + } + // Metadata/commits consume budget before a later slow request starts. + await new Promise((resolve) => { + setTimeout(resolve, 200); + }); + if (currentStage === "repository") { + return publicRepository(); + } + if (currentStage === "commits") { + return githubJson([ + { commit: { message: "Co-authored-by: Ada <20+ada@users.noreply.github.com>" } }, + ]); + } + return currentStage === "item" ? githubJson(previewPayload()) : pngResponse(); + }); + const target = previewTarget(930307, "pull"); + const settled = vi.fn(); + const load = () => + loadControlUiGitHubPreview(target, identity, fetchMock).then( + (preview) => ({ preview }), + (error: unknown) => ({ error: formatControlUiGitHubPreviewError(error) }), + ); + const pending = load().then(settled); + await vi.advanceTimersByTimeAsync(2_000); + expect(settled).toHaveBeenCalledOnce(); + if (["repository", "item", "body"].includes(stage)) { + expect(settled).toHaveBeenCalledWith({ + error: expect.objectContaining({ retryable: true }), + }); + } else { + expect(settled).toHaveBeenCalledWith({ + preview: expect.objectContaining({ login: "steipete" }), + }); + } + await pending; + const calls = fetchMock.mock.calls.length; + expect(await load()).toEqual(settled.mock.calls[0]?.[0]); + expect(fetchMock).toHaveBeenCalledTimes(calls); + expect(identity.revalidate).toHaveBeenCalled(); + }, + ); + it("keeps selected identity caches separate and revalidates cached delivery", async () => { const fixtureTarget = previewTarget(88122); const firstIdentity = managedIdentity("first-preview-identity"); diff --git a/extensions/github/src/preview.ts b/extensions/github/src/preview.ts index 1e6743bd5c2d..5afe4aa719fa 100644 --- a/extensions/github/src/preview.ts +++ b/extensions/github/src/preview.ts @@ -9,7 +9,6 @@ import { discardResponse, fetchGitHubApi, GITHUB_API_ORIGIN, - GITHUB_REQUEST_TIMEOUT_MS, readBoundedResponse, readGitHubJsonResponse, requiredString, @@ -20,6 +19,7 @@ import { parseGitHubItemTarget, type GitHubItemTarget } from "./targets.js"; const GITHUB_AVATAR_HOST = "avatars.githubusercontent.com"; const GITHUB_AVATAR_MAX_BYTES = 256 * 1024; +const GITHUB_PREVIEW_TIMEOUT_MS = 2_000; // One commits page bounds the extra request; the card only renders three faces, // so deeper paging would spend quota on people it can never show. const GITHUB_COMMITS_PAGE_SIZE = 100; @@ -65,10 +65,11 @@ export async function assertPublicGitHubRepository( fetchImpl: typeof fetch, token?: string, identity?: ControlUiGitHubPreviewIdentity, + signal?: AbortSignal, ): Promise { // Stop before item reads so shared credentials cannot probe private item numbers. const repository = await readGitHubJsonResponse( - await fetchGitHubApi(repositoryUrl, fetchImpl, token, undefined, identity), + await fetchGitHubApi(repositoryUrl, fetchImpl, token, undefined, identity, undefined, signal), ); if (!isPublicGitHubRepository(repository)) { throw new ControlUiGitHubError(404, "GitHub repository is not public"); @@ -174,6 +175,7 @@ async function fetchCoAuthors( authorLogin: string, loadCommits: () => Promise, fetchImpl: typeof fetch, + signal: AbortSignal, ): Promise<{ coAuthors: { login: string; avatarDataUrl?: string }[]; coAuthorCount: number }> { const empty = { coAuthors: [], coAuthorCount: 0 }; let commits: unknown; @@ -212,6 +214,7 @@ async function fetchCoAuthors( const avatarDataUrl = await fetchAvatarDataUrl( `https://${GITHUB_AVATAR_HOST}/u/${face.accountId}`, fetchImpl, + signal, ); return avatarDataUrl ? { login: face.login, avatarDataUrl } : { login: face.login }; }), @@ -222,6 +225,7 @@ async function fetchCoAuthors( async function fetchAvatarDataUrl( rawUrl: string | undefined, fetchImpl: typeof fetch, + signal: AbortSignal, ): Promise { const url = safeAvatarUrl(rawUrl); if (!url) { @@ -231,7 +235,7 @@ async function fetchAvatarDataUrl( const response = await fetchImpl(url, { headers: { Accept: "image/webp,image/png,image/jpeg,image/gif" }, redirect: "error", - signal: AbortSignal.timeout(GITHUB_REQUEST_TIMEOUT_MS), + signal, }); const contentType = response.headers.get("content-type")?.split(";", 1)[0]?.trim(); if ( @@ -252,13 +256,14 @@ async function fetchAvatarDataUrl( async function fetchPreview( target: ControlUiGitHubPreviewTarget, fetchImpl: typeof fetch, + signal: AbortSignal, token?: string, identity?: ControlUiGitHubPreviewIdentity, ): Promise { const request = (url: string, beforeRedirect?: (url: URL) => Promise) => - fetchGitHubApi(url, fetchImpl, token, beforeRedirect, identity); + fetchGitHubApi(url, fetchImpl, token, beforeRedirect, identity, undefined, signal); const assertPublicRepository = (url: string) => - assertPublicGitHubRepository(url, fetchImpl, token, identity); + assertPublicGitHubRepository(url, fetchImpl, token, identity, signal); const repositoryUrl = `${GITHUB_API_ORIGIN}/repos/${encodeURIComponent(target.owner)}/${encodeURIComponent(target.repo)}`; const itemUrl = `${repositoryUrl}/${target.kind === "pull" ? "pulls" : "issues"}/${target.number}`; if (token) { @@ -288,7 +293,7 @@ async function fetchPreview( // Both extra fetches run only after the public-repository assertions above, // so neither can widen what this token is allowed to read. const [avatarDataUrl, coAuthorFacts] = await Promise.all([ - fetchAvatarDataUrl(avatarUrl, fetchImpl), + fetchAvatarDataUrl(avatarUrl, fetchImpl, signal), target.kind === "pull" ? fetchCoAuthors( preview.login, @@ -298,6 +303,7 @@ async function fetchPreview( GITHUB_COMMITS_MAX_BYTES, ), fetchImpl, + signal, ) : Promise.resolve({ coAuthors: [], coAuthorCount: 0 }), ]); @@ -340,11 +346,14 @@ export async function loadControlUiGitHubPreview( previewCache.set(key, entry); } else { const successCacheMs = token ? AUTHENTICATED_SUCCESS_CACHE_MS : ANONYMOUS_SUCCESS_CACHE_MS; + // One upstream budget includes redirects, optional-auth retries and decoration; + // a slow avatar or commits page must not add another full request timeout. + const signal = AbortSignal.timeout(GITHUB_PREVIEW_TIMEOUT_MS); const request = identity && !identity.optionalAuth - ? fetchPreview(target, fetchImpl, token, identity) + ? fetchPreview(target, fetchImpl, signal, token, identity) : withOptionalGitHubAuth(token, (requestToken) => - fetchPreview(target, fetchImpl, requestToken, identity), + fetchPreview(target, fetchImpl, signal, requestToken, identity), ); const pending: CacheEntry = { expiresAt: now + successCacheMs,