Fix Greptile workspace review issues

2026-03-16 20:12:22 -05:00
parent 6dd4cc2840
commit 8d5af56fc5
4 changed files with 230 additions and 15 deletions
--- a/server/src/tests/workspace-runtime.test.ts
+++ b/server/src/tests/workspace-runtime.test.ts
@@ -218,6 +218,50 @@ describe("realizeExecutionWorkspace", () => {
    await expect(fs.readFile(path.join(reused.cwd, ".paperclip-provision-created"), "utf8")).resolves.toBe("false\n");
  });

+  it("reuses an existing branch without resetting it when recreating a missing worktree", async () => {
+    const repoRoot = await createTempRepo();
+    const branchName = "PAP-450-recreate-missing-worktree";
+
+    await runGit(repoRoot, ["checkout", "-b", branchName]);
+    await fs.writeFile(path.join(repoRoot, "feature.txt"), "preserve me\n", "utf8");
+    await runGit(repoRoot, ["add", "feature.txt"]);
+    await runGit(repoRoot, ["commit", "-m", "Add preserved feature"]);
+    const expectedHead = (await execFileAsync("git", ["rev-parse", branchName], { cwd: repoRoot })).stdout.trim();
+    await runGit(repoRoot, ["checkout", "main"]);
+
+    const workspace = await realizeExecutionWorkspace({
+      base: {
+        baseCwd: repoRoot,
+        source: "project_primary",
+        projectId: "project-1",
+        workspaceId: "workspace-1",
+        repoUrl: null,
+        repoRef: "HEAD",
+      },
+      config: {
+        workspaceStrategy: {
+          type: "git_worktree",
+          branchTemplate: "{{issue.identifier}}-{{slug}}",
+        },
+      },
+      issue: {
+        id: "issue-1",
+        identifier: "PAP-450",
+        title: "Recreate missing worktree",
+      },
+      agent: {
+        id: "agent-1",
+        name: "Codex Coder",
+        companyId: "company-1",
+      },
+    });
+
+    expect(workspace.branchName).toBe(branchName);
+    await expect(fs.readFile(path.join(workspace.cwd, "feature.txt"), "utf8")).resolves.toBe("preserve me\n");
+    const actualHead = (await execFileAsync("git", ["rev-parse", "HEAD"], { cwd: workspace.cwd })).stdout.trim();
+    expect(actualHead).toBe(expectedHead);
+  });
+
  it("removes a created git worktree and branch during cleanup", async () => {
    const repoRoot = await createTempRepo();

@@ -279,6 +323,72 @@ describe("realizeExecutionWorkspace", () => {
      stdout: "",
    });
  });
