From 98548ce2414b436c39e01ba1eabe4d47d34999c6 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Sun, 11 Oct 2026 12:44:15 +0200 Subject: [PATCH] fix: Detect Pull Request Links in Command Output for Conversation Lanes A conversation's pull request was found only by searching its branch for a PR whose head contains the recorded commit, so a single unpushed local commit hid an existing PR. The attached bash tool now hands its output to the lane recorder, which records a github.com pull request link to the lane's repository with the lane state. The link is kept across later reports on the same branch and repository and dropped when either changes. The GitHub source reads a linked PR directly and accepts it when its head is the lane branch in the lane repository; a fork's PR must carry the recorded commit. The single and batch routes both pass the link, and the batch route keys lanes by it. --- packages/api/src/code/command.ts | 10 +- packages/api/src/code/lane.spec.ts | 44 +++++++- packages/api/src/code/lane.ts | 24 +++- packages/api/src/pulls/github.spec.ts | 105 ++++++++++++++++++ packages/api/src/pulls/github.ts | 73 ++++++++---- packages/api/src/pulls/handler.spec.ts | 27 ++++- packages/api/src/pulls/handler.ts | 21 +++- packages/api/src/pulls/index.ts | 1 + packages/api/src/pulls/links.spec.ts | 44 ++++++++ packages/api/src/pulls/links.ts | 24 ++++ packages/api/src/pulls/lookup.spec.ts | 10 ++ packages/api/src/pulls/lookup.ts | 13 ++- packages/api/src/pulls/types.ts | 6 + .../src/methods/conversation.spec.ts | 76 +++++++++++++ .../data-schemas/src/methods/conversation.ts | 54 +++++++-- packages/data-schemas/src/schema/convo.ts | 2 + packages/data-schemas/src/types/convo.ts | 3 + 17 files changed, 489 insertions(+), 48 deletions(-) create mode 100644 packages/api/src/pulls/links.spec.ts create mode 100644 packages/api/src/pulls/links.ts 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; }