🔗 fix: Detect Pull Request Links in Command Output for Conversation Lanes

packages/api/src/code/command.ts

@@ -478,9 +478,9 @@ export function createAttachedWorkspaceBashTool({
   baseUrl: string;
   authHeaders: () => Promise<Record<string, string>> | Record<string, string>;
   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/<name>` 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,

packages/api/src/code/lane.spec.ts

@@ -13,8 +13,8 @@ type RecorderOptions = Parameters<typeof createRecorder>[0];
 /** Places the recorder once, as the tool does, then reports through it in call order. */
 const createLaneGitRecorder = (options: Omit<RecorderOptions, 'enabled'>) => {
   const ready = createRecorder({ enabled: true, ...options });
-  return (laneGit: Parameters<NonNullable<Awaited<typeof ready>>>[0]) =>
-    ready.then((record) => (record ? record(laneGit) : false));
+  return (laneGit: Parameters<NonNullable<Awaited<typeof ready>>>[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 () => {

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<boolean>) | undefined> {
+}): Promise<((laneGit: WorkspaceLaneGit, output?: string) => Promise<boolean>) | 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<boolean> => {
+  const write = async (
+    laneGit: WorkspaceLaneGit,
+    pullRequest: number | null,
+    reserved: Reservation,
+  ): Promise<boolean> => {
     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),
+    );
   };
 }

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<typeof createGitHubPullRequestSource>,
+    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' });
+  });
+});

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<TConversationPullRequest> => {
+        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);
     },
   };
 }