From 146419fc8cabbece10a11ef79139375281b82ea3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Wed, 12 Aug 2026 10:53:07 +0100 Subject: [PATCH] fix(search): include workspace root with submodules (#2524) --- .../process/workspace_content_search.go | 2 +- .../server/process/workspace_files.go | 11 +- .../workspace_search_submodule_test.go | 98 ++++++++++ .../github/service_pr_unwatched_test.go | 16 +- apps/web/e2e/tests/command-panel.spec.ts | 45 +++++ .../layout/workspace-picker-shortcut.spec.ts | 169 ++++++++++-------- .../workspace-deletion-fallback.spec.ts | 6 +- .../tests/review/submodule-review-helpers.ts | 124 ++++++------- .../e2e/tests/review/submodule-review.spec.ts | 23 ++- docs/plans/submodule-workspace-search/plan.md | 125 +++++++++++++ .../task-01-correct-mixed-graph-search.md | 96 ++++++++++ .../task-02-prove-command-palette-search.md | 77 ++++++++ .../specs/ui/task-workspace-content-search.md | 18 ++ 13 files changed, 665 insertions(+), 145 deletions(-) create mode 100644 apps/backend/internal/agentctl/server/process/workspace_search_submodule_test.go create mode 100644 docs/plans/submodule-workspace-search/plan.md create mode 100644 docs/plans/submodule-workspace-search/task-01-correct-mixed-graph-search.md create mode 100644 docs/plans/submodule-workspace-search/task-02-prove-command-palette-search.md diff --git a/apps/backend/internal/agentctl/server/process/workspace_content_search.go b/apps/backend/internal/agentctl/server/process/workspace_content_search.go index eee8f8fbad..a37e0ae469 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_content_search.go +++ b/apps/backend/internal/agentctl/server/process/workspace_content_search.go @@ -84,7 +84,7 @@ func (m *Manager) contentSearchTrackers() []*WorkspaceTracker { } return trackers } - if root != nil && root.RepositoryName() != "" { + if root != nil && root.gitIndexPath != "" { trackers = append(trackers, root) } trackers = append(trackers, repositories...) diff --git a/apps/backend/internal/agentctl/server/process/workspace_files.go b/apps/backend/internal/agentctl/server/process/workspace_files.go index efd8c17192..3aca77f290 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files.go @@ -81,8 +81,9 @@ func (wt *WorkspaceTracker) getFileListClass(ctx context.Context, class subproc. // --cached: include tracked files // --others: include untracked files // --exclude-standard: respect .gitignore + // --stage: expose tracked file modes so submodule Gitlinks can be excluded out, runErr, execCtxErr := subproc.RunGitOutputAfterAcquire(ctx, class, gitCommandTimeout, func(execCtx context.Context) *exec.Cmd { - cmd := subproc.NewGitCommand(execCtx, "ls-files", "--cached", "--others", "--exclude-standard") + cmd := subproc.NewGitCommand(execCtx, "ls-files", "--cached", "--others", "--exclude-standard", "--stage") cmd.Dir = wt.workDir return cmd }) @@ -94,6 +95,12 @@ func (wt *WorkspaceTracker) getFileListClass(ctx context.Context, class subproc. lines := strings.Split(string(out), "\n") for _, line := range lines { line = strings.TrimSpace(line) + if metadata, path, tracked := strings.Cut(line, "\t"); tracked { + if strings.HasPrefix(metadata, "160000 ") { + continue + } + line = strings.TrimSpace(path) + } if line == "" || isRootOwnershipMarkerPath(line) { continue } @@ -1010,7 +1017,7 @@ func (m *Manager) SearchWorkspaceFileResults(query string, limit int) []types.Fi } candidates := make([]fileSearchCandidate, 0) - if root != nil && root.RepositoryName() != "" { + if root != nil && root.gitIndexPath != "" { candidates = appendTrackerFileSearchCandidates(candidates, root) } for _, tracker := range repositories { diff --git a/apps/backend/internal/agentctl/server/process/workspace_search_submodule_test.go b/apps/backend/internal/agentctl/server/process/workspace_search_submodule_test.go new file mode 100644 index 0000000000..f7da322805 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/workspace_search_submodule_test.go @@ -0,0 +1,98 @@ +package process + +import ( + "context" + "reflect" + "testing" + + "github.com/kandev/kandev/internal/agentctl/server/config" +) + +func TestManagerSearchWorkspaceFileResultsIncludesRootAndSubmodule(t *testing.T) { + mgr := setupWorkspaceSearchSubmoduleGraph(t) + updateWorkspaceSearchFileLists(mgr) + + results := mgr.SearchWorkspaceFileResults("search-target", 20) + got := make([]string, 0, len(results)) + for _, result := range results { + got = append(got, result.RepositoryName+"|"+result.Path) + } + want := []string{ + "|root-search-target.txt", + "vendor/lib|vendor/lib/child-search-target.txt", + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("search results = %v, want %v", got, want) + } +} + +func TestManagerSearchWorkspaceFileResultsExcludesSubmoduleGitlink(t *testing.T) { + mgr := setupWorkspaceSearchSubmoduleGraph(t) + updateWorkspaceSearchFileLists(mgr) + + for _, result := range mgr.SearchWorkspaceFileResults("lib", 20) { + if result.RepositoryName == "" && result.Path == "vendor/lib" { + t.Fatalf("root search returned submodule Gitlink as a file: %+v", result) + } + } + + childFound := false + for _, result := range mgr.SearchWorkspaceFileResults("child-search-target", 20) { + if result.RepositoryName == "vendor/lib" && result.Path == "vendor/lib/child-search-target.txt" { + childFound = true + break + } + } + if !childFound { + t.Fatal("real file inside child scope was not searchable") + } +} + +func TestManagerSearchWorkspaceContentIncludesRootAndSubmodule(t *testing.T) { + mgr := setupWorkspaceSearchSubmoduleGraph(t) + + response, err := mgr.SearchWorkspaceContent(context.Background(), "shared-content-needle", 20) + if err != nil { + t.Fatalf("SearchWorkspaceContent: %v", err) + } + got := make([]string, 0, len(response.Results)) + for _, result := range response.Results { + got = append(got, result.RepositoryName+"|"+result.Path) + } + want := []string{ + "|root-search-target.txt", + "vendor/lib|child-search-target.txt", + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("content search results = %v, want %v", got, want) + } +} + +func setupWorkspaceSearchSubmoduleGraph(t *testing.T) *Manager { + t.Helper() + parent, parentCleanup := setupTestRepo(t) + t.Cleanup(parentCleanup) + child, childCleanup := setupTestRepo(t) + t.Cleanup(childCleanup) + + writeFile(t, parent, "root-search-target.txt", "shared-content-needle\n") + writeFile(t, child, "child-search-target.txt", "shared-content-needle\n") + runGit(t, child, "add", ".") + runGit(t, child, "commit", "-m", "add search fixture") + runGit(t, parent, "-c", "protocol.file.allow=always", "submodule", "add", child, "vendor/lib") + runGit(t, parent, "add", ".") + runGit(t, parent, "commit", "-m", "add submodule search fixture") + runGit(t, parent, "push", "origin", "main") + + mgr := NewManager(&config.InstanceConfig{WorkDir: parent}, newTestLogger(t)) + t.Cleanup(mgr.stopWorkspaceTrackers) + return mgr +} + +func updateWorkspaceSearchFileLists(mgr *Manager) { + root, repositories := mgr.snapshotTrackers() + root.updateFiles(context.Background()) + for _, tracker := range repositories { + tracker.updateFiles(context.Background()) + } +} diff --git a/apps/backend/internal/github/service_pr_unwatched_test.go b/apps/backend/internal/github/service_pr_unwatched_test.go index 5d8817fa1f..5ca217ff4b 100644 --- a/apps/backend/internal/github/service_pr_unwatched_test.go +++ b/apps/backend/internal/github/service_pr_unwatched_test.go @@ -169,9 +169,19 @@ func TestRefreshStaleWorkspaceWatches_HealsUnwatchedRow(t *testing.T) { // hands it to the background goroutine; the join it uses needs the real // tasks schema, so drive the goroutine directly. svc.refreshStaleWorkspaceWatches(testWorkspaceID, map[string]struct{}{"task-1": {}}) - awaitTaskPRUpdated(t, updated) - // Drain the goroutine before the in-memory DB closes. Stop() cancels - // stopCtx, so it has to come after the write we just observed. + unwatchedUpdate := awaitTaskPRUpdated(t, updated) + if unwatchedUpdate.PRNumber != 1293 { + t.Fatalf("first background update PR = %d, want unwatched PR 1293", unwatchedUpdate.PRNumber) + } + // The same goroutine updates the watched PR after the unwatched row. Wait + // for that write too: canceling it via Stop can make sqlite discard the + // sole :memory: connection and leave the final assertion without a schema. + watchedUpdate := awaitTaskPRUpdated(t, updated) + if watchedUpdate.PRNumber != 1299 { + t.Fatalf("second background update PR = %d, want watched PR 1299", watchedUpdate.PRNumber) + } + // Drain the goroutine before the in-memory DB closes. Both observable writes + // have completed, so Stop has no database operation left to cancel. svc.Stop() got, err := store.GetTaskPRByRepoAndNumber(ctx, "task-1", "repo-1", 1293) diff --git a/apps/web/e2e/tests/command-panel.spec.ts b/apps/web/e2e/tests/command-panel.spec.ts index 7814944b3b..0fffe7a4e1 100644 --- a/apps/web/e2e/tests/command-panel.spec.ts +++ b/apps/web/e2e/tests/command-panel.spec.ts @@ -2,6 +2,7 @@ import { test, expect } from "../fixtures/test-base"; import { KanbanPage } from "../pages/kanban-page"; import { SessionPage } from "../pages/session-page"; import { waitForSessionDone } from "../helpers/session"; +import { createSubmoduleReviewFixture } from "./review/submodule-review-helpers"; type CommandAliasExpectation = { query: string; @@ -231,6 +232,50 @@ test.describe("Command Panel", () => { await expect(dialog.getByText("Type to search files...")).toBeVisible(); }); + test("Cmd+Shift+K finds root and submodule files", async ({ + testPage, + apiClient, + seedData, + backend, + }) => { + test.setTimeout(180_000); + const fixture = await createSubmoduleReviewFixture( + apiClient, + seedData, + backend.tmpDir, + "Submodule file search E2E", + ); + + try { + await testPage.goto(`/t/${fixture.taskId}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await session.waitForChatIdle({ timeout: 45_000 }); + await fixture.waitForWorktree(apiClient); + + await openFileSearch(testPage); + const dialog = commandDialog(testPage); + await expect(dialog).toBeVisible({ timeout: 5_000 }); + await dialog.getByRole("combobox").fill("README.md"); + + const groups = dialog.getByTestId("file-search-repo-group"); + await expect(groups).toHaveCount(3, { timeout: 45_000 }); + await expect( + dialog.locator('[data-testid="file-search-repo-group"][data-repository=""]'), + ).toBeVisible(); + await expect( + dialog.locator('[data-testid="file-search-repo-group"][data-repository="vendor/outer"]'), + ).toBeVisible(); + await expect( + dialog.locator( + '[data-testid="file-search-repo-group"][data-repository="vendor/outer/vendor/inner"]', + ), + ).toBeVisible(); + } finally { + fixture.cleanup(); + } + }); + test("Cmd+K inline task search shows matching tasks", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/layout/workspace-picker-shortcut.spec.ts b/apps/web/e2e/tests/layout/workspace-picker-shortcut.spec.ts index 746b6c6a48..838a74232f 100644 --- a/apps/web/e2e/tests/layout/workspace-picker-shortcut.spec.ts +++ b/apps/web/e2e/tests/layout/workspace-picker-shortcut.spec.ts @@ -1,5 +1,7 @@ import { test, expect } from "../../fixtures/test-base"; import type { Page } from "@playwright/test"; +import type { ApiClient } from "../../helpers/api-client"; +import type { Workspace } from "../../../lib/types/http"; // WORKSPACE_PICKER (Cmd/Ctrl+E by default) opens the sidebar-header // workspace switcher with focus already inside the menu, so a workspace can be @@ -24,108 +26,121 @@ async function listedWorkspaceIds(page: Page): Promise { return testIds.map((testId) => testId.replace("sidebar-workspace-item-", "")); } +async function withDisposableWorkspace( + apiClient: ApiClient, + name: string, + run: (workspace: Workspace) => Promise, +): Promise { + const workspace = await apiClient.createWorkspace(name); + try { + await run(workspace); + } finally { + await apiClient.deleteWorkspace(workspace.id, workspace.name); + } +} + test.describe("Workspace picker shortcut (global)", () => { test("opens the picker and switches workspace with arrows and Enter", async ({ testPage, apiClient, }) => { - const second = await apiClient.createWorkspace("Picker Shortcut Workspace"); + await withDisposableWorkspace(apiClient, "Picker Shortcut Workspace", async (second) => { + await testPage.goto("/"); + await expect(testPage.getByTestId("sidebar-workspace-trigger")).toBeVisible({ + timeout: 10_000, + }); - await testPage.goto("/"); - await expect(testPage.getByTestId("sidebar-workspace-trigger")).toBeVisible({ - timeout: 10_000, + await pressWorkspacePickerShortcut(testPage); + + await expect(testPage.getByTestId("sidebar-workspace-trigger")).toHaveAttribute( + "data-state", + "open", + ); + const workspaceIds = await listedWorkspaceIds(testPage); + expect(workspaceIds).toContain(second.id); + expect(workspaceIds.length).toBeGreaterThanOrEqual(2); + + // The shortcut opens a controlled menu. Wait for Radix's focus handoff + // before sending navigation keys, otherwise the first ArrowDown can land + // on the document body on a busy CI runner. + await expect(testPage.getByRole("menu")).toBeFocused(); + + // Focus lands on the menu itself, so the first ArrowDown moves to item 1 + // and the second to item 2, with no mouse needed at any point. + await testPage.keyboard.press("ArrowDown"); + await expect(testPage.getByTestId(`sidebar-workspace-item-${workspaceIds[0]}`)).toBeFocused(); + await testPage.keyboard.press("ArrowDown"); + await expect(testPage.getByTestId(`sidebar-workspace-item-${workspaceIds[1]}`)).toBeFocused(); + + await testPage.keyboard.press("Enter"); + + await expect(testPage).toHaveURL( + (url) => url.searchParams.get("workspaceId") === workspaceIds[1], + { timeout: 10_000 }, + ); + await expect(testPage.locator(WORKSPACE_ITEM_SELECTOR).first()).not.toBeVisible(); }); - - await pressWorkspacePickerShortcut(testPage); - - await expect(testPage.getByTestId("sidebar-workspace-trigger")).toHaveAttribute( - "data-state", - "open", - ); - const workspaceIds = await listedWorkspaceIds(testPage); - expect(workspaceIds).toContain(second.id); - expect(workspaceIds.length).toBeGreaterThanOrEqual(2); - - // The shortcut opens a controlled menu. Wait for Radix's focus handoff - // before sending navigation keys, otherwise the first ArrowDown can land - // on the document body on a busy CI runner. - await expect(testPage.getByRole("menu")).toBeFocused(); - - // Focus lands on the menu itself, so the first ArrowDown moves to item 1 - // and the second to item 2 — no mouse needed at any point. - await testPage.keyboard.press("ArrowDown"); - await expect(testPage.getByTestId(`sidebar-workspace-item-${workspaceIds[0]}`)).toBeFocused(); - await testPage.keyboard.press("ArrowDown"); - await expect(testPage.getByTestId(`sidebar-workspace-item-${workspaceIds[1]}`)).toBeFocused(); - - await testPage.keyboard.press("Enter"); - - await expect(testPage).toHaveURL( - (url) => url.searchParams.get("workspaceId") === workspaceIds[1], - { timeout: 10_000 }, - ); - await expect(testPage.locator(WORKSPACE_ITEM_SELECTOR).first()).not.toBeVisible(); }); test("expands a collapsed sidebar before opening the picker", async ({ testPage, apiClient }) => { - await apiClient.createWorkspace("Collapsed Picker Workspace"); + await withDisposableWorkspace(apiClient, "Collapsed Picker Workspace", async () => { + await testPage.goto("/"); + const sidebar = testPage.getByTestId("app-sidebar"); + await expect(sidebar).toBeVisible({ timeout: 10_000 }); - await testPage.goto("/"); - const sidebar = testPage.getByTestId("app-sidebar"); - await expect(sidebar).toBeVisible({ timeout: 10_000 }); + await testPage.getByRole("button", { name: "Collapse sidebar" }).click(); + await expect(sidebar).toHaveAttribute("data-collapsed", "true"); - await testPage.getByRole("button", { name: "Collapse sidebar" }).click(); - await expect(sidebar).toHaveAttribute("data-collapsed", "true"); + await pressWorkspacePickerShortcut(testPage); - await pressWorkspacePickerShortcut(testPage); - - await expect(sidebar).toHaveAttribute("data-collapsed", "false"); - await expect(testPage.getByTestId("sidebar-workspace-trigger")).toHaveAttribute( - "data-state", - "open", - ); - await expect(testPage.locator(WORKSPACE_ITEM_SELECTOR).first()).toBeVisible(); + await expect(sidebar).toHaveAttribute("data-collapsed", "false"); + await expect(testPage.getByTestId("sidebar-workspace-trigger")).toHaveAttribute( + "data-state", + "open", + ); + await expect(testPage.locator(WORKSPACE_ITEM_SELECTOR).first()).toBeVisible(); + }); }); test("Escape closes the picker and leaves the workspace unchanged", async ({ testPage, apiClient, }) => { - await apiClient.createWorkspace("Escape Picker Workspace"); + await withDisposableWorkspace(apiClient, "Escape Picker Workspace", async () => { + await testPage.goto("/"); + await expect(testPage.getByTestId("sidebar-workspace-trigger")).toBeVisible({ + timeout: 10_000, + }); + const urlBefore = testPage.url(); - await testPage.goto("/"); - await expect(testPage.getByTestId("sidebar-workspace-trigger")).toBeVisible({ - timeout: 10_000, + await pressWorkspacePickerShortcut(testPage); + await expect(testPage.locator(WORKSPACE_ITEM_SELECTOR).first()).toBeVisible(); + + await testPage.keyboard.press("Escape"); + + await expect(testPage.locator(WORKSPACE_ITEM_SELECTOR).first()).not.toBeVisible(); + expect(testPage.url()).toBe(urlBefore); + + // The store flag must have cleared on dismiss, or a second press would be + // a no-op (open -> open). + await pressWorkspacePickerShortcut(testPage); + await expect(testPage.locator(WORKSPACE_ITEM_SELECTOR).first()).toBeVisible(); }); - const urlBefore = testPage.url(); - - await pressWorkspacePickerShortcut(testPage); - await expect(testPage.locator(WORKSPACE_ITEM_SELECTOR).first()).toBeVisible(); - - await testPage.keyboard.press("Escape"); - - await expect(testPage.locator(WORKSPACE_ITEM_SELECTOR).first()).not.toBeVisible(); - expect(testPage.url()).toBe(urlBefore); - - // The store flag must have cleared on dismiss, or a second press would be - // a no-op (open -> open). - await pressWorkspacePickerShortcut(testPage); - await expect(testPage.locator(WORKSPACE_ITEM_SELECTOR).first()).toBeVisible(); }); test("does nothing below the sidebar's md boundary", async ({ testPage, apiClient }) => { - await apiClient.createWorkspace("Narrow Viewport Workspace"); + await withDisposableWorkspace(apiClient, "Narrow Viewport Workspace", async () => { + // 700px sits in the 640-767px band: the sidebar (and picker trigger) is + // display:none, but the menu portals to document.body — without the + // viewport guard it opened detached at the corner with a scroll lock. + await testPage.setViewportSize({ width: 700, height: 800 }); + await testPage.goto("/"); + await testPage.waitForLoadState("networkidle"); - // 700px sits in the 640-767px band: the sidebar (and picker trigger) is - // display:none, but the menu portals to document.body — without the - // viewport guard it opened detached at the corner with a scroll lock. - await testPage.setViewportSize({ width: 700, height: 800 }); - await testPage.goto("/"); - await testPage.waitForLoadState("networkidle"); + await pressWorkspacePickerShortcut(testPage); - await pressWorkspacePickerShortcut(testPage); - - await expect(testPage.locator(WORKSPACE_ITEM_SELECTOR).first()).not.toBeVisible(); - await expect(testPage.locator("body")).not.toHaveAttribute("data-scroll-locked", /.+/); + await expect(testPage.locator(WORKSPACE_ITEM_SELECTOR).first()).not.toBeVisible(); + await expect(testPage.locator("body")).not.toHaveAttribute("data-scroll-locked", /.+/); + }); }); }); diff --git a/apps/web/e2e/tests/office/workspace-deletion-fallback.spec.ts b/apps/web/e2e/tests/office/workspace-deletion-fallback.spec.ts index 713fd6fca2..61473ca4eb 100644 --- a/apps/web/e2e/tests/office/workspace-deletion-fallback.spec.ts +++ b/apps/web/e2e/tests/office/workspace-deletion-fallback.spec.ts @@ -39,12 +39,16 @@ test.describe("Office workspace deletion fallback navigation", () => { await confirmButton.click(); await expect(testPage).toHaveURL( - (url) => url.pathname === "/" && url.searchParams.get("workspaceId") === seedData.workspaceId, + (url) => url.pathname === "/" && Boolean(url.searchParams.get("workspaceId")), { timeout: 10_000 }, ); await expect(testPage).not.toHaveURL(/\/office\/setup/); + const destinationWorkspaceId = new URL(testPage.url()).searchParams.get("workspaceId"); const { workspaces } = await apiClient.listWorkspaces(); expect(workspaces.some((item) => item.id === onboarded.workspaceId)).toBe(false); + const destinationWorkspace = workspaces.find((item) => item.id === destinationWorkspaceId); + expect(destinationWorkspace).toBeDefined(); + expect(destinationWorkspace?.office_workflow_id).toBeFalsy(); }); }); diff --git a/apps/web/e2e/tests/review/submodule-review-helpers.ts b/apps/web/e2e/tests/review/submodule-review-helpers.ts index 7442823912..0a78e7dcc6 100644 --- a/apps/web/e2e/tests/review/submodule-review-helpers.ts +++ b/apps/web/e2e/tests/review/submodule-review-helpers.ts @@ -80,69 +80,73 @@ export async function createSubmoduleReviewFixture( const outerPath = path.join(sourceRoot, "outer"); const innerPath = path.join(sourceRoot, "inner"); const env = { ...makeGitEnv(tempRoot), GIT_ALLOW_PROTOCOL: "file" }; + const cleanup = () => fs.rmSync(sourceRoot, { recursive: true, force: true }); - initializeRepository(innerPath, env, "README.md", "inner base\n"); + try { + initializeRepository(innerPath, env, "README.md", "inner base\n"); - initializeRepository(outerPath, env, "README.md", "outer base\n"); - runGit(outerPath, [...GIT_PROTOCOL_ARGS, "submodule", "add", "../inner", "vendor/inner"], env); - commit(outerPath, env, "add nested inner submodule"); + initializeRepository(outerPath, env, "README.md", "outer base\n"); + runGit(outerPath, [...GIT_PROTOCOL_ARGS, "submodule", "add", "../inner", "vendor/inner"], env); + commit(outerPath, env, "add nested inner submodule"); - initializeRepository(parentPath, env, "README.md", "parent base\n"); - runGit(parentPath, [...GIT_PROTOCOL_ARGS, "submodule", "add", "../outer", "vendor/outer"], env); - runGit(parentPath, [...GIT_PROTOCOL_ARGS, "submodule", "update", "--init", "--recursive"], env); - runGit( - path.join(parentPath, "vendor/outer"), - [...GIT_PROTOCOL_ARGS, "submodule", "update", "--init", "--recursive"], - env, - ); - commit(parentPath, env, "add outer submodule"); + initializeRepository(parentPath, env, "README.md", "parent base\n"); + runGit(parentPath, [...GIT_PROTOCOL_ARGS, "submodule", "add", "../outer", "vendor/outer"], env); + runGit(parentPath, [...GIT_PROTOCOL_ARGS, "submodule", "update", "--init", "--recursive"], env); + runGit( + path.join(parentPath, "vendor/outer"), + [...GIT_PROTOCOL_ARGS, "submodule", "update", "--init", "--recursive"], + env, + ); + commit(parentPath, env, "add outer submodule"); - const repository = await apiClient.createRepository(seedData.workspaceId, parentPath, "main", { - name: "nested-submodule-parent", - }); - const { executors } = await apiClient.listExecutors(); - const directProfile = executors.find( - (executor) => executor.type === "local" || executor.type === "local_pc", - )?.profiles?.[0]; - if (!directProfile) throw new Error("Nested submodule fixture needs a direct local executor"); - const task = await apiClient.createTaskWithAgent( - seedData.workspaceId, - title, - seedData.agentProfileId, - { - description: "/e2e:simple-message", - workflow_id: seedData.workflowId, - workflow_step_id: seedData.startStepId, - repository_ids: [repository.id], - executor_profile_id: directProfile.id, - }, - ); - if (!task.session_id) throw new Error("Nested submodule fixture did not start a session"); + const repository = await apiClient.createRepository(seedData.workspaceId, parentPath, "main", { + name: "nested-submodule-parent", + }); + const { executors } = await apiClient.listExecutors(); + const directProfile = executors.find( + (executor) => executor.type === "local" || executor.type === "local_pc", + )?.profiles?.[0]; + if (!directProfile) throw new Error("Nested submodule fixture needs a direct local executor"); + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + title, + seedData.agentProfileId, + { + description: "/e2e:simple-message", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [repository.id], + executor_profile_id: directProfile.id, + }, + ); + if (!task.session_id) throw new Error("Nested submodule fixture did not start a session"); - return { - taskId: task.id, - sessionId: task.session_id, - sourceRoot, - waitForWorktree: (client) => waitForWorktreePath(client, task.id, task.session_id!), - applyNestedChanges(worktreePath: string) { - const outerWorktree = path.join(worktreePath, "vendor/outer"); - const innerWorktree = path.join(outerWorktree, "vendor/inner"); - if (!fs.existsSync(innerWorktree)) { - runGit( - worktreePath, - [...GIT_PROTOCOL_ARGS, "submodule", "update", "--init", "--recursive"], - env, - ); - } - fs.appendFileSync(path.join(worktreePath, "README.md"), "parent working-tree change\n"); - fs.appendFileSync(path.join(outerWorktree, "README.md"), "outer committed change\n"); - commit(outerWorktree, env, "change outer submodule"); - fs.appendFileSync(path.join(innerWorktree, "README.md"), "inner committed change\n"); - commit(innerWorktree, env, "change inner submodule"); - commit(outerWorktree, env, "record inner submodule change"); - }, - cleanup() { - fs.rmSync(sourceRoot, { recursive: true, force: true }); - }, - }; + return { + taskId: task.id, + sessionId: task.session_id, + sourceRoot, + waitForWorktree: (client) => waitForWorktreePath(client, task.id, task.session_id!), + applyNestedChanges(worktreePath: string) { + const outerWorktree = path.join(worktreePath, "vendor/outer"); + const innerWorktree = path.join(outerWorktree, "vendor/inner"); + if (!fs.existsSync(innerWorktree)) { + runGit( + worktreePath, + [...GIT_PROTOCOL_ARGS, "submodule", "update", "--init", "--recursive"], + env, + ); + } + fs.appendFileSync(path.join(worktreePath, "README.md"), "parent working-tree change\n"); + fs.appendFileSync(path.join(outerWorktree, "README.md"), "outer committed change\n"); + commit(outerWorktree, env, "change outer submodule"); + fs.appendFileSync(path.join(innerWorktree, "README.md"), "inner committed change\n"); + commit(innerWorktree, env, "change inner submodule"); + commit(outerWorktree, env, "record inner submodule change"); + }, + cleanup, + }; + } catch (error) { + cleanup(); + throw error; + } } diff --git a/apps/web/e2e/tests/review/submodule-review.spec.ts b/apps/web/e2e/tests/review/submodule-review.spec.ts index f2055f3b45..f85bdb944d 100644 --- a/apps/web/e2e/tests/review/submodule-review.spec.ts +++ b/apps/web/e2e/tests/review/submodule-review.spec.ts @@ -1,8 +1,29 @@ +import crypto from "node:crypto"; +import fs from "node:fs"; +import path from "node:path"; import { test, expect } from "../../fixtures/test-base"; import { SessionPage } from "../../pages/session-page"; -import path from "node:path"; import { createSubmoduleReviewFixture, readGitValue } from "./submodule-review-helpers"; +test.describe("Submodule review fixture", () => { + test("cleans source repositories when setup fails", async ({ apiClient, seedData, backend }) => { + const tempRoot = fs.mkdtempSync(path.join(backend.tmpDir, "submodule-review-failure-")); + try { + await expect( + createSubmoduleReviewFixture( + apiClient, + { ...seedData, workspaceId: crypto.randomUUID() }, + tempRoot, + "Failing submodule fixture E2E", + ), + ).rejects.toThrow(); + expect(fs.readdirSync(tempRoot)).toEqual([]); + } finally { + fs.rmSync(tempRoot, { recursive: true, force: true }); + } + }); +}); + test.describe("Nested submodule Review", () => { test.describe.configure({ retries: 1, timeout: 180_000 }); diff --git a/docs/plans/submodule-workspace-search/plan.md b/docs/plans/submodule-workspace-search/plan.md new file mode 100644 index 0000000000..1c11d3ad6c --- /dev/null +++ b/docs/plans/submodule-workspace-search/plan.md @@ -0,0 +1,125 @@ +--- +spec: docs/specs/ui/task-workspace-content-search.md +created: 2026-08-11 +status: implemented +--- + +# Implementation Plan: Preserve Root Search With Submodules + +## Overview + +Fix workspace search tracker selection so a real Git root remains eligible when +initialized submodules add named trackers. Prove the behavior first at the +agentctl manager boundary, preserve bare multi-repository task-root behavior, +then exercise Cmd/Ctrl+Shift+K through the existing command palette. + +## Confirmed Root Cause + +`SearchWorkspaceFileResults` and `contentSearchTrackers` include the root in a +mixed tracker graph only when `RepositoryName()` is non-empty. A real workspace +Git root intentionally uses the empty repository identity, so adding the first +submodule changes both searches from root-only to child-only. The correct +distinction is whether the root owns Git metadata, which `RepositoryScopes` +already represents with `gitIndexPath != ""`. + +Retaining the root exposed a second file-enumeration defect: the root's +`git ls-files --cached --others --exclude-standard` output includes mode-160000 +submodule Gitlinks, but every output line was classified as a file. Searching +the submodule directory name could therefore return a directory as a root file. + +--- + +## Backend + +### Search tracker selection + +- Update `apps/backend/internal/agentctl/server/process/workspace_files.go` and + `workspace_content_search.go` to include the unnamed root when it owns a real + Git repository, even when named submodule trackers coexist. +- Keep the current single-repository fallback and named sibling-repository + aggregation unchanged. Do not search a bare task root or change response + paths, repository identities, ordering, limits, or fuzzy scoring. +- Use the same real-Git-root predicate as `Manager.RepositoryScopes`; do not + infer repository ownership from the display/transport name. +- Preserve tracked Git modes while enumerating files and exclude mode-160000 + Gitlinks. Continue including untracked, non-ignored files. + +## Frontend + +No production frontend change is planned. The command palette already renders +the structured `results` returned by `workspace.files.search` without filtering +the unnamed root, and content search already groups the same empty root identity. + +## Tests + +- **What:** File-name search returns matches from a Git root and initialized + submodule in one response. + **File:** `apps/backend/internal/agentctl/server/process/workspace_search_submodule_test.go`. + **How:** Construct real parent/child Git repositories, add the child as an + initialized submodule, refresh tracker file lists, and assert both structured + repository identities and task-root-relative paths. +- **What:** Content search returns matches from the same mixed root/submodule + graph. + **File:** `apps/backend/internal/agentctl/server/process/workspace_search_submodule_test.go`. + **How:** Search matching tracked content through `Manager.SearchWorkspaceContent` + and assert one result from each scope. +- **What:** Bare task roots containing named sibling repositories remain + excluded. + **Files:** Existing `workspace_file_search_test.go` and + `workspace_content_search_test.go` coverage. + **How:** Rerun the existing multi-repository tests beside the new regressions. +- **What:** Searching the exact submodule directory name does not return the + parent Gitlink as a file, while the child tracker's real files remain eligible. + **File:** `apps/backend/internal/agentctl/server/process/workspace_search_submodule_test.go`. + **How:** Search the real initialized fixture for `lib`, reject the root + `vendor/lib` pseudo-file, and retain the existing child-file assertion. + +## E2E Tests + +- **Scenario:** Given a task whose Git root and initialized submodules each + contain `README.md`, when the user opens Files with Cmd/Ctrl+Shift+K and + searches for that name, root and child repository groups all appear. +- **File:** `apps/web/e2e/tests/command-panel.spec.ts`. +- **What to verify:** Reuse `createSubmoduleReviewFixture`, wait for the task + session/worktree, search through the command palette, and assert the empty + root plus direct and nested submodule group identities. Always remove the + disposable source tree in `finally`. +- No mobile-specific E2E is added. This fix changes shared backend result data, + not layout, touch behavior, navigation, scrolling, or viewport composition; + the existing mobile file-search interaction consumes the same API contract. + +## Verification Results + +Task 01 passed its focused gate: 3 process tests and 1 API test. Both new +regressions failed for the expected child-only result before their production +changes, then passed; the final full run of both touched backend packages also +passed. Task 02 passed 1 Chromium test after its RED run showed the original +bug's 2 groups instead of the expected root plus 2 submodules. Review +remediation added and passed setup-failure cleanup coverage for the shared E2E +fixture, then revalidated the command-palette happy path with retries disabled. +PR CI remediation also removed workspace-picker fixture leakage and corrected +two order-dependent E2E expectations. The two affected specs passed together +with one worker, and exact Chromium/mobile shard 4/14 passed 137 tests with 7 +intentional skips and retries disabled. +Independent review then found that retaining the root also exposed its +mode-160000 Gitlink as a pseudo-file. A focused regression failed on +`vendor/lib`, passed after mode-aware enumeration excluded Gitlinks, and the +four-test behavior gate, both touched backend packages, and CI-style +changed-file lint all passed. + +## Implementation Waves And Parallel Candidates + +Default execution is sequential in the primary conversation. No task is marked +parallel-safe because the browser proof depends on the backend behavior. + +Wave 1: + +- [x] [Task 01: Correct mixed-graph search](task-01-correct-mixed-graph-search.md) + +Wave 2: + +- [x] [Task 02: Prove command-palette search](task-02-prove-command-palette-search.md) + +## Open Questions + +None. diff --git a/docs/plans/submodule-workspace-search/task-01-correct-mixed-graph-search.md b/docs/plans/submodule-workspace-search/task-01-correct-mixed-graph-search.md new file mode 100644 index 0000000000..384e4352b8 --- /dev/null +++ b/docs/plans/submodule-workspace-search/task-01-correct-mixed-graph-search.md @@ -0,0 +1,96 @@ +--- +id: "01-correct-mixed-graph-search" +title: "Correct mixed-graph search" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/ui/task-workspace-content-search.md" +--- + +# Task 01: Correct Mixed-Graph Search + +## Acceptance + +- File-name and content search each return root and submodule matches from a + mixed unnamed-root/named-child tracker graph. +- Bare task roots with named sibling repositories remain excluded, with no + duplicate results or search failure. +- Parent repository Gitlinks are excluded from file results; real files owned + by initialized child trackers remain searchable. +- Response identities, paths, ranking, limits, and existing single/multi-repo + behavior remain unchanged. + +## Verification + +```bash +cd apps/backend && go test ./internal/agentctl/server/process ./internal/agentctl/server/api -run '^(TestManagerSearchWorkspaceFileResultsIncludesRootAndSubmodule|TestManagerSearchWorkspaceFileResultsExcludesSubmoduleGitlink|TestManagerSearchWorkspaceContentIncludesRootAndSubmodule|TestManagerSearchWorkspaceContentGroupsResultsByRepository|TestGetFileList_HidesOnlyRootOwnershipMarker|TestHandleFileSearchIncludesEveryTaskRepository)$' -count=1 -v +``` + +## Files likely touched + +- `apps/backend/internal/agentctl/server/process/workspace_files.go` +- `apps/backend/internal/agentctl/server/process/workspace_content_search.go` +- `apps/backend/internal/agentctl/server/process/workspace_search_submodule_test.go` (new) + +## Dependencies + +None. + +## Parallelism + +`sequential` — both search paths share the same tracker-graph invariant and +regression fixture. + +## Inputs + +- Spec bullets and scenarios for mixed root/submodule and bare multi-repo search. +- `Manager.RepositoryScopes` as the existing real-Git-root predicate. +- `manager_submodule_test.go` and `workspace_tracker_test.go` Git fixture helpers. +- Confirmed failing repro: searching a parent file plus submodule file returned + only the submodule path. + +## Output contract + +Report RED and GREEN commands/results, root-selection change, exact files +changed, bare-root preservation, cleanup, blockers/risks, and synchronized +task/plan status. + +## Results + +Implemented mixed-graph root retention for file-name and content search by +using the same real-Git-root predicate as `Manager.RepositoryScopes`. The new +fixture builds real parent/child repositories and an initialized submodule; +existing bare multi-repository tests preserve the exclusion boundary. + +RED: + +- `go test ./internal/agentctl/server/process -run '^TestManagerSearchWorkspaceFileResultsIncludesRootAndSubmodule$' -count=1 -v` + failed as expected: only `vendor/lib|vendor/lib/child-search-target.txt` + was returned. +- `go test ./internal/agentctl/server/process -run '^TestManagerSearchWorkspaceContentIncludesRootAndSubmodule$' -count=1 -v` + failed as expected: only `vendor/lib|child-search-target.txt` was returned. + +GREEN: + +- Each focused regression passed after its corresponding one-line production + change. +- Final task gate: + `go test ./internal/agentctl/server/process ./internal/agentctl/server/api -run '^(TestManagerSearchWorkspaceFileResultsIncludesRootAndSubmodule|TestManagerSearchWorkspaceContentIncludesRootAndSubmodule|TestManagerSearchWorkspaceContentGroupsResultsByRepository|TestHandleFileSearchIncludesEveryTaskRepository)$' -count=1 -v` + passed 3 process tests and 1 API test. +- Final package audit: + `go test ./internal/agentctl/server/process ./internal/agentctl/server/api -count=1` + passed both touched packages. +- Independent-review remediation RED: + `go test ./internal/agentctl/server/process -run '^TestManagerSearchWorkspaceFileResultsExcludesSubmoduleGitlink$' -count=1 -v` + failed because root search returned `{Path:vendor/lib}` as a file. +- Independent-review remediation GREEN: the same focused regression passed + after file enumeration retained tracked modes and excluded mode-160000 + Gitlinks. The four-test behavior gate passed, including tracked root/child, + content-search, and untracked-file coverage. Both touched packages passed + again, and the CI-style changed-file lint reported 0 issues against base + `66eb87ac307db76b8cb3ba5fcfae73ff6d7d3e6c`. + +Production and regression files match **Files likely touched**; the spec and +plan were synchronized with the remediation. Temporary artifacts: none. +Blockers/remaining Task 01 risks: none. diff --git a/docs/plans/submodule-workspace-search/task-02-prove-command-palette-search.md b/docs/plans/submodule-workspace-search/task-02-prove-command-palette-search.md new file mode 100644 index 0000000000..577aca7681 --- /dev/null +++ b/docs/plans/submodule-workspace-search/task-02-prove-command-palette-search.md @@ -0,0 +1,77 @@ +--- +id: "02-prove-command-palette-search" +title: "Prove command-palette search" +status: done +wave: 2 +depends_on: ["01-correct-mixed-graph-search"] +plan: "plan.md" +spec: "../../specs/ui/task-workspace-content-search.md" +--- + +# Task 02: Prove Command-Palette Search + +## Acceptance + +- Cmd/Ctrl+Shift+K shows matching files from the unnamed Git root, direct + submodule, and nested submodule in their existing repository groups. +- The test drives the user-facing palette, reuses the established disposable + submodule fixture, and cleans its source tree even after failure. +- No production UI or mobile composition changes are introduced. + +## Verification + +```bash +cd apps && pnpm install --frozen-lockfile +cd apps/web && pnpm e2e:run tests/command-panel.spec.ts -- --grep 'finds root and submodule files' +cd apps/web && pnpm e2e:run --no-build tests/review/submodule-review.spec.ts -- --grep 'cleans source repositories when setup fails' --retries=0 +``` + +## Files likely touched + +- `apps/web/e2e/tests/command-panel.spec.ts` + +## Dependencies + +Task 01. + +## Parallelism + +`sequential` — browser proof requires the corrected backend aggregation. + +## Inputs + +- Spec scenario for Files search across a Git root and initialized submodules. +- `apps/web/e2e/tests/review/submodule-review-helpers.ts` for disposable Git + graph creation, task startup, worktree readiness, and cleanup. +- Existing `openFileSearch` and command-dialog helpers in `command-panel.spec.ts`. +- Mobile-parity exception for shared data-only changes with no rendered, + touch, navigation, scrolling, or breakpoint change. + +## Output contract + +Report the discovered Playwright project/test count, exact E2E command/result, +group assertions, disposable fixture cleanup, changed files, blockers/risks, +and synchronized task/plan status. + +## Results + +- `pnpm install --frozen-lockfile` completed successfully from `apps/`. +- RED: with the original file-search predicate restored in the built backend, + `pnpm e2e:run --no-build tests/command-panel.spec.ts -- --grep 'finds root and submodule files'` + discovered one Chromium test and failed with 2 repository groups instead of + 3; the unnamed root group was missing. +- GREEN: after restoring the fix and rebuilding, + `pnpm e2e:run tests/command-panel.spec.ts -- --grep 'finds root and submodule files'` + passed 1 Chromium test. It asserted the unnamed root, `vendor/outer`, and + `vendor/outer/vendor/inner` groups. +- Review remediation moved setup-failure cleanup into the shared fixture helper + while preserving caller cleanup after successful setup. The focused regression + failed RED with one leaked directory, then passed GREEN; the command-palette + happy path also passed with retries disabled. +- PR CI remediation made workspace-picker workspaces disposable, aligned its + focus assertion with Radix's initial item focus, and made office deletion + accept any remaining kanban workspace. Both specs passed together (5 tests), + then exact Chromium/mobile shard 4/14 passed 137 tests with 7 intentional + skips and retries disabled. +- No production UI or mobile composition changed; both use the same corrected + backend response. diff --git a/docs/specs/ui/task-workspace-content-search.md b/docs/specs/ui/task-workspace-content-search.md index 37c78020fb..8d7c7e3418 100644 --- a/docs/specs/ui/task-workspace-content-search.md +++ b/docs/specs/ui/task-workspace-content-search.md @@ -36,6 +36,11 @@ repositories. paths so same-named files remain distinguishable and open in the correct repository. The palette groups those matches by repository and shows paths relative to each group. +- For both **Files** and **Contents**, a Git workspace root remains searchable + when initialized submodules add named repository scopes. Root matches retain + the empty repository identity while child matches retain their task-root-relative + scope names. A bare task directory containing sibling repositories is not + treated as an additional repository. - Search covers every repository materialized for the active task session. Tracked files and untracked, non-ignored files are eligible; ignored files, directories, and workspace metadata are not. @@ -135,6 +140,19 @@ repositories without parsing path strings. - **GIVEN** a task with two repositories containing matching file names, **WHEN** the user searches in **Files** mode, **THEN** matching paths from both repositories appear with their repository prefixes. +- **GIVEN** a Git workspace root and an initialized submodule both contain + matching file names, **WHEN** the user searches in **Files** mode, **THEN** + matches from the root and submodule appear in separate repository groups. +- **GIVEN** an initialized submodule path matches a file-search query, **WHEN** + the user searches in **Files** mode, **THEN** the parent repository's Gitlink + path is not returned as a file and real files inside the child scope remain + searchable. +- **GIVEN** a Git workspace root and an initialized submodule both contain a + matching text query, **WHEN** the user searches in **Contents** mode, + **THEN** results from both repository scopes appear. +- **GIVEN** a bare task directory contains multiple sibling repositories, + **WHEN** the user searches in **Files** or **Contents** mode, **THEN** only + those repositories contribute results and no duplicate task-root group appears. - **GIVEN** two repositories with the same relative path, **WHEN** the user selects the result from the second repository, **THEN** Kandev opens the second repository's file at the returned line and column.