fix: sanitize multi-repo worktree dirs and polish task-create chip dropdown (#815)

* fix: sanitize multi-repo worktree dirs and polish task-create chip dropdown Backend: org-prefixed Repository.Name like "owner/repo" was used verbatim as the worktree subdirectory under the task root, nesting one level too deep and breaking agentctl's sibling-based multi-repo detection in scanRepositorySubdirs. Sanitize to a single path-safe segment so all worktrees land as siblings of the task root. Web: the multi-repo chip dropdown built its own option list and dropped the path Badge plus search filtering that the legacy header dropdown shipped. Restore parity (name + path), widen the popover (288 → 360 px), stack name and full path on two lines so neither has to truncate aggressively, hide the Badge for workspace repos without a local clone, and use a path-aware scorer that floors weak fuzzy matches so unrelated paths stop surfacing for short queries. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: address PR review feedback - Add ErrInvalidRepoName sentinel and use it in createInTaskDir when RepoName sanitizes to empty, replacing the misleading reuse of ErrTaskDirRequired (Claude/Greptile). - Reuse repoDirHyphenRun in SanitizeForBranch instead of recompiling the regexp per call (CodeRabbit nitpick). - Document the SanitizeRepoDirName collision edge case in the doc comment (Greptile). - Extract scoreRepo to lib/utils/repo-filter.ts and add unit tests pinning the 0.4 fuzzy-match threshold (Claude). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Kandev Agent <agent@kandev.dev> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-04 17:22:28 +01:00
parent 96c68a0af9
commit b30933240b
11 changed files with 232 additions and 21 deletions
@@ -116,8 +116,7 @@ func SanitizeForBranch(title string, maxLen int) string {
 	result = sb.String()

 	// Remove consecutive hyphens
-	re := regexp.MustCompile(`-+`)
-	result = re.ReplaceAllString(result, "-")
+	result = repoDirHyphenRun.ReplaceAllString(result, "-")

 	// Remove leading and trailing hyphens
 	result = strings.Trim(result, "-")
@@ -179,6 +178,43 @@ func SmallSuffix(maxLen int) string {
 	return string(buf)
 }

+// SanitizeRepoDirName converts a repository display name into a single,
+// filesystem-safe path segment. Path separators and other unsafe characters
+// are replaced with hyphens, runs of hyphens are collapsed, and surrounding
+// hyphens/dots are trimmed. Returns an empty string when the input has no
+// usable characters.
+//
+// This guards against names like "owner/repo" producing nested subdirectories
+// when used as the worktree directory under a multi-repo task root — the
+// extra path level breaks sibling-repo detection in agentctl.
+//
+// Limitation: distinct names that differ only in unsafe characters (e.g.
+// "acme/widget-config" and "acme-widget-config") collapse to the same
+// segment. Two such repos in one task would collide on `git worktree add`.
+// Acceptable in practice — provider repos (the common case) are uniquely
+// identified by owner/name and won't collide with each other.
+func SanitizeRepoDirName(name string) string {
+	if name == "" {
+		return ""
+	}
+	var sb strings.Builder
+	sb.Grow(len(name))
+	for _, r := range name {
+		switch {
+		case unicode.IsLetter(r), unicode.IsDigit(r):
+			sb.WriteRune(r)
+		case r == '_', r == '.', r == '-':
+			sb.WriteRune(r)
+		default:
+			sb.WriteRune('-')
+		}
+	}
+	result := repoDirHyphenRun.ReplaceAllString(sb.String(), "-")
+	return strings.Trim(result, "-.")
+}
+
+var repoDirHyphenRun = regexp.MustCompile(`-+`)
+
 // SemanticWorktreeName generates a semantic worktree directory name from a task title.
 // Format: {sanitizedTitle}_{suffix} e.g. fix-login-bug_ab12cd34
 // The title is truncated to 20 characters before adding the suffix.
@@ -0,0 +1,29 @@
+package worktree
+
+import "testing"
+
+func TestSanitizeRepoDirName(t *testing.T) {
+	cases := []struct {
+		in, want string
+	}{
+		{"widget-config", "widget-config"},
+		{"acme/widget-config", "acme-widget-config"},
+		{"acme/widget", "acme-widget"},
+		{"owner\\repo", "owner-repo"},
+		{"weird:name space", "weird-name-space"},
+		{"with..dots", "with..dots"},
+		{"trailing/", "trailing"},
+		{"/leading", "leading"},
+		{"a//b", "a-b"},
+		{"-a-b-", "a-b"},
+		{".hidden", "hidden"},
+		{"!@#$%", ""},
+		{"", ""},
+		{"under_score.dot-dash", "under_score.dot-dash"},
+	}
+	for _, c := range cases {
+		if got := SanitizeRepoDirName(c.in); got != c.want {
+			t.Errorf("SanitizeRepoDirName(%q) = %q, want %q", c.in, got, c.want)
+		}
+	}
+}
@@ -52,6 +52,11 @@ var (
 	// directory (~/.kandev/tasks/{taskDir}/{repo}/), so callers must populate
 	// both fields.
 	ErrTaskDirRequired = errors.New("worktree create requires TaskDirName and RepoName")
+
+	// ErrInvalidRepoName is returned when RepoName is set but contains no
+	// characters that survive sanitization (e.g. "!@#$%"), so it cannot be
+	// used as a directory segment.
+	ErrInvalidRepoName = errors.New("repo name has no usable characters after sanitization")
 )

 // containsAuthFailure checks if git output indicates an authentication failure.
@@ -290,8 +290,17 @@ func (m *Manager) Create(ctx context.Context, req CreateRequest) (*Worktree, err

 // createInTaskDir creates a worktree inside the task directory structure:
 // ~/.kandev/tasks/{taskDirName}/{repoName}/
+//
+// RepoName is sanitized to a single path segment so display names like
+// "owner/repo" don't produce a nested subdirectory — that would push the
+// worktree one level below the task root and break agentctl's sibling-based
+// multi-repo detection.
 func (m *Manager) createInTaskDir(ctx context.Context, req CreateRequest, baseRef string) (*Worktree, error) {
-	worktreePath, err := m.config.TaskWorktreePath(req.TaskDirName, req.RepoName)
+	repoDir := SanitizeRepoDirName(req.RepoName)
+	if repoDir == "" {
+		return nil, ErrInvalidRepoName
+	}
+	worktreePath, err := m.config.TaskWorktreePath(req.TaskDirName, repoDir)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get task worktree path: %w", err)
 	}
@@ -3,6 +3,7 @@ package worktree
 import (
 	"context"
 	"os/exec"
+	"path/filepath"
 	"strings"
 	"testing"
 )
@@ -109,3 +110,53 @@ func TestCreateInTaskDir_RemoteBaseRefDoesNotSetUpstream(t *testing.T) {
 		t.Fatalf("expected no upstream for new task branch, but got %q", strings.TrimSpace(string(out)))
 	}
 }
+
+// Org-prefixed RepoName ("owner/repo") used to nest the worktree one level
+// below the task root, breaking agentctl's sibling-based multi-repo detection.
+// The worktree directory must sit directly under the task root so two repos
+// (clean + org-prefixed) end up as siblings.
+func TestCreateInTaskDir_OrgPrefixedRepoNameLandsAsSibling(t *testing.T) {
+	cfg := newTestConfig(t)
+	store := newMockStore()
+	mgr, err := NewManager(cfg, store, newTestLogger())
+	if err != nil {
+		t.Fatalf("NewManager: %v", err)
+	}
+
+	repoA := initGitRepoWithRemote(t)
+	repoB := initGitRepoWithRemote(t)
+	const taskDir = "test-task"
+
+	wtA, err := mgr.Create(context.Background(), CreateRequest{
+		TaskID: "t-1", SessionID: "s-1", TaskTitle: "T",
+		RepositoryID:   "repo-a",
+		RepositoryPath: repoA,
+		BaseBranch:     "main",
+		TaskDirName:    taskDir,
+		RepoName:       "arthur",
+	})
+	if err != nil {
+		t.Fatalf("Create A: %v", err)
+	}
+
+	wtB, err := mgr.Create(context.Background(), CreateRequest{
+		TaskID: "t-1", SessionID: "s-1", TaskTitle: "T",
+		RepositoryID:   "repo-b",
+		RepositoryPath: repoB,
+		BaseBranch:     "main",
+		TaskDirName:    taskDir,
+		RepoName:       "acme/widget-config",
+	})
+	if err != nil {
+		t.Fatalf("Create B: %v", err)
+	}
+
+	parentA := filepath.Dir(wtA.Path)
+	parentB := filepath.Dir(wtB.Path)
+	if parentA != parentB {
+		t.Fatalf("worktrees not siblings: A parent %q vs B parent %q", parentA, parentB)
+	}
+	if base := filepath.Base(wtB.Path); base != "acme-widget-config" {
+		t.Errorf("repo B dir = %q, want %q", base, "acme-widget-config")
+	}
+}
@@ -48,13 +48,15 @@ export function useRepositoryOptions(
        renderLabel: () => (
          <span className="flex min-w-0 flex-1 items-center gap-2 overflow-hidden">
            <span className="shrink-0">{repo.name}</span>
-            <Badge
-              variant="secondary"
-              className="text-xs text-muted-foreground max-w-[140px] min-w-0 truncate ml-auto"
-              title={formatUserHomePath(repo.local_path)}
-            >
-              {truncateRepoPath(repo.local_path, 24)}
-            </Badge>
+            {repo.local_path ? (
+              <Badge
+                variant="secondary"
+                className="text-xs text-muted-foreground max-w-[140px] min-w-0 truncate ml-auto"
+                title={formatUserHomePath(repo.local_path)}
+              >
+                {truncateRepoPath(repo.local_path, 24)}
+              </Badge>
+            ) : null}
          </span>
        ),
      })),
