From a5d6649f69dc8878f960209a728d675f83778fa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Thu, 6 Aug 2026 15:04:19 +0100 Subject: [PATCH] feat(review): support nested submodule review (#2295) --- .../internal/agent/runtime/agentctl/git.go | 1 + .../agent/runtime/lifecycle/event_types.go | 3 + .../agent/runtime/lifecycle/events.go | 1 + .../lifecycle/events_git_status_test.go | 4 + .../internal/agentctl/server/api/git.go | 86 +++- .../server/api/git_multi_repo_review_test.go | 104 ++++ .../internal/agentctl/server/api/workspace.go | 2 +- .../agentctl/server/process/manager.go | 94 ++-- .../agentctl/server/process/manager_rescan.go | 101 +--- .../server/process/manager_rescan_test.go | 38 ++ .../server/process/manager_submodule_test.go | 144 ++++++ .../server/process/manager_submodules.go | 454 ++++++++++++++++++ .../agentctl/server/process/runner.go | 54 ++- .../server/process/runner_tracker_test.go | 123 +++++ .../server/process/workspace_git_status.go | 23 + .../workspace_git_status_base_branch_test.go | 27 ++ .../server/process/workspace_tracker.go | 59 +++ .../internal/agentctl/types/streams/git.go | 6 + apps/backend/internal/backendapp/helpers.go | 1 + apps/backend/internal/review/diff.go | 6 +- apps/backend/internal/review/diff_test.go | 10 +- .../review/review-dialog-submodule.test.ts | 34 ++ .../review/review-dialog.build-files.test.ts | 103 +++- apps/web/components/review/review-dialog.tsx | 31 +- .../components/review/review-diff-group.tsx | 26 + .../review/review-diff-header.test.tsx | 25 + .../components/review/review-diff-header.tsx | 10 +- .../review/review-diff-list-grouping.test.tsx | 50 +- .../review/review-diff-list-groups.tsx | 19 +- .../components/review/review-diff-list.tsx | 23 +- .../review/review-file-tree.test.tsx | 13 + .../components/review/review-file-tree.tsx | 24 +- .../review/types.multi-repo.test.ts | 65 ++- apps/web/components/review/types.ts | 195 ++++++-- .../components/task/changes-panel-body.tsx | 9 +- .../components/task/changes-panel-helpers.ts | 7 +- .../task/changes-panel-repo-groups.tsx | 8 + .../task/changes-panel-timeline.tsx | 7 + .../components/task/changes-panel-tree.tsx | 4 + .../task/dockview-desktop-layout.tsx | 7 +- .../task/task-changes-panel.test.ts | 11 +- .../components/task/task-changes-panel.tsx | 11 +- .../components/task/use-review-dialog.test.ts | 25 + apps/web/components/task/use-review-dialog.ts | 52 +- apps/web/components/vcs-split-button.tsx | 2 +- apps/web/components/vcs/vcs-dialogs.test.ts | 10 +- apps/web/components/vcs/vcs-dialogs.tsx | 55 +-- .../review/mobile-submodule-review.spec.ts | 88 ++++ .../tests/review/submodule-review-helpers.ts | 148 ++++++ .../e2e/tests/review/submodule-review.spec.ts | 144 ++++++ .../use-review-sources-normalization.test.ts | 157 ++++++ .../session/use-review-sources-scoped.test.ts | 70 +++ .../session/use-review-sources.test.ts | 8 + .../domains/session/use-review-sources.ts | 175 ++++--- .../session/use-session-git-grouping.test.ts | 26 +- .../use-session-git-repository-order.test.ts | 73 +++ .../use-session-git-repository-order.ts | 96 ++++ .../session/use-session-git-summary.test.ts | 80 +++ .../session/use-session-git-summary.ts | 70 +++ .../hooks/domains/session/use-session-git.ts | 228 +++++---- apps/web/hooks/use-git-operations.test.ts | 27 +- apps/web/hooks/use-git-operations.ts | 33 +- apps/web/lib/review/findings.ts | 6 +- .../state/dockview-env-switch-action.test.ts | 15 + .../web/lib/state/dockview-env-switch.test.ts | 12 + apps/web/lib/state/dockview-env-switch.ts | 21 +- apps/web/lib/state/dockview-store.ts | 15 +- .../session-runtime/session-runtime-slice.ts | 3 + .../set-git-status-return.test.ts | 16 + .../lib/state/slices/session-runtime/types.ts | 4 + apps/web/lib/types/git-events.ts | 2 + apps/web/lib/types/github.ts | 2 + apps/web/lib/ws/handlers/git-status.test.ts | 10 + apps/web/lib/ws/handlers/git-status.ts | 1 + apps/web/src/locales/en/common.json | 2 + apps/web/src/locales/pseudo/common.json | 2 + apps/web/src/locales/zh-cn/common.json | 2 + ...-nested-submodules-as-repository-scopes.md | 46 ++ docs/decisions/INDEX.md | 1 + docs/plans/submodule-review/plan.md | 108 +++++ ...sk-01-discover-nested-repository-scopes.md | 62 +++ .../task-02-aggregate-submodule-git-data.md | 61 +++ ...k-03-present-submodule-review-hierarchy.md | 78 +++ .../task-04-order-nested-git-mutations.md | 59 +++ .../task-05-prove-submodule-review.md | 60 +++ .../task-06-document-submodule-review.md | 58 +++ docs/public/feature-status.md | 2 +- docs/public/sessions-and-review.md | 4 +- docs/specs/INDEX.md | 1 + docs/specs/ui/submodule-review.md | 70 +++ 90 files changed, 3798 insertions(+), 515 deletions(-) create mode 100644 apps/backend/internal/agentctl/server/process/manager_submodule_test.go create mode 100644 apps/backend/internal/agentctl/server/process/manager_submodules.go create mode 100644 apps/backend/internal/agentctl/server/process/runner_tracker_test.go create mode 100644 apps/web/components/review/review-dialog-submodule.test.ts create mode 100644 apps/web/components/review/review-diff-group.tsx create mode 100644 apps/web/e2e/tests/review/mobile-submodule-review.spec.ts create mode 100644 apps/web/e2e/tests/review/submodule-review-helpers.ts create mode 100644 apps/web/e2e/tests/review/submodule-review.spec.ts create mode 100644 apps/web/hooks/domains/session/use-review-sources-scoped.test.ts create mode 100644 apps/web/hooks/domains/session/use-session-git-repository-order.test.ts create mode 100644 apps/web/hooks/domains/session/use-session-git-repository-order.ts create mode 100644 apps/web/hooks/domains/session/use-session-git-summary.test.ts create mode 100644 apps/web/hooks/domains/session/use-session-git-summary.ts create mode 100644 docs/decisions/2026-08-05-nested-submodules-as-repository-scopes.md create mode 100644 docs/plans/submodule-review/plan.md create mode 100644 docs/plans/submodule-review/task-01-discover-nested-repository-scopes.md create mode 100644 docs/plans/submodule-review/task-02-aggregate-submodule-git-data.md create mode 100644 docs/plans/submodule-review/task-03-present-submodule-review-hierarchy.md create mode 100644 docs/plans/submodule-review/task-04-order-nested-git-mutations.md create mode 100644 docs/plans/submodule-review/task-05-prove-submodule-review.md create mode 100644 docs/plans/submodule-review/task-06-document-submodule-review.md create mode 100644 docs/specs/ui/submodule-review.md diff --git a/apps/backend/internal/agent/runtime/agentctl/git.go b/apps/backend/internal/agent/runtime/agentctl/git.go index 1a2f1e2fff..5bd8ff951f 100644 --- a/apps/backend/internal/agent/runtime/agentctl/git.go +++ b/apps/backend/internal/agent/runtime/agentctl/git.go @@ -499,6 +499,7 @@ func (c *Client) GetCumulativeDiff(ctx context.Context, baseCommit, targetBranch // GitStatusResult represents the result of a git status query. type GitStatusResult struct { Success bool `json:"success"` + IsSubmodule bool `json:"is_submodule,omitempty"` Branch string `json:"branch"` RemoteBranch string `json:"remote_branch"` HeadCommit string `json:"head_commit"` diff --git a/apps/backend/internal/agent/runtime/lifecycle/event_types.go b/apps/backend/internal/agent/runtime/lifecycle/event_types.go index 077ff04eec..1174fb3099 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/event_types.go +++ b/apps/backend/internal/agent/runtime/lifecycle/event_types.go @@ -295,6 +295,9 @@ type GitStatusData struct { // multi-repo task workspaces. Empty for single-repo. Carried through to // the frontend so the Changes panel can render per-repo group headers. RepositoryName string `json:"repository_name,omitempty"` + // IsSubmodule identifies an initialized Git submodule repository so the + // frontend can render its scope boundary without guessing from its name. + IsSubmodule bool `json:"is_submodule,omitempty"` } type GitCommitData struct { diff --git a/apps/backend/internal/agent/runtime/lifecycle/events.go b/apps/backend/internal/agent/runtime/lifecycle/events.go index fce23821c4..6c59d9249c 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/events.go +++ b/apps/backend/internal/agent/runtime/lifecycle/events.go @@ -305,6 +305,7 @@ func (p *EventPublisher) PublishGitStatus(execution *AgentExecution, update *age BranchAdditions: update.BranchAdditions, BranchDeletions: update.BranchDeletions, RepositoryName: update.RepositoryName, + IsSubmodule: update.IsSubmodule, }, }) } diff --git a/apps/backend/internal/agent/runtime/lifecycle/events_git_status_test.go b/apps/backend/internal/agent/runtime/lifecycle/events_git_status_test.go index 99c853d712..5033385a7f 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/events_git_status_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/events_git_status_test.go @@ -39,6 +39,7 @@ func TestPublishGitStatus_PropagatesRepositoryName(t *testing.T) { pub.PublishGitStatus(exec, &agentctl.GitStatusUpdate{ Timestamp: time.Now(), RepositoryName: "frontend", + IsSubmodule: true, Branch: "feature/x", Modified: []string{"src/app.tsx"}, Files: map[string]agentctl.FileInfo{"src/app.tsx": {Path: "src/app.tsx"}}, @@ -56,6 +57,9 @@ func TestPublishGitStatus_PropagatesRepositoryName(t *testing.T) { if payload.Status.RepositoryName != "frontend" { t.Errorf("repository_name was dropped: got %q", payload.Status.RepositoryName) } + if !payload.Status.IsSubmodule { + t.Error("is_submodule was dropped") + } case <-time.After(2 * time.Second): t.Fatal("timed out waiting for git status event") } diff --git a/apps/backend/internal/agentctl/server/api/git.go b/apps/backend/internal/agentctl/server/api/git.go index 26738d33e2..02f6f71e2c 100644 --- a/apps/backend/internal/agentctl/server/api/git.go +++ b/apps/backend/internal/agentctl/server/api/git.go @@ -693,8 +693,8 @@ func (s *Server) handleGitLog(c *gin.Context) { } if req.Repo == "" { - if subs := s.procMgr.RepoSubpaths(); len(subs) > 0 { - s.handleGitLogMultiRepo(c, req, subs, limit) + if scopes := s.procMgr.RepositoryScopes(); shouldFanOutRepositoryScopes(scopes) { + s.handleGitLogMultiRepo(c, req, scopes, limit) return } } @@ -961,8 +961,8 @@ func (s *Server) handleGitCumulativeDiff(c *gin.Context) { } if req.Repo == "" { - if subs := s.procMgr.RepoSubpaths(); len(subs) > 0 { - s.handleGitCumulativeDiffMultiRepo(c, req, subs) + if scopes := s.procMgr.RepositoryScopes(); shouldFanOutRepositoryScopes(scopes) { + s.handleGitCumulativeDiffMultiRepo(c, req, scopes) return } } @@ -1080,7 +1080,13 @@ func (s *Server) handleGitCumulativeDiffMultiRepo( } anyOK = true merged.TotalCommits += outcome.result.TotalCommits - mergeCumulativeFiles(merged.Files, outcome.result.Files, outcome.subpath, outcome.result.BaseCommit) + mergeCumulativeFiles( + merged.Files, + outcome.result.Files, + outcome.subpath, + outcome.result.BaseCommit, + outcome.isSubmodule, + ) } if !anyOK { merged.Success = false @@ -1094,26 +1100,33 @@ func (s *Server) handleGitCumulativeDiffMultiRepo( // and was skipped; a non-nil error is a hard failure that aborts the request, // with status holding the code the serial version would have written. type perRepoDiffOutcome struct { - subpath string - result *process.CumulativeDiffResult - status int - err error + subpath string + result *process.CumulativeDiffResult + status int + err error + isSubmodule bool } // collectCumulativeDiffForRepo runs the cumulative diff for one repository of a // multi-repo fan-out. Like collectLogForRepo it takes a plain context and never // writes to the gin context, so it is safe to run concurrently. func (s *Server) collectCumulativeDiffForRepo(ctx context.Context, sub string) perRepoDiffOutcome { - base := s.resolvePerRepoBase(ctx, sub) + base, isSubmodule := s.resolvePerRepoBaseAndScope(ctx, sub) if base == "" { s.logger.Warn("cumulative diff: no per-repo base, skipping", zap.String("repo", sub)) - return perRepoDiffOutcome{subpath: sub} + return perRepoDiffOutcome{subpath: sub, isSubmodule: isSubmodule} } // Multi-repo: base is already resolved per-repo via resolvePerRepoBase, // so we pass empty target_branch to skip the second merge-base attempt. result, status, err := s.runGitCumulativeDiffForRepo(ctx, base, "", sub) - return perRepoDiffOutcome{subpath: sub, result: result, status: status, err: err} + return perRepoDiffOutcome{ + subpath: sub, + result: result, + status: status, + err: err, + isSubmodule: isSubmodule, + } } // fanOutRepos runs collect once per repository, concurrently, and returns the @@ -1163,31 +1176,45 @@ func fanOutRepos[T any](ctx context.Context, subpaths []string, collect func(con // Without this, a repo whose base branch is a merged/deleted stacked parent // lingering only as a local ref keeps inflating the commit count and diff. func (s *Server) resolvePerRepoBase(ctx context.Context, repo string) string { + base, _ := s.resolvePerRepoBaseAndScope(ctx, repo) + return base +} + +// resolvePerRepoBaseAndScope returns the repository's comparison anchor and +// whether its workspace is a submodule. The metadata travels with cumulative +// diff results so the frontend can distinguish nested repository scopes from +// ordinary sibling repositories that merely happen to have a named path. +func (s *Server) resolvePerRepoBaseAndScope(ctx context.Context, repo string) (string, bool) { tracker, err := s.procMgr.GetWorkspaceTrackerFor(repo) if err != nil { - return "" + return "", false } + isSubmodule := tracker.IsSubmodule() base, baseBranch := tracker.ResolveBaseAnchor(ctx) if base == "" || baseBranch == "" { - return base + return base, isSubmodule + } + if isSubmodule { + return base, true } gitOp, gitOpErr := s.procMgr.GitOperatorFor(repo) if gitOpErr != nil { - return base + return base, false } - return gitOp.CorrectStaleComparisonBase(ctx, base, baseBranch) + return gitOp.CorrectStaleComparisonBase(ctx, base, baseBranch), false } // mergeCumulativeFiles copies per-repo files into the merged map under a // ` ` key (NUL-separated) and decorates each file payload -// with `repository_name`, the repo-relative `path`, and the exact old-side -// `base_ref` used to produce its cumulative diff. +// with `repository_name`, the repo-relative `path`, the exact old-side +// `base_ref` used to produce its cumulative diff, and (for nested repository +// scopes) `is_submodule`. // The composite key keeps `README.md` in two repos from clashing in the map; // the frontend reads `path` and `repository_name` off the payload so the // file tree groups under the repo header without the prefix bleeding into // the displayed path. NUL is impossible in real paths, so the key is // always uniquely splittable and the displayed path is unaffected. -func mergeCumulativeFiles(dst, src map[string]interface{}, repo, baseRef string) { +func mergeCumulativeFiles(dst, src map[string]interface{}, repo, baseRef string, isSubmodule bool) { for path, payload := range src { m, ok := payload.(map[string]interface{}) if !ok { @@ -1201,7 +1228,7 @@ func mergeCumulativeFiles(dst, src map[string]interface{}, repo, baseRef string) // path, and base_ref so the caller's source map isn't mutated. Earlier // code wrote directly to `m`, which permanently rewrote the per-repo result // before it could be reused (e.g. emitted to a second consumer). - copied := make(map[string]interface{}, len(m)+3) + copied := make(map[string]interface{}, len(m)+4) for k, v := range m { copied[k] = v } @@ -1210,6 +1237,9 @@ func mergeCumulativeFiles(dst, src map[string]interface{}, repo, baseRef string) if baseRef != "" { copied["base_ref"] = baseRef } + if isSubmodule { + copied["is_submodule"] = true + } dst[fmt.Sprintf("%s\x00%s", repo, path)] = copied } } @@ -1217,6 +1247,7 @@ func mergeCumulativeFiles(dst, src map[string]interface{}, repo, baseRef string) // GitStatusResult represents the result of a git status query. type GitStatusResult struct { Success bool `json:"success"` + IsSubmodule bool `json:"is_submodule,omitempty"` Branch string `json:"branch"` RemoteBranch string `json:"remote_branch"` HeadCommit string `json:"head_commit"` @@ -1255,6 +1286,17 @@ type MultiRepoGitStatusResult struct { Error string `json:"error,omitempty"` } +func shouldFanOutRepositoryScopes(scopes []string) bool { + if len(scopes) == 0 { + return false + } + return len(scopes) > 1 || scopes[0] != "" +} + +func requiresExplicitRepositoryScope(scopes []string) bool { + return len(scopes) > 0 && scopes[0] != "" +} + // handleGitStatusMulti returns one git status entry per repo for multi-repo // task workspaces (or one untagged entry for single-repo). Used by the // session-subscribe handler in the main backend to seed per-repo state on @@ -1264,7 +1306,7 @@ type MultiRepoGitStatusResult struct { // status and run a fresh git query — used on WS subscribe so a new observer // always validates the cache against the live worktree. func (s *Server) handleGitStatusMulti(c *gin.Context) { - subpaths := s.procMgr.RepoSubpaths() + subpaths := s.procMgr.RepositoryScopes() // Single-repo: fall back to the workspace-root status with an empty repo // name so the response shape stays uniform. if len(subpaths) == 0 { @@ -1316,6 +1358,7 @@ func (s *Server) collectStatusForRepo(ctx context.Context, sub string, fresh boo RepositoryName: sub, Status: GitStatusResult{ Success: true, + IsSubmodule: status.IsSubmodule, Branch: status.Branch, RemoteBranch: status.RemoteBranch, HeadCommit: status.HeadCommit, @@ -1374,6 +1417,7 @@ func (s *Server) handleGitStatus(c *gin.Context) { c.JSON(http.StatusOK, GitStatusResult{ Success: true, + IsSubmodule: status.IsSubmodule, Branch: status.Branch, RemoteBranch: status.RemoteBranch, HeadCommit: status.HeadCommit, diff --git a/apps/backend/internal/agentctl/server/api/git_multi_repo_review_test.go b/apps/backend/internal/agentctl/server/api/git_multi_repo_review_test.go index f8663808e1..e042441525 100644 --- a/apps/backend/internal/agentctl/server/api/git_multi_repo_review_test.go +++ b/apps/backend/internal/agentctl/server/api/git_multi_repo_review_test.go @@ -1,6 +1,7 @@ package api import ( + "context" "encoding/json" "net/http" "net/http/httptest" @@ -44,6 +45,7 @@ func TestMultiRepoReviewEndpointsUseStoredBaseBranches(t *testing.T) { log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error"}) cfg := &config.InstanceConfig{WorkDir: taskRoot, BaseBranches: bases} mgr := process.NewManager(cfg, log) + t.Cleanup(func() { _ = mgr.StopForTeardown(context.Background()) }) srv := NewServer(cfg, mgr, nil, nil, log) logResponse := httptest.NewRecorder() @@ -143,6 +145,108 @@ func TestMultiRepoReviewEndpointsUseStoredBaseBranches(t *testing.T) { } } +func TestNestedSubmoduleReviewEndpointsIncludeRootAndStableChildBase(t *testing.T) { + parent, parentCleanup := setupAPITestRepo(t) + t.Cleanup(parentCleanup) + child, childCleanup := setupAPITestRepo(t) + t.Cleanup(childCleanup) + + runGitAPI(t, parent, "-c", "protocol.file.allow=always", "submodule", "add", child, "vendor/lib") + runGitAPI(t, parent, "add", ".") + runGitAPI(t, parent, "commit", "-m", "add submodule") + runGitAPI(t, parent, "push", "origin", "main") + runGitAPI(t, parent, "checkout", "-b", "feature/review") + + childBase := strings.TrimSpace(runGitAPI(t, parent, "rev-parse", "origin/main:vendor/lib")) + writeFileAPI(t, parent, "README.md", "root change\n") + childDir := filepath.Join(parent, "vendor/lib") + runGitAPI(t, childDir, "config", "user.email", "test@test.com") + runGitAPI(t, childDir, "config", "user.name", "Test User") + writeFileAPI(t, childDir, "child-change.txt", "child change\n") + runGitAPI(t, childDir, "add", ".") + runGitAPI(t, childDir, "commit", "-m", "child change") + writeFileAPI(t, childDir, "child-uncommitted.txt", "uncommitted child change\n") + + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error"}) + cfg := &config.InstanceConfig{ + WorkDir: parent, + BaseBranches: map[string]string{"": "main"}, + } + mgr := process.NewManager(cfg, log) + t.Cleanup(func() { _ = mgr.StopForTeardown(context.Background()) }) + srv := NewServer(cfg, mgr, nil, nil, log) + + statusResponse := httptest.NewRecorder() + srv.Router().ServeHTTP(statusResponse, httptest.NewRequest( + http.MethodGet, "/api/v1/git/status/multi?fresh=true", nil, + )) + if statusResponse.Code != http.StatusOK { + t.Fatalf("git status response = %d: %s", statusResponse.Code, statusResponse.Body.String()) + } + var status MultiRepoGitStatusResult + if err := json.Unmarshal(statusResponse.Body.Bytes(), &status); err != nil { + t.Fatalf("decode git status: %v", err) + } + statusByRepo := make(map[string]GitStatusResult, len(status.Repos)) + for _, repo := range status.Repos { + statusByRepo[repo.RepositoryName] = repo.Status + } + if _, ok := statusByRepo[""]; !ok { + t.Fatalf("root status missing: %s", statusResponse.Body.String()) + } + childStatus, ok := statusByRepo["vendor/lib"] + if !ok { + t.Fatalf("child repo status missing entirely: %s", statusResponse.Body.String()) + } + if !childStatus.IsSubmodule { + t.Fatalf("child repo is_submodule was false: %s", statusResponse.Body.String()) + } + if !containsString(childStatus.Modified, "child-uncommitted.txt") && + !containsString(childStatus.Untracked, "child-uncommitted.txt") { + t.Fatalf("child-uncommitted.txt not in Modified or Untracked: %s", statusResponse.Body.String()) + } + + diffResponse := httptest.NewRecorder() + srv.Router().ServeHTTP(diffResponse, httptest.NewRequest( + http.MethodGet, "/api/v1/git/cumulative-diff?base="+childBase, nil, + )) + if diffResponse.Code != http.StatusOK { + t.Fatalf("cumulative diff status = %d: %s", diffResponse.Code, diffResponse.Body.String()) + } + var diff process.CumulativeDiffResult + if err := json.Unmarshal(diffResponse.Body.Bytes(), &diff); err != nil { + t.Fatalf("decode cumulative diff: %v", err) + } + childPayload, ok := diff.Files["vendor/lib\x00child-change.txt"] + if !ok { + t.Fatalf("child cumulative diff missing: %s", diffResponse.Body.String()) + } + childFile, ok := childPayload.(map[string]interface{}) + if !ok || childFile["repository_name"] != "vendor/lib" || childFile["path"] != "child-change.txt" || childFile["base_ref"] != childBase || childFile["is_submodule"] != true { + t.Fatalf("child cumulative payload = %#v, want scoped path and base", childPayload) + } + if _, ok := diff.Files["\x00README.md"]; !ok { + t.Fatalf("root cumulative diff missing: %s", diffResponse.Body.String()) + } + + contentResponse := httptest.NewRecorder() + srv.Router().ServeHTTP(contentResponse, httptest.NewRequest( + http.MethodGet, "/api/v1/workspace/file/content-at-ref?path=README.md&ref=HEAD", nil, + )) + if contentResponse.Code != http.StatusOK { + t.Fatalf("root file content status = %d: %s", contentResponse.Code, contentResponse.Body.String()) + } +} + +func containsString(values []string, want string) bool { + for _, value := range values { + if value == want { + return true + } + } + return false +} + func TestMultiRepoReviewEndpointsCorrectStaleBases(t *testing.T) { taskRoot := t.TempDir() bases := map[string]string{ diff --git a/apps/backend/internal/agentctl/server/api/workspace.go b/apps/backend/internal/agentctl/server/api/workspace.go index de766736bf..c273f9a1f9 100644 --- a/apps/backend/internal/agentctl/server/api/workspace.go +++ b/apps/backend/internal/agentctl/server/api/workspace.go @@ -158,7 +158,7 @@ func (s *Server) handleFileContentAtRef(c *gin.Context) { } repo := c.Query("repo") - if repo == "" && len(s.procMgr.RepoSubpaths()) > 0 { + if repo == "" && requiresExplicitRepositoryScope(s.procMgr.RepositoryScopes()) { c.JSON(400, types.FileContentResponse{Path: path, Error: "repo is required for multi-repo workspace"}) return } diff --git a/apps/backend/internal/agentctl/server/process/manager.go b/apps/backend/internal/agentctl/server/process/manager.go index 95f4594cb0..bdd8bdc8c5 100644 --- a/apps/backend/internal/agentctl/server/process/manager.go +++ b/apps/backend/internal/agentctl/server/process/manager.go @@ -329,30 +329,17 @@ func NewManager(cfg *config.InstanceConfig, log *logger.Logger) *Manager { lifetimeCancel: lifetimeCancel, workspaceSourceRoots: canonicalWorkspaceSourceRoots(cfg.WorkspaceSourceRoots), } - // Multi-repo task roots hold one git worktree per repository as siblings. - // In that case build a per-repo tracker for each child so each emits its - // own GitStatusUpdate (tagged with RepositoryName) and the changes panel - // can show all repos. The root tracker covers the single-repo case via - // preferGitRepoChildIfRootIsBare; we skip its fallback when we've already - // detected a multi-repo root to avoid double-tracking the first repo. - repoChildren := scanRepositorySubdirs(cfg.WorkDir, m.workspaceSourceRoots) - if len(repoChildren) >= 2 { - // Multi-repo: root tracker bound to the bare task root (no fallback, - // no events), plus one tracker per repo subdir. - m.workspaceTracker = NewWorkspaceTrackerForRepo(cfg.WorkDir, "", log) - m.workspaceTracker.SetBaseBranch(lookupBaseBranch(cfg.BaseBranches, "")) - m.workspaceTracker.SetAllowedSourceRoots(cfg.WorkspaceSourceRoots) - for _, child := range repoChildren { - tr := NewWorkspaceTrackerForRepo(child.path, child.name, log) - tr.SetBaseBranch(lookupBaseBranch(cfg.BaseBranches, child.name)) - tr.SetAllowedSourceRoots(cfg.WorkspaceSourceRoots) - m.repoTrackers = append(m.repoTrackers, tr) - } - } else { - m.workspaceTracker = NewWorkspaceTracker(cfg.WorkDir, log) - m.workspaceTracker.SetBaseBranch(lookupBaseBranch(cfg.BaseBranches, "")) - m.workspaceTracker.SetAllowedSourceRoots(cfg.WorkspaceSourceRoots) - } + // Build the root plus any immediate sibling repositories and recursively + // declared initialized submodules. The root remains a real empty-named + // scope when it is itself a Git repository; a bare task root keeps the + // existing multi-repository behavior. + m.workspaceTracker, m.repoTrackers = m.buildWorkspaceTrackerGraph( + lifetimeCtx, + cfg.WorkDir, + m.workspaceSourceRoots, + false, + false, + ) m.processRunner = NewProcessRunner(m.workspaceTracker, log, cfg.ProcessBufferMaxBytes) m.shellMgr = shell.NewManager(cfg.WorkDir, log) m.status.Store(StatusStopped) @@ -642,6 +629,9 @@ func (m *Manager) GetWorkspaceTrackerFor(subpath string) (*WorkspaceTracker, err if cleaned == "" { return m.workspaceTracker, nil } + if tracker := m.findRepositoryTracker(cleaned); tracker != nil { + return tracker, nil + } m.workspaceTrackersMu.Lock() defer m.workspaceTrackersMu.Unlock() @@ -691,13 +681,13 @@ func (m *Manager) UpdateBaseBranches(ctx context.Context, branches map[string]st // caller's ctx so an HTTP request cancel after the field stores // can't strand half the trackers without their refresh. if root != nil { - root.SetBaseBranch(lookupBaseBranch(branches, root.RepositoryName())) + root.SetBaseBranchIfNotSubmodule(lookupBaseBranch(branches, root.RepositoryName())) } for _, t := range trackers { - t.SetBaseBranch(lookupBaseBranch(branches, t.RepositoryName())) + t.SetBaseBranchIfNotSubmodule(lookupBaseBranch(branches, t.RepositoryName())) } for subpath, t := range bySubpath { - t.SetBaseBranch(lookupBaseBranch(branches, subpath)) + t.SetBaseBranchIfNotSubmodule(lookupBaseBranch(branches, subpath)) } go m.refreshTrackersDetached(root, trackers, bySubpath) } @@ -789,9 +779,43 @@ func (m *Manager) RepoSubpaths() []string { out = append(out, t.repositoryName) } } + sort.Strings(out) return out } +// RepositoryScopes returns the stable scope order used by unpinned Git API +// fan-out. The empty scope is included only when the workspace root tracker +// owns a real Git repository; a bare multi-repository task root therefore +// retains its legacy named-only response. +func (m *Manager) RepositoryScopes() []string { + root, trackers := m.snapshotTrackers() + scopes := make([]string, 0, len(trackers)+1) + if root != nil && root.gitIndexPath != "" { + scopes = append(scopes, "") + } + for _, tracker := range trackers { + if tracker != nil && tracker.repositoryName != "" { + scopes = append(scopes, tracker.repositoryName) + } + } + if root != nil && root.gitIndexPath != "" { + sort.Strings(scopes[1:]) + } else { + sort.Strings(scopes) + } + return scopes +} + +func (m *Manager) findRepositoryTracker(repositoryName string) *WorkspaceTracker { + _, trackers := m.snapshotTrackers() + for _, tracker := range trackers { + if tracker != nil && tracker.repositoryName == repositoryName { + return tracker + } + } + return nil +} + // newTrackerForRepo builds a per-repo tracker that inherits the workspace's // current poll mode. Used for every tracker created after launch — rescan and // lazy subpath creation — because the gateway only pushes on a session mode @@ -925,7 +949,11 @@ func (m *Manager) GitOperatorFor(subpath string) (*GitOperator, error) { if op, ok := m.gitOperatorsBySubpath[cleaned]; ok { return op, nil } - op := NewGitOperatorForRepo(full, cleaned, m.logger, m.workspaceTracker) + tracker := m.findRepositoryTracker(cleaned) + if tracker == nil { + tracker = m.GetWorkspaceTracker() + } + op := NewGitOperatorForRepo(full, cleaned, m.logger, tracker) op.setEnvironmentProvider(m.gitEnvironment) if binding, ok := m.cfg.RemoteContributions[cleaned]; ok { op.setRemoteContribution(&binding) @@ -965,7 +993,13 @@ func (m *Manager) resolveSubpath(subpath string) (string, string, error) { } } - full := filepath.Join(m.cfg.WorkDir, cleaned) + // Repository scope names are also used as API keys and Git paths. Keep + // those keys slash-separated on every platform; filepath.Clean uses the + // native separator, which made Windows lookups miss the trackers built + // from Git's slash-separated gitlink paths and create an unanchored lazy + // tracker instead. + scope := filepath.ToSlash(cleaned) + full := filepath.Join(m.cfg.WorkDir, filepath.FromSlash(scope)) info, err := os.Stat(full) if err != nil { return "", "", fmt.Errorf("repo subpath not found: %w", err) @@ -973,7 +1007,7 @@ func (m *Manager) resolveSubpath(subpath string) (string, string, error) { if !info.IsDir() { return "", "", fmt.Errorf("repo subpath is not a directory: %q", subpath) } - return cleaned, full, nil + return scope, full, nil } // WorkDir returns the absolute workspace root for this agentctl instance. diff --git a/apps/backend/internal/agentctl/server/process/manager_rescan.go b/apps/backend/internal/agentctl/server/process/manager_rescan.go index db042930c0..0e7a72b08e 100644 --- a/apps/backend/internal/agentctl/server/process/manager_rescan.go +++ b/apps/backend/internal/agentctl/server/process/manager_rescan.go @@ -63,59 +63,18 @@ func (m *Manager) RescanRepositoriesWithSourceRoots(ctx context.Context, newWork return err } - proposedRoots := roots != nil if roots == nil { roots = m.currentWorkspaceSourceRoots() } else { roots = canonicalWorkspaceSourceRoots(roots) } - - // Snapshot existing trackers while rescanMu serializes the graph mutation. - // cfg.WorkDir and roots are committed only with the resulting graph below. - m.repoTrackersMu.RLock() - workDir := candidate - existingTrackers := len(m.repoTrackers) - trackers := append([]*WorkspaceTracker{m.workspaceTracker}, m.repoTrackers...) - m.repoTrackersMu.RUnlock() - for _, tracker := range trackers { - if tracker != nil { - tracker.SetAllowedSourceRoots(roots) - } - } - - children := scanRepositorySubdirs(workDir, roots) - subs := m.snapshotSubscribers() - - m.logger.Info("workspace rescan started", - zap.String("work_dir", workDir), - zap.Int("children_found", len(children)), - zap.Int("existing_repo_trackers", existingTrackers), - zap.Int("subscribers", len(subs))) - - if candidate == "" { - m.commitRescanWorkspaceState(workDir, roots) - return nil - } - // A promoted root must always replace the old single-repo file tracker, - // even when it contains just one git repository plus plain linked folders. - if len(children) < 2 && !scopeChanged && newWorkDir != "" { - // Nothing to do: a non-multi-repo workspace stays on its single - // tracker. The legacy preferGitRepoChildIfRootIsBare fallback - // covers single-repo construct-time setup. - m.commitRescanWorkspaceState(workDir, roots) - return nil - } - - if existingTrackers == 0 && scopeChanged { - m.transitionToMultiRepoMode(ctx, workDir, children, roots, subs) - return nil - } - if proposedRoots { - m.reconcileRepoTrackers(ctx, workDir, children, roots, subs) - return nil - } - m.appendNewRepoTrackers(ctx, workDir, children, roots, subs) - return nil + // A remote workspace may launch with a plain task root and no repository, + // then receive its first repository after launch. Preserve that root as the + // file-tree scope when it is rescanned; otherwise NewWorkspaceTracker's + // single-repo compatibility fallback would replace it with the new child + // and hide files that remain at the task root. + forceBareRoot := scopeChanged || m.hasBareMultiRepoTrackerGraph() || m.hasBareTaskRootTracker(candidate) + return m.reconcileWorkspaceTrackerGraph(ctx, candidate, roots, forceBareRoot) } // rescanCandidateWorkDir resolves and validates a proposed tracking root @@ -179,10 +138,7 @@ func (m *Manager) ReconcileRepositories(ctx context.Context) error { } roots := m.currentWorkspaceSourceRoots() - children := scanRepositorySubdirs(workDir, roots) - subs := m.snapshotSubscribers() - m.reconcileRepoTrackers(ctx, workDir, children, roots, subs) - return nil + return m.reconcileWorkspaceTrackerGraph(ctx, workDir, roots, true) } // RebindWorkspace replaces the complete workspace tracker graph after the @@ -220,46 +176,7 @@ func (m *Manager) RebindWorkspaceWithSourceRoots(ctx context.Context, workDir st } else { roots = canonicalWorkspaceSourceRoots(roots) } - subs := m.snapshotSubscribers() - children := scanRepositorySubdirs(resolved, roots) - bare := m.newTrackerForRepo(resolved, "") - bare.SetBaseBranch(lookupBaseBranch(m.getBaseBranches(), "")) - bare.SetAllowedSourceRoots(roots) - bare.Start(ctx) - for _, sub := range subs { - bare.AttachWorkspaceStreamSubscriber(sub) - } - repos := make([]*WorkspaceTracker, 0, len(children)) - for _, child := range children { - tracker := m.newTrackerForRepo(child.path, child.name) - tracker.SetBaseBranch(lookupBaseBranch(m.getBaseBranches(), child.name)) - tracker.SetAllowedSourceRoots(roots) - tracker.Start(ctx) - for _, sub := range subs { - tracker.AttachWorkspaceStreamSubscriber(sub) - } - repos = append(repos, tracker) - } - - m.repoTrackersMu.Lock() - oldBare, oldRepos := m.workspaceTracker, m.repoTrackers - m.cfg.WorkDir, m.workspaceTracker, m.repoTrackers, m.workspaceSourceRoots = resolved, bare, repos, append([]string(nil), roots...) - m.applyWorkspacePollModeLocked(append([]*WorkspaceTracker{bare}, repos...)...) - m.cfg.WorkspaceSourceRoots = append([]string(nil), roots...) - m.repoTrackersMu.Unlock() - if oldBare != nil { - for _, sub := range subs { - oldBare.DetachWorkspaceStreamSubscriber(sub) - } - oldBare.Stop() - } - for _, tracker := range oldRepos { - for _, sub := range subs { - tracker.DetachWorkspaceStreamSubscriber(sub) - } - tracker.Stop() - } - return nil + return m.replaceWorkspaceTrackerGraph(ctx, resolved, roots) } // transitionToMultiRepoMode replaces the single-repo workspaceTracker with a diff --git a/apps/backend/internal/agentctl/server/process/manager_rescan_test.go b/apps/backend/internal/agentctl/server/process/manager_rescan_test.go index 3bda914f0f..0c44c08e15 100644 --- a/apps/backend/internal/agentctl/server/process/manager_rescan_test.go +++ b/apps/backend/internal/agentctl/server/process/manager_rescan_test.go @@ -426,6 +426,44 @@ func TestRescanRepositories_PromotedRootTracksPlainFolderWithOneRepository(t *te } } +// A remote workspace can start as a plain task root and receive its first +// repository after the agent has launched. The root must remain the file-tree +// scope so files already present at the task root and the attached repository +// directory are visible together. +func TestRescanRepositories_PromotesBareTaskRootWhenFirstRepositoryAppears(t *testing.T) { + taskRoot := t.TempDir() + if err := os.WriteFile(filepath.Join(taskRoot, "remote-source.txt"), []byte("source\n"), 0o644); err != nil { + t.Fatal(err) + } + + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + m := NewManager(&config.InstanceConfig{WorkDir: taskRoot}, log) + t.Cleanup(m.stopWorkspaceTrackers) + + repository := filepath.Join(taskRoot, "attached-repository") + initGitRepoAt(t, repository) + + if err := m.RescanRepositories(context.Background(), ""); err != nil { + t.Fatalf("rescan after first repository appears: %v", err) + } + + if m.workspaceTracker.workDir != taskRoot { + t.Fatalf("workspace tracker path = %q, want task root %q", m.workspaceTracker.workDir, taskRoot) + } + if len(m.repoTrackers) != 1 || m.repoTrackers[0].RepositoryName() != "attached-repository" { + t.Fatalf("repository trackers = %+v, want attached-repository", m.repoTrackers) + } + + tree, err := m.workspaceTracker.GetFileTree("", 1) + if err != nil { + t.Fatalf("GetFileTree: %v", err) + } + names := fileTreeChildNames(tree) + if !names["remote-source.txt"] || !names["attached-repository"] { + t.Fatalf("task-root file scope = %v, want root file and repository directory", names) + } +} + // Source-root policy is installed after a successful API rescan. It must // therefore be applied to the bare root tracker that replaces the original // primary tracker as well as every newly-created child tracker. diff --git a/apps/backend/internal/agentctl/server/process/manager_submodule_test.go b/apps/backend/internal/agentctl/server/process/manager_submodule_test.go new file mode 100644 index 0000000000..f217e4c399 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/manager_submodule_test.go @@ -0,0 +1,144 @@ +package process + +import ( + "context" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/kandev/kandev/internal/agentctl/server/config" +) + +func TestManager_DiscoversNestedSubmoduleScopesWithStableAnchors(t *testing.T) { + parent, parentCleanup := setupTestRepo(t) + t.Cleanup(parentCleanup) + direct, directCleanup := setupTestRepo(t) + t.Cleanup(directCleanup) + nested, nestedCleanup := setupTestRepo(t) + t.Cleanup(nestedCleanup) + + runGit(t, direct, "-c", "protocol.file.allow=always", "submodule", "add", nested, "vendor/inner") + runGit(t, direct, "add", ".") + runGit(t, direct, "commit", "-m", "add nested submodule") + + runGit(t, parent, "-c", "protocol.file.allow=always", "submodule", "add", direct, "vendor/outer") + runGit(t, parent, "-c", "protocol.file.allow=always", "submodule", "update", "--init", "--recursive") + runGit(t, parent, "add", ".") + runGit(t, parent, "commit", "-m", "add direct submodule") + runGit(t, parent, "push", "origin", "main") + runGit(t, parent, "checkout", "-b", "feature/review") + + directAnchor := strings.TrimSpace(runGit(t, parent, "rev-parse", "origin/main:vendor/outer")) + nestedAnchor := strings.TrimSpace(runGit(t, filepath.Join(parent, "vendor/outer"), "rev-parse", "HEAD:vendor/inner")) + + mgr := NewManager(&config.InstanceConfig{ + WorkDir: parent, + BaseBranches: map[string]string{"": "main"}, + }, newTestLogger(t)) + t.Cleanup(mgr.stopWorkspaceTrackers) + + if mgr.GetWorkspaceTracker().RepositoryName() != "" { + t.Fatalf("root tracker RepositoryName() = %q, want empty root scope", mgr.GetWorkspaceTracker().RepositoryName()) + } + if got := mgr.RepoSubpaths(); !slices.Equal(got, []string{"vendor/outer", "vendor/outer/vendor/inner"}) { + t.Fatalf("RepoSubpaths() = %v, want nested scopes", got) + } + if got := mgr.RepositoryScopes(); !slices.Equal(got, []string{"", "vendor/outer", "vendor/outer/vendor/inner"}) { + t.Fatalf("RepositoryScopes() = %v, want root and nested scopes", got) + } + + directTracker, err := mgr.GetWorkspaceTrackerFor("vendor/outer") + if err != nil { + t.Fatalf("direct tracker: %v", err) + } + if got := directTracker.BaseBranch(); got != directAnchor { + t.Errorf("direct BaseBranch() = %q, want parent gitlink %q", got, directAnchor) + } + + nestedTracker, err := mgr.GetWorkspaceTrackerFor("vendor/outer/vendor/inner") + if err != nil { + t.Fatalf("nested tracker: %v", err) + } + if got := nestedTracker.BaseBranch(); got != nestedAnchor { + t.Errorf("nested BaseBranch() = %q, want parent gitlink %q", got, nestedAnchor) + } + + if _, err := mgr.GetWorkspaceTrackerFor("vendor/outer"); err != nil { + t.Fatalf("repeated direct tracker lookup: %v", err) + } + +} + +func TestManager_RescanDiscoversNewNestedSubmoduleAndRetainsRoot(t *testing.T) { + parent, parentCleanup := setupTestRepo(t) + t.Cleanup(parentCleanup) + child, childCleanup := setupTestRepo(t) + t.Cleanup(childCleanup) + + mgr := NewManager(&config.InstanceConfig{WorkDir: parent}, newTestLogger(t)) + t.Cleanup(mgr.stopWorkspaceTrackers) + root := mgr.GetWorkspaceTracker() + if got := mgr.RepoSubpaths(); len(got) != 0 { + t.Fatalf("initial RepoSubpaths() = %v, want none", got) + } + + runGit(t, parent, "-c", "protocol.file.allow=always", "submodule", "add", child, "vendor/lib") + runGit(t, parent, "add", ".") + runGit(t, parent, "commit", "-m", "add submodule") + + if err := mgr.RescanRepositories(context.Background(), parent); err != nil { + t.Fatalf("rescan: %v", err) + } + if mgr.GetWorkspaceTracker() != root { + t.Fatal("rescan replaced the root tracker for an unchanged workspace") + } + if got := mgr.RepoSubpaths(); !slices.Equal(got, []string{"vendor/lib"}) { + t.Fatalf("rescanned RepoSubpaths() = %v, want vendor/lib", got) + } + if tracker, err := mgr.GetWorkspaceTrackerFor("vendor/lib"); err != nil || tracker == nil { + t.Fatalf("rescanned child tracker = %v, err %v", tracker, err) + } +} + +func TestManager_SkipsUninitializedSubmoduleWithoutLosingRoot(t *testing.T) { + parent, parentCleanup := setupTestRepo(t) + t.Cleanup(parentCleanup) + child, childCleanup := setupTestRepo(t) + t.Cleanup(childCleanup) + + runGit(t, parent, "-c", "protocol.file.allow=always", "submodule", "add", child, "vendor/lib") + runGit(t, parent, "add", ".") + runGit(t, parent, "commit", "-m", "add submodule") + if err := os.RemoveAll(filepath.Join(parent, "vendor/lib")); err != nil { + t.Fatalf("remove submodule worktree: %v", err) + } + + mgr := NewManager(&config.InstanceConfig{WorkDir: parent}, newTestLogger(t)) + t.Cleanup(mgr.stopWorkspaceTrackers) + if got := mgr.RepoSubpaths(); len(got) != 0 { + t.Fatalf("uninitialized RepoSubpaths() = %v, want none", got) + } + if mgr.GetWorkspaceTracker() == nil { + t.Fatal("root tracker was lost when submodule was unavailable") + } +} + +func TestManager_SubmoduleDiscoveryStopsAtDepthLimit(t *testing.T) { + parent, parentCleanup := setupTestRepo(t) + t.Cleanup(parentCleanup) + mgr := NewManager(&config.InstanceConfig{WorkDir: parent}, newTestLogger(t)) + t.Cleanup(mgr.stopWorkspaceTrackers) + + got := mgr.discoverSubmoduleTrackers( + context.Background(), + mgr.GetWorkspaceTracker(), + nil, + parent, + maxSubmoduleDiscoveryDepth, + ) + if got != nil { + t.Fatalf("depth-limited discovery returned %v trackers, want none", len(got)) + } +} diff --git a/apps/backend/internal/agentctl/server/process/manager_submodules.go b/apps/backend/internal/agentctl/server/process/manager_submodules.go new file mode 100644 index 0000000000..638ee0a5f4 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/manager_submodules.go @@ -0,0 +1,454 @@ +package process + +import ( + "context" + "fmt" + "os" + "os/exec" + "path" + "path/filepath" + "sort" + "strings" + "time" + + "github.com/kandev/kandev/internal/agentctl/types" + "github.com/kandev/kandev/internal/common/subproc" + "go.uber.org/zap" +) + +const submoduleDiscoveryTimeout = 10 * time.Second +const maxSubmoduleDiscoveryDepth = 32 + +func (m *Manager) hasBareMultiRepoTrackerGraph() bool { + root, trackers := m.snapshotTrackers() + return root != nil && root.repositoryName == "" && root.gitIndexPath == "" && len(trackers) > 0 +} + +// hasBareTaskRootTracker reports whether the active root tracker is already a +// plain task-root scope with no per-repository children. A rescan must keep +// this scope when a repository appears beneath it so the file tree continues +// to expose both task-root files and the attached repository directory. +func (m *Manager) hasBareTaskRootTracker(workDir string) bool { + root, trackers := m.snapshotTrackers() + return root != nil && len(trackers) == 0 && root.repositoryName == "" && + root.gitIndexPath == "" && filepath.Clean(root.workDir) == filepath.Clean(workDir) +} + +// buildWorkspaceTrackerGraph preserves the existing bare multi-repository +// shape while adding initialized submodules below each real repository. A +// single sibling repository remains the unnamed root for compatibility with +// existing task workspaces. +func (m *Manager) buildWorkspaceTrackerGraph( + ctx context.Context, + workDir string, + roots []string, + includeAttachedRepos bool, + forceBareRoot bool, +) (*WorkspaceTracker, []*WorkspaceTracker) { + directRepos := scanRepositorySubdirs(workDir, roots) + rootIsGit := resolveGitIndexPath(workDir) != "" + + if !rootIsGit && (len(directRepos) >= 2 || forceBareRoot) { + root := NewWorkspaceTrackerForRepo(workDir, "", m.logger) + m.configureTracker(root, "", roots) + + trackers := make([]*WorkspaceTracker, 0, len(directRepos)) + for _, repo := range directRepos { + trackers = append(trackers, m.buildRepositoryScopeTrackers(ctx, repo, roots, workDir)...) + } + return root, trackers + } + + root := NewWorkspaceTracker(workDir, m.logger) + m.configureTracker(root, "", roots) + trackers := m.discoverSubmoduleTrackers(ctx, root, roots, workDir, 0) + if rootIsGit && includeAttachedRepos { + known := make(map[string]struct{}, len(trackers)) + for _, tracker := range trackers { + known[tracker.RepositoryName()] = struct{}{} + } + for _, repo := range directRepos { + if _, alreadyDiscovered := known[repo.name]; alreadyDiscovered { + continue + } + trackers = append(trackers, m.buildRepositoryScopeTrackers(ctx, repo, roots, workDir)...) + } + } + return root, trackers +} + +func (m *Manager) configureTracker(tracker *WorkspaceTracker, repositoryName string, roots []string) { + tracker.SetAllowedSourceRoots(roots) + if !tracker.IsSubmodule() { + tracker.SetBaseBranch(lookupBaseBranch(m.getBaseBranches(), repositoryName)) + } +} + +func (m *Manager) buildRepositoryScopeTrackers( + ctx context.Context, + repo repositorySubdir, + roots []string, + workspaceRoot string, +) []*WorkspaceTracker { + tracker := m.newTrackerForRepo(repo.path, repo.name) + m.configureTracker(tracker, repo.name, roots) + trackers := []*WorkspaceTracker{tracker} + return append(trackers, m.discoverSubmoduleTrackers(ctx, tracker, roots, workspaceRoot, 0)...) +} + +// reconcileWorkspaceTrackerGraph keeps existing tracker instances when their +// scope and path are unchanged, while adding or removing submodule trackers +// discovered by a workspace rescan. New trackers inherit subscribers and the +// current polling mode just like the existing sibling-repository rescan path. +func (m *Manager) reconcileWorkspaceTrackerGraph( + ctx context.Context, + workDir string, + roots []string, + forceBareRoot bool, +) error { + desiredRoot, desiredTrackers := m.buildWorkspaceTrackerGraph(ctx, workDir, roots, true, forceBareRoot) + subs := m.snapshotSubscribers() + oldRoot, oldTrackers := m.snapshotTrackers() + + oldByIdentity := make(map[repositoryTrackerKey]*WorkspaceTracker, len(oldTrackers)) + for _, tracker := range oldTrackers { + oldByIdentity[repositoryTrackerIdentity(tracker.RepositoryName(), tracker.workDir)] = tracker + } + + root := desiredRoot + rootReused := false + if oldRoot != nil && repositoryTrackerIdentity(oldRoot.RepositoryName(), oldRoot.workDir) == + repositoryTrackerIdentity(desiredRoot.RepositoryName(), desiredRoot.workDir) { + root = oldRoot + rootReused = true + syncTrackerConfiguration(root, desiredRoot, roots) + desiredRoot.Stop() + } + if !rootReused { + startAndAttachTracker(ctx, root, subs) + } + + retained := make([]*WorkspaceTracker, 0, len(desiredTrackers)) + used := make(map[*WorkspaceTracker]struct{}, len(desiredTrackers)) + newTrackers := make([]*WorkspaceTracker, 0) + for _, desired := range desiredTrackers { + identity := repositoryTrackerIdentity(desired.RepositoryName(), desired.workDir) + if existing := oldByIdentity[identity]; existing != nil { + syncTrackerConfiguration(existing, desired, roots) + desired.Stop() + retained = append(retained, existing) + used[existing] = struct{}{} + continue + } + startAndAttachTracker(ctx, desired, subs) + retained = append(retained, desired) + newTrackers = append(newTrackers, desired) + } + + m.repoTrackersMu.Lock() + m.workspaceTracker = root + m.repoTrackers = retained + m.cfg.WorkDir = workDir + m.workspaceSourceRoots = append([]string(nil), roots...) + m.cfg.WorkspaceSourceRoots = append([]string(nil), roots...) + m.applyWorkspacePollModeLocked(newTrackers...) + m.repoTrackersMu.Unlock() + if m.processRunner != nil { + m.processRunner.setWorkspaceTracker(root) + } + + if oldRoot != nil && oldRoot != root { + for _, sub := range subs { + oldRoot.DetachWorkspaceStreamSubscriber(sub) + } + oldRoot.Stop() + } + for _, old := range oldTrackers { + if _, keep := used[old]; keep { + continue + } + for _, sub := range subs { + old.DetachWorkspaceStreamSubscriber(sub) + } + old.Stop() + } + return nil +} + +func (m *Manager) replaceWorkspaceTrackerGraph( + ctx context.Context, + workDir string, + roots []string, +) error { + desiredRoot, desiredTrackers := m.buildWorkspaceTrackerGraph(ctx, workDir, roots, true, true) + subs := m.snapshotSubscribers() + startAndAttachTracker(ctx, desiredRoot, subs) + for _, tracker := range desiredTrackers { + startAndAttachTracker(ctx, tracker, subs) + } + + oldRoot, oldTrackers := m.snapshotTrackers() + m.repoTrackersMu.Lock() + m.workspaceTracker = desiredRoot + m.repoTrackers = desiredTrackers + m.cfg.WorkDir = workDir + m.workspaceSourceRoots = append([]string(nil), roots...) + m.cfg.WorkspaceSourceRoots = append([]string(nil), roots...) + m.applyWorkspacePollModeLocked(append([]*WorkspaceTracker{desiredRoot}, desiredTrackers...)...) + m.repoTrackersMu.Unlock() + if m.processRunner != nil { + m.processRunner.setWorkspaceTracker(desiredRoot) + } + + if oldRoot != nil { + for _, sub := range subs { + oldRoot.DetachWorkspaceStreamSubscriber(sub) + } + oldRoot.Stop() + } + for _, tracker := range oldTrackers { + for _, sub := range subs { + tracker.DetachWorkspaceStreamSubscriber(sub) + } + tracker.Stop() + } + return nil +} + +func syncTrackerConfiguration(target, source *WorkspaceTracker, roots []string) { + target.SetAllowedSourceRoots(roots) + if source.IsSubmodule() { + target.SetComparisonAnchor(source.ComparisonAnchor()) + return + } + target.SetBaseBranch(source.BaseBranch()) +} + +func startAndAttachTracker(ctx context.Context, tracker *WorkspaceTracker, subs []types.WorkspaceStreamSubscriber) { + tracker.Start(ctx) + for _, sub := range subs { + tracker.AttachWorkspaceStreamSubscriber(sub) + } +} + +// discoverSubmoduleTrackers returns initialized, declared gitlink children in +// stable path order. A child is anchored to the gitlink SHA in the parent's +// comparison tree, not to the child's current/default branch. The recursion +// only follows gitlink paths from Git, never arbitrary nested directories. +func (m *Manager) discoverSubmoduleTrackers( + ctx context.Context, + parent *WorkspaceTracker, + roots []string, + workspaceRoot string, + depth int, +) []*WorkspaceTracker { + if depth >= maxSubmoduleDiscoveryDepth { + m.logger.Warn("submodule discovery depth limit reached", + zap.String("repository_name", parent.RepositoryName()), + zap.Int("depth", depth), + zap.Int("max_depth", maxSubmoduleDiscoveryDepth)) + return nil + } + paths, err := listGitlinkPaths(ctx, parent.workDir, "HEAD") + if err != nil { + m.logger.Warn("could not discover submodules", + zap.String("repository_name", parent.RepositoryName()), + zap.String("path", parent.workDir), + zap.Error(err)) + return nil + } + parentAnchor, _ := parent.ResolveBaseAnchor(ctx) + trackers := make([]*WorkspaceTracker, 0, len(paths)) + for _, relativePath := range paths { + childPath, ok := m.initializedSubmodulePath(workspaceRoot, parent.workDir, relativePath) + if !ok { + continue + } + + name := joinRepositoryScope(parent.RepositoryName(), relativePath) + child := m.newTrackerForRepo(childPath, name) + child.SetAllowedSourceRoots(roots) + anchor := "" + if parentAnchor != "" { + anchor, err = gitlinkCommitAt(ctx, parent.workDir, parentAnchor, relativePath) + if err != nil { + m.logger.Warn("could not resolve submodule comparison anchor", + zap.String("repository_name", name), + zap.String("parent_anchor", parentAnchor), + zap.Error(err)) + } + } + child.SetComparisonAnchor(anchor) + + trackers = append(trackers, child) + trackers = append(trackers, m.discoverSubmoduleTrackers(ctx, child, roots, workspaceRoot, depth+1)...) + } + return trackers +} + +// initializedSubmodulePath resolves a Git-declared child and verifies that it +// is an initialized repository inside the task workspace. Symlinked children +// are rejected so a changed worktree cannot redirect discovery outside the +// authenticated workspace root. +func (m *Manager) initializedSubmodulePath(workspaceRoot, parentPath, relativePath string) (string, bool) { + if !safeGitRelativePath(relativePath) { + m.logger.Warn("submodule discovery rejected unsafe path", zap.String("path", relativePath)) + return "", false + } + parentRoot, err := os.OpenRoot(parentPath) + if err != nil { + m.logger.Warn("submodule discovery could not open parent worktree", + zap.String("path", parentPath), zap.Error(err)) + return "", false + } + defer func() { _ = parentRoot.Close() }() + // Root.Lstat bounds the Git-declared relative path to the authenticated + // parent worktree before the absolute path is used by the tracker. + childInfo, err := parentRoot.Lstat(filepath.FromSlash(relativePath)) + childPath := filepath.Clean(filepath.Join(parentPath, filepath.FromSlash(relativePath))) + if err != nil || !childInfo.IsDir() || childInfo.Mode()&os.ModeSymlink != 0 { + m.logger.Warn("submodule discovery rejected child worktree", + zap.String("path", childPath), zap.Error(err)) + return "", false + } + childRoot, ok := m.openVerifiedSubmoduleRoot(parentRoot, childInfo, childPath, relativePath) + if !ok { + return "", false + } + defer func() { _ = childRoot.Close() }() + resolvedRoot, err := filepath.EvalSymlinks(workspaceRoot) + if err != nil { + m.logger.Warn("submodule discovery could not resolve workspace root", + zap.String("path", workspaceRoot), zap.Error(err)) + return "", false + } + resolvedChild, err := filepath.EvalSymlinks(childPath) + if err != nil { + m.logger.Warn("submodule discovery could not resolve child worktree", + zap.String("path", childPath), zap.Error(err)) + return "", false + } + relativeToRoot, err := filepath.Rel(resolvedRoot, resolvedChild) + if err != nil || pathEscapesRoot(relativeToRoot) { + m.logger.Warn("submodule discovery rejected child outside workspace root", + zap.String("path", childPath), zap.String("workspace_root", workspaceRoot), zap.Error(err)) + return "", false + } + if resolveGitIndexPath(childPath) == "" { + m.logger.Warn("submodule discovery rejected uninitialized child", + zap.String("path", childPath)) + return "", false + } + currentChildInfo, err := parentRoot.Lstat(filepath.FromSlash(relativePath)) + if err != nil || currentChildInfo.Mode()&os.ModeSymlink != 0 || + !os.SameFile(childInfo, currentChildInfo) { + m.logger.Warn("submodule discovery rejected changed child worktree", + zap.String("path", childPath), zap.Error(err)) + return "", false + } + return childPath, true +} + +func (m *Manager) openVerifiedSubmoduleRoot( + parentRoot *os.Root, + childInfo os.FileInfo, + childPath string, + relativePath string, +) (*os.Root, bool) { + childRoot, err := parentRoot.OpenRoot(filepath.FromSlash(relativePath)) + if err != nil { + m.logger.Warn("submodule discovery could not open child worktree", + zap.String("path", childPath), zap.Error(err)) + return nil, false + } + openedChildInfo, err := childRoot.Stat(".") + if err != nil || !os.SameFile(childInfo, openedChildInfo) { + _ = childRoot.Close() + m.logger.Warn("submodule discovery rejected replaced child worktree", + zap.String("path", childPath), zap.Error(err)) + return nil, false + } + if _, err := childRoot.Lstat(".git"); err != nil { + _ = childRoot.Close() + m.logger.Warn("submodule discovery rejected child without git entry", + zap.String("path", childPath), zap.Error(err)) + return nil, false + } + return childRoot, true +} + +func listGitlinkPaths(ctx context.Context, workDir, ref string) ([]string, error) { + output, err := runGitLifecycleOutput(ctx, workDir, "ls-tree", "-r", "-z", ref) + if err != nil { + return nil, err + } + paths := make([]string, 0) + for _, entry := range strings.Split(string(output), "\x00") { + if entry == "" { + continue + } + header, relativePath, ok := strings.Cut(entry, "\t") + if !ok || !strings.HasPrefix(header, "160000 ") || !safeGitRelativePath(relativePath) { + continue + } + paths = append(paths, relativePath) + } + sort.Strings(paths) + return paths, nil +} + +func gitlinkCommitAt(ctx context.Context, workDir, ref, relativePath string) (string, error) { + if !sha1HexPattern.MatchString(ref) || !safeGitRelativePath(relativePath) { + return "", fmt.Errorf("invalid gitlink lookup") + } + output, err := runGitLifecycleOutput(ctx, workDir, "ls-tree", "-z", ref, "--", relativePath) + if err != nil { + return "", err + } + for _, entry := range strings.Split(string(output), "\x00") { + header, entryPath, ok := strings.Cut(entry, "\t") + if !ok || entryPath != relativePath || !strings.HasPrefix(header, "160000 ") { + continue + } + fields := strings.Fields(header) + if len(fields) == 3 && sha1HexPattern.MatchString(fields[2]) { + return fields[2], nil + } + } + return "", fmt.Errorf("gitlink %q not found at %s", relativePath, ref) +} + +func runGitLifecycleOutput(ctx context.Context, workDir string, args ...string) ([]byte, error) { + output, runErr, execErr := subproc.RunGitOutputAfterAcquire( + ctx, + subproc.GitLifecycle, + submoduleDiscoveryTimeout, + func(execCtx context.Context) *exec.Cmd { + cmd := subproc.NewGitCommand(execCtx, args...) + cmd.Dir = workDir + return cmd + }, + ) + if execErr != nil { + return nil, execErr + } + if runErr != nil { + return nil, runErr + } + return output, nil +} + +func safeGitRelativePath(relativePath string) bool { + if relativePath == "" || strings.ContainsRune(relativePath, '\x00') || strings.Contains(relativePath, "\\") { + return false + } + clean := path.Clean(relativePath) + return clean == relativePath && clean != "." && clean != ".." && !strings.HasPrefix(clean, "../") && !path.IsAbs(clean) +} + +func joinRepositoryScope(parent, child string) string { + if parent == "" { + return child + } + return path.Join(parent, child) +} diff --git a/apps/backend/internal/agentctl/server/process/runner.go b/apps/backend/internal/agentctl/server/process/runner.go index 4238a01081..258614faf2 100644 --- a/apps/backend/internal/agentctl/server/process/runner.go +++ b/apps/backend/internal/agentctl/server/process/runner.go @@ -148,6 +148,11 @@ type commandProcess struct { mu sync.Mutex // Protects info fields during updates } +type workspaceStreamNotifier interface { + notifyWorkspaceStreamProcessOutput(*types.ProcessOutput) + notifyWorkspaceStreamProcessStatus(*types.ProcessStatusUpdate) +} + // ProcessRunner manages multiple background processes with output streaming. // // Thread-safe: All public methods can be called concurrently from multiple goroutines. @@ -169,9 +174,10 @@ type commandProcess struct { // - Stop() kills the entire process group (handles child processes correctly) // - This ensures clean cleanup even if the process spawns subprocesses type ProcessRunner struct { - logger *logger.Logger // Scoped logger for this component - workspaceTracker *WorkspaceTracker // WebSocket stream coordinator (can be nil) - bufferMaxBytes int64 // Default output buffer size for new processes + logger *logger.Logger // Scoped logger for this component + workspaceTracker workspaceStreamNotifier // WebSocket stream coordinator (can be nil) + workspaceTrackerMu sync.RWMutex // Protects workspaceTracker during tracker graph replacement + bufferMaxBytes int64 // Default output buffer size for new processes mu sync.RWMutex // Protects processes map processes map[string]*commandProcess // Active processes by ID @@ -200,14 +206,28 @@ func (r *ProcessRunner) BeginStop() { // - bufferMaxBytes: Default output buffer size (typically 2MB). Individual processes // can override via StartProcessRequest.BufferMaxBytes. func NewProcessRunner(workspaceTracker *WorkspaceTracker, log *logger.Logger, bufferMaxBytes int64) *ProcessRunner { + var notifier workspaceStreamNotifier + if workspaceTracker != nil { + notifier = workspaceTracker + } return &ProcessRunner{ logger: log.WithFields(zap.String("component", "process-runner")), - workspaceTracker: workspaceTracker, + workspaceTracker: notifier, bufferMaxBytes: bufferMaxBytes, processes: make(map[string]*commandProcess), } } +func (r *ProcessRunner) setWorkspaceTracker(tracker *WorkspaceTracker) { + var notifier workspaceStreamNotifier + if tracker != nil { + notifier = tracker + } + r.workspaceTrackerMu.Lock() + r.workspaceTracker = notifier + r.workspaceTrackerMu.Unlock() +} + // Start spawns a new background process and returns immediately. // // The process is executed via "sh -lc ", which: @@ -740,9 +760,6 @@ func (r *ProcessRunner) waitForProcessGroupExit(ctx context.Context, pid int) bo } func (r *ProcessRunner) publishOutput(proc *commandProcess, chunk ProcessOutputChunk) { - if r.workspaceTracker == nil { - return - } proc.mu.Lock() info := proc.info proc.mu.Unlock() @@ -755,13 +772,19 @@ func (r *ProcessRunner) publishOutput(proc *commandProcess, chunk ProcessOutputC Data: chunk.Data, Timestamp: chunk.Timestamp, } - r.workspaceTracker.notifyWorkspaceStreamProcessOutput(output) + // Keep the tracker selected for the entire publication. A rescan can replace + // and stop the old tracker between selection and notification; holding the + // read lock prevents that replacement from dropping this update. + r.workspaceTrackerMu.RLock() + defer r.workspaceTrackerMu.RUnlock() + tracker := r.workspaceTracker + if tracker == nil { + return + } + tracker.notifyWorkspaceStreamProcessOutput(output) } func (r *ProcessRunner) publishStatus(proc *commandProcess) { - if r.workspaceTracker == nil { - return - } proc.mu.Lock() info := proc.info proc.mu.Unlock() @@ -777,12 +800,19 @@ func (r *ProcessRunner) publishStatus(proc *commandProcess) { ExitCode: info.ExitCode, Timestamp: time.Now().UTC(), } + // Keep the tracker selected for the entire publication; see publishOutput. + r.workspaceTrackerMu.RLock() + defer r.workspaceTrackerMu.RUnlock() + tracker := r.workspaceTracker + if tracker == nil { + return + } r.logger.Debug("process status update", zap.String("process_id", info.ID), zap.String("session_id", info.SessionID), zap.String("status", string(info.Status)), ) - r.workspaceTracker.notifyWorkspaceStreamProcessStatus(update) + tracker.notifyWorkspaceStreamProcessStatus(update) } // snapshot creates a thread-safe copy of process info at the current moment. diff --git a/apps/backend/internal/agentctl/server/process/runner_tracker_test.go b/apps/backend/internal/agentctl/server/process/runner_tracker_test.go new file mode 100644 index 0000000000..d80a2fe64b --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/runner_tracker_test.go @@ -0,0 +1,123 @@ +package process + +import ( + "testing" + "time" + + "github.com/kandev/kandev/internal/agentctl/types" +) + +type recordingWorkspaceNotifier struct { + outputs []*types.ProcessOutput + statuses []*types.ProcessStatusUpdate +} + +func (r *recordingWorkspaceNotifier) notifyWorkspaceStreamProcessOutput(output *types.ProcessOutput) { + r.outputs = append(r.outputs, output) +} + +func (r *recordingWorkspaceNotifier) notifyWorkspaceStreamProcessStatus(status *types.ProcessStatusUpdate) { + r.statuses = append(r.statuses, status) +} + +// replacementDuringNotificationNotifier models a graph rescan that replaces +// and retires the active tracker while a process update is being published. +// TryLock is deliberately used only as a test probe: the production publish +// path must keep this writer from acquiring the tracker lock until notify +// returns. +type replacementDuringNotificationNotifier struct { + runner *ProcessRunner + replacement workspaceStreamNotifier + retired bool + outputs []*types.ProcessOutput + statuses []*types.ProcessStatusUpdate +} + +func (r *replacementDuringNotificationNotifier) notifyWorkspaceStreamProcessOutput( + output *types.ProcessOutput, +) { + if r.tryReplace() { + return + } + r.outputs = append(r.outputs, output) +} + +func (r *replacementDuringNotificationNotifier) notifyWorkspaceStreamProcessStatus( + status *types.ProcessStatusUpdate, +) { + if r.tryReplace() { + return + } + r.statuses = append(r.statuses, status) +} + +func (r *replacementDuringNotificationNotifier) tryReplace() bool { + if !r.runner.workspaceTrackerMu.TryLock() { + return false + } + r.retired = true + r.runner.workspaceTracker = r.replacement + r.runner.workspaceTrackerMu.Unlock() + return true +} + +func TestProcessRunnerKeepsTrackerSelectedDuringPublication(t *testing.T) { + proc := &commandProcess{ + info: ProcessInfo{ + ID: "process-1", + SessionID: "session-1", + Kind: types.ProcessKind("dev"), + Status: types.ProcessStatus("running"), + }, + } + + tests := []struct { + name string + publish func(*ProcessRunner, *commandProcess) + got func(*replacementDuringNotificationNotifier) int + }{ + { + name: "output", + publish: func(runner *ProcessRunner, proc *commandProcess) { + runner.publishOutput(proc, ProcessOutputChunk{ + Stream: "stdout", + Data: "hello", + Timestamp: time.Now().UTC(), + }) + }, + got: func(probe *replacementDuringNotificationNotifier) int { + return len(probe.outputs) + }, + }, + { + name: "status", + publish: func(runner *ProcessRunner, proc *commandProcess) { + runner.publishStatus(proc) + }, + got: func(probe *replacementDuringNotificationNotifier) int { + return len(probe.statuses) + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + runner := NewProcessRunner(nil, newTestLogger(t), 1024) + replacement := &recordingWorkspaceNotifier{} + probe := &replacementDuringNotificationNotifier{ + runner: runner, + replacement: replacement, + } + runner.workspaceTracker = probe + + test.publish(runner, proc) + + if probe.retired { + t.Fatal("tracker replacement acquired the lock during publication") + } + if got := test.got(probe); got != 1 { + t.Fatalf("active tracker received %d publications, want 1", got) + } + }) + } +} diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_status.go b/apps/backend/internal/agentctl/server/process/workspace_git_status.go index 0a13a3aa9f..5276113a03 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_status.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_status.go @@ -202,6 +202,7 @@ func (wt *WorkspaceTracker) computeGitStatus(ctx context.Context) (types.GitStat update := types.GitStatusUpdate{ Timestamp: time.Now(), RepositoryName: wt.repositoryName, + IsSubmodule: wt.IsSubmodule(), Modified: []string{}, Added: []string{}, Deleted: []string{}, @@ -322,6 +323,16 @@ func (wt *WorkspaceTracker) getGitBranchInfo(ctx context.Context, update *types. // baseBranch is treated as user-controlled and re-sanitised here so static // analysis sees the regex barrier inline with the `git` invocation. func (wt *WorkspaceTracker) computeBaseCommit(ctx context.Context, baseBranch string) string { + if wt.IsSubmodule() { + if !sha1HexPattern.MatchString(baseBranch) { + return "" + } + out, err := wt.runGitOutput(ctx, "rev-parse", "--verify", baseBranch+"^{commit}") + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) + } // Same inline regex barrier as resolveStoredRef so CodeQL's // taint-tracker sees it co-located with the `git` subprocess call. rest, hasOriginPrefix := strings.CutPrefix(baseBranch, "origin/") @@ -552,6 +563,15 @@ func (r baseBranchResolution) log(wt *WorkspaceTracker) { // outcome is assertable in tests rather than only observable through logs. func (wt *WorkspaceTracker) resolveBaseBranchWithReason(ctx context.Context) baseBranchResolution { stored := wt.BaseBranch() + if wt.IsSubmodule() { + anchor := wt.ComparisonAnchor() + if anchor != "" { + if ref := wt.resolveStoredRef(ctx, anchor); ref != "" { + return baseBranchResolution{ref: ref, stored: anchor, reason: baseBranchStored} + } + } + return baseBranchResolution{stored: anchor, reason: baseBranchUnresolved} + } if stored != "" { if ref := wt.resolveStoredRef(ctx, stored); ref != "" { return baseBranchResolution{ref: ref, stored: stored, reason: baseBranchStored} @@ -574,6 +594,9 @@ func (wt *WorkspaceTracker) resolveBaseBranchWithReason(ctx context.Context) bas // aheadBehindFallbackCandidates list — local main/master are excluded // because they can show stale, in-progress work for divergence counts. func (wt *WorkspaceTracker) resolveAheadBehindRef(ctx context.Context) string { + if wt.IsSubmodule() { + return wt.resolveBaseBranch(ctx) + } if stored := wt.BaseBranch(); stored != "" { if ref := wt.resolveStoredRef(ctx, stored); ref != "" { return ref diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_status_base_branch_test.go b/apps/backend/internal/agentctl/server/process/workspace_git_status_base_branch_test.go index 4d31ea8980..f740c45d2d 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_status_base_branch_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_status_base_branch_test.go @@ -271,6 +271,33 @@ func TestSetBaseBranch_RejectsUnsafeRefs(t *testing.T) { } } +func TestSetBaseBranchIfNotSubmodulePreservesComparisonAnchor(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + defer cleanup() + + wt := NewWorkspaceTracker(repoDir, newTestLogger(t)) + anchor := strings.Repeat("a", 40) + wt.SetComparisonAnchor(anchor) + + if wt.SetBaseBranchIfNotSubmodule("main") { + t.Fatal("SetBaseBranchIfNotSubmodule returned true for a submodule") + } + if got := wt.BaseBranch(); got != anchor { + t.Fatalf("BaseBranch after guarded update = %q, want anchor %q", got, anchor) + } + if !wt.IsSubmodule() { + t.Fatal("guarded update cleared submodule state") + } + + nonSubmodule := NewWorkspaceTracker(repoDir, newTestLogger(t)) + if !nonSubmodule.SetBaseBranchIfNotSubmodule("main") { + t.Fatal("SetBaseBranchIfNotSubmodule returned false for a regular tracker") + } + if got := nonSubmodule.BaseBranch(); got != "main" { + t.Fatalf("regular tracker BaseBranch() = %q, want main", got) + } +} + // TestLookupBaseBranch_FallbackToEmptyKey covers the map lookup the process // manager uses to hand each tracker its base branch. Per-repo entry wins; // missing per-repo falls back to the empty-key (single-repo) entry; legacy diff --git a/apps/backend/internal/agentctl/server/process/workspace_tracker.go b/apps/backend/internal/agentctl/server/process/workspace_tracker.go index 7afac41089..636ef2d126 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_tracker.go +++ b/apps/backend/internal/agentctl/server/process/workspace_tracker.go @@ -61,6 +61,11 @@ type WorkspaceTracker struct { // Sourced from task_repositories.base_branch on the kandev backend. // Empty for legacy tasks or external branches with no recorded base. baseBranch string + // comparisonAnchor is set for an initialized submodule. Unlike a branch + // name, it must remain pinned to the gitlink commit recorded by the parent + // comparison tree, even when the submodule's own default branch moves. + comparisonAnchor string + comparisonAnchorSet bool // lastBaseBranchLog dedupes the diff-base resolution log. resolveBaseBranch // runs on every status poll (as often as every couple of seconds), so @@ -178,6 +183,25 @@ func (wt *WorkspaceTracker) RepositoryName() string { func (wt *WorkspaceTracker) SetBaseBranch(baseBranch string) { wt.mu.Lock() defer wt.mu.Unlock() + wt.setBaseBranchLocked(baseBranch) +} + +// SetBaseBranchIfNotSubmodule updates the branch override only while holding +// the same lock used to publish a comparison anchor. This keeps a concurrent +// rescan from replacing a submodule anchor with a task-level base branch. +func (wt *WorkspaceTracker) SetBaseBranchIfNotSubmodule(baseBranch string) bool { + wt.mu.Lock() + defer wt.mu.Unlock() + if wt.comparisonAnchorSet { + return false + } + wt.setBaseBranchLocked(baseBranch) + return true +} + +func (wt *WorkspaceTracker) setBaseBranchLocked(baseBranch string) { + wt.comparisonAnchor = "" + wt.comparisonAnchorSet = false if !IsSafeGitRef(baseBranch) { wt.baseBranch = "" return @@ -185,6 +209,41 @@ func (wt *WorkspaceTracker) SetBaseBranch(baseBranch string) { wt.baseBranch = baseBranch } +// SetComparisonAnchor pins this tracker to the commit recorded by its parent +// repository at the parent's comparison point. An empty anchor is still +// meaningful: it records that this is a submodule whose committed base could +// not be resolved, so later base-branch updates must not substitute an +// unrelated branch from the task configuration. +func (wt *WorkspaceTracker) SetComparisonAnchor(anchor string) { + wt.mu.Lock() + defer wt.mu.Unlock() + wt.comparisonAnchorSet = true + if !sha1HexPattern.MatchString(anchor) { + wt.comparisonAnchor = "" + wt.baseBranch = "" + return + } + wt.comparisonAnchor = anchor + wt.baseBranch = anchor +} + +// IsSubmodule reports whether this tracker represents a declared submodule, +// including a child whose parent comparison anchor could not be resolved. +func (wt *WorkspaceTracker) IsSubmodule() bool { + wt.mu.RLock() + defer wt.mu.RUnlock() + return wt.comparisonAnchorSet +} + +// ComparisonAnchor returns the exact parent-recorded gitlink commit when one +// was discovered. It is empty when the tracker is not a submodule or when the +// parent anchor could not be resolved locally. +func (wt *WorkspaceTracker) ComparisonAnchor() string { + wt.mu.RLock() + defer wt.mu.RUnlock() + return wt.comparisonAnchor +} + // IsSafeGitRef reports whether ref is safe to splice into a `git` // subprocess argument list. Empty input returns true so callers can treat // "" as "no override" without an extra branch. Delegates to the diff --git a/apps/backend/internal/agentctl/types/streams/git.go b/apps/backend/internal/agentctl/types/streams/git.go index a302f21a8c..8911abc31f 100644 --- a/apps/backend/internal/agentctl/types/streams/git.go +++ b/apps/backend/internal/agentctl/types/streams/git.go @@ -16,6 +16,12 @@ type GitStatusUpdate struct { // state on this name. RepositoryName string `json:"repository_name,omitempty"` + // IsSubmodule identifies initialized Git submodule repositories. The + // frontend uses this explicit boundary metadata when rendering review + // scope headers; repository names alone cannot distinguish submodules + // from sibling repositories. + IsSubmodule bool `json:"is_submodule,omitempty"` + // Modified contains paths of modified files. Modified []string `json:"modified"` diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index e89dc29f68..40e201e1e7 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -344,6 +344,7 @@ func buildGitStatusNotification(sessionID, repositoryName string, status client. "renamed": status.Renamed, "branch_additions": status.BranchAdditions, "branch_deletions": status.BranchDeletions, + "is_submodule": status.IsSubmodule, } if repositoryName != "" { statusPayload["repository_name"] = repositoryName diff --git a/apps/backend/internal/review/diff.go b/apps/backend/internal/review/diff.go index a8eb0936b1..2de2be1a10 100644 --- a/apps/backend/internal/review/diff.go +++ b/apps/backend/internal/review/diff.go @@ -45,7 +45,11 @@ type ChangedFile struct { // Key returns the composite \x00 identity, matching // `reviewFileKey` in apps/web/components/review/types.ts. func (f ChangedFile) Key() string { - if f.RepositoryName == "" { + // A finding from a legacy single-repository task has neither repository + // identity field and keeps the historical bare-path key. An explicit empty + // repository name with an ID is the real workspace-root scope in a + // multi-repository task and must remain distinct from that legacy key. + if f.RepositoryName == "" && f.RepositoryID == "" { return f.Path } return f.RepositoryName + fileKeySep + f.Path diff --git a/apps/backend/internal/review/diff_test.go b/apps/backend/internal/review/diff_test.go index a4f22c747f..c90b92eef7 100644 --- a/apps/backend/internal/review/diff_test.go +++ b/apps/backend/internal/review/diff_test.go @@ -228,13 +228,17 @@ func TestChangedFileKeyAndIndex(t *testing.T) { if single.Key() != "a.go" { t.Fatalf("single-repo key must be the bare path, got %q", single.Key()) } + root := ChangedFile{Path: "a.go", RepositoryID: "repo-root"} + if root.Key() != "\x00a.go" { + t.Fatalf("explicit workspace-root key must keep its scope, got %q", root.Key()) + } multi := ChangedFile{Path: "a.go", RepositoryName: "backend"} if multi.Key() != "backend\x00a.go" { t.Fatalf("multi-repo key mismatch, got %q", multi.Key()) } - index := FileByKey([]ChangedFile{single, multi}) - if len(index) != 2 { - t.Fatalf("expected both keys indexed, got %d", len(index)) + index := FileByKey([]ChangedFile{single, root, multi}) + if len(index) != 3 { + t.Fatalf("expected all scoped keys indexed, got %d", len(index)) } } diff --git a/apps/web/components/review/review-dialog-submodule.test.ts b/apps/web/components/review/review-dialog-submodule.test.ts new file mode 100644 index 0000000000..43f7eb2935 --- /dev/null +++ b/apps/web/components/review/review-dialog-submodule.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from "vitest"; +import { buildAllFiles } from "./review-dialog"; + +const CHILD_SCOPE = "vendor/lib"; +const README_PATH = "README.md"; + +describe("buildAllFiles submodule metadata", () => { + it("retains submodule metadata on PR-only files", () => { + const result = buildAllFiles( + null, + null, + [ + { + filename: README_PATH, + status: "modified", + patch: "@@ -1 +1 @@\n-old\n+new\n", + additions: 1, + deletions: 1, + is_submodule: true, + }, + ], + CHILD_SCOPE, + ); + + expect(result).toEqual([ + expect.objectContaining({ + path: README_PATH, + repository_name: CHILD_SCOPE, + source: "pr", + is_submodule: true, + }), + ]); + }); +}); diff --git a/apps/web/components/review/review-dialog.build-files.test.ts b/apps/web/components/review/review-dialog.build-files.test.ts index 0b54cb06bc..77765ba3c3 100644 --- a/apps/web/components/review/review-dialog.build-files.test.ts +++ b/apps/web/components/review/review-dialog.build-files.test.ts @@ -16,6 +16,9 @@ import type { Comment } from "@/lib/state/slices/comments"; const RENAMED_PATH = "src/new-name.ts"; const PREVIOUS_PATH = "src/old-name.ts"; const README_PATH = "README.md"; +const LIB_SCOPE = "vendor/lib"; +const OUTER_SCOPE = "vendor/outer"; +const INNER_SCOPE = `${OUTER_SCOPE}/vendor/inner`; const FRONTEND_REPO = "frontend"; const BACKEND_REPO = "backend"; const FIRST_PR_KEY = "acme/app/1"; @@ -249,6 +252,71 @@ describe("buildAllFiles", () => { }); }); +describe("buildAllFiles submodule gitlinks", () => { + it("hides an available parent gitlink when child files are present", () => { + const gitStatusFiles = { + [`\u0000${LIB_SCOPE}`]: { + path: LIB_SCOPE, + status: "modified" as const, + staged: false, + diff: "@@gitlink@@", + repository_name: "", + }, + [`${LIB_SCOPE}\u0000${README_PATH}`]: { + path: README_PATH, + status: "modified" as const, + staged: false, + diff: "@@child@@", + repository_name: LIB_SCOPE, + }, + }; + + const result = buildAllFiles(gitStatusFiles, null); + + expect(result.map(reviewFileKey)).toEqual([`${LIB_SCOPE}\u0000${README_PATH}`]); + }); + + it("hides a nested parent gitlink at its owning repository scope", () => { + const result = buildAllFiles( + { + [`${OUTER_SCOPE}\u0000vendor/inner`]: { + path: "vendor/inner", + status: "modified" as const, + staged: false, + repository_name: OUTER_SCOPE, + diff: "@@nested-gitlink@@", + }, + [`${INNER_SCOPE}\u0000${README_PATH}`]: { + path: README_PATH, + status: "modified" as const, + staged: false, + repository_name: INNER_SCOPE, + diff: "@@nested-child@@", + }, + }, + null, + ); + + expect(result.map(reviewFileKey)).toEqual([`${INNER_SCOPE}\u0000${README_PATH}`]); + }); + + it("keeps the parent gitlink when no child file source is available", () => { + const result = buildAllFiles( + { + [LIB_SCOPE]: { + path: LIB_SCOPE, + status: "modified" as const, + staged: false, + diff: "@@gitlink@@", + }, + }, + null, + ); + + expect(result.map(reviewFileKey)).toEqual([LIB_SCOPE]); + }); +}); + describe("buildAllFiles source collisions", () => { it("deduplicates a repo-stamped cumulative file against repo-unaware git status", () => { const path = "src/shared.ts"; @@ -280,12 +348,45 @@ describe("buildAllFiles source collisions", () => { }, } as unknown as CumulativeDiff; - const result = buildAllFiles(gitStatusFiles, cumulativeDiff); + const result = buildAllFiles(gitStatusFiles, cumulativeDiff, undefined, undefined, false); expect(result).toHaveLength(1); expect(result[0]).toMatchObject({ path, source: "uncommitted" }); }); + it("keeps a root file beside a same-named nested-scope file", () => { + const result = buildAllFiles( + { + [README_PATH]: { + path: README_PATH, + status: "modified" as const, + staged: false, + diff: "@@root@@", + }, + }, + { + session_id: "s1", + base_commit: "abc", + head_commit: "def", + total_commits: 1, + files: { + [`vendor/outer\u0000${README_PATH}`]: { + path: README_PATH, + repository_name: "vendor/outer", + status: "modified", + staged: false, + diff: "@@outer@@", + }, + }, + } as unknown as CumulativeDiff, + ); + + expect(result.map(reviewFileKey).sort()).toEqual([ + README_PATH, + `vendor/outer\u0000${README_PATH}`, + ]); + }); + it("uses a bare cumulative key in single-repository review mode", () => { const path = "src/committed.ts"; const cumulativeDiff = { diff --git a/apps/web/components/review/review-dialog.tsx b/apps/web/components/review/review-dialog.tsx index 194863591b..2440fb55c0 100644 --- a/apps/web/components/review/review-dialog.tsx +++ b/apps/web/components/review/review-dialog.tsx @@ -26,6 +26,7 @@ import { normalizeDiffContent, reviewFileKey, splitReviewFileKey as splitFileKey, + suppressAvailableGitlinkFiles, } from "./types"; /** @@ -58,8 +59,12 @@ function addCumulativeDiffFiles( const path = file.path ?? splitFileKey(mapKey).path; if (!path) continue; const key = fileMapKey(path, repoName); - const hasRepoUnawareCollision = key !== path && fileMap.has(path); - if (fileMap.has(key) || hasRepoUnawareCollision) continue; + // In multi-scope mode, a bare path belongs to the workspace root while a + // repository-scoped path belongs to that repository. They may share a + // filename (README.md is common), so only the canonical scoped key is a + // collision. Single-repository callers pass useRepositoryKeys=false and + // therefore still dedupe on the bare path. + if (fileMap.has(key)) continue; const diff = file.diff ? normalizeDiffContent(file.diff) : ""; fileMap.set(key, { path, @@ -73,6 +78,7 @@ function addCumulativeDiffFiles( diff_skip_reason: file.diff_skip_reason, repository_name: repoName, base_ref: file.base_ref ?? defaultBaseRef, + is_submodule: file.is_submodule, }); } } @@ -80,9 +86,12 @@ function addCumulativeDiffFiles( function addUncommittedFiles( fileMap: Map, gitStatusFiles: Record, + useRepositoryKeys: boolean, + isSubmodule = false, ) { for (const file of Object.values(gitStatusFiles)) { - const key = fileMapKey(file.path, file.repository_name); + const repositoryName = useRepositoryKeys ? file.repository_name : undefined; + const key = fileMapKey(file.path, repositoryName); if (fileMap.has(key)) continue; const diff = file.diff ? normalizeDiffContent(file.diff) : ""; fileMap.set(key, { @@ -95,17 +104,17 @@ function addUncommittedFiles( source: "uncommitted", old_path: file.old_path, diff_skip_reason: file.diff_skip_reason, - repository_name: file.repository_name, + repository_name: repositoryName, + is_submodule: file.is_submodule ?? isSubmodule, }); } } function addPRFiles(fileMap: Map, files: PRDiffFile[], repoName?: string) { - const repositoryName = repoName || undefined; + const repositoryName = repoName; for (const file of files) { const key = fileMapKey(file.filename, repositoryName); - const hasRepoUnawareCollision = !!repositoryName && fileMap.has(file.filename); - if (fileMap.has(key) || hasRepoUnawareCollision) continue; + if (fileMap.has(key)) continue; const diff = file.patch ? normalizeDiffContent(file.patch) : ""; fileMap.set(key, { path: file.filename, @@ -117,6 +126,7 @@ function addPRFiles(fileMap: Map, files: PRDiffFile[], repoN source: "pr", old_path: file.old_path, repository_name: repositoryName, + is_submodule: file.is_submodule, }); } } @@ -137,7 +147,7 @@ export function buildAllFiles( // (panel: fresh worktree content from `git-status`, dialog: stale cumulative // diff snapshot from the last fetch) — the dialog appeared to show outdated // content even though the cumulative-diff hook was successfully refetching. - if (gitStatusFiles) addUncommittedFiles(fileMap, gitStatusFiles); + if (gitStatusFiles) addUncommittedFiles(fileMap, gitStatusFiles, useRepositoryKeys); if (cumulativeDiff?.files) { addCumulativeDiffFiles( fileMap, @@ -147,11 +157,12 @@ export function buildAllFiles( ); } if (prDiffFiles) addPRFiles(fileMap, prDiffFiles, prRepoName); - return Array.from(fileMap.values()).sort((a, b) => { + const sortedFiles = Array.from(fileMap.values()).sort((a, b) => { const repoCmp = (a.repository_name ?? "").localeCompare(b.repository_name ?? ""); if (repoCmp !== 0) return repoCmp; return a.path.localeCompare(b.path); }); + return suppressAvailableGitlinkFiles(sortedFiles); } export function filterPendingDiffCommentsForSession( @@ -295,7 +306,7 @@ function useReviewDialogHandlers(opts: ReviewDialogHandlerOptions) { // discard runs in the correct repo's worktree. const { repositoryName, path } = splitFileKey(key); try { - const result = await discard([path], repositoryName || undefined); + const result = await discard([path], repositoryName); if (result.success) toast({ title: "Changes discarded", description: path, variant: "success" }); else diff --git a/apps/web/components/review/review-diff-group.tsx b/apps/web/components/review/review-diff-group.tsx new file mode 100644 index 0000000000..9a59508770 --- /dev/null +++ b/apps/web/components/review/review-diff-group.tsx @@ -0,0 +1,26 @@ +import type { ReactNode } from "react"; +import { RepoGroupHeader } from "./review-diff-list-groups"; +import type { ReviewFile } from "./types"; + +type ReviewDiffGroupProps = { + group: { repositoryName: string; items: ReviewFile[] }; + showRepoHeaders: boolean; + renderFile: (file: ReviewFile) => ReactNode; +}; + +export function ReviewDiffGroup({ group, showRepoHeaders, renderFile }: ReviewDiffGroupProps) { + return ( +
+ {showRepoHeaders && ( + file.is_submodule) + } + /> + )} + {group.items.map((file) => renderFile(file))} +
+ ); +} diff --git a/apps/web/components/review/review-diff-header.test.tsx b/apps/web/components/review/review-diff-header.test.tsx index a283df1d46..ebe921c01c 100644 --- a/apps/web/components/review/review-diff-header.test.tsx +++ b/apps/web/components/review/review-diff-header.test.tsx @@ -160,6 +160,31 @@ describe("ReviewDiffHeader responsive composition", () => { ); }); + it("keeps the submodule scope visible in the mobile sticky identity", () => { + mocks.isMobile = true; + render( + , + ); + + expect(screen.getByTestId("review-file-repository").textContent).toBe( + "vendor/outer/vendor/inner", + ); + }); + it("keeps the desktop toolbar in its existing inline actions slot", () => { render( + {file.repository_name && ( + + {file.repository_name} + + )} {name} diff --git a/apps/web/components/review/review-diff-list-grouping.test.tsx b/apps/web/components/review/review-diff-list-grouping.test.tsx index 4a85240128..9183907da4 100644 --- a/apps/web/components/review/review-diff-list-grouping.test.tsx +++ b/apps/web/components/review/review-diff-list-grouping.test.tsx @@ -59,7 +59,7 @@ afterEach(() => { mocks.isMobile = false; }); -function file(path: string, repo?: string): ReviewFile { +function file(path: string, repo?: string, isSubmodule = false): ReviewFile { return { path, diff: "@@ -1 +1 @@\n-a\n+b\n", @@ -69,6 +69,7 @@ function file(path: string, repo?: string): ReviewFile { staged: false, source: "uncommitted", repository_name: repo, + is_submodule: isSubmodule, }; } @@ -90,6 +91,8 @@ function withTooltips(node: ReactNode) { return {node}; } +const REPO_HEADER_TEST_ID = "changes-repo-header"; + function renderSingleFile(reviewFile: ReviewFile, selected = true) { const refs = new Map([[reviewFile.path, createRef()]]); render( @@ -108,7 +111,7 @@ describe("ReviewDiffList — multi-repo grouping", () => { it("renders no repo header when all files lack a repository_name", () => { const refs = new Map([["a.ts", createRef()]]); render(withTooltips()); - expect(screen.queryAllByTestId("changes-repo-header")).toHaveLength(0); + expect(screen.queryAllByTestId(REPO_HEADER_TEST_ID)).toHaveLength(0); }); it("renders one header per repo when 2+ repos are present", () => { @@ -120,7 +123,7 @@ describe("ReviewDiffList — multi-repo grouping", () => { const refs = new Map(files.map((f) => [f.path, createRef()])); render(withTooltips()); - const headers = screen.getAllByTestId("changes-repo-header"); + const headers = screen.getAllByTestId(REPO_HEADER_TEST_ID); expect(headers).toHaveLength(2); expect(headers[0].textContent).toContain("frontend"); expect(headers[0].textContent).toContain("2 files"); @@ -135,11 +138,17 @@ describe("ReviewDiffList — multi-repo grouping", () => { , ), ); - expect(screen.getByTestId("changes-repo-header").textContent).toContain("only-repo"); + expect(screen.getByTestId(REPO_HEADER_TEST_ID).textContent).toContain("only-repo"); }); it("groups files into per-repo sections with the right counts", () => { - const files: ReviewFile[] = [file("a.ts", "x"), file("b.ts", "y"), file("c.ts", "x")]; + const firstRepo = "x"; + const secondRepo = "y"; + const files: ReviewFile[] = [ + file("a.ts", firstRepo), + file("b.ts", secondRepo), + file("c.ts", firstRepo), + ]; const refs = new Map(files.map((f) => [f.path, createRef()])); render(withTooltips()); const groups = screen.getAllByTestId("changes-repo-group"); @@ -147,8 +156,9 @@ describe("ReviewDiffList — multi-repo grouping", () => { // The diff body itself is lazy-loaded via IntersectionObserver (which // doesn't fire in happy-dom), so verify grouping by counting file path // labels in each group's header rather than the inner diff body. - const xGroup = groups.find((g) => g.getAttribute("data-repository-name") === "x"); - const yGroup = groups.find((g) => g.getAttribute("data-repository-name") === "y"); + const repositoryNameAttribute = "data-repository-name"; + const xGroup = groups.find((g) => g.getAttribute(repositoryNameAttribute) === firstRepo); + const yGroup = groups.find((g) => g.getAttribute(repositoryNameAttribute) === secondRepo); // FileDiffHeader always renders the file path in a span; count those. expect(xGroup?.textContent).toContain("a.ts"); expect(xGroup?.textContent).toContain("c.ts"); @@ -156,6 +166,32 @@ describe("ReviewDiffList — multi-repo grouping", () => { expect(yGroup?.textContent).toContain("b.ts"); expect(yGroup?.textContent).not.toContain("a.ts"); }); + + it("marks named groups as submodule scopes when the root group is present", () => { + const submoduleScope = "vendor/lib"; + const files: ReviewFile[] = [file("README.md"), file("src/lib.ts", submoduleScope, true)]; + const refs = new Map(files.map((f) => [f.path, createRef()])); + render(withTooltips()); + + const childGroup = screen.getAllByTestId("changes-repo-group")[0]; + expect(childGroup.getAttribute("data-repository-name")).toBe(""); + const groups = screen.getAllByTestId("changes-repo-group"); + const submoduleGroup = groups.find( + (group) => group.getAttribute("data-repository-name") === submoduleScope, + ); + expect(submoduleGroup?.querySelector('[data-submodule-scope="true"]')).not.toBeNull(); + }); + + it("marks a direct submodule scope without a workspace-root group", () => { + const submoduleScope = "vendor"; + const reviewFile = file("src/lib.ts", submoduleScope, true); + const refs = new Map([[reviewFile.path, createRef()]]); + render(withTooltips()); + + expect(screen.getByTestId(REPO_HEADER_TEST_ID).getAttribute("data-submodule-scope")).toBe( + "true", + ); + }); }); describe("ReviewDiffList — file status rendering", () => { diff --git a/apps/web/components/review/review-diff-list-groups.tsx b/apps/web/components/review/review-diff-list-groups.tsx index 2f3d054ed0..dcfe1ac7b6 100644 --- a/apps/web/components/review/review-diff-list-groups.tsx +++ b/apps/web/components/review/review-diff-list-groups.tsx @@ -1,12 +1,24 @@ "use client"; +import { IconGitBranch } from "@tabler/icons-react"; +import { useTranslation } from "react-i18next"; + /** * Sticky per-repository section header rendered above each group of files in * the changes panel. The grouping logic lives in @/lib/group-by-repo so the * Review dialog and the dockview Changes tab share the same Map-based * bucketer. Only this presentational header is review-specific. */ -export function RepoGroupHeader({ name, fileCount }: { name: string; fileCount: number }) { +export function RepoGroupHeader({ + name, + fileCount, + isSubmodule = false, +}: { + name: string; + fileCount: number; + isSubmodule?: boolean; +}) { + const { t } = useTranslation(); // The empty-name group ("uncategorised" — no repository_name on its files) // gets a generic label so the user understands what they're looking at. const label = name || "Other changes"; @@ -14,7 +26,12 @@ export function RepoGroupHeader({ name, fileCount }: { name: string; fileCount:
+ {isSubmodule && ( +
); }); -// Per-repo grouping helpers live in ./review-diff-list-groups so this file -// stays under the 600-line lint cap. - type FileDiffSectionProps = { file: ReviewFile; /** Composite per-file key from `reviewFileKey(file)` — used as the arg diff --git a/apps/web/components/review/review-file-tree.test.tsx b/apps/web/components/review/review-file-tree.test.tsx index 82e66b151d..b1fb0463f4 100644 --- a/apps/web/components/review/review-file-tree.test.tsx +++ b/apps/web/components/review/review-file-tree.test.tsx @@ -198,6 +198,19 @@ describe("ReviewFileTree", () => { // backend's child is still visible. expect(screen.getByText("task.go")).toBeTruthy(); }); + + it("marks a nested submodule boundary with an accessible scope label", () => { + renderTree([ + file({ path: "README.md" }), + file({ path: "src/a.ts", repository_name: "vendor/lib" }), + ]); + + const node = screen.getByTestId("submodule-node"); + expect(node.textContent).toContain("lib"); + expect(within(node).getByRole("button").getAttribute("aria-label")).toBe( + "Submodule: vendor/lib", + ); + }); }); describe("ReviewFileTree multi-repo identity", () => { diff --git a/apps/web/components/review/review-file-tree.tsx b/apps/web/components/review/review-file-tree.tsx index 53f7c34b42..a53285beea 100644 --- a/apps/web/components/review/review-file-tree.tsx +++ b/apps/web/components/review/review-file-tree.tsx @@ -6,6 +6,7 @@ import { IconChevronRight, IconMessage, IconAlertTriangle, + IconGitBranch, IconSearch, IconX, } from "@tabler/icons-react"; @@ -14,9 +15,9 @@ import { cn } from "@kandev/ui/lib/utils"; import { FileStatusIcon } from "@/components/shared/file-status-icon"; import { FileIcon } from "@/components/ui/file-icon"; import { useTree, type VisibleRow } from "@/hooks/use-tree"; +import { useTranslation } from "react-i18next"; import type { ReviewFile, FileTreeNode } from "./types"; import { buildFileTree, reviewFileKey } from "./types"; -import { useTranslation } from "react-i18next"; type ReviewFileTreeProps = { files: ReviewFile[]; @@ -132,15 +133,25 @@ function ReviewTreeRow(props: ReviewTreeRowProps) { } function ReviewDirRow({ row, onToggleDir }: ReviewTreeRowProps) { + const { t } = useTranslation(); const isRepoRoot = Boolean(row.node.isRepoRoot); - const fileCount = isRepoRoot ? countLeafFiles(row.node) : 0; + const isSubmodule = Boolean(row.node.isSubmodule); + const isScopeBoundary = isRepoRoot || isSubmodule; + const fileCount = isScopeBoundary ? countLeafFiles(row.node) : 0; + const scopeLabel = row.node.repositoryName ?? row.node.name; + let testId = "dir-node"; + if (isRepoRoot) testId = "repo-root-node"; + if (isSubmodule) testId = "submodule-node"; return ( -
+