diff --git a/apps/code/src/main/services/folders/service.test.ts b/apps/code/src/main/services/folders/service.test.ts index a212471df..83e7be4b0 100644 --- a/apps/code/src/main/services/folders/service.test.ts +++ b/apps/code/src/main/services/folders/service.test.ts @@ -93,7 +93,7 @@ vi.mock("../../db/repositories/worktree-repository.js", () => ({ WorktreeRepository: vi.fn(() => mockWorktreeRepo), })); -import { isGitRepository } from "@posthog/git/queries"; +import { getRemoteUrl, isGitRepository } from "@posthog/git/queries"; import type { IDialog } from "@posthog/platform/dialog"; import type { IRepositoryRepository } from "../../db/repositories/repository-repository"; import type { IWorkspaceRepository } from "../../db/repositories/workspace-repository"; @@ -467,6 +467,32 @@ describe("FoldersService", () => { }); }); + it("normalizes a non-GitHub override and skips the local remote lookup", async () => { + vi.mocked(isGitRepository).mockResolvedValue(true); + vi.mocked(getRemoteUrl).mockResolvedValue( + "https://github.com/SomeoneElse/wrong", + ); + mockRepositoryRepo.findByPath.mockReturnValue(null); + mockRepositoryRepo.create.mockReturnValue({ + id: "folder-new", + path: "/home/user/fork", + remoteUrl: "https://gitlab.com/PostHog/posthog", + lastAccessedAt: "2024-01-01T00:00:00.000Z", + createdAt: "2024-01-01T00:00:00.000Z", + updatedAt: "2024-01-01T00:00:00.000Z", + }); + + await service.addFolder("/home/user/fork", { + remoteUrl: "https://gitlab.com/PostHog/posthog.git", + }); + + expect(mockRepositoryRepo.create).toHaveBeenCalledWith({ + path: "/home/user/fork", + remoteUrl: "https://gitlab.com/PostHog/posthog", + }); + expect(getRemoteUrl).not.toHaveBeenCalled(); + }); + it("backfills remoteUrl on an existing folder when override is supplied", async () => { vi.mocked(isGitRepository).mockResolvedValue(true); const existing = { diff --git a/apps/code/src/main/services/folders/service.ts b/apps/code/src/main/services/folders/service.ts index eb4342111..925070108 100644 --- a/apps/code/src/main/services/folders/service.ts +++ b/apps/code/src/main/services/folders/service.ts @@ -2,21 +2,10 @@ import fs from "node:fs"; import path from "node:path"; import { getRemoteUrl, isGitRepository } from "@posthog/git/queries"; import { InitRepositorySaga } from "@posthog/git/sagas/init"; - -import { normalizeRepoKey } from "@shared/utils/repo"; - -function extractRepoKey(url: string): string | null { - const httpsMatch = url.match(/github\.com\/([^/]+\/[^/]+)/); - if (httpsMatch) return normalizeRepoKey(httpsMatch[1]); - - const sshMatch = url.match(/github\.com:([^/]+\/[^/]+)/); - if (sshMatch) return normalizeRepoKey(sshMatch[1]); - - return null; -} - +import { parseGitHubUrl } from "@posthog/git/utils"; import { WorktreeManager } from "@posthog/git/worktree"; import type { IDialog } from "@posthog/platform/dialog"; +import { normalizeRepoKey } from "@shared/utils/repo"; import { inject, injectable } from "inversify"; import type { IRepositoryRepository, @@ -249,12 +238,13 @@ export class FoldersService { overrideRemoteUrl: string | undefined, ): Promise { if (overrideRemoteUrl) { - const overrideKey = extractRepoKey(overrideRemoteUrl); - if (overrideKey) return overrideKey; - return normalizeRepoKey(overrideRemoteUrl); + return ( + parseGitHubUrl(overrideRemoteUrl)?.path ?? + normalizeRepoKey(overrideRemoteUrl) + ); } const localRemoteUrl = await getRemoteUrl(folderPath); - return localRemoteUrl ? extractRepoKey(localRemoteUrl) : null; + return parseGitHubUrl(localRemoteUrl)?.path ?? null; } getRepositoryByRemoteUrl( diff --git a/apps/code/src/main/services/git/service.ts b/apps/code/src/main/services/git/service.ts index e69bb3d9d..7d20939a3 100644 --- a/apps/code/src/main/services/git/service.ts +++ b/apps/code/src/main/services/git/service.ts @@ -434,7 +434,7 @@ export class GitService extends TypedEventEmitter { let compareUrl: string | null = null; if (currentBranch && currentBranch !== defaultBranch) { - compareUrl = `https://github.com/${parsed.organization}/${parsed.repository}/compare/${defaultBranch}...${currentBranch}?expand=1`; + compareUrl = `https://github.com/${parsed.path}/compare/${defaultBranch}...${currentBranch}?expand=1`; } return { @@ -893,7 +893,6 @@ export class GitService extends TypedEventEmitter { const parsed = parseGitHubUrl(remoteUrl); if (!parsed) return null; - const repoSlug = `${parsed.organization}/${parsed.repository}`; const result = await execGh([ "pr", "list", @@ -906,7 +905,7 @@ export class GitService extends TypedEventEmitter { "--limit", "1", "--repo", - repoSlug, + parsed.path, ]); if (result.exitCode !== 0) { diff --git a/packages/git/package.json b/packages/git/package.json index 20f80840f..d37362c44 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -18,8 +18,8 @@ }, "devDependencies": { "@types/tar": "^6.1.13", - "vitest": "^2.1.8", - "typescript": "^5.5.0" + "typescript": "^5.5.0", + "vitest": "^2.1.8" }, "files": [ "dist/**/*", @@ -27,6 +27,7 @@ ], "dependencies": { "@posthog/shared": "workspace:*", + "git-url-parse": "^16.1.0", "simple-git": "^3.30.0" } } diff --git a/packages/git/src/utils.test.ts b/packages/git/src/utils.test.ts index ab558abea..441cd887a 100644 --- a/packages/git/src/utils.test.ts +++ b/packages/git/src/utils.test.ts @@ -2,7 +2,7 @@ import { chmod, mkdir, mkdtemp, rm, stat, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { forceRemove } from "./utils"; +import { forceRemove, parseGitHubUrl, parsePrUrl } from "./utils"; async function fileExists(p: string): Promise { try { @@ -66,3 +66,182 @@ describe("forceRemove", () => { expect(await fileExists(target)).toBe(false); }); }); + +describe("parseGitHubUrl", () => { + it.each([ + // HTTPS canonical forms + ["https://github.com/PostHog/code.git", "PostHog", "code"], + ["https://github.com/PostHog/code", "PostHog", "code"], + ["https://github.com/PostHog/code/", "PostHog", "code"], + ["https://github.com/PostHog/code.git/", "PostHog", "code"], + ["http://github.com/PostHog/code.git", "PostHog", "code"], + ["https://user:token@github.com/PostHog/code.git", "PostHog", "code"], + // SCP-style SSH + ["git@github.com:PostHog/code.git", "PostHog", "code"], + ["git@github.com:PostHog/code", "PostHog", "code"], + // ssh:// SSH variants + ["ssh://git@github.com/PostHog/code.git", "PostHog", "code"], + ["ssh://github.com/PostHog/code.git", "PostHog", "code"], + ["ssh://git@ssh.github.com:443/PostHog/code.git", "PostHog", "code"], + [ + "ssh://git@ssh.github.com:443/buildingapplications/bilt-landing.git", + "buildingapplications", + "bilt-landing", + ], + ["ssh://git@github.com:22/PostHog/code.git", "PostHog", "code"], + // Other protocols + ["git://github.com/PostHog/code.git", "PostHog", "code"], + ["git+https://github.com/PostHog/code.git", "PostHog", "code"], + ["git+ssh://git@github.com/PostHog/code.git", "PostHog", "code"], + // Whitespace + shorthand + [" https://github.com/PostHog/code.git\n", "PostHog", "code"], + ["\thttps://github.com/PostHog/code.git", "PostHog", "code"], + ["PostHog/code", "PostHog", "code"], + // Web URLs (path markers git-url-parse recognises) + ["https://github.com/PostHog/code/blob/main/README.md", "PostHog", "code"], + ["https://github.com/PostHog/code/tree/main", "PostHog", "code"], + ["https://github.com/PostHog/code/issues/12", "PostHog", "code"], + ["https://github.com/PostHog/code/commit/abc123", "PostHog", "code"], + // Mixed-case host (case in path is preserved) + ["git@GitHub.com:PostHog/Code.git", "PostHog", "Code"], + ["https://GITHUB.COM/PostHog/code.git", "PostHog", "code"], + ["HTTPS://github.com/PostHog/code.git", "PostHog", "code"], + // Query strings + fragments + ["https://github.com/PostHog/code.git?ref=main", "PostHog", "code"], + ["https://github.com/PostHog/code#readme", "PostHog", "code"], + // Special characters + ["https://github.com/post-hog/my-cool-repo", "post-hog", "my-cool-repo"], + ["https://github.com/PostHog/dotted.repo", "PostHog", "dotted.repo"], + ["https://github.com/Post_Hog/repo_name", "Post_Hog", "repo_name"], + ["https://github.com/123/456", "123", "456"], + ])("parses %s", (url, organization, repository) => { + expect(parseGitHubUrl(url)).toEqual({ + organization, + repository, + path: `${organization}/${repository}`, + }); + }); + + it.each([ + // Empty / nullish + "", + " ", + "\t\n", + null, + undefined, + // Non-URL strings + "not-a-url", + "PostHog", + "github.com/PostHog/code", + "//github.com/PostHog/code", + // Wrong host + "https://gitlab.com/PostHog/code.git", + "https://example.com/PostHog/code.git", + "git@gitlab.com:PostHog/code.git", + // SSH host alias (e.g. ~/.ssh/config Host github-personal). The remote may + // resolve to GitHub at connect time, but we can't know that statically. + "git@my-alias:PostHog/code.git", + "https://raw.githubusercontent.com/PostHog/code/main/README.md", + "file:///path/to/repo", + // Missing repo + "https://github.com/PostHog", + // Multiple / leading slashes + "https://github.com//PostHog/code.git", + "https://github.com/PostHog//code.git", + // Subdomains we don't trust + "https://api.github.com/repos/PostHog/code", + // GitHub web tabs git-url-parse can't isolate the repo from + "https://github.com/PostHog/code/wiki", + "https://github.com/PostHog/code/actions", + "https://github.com/PostHog/code/releases/tag/v1.0.0", + "https://github.com/PostHog/code/pull/42", + ])("returns null for %s", (url) => { + expect(parseGitHubUrl(url)).toBeNull(); + }); +}); + +describe("parsePrUrl", () => { + it.each([ + // Canonical PR URLs + ["https://github.com/PostHog/code/pull/42", "PostHog", "code", 42], + ["http://github.com/PostHog/code/pull/1", "PostHog", "code", 1], + [ + "https://github.com/buildingapplications/bilt-landing/pull/123", + "buildingapplications", + "bilt-landing", + 123, + ], + // Whitespace + [" https://github.com/PostHog/code/pull/7\n", "PostHog", "code", 7], + // PR sub-pages and tabs + ["https://github.com/PostHog/code/pull/42/files", "PostHog", "code", 42], + ["https://github.com/PostHog/code/pull/42/commits", "PostHog", "code", 42], + [ + "https://github.com/PostHog/code/pull/42/commits/abc123", + "PostHog", + "code", + 42, + ], + ["https://github.com/PostHog/code/pull/42/checks", "PostHog", "code", 42], + // Query strings + fragments + [ + "https://github.com/PostHog/code/pull/42?diff=split", + "PostHog", + "code", + 42, + ], + [ + "https://github.com/PostHog/code/pull/42#discussion_r123", + "PostHog", + "code", + 42, + ], + [ + "https://github.com/PostHog/code/pull/42/files#diff-abc", + "PostHog", + "code", + 42, + ], + // Mixed-case host + ["https://GITHUB.COM/PostHog/code/pull/42", "PostHog", "code", 42], + // Special characters in owner/repo + ["https://github.com/post-hog/my-repo/pull/42", "post-hog", "my-repo", 42], + // Large numbers (still valid integers) + ["https://github.com/PostHog/code/pull/999999", "PostHog", "code", 999999], + ])("parses %s", (url, owner, repo, number) => { + expect(parsePrUrl(url)).toEqual({ owner, repo, number }); + }); + + it.each([ + // Empty / nullish + "", + " ", + null, + undefined, + // Not a URL + "not-a-url", + // Missing /pull + "https://github.com/PostHog/code", + "git@github.com:PostHog/code.git", + // Wrong path keyword + "https://github.com/PostHog/code/issues/42", + "https://github.com/PostHog/code/pulls/42", + "https://github.com/PostHog/code/discussions/42", + // Bad number + "https://github.com/PostHog/code/pull/abc", + "https://github.com/PostHog/code/pull/0", + "https://github.com/PostHog/code/pull/-1", + "https://github.com/PostHog/code/pull/42.5", + "https://github.com/PostHog/code/pull/", + "https://github.com/PostHog/code/pull", + // Double / leading slashes in the path + "https://github.com/PostHog/code//pull/42", + "https://github.com/PostHog/code/pull//42", + "https://github.com//PostHog/code/pull/42", + // Wrong host + "https://gitlab.com/PostHog/code/pull/42", + "https://api.github.com/repos/PostHog/code/pulls/42", + ])("returns null for %s", (url) => { + expect(parsePrUrl(url)).toBeNull(); + }); +}); diff --git a/packages/git/src/utils.ts b/packages/git/src/utils.ts index d54b2e04f..34f43abd6 100644 --- a/packages/git/src/utils.ts +++ b/packages/git/src/utils.ts @@ -2,10 +2,12 @@ import { execFile } from "node:child_process"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; +import gitUrlParse from "git-url-parse"; export interface GitHubRepo { organization: string; repository: string; + path: string; } export async function safeSymlink( @@ -158,21 +160,41 @@ export interface GitHubPr { number: number; } -export function parsePrUrl(prUrl: string): GitHubPr | null { - const match = prUrl.match(/github\.com\/([^/]+)\/([^/]+)\/pull\/(\d+)/); - if (!match) return null; - return { owner: match[1], repo: match[2], number: Number(match[3]) }; +export function parsePrUrl(prUrl: string | null | undefined): GitHubPr | null { + if (!prUrl) return null; + let parsed: gitUrlParse.GitUrl; + try { + parsed = gitUrlParse(prUrl.trim()); + } catch { + return null; + } + if (parsed.source.toLowerCase() !== "github.com" || !parsed.full_name) { + return null; + } + const [owner, repo, kind, num] = parsed.full_name.split("/"); + if (!owner || !repo || kind !== "pull") return null; + const number = Number(num); + if (!Number.isInteger(number) || number <= 0) return null; + return { owner, repo, number }; } -export function parseGitHubUrl(url: string): GitHubRepo | null { - // Trim whitespace/newlines that git commands may include - const trimmedUrl = url.trim(); - - const match = - trimmedUrl.match(/github\.com[:/](.+?)\/(.+?)(\.git)?$/) || - trimmedUrl.match(/git@github\.com:(.+?)\/(.+?)(\.git)?$/); - - if (!match) return null; - - return { organization: match[1], repository: match[2].replace(/\.git$/, "") }; +export function parseGitHubUrl( + url: string | null | undefined, +): GitHubRepo | null { + if (!url) return null; + let parsed: gitUrlParse.GitUrl; + try { + parsed = gitUrlParse(url.trim()); + } catch { + return null; + } + if (parsed.source.toLowerCase() !== "github.com") return null; + // git-url-parse stuffs unhandled path segments into owner (e.g. wiki, actions, + // releases pages), so reject anything that didn't cleanly split into org/repo. + if (!parsed.owner || !parsed.name || parsed.owner.includes("/")) return null; + return { + organization: parsed.owner, + repository: parsed.name, + path: `${parsed.owner}/${parsed.name}`, + }; } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9df9e2959..35b4b176e 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -803,6 +803,9 @@ importers: '@posthog/shared': specifier: workspace:* version: link:../shared + git-url-parse: + specifier: ^16.1.0 + version: 16.1.0 simple-git: specifier: ^3.30.0 version: 3.30.0 @@ -5289,6 +5292,10 @@ packages: '@types/node@25.2.0': resolution: {integrity: sha512-DZ8VwRFUNzuqJ5khrvwMXHmvPe+zGayJhr2CDNiKB1WBE1ST8Djl00D0IC4vvNmHMdj6DlbYRIaFE7WHjlDl5w==} + '@types/parse-path@7.1.0': + resolution: {integrity: sha512-EULJ8LApcVEPbrfND0cRQqutIOdiIgJ1Mgrhpy755r14xMohPTEpkV/k28SJvuOs9bHRFW8x+KeDAEPiGQPB9Q==} + deprecated: This is a stub types definition. parse-path provides its own type definitions, so you do not need this installed. + '@types/react-dom@19.2.3': resolution: {integrity: sha512-jp2L/eY6fn+KgVVQAOqYItbF0VY/YApe5Mz2F0aykSO8gx31bYCZyvSeYxCHKvzHG5eZjc+zyaS5BrBWya2+kQ==} peerDependencies: @@ -7512,6 +7519,12 @@ packages: gifwrap@0.10.1: resolution: {integrity: sha512-2760b1vpJHNmLzZ/ubTtNnEx5WApN/PYWJvXvgS+tL1egTTthayFYIQQNi136FLEDcN/IyEY2EcGpIITD6eYUw==} + git-up@8.1.1: + resolution: {integrity: sha512-FDenSF3fVqBYSaJoYy1KSc2wosx0gCvKP+c+PRBht7cAaiCeQlBtfBDX9vgnNOHmdePlSFITVcn4pFfcgNvx3g==} + + git-url-parse@16.1.0: + resolution: {integrity: sha512-cPLz4HuK86wClEW7iDdeAKcCVlWXmrLpb2L+G9goW0Z1dtpNS6BXXSOckUTlJT/LDQViE1QZKstNORzHsLnobw==} + github-from-package@0.0.0: resolution: {integrity: sha512-SyHy3T1v2NUXn29OsWdxmK6RwHD+vkj3v8en8AOBZ1wBQ/hCAQ5bAQTD02kW4W9tUp/3Qh6J8r9EvntiyCmOOw==} @@ -7951,6 +7964,9 @@ packages: resolution: {integrity: sha512-rbku49cWloU5bSMI+zaRaXdQHXnthP6DZ/vLnfdSKyL4zUzuWnomtOEiZZOd+ioQ+avFo/qau3KPTc7Fjy1uPA==} engines: {node: '>=12'} + is-ssh@1.4.1: + resolution: {integrity: sha512-JNeu1wQsHjyHgn9NcWTaXq6zWSR6hqE0++zhfZlkFBbScNkyvxCdeV8sRkSBaeLKxmbpR21brail63ACNxJ0Tg==} + is-stream@1.1.0: resolution: {integrity: sha512-uQPm8kcs47jx38atAcWTVxyltQYoPT68y9aWYdV6yWXSyW8mzSat0TL6CiWdZeCdF3KrAvpVtnHbTv4RN+rqdQ==} engines: {node: '>=0.10.0'} @@ -9400,6 +9416,9 @@ packages: resolution: {integrity: sha512-TXfryirbmq34y8QBwgqCVLi+8oA3oWx2eAnSn62ITyEhEYaWRlVZ2DvMM9eZbMs/RfxPu/PK/aBLyGj4IrqMHw==} engines: {node: '>=18'} + parse-path@7.1.0: + resolution: {integrity: sha512-EuCycjZtfPcjWk7KTksnJ5xPMvWGA/6i4zrLYhRG0hGvC3GPU/jGUj3Cy+ZR0v30duV3e23R95T1lE2+lsndSw==} + parse-png@2.1.0: resolution: {integrity: sha512-Nt/a5SfCLiTnQAjx3fHlqp8hRgTL3z7kTQZzvIMS9uCAepnCyjpdEc6M/sz69WqMBdaDBw9sF1F1UaHROYzGkQ==} engines: {node: '>=10'} @@ -9407,6 +9426,10 @@ packages: parse-svg-path@0.1.2: resolution: {integrity: sha512-JyPSBnkTJ0AI8GGJLfMXvKq42cj5c006fnLz6fXy6zfoVjJizi8BNTpu8on8ziI1cKy9d9DGNuY17Ce7wuejpQ==} + parse-url@9.2.0: + resolution: {integrity: sha512-bCgsFI+GeGWPAvAiUv63ZorMeif3/U0zaXABGJbOWt5OH2KCaPHF6S+0ok4aqM9RuIPGyZdx9tR9l13PsW4AYQ==} + engines: {node: '>=14.13.0'} + parse5@7.3.0: resolution: {integrity: sha512-IInvU7fabl34qmi9gY8XOVxhYyMyuH2xUNpb2q8/Y+7552KlejkRvqvD19nMoUW/uQGGbqNpA6Tufu5FL5BZgw==} @@ -9831,6 +9854,9 @@ packages: resolution: {integrity: sha512-CvexbZtbov6jW2eXAvLukXjXUW1TzFaivC46BpWc/3BpcCysb5Vffu+B3XHMm8lVEuy2Mm4XGex8hBSg1yapPg==} engines: {node: '>=12.0.0'} + protocols@2.0.2: + resolution: {integrity: sha512-hHVTzba3wboROl0/aWRRG9dMytgH6ow//STBZh43l/wQgmMhYhOFi0EHWAPtoCz9IAUymsyP0TSBHkhgMEGNnQ==} + proxy-addr@2.0.7: resolution: {integrity: sha512-llQsMLSUDUPT44jdrU/O37qlnifitDP+ZwrmmZcoSKyLKvtZxpyV0n2/bD/N4tBAAZ/gJEdZU7KMraoK1+XYAg==} engines: {node: '>= 0.10'} @@ -11337,6 +11363,7 @@ packages: uuid@7.0.3: resolution: {integrity: sha512-DPSke0pXhTZgoF/d+WSt2QaKMCFSfx7QegxEWT+JOuHF5aWrKEn0G+ztjuJg/gG8/ItK+rbPCD/yNv8yyih6Cg==} + deprecated: uuid@10 and below is no longer supported. For ESM codebases, update to uuid@latest. For CommonJS codebases, use uuid@11 (but be aware this version will likely be deprecated in 2028). hasBin: true v8-to-istanbul@9.3.0: @@ -17027,6 +17054,10 @@ snapshots: dependencies: undici-types: 7.16.0 + '@types/parse-path@7.1.0': + dependencies: + parse-path: 7.1.0 + '@types/react-dom@19.2.3(@types/react@19.2.11)': dependencies: '@types/react': 19.2.11 @@ -19456,6 +19487,15 @@ snapshots: image-q: 4.0.0 omggif: 1.0.10 + git-up@8.1.1: + dependencies: + is-ssh: 1.4.1 + parse-url: 9.2.0 + + git-url-parse@16.1.0: + dependencies: + git-up: 8.1.1 + github-from-package@0.0.0: {} glob-parent@5.1.2: @@ -19933,6 +19973,10 @@ snapshots: is-regexp@3.1.0: {} + is-ssh@1.4.1: + dependencies: + protocols: 2.0.2 + is-stream@1.1.0: {} is-stream@2.0.1: {} @@ -21800,12 +21844,21 @@ snapshots: parse-ms@4.0.0: {} + parse-path@7.1.0: + dependencies: + protocols: 2.0.2 + parse-png@2.1.0: dependencies: pngjs: 3.4.0 parse-svg-path@0.1.2: {} + parse-url@9.2.0: + dependencies: + '@types/parse-path': 7.1.0 + parse-path: 7.1.0 + parse5@7.3.0: dependencies: entities: 6.0.1 @@ -22229,6 +22282,8 @@ snapshots: '@types/node': 24.12.0 long: 5.3.2 + protocols@2.0.2: {} + proxy-addr@2.0.7: dependencies: forwarded: 0.2.0