@@ -206,7 +206,7 @@ export function Pill({
      <PopoverTrigger asChild>
        {tooltip ? <TooltipTrigger asChild>{triggerButton}</TooltipTrigger> : triggerButton}
      </PopoverTrigger>
-      <PopoverContent className="w-72 p-0" align="start" portal={false}>
+      <PopoverContent className="w-[360px] p-0" align="start" portal={false}>
        <Command filter={filter}>
          <div className="flex items-center gap-1 px-2 pt-1">
            <CommandInput placeholder={searchPlaceholder} className="h-9 flex-1" />
@@ -273,7 +273,7 @@ describe("RepoChipsRow", () => {
    );
    fireEvent.click(screen.getByTestId("repo-chip-trigger"));
    expect(screen.getByText("frontend")).toBeTruthy();
-    expect(screen.getByText("projects/local-project")).toBeTruthy();
+    expect(screen.getByText("~/projects/local-project")).toBeTruthy();
    expect(screen.queryByText("frontend-dup")).toBeNull();
  });

@@ -299,7 +299,7 @@ describe("RepoChipsRow", () => {
      />,
    );
    fireEvent.click(screen.getByTestId("repo-chip-trigger"));
-    fireEvent.click(screen.getByText("projects/local-project"));
+    fireEvent.click(screen.getByText("~/projects/local-project"));
    expect(onRowRepositoryChange).toHaveBeenCalledWith("r0", "/home/me/projects/local-project");
  });

