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..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 @@ -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,23 @@ export function PRLinkCommand({ }); }, enabled: !!projectId && !!hostUrl && open, + retry: false, }); + // 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; + 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 +158,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/git/git.ts b/packages/host-service/src/trpc/router/git/git.ts index 75583c89c9b..177f4bc245e 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,17 +722,17 @@ 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) { - return { reviewThreads: [], conversationComments: [] }; + let repo: { owner: string; name: string }; + try { + repo = await resolveGithubRepo(ctx, workspace.projectId); + } 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(); @@ -741,8 +742,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 +761,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/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..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 @@ -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,41 @@ 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); + // `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: [] }; + } + 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 +142,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 +182,10 @@ export const searchGitHubIssues = protectedProcedure })), }; } catch (err) { - console.warn("[workspaceCreation.searchGitHubIssues] failed", err); - return { issues: [] }; + console.warn( + "[workspaceCreation.searchGitHubIssues] octokit fallback failed", + err, + ); + 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 d790b560a14..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 @@ -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,36 @@ export const searchPullRequests = protectedProcedure } const effectiveQuery = normalized.query; + + // 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); + 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 +195,12 @@ export const searchPullRequests = protectedProcedure }), }; } catch (err) { - console.warn("[workspaceCreation.searchPullRequests] failed", err); - return { pullRequests: [] }; + // 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, + ); + 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 1ec4c6b46b5..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 @@ -1,6 +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 { getGitHubRemotes } from "../../project/utils/git-remote"; export function projectNotSetupError(projectId: string): TRPCError { return new TRPCError({ @@ -13,20 +17,65 @@ export function projectNotSetupError(projectId: string): TRPCError { }); } +export interface ResolvedGithubRepo { + owner: string; + name: string; + /** Canonical local clone path. */ + repoPath: string; +} + +/** + * 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. + * + * `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. + * + * Remote preference: configured `remoteName` → `origin` → first GitHub remote. + */ export async function resolveGithubRepo( ctx: HostServiceContext, projectId: string, -): Promise<{ owner: string; name: string }> { - const cloudProject = await ctx.api.v2Project.get.query({ - organizationId: ctx.organizationId, - id: projectId, - }); - const repo = cloudProject.githubRepository; - if (!repo?.owner || !repo?.name) { +): Promise { + const local = ctx.db.query.projects + .findFirst({ where: eq(projects.id, projectId) }) + .sync(); + if (!local?.repoPath) { + throw projectNotSetupError(projectId); + } + + let gitRoot: string; + try { + gitRoot = ( + await simpleGit(local.repoPath).revparse(["--show-toplevel"]) + ).trim(); + } catch (err) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: `Failed to inspect git repository at ${local.repoPath}`, + cause: err, + }); + } + + 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: "Project has no linked GitHub repository", + message: `Repository at ${gitRoot} has no GitHub remote.`, }); } - return { owner: repo.owner, name: repo.name }; + + return { + owner: preferred.owner, + name: preferred.name, + repoPath: gitRoot, + }; } 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..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 @@ -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 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 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..21edfaf87a0 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,13 @@ export async function createTestHost( github: options.githubFactory ? (options.githubFactory as CreateAppOptions["github"]) : undefined, + execGh: options.execGh + ? (options.execGh as CreateAppOptions["execGh"]) + : // 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"); + }, chatRuntime: options.chatRuntime as CreateAppOptions["chatRuntime"], chatService: options.chatService as CreateAppOptions["chatService"], }; 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..9d2459ad0a3 --- /dev/null +++ b/packages/host-service/test/integration/no-snapshot-fields-for-queries.test.ts @@ -0,0 +1,94 @@ +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 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. + * + * 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 and setup pipeline — declares the columns and writes them. + "db/schema.ts", + "trpc/router/project/handlers.ts", + "trpc/router/project/project.ts", + "trpc/router/project/utils/persist-project.ts", + + // Resolver itself: mentions field names in JSDoc, no member reads. + "trpc/router/workspace-creation/shared/project-helpers.ts", + + // 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", + + // 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", +]); + +// 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 () => { + const violations: Array<{ file: string; line: number; text: string }> = []; + + for await (const file of glob("**/*.ts", { cwd: HOST_SERVICE_SRC })) { + const rel = file; + // Tests routinely assert on cached fields; rule is for production code. + 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 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("*") || + 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([]); +}); 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..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 @@ -1,9 +1,44 @@ 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"; +/** + * 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, + 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,19 +101,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" }, - }), - }, - }); + 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 () => { @@ -141,3 +174,385 @@ describe("workspaceCreation github procedures with mocked Octokit", () => { expect(calls[0].method).toBe("search.issuesAndPullRequests"); }); }); + +describe("resolveGithubRepo trusts the live local remote, never the cloud", () => { + let host: TestHost; + let repoDir: string; + const projectId = randomUUID(); + + const fakeOctokit = { + pulls: { + get: async () => ({ + data: { + number: 33, + title: "PR #33", + html_url: "https://github.com/cli/cli/pull/33", + state: "open", + user: { login: "bob" }, + draft: false, + merged_at: null, + }, + }), + }, + }; + + beforeEach(async () => { + // Cloud says `somewhere/else`; local remote is `cli/cli`. The + // resolver MUST follow the local remote. + host = await createTestHost({ + githubFactory: async () => fakeOctokit, + apiOverrides: { + "v2Project.get.query": () => ({ + id: projectId, + githubRepository: null, + repoCloneUrl: "https://github.com/somewhere/else.git", + }), + }, + }); + repoDir = await seedRepoFixture( + host, + projectId, + "https://github.com/cli/cli.git", + ); + }); + + afterEach(async () => { + await host.dispose(); + rmSync(repoDir, { recursive: true, force: true }); + }); + + 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].url).toContain("github.com/cli/cli"); + }); +}); + +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 }); + // 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"]); + 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", + }); + expect(result.pullRequests).toHaveLength(1); + expect(result.pullRequests[0].url).toContain("github.com/upstream-org/cli"); + }); +}); + +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(); + + beforeEach(async () => { + 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 }> = []; + + // Octokit must NOT be hit when gh succeeds — throws turn accidental + // fallbacks into loud failures. + const fakeOctokit = { + 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 }); + 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, + }); + 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` with cwd=repoPath", 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", + ]); + // `rev-parse --show-toplevel` canonicalizes /var → /private/var on macOS. + expect(ghCalls[0].cwd).toBe(realpathSync(repoDir)); + }); + + 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 #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, + 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.