From a7d54ec1eae348f0c974975b646df4e3beb576ed Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Wed, 6 May 2026 12:37:40 -0700 Subject: [PATCH 1/7] fix(host-service): gh CLI first-class for PR/issue search; surface failures in UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drops the cloud `github_repositories` join dependency in `resolveGithubRepo`: v2 projects created without the GitHub App installed for the org had `github_repository_id = NULL`, which made `searchPullRequests` / `searchGitHubIssues` throw `BAD_REQUEST: Project has no linked GitHub repository`. The renderer's PR/issue dropdown swallowed the error as "No open pull requests", hiding the bug from users. Resolver now reads owner/name from local `projects.repoOwner`/`repoName` or parses the cloud `repoCloneUrl`, and returns the local `repoPath` for gh's cwd. Search procedures shell `gh pr list --search …` / `gh issue list --search …` first-class (matches v1 behavior), with Octokit fallback for hosts without `gh` installed/authenticated. The renderer now surfaces tRPC errors via toast and an inline selectable error in the dropdown empty-state, so similar upstream failures don't hide silently again. Plan: plans/20260506-gh-cli-first-class.md --- .../GitHubIssueLinkCommand.tsx | 54 +++-- .../PRLinkCommand/PRLinkCommand.tsx | 57 +++-- packages/host-service/src/app.ts | 7 + .../procedures/search-github-issues.ts | 115 +++++++++- .../procedures/search-pull-requests.ts | 121 +++++++++- .../shared/project-helpers.ts | 69 +++++- .../workspace-creation/utils/exec-gh.ts | 27 ++- packages/host-service/src/types.ts | 2 + .../test/helpers/createTestHost.ts | 9 + ...kspace-creation-github.integration.test.ts | 213 ++++++++++++++++++ plans/20260506-gh-cli-first-class.md | 27 +++ 11 files changed, 642 insertions(+), 59 deletions(-) create mode 100644 plans/20260506-gh-cli-first-class.md diff --git a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/GitHubIssueLinkCommand/GitHubIssueLinkCommand.tsx b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/GitHubIssueLinkCommand/GitHubIssueLinkCommand.tsx index a7502da5a5a..49254278085 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/GitHubIssueLinkCommand/GitHubIssueLinkCommand.tsx +++ b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/GitHubIssueLinkCommand/GitHubIssueLinkCommand.tsx @@ -8,10 +8,11 @@ import { CommandList, } from "@superset/ui/command"; import { Popover, PopoverContent, PopoverTrigger } from "@superset/ui/popover"; +import { toast } from "@superset/ui/sonner"; import { Tooltip, TooltipContent, TooltipTrigger } from "@superset/ui/tooltip"; import { useQuery } from "@tanstack/react-query"; import type { ReactNode } from "react"; -import { useId, useState } from "react"; +import { useEffect, useId, useRef, useState } from "react"; import { useHostUrl } from "renderer/hooks/host-service/useHostTargetUrl"; import { useDebouncedValue } from "renderer/hooks/useDebouncedValue"; import { getHostServiceClientByUrl } from "renderer/lib/host-service-client"; @@ -58,7 +59,7 @@ export function GitHubIssueLinkCommand({ const debouncedTrimmed = debouncedQuery.trim(); const isPendingDebounce = trimmedQuery !== debouncedTrimmed; - const { data, isFetching } = useQuery({ + const { data, isFetching, error } = useQuery({ queryKey: [ "workspaceCreation", "searchGitHubIssues", @@ -78,8 +79,21 @@ export function GitHubIssueLinkCommand({ }); }, enabled: !!projectId && !!hostUrl && open, + retry: false, }); + const lastToastedError = useRef(null); + useEffect(() => { + const msg = error instanceof Error ? error.message : null; + if (!msg) { + lastToastedError.current = null; + return; + } + if (lastToastedError.current === msg) return; + lastToastedError.current = msg; + toast.error(`Couldn't load issues: ${msg}`); + }, [error]); + const searchResults = data?.issues ?? []; const repoMismatch = data && "repoMismatch" in data ? data.repoMismatch : null; @@ -142,19 +156,29 @@ export function GitHubIssueLinkCommand({ {searchResults.length === 0 && ( - {isLoading - ? debouncedTrimmed - ? "Searching..." - : "Loading..." - : repoMismatch - ? `Issue URL must match ${repoMismatch}.` - : debouncedTrimmed - ? showClosed - ? "No issues found." - : "No open issues found." - : showClosed - ? "No issues found." - : "No open issues found."} + {isLoading ? ( + debouncedTrimmed ? ( + "Searching..." + ) : ( + "Loading..." + ) + ) : error instanceof Error ? ( + + {error.message} + + ) : repoMismatch ? ( + `Issue URL must match ${repoMismatch}.` + ) : debouncedTrimmed ? ( + showClosed ? ( + "No issues found." + ) : ( + "No open issues found." + ) + ) : showClosed ? ( + "No issues found." + ) : ( + "No open issues found." + )} )} {searchResults.length > 0 && ( diff --git a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx index 2870c6c1568..3d0fcca3f18 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx +++ b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx @@ -8,10 +8,11 @@ import { CommandList, } from "@superset/ui/command"; import { Popover, PopoverContent, PopoverTrigger } from "@superset/ui/popover"; +import { toast } from "@superset/ui/sonner"; import { Tooltip, TooltipContent, TooltipTrigger } from "@superset/ui/tooltip"; import { useQuery } from "@tanstack/react-query"; import type { ReactNode } from "react"; -import { useId, useState } from "react"; +import { useEffect, useId, useRef, useState } from "react"; import { useHostUrl } from "renderer/hooks/host-service/useHostTargetUrl"; import { useDebouncedValue } from "renderer/hooks/useDebouncedValue"; import { getHostServiceClientByUrl } from "renderer/lib/host-service-client"; @@ -58,7 +59,7 @@ export function PRLinkCommand({ const debouncedTrimmed = debouncedQuery.trim(); const isPendingDebounce = trimmedQuery !== debouncedTrimmed; - const { data, isFetching } = useQuery({ + const { data, isFetching, error } = useQuery({ queryKey: [ "workspaceCreation", "searchPullRequests", @@ -78,8 +79,24 @@ export function PRLinkCommand({ }); }, enabled: !!projectId && !!hostUrl && open, + retry: false, }); + // Toast once per error transition. Previously the dropdown's empty-state + // hid PR-search failures behind "No open pull requests", which made + // upstream tRPC errors invisible to users. + const lastToastedError = useRef(null); + useEffect(() => { + const msg = error instanceof Error ? error.message : null; + if (!msg) { + lastToastedError.current = null; + return; + } + if (lastToastedError.current === msg) return; + lastToastedError.current = msg; + toast.error(`Couldn't load pull requests: ${msg}`); + }, [error]); + const pullRequests = data?.pullRequests ?? []; const repoMismatch = data && "repoMismatch" in data ? data.repoMismatch : null; @@ -142,19 +159,29 @@ export function PRLinkCommand({ {pullRequests.length === 0 && ( - {isLoading - ? debouncedTrimmed - ? "Searching..." - : "Loading..." - : repoMismatch - ? `PR URL must match ${repoMismatch}.` - : debouncedTrimmed - ? showClosed - ? "No pull requests found." - : "No open pull requests found." - : showClosed - ? "No pull requests found." - : "No open pull requests."} + {isLoading ? ( + debouncedTrimmed ? ( + "Searching..." + ) : ( + "Loading..." + ) + ) : error instanceof Error ? ( + + {error.message} + + ) : repoMismatch ? ( + `PR URL must match ${repoMismatch}.` + ) : debouncedTrimmed ? ( + showClosed ? ( + "No pull requests found." + ) : ( + "No open pull requests found." + ) + ) : showClosed ? ( + "No pull requests found." + ) : ( + "No open pull requests." + )} )} {pullRequests.length > 0 && ( diff --git a/packages/host-service/src/app.ts b/packages/host-service/src/app.ts index 554cb11fe70..a2fd4f02730 100644 --- a/packages/host-service/src/app.ts +++ b/packages/host-service/src/app.ts @@ -19,6 +19,10 @@ import { runMainWorkspaceSweep } from "./runtime/main-workspace-sweep"; import { PullRequestRuntimeManager } from "./runtime/pull-requests"; import { registerWorkspaceTerminalRoute } from "./terminal/terminal"; import { appRouter } from "./trpc/router"; +import { + execGh as defaultExecGh, + type ExecGh, +} from "./trpc/router/workspace-creation/utils/exec-gh"; import type { ApiClient } from "./types"; export interface CreateAppOptions { @@ -46,6 +50,7 @@ export interface CreateAppOptions { db?: HostDb; api?: ApiClient; github?: () => Promise; + execGh?: ExecGh; chatRuntime?: ChatRuntimeManager; chatService?: ChatService; } @@ -76,6 +81,7 @@ export function createApp(options: CreateAppOptions): CreateAppResult { } return new Octokit({ auth: token }); }); + const execGh: ExecGh = options.execGh ?? defaultExecGh; const filesystem = new WorkspaceFilesystemManager({ db }); // GitWatcher is the single source of truth for `.git/` and worktree fs @@ -161,6 +167,7 @@ export function createApp(options: CreateAppOptions): CreateAppResult { return { git, github, + execGh, api, db, runtime, diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/search-github-issues.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/search-github-issues.ts index 989066ed357..b28fedc4842 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/search-github-issues.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/search-github-issues.ts @@ -1,7 +1,87 @@ +import { z } from "zod"; import { protectedProcedure } from "../../../index"; import { normalizeGitHubQuery } from "../normalize-github-query"; import { githubSearchInputSchema } from "../schemas"; -import { resolveGithubRepo } from "../shared/project-helpers"; +import { + type ResolvedGithubRepo, + resolveGithubRepo, +} from "../shared/project-helpers"; +import type { ExecGh } from "../utils/exec-gh"; + +interface IssueResult { + issueNumber: number; + title: string; + url: string; + state: string; + authorLogin: string | null; +} + +const ghIssueSchema = z.object({ + number: z.number(), + title: z.string(), + url: z.string(), + state: z.string(), + author: z.object({ login: z.string() }).nullable().optional(), +}); + +const ISSUE_JSON_FIELDS = "number,title,url,state,author"; + +function fromGhIssue(issue: z.infer): IssueResult { + return { + issueNumber: issue.number, + title: issue.title, + url: issue.url, + state: issue.state.toLowerCase(), + authorLogin: issue.author?.login ?? null, + }; +} + +async function ghDirectLookup( + execGh: ExecGh, + repo: ResolvedGithubRepo, + issueNumber: number, +): Promise { + const raw = await execGh( + [ + "issue", + "view", + String(issueNumber), + "--repo", + `${repo.owner}/${repo.name}`, + "--json", + ISSUE_JSON_FIELDS, + ], + { cwd: repo.repoPath ?? undefined }, + ); + return fromGhIssue(ghIssueSchema.parse(raw)); +} + +async function ghSearch( + execGh: ExecGh, + repo: ResolvedGithubRepo, + query: string, + includeClosed: boolean, + limit: number, +): Promise { + const args = [ + "issue", + "list", + "--repo", + `${repo.owner}/${repo.name}`, + "--state", + includeClosed ? "all" : "open", + "--limit", + String(limit), + "--json", + ISSUE_JSON_FIELDS, + ]; + if (query) { + args.push("--search", query); + } + const raw = await execGh(args, { cwd: repo.repoPath ?? undefined }); + const arr = z.array(ghIssueSchema).parse(raw); + return arr.map(fromGhIssue); +} export const searchGitHubIssues = protectedProcedure .input(githubSearchInputSchema) @@ -9,7 +89,6 @@ export const searchGitHubIssues = protectedProcedure const repo = await resolveGithubRepo(ctx, input.projectId); const limit = input.limit ?? 30; - // Normalize the query: detect GitHub issue URLs, strip `#` shorthand const raw = input.query?.trim() ?? ""; const normalized = normalizeGitHubQuery(raw, repo, "issue"); @@ -21,10 +100,34 @@ export const searchGitHubIssues = protectedProcedure } const effectiveQuery = normalized.query; + + try { + if (normalized.isDirectLookup) { + const issueNumber = Number.parseInt(effectiveQuery, 10); + const issue = await ghDirectLookup(ctx.execGh, repo, issueNumber); + if (!input.includeClosed && issue.state !== "open") { + return { issues: [] }; + } + return { issues: [issue] }; + } + const issues = await ghSearch( + ctx.execGh, + repo, + effectiveQuery, + input.includeClosed ?? false, + limit, + ); + return { issues }; + } catch (ghErr) { + console.warn( + "[workspaceCreation.searchGitHubIssues] gh path failed; falling back to Octokit", + ghErr, + ); + } + const octokit = await ctx.github(); try { - // Direct lookup by issue number (from URL paste or `#123` shorthand) if (normalized.isDirectLookup) { const issueNumber = Number.parseInt(effectiveQuery, 10); const { data: issue } = await octokit.issues.get({ @@ -32,7 +135,6 @@ export const searchGitHubIssues = protectedProcedure repo: repo.name, issue_number: issueNumber, }); - // issues.get returns PRs too - filter them out if (issue.pull_request) { return { issues: [] }; } @@ -73,7 +175,10 @@ export const searchGitHubIssues = protectedProcedure })), }; } catch (err) { - console.warn("[workspaceCreation.searchGitHubIssues] failed", err); + console.warn( + "[workspaceCreation.searchGitHubIssues] octokit fallback failed", + err, + ); return { issues: [] }; } }); diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts index d790b560a14..73eaa34c5d6 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts @@ -1,7 +1,21 @@ +import { z } from "zod"; import { protectedProcedure } from "../../../index"; import { normalizeGitHubQuery } from "../normalize-github-query"; import { githubSearchInputSchema } from "../schemas"; -import { resolveGithubRepo } from "../shared/project-helpers"; +import { + type ResolvedGithubRepo, + resolveGithubRepo, +} from "../shared/project-helpers"; +import type { ExecGh } from "../utils/exec-gh"; + +interface PullRequestResult { + prNumber: number; + title: string; + url: string; + state: "open" | "closed" | "merged"; + isDraft: boolean; + authorLogin: string | null; +} function normalizePullRequestState( state: string, @@ -11,13 +25,82 @@ function normalizePullRequestState( return state.toLowerCase() === "closed" ? "closed" : "open"; } +const ghPrSchema = z.object({ + number: z.number(), + title: z.string(), + url: z.string(), + state: z.string(), + isDraft: z.boolean().optional(), + author: z.object({ login: z.string() }).nullable().optional(), + mergedAt: z.string().nullable().optional(), +}); + +function fromGhPr(pr: z.infer): PullRequestResult { + return { + prNumber: pr.number, + title: pr.title, + url: pr.url, + state: normalizePullRequestState(pr.state, pr.mergedAt), + isDraft: pr.isDraft ?? false, + authorLogin: pr.author?.login ?? null, + }; +} + +const PR_JSON_FIELDS = "number,title,url,state,isDraft,author,mergedAt"; + +async function ghDirectLookup( + execGh: ExecGh, + repo: ResolvedGithubRepo, + prNumber: number, +): Promise { + const raw = await execGh( + [ + "pr", + "view", + String(prNumber), + "--repo", + `${repo.owner}/${repo.name}`, + "--json", + PR_JSON_FIELDS, + ], + { cwd: repo.repoPath ?? undefined }, + ); + return fromGhPr(ghPrSchema.parse(raw)); +} + +async function ghSearch( + execGh: ExecGh, + repo: ResolvedGithubRepo, + query: string, + includeClosed: boolean, + limit: number, +): Promise { + const args = [ + "pr", + "list", + "--repo", + `${repo.owner}/${repo.name}`, + "--state", + includeClosed ? "all" : "open", + "--limit", + String(limit), + "--json", + PR_JSON_FIELDS, + ]; + if (query) { + args.push("--search", query); + } + const raw = await execGh(args, { cwd: repo.repoPath ?? undefined }); + const arr = z.array(ghPrSchema).parse(raw); + return arr.map(fromGhPr); +} + export const searchPullRequests = protectedProcedure .input(githubSearchInputSchema) .query(async ({ ctx, input }) => { const repo = await resolveGithubRepo(ctx, input.projectId); const limit = input.limit ?? 30; - // Normalize the query: detect GitHub PR URLs, strip `#` shorthand const raw = input.query?.trim() ?? ""; const normalized = normalizeGitHubQuery(raw, repo, "pull"); @@ -29,10 +112,37 @@ export const searchPullRequests = protectedProcedure } const effectiveQuery = normalized.query; + + // First-class: shell to user's `gh` — no cloud dep, picks up local + // `gh auth login`. Falls back to Octokit if gh isn't installed, + // isn't authenticated, or otherwise errors. + try { + if (normalized.isDirectLookup) { + const prNumber = Number.parseInt(effectiveQuery, 10); + const pr = await ghDirectLookup(ctx.execGh, repo, prNumber); + if (!input.includeClosed && pr.state !== "open") { + return { pullRequests: [] }; + } + return { pullRequests: [pr] }; + } + const pullRequests = await ghSearch( + ctx.execGh, + repo, + effectiveQuery, + input.includeClosed ?? false, + limit, + ); + return { pullRequests }; + } catch (ghErr) { + console.warn( + "[workspaceCreation.searchPullRequests] gh path failed; falling back to Octokit", + ghErr, + ); + } + const octokit = await ctx.github(); try { - // Direct lookup by PR number (from URL paste or `#123` shorthand) if (normalized.isDirectLookup) { const prNumber = Number.parseInt(effectiveQuery, 10); const { data: pr } = await octokit.pulls.get({ @@ -86,7 +196,10 @@ export const searchPullRequests = protectedProcedure }), }; } catch (err) { - console.warn("[workspaceCreation.searchPullRequests] failed", err); + console.warn( + "[workspaceCreation.searchPullRequests] octokit fallback failed", + err, + ); return { pullRequests: [] }; } }); diff --git a/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts b/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts index 1ec4c6b46b5..5ca4526525a 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts @@ -1,4 +1,7 @@ +import { parseGitHubRemote } from "@superset/shared/github-remote"; import { TRPCError } from "@trpc/server"; +import { eq } from "drizzle-orm"; +import { projects } from "../../../../db/schema"; import type { HostServiceContext } from "../../../../types"; import type { ProjectNotSetupCause } from "../../../error-types"; @@ -13,20 +16,70 @@ export function projectNotSetupError(projectId: string): TRPCError { }); } +export interface ResolvedGithubRepo { + owner: string; + name: string; + /** Local clone path; present iff the project is set up on this host. */ + repoPath: string | null; +} + +/** + * Resolve `{owner, name}` for a project, plus the local `repoPath` when the + * project is set up on this machine. Tries the host-side `projects` row first + * (no cloud round-trip, gives us a `cwd` for first-class `gh` calls) and + * falls back to parsing the cloud `repoCloneUrl`. + * + * Intentionally does NOT depend on the cloud `github_repositories` join: that + * row only exists if the GitHub App was installed for the org at create-time, + * and projects created any other way would otherwise be unreachable for + * PR/issue search even when `gh` would handle them just fine. + */ export async function resolveGithubRepo( ctx: HostServiceContext, projectId: string, -): Promise<{ owner: string; name: string }> { +): Promise { + const local = ctx.db.query.projects + .findFirst({ where: eq(projects.id, projectId) }) + .sync(); + + if (local?.repoOwner && local.repoName) { + return { + owner: local.repoOwner, + name: local.repoName, + repoPath: local.repoPath, + }; + } + + // `repoUrl` is set, but owner/name weren't backfilled — parse it. + if (local?.repoUrl) { + const parsed = parseGitHubRemote(local.repoUrl); + if (parsed) { + return { + owner: parsed.owner, + name: parsed.name, + repoPath: local.repoPath, + }; + } + } + const cloudProject = await ctx.api.v2Project.get.query({ organizationId: ctx.organizationId, id: projectId, }); - const repo = cloudProject.githubRepository; - if (!repo?.owner || !repo?.name) { - throw new TRPCError({ - code: "BAD_REQUEST", - message: "Project has no linked GitHub repository", - }); + + if (cloudProject.repoCloneUrl) { + const parsed = parseGitHubRemote(cloudProject.repoCloneUrl); + if (parsed) { + return { + owner: parsed.owner, + name: parsed.name, + repoPath: local?.repoPath ?? null, + }; + } } - return { owner: repo.owner, name: repo.name }; + + throw new TRPCError({ + code: "BAD_REQUEST", + message: "Project has no resolvable GitHub repository", + }); } diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts index 765ee396507..f50e62b05c1 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts @@ -4,20 +4,23 @@ import { getStrictShellEnvironment } from "../../../../terminal/clean-shell-env" const execFileAsync = promisify(execFile); +export interface ExecGhOptions { + cwd?: string; + timeout?: number; +} + /** - * Shell out to the user's `gh` CLI. Uses the user's existing gh - * authentication (`gh auth login`), which is simpler than octokit + - * credential-manager plumbing and matches V1's behavior for - * getIssueContent. - * - * Returns parsed JSON output if stdout is JSON (typical for `--json` - * queries). For commands that don't return JSON (e.g., `gh pr checkout`), - * returns the trimmed stdout string. Throws on non-zero exit. + * Shell out to the user's `gh` CLI. Uses the user's existing `gh auth login`, + * which avoids credential-manager plumbing and matches V1's behavior. Returns + * parsed JSON when stdout looks like JSON, otherwise the trimmed string. + * Throws on non-zero exit (caller treats that as an opportunity to fall back). */ -export async function execGh( +export type ExecGh = ( args: string[], - options?: { cwd?: string; timeout?: number }, -): Promise { + options?: ExecGhOptions, +) => Promise; + +export const execGh: ExecGh = async (args, options) => { const env = await getStrictShellEnvironment().catch( () => process.env as Record, ); @@ -34,4 +37,4 @@ export async function execGh( } catch { return trimmed; } -} +}; diff --git a/packages/host-service/src/types.ts b/packages/host-service/src/types.ts index cfd5adb3bb9..6a054eb2a0c 100644 --- a/packages/host-service/src/types.ts +++ b/packages/host-service/src/types.ts @@ -8,6 +8,7 @@ import type { ChatRuntimeManager } from "./runtime/chat"; import type { WorkspaceFilesystemManager } from "./runtime/filesystem"; import type { GitFactory } from "./runtime/git"; import type { PullRequestRuntimeManager } from "./runtime/pull-requests"; +import type { ExecGh } from "./trpc/router/workspace-creation/utils/exec-gh"; export type ApiClient = TRPCClient; @@ -21,6 +22,7 @@ export interface HostServiceRuntime { export interface HostServiceContext { git: GitFactory; github: () => Promise; + execGh: ExecGh; api: ApiClient; db: HostDb; runtime: HostServiceRuntime; diff --git a/packages/host-service/test/helpers/createTestHost.ts b/packages/host-service/test/helpers/createTestHost.ts index 8f1a50c0a5d..d1349aabe1b 100644 --- a/packages/host-service/test/helpers/createTestHost.ts +++ b/packages/host-service/test/helpers/createTestHost.ts @@ -38,6 +38,7 @@ export interface TestHostOptions { * ChatRuntimeManager, ChatService) are far too large to stub fully. */ githubFactory?: () => Promise; + execGh?: (args: string[], options?: unknown) => Promise; chatRuntime?: unknown; chatService?: unknown; } @@ -108,6 +109,14 @@ export async function createTestHost( github: options.githubFactory ? (options.githubFactory as CreateAppOptions["github"]) : undefined, + execGh: options.execGh + ? (options.execGh as CreateAppOptions["execGh"]) + : // Default to a stub that always rejects, so tests that don't set + // up `execGh` exercise the Octokit fallback instead of accidentally + // shelling out to a real `gh` on the host. + async () => { + throw new Error("execGh not configured in test"); + }, chatRuntime: options.chatRuntime as CreateAppOptions["chatRuntime"], chatService: options.chatService as CreateAppOptions["chatService"], }; diff --git a/packages/host-service/test/integration/workspace-creation-github.integration.test.ts b/packages/host-service/test/integration/workspace-creation-github.integration.test.ts index e08176e159c..3538f07c50a 100644 --- a/packages/host-service/test/integration/workspace-creation-github.integration.test.ts +++ b/packages/host-service/test/integration/workspace-creation-github.integration.test.ts @@ -72,6 +72,7 @@ describe("workspaceCreation github procedures with mocked Octokit", () => { "v2Project.get.query": () => ({ id: projectId, githubRepository: { owner: "octocat", name: "hello" }, + repoCloneUrl: "https://github.com/octocat/hello.git", }), }, }); @@ -141,3 +142,215 @@ describe("workspaceCreation github procedures with mocked Octokit", () => { expect(calls[0].method).toBe("search.issuesAndPullRequests"); }); }); + +describe("project has repoCloneUrl but no linked githubRepository (cloud-dep regression)", () => { + let host: TestHost; + const projectId = randomUUID(); + + const fakeOctokit = { + pulls: { + get: async () => ({ + data: { + number: 33, + title: "PR #33", + html_url: "https://github.com/octocat/hello/pull/33", + state: "open", + user: { login: "bob" }, + draft: false, + merged_at: null, + }, + }), + }, + issues: { + get: async () => ({ + data: { + number: 42, + title: "Issue #42", + html_url: "https://github.com/octocat/hello/issues/42", + state: "open", + user: { login: "alice" }, + pull_request: undefined, + }, + }), + }, + search: { + issuesAndPullRequests: async () => ({ data: { items: [] } }), + }, + }; + + beforeEach(async () => { + // Cloud project has repoCloneUrl but githubRepositoryId is NULL — + // matches the prod failure mode that prompted this fix. Resolver must + // parse repoCloneUrl rather than depending on the github_repositories + // join. + host = await createTestHost({ + githubFactory: async () => fakeOctokit, + apiOverrides: { + "v2Project.get.query": () => ({ + id: projectId, + githubRepository: null, + repoCloneUrl: "https://github.com/octocat/hello.git", + }), + }, + }); + }); + + afterEach(async () => { + await host.dispose(); + }); + + test("searchPullRequests resolves owner/name from repoCloneUrl and returns a result", async () => { + const result = await host.trpc.workspaceCreation.searchPullRequests.query({ + projectId, + query: "#33", + }); + expect(result.pullRequests).toHaveLength(1); + expect(result.pullRequests[0].prNumber).toBe(33); + }); + + test("searchGitHubIssues resolves owner/name from repoCloneUrl without throwing", async () => { + const result = await host.trpc.workspaceCreation.searchGitHubIssues.query({ + projectId, + query: "anything", + }); + expect(result.issues).toEqual([]); + }); +}); + +describe("gh CLI is first-class when execGh succeeds", () => { + let host: TestHost; + const projectId = randomUUID(); + const ghCalls: Array<{ args: string[]; cwd?: string }> = []; + + const fakeOctokit = { + // Octokit must NOT be hit when gh succeeds. Throwing here makes any + // accidental fallback fail the test loudly. + pulls: { + get: async () => { + throw new Error("octokit must not be called when gh succeeds"); + }, + }, + issues: { + get: async () => { + throw new Error("octokit must not be called when gh succeeds"); + }, + }, + search: { + issuesAndPullRequests: async () => { + throw new Error("octokit must not be called when gh succeeds"); + }, + }, + }; + + const fakeExecGh = async ( + args: string[], + options?: { cwd?: string }, + ): Promise => { + ghCalls.push({ args, cwd: options?.cwd }); + // `pr view ` returns a single object, `pr list` returns an array; + // match against the verb to pick the right shape. + const verb = args[1]; + if (verb === "view" && args[0] === "pr") { + return { + number: Number(args[2]), + title: "PR via gh", + url: `https://github.com/octocat/hello/pull/${args[2]}`, + state: "OPEN", + isDraft: false, + author: { login: "bob" }, + mergedAt: null, + }; + } + if (verb === "list" && args[0] === "pr") { + return [ + { + number: 101, + title: "search result", + url: "https://github.com/octocat/hello/pull/101", + state: "OPEN", + isDraft: false, + author: { login: "carol" }, + mergedAt: null, + }, + ]; + } + if (verb === "list" && args[0] === "issue") { + return [ + { + number: 7, + title: "issue search result", + url: "https://github.com/octocat/hello/issues/7", + state: "OPEN", + author: { login: "dave" }, + }, + ]; + } + return {}; + }; + + beforeEach(async () => { + ghCalls.length = 0; + host = await createTestHost({ + githubFactory: async () => fakeOctokit, + execGh: fakeExecGh, + apiOverrides: { + "v2Project.get.query": () => ({ + id: projectId, + githubRepository: null, + repoCloneUrl: "https://github.com/octocat/hello.git", + }), + }, + }); + }); + + afterEach(async () => { + await host.dispose(); + }); + + test("searchPullRequests #N invokes `gh pr view` against the resolved repo", async () => { + const result = await host.trpc.workspaceCreation.searchPullRequests.query({ + projectId, + query: "#33", + }); + expect(result.pullRequests).toHaveLength(1); + expect(result.pullRequests[0].prNumber).toBe(33); + expect(result.pullRequests[0].title).toBe("PR via gh"); + expect(ghCalls).toHaveLength(1); + expect(ghCalls[0].args.slice(0, 5)).toEqual([ + "pr", + "view", + "33", + "--repo", + "octocat/hello", + ]); + }); + + test("searchPullRequests free-text invokes `gh pr list --search`", async () => { + const result = await host.trpc.workspaceCreation.searchPullRequests.query({ + projectId, + query: "find me", + }); + expect(result.pullRequests).toHaveLength(1); + expect(result.pullRequests[0].prNumber).toBe(101); + expect(ghCalls).toHaveLength(1); + const args = ghCalls[0].args; + expect(args[0]).toBe("pr"); + expect(args[1]).toBe("list"); + expect(args).toContain("--repo"); + expect(args).toContain("octocat/hello"); + expect(args).toContain("--search"); + expect(args).toContain("find me"); + }); + + test("searchGitHubIssues free-text invokes `gh issue list --search`", async () => { + const result = await host.trpc.workspaceCreation.searchGitHubIssues.query({ + projectId, + query: "bug", + }); + expect(result.issues).toHaveLength(1); + expect(result.issues[0].issueNumber).toBe(7); + expect(ghCalls).toHaveLength(1); + expect(ghCalls[0].args[0]).toBe("issue"); + expect(ghCalls[0].args[1]).toBe("list"); + }); +}); diff --git a/plans/20260506-gh-cli-first-class.md b/plans/20260506-gh-cli-first-class.md new file mode 100644 index 00000000000..25a7eddca2b --- /dev/null +++ b/plans/20260506-gh-cli-first-class.md @@ -0,0 +1,27 @@ +# gh CLI first-class for GitHub access + +## Why + +`workspaceCreation.searchPullRequests` (and `searchGitHubIssues`) throw `BAD_REQUEST: Project has no linked GitHub repository` when the cloud project's `githubRepositoryId` is NULL. The `github_repositories` row only exists if the GitHub App was installed for the org at project-create time, so any project created outside that path is broken. v1 dodged this entirely by shelling to `gh` against the local repo. + +Goal: make `gh` first-class, parsed `repoCloneUrl` as the owner/name fallback, and drop the cloud-side dep. + +## Scope (this PR) + +1. `execGh` becomes injectable on `CreateAppOptions` (parallels `github` factory) so the gh path is testable. +2. `resolveGithubRepo` returns `{ owner, name, repoPath? }`, sourcing owner/name from cloud `repoCloneUrl` (parsed) instead of the `github_repositories` join. Looks up local `projects.repoPath` by `projectId` for first-class `gh`. +3. `searchPullRequests` + `searchGitHubIssues`: gh first-class (`cwd: repoPath`) when `repoPath` and `gh` are available; Octokit fallback otherwise. Same response shape. +4. Tests: existing repro tests go green via Octokit fallback. Add a first-class-path test that injects a fake `execGh` and asserts `gh pr list --search …` is invoked with `cwd=`. + +## Out of scope (follow-up PRs) + +- Migrate `getPullRequestThreads`, the pull-requests runtime poller, `mergePR` to gh. +- Delete the 7 unused `github.*` endpoints (`getPRStatus`/`getPR`/`listPRs`/`getRepo`/`listDeployments`/`listDeploymentStatuses`/`getUser`). +- Remove `Octokit` factory + `@octokit/rest` dep once nothing references it. + +## Smoke test + +1. Repro with a project whose cloud `githubRepositoryId` is NULL — search PR errors today. +2. After fix: search returns results. +3. Move `gh` aside (`mv $(which gh) /tmp/gh.bak`) → fallback still works (Octokit). +4. Project with linked `githubRepository` keeps working. From 3f11471ee3efcd86dfb244c7f590b70dd0264e32 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Wed, 6 May 2026 12:56:54 -0700 Subject: [PATCH 2/7] fix(host-service): resolve GitHub owner/name from live local git remote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolveGithubRepo` previously fell back to parsing `cloudProject.repoCloneUrl` when the local DB row's cached owner/name were missing. Both the cloud value and the cached columns are setup-time snapshots that drift (rename, fork, manual remote re-point); GitHub queries must always target wherever the local remote points right now. Resolver now reads `simpleGit(repoPath).getRemotes(true)` via the existing `resolveLocalRepo` helper and parses the live URL. Throws PROJECT_NOT_SETUP when the project has no local clone on this host (search isn't meaningful without one) and BAD_REQUEST when the local clone has no GitHub remote. Tests now seed real temp git working trees with `git remote add origin` instead of stubbing `v2Project.get.query` — only a real `.git` exercises the actual `git remote get-url` path. New test asserts cloud-vs-local divergence: when cloud says one repo and the local remote says another, the resolver follows the local remote. --- .../shared/project-helpers.ts | 71 +++------ ...kspace-creation-github.integration.test.ts | 138 ++++++++++++++---- 2 files changed, 130 insertions(+), 79 deletions(-) diff --git a/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts b/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts index 5ca4526525a..a9fc812bf6b 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts @@ -1,9 +1,9 @@ -import { parseGitHubRemote } from "@superset/shared/github-remote"; import { TRPCError } from "@trpc/server"; import { eq } from "drizzle-orm"; import { projects } from "../../../../db/schema"; import type { HostServiceContext } from "../../../../types"; import type { ProjectNotSetupCause } from "../../../error-types"; +import { resolveLocalRepo } from "../../project/utils/resolve-repo"; export function projectNotSetupError(projectId: string): TRPCError { return new TRPCError({ @@ -19,20 +19,19 @@ export function projectNotSetupError(projectId: string): TRPCError { export interface ResolvedGithubRepo { owner: string; name: string; - /** Local clone path; present iff the project is set up on this host. */ - repoPath: string | null; + /** Local clone path. Required — searches operate against the local clone. */ + repoPath: string; } /** - * Resolve `{owner, name}` for a project, plus the local `repoPath` when the - * project is set up on this machine. Tries the host-side `projects` row first - * (no cloud round-trip, gives us a `cwd` for first-class `gh` calls) and - * falls back to parsing the cloud `repoCloneUrl`. + * Resolve `{owner, name, repoPath}` for a project by reading the **live** + * GitHub remote of the local clone. Both the cloud `repoCloneUrl` and the + * cached `projects.repoOwner`/`repoName` columns are setup-time snapshots + * that can drift (rename, fork, manual remote re-point); GitHub queries + * must always target wherever the user's actual remote points right now. * - * Intentionally does NOT depend on the cloud `github_repositories` join: that - * row only exists if the GitHub App was installed for the org at create-time, - * and projects created any other way would otherwise be unreachable for - * PR/issue search even when `gh` would handle them just fine. + * Throws `PROJECT_NOT_SETUP` if the project has no local clone on this + * host, or `BAD_REQUEST` if the local clone has no GitHub remote. */ export async function resolveGithubRepo( ctx: HostServiceContext, @@ -41,45 +40,21 @@ export async function resolveGithubRepo( const local = ctx.db.query.projects .findFirst({ where: eq(projects.id, projectId) }) .sync(); - - if (local?.repoOwner && local.repoName) { - return { - owner: local.repoOwner, - name: local.repoName, - repoPath: local.repoPath, - }; - } - - // `repoUrl` is set, but owner/name weren't backfilled — parse it. - if (local?.repoUrl) { - const parsed = parseGitHubRemote(local.repoUrl); - if (parsed) { - return { - owner: parsed.owner, - name: parsed.name, - repoPath: local.repoPath, - }; - } + if (!local?.repoPath) { + throw projectNotSetupError(projectId); } - const cloudProject = await ctx.api.v2Project.get.query({ - organizationId: ctx.organizationId, - id: projectId, - }); - - if (cloudProject.repoCloneUrl) { - const parsed = parseGitHubRemote(cloudProject.repoCloneUrl); - if (parsed) { - return { - owner: parsed.owner, - name: parsed.name, - repoPath: local?.repoPath ?? null, - }; - } + const resolved = await resolveLocalRepo(local.repoPath); + if (!resolved.parsed) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: `Repository at ${local.repoPath} has no GitHub remote.`, + }); } - throw new TRPCError({ - code: "BAD_REQUEST", - message: "Project has no resolvable GitHub repository", - }); + return { + owner: resolved.parsed.owner, + name: resolved.parsed.name, + repoPath: resolved.repoPath, + }; } diff --git a/packages/host-service/test/integration/workspace-creation-github.integration.test.ts b/packages/host-service/test/integration/workspace-creation-github.integration.test.ts index 3538f07c50a..4d3837f8a14 100644 --- a/packages/host-service/test/integration/workspace-creation-github.integration.test.ts +++ b/packages/host-service/test/integration/workspace-creation-github.integration.test.ts @@ -1,9 +1,46 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { randomUUID } from "node:crypto"; +import { mkdtempSync, realpathSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import simpleGit from "simple-git"; +import { projects } from "../../src/db/schema"; import { createTestHost, type TestHost } from "../helpers/createTestHost"; +/** + * Set up a real temp git working tree with a configured GitHub remote and + * insert a host-side `projects` row pointing at it. The PR/issue search + * procedures resolve owner/name by reading the live remote of this clone, + * so tests need a real `.git` here — no fakery substitutes for the actual + * `git remote get-url`. + */ +async function seedRepoFixture( + host: TestHost, + projectId: string, + remoteUrl: string, +): Promise { + const dir = mkdtempSync(join(tmpdir(), "ws-creation-github-test-")); + const git = simpleGit(dir); + await git.init(["--initial-branch=main"]); + await git.addRemote("origin", remoteUrl); + host.db + .insert(projects) + .values({ + id: projectId, + repoPath: dir, + repoProvider: "github", + repoOwner: null, + repoName: null, + repoUrl: null, + remoteName: "origin", + }) + .run(); + return dir; +} + describe("workspaceCreation github procedures with mocked Octokit", () => { let host: TestHost; + let repoDir: string; const calls: Array<{ method: string; args: unknown }> = []; const fakeOctokit = { @@ -66,20 +103,17 @@ describe("workspaceCreation github procedures with mocked Octokit", () => { beforeEach(async () => { calls.length = 0; - host = await createTestHost({ - githubFactory: async () => fakeOctokit, - apiOverrides: { - "v2Project.get.query": () => ({ - id: projectId, - githubRepository: { owner: "octocat", name: "hello" }, - repoCloneUrl: "https://github.com/octocat/hello.git", - }), - }, - }); + host = await createTestHost({ githubFactory: async () => fakeOctokit }); + repoDir = await seedRepoFixture( + host, + projectId, + "https://github.com/octocat/hello.git", + ); }); afterEach(async () => { await host.dispose(); + rmSync(repoDir, { recursive: true, force: true }); }); test("searchGitHubIssues handles direct #123 lookup via issues.get", async () => { @@ -143,8 +177,9 @@ describe("workspaceCreation github procedures with mocked Octokit", () => { }); }); -describe("project has repoCloneUrl but no linked githubRepository (cloud-dep regression)", () => { +describe("resolveGithubRepo trusts the live local remote, never the cloud", () => { let host: TestHost; + let repoDir: string; const projectId = randomUUID(); const fakeOctokit = { @@ -153,7 +188,7 @@ describe("project has repoCloneUrl but no linked githubRepository (cloud-dep reg data: { number: 33, title: "PR #33", - html_url: "https://github.com/octocat/hello/pull/33", + html_url: "https://github.com/cli/cli/pull/33", state: "open", user: { login: "bob" }, draft: false, @@ -166,7 +201,7 @@ describe("project has repoCloneUrl but no linked githubRepository (cloud-dep reg data: { number: 42, title: "Issue #42", - html_url: "https://github.com/octocat/hello/issues/42", + html_url: "https://github.com/cli/cli/issues/42", state: "open", user: { login: "alice" }, pull_request: undefined, @@ -179,36 +214,44 @@ describe("project has repoCloneUrl but no linked githubRepository (cloud-dep reg }; beforeEach(async () => { - // Cloud project has repoCloneUrl but githubRepositoryId is NULL — - // matches the prod failure mode that prompted this fix. Resolver must - // parse repoCloneUrl rather than depending on the github_repositories - // join. host = await createTestHost({ githubFactory: async () => fakeOctokit, + // Cloud says one repo (`somewhere/else`)… apiOverrides: { "v2Project.get.query": () => ({ id: projectId, githubRepository: null, - repoCloneUrl: "https://github.com/octocat/hello.git", + repoCloneUrl: "https://github.com/somewhere/else.git", }), }, }); + // …but the local remote points at `cli/cli`. Resolver MUST follow the + // local remote, not the cloud value. + repoDir = await seedRepoFixture( + host, + projectId, + "https://github.com/cli/cli.git", + ); }); afterEach(async () => { await host.dispose(); + rmSync(repoDir, { recursive: true, force: true }); }); - test("searchPullRequests resolves owner/name from repoCloneUrl and returns a result", async () => { + test("searchPullRequests routes the call against the local remote's owner/name", async () => { const result = await host.trpc.workspaceCreation.searchPullRequests.query({ projectId, query: "#33", }); expect(result.pullRequests).toHaveLength(1); - expect(result.pullRequests[0].prNumber).toBe(33); + // repoMismatch detection runs on the resolved repo. If the resolver + // were trusting cloud `repoCloneUrl`, the cli/cli URL we'd construct + // for the result would mismatch `somewhere/else` and return no PRs. + expect(result.pullRequests[0].url).toContain("github.com/cli/cli"); }); - test("searchGitHubIssues resolves owner/name from repoCloneUrl without throwing", async () => { + test("searchGitHubIssues works without throwing 'no linked GitHub repository'", async () => { const result = await host.trpc.workspaceCreation.searchGitHubIssues.query({ projectId, query: "anything", @@ -217,8 +260,40 @@ describe("project has repoCloneUrl but no linked githubRepository (cloud-dep reg }); }); +describe("resolveGithubRepo throws PROJECT_NOT_SETUP when no local clone", () => { + let host: TestHost; + const projectId = randomUUID(); + + beforeEach(async () => { + // No projects row, no local clone — but cloud has the project. + host = await createTestHost({ + apiOverrides: { + "v2Project.get.query": () => ({ + id: projectId, + githubRepository: { owner: "octocat", name: "hello" }, + repoCloneUrl: "https://github.com/octocat/hello.git", + }), + }, + }); + }); + + afterEach(async () => { + await host.dispose(); + }); + + test("searchPullRequests refuses to query GitHub without a local clone", async () => { + await expect( + host.trpc.workspaceCreation.searchPullRequests.query({ + projectId, + query: "#1", + }), + ).rejects.toThrow(/Project is not set up on this host/); + }); +}); + describe("gh CLI is first-class when execGh succeeds", () => { let host: TestHost; + let repoDir: string; const projectId = randomUUID(); const ghCalls: Array<{ args: string[]; cwd?: string }> = []; @@ -247,8 +322,6 @@ describe("gh CLI is first-class when execGh succeeds", () => { options?: { cwd?: string }, ): Promise => { ghCalls.push({ args, cwd: options?.cwd }); - // `pr view ` returns a single object, `pr list` returns an array; - // match against the verb to pick the right shape. const verb = args[1]; if (verb === "view" && args[0] === "pr") { return { @@ -293,21 +366,20 @@ describe("gh CLI is first-class when execGh succeeds", () => { host = await createTestHost({ githubFactory: async () => fakeOctokit, execGh: fakeExecGh, - apiOverrides: { - "v2Project.get.query": () => ({ - id: projectId, - githubRepository: null, - repoCloneUrl: "https://github.com/octocat/hello.git", - }), - }, }); + repoDir = await seedRepoFixture( + host, + projectId, + "https://github.com/octocat/hello.git", + ); }); afterEach(async () => { await host.dispose(); + rmSync(repoDir, { recursive: true, force: true }); }); - test("searchPullRequests #N invokes `gh pr view` against the resolved repo", async () => { + test("searchPullRequests #N invokes `gh pr view` with cwd=repoPath", async () => { const result = await host.trpc.workspaceCreation.searchPullRequests.query({ projectId, query: "#33", @@ -323,6 +395,10 @@ describe("gh CLI is first-class when execGh succeeds", () => { "--repo", "octocat/hello", ]); + // `simpleGit.revparse(--show-toplevel)` canonicalizes through + // /private on macOS (where /var is a symlink), so compare against + // the realpath rather than the raw mkdtemp result. + expect(ghCalls[0].cwd).toBe(realpathSync(repoDir)); }); test("searchPullRequests free-text invokes `gh pr list --search`", async () => { From 5f387f3a83b475d9dde0086196bc9f345fafd61a Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Wed, 6 May 2026 14:23:54 -0700 Subject: [PATCH 3/7] refactor(host-service): canonical git remote read in resolveGithubRepo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switch resolveGithubRepo from `resolveLocalRepo` (origin-first heuristic) to `getGitHubRemotes` (canonical `git config --get-regexp ^remote\..*\.url$`) plus an explicit preference order: 1. user-configured `projects.remoteName` (recorded at project setup — stable, e.g. "upstream" when tracking an upstream fork) 2. `origin` 3. first GitHub remote on the repo Also keeps the `git rev-parse --show-toplevel` canonicalization to validate the path is a git repo and resolve through symlinks. `getGitHubRemotes`'s underlying `git config` regex is preferred over `git remote -v` because the latter appends partial-clone markers like `[blob:none]` and is otherwise human-readable, not machine-stable. New test asserts the remoteName preference: a repo with both `origin` (user's fork) and `upstream` (real source), with `remoteName=upstream` recorded at setup, must route searches against upstream — silently defaulting to origin would surface the wrong PRs. --- .../shared/project-helpers.ts | 51 ++++++++++---- ...kspace-creation-github.integration.test.ts | 67 +++++++++++++++++++ 2 files changed, 106 insertions(+), 12 deletions(-) diff --git a/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts b/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts index a9fc812bf6b..a2167db0f60 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts @@ -1,9 +1,10 @@ import { TRPCError } from "@trpc/server"; import { eq } from "drizzle-orm"; +import simpleGit from "simple-git"; import { projects } from "../../../../db/schema"; import type { HostServiceContext } from "../../../../types"; import type { ProjectNotSetupCause } from "../../../error-types"; -import { resolveLocalRepo } from "../../project/utils/resolve-repo"; +import { getGitHubRemotes } from "../../project/utils/git-remote"; export function projectNotSetupError(projectId: string): TRPCError { return new TRPCError({ @@ -19,7 +20,7 @@ export function projectNotSetupError(projectId: string): TRPCError { export interface ResolvedGithubRepo { owner: string; name: string; - /** Local clone path. Required — searches operate against the local clone. */ + /** Canonical local clone path. */ repoPath: string; } @@ -27,11 +28,20 @@ export interface ResolvedGithubRepo { * Resolve `{owner, name, repoPath}` for a project by reading the **live** * GitHub remote of the local clone. Both the cloud `repoCloneUrl` and the * cached `projects.repoOwner`/`repoName` columns are setup-time snapshots - * that can drift (rename, fork, manual remote re-point); GitHub queries - * must always target wherever the user's actual remote points right now. + * that drift (rename, fork, manual remote re-point); GitHub queries must + * always target wherever the user's actual remote points right now. * - * Throws `PROJECT_NOT_SETUP` if the project has no local clone on this - * host, or `BAD_REQUEST` if the local clone has no GitHub remote. + * Two layers of canonicalization here, both intentional: + * + * 1. `git rev-parse --show-toplevel` resolves the stored repoPath to its + * canonical git working-tree root and confirms it's a git repo. + * 2. `getGitHubRemotes` reads remote URLs via + * `git config --get-regexp ^remote\..*\.url$` — the machine-stable + * path. We avoid `git remote -v` because it appends partial-clone + * markers (`[blob:none]`) when `remote..promisor` is set. + * + * Prefers the user-configured `remoteName` from project setup, then + * `origin`, then the first GitHub remote on the repo. */ export async function resolveGithubRepo( ctx: HostServiceContext, @@ -44,17 +54,34 @@ export async function resolveGithubRepo( throw projectNotSetupError(projectId); } - const resolved = await resolveLocalRepo(local.repoPath); - if (!resolved.parsed) { + let gitRoot: string; + try { + gitRoot = ( + await simpleGit(local.repoPath).revparse(["--show-toplevel"]) + ).trim(); + } catch { + throw new TRPCError({ + code: "BAD_REQUEST", + message: `Not a git repository: ${local.repoPath}`, + }); + } + + const remotes = await getGitHubRemotes(simpleGit(gitRoot)); + const preferred = + (local.remoteName ? remotes.get(local.remoteName) : undefined) ?? + remotes.get("origin") ?? + remotes.values().next().value; + + if (!preferred) { throw new TRPCError({ code: "BAD_REQUEST", - message: `Repository at ${local.repoPath} has no GitHub remote.`, + message: `Repository at ${gitRoot} has no GitHub remote.`, }); } return { - owner: resolved.parsed.owner, - name: resolved.parsed.name, - repoPath: resolved.repoPath, + owner: preferred.owner, + name: preferred.name, + repoPath: gitRoot, }; } diff --git a/packages/host-service/test/integration/workspace-creation-github.integration.test.ts b/packages/host-service/test/integration/workspace-creation-github.integration.test.ts index 4d3837f8a14..1ee23f0255b 100644 --- a/packages/host-service/test/integration/workspace-creation-github.integration.test.ts +++ b/packages/host-service/test/integration/workspace-creation-github.integration.test.ts @@ -260,6 +260,73 @@ describe("resolveGithubRepo trusts the live local remote, never the cloud", () = }); }); +describe("resolveGithubRepo prefers the user-configured remoteName", () => { + let host: TestHost; + let repoDir: string; + const projectId = randomUUID(); + + const fakeOctokit = { + pulls: { + get: async () => ({ + data: { + number: 5, + title: "via upstream", + html_url: "https://github.com/upstream-org/cli/pull/5", + state: "open", + user: { login: "ada" }, + draft: false, + merged_at: null, + }, + }), + }, + issues: { get: async () => ({ data: {} }) }, + search: { + issuesAndPullRequests: async () => ({ data: { items: [] } }), + }, + }; + + beforeEach(async () => { + host = await createTestHost({ githubFactory: async () => fakeOctokit }); + // Repo has both `origin` (the user's fork) and `upstream` (where PRs + // actually live). Project setup recorded `upstream` as the configured + // remote — resolver must honor that, not silently default to origin. + repoDir = mkdtempSync(join(tmpdir(), "ws-creation-github-test-")); + const git = simpleGit(repoDir); + await git.init(["--initial-branch=main"]); + await git.addRemote("origin", "https://github.com/me/cli.git"); + await git.addRemote("upstream", "https://github.com/upstream-org/cli.git"); + host.db + .insert(projects) + .values({ + id: projectId, + repoPath: repoDir, + repoProvider: "github", + repoOwner: null, + repoName: null, + repoUrl: null, + remoteName: "upstream", + }) + .run(); + }); + + afterEach(async () => { + await host.dispose(); + rmSync(repoDir, { recursive: true, force: true }); + }); + + test("searchPullRequests routes against `upstream`, not `origin`", async () => { + const result = await host.trpc.workspaceCreation.searchPullRequests.query({ + projectId, + query: "#5", + }); + // Octokit fallback (no execGh wired in test): asserting the URL + // resolves to upstream-org/cli proves the resolver picked the + // configured remote over origin. + expect(result.pullRequests).toHaveLength(1); + expect(result.pullRequests[0].url).toContain("github.com/upstream-org/cli"); + }); +}); + describe("resolveGithubRepo throws PROJECT_NOT_SETUP when no local clone", () => { let host: TestHost; const projectId = randomUUID(); From 95421bdb6b157cd140267740f87f2bfd8c765748 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Wed, 6 May 2026 15:54:50 -0700 Subject: [PATCH 4/7] test(host-service): guard against snapshot-field reads in GitHub queries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an integration test that scans `packages/host-service/src/**` and fails CI when production code reads `cloudProject.repoCloneUrl` or the cached `projects.repoOwner`/`repoName` columns. Those fields drift on rename/fork/manual remote re-point — query code must derive owner/name from the live local git remote via `resolveGithubRepo(ctx, projectId)`. Allowlist covers the legitimate snapshot consumers: - schema declarations (`db/schema.ts`) - setup pipeline (`project/handlers.ts`, `project/project.ts`, `project/utils/persist-project.ts`) — cloning needs the cloud URL - the resolver itself (`workspace-creation/shared/project-helpers.ts`) - the runtime PR poller and `git.ts`'s sidebar response shape (TODO comments in the allowlist track the migration paths) Migrate `git.getPullRequestThreads` to `resolveGithubRepo` since the guard exposed it as a violation. The procedure was building octokit graphql/issues.listComments queries from the cached `project.repoOwner`/ `repoName` columns; now reads the live remote like the search procedures already do. --- .../host-service/src/trpc/router/git/git.ts | 28 +++-- .../no-snapshot-fields-for-queries.test.ts | 109 ++++++++++++++++++ 2 files changed, 122 insertions(+), 15 deletions(-) create mode 100644 packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts diff --git a/packages/host-service/src/trpc/router/git/git.ts b/packages/host-service/src/trpc/router/git/git.ts index 75583c89c9b..248280a8380 100644 --- a/packages/host-service/src/trpc/router/git/git.ts +++ b/packages/host-service/src/trpc/router/git/git.ts @@ -3,8 +3,9 @@ import { isAbsolute, join, normalize, sep } from "node:path"; import { TRPCError } from "@trpc/server"; import { eq } from "drizzle-orm"; import { z } from "zod"; -import { projects, pullRequests, workspaces } from "../../../db/schema"; +import { pullRequests, workspaces } from "../../../db/schema"; import { protectedProcedure, queryProcedure, router } from "../../index"; +import { resolveGithubRepo } from "../workspace-creation/shared/project-helpers"; import type { ChangedFile, CheckConclusionState, @@ -721,16 +722,13 @@ export const gitRouter = router({ }); } - const project = ctx.db.query.projects - .findFirst({ where: eq(projects.id, workspace.projectId) }) - .sync(); - if (!project) { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: `Project ${workspace.projectId} not found in database`, - }); - } - if (!project.repoOwner || !project.repoName) { + let repo: { owner: string; name: string }; + try { + repo = await resolveGithubRepo(ctx, workspace.projectId); + } catch { + // Project isn't set up locally on this host or has no GitHub + // remote — degrade silently like the previous implementation + // rather than throwing into the review tab. return { reviewThreads: [], conversationComments: [] }; } @@ -741,8 +739,8 @@ export const gitRouter = router({ const result: GraphQLThreadsResult = await octokit.graphql( REVIEW_THREADS_QUERY, { - owner: project.repoOwner, - name: project.repoName, + owner: repo.owner, + name: repo.name, prNumber: pr.prNumber, }, ); @@ -760,8 +758,8 @@ export const gitRouter = router({ let hasMore = true; while (hasMore) { const { data: comments } = await octokit.issues.listComments({ - owner: project.repoOwner, - repo: project.repoName, + owner: repo.owner, + repo: repo.name, issue_number: pr.prNumber, per_page: 100, page, diff --git a/packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts b/packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts new file mode 100644 index 00000000000..f736fff8368 --- /dev/null +++ b/packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts @@ -0,0 +1,109 @@ +import { expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import { glob } from "node:fs/promises"; +import { relative, resolve } from "node:path"; + +/** + * Hardening guard: GitHub queries (octokit, gh, graphql) must derive + * `owner`/`name` from the live local git remote — i.e. via + * `resolveGithubRepo(ctx, projectId)` — never from the cloud + * `repoCloneUrl` snapshot or the cached `projects.repoOwner`/`repoName` + * columns. Those drift on rename/fork/manual remote re-point and produce + * silent misrouting of queries to the wrong repo. + * + * This test scans `packages/host-service/src/**` for member reads of those + * fields. The allowlist below is for files that legitimately reference + * them: schema declarations, the setup pipeline (cloning, persistence), + * the resolver itself, and display-only project listing. + * + * If you're adding a new GitHub query and reach for one of these fields + * to satisfy this test — STOP. Call `resolveGithubRepo(ctx, projectId)` + * instead. The allowlist is for snapshot consumers, not query consumers. + */ +const HOST_SERVICE_SRC = resolve(import.meta.dir, "../../src"); + +const ALLOWLIST = new Set([ + // Schema declarations — these literally define the columns. + "db/schema.ts", + + // Setup pipeline. Cloning needs the cloud `repoCloneUrl` to know what + // to pass to `git clone`; the local cache columns are written here. + "trpc/router/project/handlers.ts", + "trpc/router/project/project.ts", + "trpc/router/project/utils/persist-project.ts", + + // The resolver itself. Mentions the field names in JSDoc; doesn't read + // them off any object — see the regex below, which only flags member + // access. Listed here defensively in case a future edit introduces one. + "trpc/router/workspace-creation/shared/project-helpers.ts", + + // The PR-runtime poller still uses cached `project.repoOwner`/`repoName` + // for its repo identity. Migrating it to the live remote is a separate + // change because the cache has TTL/invalidation semantics that need + // rethinking. TODO: route this through `resolveGithubRepo` and have the + // runtime react to `.git/config` changes via GitWatcher. + "runtime/pull-requests/pull-requests.ts", + + // `git.getPullRequestSidebar` echoes the cached `pull_requests` row's + // `repoOwner`/`repoName` back to the renderer alongside the PR list. It + // isn't constructing a new GitHub query directly, but it forwards the + // snapshot. TODO: drop these fields from the response shape, or derive + // them from `resolveGithubRepo` once per render. (Keep this allowlisted + // only as long as the rest of the file uses `resolveGithubRepo` for any + // actual querying — see `getPullRequestThreads` above for the pattern.) + "trpc/router/git/git.ts", +]); + +// Match member-access reads of the snapshot fields. Catches both bare +// identifiers (`cloudProject.repoCloneUrl`) and chained-call expressions +// (`get.query().repoCloneUrl`). Skips bare object-literal writes +// (`{ repoCloneUrl: … }`) since those don't have a leading `.`. +const FORBIDDEN = /\.(repoCloneUrl|repoOwner|repoName)\b/; + +test("snapshot fields aren't read for GitHub queries outside the allowlist", async () => { + const violations: Array<{ file: string; line: number; text: string }> = []; + + for await (const file of glob("**/*.ts", { cwd: HOST_SERVICE_SRC })) { + const rel = file; + // Test files exist to verify implementation behavior; they routinely + // assert on cached fields. The rule applies to production code paths. + if (rel.endsWith(".test.ts")) continue; + if (ALLOWLIST.has(rel)) continue; + + const abs = resolve(HOST_SERVICE_SRC, rel); + const content = readFileSync(abs, "utf8"); + const lines = content.split("\n"); + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + // Skip comment lines so JSDoc/explanatory comments don't trip + // the guard. Block comments aren't worth tracking precisely; + // the trailing `//` in a code line is rare enough. + const trimmed = line.trimStart(); + if (trimmed.startsWith("//") || trimmed.startsWith("*")) continue; + if (FORBIDDEN.test(line)) { + violations.push({ file: rel, line: i + 1, text: line.trim() }); + } + } + } + + if (violations.length > 0) { + const report = violations + .map((v) => ` ${v.file}:${v.line} ${v.text}`) + .join("\n"); + throw new Error( + [ + "Found snapshot-field reads outside the allowlist.", + "", + "GitHub queries must call `resolveGithubRepo(ctx, projectId)` to", + "get owner/name from the live local git remote — not from the", + "cached/cloud snapshot fields below:", + "", + report, + "", + `See ${relative(process.cwd(), import.meta.path)} for the rule.`, + ].join("\n"), + ); + } + + expect(violations).toEqual([]); +}); From d62b26f834bd57ba6bafb70715741b3d0517b506 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Wed, 6 May 2026 16:00:49 -0700 Subject: [PATCH 5/7] deslop: trim verbose comments to load-bearing why --- .../PRLinkCommand/PRLinkCommand.tsx | 5 +- .../procedures/search-pull-requests.ts | 5 +- .../shared/project-helpers.ts | 23 +++---- .../workspace-creation/utils/exec-gh.ts | 8 +-- .../test/helpers/createTestHost.ts | 5 +- .../no-snapshot-fields-for-queries.test.ts | 61 ++++++------------- ...kspace-creation-github.integration.test.ts | 33 +++------- 7 files changed, 47 insertions(+), 93 deletions(-) diff --git a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx index 3d0fcca3f18..17c377fdded 100644 --- a/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx +++ b/apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx @@ -82,9 +82,8 @@ export function PRLinkCommand({ retry: false, }); - // Toast once per error transition. Previously the dropdown's empty-state - // hid PR-search failures behind "No open pull requests", which made - // upstream tRPC errors invisible to users. + // One toast per error transition — without this, the dropdown's + // empty-state silently hides upstream tRPC failures. const lastToastedError = useRef(null); useEffect(() => { const msg = error instanceof Error ? error.message : null; diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts index 73eaa34c5d6..d471528aec1 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts @@ -113,9 +113,8 @@ export const searchPullRequests = protectedProcedure const effectiveQuery = normalized.query; - // First-class: shell to user's `gh` — no cloud dep, picks up local - // `gh auth login`. Falls back to Octokit if gh isn't installed, - // isn't authenticated, or otherwise errors. + // gh-first uses the user's local `gh auth login`; falls back to + // Octokit when gh is missing, unauthed, or errors. try { if (normalized.isDirectLookup) { const prNumber = Number.parseInt(effectiveQuery, 10); diff --git a/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts b/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts index a2167db0f60..5d1503d832c 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts @@ -25,23 +25,16 @@ export interface ResolvedGithubRepo { } /** - * Resolve `{owner, name, repoPath}` for a project by reading the **live** - * GitHub remote of the local clone. Both the cloud `repoCloneUrl` and the - * cached `projects.repoOwner`/`repoName` columns are setup-time snapshots - * that drift (rename, fork, manual remote re-point); GitHub queries must - * always target wherever the user's actual remote points right now. + * Resolve `{owner, name, repoPath}` for a project from the **live** local + * git remote. Cloud `repoCloneUrl` and cached `projects.repoOwner`/`repoName` + * are setup-time snapshots that drift on rename/fork/remote re-point; + * GitHub queries must target wherever the remote points right now. * - * Two layers of canonicalization here, both intentional: + * `rev-parse --show-toplevel` validates the path is a git repo. + * `getGitHubRemotes` reads via `git config --get-regexp ^remote\..*\.url$` + * to avoid `git remote -v`'s `[blob:none]` partial-clone markers. * - * 1. `git rev-parse --show-toplevel` resolves the stored repoPath to its - * canonical git working-tree root and confirms it's a git repo. - * 2. `getGitHubRemotes` reads remote URLs via - * `git config --get-regexp ^remote\..*\.url$` — the machine-stable - * path. We avoid `git remote -v` because it appends partial-clone - * markers (`[blob:none]`) when `remote..promisor` is set. - * - * Prefers the user-configured `remoteName` from project setup, then - * `origin`, then the first GitHub remote on the repo. + * Remote preference: configured `remoteName` → `origin` → first GitHub remote. */ export async function resolveGithubRepo( ctx: HostServiceContext, diff --git a/packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts b/packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts index f50e62b05c1..213dfc10358 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/utils/exec-gh.ts @@ -10,10 +10,10 @@ export interface ExecGhOptions { } /** - * Shell out to the user's `gh` CLI. Uses the user's existing `gh auth login`, - * which avoids credential-manager plumbing and matches V1's behavior. Returns - * parsed JSON when stdout looks like JSON, otherwise the trimmed string. - * Throws on non-zero exit (caller treats that as an opportunity to fall back). + * Shell to `gh`. Relies on the user's existing `gh auth login` rather than + * the git credential manager, matching V1. Returns parsed JSON when stdout + * is JSON, else the trimmed string. Throws on non-zero exit so callers can + * fall back. */ export type ExecGh = ( args: string[], diff --git a/packages/host-service/test/helpers/createTestHost.ts b/packages/host-service/test/helpers/createTestHost.ts index d1349aabe1b..21edfaf87a0 100644 --- a/packages/host-service/test/helpers/createTestHost.ts +++ b/packages/host-service/test/helpers/createTestHost.ts @@ -111,9 +111,8 @@ export async function createTestHost( : undefined, execGh: options.execGh ? (options.execGh as CreateAppOptions["execGh"]) - : // Default to a stub that always rejects, so tests that don't set - // up `execGh` exercise the Octokit fallback instead of accidentally - // shelling out to a real `gh` on the host. + : // Reject by default so unconfigured tests exercise the Octokit + // fallback rather than shelling to a real `gh` on the host. async () => { throw new Error("execGh not configured in test"); }, diff --git a/packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts b/packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts index f736fff8368..12b4416ca83 100644 --- a/packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts +++ b/packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts @@ -4,60 +4,40 @@ import { glob } from "node:fs/promises"; import { relative, resolve } from "node:path"; /** - * Hardening guard: GitHub queries (octokit, gh, graphql) must derive - * `owner`/`name` from the live local git remote — i.e. via - * `resolveGithubRepo(ctx, projectId)` — never from the cloud - * `repoCloneUrl` snapshot or the cached `projects.repoOwner`/`repoName` - * columns. Those drift on rename/fork/manual remote re-point and produce - * silent misrouting of queries to the wrong repo. + * Hardening guard. GitHub queries must derive owner/name from the live + * local remote via `resolveGithubRepo(ctx, projectId)` — never from cloud + * `repoCloneUrl` or cached `projects.repoOwner`/`repoName`, which drift on + * rename/fork/remote re-point and silently misroute queries. * - * This test scans `packages/host-service/src/**` for member reads of those - * fields. The allowlist below is for files that legitimately reference - * them: schema declarations, the setup pipeline (cloning, persistence), - * the resolver itself, and display-only project listing. - * - * If you're adding a new GitHub query and reach for one of these fields - * to satisfy this test — STOP. Call `resolveGithubRepo(ctx, projectId)` - * instead. The allowlist is for snapshot consumers, not query consumers. + * If a new query trips this test: don't add to the allowlist. Call + * `resolveGithubRepo` instead. The allowlist is for snapshot consumers + * (schema, setup pipeline, persistence), not query consumers. */ const HOST_SERVICE_SRC = resolve(import.meta.dir, "../../src"); const ALLOWLIST = new Set([ - // Schema declarations — these literally define the columns. + // Schema and setup pipeline — declares the columns and writes them. "db/schema.ts", - - // Setup pipeline. Cloning needs the cloud `repoCloneUrl` to know what - // to pass to `git clone`; the local cache columns are written here. "trpc/router/project/handlers.ts", "trpc/router/project/project.ts", "trpc/router/project/utils/persist-project.ts", - // The resolver itself. Mentions the field names in JSDoc; doesn't read - // them off any object — see the regex below, which only flags member - // access. Listed here defensively in case a future edit introduces one. + // Resolver itself: mentions field names in JSDoc, no member reads. "trpc/router/workspace-creation/shared/project-helpers.ts", - // The PR-runtime poller still uses cached `project.repoOwner`/`repoName` - // for its repo identity. Migrating it to the live remote is a separate - // change because the cache has TTL/invalidation semantics that need - // rethinking. TODO: route this through `resolveGithubRepo` and have the - // runtime react to `.git/config` changes via GitWatcher. + // TODO: PR-runtime poller still keys repo identity off cached + // `project.repoOwner`/`repoName`. Migration needs cache invalidation + // rethink (GitWatcher → bust on `.git/config` changes). "runtime/pull-requests/pull-requests.ts", - // `git.getPullRequestSidebar` echoes the cached `pull_requests` row's - // `repoOwner`/`repoName` back to the renderer alongside the PR list. It - // isn't constructing a new GitHub query directly, but it forwards the - // snapshot. TODO: drop these fields from the response shape, or derive - // them from `resolveGithubRepo` once per render. (Keep this allowlisted - // only as long as the rest of the file uses `resolveGithubRepo` for any - // actual querying — see `getPullRequestThreads` above for the pattern.) + // TODO: `git.getPullRequestSidebar` forwards cached `pull_requests.repoOwner`/ + // `repoName` to the renderer. Either drop from the response shape or + // derive via `resolveGithubRepo` per render. "trpc/router/git/git.ts", ]); -// Match member-access reads of the snapshot fields. Catches both bare -// identifiers (`cloudProject.repoCloneUrl`) and chained-call expressions -// (`get.query().repoCloneUrl`). Skips bare object-literal writes -// (`{ repoCloneUrl: … }`) since those don't have a leading `.`. +// Member-access reads only — `cloudProject.repoCloneUrl` and +// `get.query().repoCloneUrl` both match; `{ repoCloneUrl: … }` doesn't. const FORBIDDEN = /\.(repoCloneUrl|repoOwner|repoName)\b/; test("snapshot fields aren't read for GitHub queries outside the allowlist", async () => { @@ -65,8 +45,7 @@ test("snapshot fields aren't read for GitHub queries outside the allowlist", asy for await (const file of glob("**/*.ts", { cwd: HOST_SERVICE_SRC })) { const rel = file; - // Test files exist to verify implementation behavior; they routinely - // assert on cached fields. The rule applies to production code paths. + // Tests routinely assert on cached fields; rule is for production code. if (rel.endsWith(".test.ts")) continue; if (ALLOWLIST.has(rel)) continue; @@ -75,9 +54,7 @@ test("snapshot fields aren't read for GitHub queries outside the allowlist", asy const lines = content.split("\n"); for (let i = 0; i < lines.length; i++) { const line = lines[i]; - // Skip comment lines so JSDoc/explanatory comments don't trip - // the guard. Block comments aren't worth tracking precisely; - // the trailing `//` in a code line is rare enough. + // Skip comments so JSDoc explaining the rule doesn't self-trip. const trimmed = line.trimStart(); if (trimmed.startsWith("//") || trimmed.startsWith("*")) continue; if (FORBIDDEN.test(line)) { diff --git a/packages/host-service/test/integration/workspace-creation-github.integration.test.ts b/packages/host-service/test/integration/workspace-creation-github.integration.test.ts index 1ee23f0255b..e6cf13dfd02 100644 --- a/packages/host-service/test/integration/workspace-creation-github.integration.test.ts +++ b/packages/host-service/test/integration/workspace-creation-github.integration.test.ts @@ -8,11 +8,9 @@ import { projects } from "../../src/db/schema"; import { createTestHost, type TestHost } from "../helpers/createTestHost"; /** - * Set up a real temp git working tree with a configured GitHub remote and - * insert a host-side `projects` row pointing at it. The PR/issue search - * procedures resolve owner/name by reading the live remote of this clone, - * so tests need a real `.git` here — no fakery substitutes for the actual - * `git remote get-url`. + * Real temp git working tree with a remote, plus a `projects` row pointing + * at it. Procedures resolve owner/name from the live remote, so tests need + * a real `.git` — no fake substitutes for `git remote get-url`. */ async function seedRepoFixture( host: TestHost, @@ -214,9 +212,10 @@ describe("resolveGithubRepo trusts the live local remote, never the cloud", () = }; beforeEach(async () => { + // Cloud says `somewhere/else`; local remote is `cli/cli`. The + // resolver MUST follow the local remote. host = await createTestHost({ githubFactory: async () => fakeOctokit, - // Cloud says one repo (`somewhere/else`)… apiOverrides: { "v2Project.get.query": () => ({ id: projectId, @@ -225,8 +224,6 @@ describe("resolveGithubRepo trusts the live local remote, never the cloud", () = }), }, }); - // …but the local remote points at `cli/cli`. Resolver MUST follow the - // local remote, not the cloud value. repoDir = await seedRepoFixture( host, projectId, @@ -245,9 +242,6 @@ describe("resolveGithubRepo trusts the live local remote, never the cloud", () = query: "#33", }); expect(result.pullRequests).toHaveLength(1); - // repoMismatch detection runs on the resolved repo. If the resolver - // were trusting cloud `repoCloneUrl`, the cli/cli URL we'd construct - // for the result would mismatch `somewhere/else` and return no PRs. expect(result.pullRequests[0].url).toContain("github.com/cli/cli"); }); @@ -287,9 +281,8 @@ describe("resolveGithubRepo prefers the user-configured remoteName", () => { beforeEach(async () => { host = await createTestHost({ githubFactory: async () => fakeOctokit }); - // Repo has both `origin` (the user's fork) and `upstream` (where PRs - // actually live). Project setup recorded `upstream` as the configured - // remote — resolver must honor that, not silently default to origin. + // origin → user's fork, upstream → real source, configured = upstream. + // Resolver must honor `remoteName=upstream`, not default to origin. repoDir = mkdtempSync(join(tmpdir(), "ws-creation-github-test-")); const git = simpleGit(repoDir); await git.init(["--initial-branch=main"]); @@ -319,9 +312,6 @@ describe("resolveGithubRepo prefers the user-configured remoteName", () => { projectId, query: "#5", }); - // Octokit fallback (no execGh wired in test): asserting the URL - // resolves to upstream-org/cli proves the resolver picked the - // configured remote over origin. expect(result.pullRequests).toHaveLength(1); expect(result.pullRequests[0].url).toContain("github.com/upstream-org/cli"); }); @@ -332,7 +322,6 @@ describe("resolveGithubRepo throws PROJECT_NOT_SETUP when no local clone", () => const projectId = randomUUID(); beforeEach(async () => { - // No projects row, no local clone — but cloud has the project. host = await createTestHost({ apiOverrides: { "v2Project.get.query": () => ({ @@ -364,9 +353,9 @@ describe("gh CLI is first-class when execGh succeeds", () => { const projectId = randomUUID(); const ghCalls: Array<{ args: string[]; cwd?: string }> = []; + // Octokit must NOT be hit when gh succeeds — throws turn accidental + // fallbacks into loud failures. const fakeOctokit = { - // Octokit must NOT be hit when gh succeeds. Throwing here makes any - // accidental fallback fail the test loudly. pulls: { get: async () => { throw new Error("octokit must not be called when gh succeeds"); @@ -462,9 +451,7 @@ describe("gh CLI is first-class when execGh succeeds", () => { "--repo", "octocat/hello", ]); - // `simpleGit.revparse(--show-toplevel)` canonicalizes through - // /private on macOS (where /var is a symlink), so compare against - // the realpath rather than the raw mkdtemp result. + // `rev-parse --show-toplevel` canonicalizes /var → /private/var on macOS. expect(ghCalls[0].cwd).toBe(realpathSync(repoDir)); }); From 79f3fe75fd1af88daa421f4a0a8026f75ca0a9e5 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Wed, 6 May 2026 16:11:28 -0700 Subject: [PATCH 6/7] fix: address PR review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - search-pull-requests / search-github-issues: rethrow when both gh and Octokit fail so the renderer toast fires instead of silent empty state (was masking the entire user-facing error UX). Verified with new tests asserting both procedures reject when both backends throw. - search-github-issues: filter PRs out of `gh issue view` results — gh returns PR data on `gh issue view ` (verified by hitting cli/cli #13353), where Octokit's `issues.get` exposes `pull_request` for the same filter. Detect via the canonical URL containing `/pull/`. Mutation test confirms regression detection. - project-helpers: preserve `cause` on rev-parse failure so the underlying git error survives the tRPC envelope. - git.getPullRequestThreads: narrow blanket `catch` to TRPCError — expected resolver failures degrade silently, real bugs propagate. - no-snapshot-fields-for-queries guard: skip lines starting with `/*` too, so block-comment delimiters can't trip the regex. Skipping other review notes: - "githubRepository fallback dropped": resolver no longer reads either cloud field; reviewer was reasoning from the prior commit state. - "cwd missing in get-content procs": those procs use `--repo owner/name` which routes gh independent of cwd. No behavior change. - "co-locate guard test": cross-cutting invariant scan with no single source file to co-locate with. --- .../host-service/src/trpc/router/git/git.ts | 13 ++- .../procedures/search-github-issues.ts | 9 +- .../procedures/search-pull-requests.ts | 4 +- .../shared/project-helpers.ts | 5 +- .../no-snapshot-fields-for-queries.test.ts | 10 +- ...kspace-creation-github.integration.test.ts | 95 +++++++++++++++++++ 6 files changed, 126 insertions(+), 10 deletions(-) diff --git a/packages/host-service/src/trpc/router/git/git.ts b/packages/host-service/src/trpc/router/git/git.ts index 248280a8380..177f4bc245e 100644 --- a/packages/host-service/src/trpc/router/git/git.ts +++ b/packages/host-service/src/trpc/router/git/git.ts @@ -725,11 +725,14 @@ export const gitRouter = router({ let repo: { owner: string; name: string }; try { repo = await resolveGithubRepo(ctx, workspace.projectId); - } catch { - // Project isn't set up locally on this host or has no GitHub - // remote — degrade silently like the previous implementation - // rather than throwing into the review tab. - return { reviewThreads: [], conversationComments: [] }; + } catch (err) { + // Expected resolver failures (project not set up locally, no + // GitHub remote) degrade silently — the review tab just stays + // empty. Anything else is a real bug; propagate it. + if (err instanceof TRPCError) { + return { reviewThreads: [], conversationComments: [] }; + } + throw err; } const octokit = await ctx.github(); diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/search-github-issues.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/search-github-issues.ts index b28fedc4842..bd4cbc48098 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/search-github-issues.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/search-github-issues.ts @@ -105,6 +105,13 @@ export const searchGitHubIssues = protectedProcedure if (normalized.isDirectLookup) { const issueNumber = Number.parseInt(effectiveQuery, 10); const issue = await ghDirectLookup(ctx.execGh, repo, issueNumber); + // `gh issue view ` happily returns a PR when N is a PR + // number — GitHub's API surface treats PRs as a kind of issue. + // Octokit's path filters via `issue.pull_request`; we don't + // have that field over `gh`, so detect via the canonical URL. + if (issue.url.includes("/pull/")) { + return { issues: [] }; + } if (!input.includeClosed && issue.state !== "open") { return { issues: [] }; } @@ -179,6 +186,6 @@ export const searchGitHubIssues = protectedProcedure "[workspaceCreation.searchGitHubIssues] octokit fallback failed", err, ); - return { issues: [] }; + throw err; } }); diff --git a/packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts b/packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts index d471528aec1..c82587cbc8a 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts @@ -195,10 +195,12 @@ export const searchPullRequests = protectedProcedure }), }; } catch (err) { + // Both gh and Octokit failed — rethrow so the renderer's toast + // fires instead of the dropdown silently rendering "no results". console.warn( "[workspaceCreation.searchPullRequests] octokit fallback failed", err, ); - return { pullRequests: [] }; + throw err; } }); diff --git a/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts b/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts index 5d1503d832c..f6bac761ea9 100644 --- a/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts +++ b/packages/host-service/src/trpc/router/workspace-creation/shared/project-helpers.ts @@ -52,10 +52,11 @@ export async function resolveGithubRepo( gitRoot = ( await simpleGit(local.repoPath).revparse(["--show-toplevel"]) ).trim(); - } catch { + } catch (err) { throw new TRPCError({ code: "BAD_REQUEST", - message: `Not a git repository: ${local.repoPath}`, + message: `Failed to inspect git repository at ${local.repoPath}`, + cause: err, }); } diff --git a/packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts b/packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts index 12b4416ca83..9d2459ad0a3 100644 --- a/packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts +++ b/packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts @@ -55,8 +55,16 @@ test("snapshot fields aren't read for GitHub queries outside the allowlist", asy for (let i = 0; i < lines.length; i++) { const line = lines[i]; // Skip comments so JSDoc explaining the rule doesn't self-trip. + // Block comments tracked by leading delimiter; doesn't perfectly + // cover mid-line `/* */` but that's vanishingly rare here. const trimmed = line.trimStart(); - if (trimmed.startsWith("//") || trimmed.startsWith("*")) continue; + if ( + trimmed.startsWith("//") || + trimmed.startsWith("*") || + trimmed.startsWith("/*") + ) { + continue; + } if (FORBIDDEN.test(line)) { violations.push({ file: rel, line: i + 1, text: line.trim() }); } diff --git a/packages/host-service/test/integration/workspace-creation-github.integration.test.ts b/packages/host-service/test/integration/workspace-creation-github.integration.test.ts index e6cf13dfd02..b32eaeceb32 100644 --- a/packages/host-service/test/integration/workspace-creation-github.integration.test.ts +++ b/packages/host-service/test/integration/workspace-creation-github.integration.test.ts @@ -317,6 +317,68 @@ describe("resolveGithubRepo prefers the user-configured remoteName", () => { }); }); +describe("both backends failing rethrows so the renderer toast fires", () => { + let host: TestHost; + let repoDir: string; + const projectId = randomUUID(); + + const failingOctokit = { + pulls: { + get: async () => { + throw new Error("octokit pulls.get failed"); + }, + }, + issues: { + get: async () => { + throw new Error("octokit issues.get failed"); + }, + }, + search: { + issuesAndPullRequests: async () => { + throw new Error("octokit search failed"); + }, + }, + }; + const failingExecGh = async (): Promise => { + throw new Error("gh exec failed"); + }; + + beforeEach(async () => { + host = await createTestHost({ + githubFactory: async () => failingOctokit, + execGh: failingExecGh, + }); + repoDir = await seedRepoFixture( + host, + projectId, + "https://github.com/octocat/hello.git", + ); + }); + + afterEach(async () => { + await host.dispose(); + rmSync(repoDir, { recursive: true, force: true }); + }); + + test("searchPullRequests rethrows when both gh and Octokit fail", async () => { + await expect( + host.trpc.workspaceCreation.searchPullRequests.query({ + projectId, + query: "anything", + }), + ).rejects.toThrow(); + }); + + test("searchGitHubIssues rethrows when both gh and Octokit fail", async () => { + await expect( + host.trpc.workspaceCreation.searchGitHubIssues.query({ + projectId, + query: "anything", + }), + ).rejects.toThrow(); + }); +}); + describe("resolveGithubRepo throws PROJECT_NOT_SETUP when no local clone", () => { let host: TestHost; const projectId = randomUUID(); @@ -472,6 +534,39 @@ describe("gh CLI is first-class when execGh succeeds", () => { expect(args).toContain("find me"); }); + test("searchGitHubIssues #N filters out PRs leaked by `gh issue view`", async () => { + // gh CLI happily returns a PR when `gh issue view ` is + // called — the URL is the only signal we have to detect it. + const localHost = await createTestHost({ + githubFactory: async () => fakeOctokit, + execGh: async (args) => { + if (args[0] === "issue" && args[1] === "view") { + return { + number: Number(args[2]), + title: "Should be filtered", + url: `https://github.com/octocat/hello/pull/${args[2]}`, + state: "OPEN", + author: { login: "x" }, + }; + } + return {}; + }, + }); + const localRepo = await seedRepoFixture( + localHost, + projectId, + "https://github.com/octocat/hello.git", + ); + const result = + await localHost.trpc.workspaceCreation.searchGitHubIssues.query({ + projectId, + query: "#13353", + }); + expect(result.issues).toEqual([]); + await localHost.dispose(); + rmSync(localRepo, { recursive: true, force: true }); + }); + test("searchGitHubIssues free-text invokes `gh issue list --search`", async () => { const result = await host.trpc.workspaceCreation.searchGitHubIssues.query({ projectId, From b4090faf8bd64be8e47d70b3c0bc162e6e469054 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Wed, 6 May 2026 16:15:58 -0700 Subject: [PATCH 7/7] test(host-service): drop redundant 'doesn't throw' coverage in resolver-divergence describe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dropped test asserted only `expect(result.issues).toEqual([])` after calling `searchGitHubIssues` in the cloud-vs-local divergence scenario — proving nothing beyond 'didn't throw'. The sibling `searchPullRequests` test in the same describe already proves the resolver follows the local remote (URL contains cli/cli, not the cloud's somewhere/else value), and the resolver is shared, so the issues procedure inherits that guarantee. Also trim the now-unused `issues.get` field from the fake Octokit. --- ...kspace-creation-github.integration.test.ts | 23 ------------------- 1 file changed, 23 deletions(-) diff --git a/packages/host-service/test/integration/workspace-creation-github.integration.test.ts b/packages/host-service/test/integration/workspace-creation-github.integration.test.ts index b32eaeceb32..fb911d8b05f 100644 --- a/packages/host-service/test/integration/workspace-creation-github.integration.test.ts +++ b/packages/host-service/test/integration/workspace-creation-github.integration.test.ts @@ -194,21 +194,6 @@ describe("resolveGithubRepo trusts the live local remote, never the cloud", () = }, }), }, - issues: { - get: async () => ({ - data: { - number: 42, - title: "Issue #42", - html_url: "https://github.com/cli/cli/issues/42", - state: "open", - user: { login: "alice" }, - pull_request: undefined, - }, - }), - }, - search: { - issuesAndPullRequests: async () => ({ data: { items: [] } }), - }, }; beforeEach(async () => { @@ -244,14 +229,6 @@ describe("resolveGithubRepo trusts the live local remote, never the cloud", () = expect(result.pullRequests).toHaveLength(1); expect(result.pullRequests[0].url).toContain("github.com/cli/cli"); }); - - test("searchGitHubIssues works without throwing 'no linked GitHub repository'", async () => { - const result = await host.trpc.workspaceCreation.searchGitHubIssues.query({ - projectId, - query: "anything", - }); - expect(result.issues).toEqual([]); - }); }); describe("resolveGithubRepo prefers the user-configured remoteName", () => {