@@ -3,12 +3,14 @@
 import { useEffect, useMemo } from "react";
 import { IconPlus, IconX, IconCode, IconGitBranch, IconGitFork } from "@tabler/icons-react";
 import { cn, formatUserHomePath } from "@/lib/utils";
+import { Badge } from "@kandev/ui/badge";
 import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip";
 import { useBranches, type BranchSource } from "@/hooks/domains/workspace/use-repository-branches";
 import type { LocalRepository, Repository } from "@/lib/types/http";
 import type { DialogFormState, TaskRepoRow } from "@/components/task-create-dialog-types";
 import { autoSelectBranch } from "@/components/task-create-dialog-helpers";
 import { scoreBranch } from "@/lib/utils/branch-filter";
+import { scoreRepo } from "@/lib/utils/repo-filter";
 import {
  Pill,
  sortBranches,
@@ -461,11 +463,19 @@ function useRepoChipData({

  const repoOptions: PillOption[] = useMemo(
    () => [
-      ...filteredRepos.map((r) => ({ value: r.id, label: r.name })),
+      ...filteredRepos.map((r) => ({
+        value: r.id,
+        label: r.name,
+        keywords: [r.name, r.local_path, formatUserHomePath(r.local_path)].filter(
+          (s): s is string => !!s,
+        ),
+        renderLabel: () => renderWorkspaceRepoOption(r),
+      })),
      ...filteredDiscovered.map((r) => ({
        value: r.path,
-        label: shortRepoPath(r.path),
-        keywords: [r.path],
+        label: leafSegment(r.path),
+        keywords: [r.path, formatUserHomePath(r.path)],
+        renderLabel: () => renderDiscoveredRepoOption(r.path),
      })),
    ],
    [filteredRepos, filteredDiscovered],
@@ -546,6 +556,7 @@ function RepoChip({
        emptyMessage="No repositories"
        testId="repo-chip-trigger"
        tooltip={repoTooltip}
+        filter={scoreRepo}
        flat
      />
      <Pill
@@ -593,9 +604,35 @@ function normalizeRepoPath(path: string): string {
  return path.replace(/\\/g, "/").replace(/\/+$/g, "");
 }

-function shortRepoPath(path: string): string {
-  // Show the trailing folder name with one parent for context (e.g. "myorg/myrepo").
-  const parts = path.replace(/\\/g, "/").split("/").filter(Boolean);
-  if (parts.length <= 1) return path;
-  return parts.slice(-2).join("/");
+function renderWorkspaceRepoOption(repo: Repository) {
+  const display = repo.local_path ? formatUserHomePath(repo.local_path) : "";
+  return (
+    <span className="flex min-w-0 flex-1 flex-col overflow-hidden" title={display || repo.name}>
+      <span className="truncate">{repo.name}</span>
+      {display ? (
+        <span className="truncate text-[11px] text-muted-foreground">{display}</span>
+      ) : null}
+    </span>
+  );
+}
+
+function renderDiscoveredRepoOption(path: string) {
+  const display = formatUserHomePath(path);
+  return (
+    <span className="flex min-w-0 flex-1 items-center gap-2 overflow-hidden" title={display}>
+      <span className="flex min-w-0 flex-1 flex-col overflow-hidden">
+        <span className="truncate">{leafSegment(path)}</span>
+        <span className="truncate text-[11px] text-muted-foreground">{display}</span>
+      </span>
+      <Badge variant="outline" className="text-[10px] text-muted-foreground shrink-0">
+        on disk
+      </Badge>
+    </span>
+  );
+}
+
+function leafSegment(path: string): string {
+  const cleaned = path.replace(/\\/g, "/").replace(/\/+$/g, "");
+  const idx = cleaned.lastIndexOf("/");
+  return idx >= 0 ? cleaned.slice(idx + 1) : cleaned;
 }
@@ -0,0 +1,26 @@
+import { describe, it, expect } from "vitest";
+import { scoreRepo } from "./repo-filter";
+
+describe("scoreRepo", () => {
+  it("returns a positive score for an exact keyword match", () => {
+    expect(scoreRepo("uuid-123", "arthur", ["arthur"])).toBeGreaterThan(0);
+  });
+
+  it("returns a positive score for a substring/segment hit on the value", () => {
+    expect(scoreRepo("acme/widget-config", "widget")).toBeGreaterThan(0);
+  });
+
+  it("floors weak subsequence-only matches that scoreBranch would surface", () => {
+    // a-r-t-h-u-r appears in order across the path but no segment contains
+    // "arthur" — exactly the false positive scoreRepo is designed to kill.
+    expect(scoreRepo("playground/fun/thm/gameserver/lxd-alpine-builder", "arthur")).toBe(0);
+  });
+
+  it("returns a positive score when the search prefixes the value's leaf", () => {
+    expect(scoreRepo("playground/sky/arthur", "arth")).toBeGreaterThan(0);
+  });
+
+  it("returns 0 when nothing in value or keywords resembles the search", () => {
+    expect(scoreRepo("playground/foo", "completely-unrelated", ["foo"])).toBe(0);
+  });
+});
@@ -0,0 +1,16 @@
+import { scoreBranch } from "./branch-filter";
+
+// scoreRepo wraps scoreBranch but drops weak subsequence-only matches.
+//
+// scoreBranch's fuzzy fallback returns small non-zero scores when search
+// characters appear in order across an arbitrary haystack — useful for
+// branch typos, but noisy for repo paths. A query like "arthur" was
+// matching long paths like "playground/fun/thm/.../lxd-alpine-builder"
+// because a-r-t-h-u-r appears in order across path segments.
+//
+// scoreBranch awards >= 0.45 for any substring/prefix/word-boundary hit,
+// so a 0.4 floor preserves real matches while filtering coincidences.
+export function scoreRepo(value: string, search: string, keywords?: string[]): number {
+  const score = scoreBranch(value, search, keywords);
+  return score >= 0.4 ? score : 0;
+}