+
+  it("keeps an unmerged runtime-created branch and warns instead of force deleting it", async () => {
+    const repoRoot = await createTempRepo();
+
+    const workspace = await realizeExecutionWorkspace({
+      base: {
+        baseCwd: repoRoot,
+        source: "project_primary",
+        projectId: "project-1",
+        workspaceId: "workspace-1",
+        repoUrl: null,
+        repoRef: "HEAD",
+      },
+      config: {
+        workspaceStrategy: {
+          type: "git_worktree",
+          branchTemplate: "{{issue.identifier}}-{{slug}}",
+        },
+      },
+      issue: {
+        id: "issue-1",
+        identifier: "PAP-451",
+        title: "Keep unmerged branch",
+      },
+      agent: {
+        id: "agent-1",
+        name: "Codex Coder",
+        companyId: "company-1",
+      },
+    });
+
+    await fs.writeFile(path.join(workspace.cwd, "unmerged.txt"), "still here\n", "utf8");
+    await runGit(workspace.cwd, ["add", "unmerged.txt"]);
+    await runGit(workspace.cwd, ["commit", "-m", "Keep unmerged work"]);
+
+    const cleanup = await cleanupExecutionWorkspaceArtifacts({
+      workspace: {
+        id: "execution-workspace-1",
+        cwd: workspace.cwd,
+        providerType: "git_worktree",
+        providerRef: workspace.worktreePath,
+        branchName: workspace.branchName,
+        repoUrl: workspace.repoUrl,
+        baseRef: workspace.repoRef,
+        projectId: workspace.projectId,
+        projectWorkspaceId: workspace.workspaceId,
+        sourceIssueId: "issue-1",
+        metadata: {
+          createdByRuntime: true,
+        },
+      },
+      projectWorkspace: {
+        cwd: repoRoot,
+        cleanupCommand: null,
+      },
+    });
+
+    expect(cleanup.cleaned).toBe(true);
+    expect(cleanup.warnings).toHaveLength(1);
+    expect(cleanup.warnings[0]).toContain(`Skipped deleting branch "${workspace.branchName}"`);
+    await expect(
+      execFileAsync("git", ["branch", "--list", workspace.branchName!], { cwd: repoRoot }),
+    ).resolves.toMatchObject({
+      stdout: expect.stringContaining(workspace.branchName!),
+    });
+  });
 });

 describe("ensureRuntimeServicesForRun", () => {
@@ -514,6 +624,65 @@ describe("ensureRuntimeServicesForRun", () => {

    await expect(fetch(services[0]!.url!)).rejects.toThrow();
  });
+
+  it("does not stop services in sibling directories when matching by workspace cwd", async () => {
+    const workspaceParent = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-runtime-sibling-"));
+    const targetWorkspaceRoot = path.join(workspaceParent, "project");
+    const siblingWorkspaceRoot = path.join(workspaceParent, "project-extended", "service");
+    await fs.mkdir(targetWorkspaceRoot, { recursive: true });
+    await fs.mkdir(siblingWorkspaceRoot, { recursive: true });
+
+    const siblingWorkspace = buildWorkspace(siblingWorkspaceRoot);
+    const runId = "run-sibling";
+    leasedRunIds.add(runId);
+
+    const services = await ensureRuntimeServicesForRun({
+      runId,
+      agent: {
+        id: "agent-1",
+        name: "Codex Coder",
+        companyId: "company-1",
+      },
+      issue: null,
+      workspace: siblingWorkspace,
+      executionWorkspaceId: "execution-workspace-sibling",
+      config: {
+        workspaceRuntime: {
+          services: [
+            {
+              name: "web",
+              command:
+                "node -e \"require('node:http').createServer((req,res)=>res.end('ok')).listen(Number(process.env.PORT), '127.0.0.1')\"",
+              port: { type: "auto" },
+              readiness: {
+                type: "http",
+                urlTemplate: "http://127.0.0.1:{{port}}",
+                timeoutSec: 10,
+                intervalMs: 100,
+              },
+              lifecycle: "shared",
+              reuseScope: "execution_workspace",
+              stopPolicy: {
+                type: "manual",
+              },
+            },
+          ],
+        },
+      },
+      adapterEnv: {},
+    });
+
+    await stopRuntimeServicesForExecutionWorkspace({
+      executionWorkspaceId: "execution-workspace-target",
+      workspaceCwd: targetWorkspaceRoot,
+    });
+
+    const response = await fetch(services[0]!.url!);
+    expect(await response.text()).toBe("ok");
+
+    await releaseRuntimeServicesForRun(runId);
+    leasedRunIds.delete(runId);
+  });
 });

 describe("normalizeAdapterManagedRuntimeServices", () => {
--- a/server/src/services/workspace-runtime.ts
+++ b/server/src/services/workspace-runtime.ts
@@ -245,6 +245,11 @@ async function runGit(args: string[], cwd: string): Promise<string> {
  return proc.stdout.trim();
 }

+function gitErrorIncludes(error: unknown, needle: string) {
+  const message = error instanceof Error ? error.message : String(error);
+  return message.toLowerCase().includes(needle.toLowerCase());
+}
+
 async function directoryExists(value: string) {
  return fs.stat(value).then((stats) => stats.isDirectory()).catch(() => false);
 }
@@ -472,7 +477,14 @@ export async function realizeExecutionWorkspace(input: {
    throw new Error(`Configured worktree path "${worktreePath}" already exists and is not a git worktree.`);
  }

-  await runGit(["worktree", "add", "-B", branchName, worktreePath, baseRef], repoRoot);
+  try {
+    await runGit(["worktree", "add", "-b", branchName, worktreePath, baseRef], repoRoot);
+  } catch (error) {
+    if (!gitErrorIncludes(error, "already exists")) {
+      throw error;
+    }
+    await runGit(["worktree", "add", worktreePath, branchName], repoRoot);
+  }
  await provisionExecutionWorktree({
    strategy: rawStrategy,
    base: input.base,
@@ -564,9 +576,10 @@ export async function cleanupExecutionWorkspaceArtifacts(input: {
        warnings.push(`Could not resolve git repo root to delete branch "${input.workspace.branchName}".`);
      } else {
        try {
-          await runGit(["branch", "-D", input.workspace.branchName], repoRoot);
+          await runGit(["branch", "-d", input.workspace.branchName], repoRoot);
        } catch (err) {
-          warnings.push(err instanceof Error ? err.message : String(err));
+          const message = err instanceof Error ? err.message : String(err);
+          warnings.push(`Skipped deleting branch "${input.workspace.branchName}": ${message}`);
        }
      }
    }
@@ -1131,7 +1144,11 @@ export async function stopRuntimeServicesForExecutionWorkspace(input: {
    .filter((record) => {
      if (record.executionWorkspaceId === input.executionWorkspaceId) return true;
      if (!normalizedWorkspaceCwd || !record.cwd) return false;
-      return path.resolve(record.cwd).startsWith(normalizedWorkspaceCwd);
+      const resolvedCwd = path.resolve(record.cwd);
+      return (
+        resolvedCwd === normalizedWorkspaceCwd ||
+        resolvedCwd.startsWith(`${normalizedWorkspaceCwd}${path.sep}`)
+      );
    })
    .map((record) => record.id);

--- a/ui/src/components/ProjectProperties.tsx
+++ b/ui/src/components/ProjectProperties.tsx
@@ -284,6 +284,16 @@ export function ProjectProperties({ project, onUpdate, onFieldUpdate, getFieldSa
    }
  };

+  const isSafeExternalUrl = (value: string | null | undefined) => {
+    if (!value) return false;
+    try {
+      const parsed = new URL(value);
+      return parsed.protocol === "http:" || parsed.protocol === "https:";
+    } catch {
+      return false;
+    }
+  };
+
  const formatRepoUrl = (value: string) => {
    try {
      const parsed = new URL(value);
@@ -539,16 +549,23 @@ export function ProjectProperties({ project, onUpdate, onFieldUpdate, getFieldSa
              <div className="text-[11px] uppercase tracking-wide text-muted-foreground">Repo</div>
              {codebase.repoUrl ? (
                <div className="flex items-center justify-between gap-2">
-                  <a
-                    href={codebase.repoUrl}
-                    target="_blank"
-                    rel="noreferrer"
-                    className="inline-flex min-w-0 items-center gap-1.5 text-xs text-muted-foreground hover:text-foreground hover:underline"
-                  >
-                    <Github className="h-3 w-3 shrink-0" />
-                    <span className="truncate">{formatRepoUrl(codebase.repoUrl)}</span>
-                    <ExternalLink className="h-3 w-3 shrink-0" />
-                  </a>
+                  {isSafeExternalUrl(codebase.repoUrl) ? (
+                    <a
+                      href={codebase.repoUrl}
+                      target="_blank"
+                      rel="noreferrer"
+                      className="inline-flex min-w-0 items-center gap-1.5 text-xs text-muted-foreground hover:text-foreground hover:underline"
+                    >
+                      <Github className="h-3 w-3 shrink-0" />
+                      <span className="truncate">{formatRepoUrl(codebase.repoUrl)}</span>
+                      <ExternalLink className="h-3 w-3 shrink-0" />
+                    </a>
+                  ) : (
+                    <div className="inline-flex min-w-0 items-center gap-1.5 text-xs text-muted-foreground">
+                      <Github className="h-3 w-3 shrink-0" />
+                      <span className="truncate">{codebase.repoUrl}</span>
+                    </div>
+                  )}
                  <div className="flex items-center gap-1">
                    <Button
                      variant="outline"
--- a/ui/src/pages/ExecutionWorkspaceDetail.tsx
+++ b/ui/src/pages/ExecutionWorkspaceDetail.tsx
@@ -4,6 +4,16 @@ import { ExternalLink } from "lucide-react";
 import { executionWorkspacesApi } from "../api/execution-workspaces";
 import { queryKeys } from "../lib/queryKeys";

+function isSafeExternalUrl(value: string | null | undefined) {
+  if (!value) return false;
+  try {
+    const parsed = new URL(value);
+    return parsed.protocol === "http:" || parsed.protocol === "https:";
+  } catch {
+    return false;
+  }
+}
+
 function DetailRow({ label, children }: { label: string; children: React.ReactNode }) {
  return (
    <div className="flex items-start gap-3 py-1.5">
@@ -52,11 +62,13 @@ export function ExecutionWorkspaceDetail() {
          <span className="break-all font-mono text-xs">{workspace.providerRef ?? "None"}</span>
        </DetailRow>
        <DetailRow label="Repo URL">
-          {workspace.repoUrl ? (
+          {workspace.repoUrl && isSafeExternalUrl(workspace.repoUrl) ? (
            <a href={workspace.repoUrl} target="_blank" rel="noreferrer" className="inline-flex items-center gap-1 hover:underline">
              {workspace.repoUrl}
              <ExternalLink className="h-3 w-3" />
            </a>
+          ) : workspace.repoUrl ? (
+            <span className="break-all font-mono text-xs">{workspace.repoUrl}</span>
          ) : "None"}
        </DetailRow>
        <DetailRow label="Opened">{new Date(workspace.openedAt).toLocaleString()}</DetailRow>