Address Greptile PR feedback

Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
DottaandPaperclip committed 2026-05-26 13:14:31 +00:00
1 parent 2993a220a3
commit 7e21c5b872
4 files changed
+63 -78

No files matched your search

@@ -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 () => {
+52 -48
View File
@@ -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<string, unknown> | null {
async function fileExists(filePath: string): Promise<boolean> {
return access(filePath).then(() => true, () => false);
}
async function readJsonFile(filePath: string): Promise<Record<string, unknown> | null> {
try {
return JSON.parse(readFileSync(filePath, "utf8")) as Record<string, unknown>;
return JSON.parse(await readFile(filePath, "utf8")) as Record<string, unknown>;
} catch {
return null;
}
}
function findPackageJsonFiles(root: string, maxDepth = 4): string[] {
if (!existsSync(root)) return [];
async function findPackageJsonFiles(root: string, maxDepth = 4): Promise<string[]> {
if (!(await fileExists(root))) return [];
const packageJsonFiles: string[] = [];
const walk = (dir: string, depth: number) => {
const walk = async (dir: string, depth: number): Promise<void> => {
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<string, unknown>,
): { 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<AvailableBundledPlugin[]> {
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
+6 -8
View File
@@ -691,26 +691,24 @@ async function isGitCheckout(cwd: string): Promise<boolean> {
}
async function detectDefaultBranch(repoRoot: string): Promise<string | null> {
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 {
-15
View File
@@ -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,
}) && <ExperimentalBadge />}
{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,
}) && <ExperimentalBadge />}