From de1f316da95e4056712c437c8d0fa119e41205e9 Mon Sep 17 00:00:00 2001 From: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com> Date: Tue, 25 Nov 2025 04:33:01 -0500 Subject: [PATCH 1/3] fix(gh_pr, gh_issue): show GitHub App install link when private repo access fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user has OAuth linked but the GitHub App isn't installed on a private repo, show a targeted install URL instead of generic error. - Add getOwnerIdFromUsername() to fetch owner ID from public GitHub API - Add generateInstallUrl() and sendInstallPrompt() to installation-service - Refactor parseRepo() to return tuple [owner, repo] 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/api/github-client.ts | 65 +++++++++++++++++--------- src/api/user-oauth-client.ts | 31 ------------ src/github-app/installation-service.ts | 31 ++++++++++++ src/handlers/gh-issue-handler.ts | 13 ++---- src/handlers/gh-pr-handler.ts | 13 ++---- src/services/subscription-service.ts | 27 ++++------- 6 files changed, 90 insertions(+), 90 deletions(-) diff --git a/src/api/github-client.ts b/src/api/github-client.ts index 0a8e930..2956bba 100644 --- a/src/api/github-client.ts +++ b/src/api/github-client.ts @@ -36,19 +36,19 @@ export function classifyApiError(error: unknown): GitHubApiErrorType { return "unknown"; } -function parseRepo(repo: string): { owner: string; repo: string } { - const [owner, repoName] = repo.split("/"); - return { owner, repo: repoName }; +export function parseRepo(repoFullName: string): [owner: string, repo: string] { + const [owner, repo] = repoFullName.split("/"); + return [owner, repo]; } export async function validateRepo( - repo: string, + repoFullName: string, userOctokit?: Octokit ): Promise { try { const client = userOctokit || octokit; - const { owner, repo: repoName } = parseRepo(repo); - await client.repos.get({ owner, repo: repoName }); + const [owner, repo] = parseRepo(repoFullName); + await client.repos.get({ owner, repo }); return true; } catch { return false; @@ -56,43 +56,43 @@ export async function validateRepo( } export async function getIssue( - repo: string, + repoFullName: string, issueNumber: string, userOctokit?: Octokit ): Promise { const client = userOctokit || octokit; - const { owner, repo: repoName } = parseRepo(repo); + const [owner, repo] = parseRepo(repoFullName); const { data } = await client.issues.get({ owner, - repo: repoName, + repo, issue_number: parseInt(issueNumber), }); return data; } export async function getPullRequest( - repo: string, + repoFullName: string, prNumber: string, userOctokit?: Octokit ): Promise { const client = userOctokit || octokit; - const { owner, repo: repoName } = parseRepo(repo); + const [owner, repo] = parseRepo(repoFullName); const { data } = await client.pulls.get({ owner, - repo: repoName, + repo, pull_number: parseInt(prNumber), }); return data; } export async function listPullRequests( - repo: string, + repoFullName: string, count: number = 10, filters?: { state?: string; author?: string }, userOctokit?: Octokit ): Promise { const client = userOctokit || octokit; - const { owner, repo: repoName } = parseRepo(repo); + const [owner, repo] = parseRepo(repoFullName); // Determine API state (merged PRs are fetched as closed) let apiState: "open" | "closed" | "all" = "all"; @@ -109,7 +109,7 @@ export async function listPullRequests( // Use Octokit's pagination iterator const iterator = client.paginate.iterator(client.pulls.list, { owner, - repo: repoName, + repo, state: apiState, per_page: 100, sort: "created", @@ -149,19 +149,19 @@ export async function listPullRequests( } export async function listIssues( - repo: string, + repoFullName: string, count: number = 10, filters?: { state?: string; creator?: string }, userOctokit?: Octokit ): Promise { const client = userOctokit || octokit; - const { owner, repo: repoName } = parseRepo(repo); + const [owner, repo] = parseRepo(repoFullName); // Build API query parameters const apiState = (filters?.state || "all") as "open" | "closed" | "all"; const params: Parameters[0] = { owner, - repo: repoName, + repo, state: apiState, per_page: 100, sort: "created", @@ -211,19 +211,19 @@ export type GitHubEventRaw = * Returns `\{ events, etag \}` or `\{ notModified: true \}` if ETag matches */ export async function fetchRepoEvents( - repo: string, + repoFullName: string, etag?: string ): Promise< | { events: GitHubEventRaw[]; etag: string; notModified?: false } | { notModified: true; etag?: never; events?: never } > { - const { owner, repo: repoName } = parseRepo(repo); + const [owner, repo] = parseRepo(repoFullName); try { // Use Octokit's request method to support ETag headers const response = await octokit.request("GET /repos/{owner}/{repo}/events", { owner, - repo: repoName, + repo, headers: etag ? { "If-None-Match": etag } : {}, }); @@ -240,3 +240,26 @@ export async function fetchRepoEvents( throw error; } } + +/** + * Get owner ID from username or org name + * Uses public GitHub APIs (/orgs or /users) - no special auth required + */ +export async function getOwnerIdFromUsername( + owner: string +): Promise { + try { + // Try as organization first (most private repos are in orgs) + try { + const { data } = await octokit.orgs.get({ org: owner }); + return data.id; + } catch { + // If not org, try as user + const { data } = await octokit.users.getByUsername({ username: owner }); + return data.id; + } + } catch (error) { + console.warn(`Could not fetch owner ID for ${owner}:`, error); + return undefined; + } +} diff --git a/src/api/user-oauth-client.ts b/src/api/user-oauth-client.ts index 10d01ba..1a5539b 100644 --- a/src/api/user-oauth-client.ts +++ b/src/api/user-oauth-client.ts @@ -99,34 +99,3 @@ export async function getUserProfile(token: string): Promise { throw error; } } - -/** - * Get owner ID from username or org using user's OAuth token - * Fetches public profile - works even without repo access - * @param token - User's GitHub OAuth access token - * @param owner - Repository owner username or org name - * @returns Owner ID or undefined if lookup fails - */ -export async function getOwnerIdFromUsername( - token: string, - owner: string -): Promise { - try { - const octokit = new Octokit({ auth: token }); - - // Try as organization first (most private repos are in orgs) - try { - const { data } = await octokit.orgs.get({ org: owner }); - return data.id; - } catch { - // If not org, try as user - const { data } = await octokit.users.getByUsername({ - username: owner, - }); - return data.id; - } - } catch (error) { - console.warn(`Could not fetch owner ID for ${owner}:`, error); - return undefined; // Fallback: omit target_id - } -} diff --git a/src/github-app/installation-service.ts b/src/github-app/installation-service.ts index 4e2d17b..280d3f0 100644 --- a/src/github-app/installation-service.ts +++ b/src/github-app/installation-service.ts @@ -1,5 +1,7 @@ import { and, eq } from "drizzle-orm"; +import type { BotHandler } from "@towns-protocol/bot"; +import { getOwnerIdFromUsername, parseRepo } from "../api/github-client"; import { db } from "../db"; import { githubInstallations, installationRepositories } from "../db/schema"; import type { SubscriptionService } from "../services/subscription-service"; @@ -9,6 +11,35 @@ import type { } from "../types/webhooks"; import type { GitHubApp } from "./app"; +/** + * Generate GitHub App installation URL + * @param targetId - Owner ID (user or org), optional + */ +export function generateInstallUrl(targetId?: number): string { + const appSlug = process.env.GITHUB_APP_SLUG || "towns-github-bot"; + const baseUrl = `https://github.com/apps/${appSlug}/installations/new`; + return targetId ? `${baseUrl}/permissions?target_id=${targetId}` : baseUrl; +} + +/** + * Send GitHub App installation prompt when user has OAuth but no repo access + */ +export async function sendInstallPrompt( + handler: BotHandler, + channelId: string, + repoFullName: string +): Promise { + const [owner] = parseRepo(repoFullName); + const ownerId = await getOwnerIdFromUsername(owner); + const installUrl = generateInstallUrl(ownerId); + await handler.sendMessage( + channelId, + `❌ Cannot access this repository\n\n` + + `Private repositories require the GitHub App to be installed:\n` + + `[Install GitHub App](${installUrl})` + ); +} + /** * InstallationService - Manages GitHub App installation lifecycle * diff --git a/src/handlers/gh-issue-handler.ts b/src/handlers/gh-issue-handler.ts index d752e56..5ff5dbb 100644 --- a/src/handlers/gh-issue-handler.ts +++ b/src/handlers/gh-issue-handler.ts @@ -10,6 +10,7 @@ import { formatIssueDetail, formatIssueList, } from "../formatters/command-formatters"; +import { sendInstallPrompt } from "../github-app/installation-service"; import type { GitHubOAuthService } from "../services/github-oauth-service"; import type { SlashCommandEvent } from "../types/bot"; import { parseCommandArgs, validateIssueFilters } from "../utils/arg-parser"; @@ -82,11 +83,7 @@ async function handleShowIssue( ); return; } catch { - // User token also failed - they don't have access - await handler.sendMessage( - channelId, - `❌ You don't have access to this repository` - ); + await sendInstallPrompt(handler, channelId, repo); return; } } @@ -168,11 +165,7 @@ async function handleListIssues( await sendIssueList(handler, channelId, actualIssues, repo); return; } catch { - // User token also failed - they don't have access - await handler.sendMessage( - channelId, - `❌ You don't have access to this repository` - ); + await sendInstallPrompt(handler, channelId, repo); return; } } diff --git a/src/handlers/gh-pr-handler.ts b/src/handlers/gh-pr-handler.ts index f343749..b111052 100644 --- a/src/handlers/gh-pr-handler.ts +++ b/src/handlers/gh-pr-handler.ts @@ -7,6 +7,7 @@ import { type GitHubPullRequestList, } from "../api/github-client"; import { formatPrDetail, formatPrList } from "../formatters/command-formatters"; +import { sendInstallPrompt } from "../github-app/installation-service"; import type { GitHubOAuthService } from "../services/github-oauth-service"; import type { SlashCommandEvent } from "../types/bot"; import { parseCommandArgs, validatePrFilters } from "../utils/arg-parser"; @@ -76,11 +77,7 @@ async function handleShowPr( ); return; } catch { - // User token also failed - they don't have access - await handler.sendMessage( - channelId, - `❌ You don't have access to this repository` - ); + await sendInstallPrompt(handler, channelId, repo); return; } } @@ -157,11 +154,7 @@ async function handleListPrs( await sendPrList(handler, channelId, prs, repo); return; } catch { - // User token also failed - they don't have access - await handler.sendMessage( - channelId, - `❌ You don't have access to this repository` - ); + await sendInstallPrompt(handler, channelId, repo); return; } } diff --git a/src/services/subscription-service.ts b/src/services/subscription-service.ts index 92d61fe..793f56c 100644 --- a/src/services/subscription-service.ts +++ b/src/services/subscription-service.ts @@ -1,7 +1,7 @@ import { and, eq, inArray, sql } from "drizzle-orm"; +import { getOwnerIdFromUsername } from "../api/github-client"; import { - getOwnerIdFromUsername, getUserProfile, validateRepository, type RepositoryInfo, @@ -16,7 +16,10 @@ import { } from "../constants"; import { db } from "../db"; import { githubSubscriptions, pendingSubscriptions } from "../db/schema"; -import { InstallationService } from "../github-app/installation-service"; +import { + generateInstallUrl, + InstallationService, +} from "../github-app/installation-service"; import type { TownsBot } from "../types/bot"; import { GitHubOAuthService } from "./github-oauth-service"; @@ -155,7 +158,7 @@ export class SubscriptionService { // If 404 and no installation, might be a private repo that needs installation if (status === 404 && !installationId) { // Get owner ID from public profile for installation URL - const ownerId = await getOwnerIdFromUsername(githubToken, owner); + const ownerId = await getOwnerIdFromUsername(owner); // Store pending subscription for completion after installation await this.storePendingSubscription({ @@ -169,7 +172,7 @@ export class SubscriptionService { return { success: false, requiresInstallation: true, - installUrl: this.generateInstallUrl(ownerId), + installUrl: generateInstallUrl(ownerId), repoFullName: repoIdentifier, eventTypes: requestedEventTypes, error: `Repository not found or you don't have access.`, @@ -216,7 +219,7 @@ export class SubscriptionService { return { success: false, requiresInstallation: true, - installUrl: this.generateInstallUrl(repoInfo.owner.id), + installUrl: generateInstallUrl(repoInfo.owner.id), repoFullName: repoInfo.fullName, eventTypes: requestedEventTypes, error: `Private repository requires GitHub App installation`, @@ -273,7 +276,7 @@ export class SubscriptionService { deliveryMode: "polling", repoFullName: repoInfo.fullName, eventTypes: requestedEventTypes, - installUrl: this.generateInstallUrl(repoInfo.owner.id), + installUrl: generateInstallUrl(repoInfo.owner.id), }; } else { return { @@ -732,16 +735,4 @@ export class SubscriptionService { ); } } - - /** - * Generate GitHub App installation URL - * @param targetId - Owner ID (user or org), optional - * @returns Installation URL - */ - private generateInstallUrl(targetId?: number): string { - const appSlug = process.env.GITHUB_APP_SLUG || "towns-github-bot"; - const baseUrl = `https://github.com/apps/${appSlug}/installations/new`; - - return targetId ? `${baseUrl}/permissions?target_id=${targetId}` : baseUrl; - } } From 618b268db8250d0283c83a84ecd5fcd7c0735bc9 Mon Sep 17 00:00:00 2001 From: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com> Date: Tue, 25 Nov 2025 04:59:39 -0500 Subject: [PATCH 2/3] fix(gh_pr, gh_issue): distinguish "not found" from "no access" errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When fetching a specific PR/issue fails after OAuth retry, check repo access with validateRepo() before showing install prompt. If user has access, the 404 means the item doesn't exist. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/handlers/gh-issue-handler.ts | 14 +++++++++++++- src/handlers/gh-pr-handler.ts | 14 +++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/handlers/gh-issue-handler.ts b/src/handlers/gh-issue-handler.ts index 5ff5dbb..9b17f0c 100644 --- a/src/handlers/gh-issue-handler.ts +++ b/src/handlers/gh-issue-handler.ts @@ -4,6 +4,7 @@ import { classifyApiError, getIssue, listIssues, + validateRepo, type GitHubIssueList, } from "../api/github-client"; import { @@ -83,7 +84,18 @@ async function handleShowIssue( ); return; } catch { - await sendInstallPrompt(handler, channelId, repo); + // Check if user has repo access + const hasAccess = await validateRepo(repo, userOctokit); + if (hasAccess) { + // User has access but issue doesn't exist + await handler.sendMessage( + channelId, + `❌ Issue #${issueNumber} not found in **${repo}**` + ); + } else { + // User doesn't have access - show install prompt + await sendInstallPrompt(handler, channelId, repo); + } return; } } diff --git a/src/handlers/gh-pr-handler.ts b/src/handlers/gh-pr-handler.ts index b111052..7bba8bb 100644 --- a/src/handlers/gh-pr-handler.ts +++ b/src/handlers/gh-pr-handler.ts @@ -4,6 +4,7 @@ import { classifyApiError, getPullRequest, listPullRequests, + validateRepo, type GitHubPullRequestList, } from "../api/github-client"; import { formatPrDetail, formatPrList } from "../formatters/command-formatters"; @@ -77,7 +78,18 @@ async function handleShowPr( ); return; } catch { - await sendInstallPrompt(handler, channelId, repo); + // Check if user has repo access + const hasAccess = await validateRepo(repo, userOctokit); + if (hasAccess) { + // User has access but PR doesn't exist + await handler.sendMessage( + channelId, + `❌ Pull request #${prNumber} not found in **${repo}**` + ); + } else { + // User doesn't have access - show install prompt + await sendInstallPrompt(handler, channelId, repo); + } return; } } From 915b466035413a2488761f5f3a4ec83a53d61f39 Mon Sep 17 00:00:00 2001 From: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com> Date: Tue, 25 Nov 2025 05:16:09 -0500 Subject: [PATCH 3/3] fix(parseRepo): validate input format, throw on malformed repo string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prevents confusing downstream errors when repo string is missing "/". The error propagates to handler's catch block showing a clear message. Updates existing tests to expect the new validation error. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/api/github-client.ts | 5 +++++ tests/unit/handlers/gh-issue-handler.test.ts | 9 +-------- tests/unit/handlers/gh-pr-handler.test.ts | 9 +-------- 3 files changed, 7 insertions(+), 16 deletions(-) diff --git a/src/api/github-client.ts b/src/api/github-client.ts index 2956bba..3602715 100644 --- a/src/api/github-client.ts +++ b/src/api/github-client.ts @@ -38,6 +38,11 @@ export function classifyApiError(error: unknown): GitHubApiErrorType { export function parseRepo(repoFullName: string): [owner: string, repo: string] { const [owner, repo] = repoFullName.split("/"); + if (!owner || !repo) { + throw new Error( + `Invalid repository format: "${repoFullName}". Expected "owner/repo".` + ); + } return [owner, repo]; } diff --git a/tests/unit/handlers/gh-issue-handler.test.ts b/tests/unit/handlers/gh-issue-handler.test.ts index 1dbb493..19b3191 100644 --- a/tests/unit/handlers/gh-issue-handler.test.ts +++ b/tests/unit/handlers/gh-issue-handler.test.ts @@ -139,11 +139,6 @@ describe("gh_issue handler - show single issue", () => { }); test("should handle malformed repository names", async () => { - const error = new Error("GitHub API error: 400 Bad Request"); - const getIssueSpy = spyOn(githubClient, "getIssue").mockRejectedValue( - error - ); - await handleGhIssue(mockHandler, { channelId: "test-channel", args: ["invalid-repo-name", "123"], @@ -151,10 +146,8 @@ describe("gh_issue handler - show single issue", () => { expect(mockHandler.sendMessage).toHaveBeenCalledWith( "test-channel", - "❌ Error: GitHub API error: 400 Bad Request" + '❌ Error: Invalid repository format: "invalid-repo-name". Expected "owner/repo".' ); - - getIssueSpy.mockRestore(); }); test("should format labels correctly with multiple labels", async () => { diff --git a/tests/unit/handlers/gh-pr-handler.test.ts b/tests/unit/handlers/gh-pr-handler.test.ts index f016b93..4f6dd63 100644 --- a/tests/unit/handlers/gh-pr-handler.test.ts +++ b/tests/unit/handlers/gh-pr-handler.test.ts @@ -138,11 +138,6 @@ describe("gh_pr handler - show single PR", () => { }); test("should handle malformed repository names", async () => { - const error = new Error("GitHub API error: 400 Bad Request"); - const getPRSpy = spyOn(githubClient, "getPullRequest").mockRejectedValue( - error - ); - await handleGhPr(mockHandler, { channelId: "test-channel", args: ["invalid-repo-name", "123"], @@ -150,10 +145,8 @@ describe("gh_pr handler - show single PR", () => { expect(mockHandler.sendMessage).toHaveBeenCalledWith( "test-channel", - "❌ Error: GitHub API error: 400 Bad Request" + '❌ Error: Invalid repository format: "invalid-repo-name". Expected "owner/repo".' ); - - getPRSpy.mockRestore(); }); test("should strip markdown bold formatting from repo name", async () => {