diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index e6a6edce3b3..3cee887ce3a 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -51,7 +51,7 @@ const { isFatalAgentInitializationError, codeExecutionAuthHeaders, createLaneGitRecorder, - isPullRequestFeatureActive, + resolvePullRequestRecording, createAttachedWorkspaceBashTool, createRepositoryInstructionSource, createRepositoryInstructionLoader, @@ -2369,7 +2369,7 @@ async function loadToolsForExecution({ workspaceId: codeExecutionContext.codeWorkspace.workspaceId, workspaceInstanceId: codeExecutionContext.codeWorkspace.workspaceInstanceId, onLaneGit: await createLaneGitRecorder({ - enabled: isPullRequestFeatureActive( + ...resolvePullRequestRecording( req.config?.endpoints?.agents?.pullRequests, process.env, ), diff --git a/e2e/config/librechat.e2e.yaml b/e2e/config/librechat.e2e.yaml index 73d14844222..5e85f397d8c 100644 --- a/e2e/config/librechat.e2e.yaml +++ b/e2e/config/librechat.e2e.yaml @@ -146,6 +146,14 @@ endpoints: - run_in_background - stateful_code_sessions # __E2E_CODE_BRIDGE_CONFIG__ + # Pull request lookups reach the fake GitHub Enterprise host + # (e2e/setup/fake-git-host-server.js), and only for the repository the pull request + # scenarios seed, so no other conversation ever starts a lookup. + pullRequests: + host: localhost:8793 + token: '${E2E_GIT_HOST_TOKEN}' + allowedRepositories: + - e2e-org/e2e-app # Keep the shared mock profile non-interactive except for the dedicated # approval probe. This exercises real HITL pause/resume without wedging the # existing file-authoring, steering, background-tool, or MCP specs. diff --git a/e2e/playwright.config.mock.ts b/e2e/playwright.config.mock.ts index a94fda2745d..a579fcabccd 100644 --- a/e2e/playwright.config.mock.ts +++ b/e2e/playwright.config.mock.ts @@ -1,4 +1,5 @@ import { defineConfig, devices } from '@playwright/test'; +import { execFileSync } from 'child_process'; import fs from 'fs'; import path from 'path'; import { getLocalE2EEnv, getE2EBaseURL } from './setup/env'; @@ -38,6 +39,12 @@ const ragServerPath = path.resolve(rootPath, 'e2e/setup/fake-rag-server.js'); * Kept clear of the MCP (8765/8766) and label (8889) fixtures. */ const CODE_API_PORT = process.env.E2E_CODE_API_PORT || '8790'; const RAG_API_PORT = process.env.E2E_RAG_API_PORT || '8791'; +/** The fake GitHub Enterprise host pull request lookups reach (`pullRequests.host`). The template + * hard-codes 8793; `writeRuntimeMockConfig` substitutes any override into the generated copy. */ +const gitHostServerPath = path.resolve(rootPath, 'e2e/setup/fake-git-host-server.js'); +const GIT_HOST_PORT = process.env.E2E_GIT_HOST_PORT || '8793'; +const gitHostKeyPath = path.resolve(rootPath, 'e2e/.generated/git-host.key'); +const gitHostCertPath = path.resolve(rootPath, 'e2e/.generated/git-host.crt'); const fakeModelHookPath = path.resolve(rootPath, 'e2e/setup/fake-model.js'); /** Model-fixture record mode: the run hook taps the REAL provider stream into a * replayable fixture instead of overriding the model (e2e/setup/model-replay.js). */ @@ -219,6 +226,9 @@ const baseEnv = { ...(externalCodeBaseUrl ? { LIBRECHAT_CODE_BASEURL_STATEFUL: externalCodeBaseUrl } : {}), ...codeApiKeyEnv, RAG_API_URL: `http://127.0.0.1:${RAG_API_PORT}`, + /** Pull request lookups go to the fake git host, which trusts any bearer token. */ + E2E_GIT_HOST_TOKEN: 'e2e-git-host-token', + NODE_EXTRA_CA_CERTS: gitHostCertPath, ...vanillaOverrides, }; @@ -383,6 +393,9 @@ function writeRuntimeMockConfig() { if (enableDynamicMcp && MCP_DYNAMIC_PORT !== '8766') { config = config.split('127.0.0.1:8766').join(`127.0.0.1:${MCP_DYNAMIC_PORT}`); } + if (GIT_HOST_PORT !== '8793') { + config = config.split('localhost:8793').join(`localhost:${GIT_HOST_PORT}`); + } if (MCP_OAUTH_PORT !== '8767') { config = config.split('127.0.0.1:8767').join(`127.0.0.1:${MCP_OAUTH_PORT}`); } @@ -394,6 +407,65 @@ function writeRuntimeMockConfig() { } } +/** A kept certificate is replaced when it expires within this window, so no run outlives it. */ +const GIT_HOST_CERT_MIN_SECONDS = 7 * 24 * 60 * 60; + +function hasFreshGitHostCertificate(): boolean { + if (!fs.existsSync(gitHostKeyPath) || !fs.existsSync(gitHostCertPath)) { + return false; + } + try { + /** Exits non-zero when the certificate expires within the window, or cannot be read. */ + execFileSync( + 'openssl', + ['x509', '-checkend', String(GIT_HOST_CERT_MIN_SECONDS), '-noout', '-in', gitHostCertPath], + { stdio: 'ignore' }, + ); + return true; + } catch { + return false; + } +} + +/** + * The fake git host speaks TLS, as every pull request host must, with a certificate for + * `localhost` the app server trusts through `NODE_EXTRA_CA_CERTS`. Node reads that file once at + * startup, so it is made here, before any web server starts, and kept while it is valid for a + * week more. The config module loads again in every worker, which must not swap the pair under a + * running server, so only the main process ever replaces one. + */ +function ensureGitHostCertificate() { + const exists = fs.existsSync(gitHostKeyPath) && fs.existsSync(gitHostCertPath); + if (exists && process.env.TEST_WORKER_INDEX != null) { + return; + } + if (hasFreshGitHostCertificate()) { + return; + } + fs.mkdirSync(path.dirname(gitHostCertPath), { recursive: true }); + execFileSync( + 'openssl', + [ + 'req', + '-x509', + '-newkey', + 'rsa:2048', + '-nodes', + '-keyout', + gitHostKeyPath, + '-out', + gitHostCertPath, + '-days', + '30', + '-subj', + '/CN=localhost', + '-addext', + 'subjectAltName=DNS:localhost', + ], + { stdio: 'ignore' }, + ); +} + function neutralizeCredentialEnv(env: NodeJS.ProcessEnv, keep: Set) { for (const key of Object.keys(env)) { if (!keep.has(key) && SECRET_KEY_PATTERN.test(key)) { @@ -424,6 +496,7 @@ function neutralizeDotenvSecrets(envFile: string, keep: Set) { } writeRuntimeMockConfig(); +ensureGitHostCertificate(); neutralizeCredentialEnv(process.env, preservedCredentialEnvKeys); Object.assign(process.env, baseEnv); neutralizeDotenvSecrets(path.resolve(rootPath, '.env'), preservedCredentialEnvKeys); @@ -549,6 +622,22 @@ export default defineConfig({ timeout: 60_000, reuseExistingServer: false, }, + { + // Fake GitHub Enterprise host for the pull request scenarios (`pullRequests.host`). + command: `node ${gitHostServerPath}`, + cwd: rootPath, + env: { + ...process.env, + E2E_GIT_HOST_PORT: GIT_HOST_PORT, + E2E_GIT_HOST_KEY: gitHostKeyPath, + E2E_GIT_HOST_CERT: gitHostCertPath, + }, + url: `https://localhost:${GIT_HOST_PORT}/`, + ignoreHTTPSErrors: true, + stdout: 'pipe', + timeout: 60_000, + reuseExistingServer: false, + }, { // Fake RAG (vector DB) API for file-provisioning specs (RAG_API_URL). command: `node ${ragServerPath}`, diff --git a/e2e/setup/fake-git-host-server.js b/e2e/setup/fake-git-host-server.js new file mode 100644 index 00000000000..6b97666a1e0 --- /dev/null +++ b/e2e/setup/fake-git-host-server.js @@ -0,0 +1,126 @@ +/** + * HTTPS fixture standing in for a GitHub Enterprise Server host, for the pull request scenarios. + * + * The mock config points `endpoints.agents.pullRequests.host` at `localhost:`, so the + * server's GitHub source sends every request of a lookup to `/api/v3` here, over TLS with the + * self-signed certificate `playwright.config.mock.ts` generates and hands the app server through + * `NODE_EXTRA_CA_CERTS`. It serves one repository with one open pull request per branch named in + * `PULLS`, and records what it was asked so a scenario can prove which host a lookup reached. + * + * `GET /__requests` lists the recorded requests (method, path, whether a bearer token came with + * it); `POST /__reset` clears them. Neither is under `/api/v3`, so a lookup never reaches them. + */ +const fs = require('fs'); +const https = require('https'); + +const PORT = Number(process.env.E2E_GIT_HOST_PORT) || 8793; +const HOST = `localhost:${PORT}`; +const REPO = 'e2e-org/e2e-app'; +const API_PREFIX = '/api/v3'; + +/** One open pull request per branch; the scenarios record the same head on their lane. */ +const PULLS = { + 'feat/enterprise-header': { + number: 7, + title: 'Enterprise header pull request', + sha: 'a'.repeat(40), + }, + 'feat/override-ignored': { + number: 8, + title: 'Override ignored pull request', + sha: 'b'.repeat(40), + }, +}; + +const requests = []; + +const byNumber = (number) => + Object.entries(PULLS).find(([, pull]) => pull.number === number) ?? null; + +function listItem([, pull]) { + return { number: pull.number, state: 'open', head: { sha: pull.sha } }; +} + +function detail([branch, pull]) { + return { + number: pull.number, + title: pull.title, + html_url: `https://${HOST}/${REPO}/pull/${pull.number}`, + state: 'open', + merged: false, + draft: false, + additions: 12, + deletions: 3, + mergeable: true, + head: { sha: pull.sha, ref: branch, repo: { full_name: REPO } }, + }; +} + +function send(res, status, body) { + res.writeHead(status, { 'content-type': 'application/json' }); + res.end(JSON.stringify(body)); +} + +function route(url) { + const repoBase = `${API_PREFIX}/repos/${REPO}`; + if (!url.pathname.startsWith(repoBase)) return null; + const rest = url.pathname.slice(repoBase.length); + if (rest === '/pulls') { + const state = url.searchParams.get('state'); + const headFilter = url.searchParams.get('head') ?? ''; + const branch = headFilter.slice(headFilter.indexOf(':') + 1); + const pull = PULLS[branch]; + return [200, state === 'open' && pull != null ? [listItem([branch, pull])] : []]; + } + const pullMatch = /^\/pulls\/(\d+)$/.exec(rest); + if (pullMatch) { + const found = byNumber(Number(pullMatch[1])); + return found ? [200, detail(found)] : [404, { message: 'Not Found' }]; + } + if (/^\/commits\/[a-f0-9]{40}\/check-runs$/.test(rest)) { + return [200, { total_count: 1, check_runs: [{ status: 'completed', conclusion: 'success' }] }]; + } + if (/^\/commits\/[a-f0-9]{40}\/pulls$/.test(rest)) { + return [200, []]; + } + return null; +} + +const server = https.createServer( + { + key: fs.readFileSync(process.env.E2E_GIT_HOST_KEY), + cert: fs.readFileSync(process.env.E2E_GIT_HOST_CERT), + }, + (req, res) => { + const url = new URL(req.url ?? '/', `https://${HOST}`); + if (url.pathname === '/') { + send(res, 200, { ok: true }); + return; + } + if (url.pathname === '/__requests' && req.method === 'GET') { + send(res, 200, requests); + return; + } + if (url.pathname === '/__reset' && req.method === 'POST') { + requests.length = 0; + send(res, 200, { ok: true }); + return; + } + requests.push({ + method: req.method, + path: `${url.pathname}${url.search}`, + authorized: (req.headers.authorization ?? '').startsWith('Bearer '), + }); + const answer = req.method === 'GET' ? route(url) : null; + if (answer == null) { + send(res, 404, { message: 'Not Found' }); + return; + } + send(res, answer[0], answer[1]); + }, +); + +/** Every local address, since `localhost` may resolve to either family. */ +server.listen(PORT, () => { + console.log(`[fake-git-host] listening on https://${HOST}`); +}); diff --git a/e2e/specs/mock/scenarios/pull-request-enterprise-host.spec.ts b/e2e/specs/mock/scenarios/pull-request-enterprise-host.spec.ts new file mode 100644 index 00000000000..6cdbb3af1eb --- /dev/null +++ b/e2e/specs/mock/scenarios/pull-request-enterprise-host.spec.ts @@ -0,0 +1,176 @@ +import { randomUUID } from 'crypto'; +import { expect, test } from '@playwright/test'; +import type { APIRequestContext, Page } from '@playwright/test'; +import { deleteConversations, seedConversations, withMongo } from '../db'; +import { getPrimaryE2EUser } from '../../../setup/users.mock'; + +/** + * Pull request lookups against a GitHub Enterprise host. The mock config sets + * `endpoints.agents.pullRequests.host` to the fake host `e2e/setup/fake-git-host-server.js` + * serves over TLS, so a code conversation whose lane names a branch with an open pull request + * shows it in the chat header, read from that host and linking to that host's page. The host + * decides where the deployment's token goes, so a principal config override cannot move it. + */ + +const GIT_HOST = `localhost:${process.env.E2E_GIT_HOST_PORT || '8793'}`; +const REPO = 'e2e-org/e2e-app'; + +type Lane = { branch: string; head: string; title: string; number: number }; + +/** Branches and heads `fake-git-host-server.js` serves an open pull request for. */ +const HEADER_LANE: Lane = { + branch: 'feat/enterprise-header', + head: 'a'.repeat(40), + title: 'Enterprise header pull request', + number: 7, +}; +const OVERRIDE_LANE: Lane = { + branch: 'feat/override-ignored', + head: 'b'.repeat(40), + title: 'Override ignored pull request', + number: 8, +}; + +test.use({ viewport: { width: 1280, height: 800 } }); + +/** A conversation that ran in an attached workspace, with the lane the worker last reported. */ +async function seedCodeConversation(lane: Lane): Promise { + const conversationId = randomUUID(); + const { email } = getPrimaryE2EUser(); + await seedConversations(email, [ + { conversationId, title: `Lane ${lane.branch}`, updatedAt: new Date() }, + ]); + await withMongo(async (db) => { + await db.collection('conversations').updateOne( + { conversationId }, + { + $set: { + codeEnvironmentMode: 'attached', + laneGit: { branch: lane.branch, head: lane.head, repo: REPO, seq: 1 }, + }, + }, + ); + }); + return conversationId; +} + +type SeenRequest = { method: string; path: string; authorized: boolean }; + +async function gitHostRequests(request: APIRequestContext): Promise { + const res = await request.get(`https://${GIT_HOST}/__requests`, { ignoreHTTPSErrors: true }); + expect(res.ok()).toBeTruthy(); + return (await res.json()) as SeenRequest[]; +} + +/** Opens the conversation and the pull request card under its header chip. */ +async function openHeaderCard(page: Page, conversationId: string, lane: Lane) { + await page.goto(`/c/${conversationId}`); + const chip = page.getByTestId('header-pull-request-button'); + await expect(chip).toBeVisible({ timeout: 30000 }); + await expect(chip).toContainText(lane.title); + await chip.focus(); + const card = page.getByRole('dialog', { name: 'Pull request' }); + await expect(card).toBeVisible(); + await expect(card.getByText(`PR #${lane.number}`)).toBeVisible(); + return card; +} + +test.describe('Pull requests on a GitHub Enterprise host', () => { + test.describe.configure({ mode: 'serial' }); + + /** Waits for the config cache and a lookup, so a failure reports its assertion, not a timeout. */ + test.setTimeout(120_000); + + const seeded: string[] = []; + let overriddenUser: string | undefined; + + test.afterAll(async () => { + await deleteConversations(seeded); + if (overriddenUser != null) { + const principalId = overriddenUser; + await withMongo((db) => db.collection('configs').deleteMany({ principalId })); + } + }); + + test('a code conversation shows its pull request from the configured host @scenario:enterprise-host-pull-request-appears-in-header', async ({ + page, + request, + }) => { + const conversationId = await seedCodeConversation(HEADER_LANE); + seeded.push(conversationId); + + const card = await openHeaderCard(page, conversationId, HEADER_LANE); + await expect(card.getByRole('link', { name: 'Open in GitHub' })).toHaveAttribute( + 'href', + `https://${GIT_HOST}/${REPO}/pull/${HEADER_LANE.number}`, + ); + + const seen = await gitHostRequests(request); + const lookups = seen.filter((entry) => + entry.path.includes(encodeURIComponent(HEADER_LANE.branch)), + ); + expect(lookups.length).toBeGreaterThan(0); + expect(lookups.every((entry) => entry.path.startsWith(`/api/v3/repos/${REPO}/`))).toBe(true); + expect(seen.every((entry) => entry.authorized)).toBe(true); + }); + + test('a principal override naming another host does not move the lookup @scenario:role-override-cannot-move-pull-request-host', async ({ + page, + request, + }) => { + const user = getPrimaryE2EUser(); + const login = await request.post('/api/auth/login', { + data: { email: user.email, password: user.password }, + }); + expect(login.ok()).toBeTruthy(); + const { token, user: body } = (await login.json()) as { + token: string; + user: { id?: string; _id?: string }; + }; + const userId = (body.id ?? body._id) as string; + overriddenUser = userId; + const headers = { Authorization: `Bearer ${token}` }; + const overridePath = `/api/admin/config/user/${userId}`; + const marker = `pull-request-override-${Date.now()}`; + + const stored = await request.put(overridePath, { + headers, + data: { + overrides: { + interface: { customWelcome: marker }, + endpoints: { agents: { pullRequests: { host: 'pull-requests.e2e.invalid' } } }, + }, + }, + }); + expect(stored.ok()).toBeTruthy(); + try { + /** The override as a whole applies, so only the host is held back. */ + await expect + .poll( + async () => { + const res = await request.get('/api/config', { headers }); + const config = (await res.json()) as { interface?: { customWelcome?: string } }; + return config.interface?.customWelcome; + }, + { timeout: 30000, intervals: [500, 1000, 2000] }, + ) + .toBe(marker); + + const conversationId = await seedCodeConversation(OVERRIDE_LANE); + seeded.push(conversationId); + const card = await openHeaderCard(page, conversationId, OVERRIDE_LANE); + await expect(card.getByRole('link', { name: 'Open in GitHub' })).toHaveAttribute( + 'href', + `https://${GIT_HOST}/${REPO}/pull/${OVERRIDE_LANE.number}`, + ); + + const seen = await gitHostRequests(request); + expect( + seen.some((entry) => entry.path.includes(encodeURIComponent(OVERRIDE_LANE.branch))), + ).toBe(true); + } finally { + const cleared = await request.delete(overridePath, { headers }); + expect([200, 204, 404]).toContain(cleared.status()); + } + }); +}); diff --git a/packages/api/src/code/lane.spec.ts b/packages/api/src/code/lane.spec.ts index fbf9786d54b..6e7d4533203 100644 --- a/packages/api/src/code/lane.spec.ts +++ b/packages/api/src/code/lane.spec.ts @@ -119,6 +119,12 @@ describe('createLaneGitRecorder', () => { ); }); + it('records only a link on the configured host', async () => { + const { record, setConvoLaneGit } = make({ repo: 'o/r', host: 'ghe.example.com' }); + await record?.(laneGit, 'https://ghe.example.com/o/r/pull/5 https://github.com/o/r/pull/9'); + expect(setConvoLaneGit).toHaveBeenCalledWith(expect.objectContaining({ pullRequest: 5 })); + }); + it.each([ ['output without a link', 'nothing to see', 'o/r', laneGit], ['a link to another repository', 'https://github.com/x/y/pull/42', 'o/r', laneGit], diff --git a/packages/api/src/code/lane.ts b/packages/api/src/code/lane.ts index 3a2c2bb88c5..1c697ea3a25 100644 --- a/packages/api/src/code/lane.ts +++ b/packages/api/src/code/lane.ts @@ -78,6 +78,7 @@ const reservationTails = new Map>(); */ export async function createLaneGitRecorder({ enabled, + host, user, conversationId, repo, @@ -89,6 +90,8 @@ export async function createLaneGitRecorder({ }: { /** Whether the deployment shows pull requests; when it does not, nothing is read or recorded. */ enabled: boolean; + /** The configured pull request host; a link to any other host is not recorded. */ + host?: string; user: string | undefined; conversationId: string | undefined; repo?: string; @@ -177,7 +180,7 @@ export async function createLaneGitRecorder({ return (laneGit, output) => { const pullRequest = safeRepo != null && output != null && laneGit.branch != null - ? findPullRequestLink(output, safeRepo) + ? findPullRequestLink(output, safeRepo, host) : null; const previous = reservationTails.get(queueKey) ?? Promise.resolve(); const reservation = previous.then(() => reserve()); diff --git a/packages/api/src/pulls/bitbucket.spec.ts b/packages/api/src/pulls/bitbucket.spec.ts new file mode 100644 index 00000000000..5ab6fbd3984 --- /dev/null +++ b/packages/api/src/pulls/bitbucket.spec.ts @@ -0,0 +1,501 @@ +import { createBitbucketPullRequestSource, isSameCommit, summarizeStatuses } from './bitbucket'; + +const API = 'https://api.bitbucket.org/2.0/repositories/w/r/'; +const sha = 'a'.repeat(40); +const short = sha.slice(0, 12); +const newer = 'b'.repeat(12); +const pullRequest = (overrides: Record = {}) => ({ + id: 7, + title: 'Simplify single tool', + state: 'OPEN', + draft: false, + links: { html: { href: 'https://bitbucket.org/w/r/pull-requests/7' } }, + source: { + branch: { name: 'feat/x' }, + commit: { hash: short }, + repository: { full_name: 'W/R' }, + }, + ...overrides, +}); +const fork = (hash = short) => ({ + branch: { name: 'feat/x' }, + commit: { hash }, + repository: { full_name: 'someone/r' }, +}); +const page = (values: unknown[], next?: string) => ({ values, ...(next ? { next } : {}) }); +const listed = (overrides: Record = {}) => page([pullRequest(overrides)]); +const status = (state: string, hash = sha) => ({ state, key: state, commit: { hash } }); +const json = (body: unknown, init?: ResponseInit) => Response.json(body, init); + +function sourceFor(routes: Record Response>) { + const fetchFn = jest.fn(async (input: string, _init?: RequestInit) => { + const url = String(input); + /** The most specific fragment wins, so a page link can be routed apart from its first page. */ + const key = Object.keys(routes) + .filter((fragment) => url.includes(fragment)) + .sort((a, b) => b.length - a.length)[0]; + if (key == null) throw new Error(`unexpected ${url}`); + return routes[key](); + }); + return { fetchFn, source: createBitbucketPullRequestSource({ fetchFn }) }; +} + +const routes = (detail = pullRequest(), overrides: Record Response> = {}) => ({ + 'state=OPEN': () => json(listed()), + 'state=MERGED': () => json(page([])), + '/pullrequests/7/diffstat': () => json(page([{ lines_added: 12, lines_removed: 3 }])), + '/pullrequests/7/statuses': () => json(page([status('SUCCESSFUL')])), + '/pullrequests/7': () => json(detail), + ...overrides, +}); + +const find = ( + source: ReturnType, + input: Record = {}, +) => source.find({ repo: 'w/r', branch: 'feat/x', token: 't', ...input }); + +const urlsOf = (fetchFn: ReturnType['fetchFn']) => + fetchFn.mock.calls.map(([url]) => String(url)); + +describe('summarizeStatuses', () => { + it.each([ + [[], false, 'none'], + [['SUCCESSFUL'], false, 'passing'], + [['SUCCESSFUL', 'INPROGRESS'], false, 'running'], + [['INPROGRESS', 'FAILED'], false, 'failing'], + [['SUCCESSFUL', 'STOPPED'], false, 'failing'], + [['SUCCESSFUL'], true, 'running'], + [[], true, 'running'], + ])('reports %p (incomplete %p) as %s', (states, incomplete, expected) => { + expect(summarizeStatuses(states, incomplete)).toBe(expected); + }); +}); + +describe('isSameCommit', () => { + it('compares by the shorter hash, never by fewer than seven characters', () => { + expect(isSameCommit(sha, short)).toBe(true); + expect(isSameCommit(short, sha)).toBe(true); + expect(isSameCommit(sha, newer)).toBe(false); + expect(isSameCommit(sha, 'aaaaaa')).toBe(false); + }); +}); + +describe('createBitbucketPullRequestSource', () => { + it('maps an open pull request with passing statuses', async () => { + const { source } = sourceFor(routes()); + await expect(find(source)).resolves.toEqual({ + number: 7, + title: 'Simplify single tool', + url: 'https://bitbucket.org/w/r/pull-requests/7', + provider: 'bitbucket', + additions: 12, + deletions: 3, + state: 'open', + isDraft: false, + mergeable: 'unknown', + checks: 'passing', + }); + }); + + it('asks the repository on the API host only, with a bearer token', async () => { + const { source, fetchFn } = sourceFor(routes()); + await find(source); + const urls = urlsOf(fetchFn); + expect(urls.length).toBeGreaterThan(0); + expect(urls.every((url) => url.startsWith(API))).toBe(true); + for (const [, init] of fetchFn.mock.calls) { + const headers = (init?.headers ?? {}) as Record; + expect(headers.Authorization).toBe('Bearer t'); + expect(headers.Accept).toBe('application/json'); + } + }); + + it('lists open pull requests first, newest first, then the closed history', async () => { + const { source, fetchFn } = sourceFor( + routes(pullRequest({ state: 'MERGED' }), { + 'state=OPEN': () => json(page([])), + 'state=MERGED': () => json(listed({ state: 'MERGED' })), + }), + ); + await find(source); + const [open, closed] = urlsOf(fetchFn).map((url) => new URL(url).searchParams); + expect(open.get('q')).toBe('source.branch.name = "feat/x" AND (state = "OPEN")'); + expect(open.getAll('state')).toEqual(['OPEN']); + expect(open.get('sort')).toBe('-created_on'); + expect(open.get('pagelen')).toBe('10'); + expect(closed.get('q')).toBe( + 'source.branch.name = "feat/x" AND (state = "MERGED" OR state = "DECLINED" OR state = "SUPERSEDED")', + ); + expect(closed.getAll('state')).toEqual(['MERGED', 'DECLINED', 'SUPERSEDED']); + /** A full page even without a recorded commit, so a closed fork cannot hide the rest. */ + expect(closed.get('pagelen')).toBe('10'); + }); + + it('finds an older closed pull request behind a newer closed fork on the same branch', async () => { + const merged = pullRequest({ id: 3, state: 'MERGED' }); + const { source } = sourceFor( + routes(merged, { + 'state=OPEN': () => json(page([])), + 'state=MERGED': () => + json(page([pullRequest({ id: 8, state: 'DECLINED', source: fork() }), merged])), + '/pullrequests/3/diffstat': () => json(page([])), + '/pullrequests/3/statuses': () => json(page([])), + '/pullrequests/3': () => json(merged), + }), + ); + await expect(find(source)).resolves.toMatchObject({ number: 3, state: 'merged' }); + }); + + describe('closed history across pages', () => { + const merged = pullRequest({ id: 3, state: 'MERGED' }); + const declinedFork = pullRequest({ id: 8, state: 'DECLINED', source: fork() }); + const history = (next: string) => + routes(merged, { + 'state=OPEN': () => json(page([])), + /** A short first page that still links on: only a missing `next` ends the list. */ + 'state=MERGED': () => json(page([declinedFork], next)), + 'cursor=opaque': () => json(page([merged])), + '/pullrequests/3/diffstat': () => json(page([])), + '/pullrequests/3/statuses': () => json(page([])), + '/pullrequests/3': () => json(merged), + }); + + it('follows the next link Bitbucket returns instead of building page numbers', async () => { + const { source, fetchFn } = sourceFor(history(`${API}pullrequests?cursor=opaque`)); + await expect(find(source, { limits: { maxCandidatePages: 2 } })).resolves.toMatchObject({ + number: 3, + state: 'merged', + }); + expect(urlsOf(fetchFn)).toContain(`${API}pullrequests?cursor=opaque`); + expect(urlsOf(fetchFn).some((url) => /[?&]page=/.test(url))).toBe(false); + }); + + it('reads no more pages than the configured bound', async () => { + const { source, fetchFn } = sourceFor(history(`${API}pullrequests?cursor=opaque`)); + await expect(find(source, { limits: { maxCandidatePages: 1 } })).resolves.toBeNull(); + expect(urlsOf(fetchFn)).not.toContain(`${API}pullrequests?cursor=opaque`); + }); + + it('never follows a next link off the repository API', async () => { + const { source, fetchFn } = sourceFor( + history('https://evil.example/2.0/repositories/w/r/pullrequests?cursor=opaque'), + ); + await expect(find(source, { limits: { maxCandidatePages: 2 } })).rejects.toMatchObject({ + code: 'UPSTREAM_ERROR', + }); + expect(urlsOf(fetchFn).every((url) => url.startsWith(API))).toBe(true); + }); + }); + + it('skips a pull request whose source commit is gone and keeps searching', async () => { + const gone = pullRequest({ id: 5, source: { ...pullRequest().source, commit: null } }); + const { source } = sourceFor( + routes(pullRequest(), { 'state=OPEN': () => json(page([gone, pullRequest()])) }), + ); + await expect(find(source)).resolves.toMatchObject({ number: 7 }); + }); + + it('finds nothing when the detail lost its source commit', async () => { + const gone = pullRequest({ source: { ...pullRequest().source, commit: {} } }); + const { source } = sourceFor(routes(gone)); + await expect(find(source)).resolves.toBeNull(); + }); + + it.each([ + ['moved to another source branch', { branch: { name: 'other' } }], + ['moved to a fork', { repository: { full_name: 'someone/r' } }], + ])('finds nothing when the pull request %s after it was listed', async (_case, change) => { + const moved = pullRequest({ source: { ...pullRequest().source, ...change } }); + const { source } = sourceFor(routes(moved)); + await expect(find(source, { head: sha })).resolves.toBeNull(); + }); + + it('escapes a quote and a backslash in the branch name', async () => { + const branch = 'a"b\\c'; + const { source, fetchFn } = sourceFor({ + 'state=OPEN': () => json(page([])), + 'state=MERGED': () => json(page([])), + }); + await find(source, { branch }); + const q = new URL(urlsOf(fetchFn)[0]).searchParams.get('q'); + expect(q).toBe('source.branch.name = "a\\"b\\\\c" AND (state = "OPEN")'); + expect(urlsOf(fetchFn)[0]).not.toContain('"'); + }); + + it.each([ + ['another host', 'https://evil.example/w/r/pull-requests/7'], + ['plain http', 'http://bitbucket.org/w/r/pull-requests/7'], + ['a javascript url', 'javascript:alert(1)'], + ])('rejects %s as the pull request page', async (_label, href) => { + const { source } = sourceFor(routes(pullRequest({ links: { html: { href } } }))); + await expect(find(source)).rejects.toMatchObject({ code: 'UPSTREAM_ERROR' }); + }); + + it.each([ + ['MERGED', 'merged'], + ['DECLINED', 'closed'], + ['SUPERSEDED', 'closed'], + ])('reports a %s pull request as %s', async (state, expected) => { + const { source } = sourceFor( + routes(pullRequest({ state }), { + 'state=OPEN': () => json(page([])), + 'state=MERGED': () => json(listed({ state })), + }), + ); + await expect(find(source)).resolves.toMatchObject({ state: expected }); + }); + + it('reports a draft', async () => { + const { source } = sourceFor(routes(pullRequest({ draft: true }))); + await expect(find(source)).resolves.toMatchObject({ isDraft: true }); + }); + + it.each([ + ['failing', [status('SUCCESSFUL'), status('FAILED')]], + ['failing', [status('STOPPED')]], + ['running', [status('SUCCESSFUL'), status('INPROGRESS')]], + ['none', []], + ])('reports %s statuses', async (expected, statuses) => { + const { source } = sourceFor( + routes(pullRequest(), { '/pullrequests/7/statuses': () => json(page(statuses)) }), + ); + await expect(find(source)).resolves.toMatchObject({ checks: expected }); + }); + + it('ignores a status reported for another commit', async () => { + const { source } = sourceFor( + routes(pullRequest(), { + '/pullrequests/7/statuses': () => + json(page([status('SUCCESSFUL'), status('FAILED', 'c'.repeat(40))])), + }), + ); + await expect(find(source)).resolves.toMatchObject({ checks: 'passing' }); + }); + + it('never reports statuses past the page bound as passing', async () => { + const { source } = sourceFor( + routes(pullRequest(), { + '/pullrequests/7/statuses': () => + json(page([status('SUCCESSFUL')], `${API}pullrequests/7/statuses?page=2`)), + }), + ); + await expect(find(source, { limits: { maxCheckRunPages: 1 } })).resolves.toMatchObject({ + checks: 'running', + }); + }); + + it('sums the diffstat across pages', async () => { + const { source } = sourceFor( + routes(pullRequest(), { + '/pullrequests/7/diffstat?page=2': () => + json( + page([ + { lines_added: 5, lines_removed: 1 }, + { lines_added: 1, lines_removed: null }, + ]), + ), + '/pullrequests/7/diffstat': () => + json( + page([{ lines_added: 12, lines_removed: 3 }], `${API}pullrequests/7/diffstat?page=2`), + ), + }), + ); + await expect(find(source)).resolves.toMatchObject({ additions: 18, deletions: 4 }); + }); + + it.each([ + ['another host', 'https://evil.example/2.0/repositories/w/r/pullrequests/7/diffstat?page=2'], + ['plain http', 'http://api.bitbucket.org/2.0/repositories/w/r/pullrequests/7/diffstat?page=2'], + ['the site', 'https://bitbucket.org/2.0/repositories/w/r/pullrequests/7/diffstat?page=2'], + ])('never follows a next link to %s', async (_label, next) => { + const { source, fetchFn } = sourceFor( + routes(pullRequest(), { + '/pullrequests/7/diffstat': () => json(page([{ lines_added: 1, lines_removed: 1 }], next)), + }), + ); + await expect(find(source)).rejects.toMatchObject({ code: 'UPSTREAM_ERROR' }); + expect(urlsOf(fetchFn).every((url) => url.startsWith(API))).toBe(true); + }); + + it('finds nothing for a repository the token cannot see', async () => { + const { source } = sourceFor({ 'state=OPEN': () => new Response('', { status: 404 }) }); + await expect(find(source)).resolves.toBeNull(); + }); + + it('reports a rate limit with the wait Bitbucket asked for', async () => { + const { source } = sourceFor({ + 'state=OPEN': () => new Response('', { status: 429, headers: { 'retry-after': '30' } }), + }); + await expect(find(source)).rejects.toMatchObject({ + code: 'RATE_LIMITED', + retryAfterMs: 30_000, + }); + }); + + it('fails without upstream text on any other error', async () => { + const { source } = sourceFor({ + 'state=OPEN': () => new Response('secret', { status: 500 }), + }); + const error = await find(source).catch((caught: Error) => caught); + expect(error).toMatchObject({ code: 'UPSTREAM_ERROR' }); + expect(String((error as Error).message)).not.toContain('secret'); + }); + + it('fails on a malformed pull request', async () => { + const { source } = sourceFor(routes(pullRequest({ source: { commit: { hash: 'zz' } } }))); + await expect(find(source)).rejects.toMatchObject({ code: 'UPSTREAM_ERROR' }); + }); + + it('finds nothing for a lane that has a branch but no commit yet', async () => { + const { source, fetchFn } = sourceFor(routes()); + await expect(find(source, { head: null })).resolves.toBeNull(); + expect(fetchFn).not.toHaveBeenCalled(); + }); + + it('finds nothing for an empty branch', async () => { + const { source, fetchFn } = sourceFor(routes()); + await expect(find(source, { branch: '' })).resolves.toBeNull(); + expect(fetchFn).not.toHaveBeenCalled(); + }); + + it.each(['w', 'w/sub/r', 'w/..', './r', 'w/r?x', 'w/r%2F', ''])( + 'asks nothing for the path %p', + async (repo) => { + const { source, fetchFn } = sourceFor(routes()); + await expect(find(source, { repo })).resolves.toBeNull(); + expect(fetchFn).not.toHaveBeenCalled(); + }, + ); + + describe('with a recorded commit', () => { + it('accepts a head that is the recorded commit, abbreviated', async () => { + const { source, fetchFn } = sourceFor(routes()); + await expect(find(source, { head: sha })).resolves.toMatchObject({ number: 7 }); + expect(urlsOf(fetchFn).some((url) => url.includes('merge-base'))).toBe(false); + }); + + it('accepts a head that builds on the recorded commit', async () => { + const newerPull = pullRequest({ + source: { ...pullRequest().source, commit: { hash: newer } }, + }); + const { source, fetchFn } = sourceFor( + routes(newerPull, { + 'state=OPEN': () => json(page([newerPull])), + '/pullrequests/7/statuses': () => json(page([status('SUCCESSFUL', newer)])), + '/merge-base/': () => json({ hash: sha }), + }), + ); + await expect(find(source, { head: sha })).resolves.toMatchObject({ + number: 7, + checks: 'passing', + }); + expect(urlsOf(fetchFn)).toContain(`${API}merge-base/${sha}..${newer}`); + }); + + it('rejects a head that does not contain the recorded commit', async () => { + const newerPull = pullRequest({ + source: { ...pullRequest().source, commit: { hash: newer } }, + }); + const { source } = sourceFor( + routes(newerPull, { + 'state=OPEN': () => json(page([newerPull])), + '/merge-base/': () => json({ hash: 'c'.repeat(40) }), + }), + ); + await expect(find(source, { head: sha })).resolves.toBeNull(); + }); + + it.each([404, 400])('treats a merge base answered %s as no match', async (code) => { + const newerPull = pullRequest({ + source: { ...pullRequest().source, commit: { hash: newer } }, + }); + const { source } = sourceFor( + routes(newerPull, { + 'state=OPEN': () => json(page([newerPull])), + '/merge-base/': () => new Response('', { status: code }), + }), + ); + await expect(find(source, { head: sha })).resolves.toBeNull(); + }); + + it('stops comparing once the configured budget is spent', async () => { + const candidates = [1, 2, 3].map((id) => + pullRequest({ id, source: { ...pullRequest().source, commit: { hash: newer } } }), + ); + const { source, fetchFn } = sourceFor( + routes(pullRequest(), { + 'state=OPEN': () => json(page(candidates)), + '/merge-base/': () => json({ hash: 'c'.repeat(40) }), + }), + ); + await find(source, { head: sha, limits: { maxHeadComparisons: 2 } }); + const comparisons = urlsOf(fetchFn).filter((url) => url.includes('merge-base')); + expect(comparisons).toHaveLength(2); + }); + }); + + describe('forks', () => { + it('never takes a fork on its branch name alone', async () => { + const { source } = sourceFor( + routes(pullRequest({ source: fork() }), { + 'state=OPEN': () => json(listed({ source: fork() })), + }), + ); + await expect(find(source)).resolves.toBeNull(); + }); + + it('never takes a pull request whose source repository is gone', async () => { + const gone = { ...fork(), repository: null }; + const { source } = sourceFor( + routes(pullRequest({ source: gone }), { + 'state=OPEN': () => json(listed({ source: gone })), + }), + ); + await expect(find(source)).resolves.toBeNull(); + }); + + it('takes a fork that carries the recorded commit', async () => { + const { source } = sourceFor( + routes(pullRequest({ source: fork() }), { + 'state=OPEN': () => json(listed({ source: fork() })), + }), + ); + await expect(find(source, { head: sha })).resolves.toMatchObject({ number: 7 }); + }); + }); + + describe('a linked pull request', () => { + it('is read directly when its source branch is the lane branch', async () => { + const { source, fetchFn } = sourceFor(routes()); + await expect(find(source, { pullRequest: 7 })).resolves.toMatchObject({ number: 7 }); + expect(urlsOf(fetchFn).some((url) => url.includes('/pullrequests?'))).toBe(false); + }); + + it('is ignored without a source commit, and the branch is searched instead', async () => { + const gone = pullRequest({ id: 9, source: { ...pullRequest().source, commit: null } }); + const { source } = sourceFor(routes(pullRequest(), { '/pullrequests/9': () => json(gone) })); + await expect(find(source, { pullRequest: 9 })).resolves.toMatchObject({ number: 7 }); + }); + + it('is ignored for another branch, and the branch is searched instead', async () => { + const other = pullRequest({ + id: 9, + source: { ...pullRequest().source, branch: { name: 'other' } }, + }); + const { source, fetchFn } = sourceFor( + routes(pullRequest(), { '/pullrequests/9': () => json(other) }), + ); + await expect(find(source, { pullRequest: 9 })).resolves.toMatchObject({ number: 7 }); + expect(urlsOf(fetchFn).some((url) => url.includes('state=OPEN'))).toBe(true); + }); + + it('from a fork counts only with the recorded commit', async () => { + const linked = pullRequest({ source: fork() }); + const { source } = sourceFor(routes(linked, { 'state=OPEN': () => json(page([])) })); + await expect(find(source, { pullRequest: 7 })).resolves.toBeNull(); + await expect(find(source, { pullRequest: 7, head: sha })).resolves.toMatchObject({ + number: 7, + }); + }); + }); +}); diff --git a/packages/api/src/pulls/bitbucket.ts b/packages/api/src/pulls/bitbucket.ts new file mode 100644 index 00000000000..ad8743f9ec1 --- /dev/null +++ b/packages/api/src/pulls/bitbucket.ts @@ -0,0 +1,406 @@ +import type { + PullRequestChecks, + PullRequestState, + TConversationPullRequest, +} from 'librechat-data-provider'; +import type { PullRequestFetch } from './shared'; +import type { PullRequestSource } from './types'; +import { + isCount, + isRecord, + COMMIT_ID, + isPageUrl, + retryAfterMs, + isPathSegment, + MAX_TITLE_LENGTH, + DEFAULT_LOOKUP_TIMEOUT_MS, + DEFAULT_REQUEST_TIMEOUT_MS, + DEFAULT_MAX_CANDIDATE_PAGES, + DEFAULT_MAX_HEAD_COMPARISONS, + DEFAULT_MAX_CANDIDATE_PULL_REQUESTS, +} from './shared'; +import { PullRequestSourceError } from './types'; + +export const BITBUCKET_HOST = 'bitbucket.org'; +/** Every request goes under this prefix; a `next` link elsewhere is never followed. */ +const API_ROOT = 'https://api.bitbucket.org/2.0/'; +/** `workspace/repo_slug`: Bitbucket Cloud has no nested groups. */ +const REPOSITORY_PATH = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; +/** Bitbucket abbreviates commits in pull requests (12 characters today), so any length is taken. */ +const COMMIT_HASH = /^[a-f0-9]{7,64}$/; +/** The shortest prefix two commits are compared by. */ +const MIN_HASH_LENGTH = 7; +/** The most pull requests Bitbucket returns per page; a larger `pagelen` is not honored. */ +const MAX_PAGE_LENGTH = 50; +const DEFAULT_MAX_PAGES = 10; +const OPEN_STATES = ['OPEN'] as const; +const CLOSED_STATES = ['MERGED', 'DECLINED', 'SUPERSEDED'] as const; +const STATES = [...OPEN_STATES, ...CLOSED_STATES]; +const FAILING_STATUSES = new Set(['FAILED', 'STOPPED']); + +type BitbucketPullRequest = { + id: number; + title: string; + url: string; + state: PullRequestState; + draft: boolean; + /** The abbreviated head commit. */ + hash: string; + /** The branch the pull request merges from, in whichever repository it lives. */ + sourceBranch: string | null; + /** Whether the source branch lives in another repository (a fork, or one since deleted). */ + fromFork: boolean; +}; + +type ListItem = { id: number; state: string; hash: string; fromFork: boolean }; + +/** A pull request whose source commit is gone (a deleted branch, a collected commit) proves nothing. */ +type Parsed = Omit & { hash: string | null }; + +/** Two commit hashes, either possibly abbreviated, name the same commit. */ +export function isSameCommit(a: string, b: string): boolean { + const length = Math.min(a.length, b.length); + return length >= MIN_HASH_LENGTH && a.slice(0, length) === b.slice(0, length); +} + +/** + * A failed or stopped status outranks one in progress; only every status successful is passing. + * An `incomplete` read (more statuses than were read) is never reported as passing. + */ +export function summarizeStatuses( + states: readonly string[], + incomplete = false, +): PullRequestChecks { + if (states.length === 0 && !incomplete) return 'none'; + if (states.some((state) => FAILING_STATUSES.has(state))) return 'failing'; + if (incomplete || states.some((state) => state !== 'SUCCESSFUL')) return 'running'; + return 'passing'; +} + +function summarizeState(state: string): PullRequestState { + if (state === 'OPEN') return 'open'; + return state === 'MERGED' ? 'merged' : 'closed'; +} + +function parseCommon(value: unknown, repo: string) { + if ( + !isRecord(value) || + !isCount(value.id) || + typeof value.state !== 'string' || + !(STATES as readonly string[]).includes(value.state) || + !isRecord(value.source) + ) { + throw new PullRequestSourceError('UPSTREAM_ERROR'); + } + const { source } = value; + const commit = isRecord(source.commit) ? source.commit.hash : null; + if (commit != null && (typeof commit !== 'string' || !COMMIT_HASH.test(commit))) { + throw new PullRequestSourceError('UPSTREAM_ERROR'); + } + const branch = isRecord(source.branch) ? source.branch.name : null; + /** A deleted fork leaves no source repository, which is as unproven as any other fork. */ + const fullName = isRecord(source.repository) ? source.repository.full_name : null; + return { + value, + id: value.id, + state: value.state, + hash: typeof commit === 'string' ? commit : null, + sourceBranch: typeof branch === 'string' ? branch : null, + fromFork: typeof fullName !== 'string' || fullName.toLowerCase() !== repo.toLowerCase(), + }; +} + +function parsePullRequest(value: unknown, repo: string): Parsed { + const common = parseCommon(value, repo); + const links = common.value.links; + const html = isRecord(links) && isRecord(links.html) ? links.html.href : null; + if (typeof common.value.title !== 'string' || !isPageUrl(html, BITBUCKET_HOST)) { + throw new PullRequestSourceError('UPSTREAM_ERROR'); + } + return { + id: common.id, + title: common.value.title, + url: html, + state: summarizeState(common.state), + draft: common.value.draft === true, + hash: common.hash, + sourceBranch: common.sourceBranch, + fromFork: common.fromFork, + }; +} + +function parseListItem(value: unknown, repo: string): Parsed { + const { id, state, hash, fromFork } = parseCommon(value, repo); + return { id, state, hash, fromFork }; +} + +/** One page of a paginated collection: its values and the validated next page, if any. */ +function parsePage(value: unknown): { values: unknown[]; next: string | null } { + if (!isRecord(value) || !Array.isArray(value.values)) { + throw new PullRequestSourceError('UPSTREAM_ERROR'); + } + if (value.next == null) return { values: value.values, next: null }; + if (typeof value.next !== 'string') throw new PullRequestSourceError('UPSTREAM_ERROR'); + let next: URL; + try { + next = new URL(value.next); + } catch { + throw new PullRequestSourceError('UPSTREAM_ERROR'); + } + /** The token goes with the request, so a link off the API is a failure, never a fetch. */ + if (!next.href.startsWith(API_ROOT)) throw new PullRequestSourceError('UPSTREAM_ERROR'); + return { values: value.values, next: next.href }; +} + +/** The commit a status was reported for, when the status says. */ +function statusCommit(value: Record): string | null { + if (isRecord(value.commit) && typeof value.commit.hash === 'string') return value.commit.hash; + const links = value.links; + const href = isRecord(links) && isRecord(links.commit) ? links.commit.href : null; + if (typeof href !== 'string') return null; + return /\/commit\/([a-f0-9]{7,64})$/.exec(href)?.[1] ?? null; +} + +const isComplete = (item: T): item is T & { hash: string } => + item.hash != null; + +/** BBQL string literal: a backslash or a double quote inside it is escaped with a backslash. */ +const bbqlString = (value: string): string => `"${value.replace(/[\\"]/g, '\\$&')}"`; + +/** + * Reads pull requests through Bitbucket Cloud's REST API (2.0) with a token the caller supplies: + * a repository, project or workspace access token, or an OAuth access token, all sent as a bearer + * token. Only the decision (absent, rate limited, failed) leaves this module; response bodies and + * error text never do. + */ +export function createBitbucketPullRequestSource({ + fetchFn, +}: { + /** The deployment's HTTP client, so proxying and instrumentation are decided by the caller. */ + fetchFn: PullRequestFetch; +}): PullRequestSource { + type Lookup = { + token: string; + requestTimeoutMs: number; + deadline: AbortSignal; + maxCheckRunPages: number; + maxCandidatePullRequests: number; + maxHeadComparisons: number; + maxCandidatePages: number; + }; + + async function request( + url: string, + lookup: Lookup, + /** Statuses that mean "nothing there" for this request, besides 404. */ + absent: readonly number[] = [], + ): Promise { + let response: Response; + try { + response = await fetchFn(url, { + headers: { + Accept: 'application/json', + Authorization: `Bearer ${lookup.token}`, + 'User-Agent': 'LibreChat-Pull-Requests', + }, + signal: AbortSignal.any([AbortSignal.timeout(lookup.requestTimeoutMs), lookup.deadline]), + }); + } catch { + throw new PullRequestSourceError('UPSTREAM_ERROR'); + } + if (response.ok) { + try { + return await response.json(); + } catch { + throw new PullRequestSourceError('UPSTREAM_ERROR'); + } + } + /** Bitbucket sends no reset time with a rate limit, at most a `retry-after`. */ + if (response.status === 429) { + throw new PullRequestSourceError('RATE_LIMITED', retryAfterMs(response, 'retry-after')); + } + /** A repository the token cannot see is indistinguishable from one without pull requests. */ + if (response.status === 404 || absent.includes(response.status)) return null; + throw new PullRequestSourceError('UPSTREAM_ERROR'); + } + + /** + * Reads every page of a collection up to a bound, so a failure on a later page still shows. + * The pull request was read a moment ago, so a 404 here is one that vanished, not an empty list. + */ + async function readPages( + url: string, + lookup: Lookup, + ): Promise<{ values: unknown[]; incomplete: boolean }> { + const values: unknown[] = []; + let next: string | null = url; + for (let page = 0; page < lookup.maxCheckRunPages && next != null; page++) { + const body = await request(next, lookup); + if (body == null) throw new PullRequestSourceError('UPSTREAM_ERROR'); + const parsed = parsePage(body); + values.push(...parsed.values); + next = parsed.next; + } + return { values, incomplete: next != null }; + } + + return { + async find({ repo, branch, head: recordedHead, pullRequest, token, limits }) { + /** As on GitHub: a lane with a branch and no commit yet has nothing to match against. */ + if (recordedHead === null) return null; + if (!REPOSITORY_PATH.test(repo) || !repo.split('/').every(isPathSegment)) return null; + if (branch.length === 0) return null; + const lookup: Lookup = { + token, + requestTimeoutMs: limits?.requestTimeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS, + deadline: AbortSignal.timeout(limits?.lookupTimeoutMs ?? DEFAULT_LOOKUP_TIMEOUT_MS), + maxCheckRunPages: limits?.maxCheckRunPages ?? DEFAULT_MAX_PAGES, + maxCandidatePullRequests: + limits?.maxCandidatePullRequests ?? DEFAULT_MAX_CANDIDATE_PULL_REQUESTS, + maxHeadComparisons: limits?.maxHeadComparisons ?? DEFAULT_MAX_HEAD_COMPARISONS, + maxCandidatePages: limits?.maxCandidatePages ?? DEFAULT_MAX_CANDIDATE_PAGES, + }; + const [workspace, slug] = repo.split('/'); + const base = `${API_ROOT}repositories/${encodeURIComponent(workspace)}/${encodeURIComponent(slug)}`; + const getJson = (pathname: string, absent: readonly number[] = []) => + request(`${base}${pathname}`, lookup, absent); + + const recorded = + typeof recordedHead === 'string' && COMMIT_ID.test(recordedHead) ? recordedHead : undefined; + let comparisons = 0; + /** + * With a recorded commit a pull request counts only when its head is that commit or builds + * on it: their merge base is the recorded commit. Every comparison spends the configured + * budget, and a commit Bitbucket does not know is no match. + */ + const contains = async (hash: string): Promise => { + if (recorded == null || recorded.startsWith(hash)) return true; + if (comparisons >= lookup.maxHeadComparisons) return false; + comparisons += 1; + const merged = await getJson(`/merge-base/${recorded}..${hash}`, [400]); + if (merged == null) return false; + if (!isRecord(merged) || typeof merged.hash !== 'string') { + throw new PullRequestSourceError('UPSTREAM_ERROR'); + } + return isSameCommit(merged.hash, recorded); + }; + /** + * A branch name proves nothing for a fork (anyone can open `patch-1`), so a fork counts only + * when it carries the recorded commit. + */ + const matches = async (item: { hash: string; fromFork: boolean }): Promise => + item.fromFork ? recorded != null && contains(item.hash) : contains(item.hash); + + const report = async (pull: BitbucketPullRequest): Promise => { + const path = `/pullrequests/${pull.id}`; + const [diffstat, statuses] = await Promise.all([ + readPages(`${base}${path}/diffstat`, lookup), + readPages(`${base}${path}/statuses`, lookup), + ]); + /** + * Past the page bound the counts cover only the files read. They are still reported: a + * pull request that large is better shown with a lower bound than with no size at all. + */ + let additions = 0; + let deletions = 0; + for (const file of diffstat.values) { + if (!isRecord(file)) throw new PullRequestSourceError('UPSTREAM_ERROR'); + const added = file.lines_added ?? 0; + const removed = file.lines_removed ?? 0; + if (!isCount(added) || !isCount(removed)) { + throw new PullRequestSourceError('UPSTREAM_ERROR'); + } + additions += added; + deletions += removed; + } + const states: string[] = []; + for (const status of statuses.values) { + if (!isRecord(status) || typeof status.state !== 'string') { + throw new PullRequestSourceError('UPSTREAM_ERROR'); + } + /** A status of an earlier head says nothing about this one. */ + const commit = statusCommit(status); + if (commit != null && !isSameCommit(commit, pull.hash)) continue; + states.push(status.state); + } + return { + number: pull.id, + title: pull.title.slice(0, MAX_TITLE_LENGTH), + url: pull.url, + provider: 'bitbucket', + additions, + deletions, + state: pull.state, + isDraft: pull.draft, + /** Bitbucket's API reports no conflict state for a pull request, only a diff. */ + mergeable: 'unknown', + checks: summarizeStatuses(states, statuses.incomplete), + }; + }; + + /** A pull request the conversation's own output linked to is read directly. */ + if (pullRequest != null && Number.isSafeInteger(pullRequest) && pullRequest > 0) { + const linked = await getJson(`/pullrequests/${pullRequest}`); + if (linked != null) { + const pull = parsePullRequest(linked, repo); + if (isComplete(pull) && pull.sourceBranch === branch && (await matches(pull))) { + return report(pull); + } + } + } + + /** + * A full page even without a recorded commit: the newest closed one may be a fork, which + * cannot match, and must not hide an older one from the repository itself. Later pages come + * from the `next` link Bitbucket returns, which is opaque, and only its absence ends the + * list. `undefined`: the repository is not visible. An empty list: it is, and nothing matched. + */ + const listAll = async (closed: boolean): Promise => { + const states = closed ? CLOSED_STATES : OPEN_STATES; + const pageLength = Math.min(Math.max(lookup.maxCandidatePullRequests, 1), MAX_PAGE_LENGTH); + const filter = states.map((state) => `state = "${state}"`).join(' OR '); + const query = new URLSearchParams({ + q: `source.branch.name = ${bbqlString(branch)} AND (${filter})`, + sort: '-created_on', + pagelen: String(pageLength), + }); + /** The default list is open pull requests only. The `state` parameters lift that default + * and `q` names the same states, so either filter alone lists the same pull requests. */ + for (const state of states) query.append('state', state); + const all: ListItem[] = []; + let next: string | null = `${base}/pullrequests?${query.toString()}`; + for (let page = 1; page <= lookup.maxCandidatePages && next != null; page++) { + const listed = await request(next, lookup, []); + if (listed == null) return page === 1 ? undefined : all; + const parsed = parsePage(listed); + all.push(...parsed.values.map((item) => parseListItem(item, repo)).filter(isComplete)); + next = parsed.next; + } + return all; + }; + const choose = async (candidates: ListItem[]): Promise => { + for (const candidate of candidates) { + if (await matches(candidate)) return candidate; + } + return null; + }; + + /** Open first on its own, so closed history on a reused branch name cannot hide it. */ + const open = await listAll(false); + if (open === undefined) return null; + const chosen = (await choose(open)) ?? (await choose((await listAll(true)) ?? [])); + if (chosen == null) return null; + + const detail = await getJson(`/pullrequests/${chosen.id}`); + if (detail == null) return null; + const pull = parsePullRequest(detail, repo); + /** The pull request may have changed between the list and the detail: its source branch, + * the repository it comes from, or a push that moved the head off the recorded commit. */ + if (!isComplete(pull) || pull.sourceBranch !== branch || pull.fromFork !== chosen.fromFork) { + return null; + } + if (!isSameCommit(pull.hash, chosen.hash) && !(await matches(pull))) return null; + return report(pull); + }, + }; +} diff --git a/packages/api/src/pulls/gitlab.spec.ts b/packages/api/src/pulls/gitlab.spec.ts index 347425a7681..178e698a14c 100644 --- a/packages/api/src/pulls/gitlab.spec.ts +++ b/packages/api/src/pulls/gitlab.spec.ts @@ -29,6 +29,8 @@ function sourceFor(routes: Record Response>) { const fetchFn = jest.fn(async (input: string, _init?: RequestInit) => { const url = String(input); const key = Object.keys(routes).find((fragment) => url.includes(fragment)); + /** Nothing is being merged unless a test says so. */ + if (key == null && url.includes('state=locked')) return json([]); if (key == null) throw new Error(`unexpected ${url}`); return routes[key](); }); @@ -37,7 +39,8 @@ function sourceFor(routes: Record Response>) { const routes = (detail = mergeRequest(), overrides: Record Response> = {}) => ({ 'state=opened': () => json(listed()), - 'state=all': () => json([]), + 'state=merged': () => json([]), + 'state=closed': () => json([]), '/merge_requests/7': () => json(detail), '/api/graphql': () => json(stats()), ...overrides, @@ -118,7 +121,8 @@ describe('createGitLabPullRequestSource', () => { ])('reports a %s merge request as %s, with no conflict state', async (state, expected) => { const { source } = sourceFor({ 'state=opened': () => json([]), - 'state=all': () => json(listed({ state })), + 'state=merged': () => json([]), + 'state=closed': () => json(listed({ state })), '/merge_requests/7': () => json(mergeRequest({ state, has_conflicts: true })), '/api/graphql': () => json(stats()), }); @@ -237,22 +241,70 @@ describe('createGitLabPullRequestSource', () => { }); }); - it('finds a closed merge request behind a newer open fork, without a recorded commit', async () => { - const fork = { iid: 9, state: 'opened', sha, source_project_id: 2, target_project_id: 1 }; + it('finds a merged merge request behind more open forks than one page holds', async () => { + const forks = Array.from({ length: 10 }, (_, index) => ({ + iid: 20 + index, + state: 'opened', + sha, + source_project_id: 2, + target_project_id: 1, + })); const merged = { iid: 7, state: 'merged', sha, source_project_id: 1, target_project_id: 1 }; const { source, fetchFn } = sourceFor({ - 'state=opened': () => json([]), - 'state=all': () => json([fork, merged]), + 'state=opened': () => json(forks), + 'state=merged': () => json([merged]), + 'state=closed': () => json([]), '/merge_requests/7': () => json(mergeRequest({ state: 'merged' })), '/api/graphql': () => json(stats()), }); await expect(find(source)).resolves.toMatchObject({ number: 7, state: 'merged' }); const history = fetchFn.mock.calls .map(([url]) => String(url)) - .find((url) => url.includes('state=all')); + .find((url) => url.includes('state=merged')); expect(new URL(history ?? '').searchParams.get('per_page')).toBe('10'); }); + it('finds a merge request while GitLab is merging it, ahead of older history', async () => { + const item = (iid: number, state: string) => ({ + iid, + state, + sha, + source_project_id: 1, + target_project_id: 1, + }); + const { source, fetchFn } = sourceFor({ + 'state=opened': () => json([]), + 'state=locked': () => json([item(9, 'locked')]), + 'state=merged': () => json([item(3, 'merged')]), + 'state=closed': () => json([]), + '/merge_requests/9': () => json(mergeRequest({ iid: 9, state: 'locked' })), + '/api/graphql': () => json(stats()), + }); + await expect(find(source)).resolves.toMatchObject({ number: 9 }); + const locked = fetchFn.mock.calls + .map(([url]) => new URL(String(url))) + .find((url) => url.searchParams.get('state') === 'locked'); + expect(locked?.searchParams.get('source_branch')).toBe('feat/x'); + }); + + it('takes the newest of the merged and closed merge requests', async () => { + const item = (iid: number, state: string) => ({ + iid, + state, + sha, + source_project_id: 1, + target_project_id: 1, + }); + const { source } = sourceFor({ + 'state=opened': () => json([]), + 'state=merged': () => json([item(3, 'merged')]), + 'state=closed': () => json([item(7, 'closed')]), + '/merge_requests/7': () => json(mergeRequest({ state: 'closed' })), + '/api/graphql': () => json(stats()), + }); + await expect(find(source)).resolves.toMatchObject({ number: 7, state: 'closed' }); + }); + it('drops a merge request whose source branch changed after it was listed', async () => { const { source } = sourceFor(routes(mergeRequest({ source_branch: 'other' }))); await expect(find(source)).resolves.toBeNull(); diff --git a/packages/api/src/pulls/gitlab.ts b/packages/api/src/pulls/gitlab.ts index 402a1d84cc1..9f552628942 100644 --- a/packages/api/src/pulls/gitlab.ts +++ b/packages/api/src/pulls/gitlab.ts @@ -47,6 +47,7 @@ type GitLabMergeRequest = { }; type ListItem = { iid: number; state: string; sha: string; fromFork: boolean }; +type ListState = 'opened' | 'locked' | 'merged' | 'closed'; /** A failed or canceled pipeline outranks one still running; anything unfinished is running. */ export function summarizePipeline(status: string | null): PullRequestChecks { @@ -297,7 +298,7 @@ export function createGitLabPullRequestSource({ } /** `undefined`: the project is not visible. An empty list: it is, and nothing matched. */ - const list = async (state: 'opened' | 'all', perPage: number, page: number) => { + const list = async (state: ListState, perPage: number, page: number) => { const query = new URLSearchParams({ state, source_branch: branch, @@ -312,11 +313,11 @@ export function createGitLabPullRequestSource({ return listed.map(parseListItem); }; /** - * Reads pages of one state up to the configured budget. The history is read a full page at - * a time even without a recorded commit: it also lists open merge requests from forks, which - * are skipped, so its newest entry is not necessarily the one that matches. + * Reads pages of one state up to the configured budget, a full page at a time even without + * a recorded commit: a fork's merge request never matches without one, so the newest entry + * is not necessarily the one that does. */ - const listAll = async (state: 'opened' | 'all'): Promise => { + const listAll = async (state: ListState): Promise => { const perPage = lookup.maxCandidatePullRequests; const pages = lookup.maxCandidatePages; const all: ListItem[] = []; @@ -335,12 +336,22 @@ export function createGitLabPullRequestSource({ return null; }; - /** Open first on its own, so closed history on a reused branch name cannot hide it. */ - const open = await listAll('opened'); - if (open === undefined) return null; - const chosen = - (await choose(open)) ?? - (await choose(((await listAll('all')) ?? []).filter((item) => item.state !== 'opened'))); + /** A project's `iid` grows with every merge request it creates, so it orders by age. */ + const newestFirst = (...lists: (ListItem[] | undefined)[]): ListItem[] => + lists.flatMap((items) => items ?? []).sort((a, b) => b.iid - a.iid); + /** Open first on its own, so closed history on a reused branch name cannot hide it. GitLab + * lists a merge request that is being merged as `locked`, under no other state filter, and + * it is still the branch's live merge request, so it is read with the open ones. */ + const [opened, locked] = await Promise.all([listAll('opened'), listAll('locked')]); + if (opened === undefined) return null; + const open = newestFirst(opened, locked); + /** Merged and closed are listed apart from open ones, so any number of open fork merge + * requests on the branch name cannot crowd the history out of its page budget. */ + const history = async (): Promise => { + const [merged, closed] = await Promise.all([listAll('merged'), listAll('closed')]); + return newestFirst(merged, closed); + }; + const chosen = (await choose(open)) ?? (await choose(await history())); if (chosen == null) return null; const detail = await getJson(`/merge_requests/${chosen.iid}`); diff --git a/packages/api/src/pulls/handler.spec.ts b/packages/api/src/pulls/handler.spec.ts index 9f3602220d7..80c28de9534 100644 --- a/packages/api/src/pulls/handler.spec.ts +++ b/packages/api/src/pulls/handler.spec.ts @@ -1083,6 +1083,70 @@ describe('createConversationPullRequestsHandler', () => { }); }); + it('does not join a lookup started for another host', async () => { + const hosts: Array = []; + const lookup = jest.fn( + (input: { host?: string }) => + new Promise(() => { + hosts.push(input.host); + }), + ); + const settings = { ...enabled, batchTimeoutSeconds: 1 }; + const getAppConfig = jest + .fn() + .mockResolvedValueOnce(configWith(settings)) + .mockResolvedValueOnce(configWith(settings)) + .mockResolvedValue(configWith({ ...settings, host: 'ghe.example.com' })); + const handler = createConversationPullRequestsHandler({ + getConvosLaneGit: jest.fn(async () => [lane('a', 'o/r', 'feat/shared')]), + getAppConfig, + lookup: lookup as never, + env: { GH_TOKEN: 'ghp_secret' }, + }); + const call = () => + handler( + { user: { id: 'u1' }, body: { conversationIds: ['a'] } } as unknown as ServerRequest, + { status: jest.fn().mockReturnThis(), json: jest.fn() } as unknown as Response, + ); + void call(); + await flush(); + void call(); + await flush(); + expect(hosts).toEqual(['github.com', 'ghe.example.com']); + }); + + it('does not join a lookup started for another provider on the same host', async () => { + const providers: Array = []; + const lookup = jest.fn( + (input: { provider?: string }) => + new Promise(() => { + providers.push(input.provider); + }), + ); + const settings = { ...enabled, host: 'git.example.com', batchTimeoutSeconds: 1 }; + const getAppConfig = jest + .fn() + .mockResolvedValueOnce(configWith(settings)) + .mockResolvedValueOnce(configWith(settings)) + .mockResolvedValue(configWith({ ...settings, provider: 'gitlab' as const })); + const handler = createConversationPullRequestsHandler({ + getConvosLaneGit: jest.fn(async () => [lane('a', 'o/r', 'feat/shared')]), + getAppConfig, + lookup: lookup as never, + env: { GH_TOKEN: 'ghp_secret' }, + }); + const call = () => + handler( + { user: { id: 'u1' }, body: { conversationIds: ['a'] } } as unknown as ServerRequest, + { status: jest.fn().mockReturnThis(), json: jest.fn() } as unknown as Response, + ); + void call(); + await flush(); + void call(); + await flush(); + expect(providers).toEqual(['github', 'gitlab']); + }); + it('gives every conversation on a shared branch the same failure when that lookup fails', async () => { const lookup = jest .fn() diff --git a/packages/api/src/pulls/handler.ts b/packages/api/src/pulls/handler.ts index 84fa4ee8372..4f7a190dc26 100644 --- a/packages/api/src/pulls/handler.ts +++ b/packages/api/src/pulls/handler.ts @@ -467,6 +467,8 @@ export function createConversationPullRequestsHandler(deps: { const input = lookupInput(settings, token, lane); /** The same lane under the same policy is one lookup for every request that wants it. */ const shareKey = `${scope}\0${JSON.stringify([ + input.provider, + input.host, input.repo, input.branch, input.head, diff --git a/packages/api/src/pulls/index.ts b/packages/api/src/pulls/index.ts index 122c4d868dd..fe3776e1daa 100644 --- a/packages/api/src/pulls/index.ts +++ b/packages/api/src/pulls/index.ts @@ -2,6 +2,7 @@ export * from './types'; export * from './links'; export * from './github'; export * from './gitlab'; +export * from './bitbucket'; export * from './sources'; export * from './lookup'; export * from './handler'; diff --git a/packages/api/src/pulls/links.spec.ts b/packages/api/src/pulls/links.spec.ts index 93f9e0db7f3..3798e208dc1 100644 --- a/packages/api/src/pulls/links.spec.ts +++ b/packages/api/src/pulls/links.spec.ts @@ -16,25 +16,50 @@ describe('findPullRequestLink', () => { expect(findPullRequestLink(output, 'o/r')).toBe(8); }); - it('finds a link on a GitHub Enterprise host, which the lookup then checks', () => { - expect(findPullRequestLink('https://ghe.example.com/o/r/pull/9', 'o/r')).toBe(9); - expect(findPullRequestLink('https://ghe.local:8443/o/r/pull/9', 'o/r')).toBe(9); + it('finds a link only on the configured host', () => { + expect(findPullRequestLink('https://ghe.example.com/o/r/pull/9', 'o/r')).toBeNull(); + expect( + findPullRequestLink('https://ghe.example.com/o/r/pull/9', 'o/r', 'ghe.example.com'), + ).toBe(9); + expect(findPullRequestLink('https://GHE.local:8443/o/r/pull/9', 'o/r', 'ghe.local:8443')).toBe( + 9, + ); + expect(findPullRequestLink('https://ghe.local:443/o/r/pull/9', 'o/r', 'ghe.local')).toBe(9); + expect(findPullRequestLink('https://ghe.local:08443/o/r/pull/9', 'o/r', 'ghe.local:8443')).toBe( + 9, + ); + expect( + findPullRequestLink('https://github.com/o/r/pull/9', 'o/r', 'ghe.example.com'), + ).toBeNull(); + }); + + it('keeps the last link on the configured host when another installation is linked after it', () => { + const output = 'https://ghe.example.com/o/r/pull/4 and https://github.com/o/r/pull/7'; + expect(findPullRequestLink(output, 'o/r', 'ghe.example.com')).toBe(4); }); it('finds a GitLab merge request, in a nested group too', () => { const output = 'View it on GitLab: https://gitlab.com/g/r/-/merge_requests/14\n'; - expect(findPullRequestLink(output, 'g/r')).toBe(14); + expect(findPullRequestLink(output, 'g/r', 'gitlab.com')).toBe(14); expect( - findPullRequestLink('https://gitlab.example.com/g/sub/r/-/merge_requests/3', 'g/sub/r'), + findPullRequestLink( + 'https://gitlab.example.com/g/sub/r/-/merge_requests/3', + 'g/sub/r', + 'gitlab.example.com', + ), ).toBe(3); }); it('does not take a merge request of a project nested under the lane one', () => { - expect(findPullRequestLink('https://gitlab.com/g/r/x/-/merge_requests/3', 'g/r')).toBeNull(); + expect( + findPullRequestLink('https://gitlab.com/g/r/x/-/merge_requests/3', 'g/r', 'gitlab.com'), + ).toBeNull(); }); it('finds a Bitbucket pull request', () => { - expect(findPullRequestLink('https://bitbucket.org/w/r/pull-requests/21', 'w/r')).toBe(21); + expect( + findPullRequestLink('https://bitbucket.org/w/r/pull-requests/21', 'w/r', 'bitbucket.org'), + ).toBe(21); }); it('accepts a link to a page of the pull request', () => { @@ -58,6 +83,7 @@ describe('findPullRequestLink', () => { ['a bare number', 'opened #4'], ])('ignores %s', (_, output) => { expect(findPullRequestLink(output, 'o/r')).toBeNull(); + expect(findPullRequestLink(output, 'o/r', 'gitlab.com')).toBeNull(); }); it('ignores a number too large to be a pull request', () => { diff --git a/packages/api/src/pulls/links.ts b/packages/api/src/pulls/links.ts index de218899668..0fb084003ef 100644 --- a/packages/api/src/pulls/links.ts +++ b/packages/api/src/pulls/links.ts @@ -1,25 +1,35 @@ +import { resolvePullRequestHost } from './settings'; + +const DEFAULT_HOST = 'github.com'; + /** * A pull request page on any provider, ending where the number does: GitHub's * `/{owner}/{name}/pull/{n}`, GitLab's `/{group}/.../{name}/-/merge_requests/{n}` and Bitbucket's * `/{workspace}/{name}/pull-requests/{n}`. A `/pull/new/` page (what `git push` suggests) has no - * number and never matches. Any host matches: the number is only a candidate, which the lookup - * reads from the configured provider and accepts only when its head is the lane's branch. + * number and never matches. */ const PULL_REQUEST_URL = - /https:\/\/[A-Za-z0-9.-]+(?::\d{1,5})?\/((?:[A-Za-z0-9_.-]+\/)+?[A-Za-z0-9_.-]+)\/(?:pull|-\/merge_requests|pull-requests)\/(\d+)(?![\w-])/g; + /https:\/\/([A-Za-z0-9.-]+(?::\d{1,5})?)\/((?:[A-Za-z0-9_.-]+\/)+?[A-Za-z0-9_.-]+)\/(?:pull|-\/merge_requests|pull-requests)\/(\d+)(?![\w-])/g; const MARKERS = ['/pull/', '/merge_requests/', '/pull-requests/']; /** * The pull request of `repo` that command output links to, the last one when it links several, * since tools such as `gh pr create` print the link they made last. Links to any other repository - * are ignored, so output that mentions another project's pull request cannot claim the lane. + * or to any host but `host` (the configured one, as `resolvePullRequestHost` returns it) are + * ignored, so output that mentions another project's or another installation's pull request + * cannot claim the lane. */ -export function findPullRequestLink(output: string, repo: string): number | null { +export function findPullRequestLink( + output: string, + repo: string, + host: string = DEFAULT_HOST, +): number | null { if (!MARKERS.some((marker) => output.includes(marker))) return null; const wanted = repo.toLowerCase(); let found: number | null = null; for (const match of output.matchAll(PULL_REQUEST_URL)) { - const [, path, digits] = match; + const [, linkHost, path, digits] = match; + if (resolvePullRequestHost(linkHost) !== host) continue; if (path.toLowerCase() !== wanted) continue; const number = Number(digits); if (Number.isSafeInteger(number) && number > 0) found = number; diff --git a/packages/api/src/pulls/lookup.spec.ts b/packages/api/src/pulls/lookup.spec.ts index 8fc89025380..76bd6e409c5 100644 --- a/packages/api/src/pulls/lookup.spec.ts +++ b/packages/api/src/pulls/lookup.spec.ts @@ -192,6 +192,18 @@ describe('rate limit cooldown', () => { }); }); + it('does not block the same credential on another host', async () => { + const find = jest.fn().mockRejectedValueOnce(limited(60_000)).mockResolvedValue(value); + const lookup = createPullRequestLookup({ source: { find } }); + await lookup(input); + await expect(lookup({ ...input, host: 'ghe.example.com' })).resolves.toEqual({ + ok: true, + value, + }); + await expect(lookup({ ...input, provider: 'gitlab' })).resolves.toEqual({ ok: true, value }); + await expect(lookup({ ...input, branch: 'other' })).resolves.toEqual(rateLimited); + }); + it('waits at least a short floor when GitHub gives no retry time', async () => { let clock = 0; const find = jest.fn().mockRejectedValueOnce(limited()).mockResolvedValue(value); diff --git a/packages/api/src/pulls/lookup.ts b/packages/api/src/pulls/lookup.ts index 3bf197a3081..8e98f5622d8 100644 --- a/packages/api/src/pulls/lookup.ts +++ b/packages/api/src/pulls/lookup.ts @@ -141,10 +141,12 @@ export function createPullRequestLookup({ if (cached != null && cached.expiresAt > now()) return cached.result; const pending = inflight.get(key); if (pending != null) return pending; - const until = cooldowns.get(scope); + /** A rate limit belongs to the credential on one provider host; elsewhere it is not limited. */ + const limitScope = `${scope}\0${provider ?? ''}\0${host ?? ''}`; + const until = cooldowns.get(limitScope); if (until != null) { if (until > now()) return rateLimited; - cooldowns.delete(scope); + cooldowns.delete(limitScope); } const run = (async (): Promise => { @@ -170,7 +172,7 @@ export function createPullRequestLookup({ } const code = error instanceof PullRequestSourceError ? error.code : 'UPSTREAM_ERROR'; if (error instanceof PullRequestSourceError && error.code === 'RATE_LIMITED') { - startCooldown(scope, error.retryAfterMs); + startCooldown(limitScope, error.retryAfterMs); } result = { ok: false, error: { code } }; } diff --git a/packages/api/src/pulls/settings.spec.ts b/packages/api/src/pulls/settings.spec.ts index 7f9b9cc94f6..2104436adf8 100644 --- a/packages/api/src/pulls/settings.spec.ts +++ b/packages/api/src/pulls/settings.spec.ts @@ -3,6 +3,7 @@ import { resolvePullRequestHost, resolvePullRequestToken, isPullRequestFeatureActive, + resolvePullRequestRecording, resolvePullRequestProvider, } from './settings'; @@ -174,3 +175,31 @@ describe('isPullRequestFeatureActive', () => { ); }); }); + +describe('resolvePullRequestRecording', () => { + const env = { GITHUB_TOKEN: 't' }; + it('records with the configured host when the feature is on', () => { + expect( + resolvePullRequestRecording({ host: 'GHE.example.com:443', allowAllRepositories: true }, env), + ).toEqual({ enabled: true, host: 'ghe.example.com' }); + expect(resolvePullRequestRecording({ allowAllRepositories: true }, env)).toEqual({ + enabled: true, + host: 'github.com', + }); + }); + + it('records links on the default host of the configured provider', () => { + expect( + resolvePullRequestRecording( + { provider: 'gitlab', allowAllRepositories: true }, + { GITLAB_TOKEN: 't' }, + ), + ).toEqual({ enabled: true, host: 'gitlab.com' }); + }); + + it('records nothing when the feature is off', () => { + expect(resolvePullRequestRecording({ allowAllRepositories: true }, {})).toEqual({ + enabled: false, + }); + }); +}); diff --git a/packages/api/src/pulls/settings.ts b/packages/api/src/pulls/settings.ts index 0f3e6c5e035..c1409fd63c4 100644 --- a/packages/api/src/pulls/settings.ts +++ b/packages/api/src/pulls/settings.ts @@ -98,3 +98,18 @@ export function isPullRequestFeatureActive( settings?.allowAllRepositories === true || (settings?.allowedRepositories?.length ?? 0) > 0 ); } + +/** + * What a lane recorder needs from the settings: whether to record at all, and the host whose pull + * request links it keeps. + */ +export function resolvePullRequestRecording( + settings: Partial | null | undefined, + env: Env, +): { enabled: boolean; host?: string } { + const enabled = isPullRequestFeatureActive(settings, env); + const provider = resolvePullRequestProvider(settings?.provider); + const host = + enabled && provider != null ? resolvePullRequestHost(settings?.host, provider) : null; + return host != null ? { enabled, host } : { enabled }; +} diff --git a/packages/api/src/pulls/sources.spec.ts b/packages/api/src/pulls/sources.spec.ts index 652eb5e2abb..21c138ec969 100644 --- a/packages/api/src/pulls/sources.spec.ts +++ b/packages/api/src/pulls/sources.spec.ts @@ -1,12 +1,13 @@ import type { PullRequestSource } from './types'; -import { routePullRequestSources } from './sources'; +import { createPullRequestSource, routePullRequestSources } from './sources'; const input = { repo: 'o/r', branch: 'feat/x', token: 't' }; describe('routePullRequestSources', () => { const github: PullRequestSource = { find: jest.fn().mockResolvedValue(null) }; const gitlab: PullRequestSource = { find: jest.fn().mockResolvedValue(null) }; - const source = routePullRequestSources({ github, gitlab }); + const bitbucket: PullRequestSource = { find: jest.fn().mockResolvedValue(null) }; + const source = routePullRequestSources({ github, gitlab, bitbucket }); beforeEach(() => jest.clearAllMocks()); @@ -20,11 +21,28 @@ describe('routePullRequestSources', () => { await source.find({ ...input, provider: 'gitlab' }); expect(gitlab.find).toHaveBeenCalledTimes(1); expect(github.find).not.toHaveBeenCalled(); + await source.find({ ...input, provider: 'bitbucket' }); + expect(bitbucket.find).toHaveBeenCalledTimes(1); + expect(gitlab.find).toHaveBeenCalledTimes(1); }); it('reports a provider without a source as not configured', async () => { - await expect(source.find({ ...input, provider: 'bitbucket' })).rejects.toMatchObject({ + const githubOnly = routePullRequestSources({ github }); + await expect(githubOnly.find({ ...input, provider: 'gitlab' })).rejects.toMatchObject({ code: 'NOT_CONFIGURED', }); }); }); + +describe('createPullRequestSource', () => { + it('routes Bitbucket to Bitbucket Cloud', async () => { + const fetchFn = jest.fn(async (_input: string, _init?: RequestInit) => + Response.json({ values: [] }), + ); + const source = createPullRequestSource({ fetchFn }); + await expect(source.find({ ...input, provider: 'bitbucket' })).resolves.toBeNull(); + expect(String(fetchFn.mock.calls[0][0])).toMatch( + /^https:\/\/api\.bitbucket\.org\/2\.0\/repositories\/o\/r\/pullrequests\?/, + ); + }); +}); diff --git a/packages/api/src/pulls/sources.ts b/packages/api/src/pulls/sources.ts index 1eb588d9c79..4dfe8eb2b05 100644 --- a/packages/api/src/pulls/sources.ts +++ b/packages/api/src/pulls/sources.ts @@ -1,6 +1,7 @@ import type { PullRequestProvider } from 'librechat-data-provider'; import type { PullRequestFetch } from './shared'; import type { PullRequestSource } from './types'; +import { createBitbucketPullRequestSource } from './bitbucket'; import { createGitHubPullRequestSource } from './github'; import { createGitLabPullRequestSource } from './gitlab'; import { PullRequestSourceError } from './types'; @@ -31,5 +32,6 @@ export function createPullRequestSource({ return routePullRequestSources({ github: createGitHubPullRequestSource({ fetchFn }), gitlab: createGitLabPullRequestSource({ fetchFn }), + bitbucket: createBitbucketPullRequestSource({ fetchFn }), }); } diff --git a/packages/data-provider/src/code/environment.spec.ts b/packages/data-provider/src/code/environment.spec.ts new file mode 100644 index 00000000000..420095cc12a --- /dev/null +++ b/packages/data-provider/src/code/environment.spec.ts @@ -0,0 +1,13 @@ +import { isCodeWorkspaceEnvironment } from './workspace'; + +const environment = (repo: string) => ({ fingerprint: 'a'.repeat(64), repo, actions: [] }); + +describe('isCodeWorkspaceEnvironment repository', () => { + it.each(['o/r', 'group/sub/project', 'a.b/c_d/e-f'])('accepts %s', (repo) => { + expect(isCodeWorkspaceEnvironment(environment(repo))).toBe(true); + }); + + it.each(['r', 'group/../project', './r', 'o/..', 'o//r'])('rejects %s', (repo) => { + expect(isCodeWorkspaceEnvironment(environment(repo))).toBe(false); + }); +}); diff --git a/packages/data-provider/src/code/workspace.ts b/packages/data-provider/src/code/workspace.ts index 586f1b46d6f..6b7da4bbce8 100644 --- a/packages/data-provider/src/code/workspace.ts +++ b/packages/data-provider/src/code/workspace.ts @@ -133,7 +133,8 @@ export function isCodeWorkspaceEnvironment( (environment.repo === undefined || (typeof environment.repo === 'string' && environment.repo.length <= 256 && - /^[A-Za-z0-9_.-]+(?:\/[A-Za-z0-9_.-]+){1,19}$/.test(environment.repo))) && + /^[A-Za-z0-9_.-]+(?:\/[A-Za-z0-9_.-]+){1,19}$/.test(environment.repo) && + environment.repo.split('/').every((segment) => segment !== '.' && segment !== '..'))) && (environment.ref === undefined || (typeof environment.ref === 'string' && environment.ref.trim().length > 0 && diff --git a/packages/data-provider/src/config.ts b/packages/data-provider/src/config.ts index 5ed9928616f..ecb9f888d02 100644 --- a/packages/data-provider/src/config.ts +++ b/packages/data-provider/src/config.ts @@ -112,6 +112,13 @@ export const BASE_ONLY_CONFIG_SECTIONS = ['filters', 'mcpAppSandbox'] as const; /** Sections that may be stored in the tenant's base config document but must * not be overridden or tombstoned by role, group, or user config documents. */ export const BASE_PRINCIPAL_CONFIG_SECTIONS = ['langfuse'] as const; +/** Fields inside overridable sections that only the operator's YAML sets: a role, group, user + * or tenant config document can neither change nor remove them. The pull request provider and + * host decide where the deployment's token is sent. */ +export const BASE_ONLY_CONFIG_PATHS = [ + 'endpoints.agents.pullRequests.provider', + 'endpoints.agents.pullRequests.host', +] as const; export const defaultRetrievalModels = [ 'gpt-4o', diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index f1ed8a393c0..504bee0c095 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -141,6 +141,43 @@ describe('mergeConfigOverrides', () => { expect(mergeConfigOverrides(base, configs).mcpAppSandbox).toEqual(base.mcpAppSandbox); }); + it('keeps the pull request host from the base config whatever an override does', () => { + const pullRequests = { host: 'ghe.example.com', allowedRepositories: ['acme/*'] }; + const base = { endpoints: { agents: { pullRequests } } } as unknown as AppConfig; + const read = (configs: IConfig[]) => + ( + mergeConfigOverrides(base, configs) as unknown as { + endpoints: { agents: { pullRequests: Record } }; + } + ).endpoints.agents.pullRequests; + + const changed = read([ + fakeConfig( + { + endpoints: { + agents: { pullRequests: { host: 'evil.example.com', cacheTtlSeconds: 60 } }, + }, + }, + 10, + ), + ]); + expect(changed).toMatchObject({ host: 'ghe.example.com', cacheTtlSeconds: 60 }); + + const provider = read([ + fakeConfig({ endpoints: { agents: { pullRequests: { provider: 'gitlab' } } } }, 10), + ]); + expect(provider.provider).toBeUndefined(); + + const removed = read([fakeConfig({}, 10, ['endpoints.agents.pullRequests.host'])]); + expect(removed.host).toBe('ghe.example.com'); + + const unset = { endpoints: { agents: { pullRequests: {} } } } as unknown as AppConfig; + const added = mergeConfigOverrides(unset, [ + fakeConfig({ endpoints: { agents: { pullRequests: { host: 'evil.example.com' } } } }, 10), + ]) as unknown as { endpoints: { agents: { pullRequests: Record } } }; + expect(added.endpoints.agents.pullRequests.host).toBeUndefined(); + }); + it('applies tenant-wide Langfuse settings only from the base principal', () => { const configs = [ fakeConfig( diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index c15a7b247ce..49b22269922 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -1,6 +1,7 @@ import { BASE_PRINCIPAL_CONFIG_SECTIONS, BASE_ONLY_CONFIG_SECTIONS, + BASE_ONLY_CONFIG_PATHS, INTERFACE_PERMISSION_FIELDS, RUNTIME_CONFIG_INTERFACE_FIELDS, PERMISSION_SUB_KEYS, @@ -852,7 +853,30 @@ export function mergeConfigOverrides(baseConfig: AppConfig, configs: IConfig[]): } } - return preserveRuntimeStops(baseConfig, merged); + return preserveBaseOnlyPaths(baseConfig, preserveRuntimeStops(baseConfig, merged)); +} + +function getPath(target: unknown, segments: string[]): unknown { + let current = target; + for (const segment of segments) { + if (!isPlainObject(current)) return undefined; + current = current[segment]; + } + return current; +} + +/** + * Puts back the base value of every `BASE_ONLY_CONFIG_PATHS` field, whatever an override set or a + * tombstone removed on the way, so every reader of the merged config sees the operator's value. + */ +function preserveBaseOnlyPaths(baseConfig: AppConfig, merged: T): T { + let result: unknown = merged; + for (const path of BASE_ONLY_CONFIG_PATHS) { + const segments = path.split('.'); + const value = getPath(baseConfig, segments); + if (getPath(result, segments) !== value) result = setPath(result, segments, value); + } + return result as T; } /** Whether a runtime-config interface field reads as OFF in either of its two shapes: diff --git a/packages/data-schemas/src/methods/conversation.spec.ts b/packages/data-schemas/src/methods/conversation.spec.ts index 3741259decc..c971f1b4089 100644 --- a/packages/data-schemas/src/methods/conversation.spec.ts +++ b/packages/data-schemas/src/methods/conversation.spec.ts @@ -9504,6 +9504,13 @@ describe('laneGit', () => { await expect(read(conversationId)).resolves.not.toHaveProperty('pullRequest'); }); + it('treats an empty repository like an unknown one when keeping the link', async () => { + const conversationId = await seed(); + await record({ user, conversationId, laneGit: lane, pullRequest: 7 }); + await record({ user, conversationId, laneGit: lane, repo: '' }); + await expect(read(conversationId)).resolves.toEqual({ ...lane, pullRequest: 7 }); + }); + it('replaces the link with a newer one', async () => { const conversationId = await seed(); await record({ user, conversationId, laneGit: lane, repo: 'o/r', pullRequest: 7 }); diff --git a/packages/data-schemas/src/methods/conversation.ts b/packages/data-schemas/src/methods/conversation.ts index afc058fce5c..65d8a07c3c1 100644 --- a/packages/data-schemas/src/methods/conversation.ts +++ b/packages/data-schemas/src/methods/conversation.ts @@ -11,6 +11,7 @@ import type { Model, SortOrder, Types, + UpdateQuery, } from 'mongoose'; import type { SearchParams } from 'meilisearch'; import type { @@ -3176,29 +3177,6 @@ export function createConversationMethods( workspace?: { environmentId: string; workspaceId: string; required?: boolean; epoch?: number }; }): Promise { const Conversation = mongoose.models.Conversation as Model; - /** - * A pipeline update, so a report without a link keeps the one already recorded while the lane - * is still on that branch and repository. Values are literals: a branch may start with `$`. - */ - const keptPullRequest = { - $cond: [ - { - $and: [ - { $eq: ['$laneGit.branch', { $literal: laneGit.branch }] }, - { $eq: ['$laneGit.repo', repo ? { $literal: repo } : '$$REMOVE'] }, - ], - }, - '$laneGit.pullRequest', - '$$REMOVE', - ], - }; - const next = { - branch: { $literal: laneGit.branch }, - head: { $literal: laneGit.head }, - ...(repo ? { repo: { $literal: repo } } : {}), - pullRequest: pullRequest != null ? { $literal: pullRequest } : keptPullRequest, - seq: { $literal: seq }, - }; const epochFence = workspace?.epoch == null ? [] @@ -3237,28 +3215,62 @@ export function createConversationMethods( ], }, ]; - const result = await withoutMeiliIndexing( - Conversation.updateOne( - { - user, - conversationId, - ...activeExpirationFilter(), - $and: [ - { - $or: [ - { 'laneGit.seq': { $exists: false } }, - { 'laneGit.seq': null }, - { 'laneGit.seq': { $lt: seq } }, - ], - }, - ...workspaceFence, - ], + const fences: FilterQuery[] = [ + { + $or: [ + { 'laneGit.seq': { $exists: false } }, + { 'laneGit.seq': null }, + { 'laneGit.seq': { $lt: seq } }, + ], + }, + ...workspaceFence, + ]; + const write = async ( + lane: FilterQuery | null, + update: UpdateQuery, + ): Promise => { + const result = await withoutMeiliIndexing( + Conversation.updateOne( + { + user, + conversationId, + ...activeExpirationFilter(), + $and: lane == null ? fences : [...fences, lane], + }, + update, + { timestamps: false }, + ), + ); + return result.modifiedCount === 1; + }; + const replaced = { + branch: laneGit.branch, + head: laneGit.head, + ...(repo ? { repo } : {}), + ...(pullRequest != null ? { pullRequest } : {}), + seq, + }; + if (pullRequest != null) return write(null, { $set: { laneGit: replaced } }); + /** + * A report without a link keeps the one already recorded while the lane is still on that + * branch and repository, and drops it otherwise. Plain updates rather than an aggregation + * pipeline, which Amazon DocumentDB rejects: the first matches only the same lane and sets its + * fields one by one, the second only another lane and replaces the whole state. A concurrent + * write can move the stored lane between the two, so they are retried once. + */ + const sameLane = { 'laneGit.branch': laneGit.branch, 'laneGit.repo': repo || null }; + for (let attempt = 0; attempt < 2; attempt++) { + const kept = await write(sameLane, { + $set: { + 'laneGit.branch': laneGit.branch, + 'laneGit.head': laneGit.head, + 'laneGit.seq': seq, }, - [{ $set: { laneGit: next } }], - { timestamps: false }, - ), - ); - return result.modifiedCount === 1; + }); + if (kept) return true; + if (await write({ $nor: [sameLane] }, { $set: { laneGit: replaced } })) return true; + } + return false; } /** The last reported lane state, or null when the conversation has none or is not the owner's. */