diff --git a/packages/api/src/code/command.ts b/packages/api/src/code/command.ts index 593f96d2949..81e5750af31 100644 --- a/packages/api/src/code/command.ts +++ b/packages/api/src/code/command.ts @@ -478,9 +478,9 @@ export function createAttachedWorkspaceBashTool({ baseUrl: string; authHeaders: () => Promise> | Record; workspaceId: string; - /** Receives the lane's branch and head when a finished command reports them. It is not awaited - * and must not throw: a failure here never changes the command's result. */ - onLaneGit?: (laneGit: WorkspaceLaneGit) => void; + /** Receives the lane's branch and head when a finished command reports them, with the command's + * output. It is not awaited and must not throw: a failure here never changes the result. */ + onLaneGit?: (laneGit: WorkspaceLaneGit, output: string) => void; workspaceInstanceId?: string; /** The worker runs each `.worktrees/` in its own lane; a matching `cwd` is routed there. */ linkedWorktrees?: boolean; @@ -617,7 +617,9 @@ export function createAttachedWorkspaceBashTool({ throw new Error('Attached workspace returned an unexpected command result.'); } logger.debug('[BYOMCommand] transport completed', trace); - if (result.laneGit != null) onLaneGit?.(result.laneGit); + if (result.laneGit != null) { + onLaneGit?.(result.laneGit, `${result.stdout}\n${result.stderr}`); + } let content = formatCommandResult( result, selectedTimeoutMs, diff --git a/packages/api/src/code/lane.spec.ts b/packages/api/src/code/lane.spec.ts index e7038e9f99d..68070862255 100644 --- a/packages/api/src/code/lane.spec.ts +++ b/packages/api/src/code/lane.spec.ts @@ -13,8 +13,8 @@ type RecorderOptions = Parameters[0]; /** Places the recorder once, as the tool does, then reports through it in call order. */ const createLaneGitRecorder = (options: Omit) => { const ready = createRecorder({ enabled: true, ...options }); - return (laneGit: Parameters>>[0]) => - ready.then((record) => (record ? record(laneGit) : false)); + return (laneGit: Parameters>>[0], output?: string) => + ready.then((record) => (record ? record(laneGit, output) : false)); }; const head = 'a'.repeat(40); @@ -103,6 +103,31 @@ describe('createLaneGitRecorder', () => { expect(JSON.stringify((logger.warn as jest.Mock).mock.calls)).not.toContain('secret'); }); + it('records the pull request the output links to in the lane repository', async () => { + const { record, setConvoLaneGit } = make({ repo: 'o/r' }); + await record?.(laneGit, 'Created https://github.com/o/r/pull/42\n'); + expect(setConvoLaneGit).toHaveBeenCalledWith( + expect.objectContaining({ repo: 'o/r', pullRequest: 42 }), + ); + }); + + 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], + [ + 'no repository to match the link against', + 'https://github.com/o/r/pull/42', + undefined, + laneGit, + ], + ['a detached lane', 'https://github.com/o/r/pull/42', 'o/r', { branch: null, head }], + ])('records no pull request for %s', async (_label, output, repo, lane) => { + const { record, setConvoLaneGit } = make({ repo }); + await record?.(lane, output); + expect(setConvoLaneGit).toHaveBeenCalledTimes(1); + expect(setConvoLaneGit.mock.calls[0][0]).not.toHaveProperty('pullRequest'); + }); + it('writes nothing when the conversation is not saved yet', async () => { const { record, setConvoLaneGit } = make({ reserveConvoLaneGitSeq: jest.fn().mockResolvedValue(null), @@ -140,7 +165,18 @@ describe('attached bash tool lane reporting', () => { jest.fn(async () => response({ laneGit })), onLaneGit, ).invoke({ command: 'git status' }); - expect(onLaneGit).toHaveBeenCalledWith(laneGit); + expect(onLaneGit).toHaveBeenCalledWith(laneGit, 'ok\n'); + }); + + it('hands over stdout and stderr, so a link printed to either is seen', async () => { + const onLaneGit = jest.fn(); + await build( + jest.fn(async () => + response({ laneGit, stdout: 'https://github.com/o/r/pull/7', stderr: 'warning' }), + ), + onLaneGit, + ).invoke({ command: 'gh pr create' }); + expect(onLaneGit).toHaveBeenCalledWith(laneGit, 'https://github.com/o/r/pull/7\nwarning'); }); it('hands over a detached lane as nulls, not as unknown', async () => { @@ -150,7 +186,7 @@ describe('attached bash tool lane reporting', () => { jest.fn(async () => response({ laneGit: detached })), onLaneGit, ).invoke({ command: 'git status' }); - expect(onLaneGit).toHaveBeenCalledWith(detached); + expect(onLaneGit).toHaveBeenCalledWith(detached, 'ok\n'); }); it('does not call the recorder when the worker sent no laneGit', async () => { diff --git a/packages/api/src/code/lane.ts b/packages/api/src/code/lane.ts index 77322e9722c..47c1740ea15 100644 --- a/packages/api/src/code/lane.ts +++ b/packages/api/src/code/lane.ts @@ -1,5 +1,6 @@ import { logger } from '@librechat/data-schemas'; import type { WorkspaceLaneGit } from './workspace'; +import { findPullRequestLink } from '~/pulls/links'; import { getSafeErrorMetadata } from '~/utils'; /** @@ -18,6 +19,8 @@ export type LaneGitWriter = (input: { conversationId: string; laneGit: WorkspaceLaneGit; repo?: string; + /** A pull request of `repo` the command's output linked to; see `findPullRequestLink`. */ + pullRequest?: number; /** Reserved when the command settled; the database ignores a report below the one it holds. */ seq: number; /** The database ignores the report once the conversation is no longer on this workspace. */ @@ -100,7 +103,7 @@ export async function createLaneGitRecorder({ getConvoLaneContext?: LaneContextReader; reserveConvoLaneGitSeq: LaneSeqReserver; setConvoLaneGit: LaneGitWriter; -}): Promise<((laneGit: WorkspaceLaneGit) => Promise) | undefined> { +}): Promise<((laneGit: WorkspaceLaneGit, output?: string) => Promise) | undefined> { if (!enabled || !user || !conversationId) return undefined; const safeRepo = isSafeRepo(repo) ? repo : undefined; @@ -131,13 +134,18 @@ export async function createLaneGitRecorder({ } }; - const write = async (laneGit: WorkspaceLaneGit, reserved: Reservation): Promise => { + const write = async ( + laneGit: WorkspaceLaneGit, + pullRequest: number | null, + reserved: Reservation, + ): Promise => { try { return await setConvoLaneGit({ user, conversationId: reserved.conversationId, laneGit, ...(safeRepo ? { repo: safeRepo } : {}), + ...(pullRequest != null ? { pullRequest } : {}), seq: reserved.seq, ...(workspace ? { @@ -163,14 +171,22 @@ export async function createLaneGitRecorder({ * last wrote, and the database already ignores a write that changes nothing the reader can see. * Resolves to whether the write applied, which is false when a newer report is already stored * and for a conversation not saved yet. A repo that is not a plain `owner/name` is dropped. + * `output` is what the command printed; a pull request link to the lane's repository in it is + * recorded with the lane, so the header can show that pull request without searching for it. */ - return (laneGit) => { + return (laneGit, output) => { + const pullRequest = + safeRepo != null && output != null && laneGit.branch != null + ? findPullRequestLink(output, safeRepo) + : null; const previous = reservationTails.get(queueKey) ?? Promise.resolve(); const reservation = previous.then(() => reserve()); reservationTails.set(queueKey, reservation); void reservation.then(() => { if (reservationTails.get(queueKey) === reservation) reservationTails.delete(queueKey); }); - return reservation.then((reserved) => (reserved == null ? false : write(laneGit, reserved))); + return reservation.then((reserved) => + reserved == null ? false : write(laneGit, pullRequest, reserved), + ); }; } diff --git a/packages/api/src/pulls/github.spec.ts b/packages/api/src/pulls/github.spec.ts index 93e9b2f2e6e..480c62946ec 100644 --- a/packages/api/src/pulls/github.spec.ts +++ b/packages/api/src/pulls/github.spec.ts @@ -934,3 +934,108 @@ describe('reading more than one page of candidates', () => { ).toHaveLength(3); }); }); + +describe('a pull request the conversation linked to', () => { + const recorded = 'c'.repeat(40); + const linkedHead = { sha, ref: 'feat/x', repo: { full_name: 'O/R' } }; + const forkHead = { sha, ref: 'feat/x', repo: { full_name: 'someone/r' } }; + const run = ( + source: ReturnType, + pullRequest: number | null = 7, + ) => source.find({ repo: 'o/r', branch: 'feat/x', head: recorded, pullRequest, token: 't' }); + const urls = (fetchFn: jest.Mock) => fetchFn.mock.calls.map(([url]) => String(url)); + + it('reads it directly, without searching or comparing, so unpushed commits do not hide it', async () => { + const { source, fetchFn } = sourceFor({ + '/pulls/7': () => json(pull({ head: linkedHead })), + '/check-runs': () => json({ total_count: 0, check_runs: [] }), + }); + await expect(run(source)).resolves.toMatchObject({ number: 7, state: 'open' }); + expect(urls(fetchFn).some((url) => url.includes('/pulls?'))).toBe(false); + expect(urls(fetchFn).some((url) => url.includes('/compare/'))).toBe(false); + }); + + it('reads checks for the head the linked pull request reports', async () => { + const { source, fetchFn } = sourceFor({ + '/pulls/7': () => json(pull({ head: linkedHead })), + '/check-runs': () => json({ total_count: 0, check_runs: [] }), + }); + await run(source); + expect(urls(fetchFn).some((url) => url.includes(`/commits/${sha}/check-runs`))).toBe(true); + }); + + it('keeps a merged link, the way the branch search would', async () => { + const { source } = sourceFor({ + '/pulls/7': () => json(pull({ state: 'closed', merged: true, head: linkedHead })), + '/check-runs': () => json({ total_count: 0, check_runs: [] }), + }); + await expect(run(source)).resolves.toMatchObject({ number: 7, state: 'merged' }); + }); + + it('searches the branch when the link is a pull request from another branch', async () => { + const { source } = sourceFor({ + '/pulls/3': () => json(pull({ number: 3, head: { sha, ref: 'feat/parent' } })), + '/pulls?state=open': () => json([]), + '/pulls?state=closed': () => json([]), + }); + await expect(run(source, 3)).resolves.toBeNull(); + }); + + it('searches the branch when the link names a pull request GitHub does not have', async () => { + const { source, fetchFn } = sourceFor({ + '/pulls/9': () => new Response('', { status: 404 }), + '/pulls?state=open': () => json([]), + '/pulls?state=closed': () => json([]), + }); + await expect(run(source, 9)).resolves.toBeNull(); + expect(urls(fetchFn).some((url) => url.includes('/pulls?state=open'))).toBe(true); + }); + + it('cannot verify a pull request whose head names no branch, and searches instead', async () => { + const { source, fetchFn } = sourceFor({ + '/pulls/3': () => json(pull({ number: 3 })), + '/pulls?state=open': () => json([]), + '/pulls?state=closed': () => json([]), + }); + await expect(run(source, 3)).resolves.toBeNull(); + expect(urls(fetchFn).some((url) => url.includes('/pulls?state=open'))).toBe(true); + }); + + it('does not take a fork pull request on a branch of the same name without the recorded commit', async () => { + const { source, fetchFn } = sourceFor({ + '/pulls/7': () => json(pull({ head: forkHead })), + '/pulls?state=open': () => json([]), + '/pulls?state=closed': () => json([]), + }); + await expect( + source.find({ repo: 'o/r', branch: 'feat/x', pullRequest: 7, token: 't' }), + ).resolves.toBeNull(); + expect(urls(fetchFn).some((url) => url.includes('/pulls?state=open'))).toBe(true); + }); + + it('does not take a fork pull request whose commits do not contain the recorded one', async () => { + const { source } = sourceFor({ + '/pulls/7': () => json(pull({ head: forkHead })), + '/compare/': () => json({ status: 'diverged' }), + '/pulls?state=open': () => json([]), + '/pulls?state=closed': () => json([]), + }); + await expect(run(source)).resolves.toBeNull(); + }); + + it('takes a fork pull request that carries the recorded commit', async () => { + const { source } = sourceFor({ + '/pulls/7': () => json(pull({ head: forkHead })), + '/compare/': () => json({ status: 'ahead' }), + '/check-runs': () => json({ total_count: 0, check_runs: [] }), + }); + await expect(run(source)).resolves.toMatchObject({ number: 7 }); + }); + + it('lets a rate limit on the linked read fail the lookup', async () => { + const { source } = sourceFor({ + '/pulls/7': () => new Response('', { status: 429 }), + }); + await expect(run(source)).rejects.toMatchObject({ code: 'RATE_LIMITED' }); + }); +}); diff --git a/packages/api/src/pulls/github.ts b/packages/api/src/pulls/github.ts index 80d3cff1117..e4f637686a1 100644 --- a/packages/api/src/pulls/github.ts +++ b/packages/api/src/pulls/github.ts @@ -52,6 +52,8 @@ type GitHubPull = { headSha: string; /** The repository that head lives in; a fork's checks run there, not in the base repository. */ headRepo?: string; + /** The branch the pull request merges from, in whichever repository it lives; null if unsaid. */ + headRef: string | null; }; type GitHubCheckRun = { status: string; conclusion: string | null }; @@ -107,6 +109,7 @@ function parsePull(value: unknown): GitHubPull { mergeable: value.mergeable, headSha: value.head.sha, ...(headRepo ? { headRepo } : {}), + headRef: typeof value.head.ref === 'string' ? value.head.ref : null, }; } @@ -330,7 +333,15 @@ export function createGitHubPullRequestSource({ } return { - async find({ repo, branch, head: recordedHead, token, limits, allowedRepositories }) { + async find({ + repo, + branch, + head: recordedHead, + pullRequest, + token, + limits, + allowedRepositories, + }) { /** * A lane with a branch and no commit yet has nothing to match a pull request against, and * every candidate would be accepted, including an earlier incarnation of a reused branch @@ -352,6 +363,27 @@ export function createGitHubPullRequestSource({ const [, owner, name] = match; if (!isPathSegment(owner) || !isPathSegment(name)) return null; const base = `/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}`; + /** + * A fork's checks run in the fork. Its repository is read only when an administrator named + * it, so the server's token is never used against a repository nobody authorized. + */ + const checksBaseOf = (pull: GitHubPull): string => { + const headRepo = pull.headRepo; + if (headRepo == null || headRepo.toLowerCase() === `${owner}/${name}`.toLowerCase()) { + return base; + } + const parts = REPO_PATTERN.exec(headRepo); + if (parts == null || !isPathSegment(parts[1]) || !isPathSegment(parts[2])) return base; + if (!isAllowedRepository(headRepo, allowedRepositories)) return base; + return `/repos/${encodeURIComponent(parts[1])}/${encodeURIComponent(parts[2])}`; + }; + const report = async (pull: GitHubPull): Promise => { + const checks = await readCheckRuns( + `${checksBaseOf(pull)}/commits/${pull.headSha}/check-runs`, + lookup, + ); + return toConversationPullRequest(pull, checks.runs, checks.incomplete); + }; const headFilter = encodeURIComponent(`${owner}:${branch}`); /** `undefined`: the repository is not visible. An empty list: it is, and nothing matched. */ const listPulls = async (state: 'open' | 'closed', perPage: number, page = 1) => { @@ -409,6 +441,25 @@ export function createGitHubPullRequestSource({ } return all; }; + /** + * A pull request the conversation's own output linked to is read directly, so a lane with + * commits not pushed yet still shows it. Its head must be the lane's branch in the lane's + * repository. A branch name alone proves nothing for a fork (anyone can open `patch-1`), so + * a fork's pull request counts only when it carries the recorded commit, as in the search. + */ + if (pullRequest != null && Number.isSafeInteger(pullRequest) && pullRequest > 0) { + const linked = await getJson(`${base}/pulls/${pullRequest}`, lookup); + if (linked != null) { + const pull = parsePull(linked); + const sameRepository = pull.headRepo?.toLowerCase() === repo.toLowerCase(); + if ( + pull.headRef === branch && + (sameRepository || (recorded != null && (await contains(pull.headSha)))) + ) { + return report(pull); + } + } + } /** Open first on its own, so closed history on a reused branch name cannot hide it. */ const open = await listAll('open'); if (open === undefined) return null; @@ -453,25 +504,7 @@ export function createGitHubPullRequestSource({ const pull = parsePull(detail); /** A push between the list and the detail can move the head off the recorded commit. */ if (pull.headSha !== chosen.sha && !(await contains(pull.headSha))) return null; - /** - * A fork's checks run in the fork. Its repository is read only when an administrator named - * it, so the server's token is never used against a repository nobody authorized. - */ - const checksBase = (() => { - const headRepo = pull.headRepo; - if (headRepo == null || headRepo.toLowerCase() === `${owner}/${name}`.toLowerCase()) { - return base; - } - const parts = REPO_PATTERN.exec(headRepo); - if (parts == null || !isPathSegment(parts[1]) || !isPathSegment(parts[2])) return base; - if (!isAllowedRepository(headRepo, allowedRepositories)) return base; - return `/repos/${encodeURIComponent(parts[1])}/${encodeURIComponent(parts[2])}`; - })(); - const checks = await readCheckRuns( - `${checksBase}/commits/${pull.headSha}/check-runs`, - lookup, - ); - return toConversationPullRequest(pull, checks.runs, checks.incomplete); + return report(pull); }, }; } diff --git a/packages/api/src/pulls/handler.spec.ts b/packages/api/src/pulls/handler.spec.ts index f44e9de9642..0ed446e2741 100644 --- a/packages/api/src/pulls/handler.spec.ts +++ b/packages/api/src/pulls/handler.spec.ts @@ -34,7 +34,12 @@ const configWith = (pullRequests?: Record) => ({ endpoints: { agents: { pullRequests } }, }); -type Lane = { branch: string | null; head: string | null; repo?: string } | null; +type Lane = { + branch: string | null; + head: string | null; + repo?: string; + pullRequest?: number; +} | null; function setup( options: { @@ -102,6 +107,14 @@ describe('createConversationPullRequestHandler', () => { expect(res.json).toHaveBeenCalledWith({ pullRequest: pr }); }); + it('passes the pull request the conversation linked to', async () => { + const { run, lookup } = setup({ + laneGit: { branch: 'feat/x', head: null, repo: 'o/r', pullRequest: 7 }, + }); + await run(); + expect(lookup).toHaveBeenCalledWith(expect.objectContaining({ pullRequest: 7 })); + }); + it('passes the configured lookup bounds to GitHub', async () => { const { run, lookup } = setup({ settings: { @@ -404,6 +417,18 @@ describe('createConversationPullRequestsHandler', () => { return { run, res, lookup, getConvosLaneGit }; } + it('keeps conversations on one lane apart when they linked different pull requests', async () => { + const linked = (conversationId: string, pullRequest: number) => ({ + conversationId, + laneGit: { branch: 'feat/x', head: null, repo: 'o/r', pullRequest }, + }); + const { run, lookup } = batch({ lanes: [linked('a', 7), linked('b', 9)] }); + await run({ conversationIds: ['a', 'b'] }); + expect(lookup).toHaveBeenCalledTimes(2); + expect(lookup).toHaveBeenCalledWith(expect.objectContaining({ pullRequest: 7 })); + expect(lookup).toHaveBeenCalledWith(expect.objectContaining({ pullRequest: 9 })); + }); + it('looks up any repository on the batch route too, when the administrator opted in', async () => { const { run, lookup } = batch({ settings: { allowAllRepositories: true }, diff --git a/packages/api/src/pulls/handler.ts b/packages/api/src/pulls/handler.ts index 7b5798f6ba4..8a62ae4f56c 100644 --- a/packages/api/src/pulls/handler.ts +++ b/packages/api/src/pulls/handler.ts @@ -21,14 +21,19 @@ import { getSafeErrorMetadata } from '~/utils'; const MAX_CONVERSATION_ID_LENGTH = 256; -export type ConversationLaneGit = { branch: string | null; head: string | null; repo?: string }; +export type ConversationLaneGit = { + branch: string | null; + head: string | null; + repo?: string; + pullRequest?: number; +}; const validConversationId = (value: string | undefined): value is string => value != null && value.trim() !== '' && value.length <= MAX_CONVERSATION_ID_LENGTH; const NONE: TConversationPullRequestResponse = { pullRequest: null }; -type EligibleLane = { branch: string; head: string | null; repo: string }; +type EligibleLane = { branch: string; head: string | null; repo: string; pullRequest?: number }; /** Lookups in flight at once for one batch, unless configured. */ const DEFAULT_BATCH_CONCURRENCY = 4; @@ -164,7 +169,12 @@ function eligibleLane( ): EligibleLane | null { if (laneGit?.branch == null || laneGit.repo == null) return null; if (!isAllowedRepository(laneGit.repo, repositoryScope(settings))) return null; - return { branch: laneGit.branch, head: laneGit.head, repo: laneGit.repo }; + return { + branch: laneGit.branch, + head: laneGit.head, + repo: laneGit.repo, + ...(laneGit.pullRequest != null ? { pullRequest: laneGit.pullRequest } : {}), + }; } /** Everything one lookup needs from the settings, so the single and batch routes cannot differ. */ @@ -173,6 +183,7 @@ function lookupInput(settings: PullRequestSettings, token: string, lane: Eligibl repo: lane.repo, branch: lane.branch, head: lane.head, + ...(lane.pullRequest != null ? { pullRequest: lane.pullRequest } : {}), token, ttlMs: (settings.cacheTtlSeconds ?? 30) * 1000, cacheMaxEntries: settings.cacheMaxEntries ?? 500, @@ -401,7 +412,8 @@ export function createConversationPullRequestsHandler(deps: { * already coalesces them, so each distinct lookup takes one slot and one deadline, not one * per conversation. Otherwise duplicates would hold every slot while a single request ran. */ - const keyOf = (lane: EligibleLane) => `${lane.repo}\0${lane.branch}\0${lane.head ?? ''}`; + const keyOf = (lane: EligibleLane) => + `${lane.repo}\0${lane.branch}\0${lane.head ?? ''}\0${lane.pullRequest ?? ''}`; const distinct = new Map(); for (const lane of eligible.values()) distinct.set(keyOf(lane), lane); type Outcome = @@ -421,6 +433,7 @@ export function createConversationPullRequestsHandler(deps: { input.repo, input.branch, input.head, + input.pullRequest, input.ttlMs, input.limits, input.allowedRepositories, diff --git a/packages/api/src/pulls/index.ts b/packages/api/src/pulls/index.ts index f76b51e1a2e..ae357681c80 100644 --- a/packages/api/src/pulls/index.ts +++ b/packages/api/src/pulls/index.ts @@ -1,4 +1,5 @@ export * from './types'; +export * from './links'; export * from './github'; export * from './lookup'; export * from './handler'; diff --git a/packages/api/src/pulls/links.spec.ts b/packages/api/src/pulls/links.spec.ts new file mode 100644 index 00000000000..e2a5eb96b24 --- /dev/null +++ b/packages/api/src/pulls/links.spec.ts @@ -0,0 +1,44 @@ +import { findPullRequestLink } from './links'; + +describe('findPullRequestLink', () => { + it('finds the pull request `gh pr create` prints', () => { + const output = + 'Creating pull request for feat/x into dev in o/r\n\nhttps://github.com/o/r/pull/42\n'; + expect(findPullRequestLink(output, 'o/r')).toBe(42); + }); + + it('matches the repository without case, as GitHub does', () => { + expect(findPullRequestLink('https://github.com/Owner/Repo/pull/5', 'owner/repo')).toBe(5); + }); + + it('takes the last link to the repository when there are several', () => { + const output = 'see https://github.com/o/r/pull/3 then https://github.com/o/r/pull/8.'; + expect(findPullRequestLink(output, 'o/r')).toBe(8); + }); + + it('accepts a link to a page of the pull request', () => { + expect(findPullRequestLink('https://github.com/o/r/pull/12/files', 'o/r')).toBe(12); + }); + + it('accepts trailing punctuation after the number', () => { + expect(findPullRequestLink('(https://github.com/o/r/pull/12)', 'o/r')).toBe(12); + }); + + it.each([ + ['another repository', 'https://github.com/other/r/pull/4'], + ['a repository that only starts the same', 'https://github.com/o/r-fork/pull/4'], + ['an issue', 'https://github.com/o/r/issues/4'], + ['the page git push suggests', 'https://github.com/o/r/pull/new/feat/x'], + ['another host', 'https://example.com/o/r/pull/4'], + ['plain http', 'http://github.com/o/r/pull/4'], + ['a number glued to more text', 'https://github.com/o/r/pull/4abc'], + ['pull request zero', 'https://github.com/o/r/pull/0'], + ['a bare number', 'opened #4'], + ])('ignores %s', (_, output) => { + expect(findPullRequestLink(output, 'o/r')).toBeNull(); + }); + + it('ignores a number too large to be a pull request', () => { + expect(findPullRequestLink(`https://github.com/o/r/pull/${'9'.repeat(20)}`, 'o/r')).toBeNull(); + }); +}); diff --git a/packages/api/src/pulls/links.ts b/packages/api/src/pulls/links.ts new file mode 100644 index 00000000000..99f5c4ee55a --- /dev/null +++ b/packages/api/src/pulls/links.ts @@ -0,0 +1,24 @@ +/** + * `https://github.com/{owner}/{name}/pull/{number}`, ending where the number does. A `/pull/new/` + * page (what `git push` suggests) has no number and never matches. + */ +const PULL_REQUEST_URL = + /https:\/\/github\.com\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)\/pull\/(\d+)(?![\w-])/g; + +/** + * 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. + */ +export function findPullRequestLink(output: string, repo: string): number | null { + if (!output.includes('/pull/')) return null; + const wanted = repo.toLowerCase(); + let found: number | null = null; + for (const match of output.matchAll(PULL_REQUEST_URL)) { + const [, owner, name, digits] = match; + if (`${owner}/${name}`.toLowerCase() !== wanted) continue; + const number = Number(digits); + if (Number.isSafeInteger(number) && number > 0) found = number; + } + return found; +} diff --git a/packages/api/src/pulls/lookup.spec.ts b/packages/api/src/pulls/lookup.spec.ts index ae2ac28a624..eb36f1f991a 100644 --- a/packages/api/src/pulls/lookup.spec.ts +++ b/packages/api/src/pulls/lookup.spec.ts @@ -66,6 +66,16 @@ describe('createPullRequestLookup', () => { expect(find).toHaveBeenCalledTimes(3); }); + it('keys by the linked pull request and hands it to the source', async () => { + const find = jest.fn().mockResolvedValue(value); + const lookup = createPullRequestLookup({ source: { find } }); + await lookup(input); + await lookup({ ...input, pullRequest: 1 }); + await lookup({ ...input, pullRequest: 1 }); + expect(find).toHaveBeenCalledTimes(2); + expect(find).toHaveBeenLastCalledWith(expect.objectContaining({ pullRequest: 1 })); + }); + it('returns a failure as a coded result and retries it sooner than a success', async () => { let clock = 0; const find = jest diff --git a/packages/api/src/pulls/lookup.ts b/packages/api/src/pulls/lookup.ts index ed71c84ed53..3aa16e5efff 100644 --- a/packages/api/src/pulls/lookup.ts +++ b/packages/api/src/pulls/lookup.ts @@ -110,6 +110,7 @@ export function createPullRequestLookup({ repo, branch, head, + pullRequest, token, ttlMs, limits, @@ -133,7 +134,7 @@ export function createPullRequestLookup({ .sort() .join('|'), ].join(','); - const key = `${scope}\0${repo}#${branch}\0${head ?? ''}\0${policy}`; + const key = `${scope}\0${repo}#${branch}\0${head ?? ''}\0${pullRequest ?? ''}\0${policy}`; const cached = touch(scope)?.get(key); if (cached != null && cached.expiresAt > now()) return cached.result; const pending = inflight.get(key); @@ -149,7 +150,15 @@ export function createPullRequestLookup({ try { result = { ok: true, - value: await source.find({ repo, branch, head, token, limits, allowedRepositories }), + value: await source.find({ + repo, + branch, + head, + pullRequest, + token, + limits, + allowedRepositories, + }), }; } catch (error) { if (!(error instanceof PullRequestSourceError)) { diff --git a/packages/api/src/pulls/types.ts b/packages/api/src/pulls/types.ts index c06c1ca1608..b864c282104 100644 --- a/packages/api/src/pulls/types.ts +++ b/packages/api/src/pulls/types.ts @@ -43,6 +43,12 @@ export type PullRequestFindInput = { branch: string; /** The commit the conversation last ran at; a pull request must carry it to be the match. */ head?: string | null; + /** + * A pull request the conversation's own command output linked to. It is the answer when its + * head branch is `branch`, whether or not GitHub has the recorded commit yet; otherwise the + * branch is searched as if no link had been seen. + */ + pullRequest?: number | null; token: string; limits?: PullRequestLookupLimits; /** The repositories the token may be used for. A pull request from a fork has its checks read diff --git a/packages/data-schemas/src/methods/conversation.spec.ts b/packages/data-schemas/src/methods/conversation.spec.ts index b1241e93eea..3741259decc 100644 --- a/packages/data-schemas/src/methods/conversation.spec.ts +++ b/packages/data-schemas/src/methods/conversation.spec.ts @@ -9459,6 +9459,82 @@ describe('laneGit', () => { await expect(methods.getConvoLaneGit('lane-user', conversationId)).resolves.toEqual(laneGit); }); + describe('linked pull request', () => { + const user = 'lane-user'; + const lane = { branch: 'feat/x', head }; + const read = (conversationId: string) => methods.getConvoLaneGit(user, conversationId); + + it('stores the link and keeps it through later reports on the same branch', async () => { + const conversationId = await seed(); + await record({ user, conversationId, laneGit: lane, repo: 'o/r', pullRequest: 7 }); + const moved = { branch: 'feat/x', head: 'b'.repeat(40) }; + await expect(record({ user, conversationId, laneGit: moved, repo: 'o/r' })).resolves.toBe( + true, + ); + await expect(read(conversationId)).resolves.toEqual({ + ...moved, + repo: 'o/r', + pullRequest: 7, + }); + }); + + it('returns the link from the batch read too', async () => { + const conversationId = await seed(); + await record({ user, conversationId, laneGit: lane, repo: 'o/r', pullRequest: 7 }); + await expect(methods.getConvosLaneGit(user, [conversationId])).resolves.toEqual([ + { conversationId, laneGit: { ...lane, repo: 'o/r', pullRequest: 7 } }, + ]); + }); + + it('drops the link when the branch changes', async () => { + const conversationId = await seed(); + await record({ user, conversationId, laneGit: lane, repo: 'o/r', pullRequest: 7 }); + await record({ user, conversationId, laneGit: { branch: 'dev', head }, repo: 'o/r' }); + await expect(read(conversationId)).resolves.not.toHaveProperty('pullRequest'); + }); + + it('drops the link when the repository changes or is no longer known', async () => { + const conversationId = await seed(); + await record({ user, conversationId, laneGit: lane, repo: 'o/r', pullRequest: 7 }); + await record({ user, conversationId, laneGit: lane, repo: 'o/other' }); + await expect(read(conversationId)).resolves.not.toHaveProperty('pullRequest'); + + await record({ user, conversationId, laneGit: lane, repo: 'o/r', pullRequest: 7 }); + await record({ user, conversationId, laneGit: lane }); + await expect(read(conversationId)).resolves.not.toHaveProperty('pullRequest'); + }); + + it('replaces the link with a newer one', async () => { + const conversationId = await seed(); + await record({ user, conversationId, laneGit: lane, repo: 'o/r', pullRequest: 7 }); + await record({ user, conversationId, laneGit: lane, repo: 'o/r', pullRequest: 9 }); + await expect(read(conversationId)).resolves.toMatchObject({ pullRequest: 9 }); + }); + + it('ignores a link carried by a report with a lower sequence number', async () => { + const conversationId = await seed(); + const first = (await methods.reserveConvoLaneGitSeq(user, conversationId)) as number; + const second = (await methods.reserveConvoLaneGitSeq(user, conversationId)) as number; + await record({ user, conversationId, laneGit: lane, repo: 'o/r', seq: second }); + await expect( + record({ user, conversationId, laneGit: lane, repo: 'o/r', pullRequest: 7, seq: first }), + ).resolves.toBe(false); + await expect(read(conversationId)).resolves.not.toHaveProperty('pullRequest'); + }); + + it('stores a branch or repository that looks like a field path as plain text', async () => { + const conversationId = await seed(); + const dollar = { branch: '$laneGit.head', head }; + await record({ user, conversationId, laneGit: dollar, repo: 'o/r', pullRequest: 7 }); + await record({ user, conversationId, laneGit: dollar, repo: 'o/r' }); + await expect(read(conversationId)).resolves.toEqual({ + ...dollar, + repo: 'o/r', + pullRequest: 7, + }); + }); + }); + it('does not write for another owner or an unknown conversation', async () => { const conversationId = await seed(); const laneGit = { branch: 'main', head }; diff --git a/packages/data-schemas/src/methods/conversation.ts b/packages/data-schemas/src/methods/conversation.ts index 107b7e0211c..afc058fce5c 100644 --- a/packages/data-schemas/src/methods/conversation.ts +++ b/packages/data-schemas/src/methods/conversation.ts @@ -355,6 +355,8 @@ export interface ConversationMethods { conversationId: string; laneGit: Pick, 'branch' | 'head'>; repo?: string; + /** A pull request the command's output linked to; replaces the one recorded for the lane. */ + pullRequest?: number; /** Reserved with `reserveConvoLaneGitSeq` when the command settled. */ seq: number; /** The workspace the command ran in; the write applies only while the chat is still on it. */ @@ -3161,6 +3163,7 @@ export function createConversationMethods( conversationId, laneGit, repo, + pullRequest, seq, workspace, }: { @@ -3168,15 +3171,33 @@ export function createConversationMethods( conversationId: string; laneGit: Pick, 'branch' | 'head'>; repo?: string; + pullRequest?: number; seq: number; 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: laneGit.branch, - head: laneGit.head, - ...(repo ? { repo } : {}), - seq, + 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 @@ -3233,7 +3254,7 @@ export function createConversationMethods( ...workspaceFence, ], }, - { $set: { laneGit: next } }, + [{ $set: { laneGit: next } }], { timestamps: false }, ), ); @@ -3254,8 +3275,13 @@ export function createConversationMethods( .select('laneGit') .lean | null>(); if (stored?.laneGit == null) return null; - const { branch, head, repo } = stored.laneGit; - return { branch, head, ...(repo ? { repo } : {}) }; + const { branch, head, repo, pullRequest } = stored.laneGit; + return { + branch, + head, + ...(repo ? { repo } : {}), + ...(pullRequest != null ? { pullRequest } : {}), + }; } /** @@ -3279,8 +3305,18 @@ export function createConversationMethods( .lean>>(); return stored.flatMap(({ conversationId, laneGit }) => { if (laneGit == null || conversationId == null) return []; - const { branch, head, repo } = laneGit; - return [{ conversationId, laneGit: { branch, head, ...(repo ? { repo } : {}) } }]; + const { branch, head, repo, pullRequest } = laneGit; + return [ + { + conversationId, + laneGit: { + branch, + head, + ...(repo ? { repo } : {}), + ...(pullRequest != null ? { pullRequest } : {}), + }, + }, + ]; }); } diff --git a/packages/data-schemas/src/schema/convo.ts b/packages/data-schemas/src/schema/convo.ts index 8aa5f4d6036..3a24aae0f5a 100644 --- a/packages/data-schemas/src/schema/convo.ts +++ b/packages/data-schemas/src/schema/convo.ts @@ -54,6 +54,8 @@ const convoSchema: Schema = new Schema( branch: { type: String, default: null }, head: { type: String, default: null }, repo: { type: String, default: undefined }, + /** Pull request number a command's output linked to on this branch and repository. */ + pullRequest: { type: Number, default: undefined }, /** Sequence number reserved when the command settled; fences out an older report. */ seq: { type: Number, default: undefined }, }, diff --git a/packages/data-schemas/src/types/convo.ts b/packages/data-schemas/src/types/convo.ts index 6279fd0c591..a50357e0f00 100644 --- a/packages/data-schemas/src/types/convo.ts +++ b/packages/data-schemas/src/types/convo.ts @@ -27,6 +27,9 @@ export interface IConversationLaneGit { head: string | null; /** `owner/name` of the lane's repository, as its worker registered it. */ repo?: string; + /** A pull request of `repo` a command's output linked to while the lane was on `branch`. Kept + * across later reports on the same branch and repository, dropped when either changes. */ + pullRequest?: number; /** Reserved sequence number; internal, never returned by `getConvoLaneGit`. */ seq?: number; }