From 170e42d0d7b7a73b8485080bd459b7474b9c97d6 Mon Sep 17 00:00:00 2001 From: Sleeyax Date: Wed, 14 Oct 2026 16:57:44 +0200 Subject: [PATCH] fix(guided-review): load the review list when glab is not installed The GitLab listing asks `glab auth status --all` for its hosts, and a glab that could not be found threw out of it, so the whole review list failed, GitHub's PRs included. A CLI that is not found now throws a `CliNotFoundError`, which the GitLab listing reads as no GitLab hosts. A glab that is there but fails, cannot be executed or times out still fails the listing as before. Reported by a GitHub-only reviewer, who worked around it by setting the glab path to `/usr/bin/true`. Co-Authored-By: Claude Opus 5.5 --- plugins/guided-review/server/forge/cli.ts | 10 +++++++--- plugins/guided-review/server/forge/github.test.ts | 3 ++- plugins/guided-review/server/forge/gitlab.test.ts | 9 ++++++++- plugins/guided-review/server/forge/gitlab.ts | 12 ++++++++++-- 4 files changed, 27 insertions(+), 7 deletions(-) diff --git a/plugins/guided-review/server/forge/cli.ts b/plugins/guided-review/server/forge/cli.ts index c5a9005..d41f038 100644 --- a/plugins/guided-review/server/forge/cli.ts +++ b/plugins/guided-review/server/forge/cli.ts @@ -26,6 +26,11 @@ type FileCall = CallOptions & { maxBytes: number }; /** Long enough for a paginated file list on a large PR, short enough that a hung call ends. */ export const DEFAULT_CLI_TIMEOUT_MS = 120_000; +/** The CLI is not where the settings say, which for a reviewer on only the other forge means it is not installed. */ +export class CliNotFoundError extends ForgeError { + override name = "CliNotFoundError"; +} + /** One forge CLI, whose failures come back as a `ForgeError` in a sentence. */ export type Cli = { text(args: readonly string[], options?: CallOptions): Promise; @@ -61,9 +66,8 @@ export function createCli(options: CliOptions): Cli { } if (result.spawnError !== null) { if (/ENOENT|EACCES/.test(result.spawnError)) { - throw new ForgeError( - `Could not run ${options.name} at "${binary}" (${result.spawnError}). Set the ${options.name} path in the Guided Review settings.`, - ); + const message = `Could not run ${options.name} at "${binary}" (${result.spawnError}). Set the ${options.name} path in the Guided Review settings.`; + throw /ENOENT/.test(result.spawnError) ? new CliNotFoundError(message) : new ForgeError(message); } throw new ForgeError(`${options.name} did not finish: ${result.spawnError}.`); } diff --git a/plugins/guided-review/server/forge/github.test.ts b/plugins/guided-review/server/forge/github.test.ts index 31f7878..1ed0dfb 100644 --- a/plugins/guided-review/server/forge/github.test.ts +++ b/plugins/guided-review/server/forge/github.test.ts @@ -3,6 +3,7 @@ import { readFileSync } from "node:fs"; import path from "node:path"; import test from "node:test"; import { fakeCommandRunner, type FakeCommandRunner, type ScriptedResult } from "../fake-command-runner.ts"; +import { CliNotFoundError } from "./cli.ts"; import { ADD_THREAD_MUTATION, AUTHORED_SEARCH_QUERY, @@ -182,7 +183,7 @@ test("a gh that cannot run points at the setting", async () => { await assert.rejects( forge.fetchChangeRequest(PR_105), - new ForgeError( + new CliNotFoundError( 'Could not run gh at "/opt/gh/bin/gh" (spawn /opt/gh/bin/gh ENOENT). Set the gh path in the Guided Review settings.', ), ); diff --git a/plugins/guided-review/server/forge/gitlab.test.ts b/plugins/guided-review/server/forge/gitlab.test.ts index 533bb12..653e87a 100644 --- a/plugins/guided-review/server/forge/gitlab.test.ts +++ b/plugins/guided-review/server/forge/gitlab.test.ts @@ -5,6 +5,7 @@ import test from "node:test"; import type { LineRef } from "../../shared/drafts.ts"; import { anchorAt } from "../anchors.ts"; import { fakeCommandRunner, type ScriptedResult } from "../fake-command-runner.ts"; +import { CliNotFoundError } from "./cli.ts"; import { createGitLabForge, parseAuthStatus, parseMergeRequestUrl, REQUEST_CHANGES_MUTATION, REVIEW_LIST_QUERY, uploadOf } from "./gitlab.ts"; import { ForgeError, type AnchorLine, type ChangeRequestRef, type DraftAnchor, type DraftTarget } from "./port.ts"; @@ -290,7 +291,7 @@ test("a glab that cannot run points at the setting", async () => { await assert.rejects( forge.matchUrl(MR_3931.url), - new ForgeError('Could not run glab at "/usr/bin/glab" (spawn /usr/bin/glab ENOENT). Set the glab path in the Guided Review settings.'), + new CliNotFoundError('Could not run glab at "/usr/bin/glab" (spawn /usr/bin/glab ENOENT). Set the glab path in the Guided Review settings.'), ); }); @@ -1219,6 +1220,12 @@ test("lists the open MRs the viewer reviews on each host glab is logged in to, a }); }); +test("lists no host when glab is not installed, so a GitHub-only reviewer's list still loads", async () => { + const { forge } = forgeReplaying([{ exitCode: null, spawnError: "spawn /usr/bin/glab ENOENT" }]); + + assert.deepEqual(await forge.listChangeRequests(), []); +}); + test("an MR reads by the viewer's own reviewer record, changed when the head commit is newer than it", async () => { const { forge } = forgeReplaying(LIST_REVIEWS); diff --git a/plugins/guided-review/server/forge/gitlab.ts b/plugins/guided-review/server/forge/gitlab.ts index ae0f735..9c09777 100644 --- a/plugins/guided-review/server/forge/gitlab.ts +++ b/plugins/guided-review/server/forge/gitlab.ts @@ -4,7 +4,7 @@ import type { Draft, DraftLocation, LineRef } from "../../shared/drafts.ts"; import type { CommandRunner } from "../command-runner.ts"; import type { Verdict } from "../../shared/submit.ts"; import type { AuthoredChangeRequest, CiState, ListedChangeRequest, ListedHost, ReviewerState, ReviewRequest } from "../../shared/inbox.ts"; -import { createCli, type Cli } from "./cli.ts"; +import { CliNotFoundError, createCli, type Cli } from "./cli.ts"; import { GITHUB_HOST } from "./github.ts"; import { CLONE_TIMEOUT_MS, COMMIT_READS_AT_ONCE, mapAtMost, MAX_BRANCH_CHANGE_REQUESTS, MAX_COMMITS, MAX_LINKED_ISSUES, latestOf, reviewStatusOf, unlistedHost, userOf } from "./common.ts"; import { SubmitSteps } from "./submit-steps.ts"; @@ -904,7 +904,15 @@ export function createGitLabForge(options: GitLabForgeOptions): Forge { }, async listChangeRequests(): Promise { - const statuses = parseAuthStatus(await glab.report(["auth", "status", "--all"])); + let report: string; + try { + report = await glab.report(["auth", "status", "--all"]); + } catch (error) { + // GitLab has no host of its own to report as unlisted, so a reviewer without glab has none. + if (error instanceof CliNotFoundError) return []; + throw error; + } + const statuses = parseAuthStatus(report); return Promise.all( statuses.map(async ({ host, loggedIn, problem }): Promise => { if (!loggedIn) {