From 0e50163a1a193d36786ae5b0a48b23e44a609312 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 8 Apr 2026 15:53:38 -0700 Subject: [PATCH] refactor(FN-1288): consolidate GitHub remote parsing in @fusion/core - Replace dashboard calls to local getCurrentGitHubRepo helpers with getCurrentRepo from @fusion/core - Update engine scheduler PR-monitor startup paths to use shared core repo resolution - Remove duplicated remote parsing implementations from dashboard and engine packages - Mark gap analysis finding 6.3 as resolved after centralizing parsing logic --- docs/gap-analysis.md | 8 +++---- packages/dashboard/src/github.ts | 38 -------------------------------- packages/dashboard/src/routes.ts | 16 +++++++------- packages/dashboard/src/server.ts | 2 +- packages/engine/src/github.ts | 37 ------------------------------- packages/engine/src/scheduler.ts | 7 +++--- 6 files changed, 16 insertions(+), 92 deletions(-) delete mode 100644 packages/engine/src/github.ts diff --git a/docs/gap-analysis.md b/docs/gap-analysis.md index 60f282117..20ecb7c24 100644 --- a/docs/gap-analysis.md +++ b/docs/gap-analysis.md @@ -204,13 +204,13 @@ Assessment: Cross-reference: - Existing tasks **FN-1201** and **FN-1202** already target server-vs-client persistence boundaries. -### Finding 6.3 — Duplication in GitHub remote parsing logic across packages (**Medium**) +### Finding 6.3 — GitHub remote parsing logic consolidation (**Resolved**) -- `packages/core/src/gh-cli.ts` implements `parseRepoFromRemote()` + gh execution wrappers. -- `packages/engine/src/github.ts` separately implements similar `parseGitHubRemote()` and `getCurrentGitHubRepo()` logic. +- `packages/core/src/gh-cli.ts` is the canonical home for `parseRepoFromRemote()` and `getCurrentRepo()`. +- Engine and dashboard consumers now import the shared `@fusion/core` helpers directly. Impact: -- Risk of drift in URL parsing behavior and edge-case handling between scheduler/engine and core CLI helpers. +- Eliminates drift risk in URL parsing behavior and keeps GitHub remote resolution logic centralized. ### Finding 6.4 — Layering is healthy: no core → engine/dashboard circular import leak (**Good / no gap**) diff --git a/packages/dashboard/src/github.ts b/packages/dashboard/src/github.ts index 404af52d3..348fcd10a 100644 --- a/packages/dashboard/src/github.ts +++ b/packages/dashboard/src/github.ts @@ -2344,41 +2344,3 @@ export function parseGitHubBadgeUrl(url: string): { owner: string; repo: string return { owner: parsed.owner, repo: parsed.repo }; } -/** - * Extract owner/repo from a GitHub remote URL or return null if not a GitHub remote. - * @deprecated Use parseRepoFromRemote from gh-cli.ts instead - */ -export function parseGitHubRemote(remoteUrl: string): { owner: string; repo: string } | null { - // Handle HTTPS: https://github.com/owner/repo.git or https://github.com/owner/repo - const httpsMatch = remoteUrl.match(/github\.com\/([^\/]+)\/([^\/\.]+)(?:\.git)?$/); - if (httpsMatch) { - return { owner: httpsMatch[1], repo: httpsMatch[2] }; - } - - // Handle SSH: git@github.com:owner/repo.git or git@github.com:owner/repo - const sshMatch = remoteUrl.match(/github\.com:([^\/]+)\/([^\/\.]+)(?:\.git)?$/); - if (sshMatch) { - return { owner: sshMatch[1], repo: sshMatch[2] }; - } - - return null; -} - -/** - * Get the current GitHub remote owner/repo from the git config. - * @deprecated Use getCurrentRepo from gh-cli.ts instead - */ -export function getCurrentGitHubRepo(cwd: string): { owner: string; repo: string } | null { - const { execFileSync } = require("node:child_process"); - try { - const remoteUrl = execFileSync("git", ["remote", "get-url", "origin"], { - cwd, - encoding: "utf-8", - stdio: ["pipe", "pipe", "ignore"], - }).trim(); - - return parseGitHubRemote(remoteUrl); - } catch { - return null; - } -} diff --git a/packages/dashboard/src/routes.ts b/packages/dashboard/src/routes.ts index e1dbd9a3a..008992a2e 100644 --- a/packages/dashboard/src/routes.ts +++ b/packages/dashboard/src/routes.ts @@ -8,9 +8,9 @@ import { tmpdir } from "node:os"; import * as nodeFs from "node:fs"; import * as nodeChildProcess from "node:child_process"; import type { TaskStore, Column, MergeResult, ScheduleType, ActivityEventType, ModelPreset, AutomationStep, MessageType, ParticipantType, MessageCreateInput } from "@fusion/core"; -import { COLUMNS, VALID_TRANSITIONS, GLOBAL_SETTINGS_KEYS, type BatchStatusEntry, type BatchStatusResponse, type BatchStatusResult, type IssueInfo, type PrInfo, type Task, isGhAuthenticated, AUTOMATION_PRESETS, AutomationStore, validateBackupSchedule, validateBackupRetention, validateBackupDir, syncBackupAutomation, exportSettings, importSettings, validateImportData, MessageStore, MEMORY_FILE_PATH } from "@fusion/core"; +import { COLUMNS, VALID_TRANSITIONS, GLOBAL_SETTINGS_KEYS, type BatchStatusEntry, type BatchStatusResponse, type BatchStatusResult, type IssueInfo, type PrInfo, type Task, getCurrentRepo, isGhAuthenticated, AUTOMATION_PRESETS, AutomationStore, validateBackupSchedule, validateBackupRetention, validateBackupDir, syncBackupAutomation, exportSettings, importSettings, validateImportData, MessageStore, MEMORY_FILE_PATH } from "@fusion/core"; import type { ServerOptions } from "./server.js"; -import { GitHubClient, getCurrentGitHubRepo, parseBadgeUrl } from "./github.js"; +import { GitHubClient, parseBadgeUrl } from "./github.js"; import { githubRateLimiter } from "./github-poll.js"; import { terminalSessionManager } from "./terminal.js"; import { getTerminalService } from "./terminal-service.js"; @@ -4233,7 +4233,7 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout owner = o; repo = r; } else { - const gitRepo = getCurrentGitHubRepo(scopedStore.getRootDir()); + const gitRepo = getCurrentRepo(scopedStore.getRootDir()); if (!gitRepo) { throw badRequest("Could not determine GitHub repository. Set GITHUB_REPOSITORY env var or configure git remote."); } @@ -4520,7 +4520,7 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout owner = o; repo = r; } else { - const gitRepo = getCurrentGitHubRepo(scopedStore.getRootDir()); + const gitRepo = getCurrentRepo(scopedStore.getRootDir()); if (!gitRepo) { throw badRequest("Could not determine GitHub repository"); } @@ -4644,7 +4644,7 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout owner = o; repo = r; } else { - const gitRepo = getCurrentGitHubRepo(scopedStore.getRootDir()); + const gitRepo = getCurrentRepo(scopedStore.getRootDir()); if (!gitRepo) { throw badRequest("Could not determine GitHub repository"); } @@ -10980,7 +10980,7 @@ function getDefaultGitHubRepo(store: TaskStore): { owner: string; repo: string } } const rootDir = typeof store.getRootDir === "function" ? store.getRootDir() : process.cwd(); - return getCurrentGitHubRepo(rootDir); + return getCurrentRepo(rootDir); } function isBatchStatusStale(info: { lastCheckedAt?: string } | undefined, updatedAt?: string): boolean { @@ -11022,7 +11022,7 @@ async function refreshPrInBackground(store: TaskStore, taskId: string, currentPr owner = o; repo = r; } else { - const gitRepo = getCurrentGitHubRepo(store.getRootDir()); + const gitRepo = getCurrentRepo(store.getRootDir()); if (!gitRepo) return; // Silent fail - can't determine repo owner = gitRepo.owner; repo = gitRepo.repo; @@ -11066,7 +11066,7 @@ async function refreshIssueInBackground( owner = o; repo = r; } else { - const gitRepo = getCurrentGitHubRepo(store.getRootDir()); + const gitRepo = getCurrentRepo(store.getRootDir()); if (!gitRepo) return; owner = gitRepo.owner; repo = gitRepo.repo; diff --git a/packages/dashboard/src/server.ts b/packages/dashboard/src/server.ts index 1ba4849e2..a5ac4151f 100644 --- a/packages/dashboard/src/server.ts +++ b/packages/dashboard/src/server.ts @@ -13,7 +13,7 @@ import { getOrCreateProjectStore, evictAllProjectStores } from "./project-store- import { getTerminalService, type TerminalSession, STALE_SESSION_THRESHOLD_MS } from "./terminal-service.js"; import { WebSocketServer, type WebSocket } from "ws"; import { terminalSessionManager } from "./terminal.js"; -import { getCurrentGitHubRepo, parseBadgeUrl } from "./github.js"; +import { parseBadgeUrl } from "./github.js"; import { WebSocketManager, type BadgeSnapshot } from "./websocket.js"; import type { BadgePubSub } from "./badge-pubsub.js"; import { createBadgePubSub, type BadgePubSubMessage } from "./badge-pubsub.js"; diff --git a/packages/engine/src/github.ts b/packages/engine/src/github.ts deleted file mode 100644 index b010bc41e..000000000 --- a/packages/engine/src/github.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { execFileSync } from "node:child_process"; - -/** - * Extract owner/repo from a GitHub remote URL or return null if not a GitHub remote. - */ -export function parseGitHubRemote(remoteUrl: string): { owner: string; repo: string } | null { - // Handle HTTPS: https://github.com/owner/repo.git or https://github.com/owner/repo - const httpsMatch = remoteUrl.match(/github\.com\/([^\/]+)\/([^\/\.]+)(?:\.git)?$/); - if (httpsMatch) { - return { owner: httpsMatch[1], repo: httpsMatch[2] }; - } - - // Handle SSH: git@github.com:owner/repo.git or git@github.com:owner/repo - const sshMatch = remoteUrl.match(/github\.com:([^\/]+)\/([^\/\.]+)(?:\.git)?$/); - if (sshMatch) { - return { owner: sshMatch[1], repo: sshMatch[2] }; - } - - return null; -} - -/** - * Get the current GitHub remote owner/repo from the git config. - */ -export function getCurrentGitHubRepo(cwd: string): { owner: string; repo: string } | null { - try { - const remoteUrl = execFileSync("git", ["remote", "get-url", "origin"], { - cwd, - encoding: "utf-8", - stdio: ["pipe", "pipe", "ignore"], - }).trim(); - - return parseGitHubRemote(remoteUrl); - } catch { - return null; - } -} diff --git a/packages/engine/src/scheduler.ts b/packages/engine/src/scheduler.ts index bb44972f4..cd2406074 100644 --- a/packages/engine/src/scheduler.ts +++ b/packages/engine/src/scheduler.ts @@ -1,4 +1,4 @@ -import { resolveDependencyOrder, type TaskStore, type Task, type MissionStore, type PrInfo } from "@fusion/core"; +import { getCurrentRepo, resolveDependencyOrder, type TaskStore, type Task, type MissionStore, type PrInfo } from "@fusion/core"; import { existsSync } from "node:fs"; import { readFile } from "node:fs/promises"; import { join } from "node:path"; @@ -6,7 +6,6 @@ import type { AgentSemaphore } from "./concurrency.js"; import { generateReservedWorktreeName, slugify } from "./worktree-names.js"; import { schedulerLog } from "./logger.js"; import { type PrMonitor, type PrComment } from "./pr-monitor.js"; -import { getCurrentGitHubRepo } from "./github.js"; /** * Check whether two sets of file scope paths overlap. @@ -170,7 +169,7 @@ export class Scheduler { if (this.options.prMonitor) { if (to === "in-review" && task.prInfo) { // Start monitoring existing PR - const repo = getCurrentGitHubRepo(this.store.getRootDir()); + const repo = getCurrentRepo(this.store.getRootDir()); if (repo) { this.options.prMonitor.startMonitoring(task.id, repo.owner, repo.repo, task.prInfo); } @@ -263,7 +262,7 @@ export class Scheduler { return; } - const repo = getCurrentGitHubRepo(this.store.getRootDir()); + const repo = getCurrentRepo(this.store.getRootDir()); if (repo) { this.options.prMonitor.startMonitoring(task.id, repo.owner, repo.repo, task.prInfo); }