diff --git a/apps/backend/internal/task/service/repository_discovery.go b/apps/backend/internal/task/service/repository_discovery.go index 85bddf606e..00fcc7acd1 100644 --- a/apps/backend/internal/task/service/repository_discovery.go +++ b/apps/backend/internal/task/service/repository_discovery.go @@ -763,11 +763,38 @@ func normalizeRoots(roots []string) []string { return normalized } -func scanRootForRepos(ctx context.Context, root string, maxDepth int) ([]LocalRepository, error) { +type repositoryDiscoveryScanWarning struct { + path string + err error +} + +type repositoryDiscoveryScanResult struct { + repositories []LocalRepository + warnings []repositoryDiscoveryScanWarning +} + +type repositoryDiscoveryWalkDir func(string, fs.WalkDirFunc) error + +func scanRootForRepos( + ctx context.Context, + root string, + maxDepth int, +) (repositoryDiscoveryScanResult, error) { + return scanRootForReposWithWalk(ctx, root, maxDepth, filepath.WalkDir) +} + +func scanRootForReposWithWalk( + ctx context.Context, + root string, + maxDepth int, + walkDir repositoryDiscoveryWalkDir, +) (repositoryDiscoveryScanResult, error) { if _, err := os.Stat(root); err != nil { - return nil, err + return repositoryDiscoveryScanResult{}, err } repos := make([]LocalRepository, 0) + warnings := make([]repositoryDiscoveryScanWarning, 0) + warnedPaths := make(map[string]struct{}) home, _ := os.UserHomeDir() walker := &repoWalker{ root: root, @@ -777,8 +804,15 @@ func scanRootForRepos(ctx context.Context, root string, maxDepth int) ([]LocalRe homeRoot: home, goos: runtime.GOOS, ctx: ctx, + onAccessDenied: func(path string, err error) { + if _, ok := warnedPaths[path]; ok { + return + } + warnedPaths[path] = struct{}{} + warnings = append(warnings, repositoryDiscoveryScanWarning{path: path, err: err}) + }, } - err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + err := walkDir(root, func(path string, d fs.DirEntry, err error) error { repo, walkErr := walker.visit(path, d, err) if walkErr != nil { return walkErr @@ -789,36 +823,43 @@ func scanRootForRepos(ctx context.Context, root string, maxDepth int) ([]LocalRe return nil }) if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { - return nil, err + return repositoryDiscoveryScanResult{}, err } if err != nil { - return nil, err + return repositoryDiscoveryScanResult{}, err } - return repos, nil + return repositoryDiscoveryScanResult{repositories: repos, warnings: warnings}, nil } // repoWalker holds state for the WalkDir callback used in scanRootForRepos. type repoWalker struct { - root string - maxDepth int - libraryRoot string - cacheRoot string - homeRoot string - goos string - ctx context.Context + root string + maxDepth int + libraryRoot string + cacheRoot string + homeRoot string + goos string + ctx context.Context + onAccessDenied func(path string, err error) } // visit is the WalkDir callback. Returns a non-nil *LocalRepository when a git repo is found. func (w *repoWalker) visit(path string, d fs.DirEntry, err error) (*LocalRepository, error) { + if contextErr := w.ctx.Err(); contextErr != nil { + return nil, contextErr + } if err != nil { - if path == w.root || fsdiagnostics.IsAccessDenied(err) { + if path == w.root { return nil, err } + if fsdiagnostics.IsAccessDenied(err) { + if w.onAccessDenied != nil { + w.onAccessDenied(path, err) + } + return nil, nil + } return nil, nil //nolint:nilerr // skip non-permission entries that cannot be accessed } - if w.ctx.Err() != nil { - return nil, w.ctx.Err() - } if path == w.root { return nil, nil } diff --git a/apps/backend/internal/task/service/repository_discovery_recovery_test.go b/apps/backend/internal/task/service/repository_discovery_recovery_test.go new file mode 100644 index 0000000000..e42d6fec0b --- /dev/null +++ b/apps/backend/internal/task/service/repository_discovery_recovery_test.go @@ -0,0 +1,436 @@ +package service + +import ( + "context" + "errors" + "io/fs" + "os" + "path/filepath" + "testing" + + "github.com/kandev/kandev/internal/common/logger" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" +) + +func TestDiscoveryRecoveryMixedRootsReplaceOnlySuccessfulRoot(t *testing.T) { + rootA := filepath.Join(t.TempDir(), "root-a") + rootB := filepath.Join(t.TempDir(), "root-b") + svc := newDiscoveryService(t, rootA) + svc.discoveryConfig.Roots = []string{rootA, rootB} + + oldA := LocalRepository{Path: filepath.Join(rootA, "old-a"), Name: "old-a"} + oldB := LocalRepository{Path: filepath.Join(rootB, "old-b"), Name: "old-b"} + newA := LocalRepository{Path: filepath.Join(rootA, "new-a"), Name: "new-a"} + newB := LocalRepository{Path: filepath.Join(rootB, "new-b"), Name: "new-b"} + scanNumber := 0 + svc.discoveryScanRoot = func(_ context.Context, root string, _ int) (repositoryDiscoveryScanResult, error) { + switch scanNumber { + case 0: + if root == rootA { + return repositoryDiscoveryScanResult{repositories: []LocalRepository{oldA}}, nil + } + return repositoryDiscoveryScanResult{repositories: []LocalRepository{oldB}}, nil + case 1: + if root == rootA { + return repositoryDiscoveryScanResult{repositories: []LocalRepository{newA}}, nil + } + return repositoryDiscoveryScanResult{}, errors.Join( + os.ErrNotExist, + errors.New("clone root is missing"), + ) + default: + if root == rootA { + return repositoryDiscoveryScanResult{}, nil + } + return repositoryDiscoveryScanResult{repositories: []LocalRepository{newB}}, nil + } + } + + first, err := svc.RefreshLocalRepositoryDiscovery(context.Background(), "") + if err != nil { + t.Fatalf("initial discovery refresh: %v", err) + } + assertRepositoryPaths(t, first.Repositories, oldA.Path, oldB.Path) + scanTime := first.ScanTime + + scanNumber++ + second, err := svc.RefreshLocalRepositoryDiscovery(context.Background(), "") + if err != nil { + t.Fatalf("partial discovery refresh: %v", err) + } + assertRepositoryPaths(t, second.Repositories, newA.Path, oldB.Path) + if !second.Cached { + t.Fatal("partial failure should retain the warm cache marker") + } + if second.ScanTime == nil || scanTime == nil || !second.ScanTime.Equal(*scanTime) { + t.Fatalf("scan time = %v, want previous scan time %v", second.ScanTime, scanTime) + } + if len(second.FailedRoots) != 1 || second.FailedRoots[0] != rootB { + t.Fatalf("partial failed roots = %v, want [%q]", second.FailedRoots, rootB) + } + + scanNumber++ + third, err := svc.RefreshLocalRepositoryDiscovery(context.Background(), "") + if err != nil { + t.Fatalf("recovered discovery refresh: %v", err) + } + assertRepositoryPaths(t, third.Repositories, newB.Path) + if len(third.FailedRoots) != 0 { + t.Fatalf("recovered failed roots = %v, want none", third.FailedRoots) + } +} + +// @covers AC-WORKSPACES-LOCAL-REPOSITORIES-003.10, AC-WORKSPACES-LOCAL-REPOSITORIES-003.11 +func TestDiscoveryRecoveryRootSetChangesRetainUnchangedRootSnapshots(t *testing.T) { + t.Run("add root", func(t *testing.T) { + rootA := t.TempDir() + rootB := t.TempDir() + svc := newDiscoveryService(t, rootA) + svc.discoveryConfig = RepositoryDiscoveryConfig{DesktopRuntime: true, MaxDepth: 6} + oldA := LocalRepository{Path: filepath.Join(rootA, "old-a"), Name: "old-a"} + newB := LocalRepository{Path: filepath.Join(rootB, "new-b"), Name: "new-b"} + failA := false + svc.discoveryScanRoot = func(_ context.Context, root string, _ int) (repositoryDiscoveryScanResult, error) { + if root == rootA { + if failA { + return repositoryDiscoveryScanResult{}, os.ErrPermission + } + return repositoryDiscoveryScanResult{repositories: []LocalRepository{oldA}}, nil + } + return repositoryDiscoveryScanResult{repositories: []LocalRepository{newB}}, nil + } + + if _, err := svc.AddDesktopDiscoveryRoot(context.Background(), rootA); err != nil { + t.Fatalf("add initial root: %v", err) + } + failA = true + if _, err := svc.AddDesktopDiscoveryRoot(context.Background(), rootB); err != nil { + t.Fatalf("add second root: %v", err) + } + + snapshot, err := svc.GetLocalRepositoryDiscovery(context.Background(), "") + if err != nil { + t.Fatalf("get discovery snapshot: %v", err) + } + assertRepositoryPaths(t, snapshot.Repositories, oldA.Path, newB.Path) + if len(snapshot.FailedRoots) != 1 || snapshot.FailedRoots[0] != rootA { + t.Fatalf("failed roots = %v, want [%q]", snapshot.FailedRoots, rootA) + } + }) + + t.Run("reconnect root", func(t *testing.T) { + rootA := t.TempDir() + rootB := t.TempDir() + newA := t.TempDir() + svc := newDiscoveryService(t, rootA) + svc.discoveryConfig = RepositoryDiscoveryConfig{DesktopRuntime: true, MaxDepth: 6} + oldA := LocalRepository{Path: filepath.Join(rootA, "old-a"), Name: "old-a"} + oldB := LocalRepository{Path: filepath.Join(rootB, "old-b"), Name: "old-b"} + newARepo := LocalRepository{Path: filepath.Join(newA, "new-a"), Name: "new-a"} + failB := false + svc.discoveryScanRoot = func(_ context.Context, root string, _ int) (repositoryDiscoveryScanResult, error) { + if root == rootB { + if failB { + return repositoryDiscoveryScanResult{}, os.ErrPermission + } + return repositoryDiscoveryScanResult{repositories: []LocalRepository{oldB}}, nil + } + if root == newA { + return repositoryDiscoveryScanResult{repositories: []LocalRepository{newARepo}}, nil + } + return repositoryDiscoveryScanResult{repositories: []LocalRepository{oldA}}, nil + } + + if _, err := svc.AddDesktopDiscoveryRoot(context.Background(), rootA); err != nil { + t.Fatalf("add initial root: %v", err) + } + if _, err := svc.AddDesktopDiscoveryRoot(context.Background(), rootB); err != nil { + t.Fatalf("add second root: %v", err) + } + failB = true + if _, err := svc.ReconnectDesktopDiscoveryRoot(context.Background(), rootA, newA); err != nil { + t.Fatalf("reconnect root: %v", err) + } + + snapshot, err := svc.GetLocalRepositoryDiscovery(context.Background(), "") + if err != nil { + t.Fatalf("get discovery snapshot: %v", err) + } + assertRepositoryPaths(t, snapshot.Repositories, oldB.Path, newARepo.Path) + if len(snapshot.FailedRoots) != 1 || snapshot.FailedRoots[0] != rootB { + t.Fatalf("failed roots = %v, want [%q]", snapshot.FailedRoots, rootB) + } + }) +} + +func TestDiscoveryRecoveryAllFailedRootsRetainWarmSnapshot(t *testing.T) { + rootA := filepath.Join(t.TempDir(), "root-a") + rootB := filepath.Join(t.TempDir(), "root-b") + svc := newDiscoveryService(t, rootA) + svc.discoveryConfig.Roots = []string{rootA, rootB} + oldA := LocalRepository{Path: filepath.Join(rootA, "old-a"), Name: "old-a"} + oldB := LocalRepository{Path: filepath.Join(rootB, "old-b"), Name: "old-b"} + fail := false + svc.discoveryScanRoot = func(_ context.Context, root string, _ int) (repositoryDiscoveryScanResult, error) { + if fail { + return repositoryDiscoveryScanResult{}, os.ErrPermission + } + if root == rootA { + return repositoryDiscoveryScanResult{repositories: []LocalRepository{oldA}}, nil + } + return repositoryDiscoveryScanResult{repositories: []LocalRepository{oldB}}, nil + } + + first, err := svc.RefreshLocalRepositoryDiscovery(context.Background(), "") + if err != nil { + t.Fatalf("initial discovery refresh: %v", err) + } + scanTime := first.ScanTime + fail = true + second, err := svc.RefreshLocalRepositoryDiscovery(context.Background(), "") + if err != nil { + t.Fatalf("all-root failure refresh: %v", err) + } + assertRepositoryPaths(t, second.Repositories, oldA.Path, oldB.Path) + if !second.Cached { + t.Fatal("all-root failure should retain the warm cache marker") + } + if len(second.FailedRoots) != 2 || second.FailedRoots[0] != rootA || second.FailedRoots[1] != rootB { + t.Fatalf("failed roots = %v, want [%q %q]", second.FailedRoots, rootA, rootB) + } + if second.ScanTime == nil || scanTime == nil || !second.ScanTime.Equal(*scanTime) { + t.Fatalf("scan time = %v, want previous scan time %v", second.ScanTime, scanTime) + } +} + +func TestDiscoveryRecoveryOverlappingRootsDeduplicateByPath(t *testing.T) { + rootA := filepath.Join(t.TempDir(), "root-a") + rootB := filepath.Join(t.TempDir(), "root-b") + svc := newDiscoveryService(t, rootA) + svc.discoveryConfig.Roots = []string{rootA, rootB} + sharedPath := filepath.Join(t.TempDir(), "shared") + svc.discoveryScanRoot = func(_ context.Context, _ string, _ int) (repositoryDiscoveryScanResult, error) { + return repositoryDiscoveryScanResult{ + repositories: []LocalRepository{{Path: sharedPath, Name: "shared"}}, + }, nil + } + + result, err := svc.RefreshLocalRepositoryDiscovery(context.Background(), "") + if err != nil { + t.Fatalf("overlapping discovery refresh: %v", err) + } + assertRepositoryPaths(t, result.Repositories, sharedPath) +} + +func TestDiscoveryRecoveryReturnedRepositoriesDoNotMutateCache(t *testing.T) { + root := t.TempDir() + repositoryPath := filepath.Join(root, "project") + svc := newDiscoveryService(t, root) + svc.discoveryConfig.Roots = []string{root} + svc.discoveryScanRoot = func(_ context.Context, _ string, _ int) (repositoryDiscoveryScanResult, error) { + return repositoryDiscoveryScanResult{ + repositories: []LocalRepository{{Path: repositoryPath, Name: "project"}}, + }, nil + } + + result, err := svc.RefreshLocalRepositoryDiscovery(context.Background(), "") + if err != nil { + t.Fatalf("discovery refresh: %v", err) + } + result.Repositories[0].Name = "mutated" + + snapshot, err := svc.GetLocalRepositoryDiscovery(context.Background(), "") + if err != nil { + t.Fatalf("get discovery snapshot: %v", err) + } + if len(snapshot.Repositories) != 1 || snapshot.Repositories[0].Name != "project" { + t.Fatalf("cached repositories = %+v, want original repository", snapshot.Repositories) + } +} + +func TestScanRootForReposRootAccessFailureRemainsFatal(t *testing.T) { + missingRoot := filepath.Join(t.TempDir(), "missing") + _, err := scanRootForRepos(context.Background(), missingRoot, 6) + if !errors.Is(err, os.ErrNotExist) { + t.Fatalf("missing root error = %v, want not-exist", err) + } +} + +// @covers AC-WORKSPACES-LOCAL-REPOSITORIES-003.9, AC-WORKSPACES-LOCAL-REPOSITORIES-004.2 +func TestDiscoveryRecoveryDescendantPermissionKeepsSiblingsAndLogsTarget(t *testing.T) { + root := t.TempDir() + before := filepath.Join(root, "before") + private := filepath.Join(root, "Pictures", "Photo Booth Library") + after := filepath.Join(root, "after") + core, logs := observer.New(zapcore.WarnLevel) + observedLogger, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("create observer logger: %v", err) + } + + svc := newDiscoveryService(t, root) + svc.logger = observedLogger + svc.discoveryScanRoot = func(ctx context.Context, scanRoot string, maxDepth int) (repositoryDiscoveryScanResult, error) { + return scanRootForReposWithWalk( + ctx, + scanRoot, + maxDepth, + permissionDiscoveryWalk(before, private, after), + ) + } + + result, err := svc.refreshLocalRepositoryDiscovery( + context.Background(), "", "", discoveryTriggerStaleRefresh, + ) + if err != nil { + t.Fatalf("refresh discovery: %v", err) + } + assertRepositoryPaths(t, result.Repositories, after, before) + if len(result.FailedRoots) != 0 { + t.Fatalf("failed roots = %v, want none", result.FailedRoots) + } + + entries := logs.FilterMessage("filesystem.access_denied").All() + if len(entries) != 1 { + t.Fatalf("access-denied warnings = %d, want 1", len(entries)) + } + fields := entries[0].ContextMap() + if fields["target"] != private { + t.Fatalf("warning target = %v, want %q", fields["target"], private) + } + if fields["operation"] != "repository.discovery.scan" { + t.Fatalf("warning operation = %v", fields["operation"]) + } + if fields["trigger"] != discoveryTriggerStaleRefresh { + t.Fatalf("warning trigger = %v", fields["trigger"]) + } + if fields["runtime"] != "server" { + t.Fatalf("warning runtime = %v", fields["runtime"]) + } +} + +// @covers AC-WORKSPACES-LOCAL-REPOSITORIES-003.9 +func TestDiscoveryRecoveryDescendantPermissionWarnsOncePerPath(t *testing.T) { + root := t.TempDir() + private := filepath.Join(root, "private") + result, err := scanRootForReposWithWalk( + context.Background(), + root, + 6, + func(root string, visit fs.WalkDirFunc) error { + for _, item := range []struct { + path string + err error + }{ + {path: root}, + {path: private, err: errors.Join(os.ErrPermission, errors.New("first denial"))}, + {path: private, err: errors.Join(os.ErrPermission, errors.New("second denial"))}, + } { + if err := visit(item.path, nil, item.err); err != nil { + return err + } + } + return nil + }, + ) + if err != nil { + t.Fatalf("scan root: %v", err) + } + if len(result.warnings) != 1 || result.warnings[0].path != private { + t.Fatalf("scan warnings = %+v, want one warning for %q", result.warnings, private) + } +} + +// @covers AC-WORKSPACES-LOCAL-REPOSITORIES-003.7 +func TestDiscoveryRecoveryCancellationDoesNotPublishPartialSnapshot(t *testing.T) { + rootA := filepath.Join(t.TempDir(), "root-a") + rootB := filepath.Join(t.TempDir(), "root-b") + svc := newDiscoveryService(t, rootA) + svc.discoveryConfig.Roots = []string{rootA, rootB} + partial := LocalRepository{Path: filepath.Join(rootA, "partial"), Name: "partial"} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + svc.discoveryScanRoot = func(ctx context.Context, root string, _ int) (repositoryDiscoveryScanResult, error) { + if root == rootA { + return repositoryDiscoveryScanResult{repositories: []LocalRepository{partial}}, nil + } + cancel() + return repositoryDiscoveryScanResult{}, ctx.Err() + } + + if _, err := svc.RefreshLocalRepositoryDiscovery(ctx, ""); !errors.Is(err, context.Canceled) { + t.Fatalf("cancelled refresh error = %v, want context canceled", err) + } + + snapshot, err := svc.GetLocalRepositoryDiscovery(context.Background(), "") + if err != nil { + t.Fatalf("get cancelled snapshot: %v", err) + } + if snapshot.Cached || len(snapshot.Repositories) != 0 || len(snapshot.FailedRoots) != 0 { + t.Fatalf("cancelled snapshot = %+v, want no published data", snapshot) + } +} + +type fakeDiscoveryDirEntry struct { + name string + dir bool +} + +func (entry fakeDiscoveryDirEntry) Name() string { return entry.name } +func (entry fakeDiscoveryDirEntry) IsDir() bool { return entry.dir } +func (entry fakeDiscoveryDirEntry) Type() fs.FileMode { return modeForDiscoveryDirEntry(entry.dir) } +func (entry fakeDiscoveryDirEntry) Info() (fs.FileInfo, error) { + return nil, errors.New("not available") +} + +func modeForDiscoveryDirEntry(isDir bool) fs.FileMode { + if isDir { + return fs.ModeDir + } + return 0 +} + +func permissionDiscoveryWalk(before, private, after string) repositoryDiscoveryWalkDir { + return func(root string, visit fs.WalkDirFunc) error { + items := []struct { + path string + entry fs.DirEntry + err error + }{ + {path: root, entry: fakeDiscoveryDirEntry{name: filepath.Base(root), dir: true}}, + {path: before, entry: fakeDiscoveryDirEntry{name: filepath.Base(before), dir: true}}, + {path: filepath.Join(before, ".git"), entry: fakeDiscoveryDirEntry{name: ".git"}}, + {path: private, err: errors.Join(os.ErrPermission, errors.New("photo library denied"))}, + {path: after, entry: fakeDiscoveryDirEntry{name: filepath.Base(after), dir: true}}, + {path: filepath.Join(after, ".git"), entry: fakeDiscoveryDirEntry{name: ".git"}}, + } + for _, item := range items { + if err := visit(item.path, item.entry, item.err); err != nil { + return err + } + } + return nil + } +} + +func assertRepositoryPaths(t *testing.T, repositories []LocalRepository, expected ...string) { + t.Helper() + if len(repositories) != len(expected) { + t.Fatalf("repository paths = %#v, want %#v", repositoryPaths(repositories), expected) + } + for index, path := range expected { + if repositories[index].Path != path { + t.Fatalf("repository paths = %#v, want %#v", repositoryPaths(repositories), expected) + } + } +} + +func repositoryPaths(repositories []LocalRepository) []string { + paths := make([]string, 0, len(repositories)) + for _, repository := range repositories { + paths = append(paths, repository.Path) + } + return paths +} diff --git a/apps/backend/internal/task/service/repository_discovery_state.go b/apps/backend/internal/task/service/repository_discovery_state.go index 1e05c1a2cf..766a4a3ce2 100644 --- a/apps/backend/internal/task/service/repository_discovery_state.go +++ b/apps/backend/internal/task/service/repository_discovery_state.go @@ -42,11 +42,18 @@ func NormalizeRepositoryDiscoveryTrigger(trigger string) string { } type discoveryCacheEntry struct { - roots []string + roots []string + repositoriesByRoot map[string][]LocalRepository + rootStates []models.DesktopDiscoveryRoot + scanTime *time.Time + failedRoots []string +} + +// discoveryRootCacheEntry keeps the last successful result for one exact +// root so a root-set change does not discard healthy choices while another +// root is being scanned. +type discoveryRootCacheEntry struct { repositories []LocalRepository - rootStates []models.DesktopDiscoveryRoot - scanTime *time.Time - failedRoots []string } type discoveryFlight struct { @@ -125,7 +132,7 @@ func (s *Service) getLocalRepositoryDiscovery( } return RepositoryDiscoveryResult{ Roots: roots, - Repositories: cloneRepositories(entry.repositories), + Repositories: repositoriesForRoots(roots, entry.repositoriesByRoot), DesktopRuntime: s.discoveryConfig.DesktopRuntime, RootStates: states, ScanTime: cloneTime(entry.scanTime), @@ -216,6 +223,9 @@ func (s *Service) refreshLocalRepositoryDiscovery( } } previous, hasPrevious := s.discoveryCache[key] + if !hasPrevious { + previous = s.discoveryEntryFromRootCacheLocked(roots) + } flight := &discoveryFlight{done: make(chan struct{})} s.discoveryFlights[key] = flight s.discoveryCacheMu.Unlock() @@ -239,14 +249,14 @@ func (s *Service) scanDiscoveryRoots( trigger string, ) (RepositoryDiscoveryResult, error) { trigger = NormalizeRepositoryDiscoveryTrigger(trigger) - found := make([]LocalRepository, 0) + repositoriesByRoot := cloneRepositoriesByRoot(previous.repositoriesByRoot) failed := make([]string, 0) for _, root := range roots { if err := ctx.Err(); err != nil { return RepositoryDiscoveryResult{}, err } s.logFilesystemInfo("repository discovery scan started", "repository.discovery.scan", root, trigger) - repositories, err := s.discoveryScanRoot(ctx, root, s.discoveryMaxDepth()) + scan, err := s.discoveryScanRoot(ctx, root, s.discoveryMaxDepth()) if err != nil { if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return RepositoryDiscoveryResult{}, err @@ -254,9 +264,16 @@ func (s *Service) scanDiscoveryRoots( failed = append(failed, root) s.logFilesystemFailure("repository.discovery.scan", root, trigger, err) s.recordDesktopRootFailure(ctx, root, err) + if _, ok := repositoriesByRoot[root]; !ok { + repositoriesByRoot[root] = nil + } continue } - found = appendUniqueRepositories(found, repositories) + for _, warning := range scan.warnings { + s.logFilesystemFailure("repository.discovery.scan", warning.path, trigger, warning.err) + } + repositories := scan.repositories + repositoriesByRoot[root] = cloneRepositories(repositories) s.recordDesktopRootSuccess(ctx, root) } @@ -265,19 +282,18 @@ func (s *Service) scanDiscoveryRoots( return RepositoryDiscoveryResult{}, err } if len(failed) > 0 { - repositories := found + repositories := repositoriesForRoots(roots, repositoriesByRoot) scanTime := (*time.Time)(nil) cached := hasPrevious if hasPrevious { - repositories = cloneRepositories(previous.repositories) scanTime = cloneTime(previous.scanTime) } entry := discoveryCacheEntry{ - roots: append([]string(nil), roots...), - repositories: cloneRepositories(repositories), - rootStates: cloneRootStates(states), - scanTime: cloneTime(scanTime), - failedRoots: append([]string(nil), failed...), + roots: roots, + repositoriesByRoot: repositoriesByRoot, + rootStates: states, + scanTime: scanTime, + failedRoots: failed, } s.storeDiscoveryCache(roots, entry) return RepositoryDiscoveryResult{ @@ -294,15 +310,15 @@ func (s *Service) scanDiscoveryRoots( now := s.discoveryClockNow() entry := discoveryCacheEntry{ - roots: append([]string(nil), roots...), - repositories: cloneRepositories(found), - rootStates: cloneRootStates(states), - scanTime: &now, + roots: roots, + repositoriesByRoot: repositoriesByRoot, + rootStates: states, + scanTime: &now, } s.storeDiscoveryCache(roots, entry) return RepositoryDiscoveryResult{ Roots: roots, - Repositories: found, + Repositories: repositoriesForRoots(roots, repositoriesByRoot), DesktopRuntime: s.discoveryConfig.DesktopRuntime, RootStates: states, ScanTime: &now, @@ -311,12 +327,56 @@ func (s *Service) scanDiscoveryRoots( } func (s *Service) storeDiscoveryCache(roots []string, entry discoveryCacheEntry) { - key := discoveryCacheKey(roots, s.discoveryMaxDepth()) + maxDepth := s.discoveryMaxDepth() + key := discoveryCacheKey(roots, maxDepth) + entry.roots = append([]string(nil), entry.roots...) + entry.repositoriesByRoot = cloneRepositoriesByRoot(entry.repositoriesByRoot) + entry.rootStates = cloneRootStates(entry.rootStates) + entry.scanTime = cloneTime(entry.scanTime) + entry.failedRoots = append([]string(nil), entry.failedRoots...) s.discoveryCacheMu.Lock() + if s.discoveryRootCache == nil { + s.discoveryRootCache = make(map[string]discoveryRootCacheEntry) + } s.discoveryCache[key] = entry + failedRoots := make(map[string]struct{}, len(entry.failedRoots)) + for _, root := range entry.failedRoots { + failedRoots[root] = struct{}{} + } + for _, root := range entry.roots { + if _, failed := failedRoots[root]; failed { + continue + } + repositories, ok := entry.repositoriesByRoot[root] + if !ok { + continue + } + s.discoveryRootCache[discoveryRootCacheKey(root, maxDepth)] = discoveryRootCacheEntry{ + repositories: repositories, + } + } s.discoveryCacheMu.Unlock() } +// discoveryEntryFromRootCacheLocked reconstructs only per-root repository +// snapshots. It intentionally does not provide an aggregate scan time or +// warm-cache marker because the current root set has not completed together. +func (s *Service) discoveryEntryFromRootCacheLocked(roots []string) discoveryCacheEntry { + repositoriesByRoot := make(map[string][]LocalRepository) + maxDepth := s.discoveryMaxDepth() + for _, root := range roots { + cached, ok := s.discoveryRootCache[discoveryRootCacheKey(root, maxDepth)] + if !ok { + continue + } + repositoriesByRoot[root] = cloneRepositories(cached.repositories) + } + return discoveryCacheEntry{ + roots: append([]string(nil), roots...), + repositoriesByRoot: repositoriesByRoot, + } +} + func (s *Service) resolveDiscoveryRoots(ctx context.Context, requestedRoot string) ([]string, error) { roots, err := s.effectiveDiscoveryRoots(ctx) if err != nil { @@ -676,10 +736,33 @@ func discoveryCacheKey(roots []string, maxDepth int) string { return fmt.Sprintf("%d:%s", maxDepth, strings.Join(ordered, "\x00")) } +func discoveryRootCacheKey(root string, maxDepth int) string { + return discoveryCacheKey([]string{root}, maxDepth) +} + func cloneRepositories(repositories []LocalRepository) []LocalRepository { return append([]LocalRepository(nil), repositories...) } +func cloneRepositoriesByRoot(repositoriesByRoot map[string][]LocalRepository) map[string][]LocalRepository { + if repositoriesByRoot == nil { + return make(map[string][]LocalRepository) + } + result := make(map[string][]LocalRepository, len(repositoriesByRoot)) + for root, repositories := range repositoriesByRoot { + result[root] = cloneRepositories(repositories) + } + return result +} + +func repositoriesForRoots(roots []string, repositoriesByRoot map[string][]LocalRepository) []LocalRepository { + found := make([]LocalRepository, 0) + for _, root := range roots { + found = appendUniqueRepositories(found, repositoriesByRoot[root]) + } + return found +} + func cloneRootStates(states []models.DesktopDiscoveryRoot) []models.DesktopDiscoveryRoot { result := make([]models.DesktopDiscoveryRoot, 0, len(states)) for i := range states { diff --git a/apps/backend/internal/task/service/repository_discovery_test.go b/apps/backend/internal/task/service/repository_discovery_test.go index 42e1b3c04b..251e979d45 100644 --- a/apps/backend/internal/task/service/repository_discovery_test.go +++ b/apps/backend/internal/task/service/repository_discovery_test.go @@ -94,9 +94,9 @@ func TestDesktopDiscoveryWithoutRootDoesNotScanHome(t *testing.T) { svc := newDiscoveryService(t, home) svc.discoveryConfig = RepositoryDiscoveryConfig{DesktopRuntime: true, MaxDepth: 6} var scanCalls int - svc.discoveryScanRoot = func(context.Context, string, int) ([]LocalRepository, error) { + svc.discoveryScanRoot = func(context.Context, string, int) (repositoryDiscoveryScanResult, error) { scanCalls++ - return nil, nil + return repositoryDiscoveryScanResult{}, nil } result, err := svc.GetLocalRepositoryDiscovery(context.Background(), "") @@ -135,8 +135,8 @@ func TestRepositoryDiscoveryScanUsesValidatedTrigger(t *testing.T) { } svc.logger = observedLogger svc.discoveryConfig = RepositoryDiscoveryConfig{Roots: []string{root}, MaxDepth: 6} - svc.discoveryScanRoot = func(context.Context, string, int) ([]LocalRepository, error) { - return nil, nil + svc.discoveryScanRoot = func(context.Context, string, int) (repositoryDiscoveryScanResult, error) { + return repositoryDiscoveryScanResult{}, nil } if _, err := svc.refreshLocalRepositoryDiscovery( @@ -196,13 +196,15 @@ func TestDesktopDiscoveryRefreshSharesOneScan(t *testing.T) { release := make(chan struct{}) var callsMu sync.Mutex calls := 0 - svc.discoveryScanRoot = func(context.Context, string, int) ([]LocalRepository, error) { + svc.discoveryScanRoot = func(context.Context, string, int) (repositoryDiscoveryScanResult, error) { callsMu.Lock() calls++ callsMu.Unlock() close(started) <-release - return []LocalRepository{{Path: filepath.Join(root, "project"), Name: "project"}}, nil + return repositoryDiscoveryScanResult{ + repositories: []LocalRepository{{Path: filepath.Join(root, "project"), Name: "project"}}, + }, nil } results := make(chan RepositoryDiscoveryResult, 2) @@ -250,7 +252,7 @@ func TestDesktopDiscoveryCancelledFlightDoesNotPoisonWaitingCaller(t *testing.T) release := make(chan struct{}) var callsMu sync.Mutex calls := 0 - svc.discoveryScanRoot = func(ctx context.Context, _ string, _ int) ([]LocalRepository, error) { + svc.discoveryScanRoot = func(ctx context.Context, _ string, _ int) (repositoryDiscoveryScanResult, error) { callsMu.Lock() calls++ callNumber := calls @@ -258,9 +260,11 @@ func TestDesktopDiscoveryCancelledFlightDoesNotPoisonWaitingCaller(t *testing.T) if callNumber == 1 { close(started) <-release - return nil, ctx.Err() + return repositoryDiscoveryScanResult{}, ctx.Err() } - return []LocalRepository{{Path: filepath.Join(root, "project"), Name: "project"}}, nil + return repositoryDiscoveryScanResult{ + repositories: []LocalRepository{{Path: filepath.Join(root, "project"), Name: "project"}}, + }, nil } ownerCtx, cancelOwner := context.WithCancel(context.Background()) @@ -304,7 +308,7 @@ func TestDesktopDiscoveryCancelledFlightDoesNotPoisonWaitingCaller(t *testing.T) } } -func TestRepoWalkerPropagatesAccessDenied(t *testing.T) { +func TestRepoWalkerPropagatesRootAccessDeniedButSkipsChild(t *testing.T) { root := t.TempDir() walker := &repoWalker{root: root, ctx: context.Background()} @@ -312,8 +316,8 @@ func TestRepoWalkerPropagatesAccessDenied(t *testing.T) { t.Fatalf("root access error = %v, want permission denied", err) } child := filepath.Join(root, "private") - if _, err := walker.visit(child, nil, os.ErrPermission); !errors.Is(err, os.ErrPermission) { - t.Fatalf("child access error = %v, want permission denied", err) + if _, err := walker.visit(child, nil, os.ErrPermission); err != nil { + t.Fatalf("child access error = %v, want skipped child", err) } } @@ -356,11 +360,13 @@ func TestDesktopDiscoveryFailurePreservesCachedRepositories(t *testing.T) { svc := newDiscoveryService(t, root) svc.discoveryConfig = RepositoryDiscoveryConfig{DesktopRuntime: true, MaxDepth: 6} var fail bool - svc.discoveryScanRoot = func(context.Context, string, int) ([]LocalRepository, error) { + svc.discoveryScanRoot = func(context.Context, string, int) (repositoryDiscoveryScanResult, error) { if fail { - return nil, os.ErrPermission + return repositoryDiscoveryScanResult{}, os.ErrPermission } - return []LocalRepository{{Path: repositoryPath, Name: "project"}}, nil + return repositoryDiscoveryScanResult{ + repositories: []LocalRepository{{Path: repositoryPath, Name: "project"}}, + }, nil } if _, err := svc.AddDesktopDiscoveryRoot(context.Background(), root); err != nil { diff --git a/apps/backend/internal/task/service/service.go b/apps/backend/internal/task/service/service.go index 5e82cded75..a6a9e8294f 100644 --- a/apps/backend/internal/task/service/service.go +++ b/apps/backend/internal/task/service/service.go @@ -423,9 +423,10 @@ type Service struct { discoveryConfig RepositoryDiscoveryConfig discoveryCacheMu sync.Mutex discoveryCache map[string]discoveryCacheEntry + discoveryRootCache map[string]discoveryRootCacheEntry discoveryFlights map[string]*discoveryFlight discoveryNow func() time.Time - discoveryScanRoot func(context.Context, string, int) ([]LocalRepository, error) + discoveryScanRoot func(context.Context, string, int) (repositoryDiscoveryScanResult, error) filesystemWarnings *fsdiagnostics.WarningLimiter worktreeCleanup WorktreeCleanup canvasCleanup CanvasCleanup @@ -699,6 +700,7 @@ func NewService(repos Repos, eventBus bus.EventBus, log *logger.Logger, discover logger: log, discoveryConfig: discoveryConfig, discoveryCache: make(map[string]discoveryCacheEntry), + discoveryRootCache: make(map[string]discoveryRootCacheEntry), discoveryFlights: make(map[string]*discoveryFlight), discoveryNow: time.Now, discoveryScanRoot: scanRootForRepos, diff --git a/apps/web/app/settings/workspace/workspace-repositories-dialog.tsx b/apps/web/app/settings/workspace/workspace-repositories-dialog.tsx index cdfc565163..47fd2fd0e2 100644 --- a/apps/web/app/settings/workspace/workspace-repositories-dialog.tsx +++ b/apps/web/app/settings/workspace/workspace-repositories-dialog.tsx @@ -170,8 +170,11 @@ export function DiscoverRepoDialog({ variant="outline" className="[@media(pointer:coarse)]:h-11" onClick={onRefreshDiscovery} + disabled={isLoading} > - {t("workspaces:refreshRepositories")} + {isLoading + ? t("workspaces:scanningRepositories") + : t("workspaces:refreshRepositories")} )} diff --git a/apps/web/components/repository-discovery-controls.test.tsx b/apps/web/components/repository-discovery-controls.test.tsx index 3432dbb6e4..227ee730a9 100644 --- a/apps/web/components/repository-discovery-controls.test.tsx +++ b/apps/web/components/repository-discovery-controls.test.tsx @@ -10,6 +10,7 @@ const discovery = vi.hoisted(() => ({ { id: "", path: "/configured", display_path: "/configured", state: "connected" }, ], homeConfirmationRequired: true, + failedRoots: [] as string[], })); const actions = vi.hoisted(() => ({ refreshDiscovery: vi.fn(), @@ -49,6 +50,8 @@ import { RepositoryDiscoveryControls } from "./repository-discovery-controls"; afterEach(() => { cleanup(); vi.clearAllMocks(); + discovery.desktopRuntime = true; + discovery.failedRoots = []; }); describe("RepositoryDiscoveryControls", () => { @@ -66,4 +69,13 @@ describe("RepositoryDiscoveryControls", () => { expect(screen.queryByTestId("root-controls")).toBeNull(); }); + + it("does not render failed-root diagnostics for a non-desktop picker", () => { + discovery.desktopRuntime = false; + discovery.failedRoots = ["/missing-repositories"]; + + render(); + + expect(screen.queryByTestId("root-controls")).toBeNull(); + }); }); diff --git a/apps/web/components/repository-discovery-controls.tsx b/apps/web/components/repository-discovery-controls.tsx index daf0c7baf9..b9cf187c14 100644 --- a/apps/web/components/repository-discovery-controls.tsx +++ b/apps/web/components/repository-discovery-controls.tsx @@ -15,10 +15,9 @@ type RepositoryDiscoveryControlsProps = { }; /** - * The shared consent and recovery surface for every repository selector. - * Keeping the discovery lease and root actions together prevents a picker - * from showing an empty result without also offering the action that can - * establish or repair its filesystem access. + * Renders desktop discovery-root consent and recovery controls. Browser and + * phone selectors keep their existing repository-list actions while discovery + * diagnostics remain outside the selector UI. */ export function RepositoryDiscoveryControls({ workspaceId, diff --git a/apps/web/components/repository-discovery-root-controls.test.tsx b/apps/web/components/repository-discovery-root-controls.test.tsx new file mode 100644 index 0000000000..4d056d9623 --- /dev/null +++ b/apps/web/components/repository-discovery-root-controls.test.tsx @@ -0,0 +1,63 @@ +import { cleanup, render, screen } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); +vi.mock("@/components/folder-picker", () => ({ + FolderPicker: () => , +})); + +import { RepositoryDiscoveryRootControls } from "./repository-discovery-root-controls"; + +const baseProps = { + isLoading: false, + discoveryRoots: [], + homeConfirmationRequired: false, + onChooseDiscoveryRoot: vi.fn(), + onRefreshDiscovery: vi.fn(), + onReconnectDiscoveryRoot: vi.fn(), + onRemoveDiscoveryRoot: vi.fn(), +}; + +afterEach(() => { + cleanup(); + vi.clearAllMocks(); +}); + +describe("RepositoryDiscoveryRootControls", () => { + it("keeps the folder and refresh actions available", () => { + render(); + + expect(screen.getByRole("button", { name: "Folder picker" })).toBeTruthy(); + expect(screen.getByRole("button", { name: "workspaces:refreshRepositories" })).toBeTruthy(); + }); + + it("leaves saved root recovery controls in place", () => { + render( + , + ); + + expect(screen.getByRole("button", { name: "workspaces:refreshRepositories" })).toBeTruthy(); + expect(screen.getByText("workspaces:removeDiscoveryRoot")).toBeTruthy(); + }); + + it("disables refresh while discovery is refreshing", () => { + render(); + + expect( + (screen.getByRole("button", { name: "workspaces:refreshRepositories" }) as HTMLButtonElement) + .disabled, + ).toBe(true); + }); +}); diff --git a/apps/web/components/repository-discovery-root-controls.tsx b/apps/web/components/repository-discovery-root-controls.tsx index 8307aedb00..4ba8bb257b 100644 --- a/apps/web/components/repository-discovery-root-controls.tsx +++ b/apps/web/components/repository-discovery-root-controls.tsx @@ -18,6 +18,47 @@ export type RepositoryDiscoveryRootControlsProps = { onRemoveDiscoveryRoot: (path: string) => void; }; +function SavedDiscoveryRootList({ + discoveryRoots, + onReconnectDiscoveryRoot, + onRemoveDiscoveryRoot, +}: { + discoveryRoots: DesktopDiscoveryRoot[]; + onReconnectDiscoveryRoot: (oldPath: string, newPath: string) => void; + onRemoveDiscoveryRoot: (path: string) => void; +}) { + const { t } = useTranslation(); + return ( + <> + {discoveryRoots.map((root) => ( +
+ + {root.display_path || root.path} + + {root.state === "reconnect_required" && ( + onReconnectDiscoveryRoot(root.path, newPath)} + /> + )} + +
+ ))} + + ); +} + export function RepositoryDiscoveryRootControls({ className, presentation = "card", @@ -79,31 +120,11 @@ export function RepositoryDiscoveryRootControls({ )} - {discoveryRoots.map((root) => ( -
- - {root.display_path || root.path} - - {root.state === "reconnect_required" && ( - onReconnectDiscoveryRoot(root.path, newPath)} - /> - )} - -
- ))} + ); } diff --git a/apps/web/components/task-create-dialog-pill-command-list.tsx b/apps/web/components/task-create-dialog-pill-command-list.tsx new file mode 100644 index 0000000000..a31967bb8f --- /dev/null +++ b/apps/web/components/task-create-dialog-pill-command-list.tsx @@ -0,0 +1,89 @@ +import { IconCheck } from "@tabler/icons-react"; +import type { PillOption } from "./task-create-dialog-pill"; +import { cn } from "@/lib/utils"; +import { prioritizeSelectedOption, selectorOptionClassName } from "@/lib/utils/selector-options"; +import { CommandEmpty, CommandGroup, CommandItem, CommandList } from "@kandev/ui/command"; + +export function PillCommandList({ + options, + value, + onSelect, + onPointerSelect, + setOpen, + emptyMessage, +}: { + options: PillOption[]; + value: string; + onSelect: (value: string) => void; + onPointerSelect: (pointerType: string) => void; + setOpen: (open: boolean) => void; + emptyMessage: string; +}) { + const groups = new Map(); + for (const option of options) { + const key = option.group ?? ""; + const group = groups.get(key) ?? { label: option.groupLabel, options: [] }; + group.options.push(option); + groups.set(key, group); + } + const groupOrder = new Map([ + ["policies", 0], + ["branches", 1], + ]); + const orderedGroups = Array.from(groups.entries()).sort( + ([firstKey], [secondKey]) => + (groupOrder.get(firstKey) ?? Number.MAX_SAFE_INTEGER) - + (groupOrder.get(secondKey) ?? Number.MAX_SAFE_INTEGER), + ); + + return ( + + {emptyMessage} + {orderedGroups.map(([key, group]) => ( + + {prioritizeSelectedOption(group.options, value, (option) => option.value).map( + (option) => { + const selected = option.value === value; + const item = ( + onPointerSelect(event.pointerType)} + onSelect={() => { + onSelect(option.value); + setOpen(false); + }} + className={cn( + selectorOptionClassName(selected), + option.renderAccessory && "pr-14", + )} + > +
+ {option.renderLabel ? option.renderLabel() : option.label} +
+ + + ); + if (!option.renderAccessory) return item; + return ( +
+ {item} +
+ {option.renderAccessory()} +
+
+ ); + }, + )} + + ))} + + ); +} diff --git a/apps/web/components/task-create-dialog-pill.tsx b/apps/web/components/task-create-dialog-pill.tsx index e854f89a1d..e9c5dd3d02 100644 --- a/apps/web/components/task-create-dialog-pill.tsx +++ b/apps/web/components/task-create-dialog-pill.tsx @@ -1,20 +1,12 @@ "use client"; import { useCallback, useRef, useState } from "react"; -import { IconCheck } from "@tabler/icons-react"; import { cn } from "@/lib/utils"; -import { prioritizeSelectedOption, selectorOptionClassName } from "@/lib/utils/selector-options"; import { Popover, PopoverContent, PopoverTrigger } from "@kandev/ui/popover"; import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; -import { - Command, - CommandEmpty, - CommandGroup, - CommandInput, - CommandItem, - CommandList, -} from "@kandev/ui/command"; +import { Command, CommandInput } from "@kandev/ui/command"; import { BranchRefreshButton } from "@/components/branch-refresh-button"; +import { PillCommandList } from "@/components/task-create-dialog-pill-command-list"; import { controlSizingClassName } from "@kandev/ui/control-sizing"; import { useTaskCreateDialogPopoverContainer } from "@/hooks/use-task-create-dialog-popover-container"; import { usePillTooltipSuppression } from "@/hooks/use-pill-tooltip-suppression"; @@ -93,90 +85,6 @@ function pillActiveClass(flat: boolean): string { return "hover:bg-muted hover:border-border cursor-pointer"; } -function PillCommandList({ - options, - value, - onSelect, - onPointerSelect, - setOpen, - emptyMessage, -}: { - options: PillOption[]; - value: string; - onSelect: (value: string) => void; - onPointerSelect: (pointerType: string) => void; - setOpen: (open: boolean) => void; - emptyMessage: string; -}) { - const groups = new Map(); - for (const option of options) { - const key = option.group ?? ""; - const group = groups.get(key) ?? { label: option.groupLabel, options: [] }; - group.options.push(option); - groups.set(key, group); - } - const groupOrder = new Map([ - ["policies", 0], - ["branches", 1], - ]); - const orderedGroups = Array.from(groups.entries()).sort( - ([firstKey], [secondKey]) => - (groupOrder.get(firstKey) ?? Number.MAX_SAFE_INTEGER) - - (groupOrder.get(secondKey) ?? Number.MAX_SAFE_INTEGER), - ); - - return ( - - {emptyMessage} - {orderedGroups.map(([key, group]) => ( - - {prioritizeSelectedOption(group.options, value, (option) => option.value).map( - (option) => { - const selected = option.value === value; - const item = ( - onPointerSelect(event.pointerType)} - onSelect={() => { - onSelect(option.value); - setOpen(false); - }} - className={cn( - selectorOptionClassName(selected), - option.renderAccessory && "pr-14", - )} - > -
- {option.renderLabel ? option.renderLabel() : option.label} -
- - - ); - if (!option.renderAccessory) return item; - return ( -
- {item} -
- {option.renderAccessory()} -
-
- ); - }, - )} - - ))} - - ); -} - /** * Builds the className for the pill trigger button. Extracted so the inline * trigger JSX stays compact (the Pill function is right at the complexity cap). diff --git a/apps/web/e2e/tests/task/mobile-repository-discovery.spec.ts b/apps/web/e2e/tests/task/mobile-repository-discovery.spec.ts index 7c610e80dc..add725a235 100644 --- a/apps/web/e2e/tests/task/mobile-repository-discovery.spec.ts +++ b/apps/web/e2e/tests/task/mobile-repository-discovery.spec.ts @@ -2,6 +2,10 @@ import { expect, test } from "../../fixtures/test-base"; import { useRegularMode } from "../../helpers/regular-mode"; import { waitForFiniteAnimations } from "../../helpers/animations"; import { MobileKanbanPage } from "../../pages/mobile-kanban-page"; +import { + DISCOVERY_FAILURE_ROOT, + installRepositoryDiscoveryFailureRoute, +} from "./repository-discovery-failure-helpers"; import fs from "node:fs"; useRegularMode(); @@ -47,6 +51,57 @@ test.describe("Mobile repository discovery consent", () => { expect(refreshCssHeight).toBe(44); }); + test("keeps failed-root diagnostics out of the phone selector", async ({ testPage, backend }) => { + test.setTimeout(120_000); + await installRepositoryDiscoveryFailureRoute(testPage); + await backend.restart({ KANDEV_DESKTOP_RUNTIME: "false" }); + + const mobile = new MobileKanbanPage(testPage); + await mobile.goto(); + await mobile.mobileFab.tap(); + + const dialog = testPage.getByTestId("create-task-dialog"); + await expect(dialog).toBeVisible(); + await dialog.getByTestId("repo-chip-trigger").first().tap(); + + await expect(testPage.getByTestId("discovery-root-controls")).toHaveCount(0); + await expect(testPage.getByTestId("discovery-failure")).toHaveCount(0); + await expect(testPage.getByText(DISCOVERY_FAILURE_ROOT, { exact: true })).toHaveCount(0); + const viewport = testPage.viewportSize(); + expect(viewport).not.toBeNull(); + const refresh = testPage.getByTestId("repo-refresh-button"); + const refreshBox = await refresh.boundingBox(); + expect(refreshBox).not.toBeNull(); + expect(refreshBox!.y).toBeGreaterThanOrEqual(0); + expect(refreshBox!.y + refreshBox!.height).toBeLessThanOrEqual(viewport!.height); + const availableRepository = testPage.getByRole("option", { name: /healthy-project/ }); + await expect(availableRepository).toBeEnabled(); + const availableRepositoryBox = await availableRepository.boundingBox(); + expect(availableRepositoryBox).not.toBeNull(); + expect(availableRepositoryBox!.y).toBeGreaterThanOrEqual(0); + expect(availableRepositoryBox!.y + availableRepositoryBox!.height).toBeLessThanOrEqual( + viewport!.height, + ); + await availableRepository.tap(); + await expect(dialog.getByTestId("repo-chip-trigger").first()).toContainText("healthy-project"); + + await dialog.getByTestId("repo-chip-trigger").first().tap(); + const refreshResponse = testPage.waitForResponse( + (response) => + /\/api\/v1\/workspaces\/[^/]+\/repositories$/.test(new URL(response.url()).pathname) && + response.request().method() === "GET" && + response.ok(), + ); + expect(refreshBox!.height).toBeGreaterThanOrEqual(44); + await refresh.tap(); + await refreshResponse; + await expect(testPage.getByTestId("discovery-failure")).toHaveCount(0); + await expect(testPage.getByRole("option", { name: /healthy-project/ })).toBeVisible(); + expect( + await testPage.evaluate(() => document.documentElement.scrollWidth <= window.innerWidth), + ).toBe(true); + }); + test("uses the HTTP picker on a mobile browser connected to a desktop backend", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/task/repository-discovery-consent.spec.ts b/apps/web/e2e/tests/task/repository-discovery-consent.spec.ts index 24b5339f10..d4e5242fd6 100644 --- a/apps/web/e2e/tests/task/repository-discovery-consent.spec.ts +++ b/apps/web/e2e/tests/task/repository-discovery-consent.spec.ts @@ -2,6 +2,10 @@ import { expect, test } from "../../fixtures/test-base"; import { useRegularMode } from "../../helpers/regular-mode"; import { waitForFiniteAnimations } from "../../helpers/animations"; import { KanbanPage } from "../../pages/kanban-page"; +import { + DISCOVERY_FAILURE_ROOT, + installRepositoryDiscoveryFailureRoute, +} from "./repository-discovery-failure-helpers"; import fs from "node:fs"; import path from "node:path"; @@ -41,6 +45,43 @@ test.describe("Desktop repository discovery consent", () => { expect(refreshBox!.height).toBeLessThanOrEqual(32); }); + test("keeps failed-root diagnostics out of the server selector", async ({ + testPage, + backend, + }) => { + test.setTimeout(120_000); + await installRepositoryDiscoveryFailureRoute(testPage); + await backend.restart({ KANDEV_DESKTOP_RUNTIME: "false" }); + + const kanban = new KanbanPage(testPage); + await kanban.goto(); + await kanban.createTaskButton.first().click(); + + const dialog = testPage.getByTestId("create-task-dialog"); + await expect(dialog).toBeVisible(); + await dialog.getByTestId("repo-chip-trigger").first().click(); + + await expect(testPage.getByTestId("discovery-root-controls")).toHaveCount(0); + await expect(testPage.getByTestId("discovery-failure")).toHaveCount(0); + await expect(testPage.getByText(DISCOVERY_FAILURE_ROOT, { exact: true })).toHaveCount(0); + const availableRepository = testPage.getByRole("option", { name: /healthy-project/ }); + await expect(availableRepository).toBeEnabled(); + await availableRepository.click(); + await expect(dialog.getByTestId("repo-chip-trigger").first()).toContainText("healthy-project"); + + await dialog.getByTestId("repo-chip-trigger").first().click(); + const refreshResponse = testPage.waitForResponse( + (response) => + /\/api\/v1\/workspaces\/[^/]+\/repositories$/.test(new URL(response.url()).pathname) && + response.request().method() === "GET" && + response.ok(), + ); + await testPage.getByTestId("repo-refresh-button").click(); + await refreshResponse; + await expect(testPage.getByTestId("discovery-failure")).toHaveCount(0); + await expect(testPage.getByRole("option", { name: /healthy-project/ })).toBeVisible(); + }); + test("uses the native picker and keeps the discovery root recoverable", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/task/repository-discovery-failure-helpers.ts b/apps/web/e2e/tests/task/repository-discovery-failure-helpers.ts new file mode 100644 index 0000000000..e8a12e4169 --- /dev/null +++ b/apps/web/e2e/tests/task/repository-discovery-failure-helpers.ts @@ -0,0 +1,49 @@ +import type { Page } from "@playwright/test"; + +export const DISCOVERY_FAILURE_ROOT = "/srv/repositories/inaccessible-root"; +export const DISCOVERED_REPOSITORY_PATH = "/srv/repositories/healthy-project"; + +type DiscoveryResponse = { + roots: string[]; + repositories: Array<{ path: string; name: string; default_branch: string }>; + total: number; + desktop_runtime: boolean; + root_states: []; + scan_time: string; + refreshing: boolean; + cached: boolean; + home_confirmation_required: boolean; + failed_roots: string[]; +}; + +function discoveryResponse(): DiscoveryResponse { + const repositories = [ + { + path: DISCOVERED_REPOSITORY_PATH, + name: "healthy-project", + default_branch: "main", + }, + ]; + return { + roots: ["/srv/repositories", DISCOVERY_FAILURE_ROOT], + repositories, + total: repositories.length, + desktop_runtime: false, + root_states: [], + scan_time: "2026-09-17T10:00:00.000Z", + refreshing: false, + cached: true, + home_confirmation_required: false, + failed_roots: [DISCOVERY_FAILURE_ROOT], + }; +} + +export async function installRepositoryDiscoveryFailureRoute(page: Page): Promise { + await page.route("**/api/v1/workspaces/*/repositories/discovery**", async (route) => { + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify(discoveryResponse()), + }); + }); +} diff --git a/apps/web/hooks/domains/workspace/use-repository-discovery.test.ts b/apps/web/hooks/domains/workspace/use-repository-discovery.test.ts index f404f8ef33..2d2e722e5a 100644 --- a/apps/web/hooks/domains/workspace/use-repository-discovery.test.ts +++ b/apps/web/hooks/domains/workspace/use-repository-discovery.test.ts @@ -41,6 +41,15 @@ function response(scanTime = oldScan): RepositoryDiscoveryResponse { }; } +function failedResponse( + scanTime = new Date(currentTime).toISOString(), +): RepositoryDiscoveryResponse { + return { + ...response(scanTime), + failed_roots: ["/missing"], + }; +} + function client(overrides: Partial = {}): RepositoryDiscoveryClient { return { getSnapshot: vi.fn(async () => response()), @@ -139,6 +148,41 @@ describe("RepositoryDiscoveryCoordinator", () => { coordinator.dispose(); }); + it("does not auto-refresh a snapshot with failed roots", async () => { + const api = client({ + getSnapshot: vi.fn(async () => failedResponse(oldScan)), + }); + const coordinator = new RepositoryDiscoveryCoordinator(api, { + now: () => currentTime, + }); + + const release = coordinator.acquire(workspaceId); + await vi.waitFor(() => expect(api.getSnapshot).toHaveBeenCalledTimes(1)); + expect(api.refresh).not.toHaveBeenCalled(); + expect(coordinator.getSnapshot(workspaceId).response?.failed_roots).toEqual(["/missing"]); + release(); + coordinator.dispose(); + }); + + it("allows an explicit refresh to recover a failed snapshot", async () => { + const api = client({ + getSnapshot: vi.fn(async () => failedResponse()), + refresh: vi.fn(async () => response(new Date(currentTime).toISOString())), + }); + const coordinator = new RepositoryDiscoveryCoordinator(api, { + now: () => currentTime, + }); + + const release = coordinator.acquire(workspaceId); + await vi.waitFor(() => expect(api.getSnapshot).toHaveBeenCalledTimes(1)); + await coordinator.refresh(workspaceId); + + expect(api.refresh).toHaveBeenCalledWith(workspaceId, "manual_refresh"); + expect(coordinator.getSnapshot(workspaceId).response?.failed_roots).toEqual([]); + release(); + coordinator.dispose(); + }); + it("joins a refresh reported by a fresh cached snapshot", async () => { const api = client({ getSnapshot: vi.fn(async () => ({ diff --git a/docs/plans/repository-discovery-failure-recovery/plan.md b/docs/plans/repository-discovery-failure-recovery/plan.md new file mode 100644 index 0000000000..b49d620676 --- /dev/null +++ b/docs/plans/repository-discovery-failure-recovery/plan.md @@ -0,0 +1,162 @@ +--- +created: 2026-09-17 +status: done +requirements: + - REQ-WORKSPACES-LOCAL-REPOSITORIES-003 + - REQ-WORKSPACES-LOCAL-REPOSITORIES-004 +system_design: + - ../../specs/workspaces/system-design/local-repositories.md +legacy_specs: [] +--- + +# Implementation Plan: Repository Discovery Failure Recovery + +## Overview + +Keep accessible repositories visible when filesystem discovery encounters denied +descendants or failed roots. First correct traversal and cache recovery. Then +keep failed-root diagnostics in structured logs while preserving existing +repository choices and Refresh actions. + +## Evidence and assumption check + +The v0.94.0 report shows an EPERM below Home at `Pictures/Photo Booth Library`. +The clone directory is also absent. `repoWalker.visit` propagates descendant +permission errors, and `scanRootForRepos` discards accumulated repositories. +`scanDiscoveryRoots` then replaces fresh successes with the entire previous +snapshot whenever any root fails. Server pickers hide `failed_roots`. + +A temporary test established a readable repository, injected EPERM through the +real walker callback, and observed an empty service result without an API error. +The reproduction and two existing discovery tests passed. The temporary test +was removed. This Linux run did not reproduce native macOS privacy enforcement. +Commit `d796cf131d` introduced the fatal descendant-error branch. + +The user requested a fix after this diagnosis. Workspace discovery owns the +contract. Existing AC-003.7 requires preservation and recovery. +New AC-003.9 through 003.12 clarify descendant failures and mixed root outcomes. +No unresolved product choice blocks this package. + +## Scope + +### In scope + +- Preserve accessible siblings after descendant EACCES or EPERM. +- Retain root failures, cancellation, and structured diagnostics. +- Merge fresh and cached results by exact scan root. +- Keep available choices and the normal Refresh action in server and phone + repository selectors. Failed-root diagnostics are logs-only. +- Preserve saved desktop root recovery controls. +- Update public recovery guidance during implementation. + +### Out of scope + +- Broaden Home exclusions, filesystem permissions, or repository grants. +- Create absent clone directories or suppress their failure reports. +- Change public API fields, root persistence, or background refresh policy. +- Change native folder selection, provider discovery, or workspace polling. + +## Technical approach + +`repository_discovery.go` skips inaccessible descendants while retaining root +errors. The scan forwards trigger and runtime context to existing diagnostics. +`repository_discovery_state.go` retains internal result slices by exact root +and keeps independent per-root snapshots across aggregate cache invalidation. +It deduplicates only the final response. Successful empty scans clear stale +entries. Cancellation leaves the prior snapshot unchanged. + +`RepositoryDiscoveryControls` keeps the existing desktop root-management +surface. Server and phone selectors do not render `failedRoots`; they keep +available choices and their normal Refresh action. The backend retains failed +root details in structured logs and the coordinator retains its retry policy. + +## ASCII UI preview + +UI-01: Create Task repository selector during a failed scan. +Failed-root diagnostics remain in backend logs. The selector keeps its normal +choices and Refresh action without adding a warning or path list. + +```text +Desktop picker Phone picker (same surface) ++-------------------------------+ +-------------------------------+ +| Search repositories [Refresh] | | Search repositories [Refresh] | +| project-a | | project-a | +| project-b | | project-b | ++-------------------------------+ +-------------------------------+ +``` + +UI-01 structure is required, but copy and spacing are illustrative. Existing +picker search, selection, dismissal, focus return, and Refresh behavior remain +intact. No failed-root warning or path list is rendered. Failed-root details +remain available in structured backend logs. The existing action is disabled +during refresh and shows progress where that surface already supports it. + +The phone entry point remains the repository selector. Reuse its current +surface, with one scroll owner and existing safe-area clearance. The Add +Workspace Sources drawer is the inline-error exemplar. No new overlay is needed. +Use 28-pixel desktop controls and at least 44-pixel touch targets. AC-003.11 +maps to desktop and mobile discovery E2E tests. + +## Tests + +All AC references below use the prefix `AC-WORKSPACES-LOCAL-REPOSITORIES`. + +| Criteria | Evidence | +| --- | --- | +| 003.9, 004.2 | New `repository_discovery_recovery_test.go`: descendant EACCES/EPERM, readable siblings before and after, root denial, and exact structured diagnostic target | +| 003.7, 003.10, 003.12 | Same file: cold cache, warm cache, missing clone root across two refreshes, successful empty root, overlapping roots, all roots fail, recovery | +| 003.6 | Existing discovery concurrency tests plus race-enabled recovery tests | +| 003.11, 003.8 | `repository-discovery-controls.test.tsx` and discovery E2E: failed-root paths are absent, available choices remain selectable, normal Refresh remains usable, and desktop root controls remain available | +| 003.4, 003.5 | `use-repository-discovery.test.ts`: failed snapshots do not auto-retry, explicit refresh still runs | + +## E2E tests + +- Extend `tests/task/repository-discovery-consent.spec.ts` in `chromium` with + server partial failure, hidden failed-root diagnostics, usable choices, and + normal Refresh. +- Extend `tests/task/mobile-repository-discovery.spec.ts` in `mobile-chrome` + with the same outcome and touch-target coverage for the existing Refresh + action. +- Use controlled discovery responses for deterministic UI failures. Backend + tests prove filesystem behavior without relying on root-bypassed chmod. + +## Work orders + +- [x] [Task 01: Preserve repositories across scan failures](task-01-scan-recovery.md) (done) +- [x] [Task 02: Preserve selector recovery without visible failure paths](task-02-picker-recovery.md) (done) + +Task 02 followed Task 01. Work was completed sequentially in the primary session. +The [original consent package](../desktop-repository-discovery-consent/plan.md) +retains its historical results. This package owns the additional regressions. + +## Verification results + +Implementation completed on 2026-09-17. Backend recovery, selector behavior, +E2E coverage, and public recovery guidance are in place. Failed-root paths +remain logs-only; no warning translations or selector path list are shipped. + +Passed checks: + +- `go test -tags fts5 ./internal/task/service -count=1` +- `go test -tags fts5 -race ./internal/task/service -run 'Discovery|DiscoverLocal|RepoWalker|ScanRoot|MacOSHome' -count=1` +- `go test -tags fts5 ./internal/common/fsdiagnostics -count=1` +- Focused frontend Vitest run: 82 tests passed across 6 files +- `pnpm run typecheck`, `pnpm run i18n:check`, and targeted ESLint +- Chromium and mobile-chrome discovery E2E suites: 3 tests passed in each +- `make build-web` and `make build-backend` +- Public-doc tests and validator, specification validation and lint, and + `git diff --check` + +Native macOS privacy enforcement was not available on this Linux host. The + regression is covered through deterministic walk-callback injection and the + browser flows use controlled discovery responses. + +## Risks + +- Overlapping roots require exact scan provenance to avoid stale duplicates. +- A successful root with a denied descendant cannot prove that missing cached + repositories were deleted. Its fresh accessible results replace its cache. +- A missing clone directory remains a structured root failure until it exists. +- Native macOS privacy dialogs require a Mac for final platform confirmation. +- Backend log access must be treated as sensitive because it can contain local + paths. diff --git a/docs/plans/repository-discovery-failure-recovery/task-01-scan-recovery.md b/docs/plans/repository-discovery-failure-recovery/task-01-scan-recovery.md new file mode 100644 index 0000000000..c964caa72f --- /dev/null +++ b/docs/plans/repository-discovery-failure-recovery/task-01-scan-recovery.md @@ -0,0 +1,116 @@ +--- +id: "01-scan-recovery" +title: "Preserve repositories across scan failures" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-WORKSPACES-LOCAL-REPOSITORIES-003 + - REQ-WORKSPACES-LOCAL-REPOSITORIES-004 +acceptance_criteria: + - AC-WORKSPACES-LOCAL-REPOSITORIES-003.6 + - AC-WORKSPACES-LOCAL-REPOSITORIES-003.7 + - AC-WORKSPACES-LOCAL-REPOSITORIES-003.9 + - AC-WORKSPACES-LOCAL-REPOSITORIES-003.10 + - AC-WORKSPACES-LOCAL-REPOSITORIES-003.12 + - AC-WORKSPACES-LOCAL-REPOSITORIES-004.2 +system_design: + - ../../specs/workspaces/system-design/local-repositories.md +--- + +# Task 01: Preserve repositories across scan failures + +## Summary + +Continue discovery after denied descendants and preserve usable results from +successful roots. Retain root failure diagnostics and explicit recovery policy. + +## In scope + +- Write `TestDiscoveryRecoveryDescendantPermission` before the correction. + Assert readable siblings before and after EACCES and EPERM remain visible. +- Keep root stat/read failures fatal and context cancellation distinct. +- Forward scan trigger and runtime into warnings for exact denied descendants. +- Store copied result slices per normalized root inside the existing cache. +- Write `TestDiscoveryRecoveryMixedRoots`, including two refreshes while the + clone root remains absent. Assert fresh successful results replace old ones. +- Cover all-root failure, recovery, empty successes, overlap, and cancellation. +- Update `TestRepoWalkerPropagatesAccessDenied` to distinguish root and child. + +## Out of scope + +UI, API schema changes, root persistence changes, directory creation, and new +Home exclusions are outside this work order. + +## Acceptance + +- Descendant errors preserve accessible results and diagnostic context. Root + errors remain reported. Cancellation does not publish an incomplete snapshot. +- Every successful root supplies fresh results, even when another root fails. + Only failed roots retain prior results. Overlap produces no duplicate paths. +- Existing single-flight, consent, cache-copy, and root-recovery tests pass. + +## Verification + +From the repository root: + +```bash +(cd apps/backend && go test -tags fts5 ./internal/task/service -run 'TestDiscoveryRecovery' -count=1 -v) +(cd apps/backend && go test -tags fts5 -race ./internal/task/service -run 'Discovery|DiscoverLocal|RepoWalker|ScanRoot|MacOSHome' -count=1) +(cd apps/backend && go test -tags fts5 ./internal/common/fsdiagnostics -count=1) +git diff --check +``` + +First record expected failures from the new regression tests. Use deterministic +walk-callback error injection when filesystem permissions cannot be enforced. +Drive the real scan and service path, not a duplicate traversal implementation. +If a real chmod fixture is used, probe enforcement and skip only that fixture +when the executor bypasses permissions. Deterministic coverage must still run. + +## Files likely touched + +- `apps/backend/internal/task/service/repository_discovery.go` +- `apps/backend/internal/task/service/repository_discovery_state.go` +- `apps/backend/internal/task/service/repository_discovery_test.go` +- New `apps/backend/internal/task/service/repository_discovery_recovery_test.go` +- `apps/backend/internal/task/service/service.go` if the scan function type changes +- `apps/backend/internal/task/service/filesystem_diagnostics.go` +- `apps/backend/internal/common/fsdiagnostics/context.go` as the existing diagnostic contract + +## Dependencies + +None. + +## Risks + +Do not reconstruct provenance from repository path containment. Do not mutate +shared cached maps or slices during an in-flight scan. Keep aggregate scan time +unchanged until all roots succeed. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/workspaces/requirements/local-repositories.md), REQ-003 and REQ-004. +- [Design](../../specs/workspaces/system-design/local-repositories.md), Partial scan recovery and Diagnostics. +- Existing `TestDesktopDiscoveryFailurePreservesCachedRepositories` and discovery concurrency tests. + +## Results + +Implemented descendant-permission recovery and per-root cache replacement. +Inaccessible descendants now produce bounded structured warnings while root +failures remain visible. Successful roots replace their own cached results, +failed roots retain prior results, and independent exact-root snapshots survive +aggregate invalidation during Add and Reconnect. Successful empty scans clear +stale entries, and cancellation does not publish a partial snapshot. + +Verification passed: + +- `go test -tags fts5 ./internal/task/service -run 'TestDiscoveryRecovery|TestScanRootForReposRootAccessFailureRemainsFatal|TestRepoWalkerPropagatesRootAccessDeniedButSkipsChild' -count=1 -v` +- `go test -tags fts5 ./internal/task/service -run 'TestDiscoveryRecoveryRootSetChangesRetainUnchangedRootSnapshots' -count=1` +- `go test -tags fts5 -race ./internal/task/service -run 'Discovery|DiscoverLocal|RepoWalker|ScanRoot|MacOSHome' -count=1` +- `go test -tags fts5 ./internal/common/fsdiagnostics -count=1` +- `git diff --check` diff --git a/docs/plans/repository-discovery-failure-recovery/task-02-picker-recovery.md b/docs/plans/repository-discovery-failure-recovery/task-02-picker-recovery.md new file mode 100644 index 0000000000..8c17e057d0 --- /dev/null +++ b/docs/plans/repository-discovery-failure-recovery/task-02-picker-recovery.md @@ -0,0 +1,159 @@ +--- +id: "02-picker-recovery" +title: "Preserve selector recovery without visible failure paths" +status: done +wave: 2 +depends_on: ["01-scan-recovery"] +plan: "plan.md" +requirements: + - REQ-WORKSPACES-LOCAL-REPOSITORIES-003 +acceptance_criteria: + - AC-WORKSPACES-LOCAL-REPOSITORIES-003.4 + - AC-WORKSPACES-LOCAL-REPOSITORIES-003.5 + - AC-WORKSPACES-LOCAL-REPOSITORIES-003.7 + - AC-WORKSPACES-LOCAL-REPOSITORIES-003.8 + - AC-WORKSPACES-LOCAL-REPOSITORIES-003.11 +system_design: + - ../../specs/workspaces/system-design/local-repositories.md +--- + +# Task 02: Preserve selector recovery without visible failure paths + +## Summary + +Keep failed-root diagnostics in structured backend logs while preserving +available choices and the normal Refresh action in existing selectors. + +## In scope + +- Add component and E2E coverage for a server response with repositories and + failed roots, asserting that failed paths are not rendered. +- Keep existing normal Refresh behavior available during partial failure and + refresh the repository list without a background retry. +- Keep saved desktop Reconnect/Remove controls unchanged. +- Retain `failedRoots` for coordinator retry suppression and structured + diagnostics; preserve the existing repository-list Refresh in selectors. +- Check all existing consumers: task creation, workspace sources, repository + settings, automations, and Office project setup. +- Remove warning-only locale keys that no longer have a selector consumer. +- Extend desktop and mobile discovery E2E with partial-failure and selection + flows. +- Update recovery guidance in configuration and usage documentation. + +## Out of scope + +New picker layouts, native bridge changes, new discovery endpoints, and +automatic retry changes are outside this work order. + +## Acceptance + +- Server and phone selectors do not show failed-root warnings or paths when a + discovery response contains `failed_roots`. +- Available choices remain selectable during failure, and the existing normal + Refresh action remains reachable and refreshes the repository list without a + background retry. +- Desktop E2E keeps explicit saved-root Reconnect/Remove management. Phone E2E + keeps the existing Refresh action at a 44-pixel touch target. + +## ASCII UI preview + +UI-01: Failed scan, excerpt from [the plan](plan.md#ascii-ui-preview). + +```text +Desktop selector Phone selector +Search + Refresh Search + Refresh +Available repository choices Available repository choices +``` + +Keep the existing selector surface and scroll owner. Do not add a failure +warning, path list, or selector-specific containment for failed roots. During +refresh, preserve the existing action state and available choices. Failed-root +details remain in structured backend logs. These structural requirements +implement AC-003.11. + +## Verification + +From the repository root, install workspace dependencies if this worktree has none. + +```bash +(cd apps && pnpm install --frozen-lockfile) +(cd apps/web && pnpm exec vitest run components/repository-discovery-controls.test.tsx components/repository-discovery-root-controls.test.tsx hooks/domains/workspace/use-repository-discovery.test.ts) +(cd apps/web && pnpm run typecheck) +(cd apps/web && pnpm run i18n:check) +(cd apps/web && pnpm exec eslint components/repository-discovery-controls.tsx) +make build-web +make build-backend +(cd apps/web && pnpm e2e:run --project chromium tests/task/repository-discovery-consent.spec.ts) +(cd apps/web && pnpm e2e:run --project mobile-chrome tests/task/mobile-repository-discovery.spec.ts) +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +Run added component/helper tests explicitly if implementation creates separate files. +Inspect the rendered phone selector and compare it with UI-01. Keep E2E runs +sequential under the existing worker limits. Use controlled API responses for +partial discovery results. Task 01 supplies deterministic filesystem +regression coverage. + +## Files likely touched + +- `apps/web/components/repository-discovery-controls.tsx` and its test +- `apps/web/hooks/domains/workspace/use-repository-discovery.test.ts` +- `apps/web/components/repository-discovery-root-controls.test.tsx` +- `apps/web/components/task-create-dialog-pill.tsx` +- `apps/web/e2e/tests/task/repository-discovery-consent.spec.ts` +- `apps/web/e2e/tests/task/mobile-repository-discovery.spec.ts` +- `apps/web/src/locales/` required catalogs +- `docs/public/configuration.md` (reference) +- `docs/public/use-kandev.md` (how-to) + +## Dependencies + +Task 01 provides the corrected result semantics. Public docs describe the final +implementation, not draft behavior. + +## Risks + +Some selectors already provide Refresh. Preserve the existing action instead +of adding a second recovery control. Do not offer root mutations for +operator-configured paths. Preserve accessibility and existing desktop controls. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/workspaces/requirements/local-repositories.md), REQ-003. +- [Design](../../specs/workspaces/system-design/local-repositories.md), User interface. +- Existing discovery-controls tests and desktop/mobile consent E2E fixtures. +- Mobile-parity, E2E, TDD, and docs-maintainer skills. + +## Results + +Kept failed-root details in structured backend logs and removed their selector +warning/path list. Browser and phone selectors retain available repositories +and the normal Refresh action during partial failure. Saved desktop roots retain +Reconnect and Remove controls. + +Updated component, hook, and Chromium/mobile-chrome E2E coverage. Removed the +unused warning locale keys and updated public configuration, desktop, and usage +guidance. + +Verification passed: + +- Correction-focused Vitest run: 29 tests passed across 4 files +- `pnpm run typecheck` +- `pnpm run i18n:check` +- Targeted ESLint for discovery components and workspace repository settings +- `pnpm e2e:run --project chromium tests/task/repository-discovery-consent.spec.ts` (3 passed) +- `pnpm e2e:run --project mobile-chrome tests/task/mobile-repository-discovery.spec.ts` (3 passed) +- Mobile E2E covers failed-root diagnostics staying out of the selector, + reachable Refresh, and healthy repository selection. +- `make build-web` +- `node --test scripts/validate-public-docs.test.mjs` +- `node scripts/validate-public-docs.mjs` +- `git diff --check` diff --git a/docs/public/configuration.md b/docs/public/configuration.md index 714b4d769c..0a79117d6f 100644 --- a/docs/public/configuration.md +++ b/docs/public/configuration.md @@ -334,6 +334,15 @@ details. A browser can connect to a desktop backend and use the HTTP folder picker; the browser's picker capability does not change the backend's discovery policy. +If discovery cannot read a root, repository selectors keep repositories from +successful roots available and retain their normal **Refresh repositories** +action. Failed-root paths stay in structured backend logs and are not shown in +selectors. A denied descendant does not require root reconnection. If a saved +Desktop root itself fails, use **Reconnect** or **Remove**. Kandev does not +create a missing clone directory during recovery. +You can also enter an absolute repository path in **Add Local Repository** and +select **Validate**; this explicit check is independent of discovery roots. + ### Debug configuration | YAML key | Environment variable | Default | Current behavior | diff --git a/docs/public/desktop-app.md b/docs/public/desktop-app.md index d399fb372b..ab6f56c520 100644 --- a/docs/public/desktop-app.md +++ b/docs/public/desktop-app.md @@ -144,6 +144,14 @@ shows **Reconnect** and **Remove** actions. Reconnect selects the folder again and starts a fresh scan. Removal forgets that desktop discovery folder; it does not remove repositories or files. +If one root fails while another succeeds, repository selectors keep the +successful results and their normal **Refresh repositories** action. Failed-root +paths stay in structured backend logs and are not shown in selectors. An +inaccessible descendant does not make its accessible root require +reconnection. You can use **Add Local Repository** to validate an absolute path +when automatic discovery cannot reach its root. Kandev does not create missing +clone directories during recovery. + macOS privacy access belongs to the application identity. A replacement that is unsigned, re-signed, or otherwise has a different identity can require access again. Kandev cannot promise that consent survives every unsigned diff --git a/docs/public/use-kandev.md b/docs/public/use-kandev.md index c0d82bf100..6fdf62daa5 100644 --- a/docs/public/use-kandev.md +++ b/docs/public/use-kandev.md @@ -127,6 +127,14 @@ workspace there from the menu sheet instead. 4. Select **Use Repository**. This opens an unsaved repository card. 5. Review the repository name, worktree branch template, pull behavior, setup/cleanup/dev scripts, copied files, and custom commands. Then select **Save changes**. +If discovery cannot scan a root, the picker keeps repositories from successful +roots available and retains its normal **Refresh repositories** action. Failed +root paths stay in structured backend logs and are not shown in selectors. A +missing or inaccessible root does not prevent you from validating an absolute +repository path. On Desktop, a saved root that fails also offers **Reconnect** +and **Remove**. Kandev does not retry failed roots in the background or create +missing clone directories. + New local repository records default to: - source type `local`; diff --git a/docs/specs/workspaces/requirements/local-repositories.md b/docs/specs/workspaces/requirements/local-repositories.md index 05e1072625..aa6e21e6e6 100644 --- a/docs/specs/workspaces/requirements/local-repositories.md +++ b/docs/specs/workspaces/requirements/local-repositories.md @@ -2,7 +2,7 @@ status: active system: workspaces created: 2026-07-20 -updated: 2026-08-30 +updated: 2026-09-17 owners: - kandev --- @@ -108,9 +108,28 @@ can display a permission dialog after the user leaves the application. - **AC-WORKSPACES-LOCAL-REPOSITORIES-003.6:** Concurrent requests for the same roots shall share one scan. - **AC-WORKSPACES-LOCAL-REPOSITORIES-003.7:** A failed refresh shall preserve the - last successful result and shall expose the failed root and recovery action. + last successful result. Failed-root details shall remain in structured backend + diagnostics. Repository selectors shall retain their existing manual Refresh + action, and saved desktop roots shall retain their explicit recovery actions. - **AC-WORKSPACES-LOCAL-REPOSITORIES-003.8:** An empty or filtered discovery result shall show a visible manual Refresh action. +- **AC-WORKSPACES-LOCAL-REPOSITORIES-003.9:** When a descendant is inaccessible, + discovery shall continue through accessible siblings and retain repositories + already found. An accessible root shall not require reconnection solely + because a descendant is inaccessible. +- **AC-WORKSPACES-LOCAL-REPOSITORIES-003.10:** When at least one root succeeds + and another fails, discovery shall return fresh results from every successful + root. It shall retain previous results only for failed roots. Successful + empty scans shall remove obsolete results from those roots. +- **AC-WORKSPACES-LOCAL-REPOSITORIES-003.11:** When roots fail, browser and phone + repository selectors shall keep available repositories selectable and retain + their normal manual Refresh action. They shall not render failed-root paths or + a failed-root warning. Failed-root details shall remain in structured backend + diagnostics. Saved desktop roots shall retain their Reconnect and Remove + actions. +- **AC-WORKSPACES-LOCAL-REPOSITORIES-003.12:** A missing clone directory shall + not prevent results from other roots from appearing on initial or later scans. + Discovery shall not create the directory to recover from this condition. ### REQ-WORKSPACES-LOCAL-REPOSITORIES-004: Filesystem access diagnostics @@ -281,6 +300,8 @@ the repository record removes that exact durable grant from the workspace. ## Implementation Plans +- [Repository Discovery Failure Recovery](../../../plans/repository-discovery-failure-recovery/plan.md) + - [Explicit Local Repository Trust](../../../plans/explicit-local-repository-trust/plan.md) - [Local-only Merge and Rebase](../../../plans/local-only-merge-rebase/plan.md) - [Desktop Repository Discovery Consent](../../../plans/desktop-repository-discovery-consent/plan.md) diff --git a/docs/specs/workspaces/system-design/local-repositories.md b/docs/specs/workspaces/system-design/local-repositories.md index c2410d3d0a..6355012d6f 100644 --- a/docs/specs/workspaces/system-design/local-repositories.md +++ b/docs/specs/workspaces/system-design/local-repositories.md @@ -133,8 +133,11 @@ does not copy these roots into SQLite. The workspace-scoped discovery endpoint returns repositories for one workspace. It does not give workspace scope to the desktop root records. -The repository-discovery cache is keyed by the normalized root set and maximum -depth. It stores the last successful repositories, scan time, and root state. +The aggregate repository-discovery cache is keyed by the normalized root set +and maximum depth. It stores the last successful repositories, scan time, and +root state. A secondary snapshot cache is keyed by each exact normalized root +and maximum depth. It survives aggregate invalidation so an unchanged root can +retain its last successful repositories while a changed root set is scanned. One single-flight scan serves concurrent workspace requests for the same key. ## Upgrade behavior @@ -160,8 +163,46 @@ migration. 3. The frontend renders saved and cached repositories immediately. 4. If the surface is active and the snapshot is 30 minutes old, it requests a refresh. 5. The backend shares an existing scan or starts one scan for the root set. -6. Success replaces the cache and broadcasts the new snapshot. -7. Failure preserves the cache and marks only the failed root for recovery. +6. Each successful root replaces its cached results, including an empty result. +7. Each failed root retains only its previous results and reports recovery state. + +### Partial scan recovery + +This section defines AC-WORKSPACES-LOCAL-REPOSITORIES-003.9 through 003.12. +`repoWalker.visit` distinguishes root errors from descendant errors. +An inaccessible root fails its scan. An inaccessible descendant produces a +structured warning and does not terminate traversal of accessible siblings. +Cancellation and deadline expiry abort the operation without a cache write. +The existing Home exclusions and explicit-path validation remain unchanged. + +`scanRootForRepos` retains repositories found before a descendant error. +The scan carries the existing runtime and trigger context into descendant +diagnostics. Warnings identify the denied descendant, not just its root. +The walker emits at most one warning per denied path during one scan. +It does not retry denied paths within that scan. + +The aggregate `discoveryCacheEntry` retains results by exact normalized scan +root internally. A secondary per-root snapshot retains each root's last +successful result independently of the aggregate root-set key. This preserves +unchanged roots across Add or Reconnect invalidation without borrowing results +from a different normalized path or maximum depth. `scanDiscoveryRoots` +replaces successful root entries and retains failed root entries. A root +without previous results contributes an empty list on failure. +The response deduplicates the union by repository path. Root membership comes +from scan provenance, not a path-prefix guess, because effective roots can overlap. +Cache snapshots and responses copy their slices to prevent concurrent mutation. + +The public response keeps its existing fields. `failed_roots` lists current +root failures. Descendant denials do not mark accessible roots as failed. +An absent clone root remains a reported root failure, but never replaces fresh +results from another root. Discovery does not create directories. Failed-root +paths are diagnostic data and are not rendered by repository selectors. + +The aggregate `scan_time` advances only after all roots succeed. During partial +failure, it retains the previous complete-scan time, or remains absent. +The coordinator retains automatic-retry suppression for failed snapshots. +Manual Refresh retries the effective roots through the existing single-flight +operation. This repair does not introduce background retries. The shared discovery coordinator owns one activation count in each browser tab. An open consumer acquires one activation lease. It releases the lease when the @@ -202,6 +243,19 @@ browser on a desktop backend uses desktop policy and the HTTP folder browser. Temporary repository choices use the current phone-native picker or drawer composition. No native Tauri control appears in a browser or mobile viewport. +`RepositoryDiscoveryControls` remains the desktop root-management surface. It +does not render failed-root warnings or paths for server, browser, or phone +selectors. Those selectors keep their available repository choices and their +normal manual Refresh action. The backend and coordinator retain failed-root +data for structured diagnostics and automatic-retry suppression, but that data +is logs-only from the selector's perspective. + +Saved desktop roots retain their existing Reconnect and Remove actions. Those +controls manage explicit saved roots and do not extend to operator-configured +roots. Phone presentation reuses each existing selector and its scroll owner; +no warning details or extra scroll owner is added. Desktop actions remain 28 +pixels high. Phone and coarse-pointer actions have at least 44-pixel hit targets. + ## Workspace polling Repository discovery and workspace monitoring remain separate functions. Normal @@ -268,7 +322,8 @@ Suggested event names are: - Picker cancellation changes no grant and starts no scan. - An invalid selected path is rejected without persistence. -- A partial scan returns the last successful cache and identifies failed roots. +- A partial scan combines fresh successful roots with cached failed roots. +- An inaccessible descendant does not discard accessible sibling results. - A denied root moves to `reconnect_required` and receives no automatic retry. - An unsigned update can change macOS code identity. The UI offers Reconnect and does not claim that one consent will survive every update. @@ -333,6 +388,10 @@ repository trust](../../../decisions/2026-08-28-explicit-submodule-repository-tr - Lifecycle tests cover final refresh, paused delivery, startup fallback, focus, and new operation activity. +## Implementation plans + +- [Repository Discovery Failure Recovery](../../../plans/repository-discovery-failure-recovery/plan.md) + ## Decisions - [Use Explicit Roots for Desktop Repository Discovery](../../../decisions/2026-08-27-explicit-desktop-repository-discovery-roots.md)