From 7e21c5b8724d706dd945d5e56f724fec0f1e4e73 Mon Sep 17 00:00:00 2001 From: Dotta Date: Tue, 26 May 2026 13:14:31 +0000 Subject: [PATCH] Address Greptile PR feedback Co-Authored-By: Paperclip --- .../src/__tests__/workspace-runtime.test.ts | 12 +-- server/src/routes/plugins.ts | 100 +++++++++--------- server/src/services/workspace-runtime.ts | 14 ++- ui/src/pages/PluginManager.tsx | 15 --- 4 files changed, 63 insertions(+), 78 deletions(-) diff --git a/server/src/__tests__/workspace-runtime.test.ts b/server/src/__tests__/workspace-runtime.test.ts index 43e80193080..1423e1da1f1 100644 --- a/server/src/__tests__/workspace-runtime.test.ts +++ b/server/src/__tests__/workspace-runtime.test.ts @@ -1922,18 +1922,16 @@ describe("realizeExecutionWorkspace", () => { }, 10_000); it("auto-detects the default branch via symbolic-ref when origin/HEAD is set", async () => { - // Create a repo with "master" as default branch - const repoRoot = await createTempRepo("master"); + const repoRoot = await createTempRepo("main"); - // Set up a bare remote and push const bareRemote = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-worktree-bare-symref-")); await runGit(bareRemote, ["init", "--bare"]); await runGit(repoRoot, ["remote", "add", "origin", bareRemote]); - await runGit(repoRoot, ["push", "-u", "origin", "master"]); + await runGit(repoRoot, ["push", "-u", "origin", "main", "master"]); await runGit(repoRoot, ["fetch", "origin"]); // Explicitly set refs/remotes/origin/HEAD to exercise the symbolic-ref path // (git remote set-head -a requires the remote to advertise HEAD, so we set it manually) - await runGit(repoRoot, ["remote", "set-head", "origin", "master"]); + await runGit(repoRoot, ["remote", "set-head", "origin", "main"]); const { recorder, operations } = createWorkspaceOperationRecorderDouble(); @@ -1949,7 +1947,7 @@ describe("realizeExecutionWorkspace", () => { config: { workspaceStrategy: { type: "git_worktree", - // No baseRef configured — origin/master is preferred over the symbolic-ref. + // No baseRef configured — origin/HEAD should win over fallback branches. }, }, issue: { @@ -1969,7 +1967,7 @@ describe("realizeExecutionWorkspace", () => { expect(workspace.created).toBe(true); const worktreeOp = operations.find(op => op.phase === "worktree_prepare" && op.metadata?.created); expect(worktreeOp).toBeDefined(); - expect(worktreeOp!.metadata!.baseRef).toBe("origin/master"); + expect(worktreeOp!.metadata!.baseRef).toBe("origin/main"); }, 10_000); it("removes a created git worktree and branch during cleanup", async () => { diff --git a/server/src/routes/plugins.ts b/server/src/routes/plugins.ts index 16434ef9011..ff05f1369a2 100644 --- a/server/src/routes/plugins.ts +++ b/server/src/routes/plugins.ts @@ -18,7 +18,7 @@ * @see doc/plugins/PLUGIN_SPEC.md for the full plugin specification */ -import { existsSync, readdirSync, readFileSync } from "node:fs"; +import { access, readdir, readFile } from "node:fs/promises"; import path from "node:path"; import { randomUUID } from "node:crypto"; import { fileURLToPath } from "node:url"; @@ -167,34 +167,39 @@ function titleCasePluginName(packageName: string): string { .join(" "); } -function readJsonFile(filePath: string): Record | null { +async function fileExists(filePath: string): Promise { + return access(filePath).then(() => true, () => false); +} + +async function readJsonFile(filePath: string): Promise | null> { try { - return JSON.parse(readFileSync(filePath, "utf8")) as Record; + return JSON.parse(await readFile(filePath, "utf8")) as Record; } catch { return null; } } -function findPackageJsonFiles(root: string, maxDepth = 4): string[] { - if (!existsSync(root)) return []; +async function findPackageJsonFiles(root: string, maxDepth = 4): Promise { + if (!(await fileExists(root))) return []; const packageJsonFiles: string[] = []; - const walk = (dir: string, depth: number) => { + const walk = async (dir: string, depth: number): Promise => { if (depth > maxDepth) return; - for (const entry of readdirSync(dir, { withFileTypes: true })) { + const entries = await readdir(dir, { withFileTypes: true }).catch(() => []); + for (const entry of entries) { if (entry.name === "node_modules" || entry.name === "dist") continue; const entryPath = path.join(dir, entry.name); if (entry.isFile() && entry.name === "package.json") { packageJsonFiles.push(entryPath); } else if (entry.isDirectory()) { - walk(entryPath, depth + 1); + await walk(entryPath, depth + 1); } } }; - walk(root, 0); + await walk(root, 0); return packageJsonFiles; } @@ -224,15 +229,15 @@ function firstStringLiteral(source: string, key: string): string | null { return match?.[1] ?? match?.[2] ?? match?.[3] ?? null; } -function bundledPluginMetadata( +async function bundledPluginMetadata( packageRoot: string, pkgJson: Record, -): { pluginKey?: string; displayName?: string; description?: string } { +): Promise<{ pluginKey?: string; displayName?: string; description?: string }> { const sourcePath = manifestSourcePath(packageRoot, pkgJson); - if (!sourcePath || !existsSync(sourcePath)) return {}; + if (!sourcePath || !(await fileExists(sourcePath))) return {}; try { - const source = readFileSync(sourcePath, "utf8"); + const source = await readFile(sourcePath, "utf8"); const pluginId = source .match(/(?:export\s+)?const\s+PLUGIN_ID\s*=\s*(?:"([^"]*)"|'([^']*)'|`([^`]*)`)/) ?.slice(1) @@ -257,44 +262,43 @@ function isExperimentalBundledPlugin(packageRoot: string, packageName: string): ); } -function listBundledPlugins(): AvailableBundledPlugin[] { +async function listBundledPlugins(): Promise { const pluginRoot = path.resolve(REPO_ROOT, "packages/plugins"); - return findPackageJsonFiles(pluginRoot) - .flatMap((packageJsonPath): AvailableBundledPlugin[] => { - const packageRoot = path.dirname(packageJsonPath); - const pkgJson = readJsonFile(packageJsonPath); - const paperclipPlugin = pkgJson?.paperclipPlugin; - if ( - !pkgJson - || !paperclipPlugin - || typeof paperclipPlugin !== "object" - || Array.isArray(paperclipPlugin) - ) { - return []; - } + const bundledPlugins: AvailableBundledPlugin[] = []; + for (const packageJsonPath of await findPackageJsonFiles(pluginRoot)) { + const packageRoot = path.dirname(packageJsonPath); + const pkgJson = await readJsonFile(packageJsonPath); + const paperclipPlugin = pkgJson?.paperclipPlugin; + if ( + !pkgJson + || !paperclipPlugin + || typeof paperclipPlugin !== "object" + || Array.isArray(paperclipPlugin) + ) { + continue; + } - const packageName = pkgJson.name; - if (typeof packageName !== "string" || packageName.length === 0) return []; + const packageName = pkgJson.name; + if (typeof packageName !== "string" || packageName.length === 0) continue; - const metadata = bundledPluginMetadata(packageRoot, pkgJson); - const tag = packageRoot.includes(`${path.sep}examples${path.sep}`) ? "example" : "first-party"; - return [ - { - packageName, - pluginKey: metadata.pluginKey ?? packageName, - displayName: metadata.displayName ?? titleCasePluginName(packageName), - description: metadata.description - ?? `Bundled Paperclip plugin from ${path.relative(REPO_ROOT, packageRoot)}.`, - localPath: packageRoot, - tag, - experimental: isExperimentalBundledPlugin(packageRoot, packageName), - }, - ]; - }) - .sort((left, right) => { - if (left.tag !== right.tag) return left.tag === "first-party" ? -1 : 1; - return left.displayName.localeCompare(right.displayName); + const metadata = await bundledPluginMetadata(packageRoot, pkgJson); + const tag = packageRoot.includes(`${path.sep}examples${path.sep}`) ? "example" : "first-party"; + bundledPlugins.push({ + packageName, + pluginKey: metadata.pluginKey ?? packageName, + displayName: metadata.displayName ?? titleCasePluginName(packageName), + description: metadata.description + ?? `Bundled Paperclip plugin from ${path.relative(REPO_ROOT, packageRoot)}.`, + localPath: packageRoot, + tag, + experimental: isExperimentalBundledPlugin(packageRoot, packageName), }); + } + + return bundledPlugins.sort((left, right) => { + if (left.tag !== right.tag) return left.tag === "first-party" ? -1 : 1; + return left.displayName.localeCompare(right.displayName); + }); } /** @@ -777,7 +781,7 @@ export function pluginRoutes( */ router.get("/plugins/examples", async (req, res) => { assertBoardOrgAccess(req); - res.json(listBundledPlugins()); + res.json(await listBundledPlugins()); }); // IMPORTANT: Static routes must come before parameterized routes diff --git a/server/src/services/workspace-runtime.ts b/server/src/services/workspace-runtime.ts index 11345d4f8fd..4daf8523b06 100644 --- a/server/src/services/workspace-runtime.ts +++ b/server/src/services/workspace-runtime.ts @@ -691,26 +691,24 @@ async function isGitCheckout(cwd: string): Promise { } async function detectDefaultBranch(repoRoot: string): Promise { - const originMasterRef = "origin/master"; - await refreshRemoteTrackingBaseRef(repoRoot, originMasterRef); - if (await resolveBaseRefSha(repoRoot, originMasterRef)) { - return originMasterRef; - } - // Try the explicit remote HEAD first (set by git clone or git remote set-head) try { const remoteHead = await runGit( ["symbolic-ref", "--quiet", "--short", "refs/remotes/origin/HEAD"], repoRoot, ); - if (remoteHead) return remoteHead; + if (remoteHead) { + await refreshRemoteTrackingBaseRef(repoRoot, remoteHead); + if (await resolveBaseRefSha(repoRoot, remoteHead)) return remoteHead; + } } catch { // Not set — fall through to heuristic } // Fallback: check for common default branch names on the remote - for (const candidate of ["origin/main", "main", "master"]) { + for (const candidate of ["origin/master", "origin/main", "main", "master"]) { try { + await refreshRemoteTrackingBaseRef(repoRoot, candidate); await runGit(["rev-parse", "--verify", `${candidate}^{commit}`], repoRoot); return candidate; } catch { diff --git a/ui/src/pages/PluginManager.tsx b/ui/src/pages/PluginManager.tsx index 29c0e08ef7d..4293d767435 100644 --- a/ui/src/pages/PluginManager.tsx +++ b/ui/src/pages/PluginManager.tsx @@ -43,19 +43,9 @@ function getPluginErrorSummary(plugin: PluginRecord): string { return firstNonEmptyLine(plugin.lastError) ?? "Plugin entered an error state without a stored error message."; } -const EXPERIMENTAL_PLUGIN_PACKAGE_NAMES = new Set([ - "@paperclipai/plugin-llm-wiki", - "@paperclipai/plugin-workspace-diff", -]); -const EXPERIMENTAL_PLUGIN_KEYS = new Set([ - "paperclip.workspace-diff", - "paperclipai.plugin-llm-wiki", -]); - function isExperimentalPluginIdentity(input: { packageName?: string | null; packagePath?: string | null; - pluginKey?: string | null; manifestJson?: PluginRecord["manifestJson"] | null; bundledExperimental?: boolean; }) { @@ -63,9 +53,6 @@ function isExperimentalPluginIdentity(input: { const packageName = input.packageName ?? ""; const packagePath = input.packagePath ?? ""; - const pluginKey = input.pluginKey ?? input.manifestJson?.id ?? ""; - if (EXPERIMENTAL_PLUGIN_PACKAGE_NAMES.has(packageName)) return true; - if (EXPERIMENTAL_PLUGIN_KEYS.has(pluginKey)) return true; if (packageName.includes("sandbox") || packagePath.includes("sandbox")) return true; return input.manifestJson?.environmentDrivers?.some((driver) => driver.kind === "sandbox_provider") === true; } @@ -290,7 +277,6 @@ export function PluginManager() { {isExperimentalPluginIdentity({ packageName: bundledPlugin.packageName, packagePath: bundledPlugin.localPath, - pluginKey: bundledPlugin.pluginKey, bundledExperimental: bundledPlugin.experimental, }) && } {installedPlugin ? ( @@ -389,7 +375,6 @@ export function PluginManager() { {isExperimentalPluginIdentity({ packageName: plugin.packageName, packagePath: plugin.packagePath, - pluginKey: plugin.pluginKey, manifestJson: plugin.manifestJson, bundledExperimental: bundledByPackageName.get(plugin.packageName)?.experimental, }) && }