feat(review): support nested submodule review (#2295)

This commit is contained in:
Carlos Florêncio
2026-08-06 15:04:19 +01:00
committed by GitHub
parent 92e2256544
commit a5d6649f69
90 changed files with 3798 additions and 515 deletions
@@ -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"`
@@ -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 {
@@ -305,6 +305,7 @@ func (p *EventPublisher) PublishGitStatus(execution *AgentExecution, update *age
BranchAdditions: update.BranchAdditions,
BranchDeletions: update.BranchDeletions,
RepositoryName: update.RepositoryName,
IsSubmodule: update.IsSubmodule,
},
})
}
@@ -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")
}
@@ -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
// `<repo> <path>` 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,
@@ -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{
@@ -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
}
@@ -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.
@@ -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
@@ -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.
@@ -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))
}
}
@@ -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)
}
@@ -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 <command>", 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.
@@ -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)
}
})
}
}
@@ -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
@@ -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
@@ -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
@@ -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"`
@@ -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
+5 -1
View File
@@ -45,7 +45,11 @@ type ChangedFile struct {
// Key returns the composite <repository>\x00<path> 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
+7 -3
View File
@@ -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))
}
}
@@ -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,
}),
]);
});
});
@@ -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 = {
+21 -10
View File
@@ -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<string, ReviewFile>,
gitStatusFiles: Record<string, FileInfo>,
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<string, ReviewFile>, 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<string, ReviewFile>, 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
@@ -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 (
<div data-testid="changes-repo-group" data-repository-name={group.repositoryName}>
{showRepoHeaders && (
<RepoGroupHeader
name={group.repositoryName}
fileCount={group.items.length}
isSubmodule={
Boolean(group.repositoryName) && group.items.some((file) => file.is_submodule)
}
/>
)}
{group.items.map((file) => renderFile(file))}
</div>
);
}
@@ -160,6 +160,31 @@ describe("ReviewDiffHeader responsive composition", () => {
);
});
it("keeps the submodule scope visible in the mobile sticky identity", () => {
mocks.isMobile = true;
render(
<ReviewDiffHeader
file={{ ...file, repository_name: "vendor/outer/vendor/inner" }}
isReviewed={false}
isStale={false}
sessionId={SESSION_ID}
collapsed={false}
wordWrap={false}
expandUnchanged={false}
baseBranchByRepo={{ "vendor/outer/vendor/inner": "abc123" }}
onCheckboxChange={vi.fn()}
onDiscard={vi.fn()}
onToggleCollapse={vi.fn()}
onToggleExpandUnchanged={vi.fn()}
onToggleWordWrap={vi.fn()}
/>,
);
expect(screen.getByTestId("review-file-repository").textContent).toBe(
"vendor/outer/vendor/inner",
);
});
it("keeps the desktop toolbar in its existing inline actions slot", () => {
render(
<ReviewDiffHeader
@@ -107,8 +107,16 @@ function MobileReviewFileDetails({ file, isStale }: { file: ReviewFile; isStale:
return (
<span
className="flex min-w-0 flex-1 flex-col justify-center overflow-hidden text-left leading-none"
title={file.path}
title={file.repository_name ? `${file.repository_name}/${file.path}` : file.path}
>
{file.repository_name && (
<span
data-testid="review-file-repository"
className="truncate text-[10px] font-medium leading-3 text-primary"
>
{file.repository_name}
</span>
)}
<span data-review-file-name className="truncate text-[13px] font-medium leading-4">
{name}
</span>
@@ -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 <TooltipProvider>{node}</TooltipProvider>;
}
const REPO_HEADER_TEST_ID = "changes-repo-header";
function renderSingleFile(reviewFile: ReviewFile, selected = true) {
const refs = new Map([[reviewFile.path, createRef<HTMLDivElement>()]]);
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<HTMLDivElement>()]]);
render(withTooltips(<ReviewDiffList {...baseProps} files={[file("a.ts")]} fileRefs={refs} />));
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<HTMLDivElement>()]));
render(withTooltips(<ReviewDiffList {...baseProps} files={files} fileRefs={refs} />));
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", () => {
<ReviewDiffList {...baseProps} files={[file("a.ts", "only-repo")]} fileRefs={refs} />,
),
);
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<HTMLDivElement>()]));
render(withTooltips(<ReviewDiffList {...baseProps} files={files} fileRefs={refs} />));
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<HTMLDivElement>()]));
render(withTooltips(<ReviewDiffList {...baseProps} files={files} fileRefs={refs} />));
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<HTMLDivElement>()]]);
render(withTooltips(<ReviewDiffList {...baseProps} files={[reviewFile]} fileRefs={refs} />));
expect(screen.getByTestId(REPO_HEADER_TEST_ID).getAttribute("data-submodule-scope")).toBe(
"true",
);
});
});
describe("ReviewDiffList — file status rendering", () => {
@@ -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:
<div
className="sticky top-0 z-20 flex items-center gap-2 px-4 py-1.5 bg-muted/40 backdrop-blur-sm border-y border-border/60 text-xs font-medium text-foreground"
data-testid="changes-repo-header"
data-submodule-scope={isSubmodule ? "true" : undefined}
aria-label={isSubmodule ? t("common:reviewSubmoduleBoundary", { scope: name }) : undefined}
>
{isSubmodule && (
<IconGitBranch aria-hidden="true" className="h-3.5 w-3.5 shrink-0 text-primary" />
)}
<span className="truncate">{label}</span>
<span className="text-muted-foreground/70">
{fileCount} {fileCount === 1 ? "file" : "files"}
@@ -18,7 +18,7 @@ import {
reviewFileKey,
} from "./types";
import type { ReviewFile } from "./types";
import { RepoGroupHeader } from "./review-diff-list-groups";
import { ReviewDiffGroup } from "./review-diff-group";
import { ReviewDiffHeader, type ReviewExternalLinkContext } from "./review-diff-header";
import { extractReviewMarkdownPreview } from "./review-markdown-diff-preview";
import { ReviewMarkdownDiffPreviewContent } from "./review-markdown-diff-preview-content";
@@ -107,15 +107,11 @@ export const ReviewDiffList = memo(function ReviewDiffList({
onKeyDownCapture={handleKeyDown}
>
{groups.map((group) => (
<div
<ReviewDiffGroup
key={group.repositoryName || "__no_repo__"}
data-testid="changes-repo-group"
data-repository-name={group.repositoryName || ""}
>
{showRepoHeaders && (
<RepoGroupHeader name={group.repositoryName} fileCount={group.items.length} />
)}
{group.items.map((file) => {
group={group}
showRepoHeaders={showRepoHeaders}
renderFile={(file) => {
const key = reviewFileKey(file);
return (
<FileDiffSection
@@ -131,7 +127,7 @@ export const ReviewDiffList = memo(function ReviewDiffList({
isSelected={selectedFile === key}
forceLoad={
selectedIndex >= 0 &&
files.findIndex((f) => reviewFileKey(f) === key) <= selectedIndex
files.findIndex((candidate) => reviewFileKey(candidate) === key) <= selectedIndex
}
onToggleReviewed={onToggleReviewed}
onDiscard={onDiscard}
@@ -149,16 +145,13 @@ export const ReviewDiffList = memo(function ReviewDiffList({
}}
/>
);
})}
</div>
}}
/>
))}
</div>
);
});
// 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
@@ -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", () => {
@@ -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 (
<div data-testid={isRepoRoot ? "repo-root-node" : "dir-node"}>
<div data-testid={testId} data-repository-name={row.node.repositoryName}>
<button
type="button"
aria-label={
isSubmodule ? t("common:reviewSubmoduleBoundary", { scope: scopeLabel }) : undefined
}
className={cn(
"flex items-center w-full gap-1 px-2 py-1 hover:bg-muted/50 transition-colors cursor-pointer",
isRepoRoot && "border-t border-border/40 first:border-t-0 mt-1 first:mt-0",
isScopeBoundary && "border-t border-border/40 first:border-t-0 mt-1 first:mt-0",
)}
style={{ paddingLeft: `${row.depth * 12 + 8}px` }}
onClick={onToggleDir}
@@ -150,10 +161,13 @@ function ReviewDirRow({ row, onToggleDir }: ReviewTreeRowProps) {
) : (
<IconChevronRight className="h-3.5 w-3.5 shrink-0 text-muted-foreground" />
)}
{isSubmodule && (
<IconGitBranch aria-hidden="true" className="h-3.5 w-3.5 shrink-0 text-primary" />
)}
<span
className={cn(
"text-[13px] truncate",
isRepoRoot ? "font-medium text-foreground" : "text-muted-foreground",
isScopeBoundary ? "font-medium text-foreground" : "text-muted-foreground",
)}
>
{row.node.name}
@@ -81,6 +81,54 @@ describe("buildFileTree — multi-repo", () => {
expect(frontend?.children?.[0].file?.repository_id).toBe("f");
expect(backend?.children?.[0].file?.repository_id).toBe("b");
});
it("places nested submodule scopes beneath their workspace directory hierarchy", () => {
const tree = buildFileTree([
file({ path: "README.md" }),
file({ path: "src/main.ts", repository_name: "vendor/outer" }),
file({ path: "README.md", repository_name: "vendor/outer/vendor/inner" }),
]);
const vendor = tree.find((node) => node.name === "vendor");
const outer = vendor?.children?.find((node) => node.name === "outer");
const nestedVendor = outer?.children?.find((node) => node.name === "vendor");
const inner = nestedVendor?.children?.find((node) => node.name === "inner");
expect(vendor).toBeDefined();
expect(outer?.isSubmodule).toBe(true);
expect(outer?.repositoryName).toBe("vendor/outer");
expect(inner?.isSubmodule).toBe(true);
expect(inner?.repositoryName).toBe("vendor/outer/vendor/inner");
expect(inner?.children?.[0].file?.repository_name).toBe("vendor/outer/vendor/inner");
});
it("merges repeated directory segments within a repo despite interleaved input", () => {
const tree = buildFileTree([
file({ path: "src/a.ts", repository_name: "frontend", repository_id: "f" }),
file({ path: "README.md", repository_name: "frontend", repository_id: "f" }),
file({ path: "src/b.ts", repository_name: "frontend", repository_id: "f" }),
file({ path: "main.go", repository_name: "backend", repository_id: "b" }),
]);
const frontend = tree.find((node) => node.name === "frontend");
const srcNodes = frontend?.children?.filter((node) => node.name === "src") ?? [];
expect(srcNodes).toHaveLength(1);
expect(srcNodes[0].children?.map((node) => node.file?.path)).toEqual(["src/a.ts", "src/b.ts"]);
});
it("keeps identical relative directories distinct across repositories", () => {
const tree = buildFileTree([
file({ path: "src/frontend.ts", repository_name: "frontend", repository_id: "f" }),
file({ path: "src/backend.ts", repository_name: "backend", repository_id: "b" }),
]);
const srcNodes = tree.map((root) => root.children?.find((node) => node.name === "src"));
expect(srcNodes).toHaveLength(2);
expect(srcNodes.map((node) => node?.children?.[0].file?.repository_name)).toEqual([
"frontend",
"backend",
]);
});
});
// reviewFileKey + splitReviewFileKey are the dedup primitive for the
@@ -92,7 +140,10 @@ describe("buildFileTree — multi-repo", () => {
describe("reviewFileKey", () => {
it("returns the bare path when no repository_name is set", () => {
expect(reviewFileKey({ path: "README.md" })).toBe("README.md");
expect(reviewFileKey({ path: "src/index.ts", repository_name: "" })).toBe("src/index.ts");
});
it("keeps an explicit root scope distinct from a legacy bare path", () => {
expect(reviewFileKey({ path: "src/index.ts", repository_name: "" })).toBe(`${SEP}src/index.ts`);
});
it("joins repository_name and path with the NUL separator", () => {
@@ -182,15 +233,21 @@ describe("splitReviewFileKey", () => {
});
});
it("treats a key with empty repository_name as a legacy bare-path key", () => {
// reviewFileKey({path, repository_name: ""}) returns the bare path,
// so split should mirror that — no NUL in input means repo is "".
it("splits an explicit empty repository_name root key", () => {
expect(splitReviewFileKey(`${SEP}${APP_PATH}`)).toEqual({
repositoryName: "",
path: APP_PATH,
});
});
it("keeps a legacy bare-path key without a repository scope", () => {
expect(splitReviewFileKey(APP_PATH)).toEqual({ repositoryName: "", path: APP_PATH });
});
it("round-trips through reviewFileKey", () => {
const cases = [
{ path: "README.md" },
{ path: "root.md", repository_name: "" },
{ path: "src/index.ts", repository_name: "frontend" },
{ path: "deep/nested/path/file.go", repository_name: "backend" },
{ path: "with spaces/file.md", repository_name: "shared" },
+149 -46
View File
@@ -22,6 +22,8 @@ export type ReviewFile = {
repository_name?: string;
/** Exact git ref used as the old side when this committed patch was built. */
base_ref?: string;
/** True when this file belongs to an initialized Git submodule scope. */
is_submodule?: boolean;
};
/**
@@ -31,12 +33,15 @@ export type ReviewFile = {
* impossible to embed in a real path or repository name, so the key is
* always uniquely splittable. Single-repo files (no `repository_name`)
* keep the legacy bare-path key for backwards compatibility with existing
* `session_file_reviews` rows.
* `session_file_reviews` rows. An explicit empty name represents the real
* workspace root in a multi-repo payload and therefore keeps the separator.
*/
const FILE_KEY_SEP = "\u0000";
export function reviewFileKey(file: { path: string; repository_name?: string }): string {
return file.repository_name ? `${file.repository_name}${FILE_KEY_SEP}${file.path}` : file.path;
return file.repository_name === undefined
? file.path
: `${file.repository_name}${FILE_KEY_SEP}${file.path}`;
}
/** Mirrors backend `worktree.SanitizeRepoDirName`, which defines the
@@ -73,11 +78,40 @@ export function isReviewMultiRepo(
): boolean {
if (taskRepositoryCount > 1) return true;
const namedRepositories = new Set<string>();
let hasWorkspaceRoot = false;
for (const name of repositoryNames) {
if (!name) hasWorkspaceRoot = true;
if (name) namedRepositories.add(name);
if (namedRepositories.size > 1) return true;
}
return false;
return hasWorkspaceRoot && namedRepositories.size > 0;
}
/**
* A parent repository reports a changed submodule as a gitlink file at the
* child's workspace-relative scope. Once the child contributes any file,
* that synthetic parent row is redundant; when it contributes nothing, the
* row is the only available evidence and must remain visible.
*/
export function suppressAvailableGitlinkFiles(files: ReviewFile[]): ReviewFile[] {
const childScopesWithFiles = new Set<string>();
for (const file of files) {
if (file.repository_name) childScopesWithFiles.add(file.repository_name);
}
if (childScopesWithFiles.size === 0) return files;
const gitlinkKeys = new Set<string>();
for (const childScope of childScopesWithFiles) {
const boundaries = [
-1,
...Array.from(childScope.matchAll(/\//g), (match) => match.index ?? -1),
];
for (const boundary of boundaries) {
const parentScope = boundary < 0 ? "" : childScope.slice(0, boundary);
const childPath = childScope.slice(parentScope.length + (parentScope ? 1 : 0));
gitlinkKeys.add(reviewFileKey({ path: childPath, repository_name: parentScope }));
}
}
return files.filter((file) => !gitlinkKeys.has(reviewFileKey(file)));
}
export function getCumulativeReviewRepositoryNames(
@@ -167,6 +201,10 @@ export type FileTreeNode = {
isRepoRoot?: boolean;
/** Repository id this node represents (only set when isRepoRoot is true). */
repositoryId?: string;
/** True when this node is the boundary of an initialized submodule scope. */
isSubmodule?: boolean;
/** Full task-workspace-relative repository scope for a submodule boundary. */
repositoryName?: string;
};
/**
@@ -203,58 +241,123 @@ export const hashDiff = djb2Hash;
* file paths inside them stay relative to the repo root.
*/
export function buildFileTree(files: ReviewFile[]): FileTreeNode[] {
const repoNames = new Set<string>();
for (const f of files) {
if (f.repository_name) repoNames.add(f.repository_name);
}
const isMultiRepo = repoNames.size > 1;
if (isMultiRepo) return buildMultiRepoTree(files);
if (files.some((file) => Boolean(file.repository_name))) return buildScopedTree(files);
return buildFlatTree(files);
}
function buildMultiRepoTree(files: ReviewFile[]): FileTreeNode[] {
// Group by repo first, then build a sub-tree for each.
const byRepo = new Map<string, { name: string; id: string; files: ReviewFile[] }>();
for (const f of files) {
const name = f.repository_name ?? "(unspecified)";
const id = f.repository_id ?? name;
const key = id;
let entry = byRepo.get(key);
if (!entry) {
entry = { name, id, files: [] };
byRepo.set(key, entry);
function buildScopedTree(files: ReviewFile[]): FileTreeNode[] {
const scopeNames = new Set(
files.map((file) => file.repository_name).filter((name): name is string => Boolean(name)),
);
const hasWorkspaceRootFiles = files.some((file) => !file.repository_name);
const topLevelRepoRoots = new Set<string>();
if (!hasWorkspaceRootFiles && scopeNames.size > 1) {
for (const scope of scopeNames) topLevelRepoRoots.add(scope.split("/")[0]);
}
const repositoryIds = new Map<string, string>();
for (const file of files) {
if (file.repository_name && file.repository_id) {
repositoryIds.set(file.repository_name, file.repository_id);
}
entry.files.push(f);
}
const repoRoots: FileTreeNode[] = [];
for (const [, entry] of byRepo) {
const repoPath = `__repo__:${entry.id}`;
const subtree = buildFlatTree(entry.files);
// Two repos can contain a same-named path ("src/foo.ts" in both
// frontend and backend). Prefix every node path under this repo with
// the repo root path so tree-node paths are globally unique — required
// for useTree's expansion Set to track them independently.
const prefixedSubtree = subtree.map((n) => prefixPaths(n, repoPath));
repoRoots.push({
name: entry.name,
path: repoPath,
isDir: true,
isRepoRoot: true,
repositoryId: entry.id,
children: prefixedSubtree,
});
const root: FileTreeNode = { name: "", path: "", isDir: true, children: [] };
const context: ScopedTreeContext = {
scopeNames,
topLevelRepoRoots,
repositoryIds,
};
for (const file of files) {
appendScopedFile(root, file, context);
}
// Stable alphabetical order by repo name.
return repoRoots.sort((a, b) => a.name.localeCompare(b.name));
return collapseScopedTree(root);
}
function prefixPaths(node: FileTreeNode, prefix: string): FileTreeNode {
return {
...node,
path: `${prefix}/${node.path}`,
children: node.children?.map((c) => prefixPaths(c, prefix)),
type ScopedTreeContext = {
scopeNames: Set<string>;
topLevelRepoRoots: Set<string>;
repositoryIds: Map<string, string>;
};
function appendScopedFile(root: FileTreeNode, file: ReviewFile, context: ScopedTreeContext) {
const { scopeNames, topLevelRepoRoots, repositoryIds } = context;
const scope = file.repository_name ?? "";
const scopeParts = scope ? scope.split("/") : [];
const parts = [...scopeParts, ...file.path.split("/")];
let current = root;
for (let i = 0; i < parts.length; i++) {
const part = parts[i];
const logicalPath = parts.slice(0, i + 1).join("/");
if (i === parts.length - 1) {
current.children!.push({ name: part, path: logicalPath, isDir: false, file });
continue;
}
const isScopeBoundary = Boolean(scope) && i + 1 === scopeParts.length;
const isTopLevelScope = i === 0 && topLevelRepoRoots.has(part);
const child = getOrAppendScopedDirectorySegment(current, part, logicalPath);
if (isScopeBoundary && scopeNames.has(scope)) {
child.isSubmodule = !isTopLevelScope;
child.repositoryName = scope;
}
if (isTopLevelScope) {
child.isRepoRoot = true;
child.repositoryName = isScopeBoundary && scopeNames.has(scope) ? scope : part;
child.repositoryId = repositoryIds.get(part);
}
current = child;
}
}
function getOrAppendScopedDirectorySegment(
parent: FileTreeNode,
name: string,
logicalPath: string,
): FileTreeNode {
const existing = parent.children!.find(
(child) => child.isDir && child.name === name && child.path === logicalPath,
);
if (existing) return existing;
const child: FileTreeNode = {
name,
path: logicalPath,
isDir: true,
children: [],
};
parent.children!.push(child);
return child;
}
function collapseScopedTree(root: FileTreeNode): FileTreeNode[] {
function collapse(node: FileTreeNode): FileTreeNode {
if (!node.isDir || !node.children) return node;
node.children = node.children.map(collapse);
if (
node.children.length === 1 &&
node.children[0].isDir &&
node.name !== "" &&
!node.isRepoRoot &&
!node.isSubmodule &&
!node.children[0].isSubmodule
) {
const child = node.children[0];
return {
...child,
name: `${node.name}/${child.name}`,
};
}
return node;
}
return collapse(root).children ?? [];
}
function getOrAppendDirectorySegment(
@@ -93,6 +93,7 @@ type WorkingTreeProps = Pick<
| "unstagedFiles"
| "stagedFiles"
| "pendingStageFiles"
| "isLoading"
| "loadingOperation"
| "dialogs"
| "onOpenDiffFile"
@@ -120,7 +121,7 @@ function WorkingTreeSections(props: WorkingTreeProps) {
files={props.unstagedFiles}
pendingStageFiles={props.pendingStageFiles}
actionLabel="Stage all"
isActionLoading={isBulkOp && props.loadingOperation === "stage"}
isActionLoading={props.isLoading || (isBulkOp && props.loadingOperation === "stage")}
onAction={props.onStageAll}
onOpenDiff={props.onOpenDiffFile}
onEditFile={props.onEditFile}
@@ -139,10 +140,12 @@ function WorkingTreeSections(props: WorkingTreeProps) {
files={props.stagedFiles}
pendingStageFiles={props.pendingStageFiles}
actionLabel="Commit"
isActionLoading={props.loadingOperation === "commit"}
isActionLoading={props.isLoading || props.loadingOperation === "commit"}
onAction={() => props.dialogs.openCommitDialog()}
secondaryActionLabel="Unstage all"
isSecondaryActionLoading={isBulkOp && props.loadingOperation === "unstage"}
isSecondaryActionLoading={
props.isLoading || (isBulkOp && props.loadingOperation === "unstage")
}
onSecondaryAction={props.onUnstageAll}
onOpenDiff={props.onOpenDiffFile}
onEditFile={props.onEditFile}
@@ -121,12 +121,13 @@ function addReviewSource(
diff?: string;
},
): void {
const collidesWithHigherPriority = source.repositoryName
? winningDiffs.has(source.path)
const isScoped = source.repositoryName !== undefined;
const collidesWithHigherPriority = isScoped
? source.repositoryName !== "" && winningDiffs.has(source.path)
: paths.has(source.path);
if (winningDiffs.has(source.key) || collidesWithHigherPriority) return;
winningDiffs.set(source.key, source.diff);
paths.add(source.path);
if (!isScoped) paths.add(source.path);
}
function buildReviewProgressIndex(
@@ -32,6 +32,7 @@ export function RepoGroupItem({
onRepoAction,
onRepoSecondaryAction,
displayName,
disabled = false,
}: {
group: RepoGroup;
collapsed: boolean;
@@ -43,6 +44,7 @@ export function RepoGroupItem({
onRepoSecondaryAction?: (repo: string) => void;
/** Optional display label override; defaults to group.repositoryName. */
displayName?: string;
disabled?: boolean;
}) {
const stop = (e: React.MouseEvent) => e.stopPropagation();
const label = displayName || group.repositoryName || "Repository";
@@ -74,6 +76,7 @@ export function RepoGroupItem({
variant="ghost"
className="h-5 text-[10px] px-1.5 cursor-pointer"
data-testid="repo-group-action"
disabled={disabled}
onClick={() => onRepoAction(group.repositoryName)}
>
{primaryLabel}
@@ -85,6 +88,7 @@ export function RepoGroupItem({
variant="ghost"
className="h-5 text-[10px] px-1.5 cursor-pointer text-muted-foreground"
data-testid="repo-group-secondary-action"
disabled={disabled}
onClick={() => onRepoSecondaryAction(group.repositoryName)}
>
{secondaryLabel}
@@ -107,11 +111,13 @@ export function FileSectionActions({
secondaryLabel,
onAction,
onSecondaryAction,
disabled = false,
}: {
primaryLabel: string;
secondaryLabel?: string;
onAction?: (repo: string) => void;
onSecondaryAction?: (repo: string) => void;
disabled?: boolean;
}) {
if (!onAction && !onSecondaryAction) return null;
return (
@@ -122,6 +128,7 @@ export function FileSectionActions({
variant="ghost"
className="h-5 text-[10px] px-1.5 cursor-pointer"
data-testid="repo-group-action"
disabled={disabled}
onClick={() => onAction("")}
>
{primaryLabel}
@@ -133,6 +140,7 @@ export function FileSectionActions({
variant="ghost"
className="h-5 text-[10px] px-1.5 cursor-pointer text-muted-foreground"
data-testid="repo-group-secondary-action"
disabled={disabled}
onClick={() => onSecondaryAction("")}
>
{secondaryLabel}
@@ -248,6 +248,8 @@ type FileListSectionProps = {
onRepoSecondaryAction?: (repo: string) => void;
/** Maps a repository_name to its display label; called per group header. */
repoDisplayName?: (repositoryName: string) => string | undefined;
/** Disable section and per-repository actions while another git operation runs. */
disabled?: boolean;
};
/**
@@ -274,6 +276,7 @@ type FileListBodyProps = {
secondaryLabel?: string;
/** Maps a repository_name to its display label (e.g. "" → workspace primary repo name). */
repoDisplayName?: (repositoryName: string) => string | undefined;
disabled?: boolean;
};
function FileListBody(props: FileListBodyProps) {
@@ -391,6 +394,7 @@ function TreeFileListBody(props: FileListBranchProps) {
secondaryLabel={props.secondaryLabel}
onRepoAction={props.onRepoAction}
onRepoSecondaryAction={props.onRepoSecondaryAction}
disabled={props.disabled}
multiSelect={multiSelect}
/>
))
@@ -425,6 +429,7 @@ function FlatFileListBody(
onRepoAction={props.onRepoAction}
onRepoSecondaryAction={props.onRepoSecondaryAction}
displayName={props.repoDisplayName?.(group.repositoryName)}
disabled={props.disabled}
/>
))}
</ul>
@@ -476,6 +481,7 @@ export function FileListSection(props: FileListSectionProps) {
secondaryLabel={props.secondaryActionLabel}
onAction={props.onRepoAction}
onSecondaryAction={props.onRepoSecondaryAction}
disabled={props.isActionLoading || props.isSecondaryActionLoading}
/>
) : undefined
}
@@ -497,6 +503,7 @@ export function FileListSection(props: FileListSectionProps) {
onRepoAction={props.onRepoAction}
onRepoSecondaryAction={props.onRepoSecondaryAction}
repoDisplayName={props.repoDisplayName}
disabled={props.isActionLoading || props.isSecondaryActionLoading}
/>
)}
{files.length > 0 && hasSelection && (
@@ -181,6 +181,7 @@ type RepoTreeGroupProps = {
secondaryLabel?: string;
onRepoAction?: (repo: string) => void;
onRepoSecondaryAction?: (repo: string) => void;
disabled?: boolean;
multiSelect: ReturnType<typeof useMultiSelect>;
};
@@ -197,6 +198,7 @@ export function RepoTreeGroup(props: RepoTreeGroupProps) {
secondaryLabel,
onRepoAction,
onRepoSecondaryAction,
disabled = false,
} = props;
const label = displayName || repositoryName || "Repository";
const stop = (e: React.MouseEvent) => e.stopPropagation();
@@ -232,6 +234,7 @@ export function RepoTreeGroup(props: RepoTreeGroupProps) {
variant="ghost"
className="h-5 text-[10px] px-1.5 cursor-pointer"
data-testid="repo-group-action"
disabled={disabled}
onClick={() => onRepoAction(repositoryName)}
>
{primaryLabel}
@@ -243,6 +246,7 @@ export function RepoTreeGroup(props: RepoTreeGroupProps) {
variant="ghost"
className="h-5 text-[10px] px-1.5 cursor-pointer text-muted-foreground"
data-testid="repo-group-secondary-action"
disabled={disabled}
onClick={() => onRepoSecondaryAction(repositoryName)}
>
{secondaryLabel}
@@ -192,6 +192,7 @@ function useEnvSwitchCleanup(
effectiveSessionId: string | null,
effectiveEnvId: string | null,
activeTaskId: string | null,
initialLayout?: string | null,
) {
const prevEnvRef = useRef<string | null | undefined>(undefined);
const prevTaskRef = useRef<string | null | undefined>(undefined);
@@ -244,9 +245,9 @@ function useEnvSwitchCleanup(
if (effectiveSessionId && !currentSessionIds.includes(effectiveSessionId)) {
currentSessionIds.unshift(effectiveSessionId);
}
performLayoutSwitch(oldEnvId, newEnvId, effectiveSessionId, currentSessionIds);
performLayoutSwitch(oldEnvId, newEnvId, effectiveSessionId, currentSessionIds, initialLayout);
}
}, [effectiveEnvId, effectiveSessionId, activeTaskId, currentSessionIdsKey]);
}, [effectiveEnvId, effectiveSessionId, activeTaskId, currentSessionIdsKey, initialLayout]);
}
// ---------------------------------------------------------------------------
@@ -408,7 +409,7 @@ export const DockviewDesktopLayout = memo(function DockviewDesktopLayout({
// IMPORTANT: this must run BEFORE useAutoSessionTab so the old layout is
// saved before a new session tab is created — otherwise the new session's
// panel could leak into the old session's persisted layout.
useEnvSwitchCleanup(effectiveSessionId, effectiveEnvId, activeTaskId);
useEnvSwitchCleanup(effectiveSessionId, effectiveEnvId, activeTaskId, initialLayout);
useAutoSessionTab(effectiveSessionId);
useChangesPanelAutoFocus(changesFocusKey);
@@ -1,6 +1,6 @@
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
import { renderHook } from "@testing-library/react";
import { scrollToFileAndClear } from "./task-changes-panel";
import { discardTargetFromReviewFileKey, scrollToFileAndClear } from "./task-changes-panel";
import {
filterVisibleFiles,
resolveSelectedFileRepositoryName,
@@ -13,6 +13,15 @@ import type { ReviewFile } from "@/components/review/types";
const PATH = "src/foo.ts";
describe("discardTargetFromReviewFileKey", () => {
it("preserves an explicit workspace-root repository scope", () => {
expect(discardTargetFromReviewFileKey("\u0000README.md")).toEqual({
repositoryName: "",
path: "README.md",
});
});
});
describe("shouldCloseFileDiffPanel", () => {
it("returns false when gitStatus is undefined (not loaded yet)", () => {
expect(shouldCloseFileDiffPanel(undefined, PATH)).toBe(false);
@@ -76,6 +76,13 @@ function scrollToFileAndClear(
}
}
export function discardTargetFromReviewFileKey(key: string): {
repositoryName: string;
path: string;
} {
return splitReviewFileKey(key);
}
function useChangesView(
selectedDiff: SelectedDiff | null,
onClearSelected: () => void,
@@ -260,9 +267,9 @@ function useChangesActions(
const handleDiscard = useCallback(
async (key: string) => {
const { path } = splitReviewFileKey(key);
const { repositoryName, path } = discardTargetFromReviewFileKey(key);
try {
const result = await discard([path]);
const result = await discard([path], repositoryName || undefined);
if (result.success) {
toast({ title: "Changes discarded", description: path, variant: "success" });
} else {
@@ -106,4 +106,29 @@ describe("buildReviewGitStatusFiles", () => {
expect(result.isMultiRepo).toBe(true);
expect(Object.keys(result.files ?? {})).toEqual(["frontend\u0000README.md"]);
});
it("keeps the real root status when a submodule status is also present", () => {
const result = buildReviewGitStatusFiles(
undefined,
[
{
repository_name: "",
status: status({ "README.md": file() }),
},
{
repository_name: "vendor/lib",
status: status({ "src/lib.ts": file("src/lib.ts") }),
},
],
1,
);
expect(result.isMultiRepo).toBe(true);
expect(Object.keys(result.files ?? {}).sort()).toEqual([
"\u0000README.md",
"vendor/lib\u0000src/lib.ts",
]);
expect(result.files?.["\u0000README.md"]?.repository_name).toBe("");
expect(result.files?.["vendor/lib\u0000src/lib.ts"]?.repository_name).toBe("vendor/lib");
});
});
+40 -12
View File
@@ -13,7 +13,11 @@ import { usePRDiff } from "@/hooks/domains/github/use-pr-diff";
import { usePRReviewRepositoryIdentity } from "@/hooks/domains/github/use-pr-review-repository-identity";
import { useTaskRepositories } from "@/hooks/domains/kanban/use-task-repositories";
import { formatReviewCommentsAsMarkdown } from "@/components/task/chat/messages/review-comments-attachment";
import { getCumulativeReviewRepositoryNames, isReviewMultiRepo } from "@/components/review/types";
import {
getCumulativeReviewRepositoryNames,
isReviewMultiRepo,
reviewFileKey,
} from "@/components/review/types";
import { getWebSocketClient } from "@/lib/ws/connection";
import { generateUUID } from "@/lib/utils";
import { useToast } from "@/components/toast-provider";
@@ -25,6 +29,24 @@ type ReviewGitStatusFiles = {
isMultiRepo: boolean;
};
function buildMultiRepoReviewFiles(
statusByRepo: Array<{ repository_name: string; status: GitStatusEntry }>,
): Record<string, FileInfo> {
const files: Record<string, FileInfo> = {};
for (const { repository_name, status } of statusByRepo) {
if (!status?.files) continue;
for (const [path, file] of Object.entries(status.files)) {
const key = reviewFileKey({ path, repository_name });
files[key] = {
...file,
repository_name,
is_submodule: file.is_submodule ?? status.is_submodule,
};
}
}
return files;
}
export function buildReviewGitStatusFiles(
reviewGitStatus: GitStatusEntry | undefined,
statusByRepo: Array<{ repository_name: string; status: GitStatusEntry }>,
@@ -34,11 +56,22 @@ export function buildReviewGitStatusFiles(
const named = statusByRepo.filter((entry) => entry.repository_name !== "");
const isMultiRepo = isReviewMultiRepo(
taskRepositoryCount,
named.map((entry) => entry.repository_name).concat(Array.from(cumulativeRepositoryNames)),
statusByRepo
.map((entry) => entry.repository_name)
.concat(Array.from(cumulativeRepositoryNames)),
);
if (!isMultiRepo) {
const sourceStatus = reviewGitStatus?.files ? reviewGitStatus : named[0]?.status;
const files = sourceStatus?.files
? Object.fromEntries(
Object.entries(sourceStatus.files).map(([path, file]) => [
path,
{ ...file, is_submodule: file.is_submodule ?? sourceStatus.is_submodule },
]),
)
: null;
return {
files: reviewGitStatus?.files ?? named[0]?.status.files ?? null,
files,
isMultiRepo: false,
};
}
@@ -49,13 +82,7 @@ export function buildReviewGitStatusFiles(
};
}
const files: Record<string, FileInfo> = {};
for (const { repository_name, status } of named) {
if (!status?.files) continue;
for (const [path, file] of Object.entries(status.files)) {
files[`${repository_name}\u0000${path}`] = { ...file, repository_name };
}
}
const files = buildMultiRepoReviewFiles(statusByRepo);
return {
files: Object.keys(files).length > 0 ? files : null,
isMultiRepo: true,
@@ -66,8 +93,9 @@ export function buildReviewGitStatusFiles(
* Builds the unified gitStatus.files map fed into the ReviewDialog. Multi-repo
* tasks have one git status per repo, and two repos can have files at the same
* relative path (`README.md` in both), so the map key is `repo\u0000path` and
* every FileInfo is stamped with its `repository_name`. Single-repo tasks keep
* the legacy path-only keying.
* every FileInfo is stamped with its `repository_name`, including the explicit
* empty scope for the real workspace root. Single-repo tasks keep the legacy
* path-only keying.
*/
function useReviewGitStatusFiles(
sessionId: string | null,
+1 -1
View File
@@ -338,7 +338,7 @@ const VcsSplitButton = memo(function VcsSplitButton({
const isGitLoading = git.isLoading;
// Multi-repo when there's more than one named repo. Single-repo workspaces
// get either a single empty-name entry or no entries at all in repoNames.
const isMultiRepo = git.repoNames.filter((r) => r !== "").length > 1;
const isMultiRepo = git.repoNames.length > 1;
const primaryAction = determinePrimaryAction(
uncommittedFileCount,
+6 -4
View File
@@ -26,13 +26,15 @@ describe("pickRepoLabel", () => {
});
it("translates the multi-repo fan-out label when no repo is scoped", () => {
expect(pickRepoLabel("", true, noDisplayName, t)).toBe(t("integrations:allRepos"));
expect(pickRepoLabel("", true, noDisplayName, t)).toBe("All repos");
expect(pickRepoLabel(undefined, true, noDisplayName, t)).toBe(t("integrations:allRepos"));
});
it("translates an explicit workspace-root scope separately from fan-out", () => {
expect(pickRepoLabel("", true, noDisplayName, t)).toBe(t("integrations:repository"));
});
it("translates the single-repo fallback when the primary repo has no display name", () => {
expect(pickRepoLabel("", false, noDisplayName, t)).toBe(t("integrations:repository"));
expect(pickRepoLabel("", false, noDisplayName, t)).toBe("Repository");
expect(pickRepoLabel(undefined, false, noDisplayName, t)).toBe(t("integrations:repository"));
});
it("prefers the primary repo's display name over the translated fallback", () => {
+25 -30
View File
@@ -233,9 +233,9 @@ type UseCommitDialogReturn = {
setBody: (v: string) => void;
stageAll: boolean;
setStageAll: (v: boolean) => void;
/** Repo this commit is scoped to in multi-repo mode; "" = all repos with staged. */
repo: string;
setRepo: (v: string) => void;
/** Undefined means all repos; "" is an explicit workspace-root scope. */
repo: string | undefined;
setRepo: (v: string | undefined) => void;
openDialog: (repo?: string) => void;
};
@@ -244,14 +244,14 @@ function useCommitDialogState(): UseCommitDialogReturn {
const [message, setMessage] = useState("");
const [body, setBody] = useState("");
const [stageAll, setStageAll] = useState(false);
const [repo, setRepo] = useState("");
const [repo, setRepo] = useState<string | undefined>(undefined);
const openDialog = useCallback((nextRepo?: string) => {
setMessage("");
setBody("");
setStageAll(false);
// Defensive: callers binding `openDialog` directly to onClick can leak the
// React MouseEvent into nextRepo. Only accept actual repo strings.
setRepo(typeof nextRepo === "string" ? nextRepo : "");
setRepo(typeof nextRepo === "string" ? nextRepo : undefined);
setOpen(true);
}, []);
return {
@@ -280,9 +280,9 @@ type UsePRDialogReturn = {
setDraft: (v: boolean) => void;
branchPushed: boolean;
setBranchPushed: (v: boolean) => void;
/** Repo this PR is scoped to in multi-repo mode; "" = workspace root. */
repo: string;
setRepo: (v: string) => void;
/** Undefined means the default scope; "" is an explicit workspace-root scope. */
repo: string | undefined;
setRepo: (v: string | undefined) => void;
openDialog: (taskTitle?: string, repo?: string) => void;
};
@@ -292,14 +292,14 @@ function usePRDialogState(): UsePRDialogReturn {
const [body, setBody] = useState("");
const [draft, setDraft] = useState(true);
const [branchPushed, setBranchPushed] = useState(false);
const [repo, setRepo] = useState("");
const [repo, setRepo] = useState<string | undefined>(undefined);
const openDialog = useCallback((taskTitle?: string, nextRepo?: string) => {
setTitle(taskTitle || "");
setBody("");
setBranchPushed(false);
// Defensive: callers binding `openDialog` directly to onClick can leak the
// React MouseEvent into nextRepo. Only accept actual repo strings.
setRepo(typeof nextRepo === "string" ? nextRepo : "");
setRepo(typeof nextRepo === "string" ? nextRepo : undefined);
setOpen(true);
}, []);
return {
@@ -321,8 +321,8 @@ function usePRDialogState(): UsePRDialogReturn {
/**
* Computes the file summary for the commit dialog: an explicit repo scope
* uses that repo's files; an empty scope in multi-repo sums across every
* repo (showing the fan-out total); single-repo falls back to the legacy
* uses that repo's files; no scope in multi-repo sums across every repo
* (showing the fan-out total); single-repo falls back to the legacy
* workspace-level status.
*/
function useScopedFileSummary({
@@ -332,7 +332,7 @@ function useScopedFileSummary({
isMultiRepo,
stageAll,
}: {
scopedRepo: string;
scopedRepo: string | undefined;
statusByRepo: ReturnType<typeof useSessionGitStatusByRepo>;
gitStatus: ReturnType<typeof useSessionGitStatus>;
isMultiRepo: boolean;
@@ -340,7 +340,7 @@ function useScopedFileSummary({
stageAll: boolean;
}): FileSummary {
return useMemo(() => {
if (scopedRepo) {
if (scopedRepo !== undefined) {
const scoped = statusByRepo.find((s) => s.repository_name === scopedRepo);
return computeFileSummary(scoped?.status?.files, stageAll);
}
@@ -366,7 +366,7 @@ function useScopedFileSummary({
* repos" when the workspace has multiple repos and the dialog is fanning out.
*/
export function pickRepoLabel(
scopedRepo: string,
scopedRepo: string | undefined,
isMultiRepo: boolean,
resolveDisplayName: (name: string) => string | undefined,
// Structural, matching the other translate-taking helpers in the app: this
@@ -374,7 +374,11 @@ export function pickRepoLabel(
// from `@/lib/i18n` qualify. The branded `TFunction` excluded the latter.
t: (key: string) => string,
): string {
if (scopedRepo) return resolveDisplayName(scopedRepo) || scopedRepo;
if (scopedRepo !== undefined) {
return scopedRepo
? resolveDisplayName(scopedRepo) || scopedRepo
: resolveDisplayName("") || t("integrations:repository");
}
if (isMultiRepo) return t("integrations:allRepos");
return resolveDisplayName("") || t("integrations:repository");
}
@@ -393,13 +397,7 @@ function useCreatePRHandler(
if (!ps.title.trim()) return;
ps.setOpen(false);
try {
const result = await createPR(
ps.title.trim(),
ps.body.trim(),
baseBranch,
ps.draft,
ps.repo || undefined,
);
const result = await createPR(ps.title.trim(), ps.body.trim(), baseBranch, ps.draft, ps.repo);
if (result.success) {
const terms = resolveChangeRequestTerminology(result.provider, defaultTerminology);
const title = ps.draft
@@ -464,8 +462,8 @@ function useVcsDialogsState(
// tasks because the task root isn't itself a git repo (exit 1).
const { commit, createPR, repoNames, isLoading: isGitLoading } = useSessionGit(sessionId);
const repoDisplayName = useRepoDisplayName(sessionId);
const isMultiRepo = repoNames.filter((r) => r !== "").length > 1;
const changeRequestTerminology = useChangeRequestTerminology(sessionId, ps.repo || undefined);
const isMultiRepo = repoNames.length > 1;
const changeRequestTerminology = useChangeRequestTerminology(sessionId, ps.repo);
const fileSummary = useScopedFileSummary({
scopedRepo: cs.repo,
statusByRepo,
@@ -484,13 +482,10 @@ function useVcsDialogsState(
// hook is not migrated. Translating only the label would produce a
// half-translated toast.
const label = cs.repo ? `Commit (${cs.repo})` : "Commit";
await gitWithFeedback(
() => commit(fullMessage, cs.stageAll, false, cs.repo || undefined),
label,
);
await gitWithFeedback(() => commit(fullMessage, cs.stageAll, false, cs.repo), label);
cs.setMessage("");
cs.setBody("");
cs.setRepo("");
cs.setRepo(undefined);
}, [cs, gitWithFeedback, commit]);
const handleCreatePR = useCreatePRHandler(
ps,
@@ -0,0 +1,88 @@
import { test, expect } from "../../fixtures/test-base";
import { SessionPage } from "../../pages/session-page";
import { createSubmoduleReviewFixture } from "./submodule-review-helpers";
test.describe("Nested submodule Review on mobile", () => {
test.describe.configure({ retries: 1, timeout: 180_000 });
test("keeps nested scope and diff context touch-reachable", async ({
testPage,
apiClient,
seedData,
backend,
}) => {
const fixture = await createSubmoduleReviewFixture(
apiClient,
seedData,
backend.tmpDir,
"Mobile nested submodule Review E2E",
);
try {
await testPage.goto(`/t/${fixture.taskId}`);
const session = new SessionPage(testPage);
await session.waitForLoad();
await session.waitForChatIdle({ timeout: 45_000 });
const worktreePath = await fixture.waitForWorktree(apiClient);
fixture.applyNestedChanges(worktreePath);
await testPage.getByRole("button", { name: "Changes" }).tap();
const changesPanel = testPage.getByTestId("mobile-changes-panel");
await expect(changesPanel).toBeVisible({ timeout: 15_000 });
await expect(changesPanel.getByText("README.md").first()).toBeVisible({
timeout: 45_000,
});
await changesPanel.getByRole("button", { name: "Review", exact: true }).tap();
const review = session.reviewDialog();
await expect(review).toBeVisible({ timeout: 15_000 });
const repositoryLabels = review.getByTestId("review-file-repository");
await expect(repositoryLabels).toHaveCount(2);
await expect(repositoryLabels.filter({ hasText: /^vendor\/outer$/ })).toBeVisible();
const innerLabel = repositoryLabels.filter({
hasText: /^vendor\/outer\/vendor\/inner$/,
});
await expect(innerLabel).toBeVisible({ timeout: 15_000 });
const innerHeader = review
.getByTestId("review-file-header")
.filter({ hasText: "vendor/outer/vendor/inner" });
await expect(innerHeader).toBeVisible({ timeout: 15_000 });
await innerHeader.scrollIntoViewIfNeeded();
await expect
.poll(() => session.reviewDiffText(), { timeout: 45_000 })
.toContain("inner committed change");
const nestedScopeHeaders = review.locator(
'[data-testid="changes-repo-header"][data-submodule-scope="true"]',
);
await expect(nestedScopeHeaders).toHaveCount(2);
await expect(
nestedScopeHeaders.filter({ hasText: "vendor/outer/vendor/inner" }),
).toBeVisible();
const [dialogBox, innerLabelBox, viewport] = await Promise.all([
review.boundingBox(),
innerLabel.boundingBox(),
testPage.evaluate(() => ({ width: window.innerWidth, height: window.innerHeight })),
]);
if (!dialogBox || !innerLabelBox) {
throw new Error("Nested submodule Review geometry is unavailable");
}
expect(dialogBox.x).toBeGreaterThanOrEqual(0);
expect(dialogBox.y).toBeGreaterThanOrEqual(0);
expect(dialogBox.x + dialogBox.width).toBeLessThanOrEqual(viewport.width + 1);
expect(dialogBox.y + dialogBox.height).toBeLessThanOrEqual(viewport.height + 1);
expect(innerLabelBox.height).toBeGreaterThan(0);
const overflow = await testPage.evaluate(() => ({
viewport: document.documentElement.clientWidth,
document: document.documentElement.scrollWidth,
}));
expect(overflow.document).toBeLessThanOrEqual(overflow.viewport + 1);
} finally {
fixture.cleanup();
}
});
});
@@ -0,0 +1,148 @@
import crypto from "node:crypto";
import fs from "node:fs";
import path from "node:path";
import { execFileSync } from "node:child_process";
import type { ApiClient } from "../../helpers/api-client";
import type { SeedData } from "../../fixtures/test-base";
import { makeGitEnv } from "../../helpers/git-helper";
const GIT_PROTOCOL_ARGS = ["-c", "protocol.file.allow=always"];
export type SubmoduleReviewFixture = {
taskId: string;
sessionId: string;
sourceRoot: string;
waitForWorktree: (apiClient: ApiClient) => Promise<string>;
applyNestedChanges: (worktreePath: string) => void;
cleanup: () => void;
};
function runGit(repoPath: string, args: string[], env: NodeJS.ProcessEnv): string {
return execFileSync("git", args, {
cwd: repoPath,
env,
encoding: "utf8",
stdio: ["ignore", "pipe", "pipe"],
});
}
function initializeRepository(
repoPath: string,
env: NodeJS.ProcessEnv,
fileName: string,
content: string,
) {
fs.mkdirSync(repoPath, { recursive: true });
runGit(repoPath, ["init", "-b", "main"], env);
runGit(repoPath, ["config", "protocol.file.allow", "always"], env);
fs.writeFileSync(path.join(repoPath, fileName), content);
runGit(repoPath, ["add", fileName], env);
runGit(repoPath, ["commit", "-m", "initial"], env);
}
export function readGitValue(repoPath: string, args: string[], tempRoot: string): string {
return runGit(repoPath, args, {
...makeGitEnv(tempRoot),
GIT_ALLOW_PROTOCOL: "file",
}).trim();
}
function commit(repoPath: string, env: NodeJS.ProcessEnv, message: string): void {
runGit(repoPath, ["add", "-A"], env);
runGit(repoPath, ["commit", "-m", message], env);
}
async function waitForWorktreePath(
apiClient: ApiClient,
taskId: string,
sessionId: string,
): Promise<string> {
const deadline = Date.now() + 90_000;
while (Date.now() < deadline) {
const { sessions } = await apiClient.listTaskSessions(taskId);
const session = sessions.find((candidate) => candidate.id === sessionId);
const worktreePath =
session?.worktree_path ?? session?.workspace_path ?? session?.worktrees?.[0]?.worktree_path;
if (worktreePath) return worktreePath;
await new Promise((resolve) => setTimeout(resolve, 250));
}
throw new Error(`Timed out waiting for the nested-review worktree for ${sessionId}`);
}
export async function createSubmoduleReviewFixture(
apiClient: ApiClient,
seedData: SeedData,
tempRoot: string,
title: string,
): Promise<SubmoduleReviewFixture> {
const sourceRoot = path.join(tempRoot, `submodule-review-${crypto.randomUUID()}`);
const parentPath = path.join(sourceRoot, "parent");
const outerPath = path.join(sourceRoot, "outer");
const innerPath = path.join(sourceRoot, "inner");
const env = { ...makeGitEnv(tempRoot), GIT_ALLOW_PROTOCOL: "file" };
initializeRepository(innerPath, env, "README.md", "inner base\n");
initializeRepository(outerPath, env, "README.md", "outer base\n");
runGit(outerPath, [...GIT_PROTOCOL_ARGS, "submodule", "add", "../inner", "vendor/inner"], env);
commit(outerPath, env, "add nested inner submodule");
initializeRepository(parentPath, env, "README.md", "parent base\n");
runGit(parentPath, [...GIT_PROTOCOL_ARGS, "submodule", "add", "../outer", "vendor/outer"], env);
runGit(parentPath, [...GIT_PROTOCOL_ARGS, "submodule", "update", "--init", "--recursive"], env);
runGit(
path.join(parentPath, "vendor/outer"),
[...GIT_PROTOCOL_ARGS, "submodule", "update", "--init", "--recursive"],
env,
);
commit(parentPath, env, "add outer submodule");
const repository = await apiClient.createRepository(seedData.workspaceId, parentPath, "main", {
name: "nested-submodule-parent",
});
const { executors } = await apiClient.listExecutors();
const directProfile = executors.find(
(executor) => executor.type === "local" || executor.type === "local_pc",
)?.profiles?.[0];
if (!directProfile) throw new Error("Nested submodule fixture needs a direct local executor");
const task = await apiClient.createTaskWithAgent(
seedData.workspaceId,
title,
seedData.agentProfileId,
{
description: "/e2e:simple-message",
workflow_id: seedData.workflowId,
workflow_step_id: seedData.startStepId,
repository_ids: [repository.id],
executor_profile_id: directProfile.id,
},
);
if (!task.session_id) throw new Error("Nested submodule fixture did not start a session");
return {
taskId: task.id,
sessionId: task.session_id,
sourceRoot,
waitForWorktree: (client) => waitForWorktreePath(client, task.id, task.session_id!),
applyNestedChanges(worktreePath: string) {
const outerWorktree = path.join(worktreePath, "vendor/outer");
const innerWorktree = path.join(outerWorktree, "vendor/inner");
if (!fs.existsSync(innerWorktree)) {
runGit(
worktreePath,
[...GIT_PROTOCOL_ARGS, "submodule", "update", "--init", "--recursive"],
env,
);
}
fs.appendFileSync(path.join(worktreePath, "README.md"), "parent working-tree change\n");
fs.appendFileSync(path.join(outerWorktree, "README.md"), "outer committed change\n");
commit(outerWorktree, env, "change outer submodule");
fs.appendFileSync(path.join(innerWorktree, "README.md"), "inner committed change\n");
commit(innerWorktree, env, "change inner submodule");
commit(outerWorktree, env, "record inner submodule change");
},
cleanup() {
fs.rmSync(sourceRoot, { recursive: true, force: true });
},
};
}
@@ -0,0 +1,144 @@
import { test, expect } from "../../fixtures/test-base";
import { SessionPage } from "../../pages/session-page";
import path from "node:path";
import { createSubmoduleReviewFixture, readGitValue } from "./submodule-review-helpers";
test.describe("Nested submodule Review", () => {
test.describe.configure({ retries: 1, timeout: 180_000 });
test("shows nested scopes and commits child gitlinks through the UI", async ({
testPage,
apiClient,
seedData,
backend,
}) => {
const fixture = await createSubmoduleReviewFixture(
apiClient,
seedData,
backend.tmpDir,
"Nested submodule Review E2E",
);
try {
await testPage.goto(`/t/${fixture.taskId}`);
const session = new SessionPage(testPage);
await session.waitForLoad();
await session.waitForChatIdle({ timeout: 45_000 });
const worktreePath = await fixture.waitForWorktree(apiClient);
fixture.applyNestedChanges(worktreePath);
const parentBaseSha = readGitValue(worktreePath, ["rev-parse", "HEAD"], backend.tmpDir);
await session.clickTab("Changes");
await expect(session.changes).toBeVisible({ timeout: 15_000 });
await expect
.poll(() => session.changes.getByTestId("file-row-README.md").count(), {
timeout: 45_000,
})
.toBeGreaterThan(0);
await testPage.evaluate(() => window.dispatchEvent(new CustomEvent("open-review-dialog")));
const review = session.reviewDialog();
await expect(review).toBeVisible({ timeout: 15_000 });
await expect
.poll(() => review.getByTestId("submodule-node").count(), { timeout: 45_000 })
.toBeGreaterThan(0);
const outerNode = review.locator(
'[data-testid="submodule-node"][data-repository-name="vendor/outer"]',
);
await expect(outerNode).toBeVisible();
const innerNode = review.locator(
'[data-testid="submodule-node"][data-repository-name="vendor/outer/vendor/inner"]',
);
if ((await innerNode.count()) === 0) {
await outerNode.getByRole("button").click();
}
await expect(innerNode).toBeVisible();
const readmeRows = review.locator(
'[data-testid="review-file-row"][data-file-path="README.md"]',
);
await expect(readmeRows).toHaveCount(3);
await expect
.poll(() => session.reviewDiffText(), { timeout: 45_000 })
.toEqual(expect.stringContaining("parent working-tree change"));
for (const expected of ["outer committed change", "inner committed change"]) {
await expect.poll(() => session.reviewDiffText(), { timeout: 45_000 }).toContain(expected);
}
await expect(
review.locator('[data-testid="review-file-row"][data-file-path="vendor/outer"]'),
).toHaveCount(0);
await expect(
review.locator(
'[data-testid="review-file-row"][data-file-path="vendor/inner"][data-repository-name="vendor/outer"]',
),
).toHaveCount(0);
await review.getByRole("button", { name: "Close review" }).click();
await expect(review).not.toBeVisible();
// Stage one root file to expose the global staged Commit action. The
// commit dialog's "Stage all" option then reruns staging in dependency
// order immediately before committing each scope.
const unstaged = session.changes.getByTestId("unstaged-files-section");
const stageAllButton = unstaged.getByRole("button", { name: "Stage all", exact: true });
await expect(stageAllButton).toBeVisible({ timeout: 20_000 });
await expect(stageAllButton).toBeEnabled({ timeout: 20_000 });
await stageAllButton.click();
const staged = session.changes.getByTestId("staged-files-section");
const commitButton = staged.getByRole("button", { name: "Commit", exact: true });
await expect(commitButton).toBeVisible({ timeout: 20_000 });
await expect(commitButton).toBeEnabled({ timeout: 20_000 });
await commitButton.click();
const commitDialog = testPage.getByRole("dialog");
await expect(commitDialog.getByTestId("commit-title-input")).toBeVisible();
await commitDialog.getByTestId("commit-title-input").fill("e2e nested submodule commit");
await commitDialog.getByRole("checkbox").check();
await commitDialog.getByRole("button", { name: "Commit", exact: true }).click();
await expect(commitDialog).not.toBeVisible({ timeout: 45_000 });
const outerPath = path.join(worktreePath, "vendor/outer");
const innerPath = path.join(outerPath, "vendor/inner");
const expectedOuterSha = readGitValue(outerPath, ["rev-parse", "HEAD"], backend.tmpDir);
const expectedInnerSha = readGitValue(innerPath, ["rev-parse", "HEAD"], backend.tmpDir);
// The dialog closes as soon as the request starts. Wait for the deepest
// commits and their parent gitlinks to settle before asserting the
// filesystem state, otherwise a successful dependency-wave commit can
// look flaky while the root request is still in flight.
await expect
.poll(
() => readGitValue(worktreePath, ["rev-parse", "HEAD:vendor/outer"], backend.tmpDir),
{ timeout: 45_000 },
)
.toBe(expectedOuterSha);
await expect
.poll(() => readGitValue(outerPath, ["rev-parse", "HEAD:vendor/inner"], backend.tmpDir), {
timeout: 45_000,
})
.toBe(expectedInnerSha);
await expect
.poll(() => readGitValue(worktreePath, ["status", "--porcelain"], backend.tmpDir), {
timeout: 45_000,
})
.toBe("");
await expect
.poll(() => readGitValue(outerPath, ["status", "--porcelain"], backend.tmpDir), {
timeout: 45_000,
})
.toBe("");
await expect
.poll(() => readGitValue(innerPath, ["status", "--porcelain"], backend.tmpDir), {
timeout: 45_000,
})
.toBe("");
expect(readGitValue(worktreePath, ["rev-parse", "HEAD"], backend.tmpDir)).not.toBe(
parentBaseSha,
);
} finally {
fixture.cleanup();
}
});
});
@@ -3,6 +3,8 @@ import { reviewFileKey } from "@/components/review/types";
import type { PRDiffFile } from "@/lib/types/github";
import { buildReviewSources, normalizeReviewStatusSources } from "./use-review-sources";
const NESTED_SCOPE = "vendor/lib";
const namedStatus = {
repository_name: "frontend",
status: {
@@ -12,6 +14,15 @@ const namedStatus = {
},
};
const nestedStatus = {
repository_name: NESTED_SCOPE,
status: {
files: {
"src/local.ts": { diff: "@@nested@@", status: "modified", additions: 1, deletions: 0 },
},
},
};
const prDiffFiles = [
{ filename: "src/pr.ts", status: "modified", patch: "@@pr@@" },
] as PRDiffFile[];
@@ -97,3 +108,149 @@ describe("review source key mode", () => {
]);
});
});
describe("nested scope review sources", () => {
it("retains a real workspace root alongside a named submodule scope", () => {
const rootStatus = {
repository_name: "",
status: {
files: {
"README.md": { diff: "@@root@@", status: "modified", additions: 1, deletions: 0 },
},
},
};
const normalized = normalizeReviewStatusSources({
gitStatus: undefined,
statusByRepo: [rootStatus, nestedStatus],
taskRepositoryCount: 1,
});
expect(normalized.useRepositoryKeys).toBe(true);
expect(normalized.normalizedStatusByRepo?.map((entry) => entry.repository_name)).toEqual([
"",
NESTED_SCOPE,
]);
const result = buildReviewSources({
gitStatus: normalized.normalizedGitStatus,
statusByRepo: normalized.normalizedStatusByRepo,
cumulativeDiff: null,
prDiffFiles: undefined,
useRepositoryKeys: normalized.useRepositoryKeys,
});
expect(result.allFiles.map(reviewFileKey)).toEqual([
"\u0000README.md",
`${NESTED_SCOPE}\u0000src/local.ts`,
]);
});
it("keeps same-named root and nested-scope cumulative files distinct", () => {
const result = buildReviewSources({
gitStatus: undefined,
statusByRepo: [
{
repository_name: "",
status: {
files: {
"README.md": {
diff: "@@root@@",
status: "modified",
additions: 1,
deletions: 0,
},
},
},
},
],
cumulativeDiff: {
files: {
[`${NESTED_SCOPE}\u0000README.md`]: {
path: "README.md",
repository_name: NESTED_SCOPE,
diff: "@@child@@",
status: "modified",
additions: 1,
deletions: 0,
},
},
},
prDiffFiles: undefined,
useRepositoryKeys: true,
});
expect(result.allFiles.map(reviewFileKey).sort()).toEqual([
"\u0000README.md",
`${NESTED_SCOPE}\u0000README.md`,
]);
});
it("keeps a root cumulative file beside a same-named child uncommitted file", () => {
const result = buildReviewSources({
gitStatus: undefined,
statusByRepo: [nestedStatus],
cumulativeDiff: {
files: {
"README.md": {
path: "README.md",
diff: "@@root-committed@@",
status: "modified",
additions: 1,
deletions: 0,
},
},
},
prDiffFiles: undefined,
useRepositoryKeys: true,
});
expect(result.allFiles.map(reviewFileKey).sort()).toEqual([
"README.md",
`${NESTED_SCOPE}\u0000src/local.ts`,
]);
expect(result.allFiles.find((file) => file.repository_name === undefined)?.source).toBe(
"committed",
);
expect(result.allFiles.find((file) => file.repository_name === NESTED_SCOPE)?.source).toBe(
"uncommitted",
);
});
});
describe("nested scope gitlink sources", () => {
it("suppresses an available parent gitlink but keeps an unavailable one", () => {
const rootStatus = {
repository_name: "",
status: {
files: {
[NESTED_SCOPE]: { diff: "@@gitlink@@", status: "modified", additions: 1, deletions: 1 },
},
},
};
const childStatus = {
repository_name: NESTED_SCOPE,
status: {
files: {
"README.md": { diff: "@@child@@", status: "modified", additions: 1, deletions: 0 },
},
},
};
const available = buildReviewSources({
gitStatus: undefined,
statusByRepo: [rootStatus, childStatus],
cumulativeDiff: null,
prDiffFiles: undefined,
useRepositoryKeys: true,
});
expect(available.allFiles.map(reviewFileKey)).toEqual([`${NESTED_SCOPE}\u0000README.md`]);
const unavailable = buildReviewSources({
gitStatus: undefined,
statusByRepo: [rootStatus],
cumulativeDiff: null,
prDiffFiles: undefined,
useRepositoryKeys: true,
});
expect(unavailable.allFiles.map(reviewFileKey)).toEqual([`\u0000${NESTED_SCOPE}`]);
});
});
@@ -0,0 +1,70 @@
import { describe, expect, it } from "vitest";
import { buildReviewSources } from "./use-review-sources";
const CHILD_SCOPE = "vendor/lib";
const README_PATH = "README.md";
describe("buildReviewSources scoped metadata", () => {
it("preserves submodule metadata on PR-only files", () => {
const result = buildReviewSources({
gitStatus: undefined,
statusByRepo: undefined,
cumulativeDiff: null,
prDiffFiles: [
{
filename: README_PATH,
status: "modified",
patch: "@@ -1 +1 @@\n-old\n+new\n",
additions: 1,
deletions: 1,
is_submodule: true,
},
],
prRepoName: CHILD_SCOPE,
});
expect(result.allFiles).toEqual([
expect.objectContaining({
path: README_PATH,
repository_name: CHILD_SCOPE,
source: "pr",
is_submodule: true,
}),
]);
});
it("keeps fallback root status distinct from a same-named child file", () => {
const result = buildReviewSources({
gitStatus: {
files: {
[README_PATH]: {
diff: "@@root@@",
status: "modified",
additions: 1,
deletions: 0,
},
},
},
statusByRepo: undefined,
cumulativeDiff: {
files: {
[`${CHILD_SCOPE}\u0000${README_PATH}`]: {
path: README_PATH,
repository_name: CHILD_SCOPE,
diff: "@@child@@",
status: "modified",
additions: 1,
deletions: 0,
},
},
},
prDiffFiles: undefined,
useRepositoryKeys: true,
});
expect(result.allFiles.map((file) => [file.repository_name, file.path])).toEqual([
["", README_PATH],
[CHILD_SCOPE, README_PATH],
]);
});
});
@@ -208,9 +208,11 @@ describe("buildReviewSources", () => {
},
},
prDiffFiles: prFiles,
useRepositoryKeys: false,
});
expect(result.allFiles).toHaveLength(1);
expect(result.allFiles[0].source).toBe("uncommitted");
expect(result.allFiles[0].repository_name).toBeUndefined();
expect(result.sourceCounts).toEqual({ uncommitted: 1, committed: 0, pr: 0 });
});
@@ -307,9 +309,11 @@ describe("buildReviewSources", () => {
},
},
prDiffFiles: undefined,
useRepositoryKeys: false,
});
expect(result.allFiles).toHaveLength(1);
expect(result.allFiles[0].source).toBe("uncommitted");
expect(result.allFiles[0].repository_name).toBeUndefined();
expect(result.sourceCounts).toEqual({ uncommitted: 1, committed: 0, pr: 0 });
});
@@ -376,9 +380,11 @@ describe("buildReviewSources", () => {
},
],
prRepoName: "frontend",
useRepositoryKeys: false,
});
expect(result.allFiles).toHaveLength(1);
expect(result.allFiles[0].source).toBe("uncommitted");
expect(result.allFiles[0].repository_name).toBeUndefined();
expect(result.sourceCounts).toEqual({ uncommitted: 1, committed: 0, pr: 0 });
});
@@ -407,9 +413,11 @@ describe("buildReviewSources", () => {
},
},
prDiffFiles: undefined,
useRepositoryKeys: false,
});
expect(result.allFiles).toHaveLength(1);
expect(result.allFiles[0].source).toBe("uncommitted");
expect(result.allFiles[0].repository_name).toBeUndefined();
expect(result.sourceCounts).toEqual({ uncommitted: 1, committed: 0, pr: 0 });
});
@@ -15,6 +15,7 @@ import {
normalizeDiffContent,
reviewFileKey,
splitReviewFileKey,
suppressAvailableGitlinkFiles,
} from "@/components/review/types";
import { createDebugLogger } from "@/lib/debug/log";
import type { ReviewFile } from "@/components/review/types";
@@ -36,6 +37,7 @@ type UncommittedFile = {
additions?: number;
deletions?: number;
staged?: boolean;
is_submodule?: boolean;
};
type CumulativeFile = {
@@ -53,12 +55,14 @@ type CumulativeFile = {
* this and carry the path only on the map key.
*/
path?: string;
is_submodule?: boolean;
};
function addUncommittedFiles(
fileMap: Map<string, ReviewFile>,
files: Record<string, UncommittedFile>,
repositoryName?: string,
isSubmodule?: boolean,
) {
for (const [path, file] of Object.entries(files)) {
const diff = file.diff ? normalizeDiffContent(file.diff) : "";
@@ -75,6 +79,7 @@ function addUncommittedFiles(
old_path: file.old_path,
diff_skip_reason: skipReason,
repository_name: repositoryName,
is_submodule: file.is_submodule ?? isSubmodule,
});
}
}
@@ -95,8 +100,11 @@ function addCumulativeFiles(
const path = file.path ?? splitReviewFileKey(mapKey).path;
const repositoryName = useRepositoryKeys ? file.repository_name : undefined;
const key = reviewFileKey({ path, repository_name: repositoryName });
const hasRepoUnawareCollision = key !== path && fileMap.has(path);
if (fileMap.has(key) || uncommittedPaths.has(key) || hasRepoUnawareCollision) continue;
// A bare path is the workspace-root scope in a multi-repository review;
// it must not hide a same-named file from a child repository. The exact
// key still preserves uncommitted precedence for the owning scope, while
// useRepositoryKeys=false keeps legacy single-repository deduplication.
if (fileMap.has(key) || uncommittedPaths.has(key)) continue;
const diff = file.diff ? normalizeDiffContent(file.diff) : "";
fileMap.set(key, {
path,
@@ -110,6 +118,7 @@ function addCumulativeFiles(
diff_skip_reason: file.diff_skip_reason,
repository_name: repositoryName,
base_ref: file.base_ref ?? defaultBaseRef,
is_submodule: file.is_submodule,
});
}
}
@@ -122,8 +131,7 @@ function addPRFiles(
) {
for (const file of files) {
const key = reviewFileKey({ path: file.filename, repository_name: repoName });
const hasRepoUnawareCollision = key !== file.filename && fileMap.has(file.filename);
if (fileMap.has(key) || uncommittedPaths.has(key) || hasRepoUnawareCollision) continue;
if (fileMap.has(key) || uncommittedPaths.has(key)) continue;
const diff = file.patch ? normalizeDiffContent(file.patch) : "";
fileMap.set(key, {
path: file.filename,
@@ -135,6 +143,7 @@ function addPRFiles(
source: "pr",
old_path: file.old_path,
repository_name: repoName,
is_submodule: file.is_submodule,
});
}
}
@@ -143,19 +152,18 @@ function collectPathsFromFiles(
paths: Set<string>,
files: Record<string, UncommittedFile>,
repositoryName?: string,
useRepositoryKeys: boolean = true,
): void {
for (const path of Object.keys(files)) {
// Always add bare path (for deduping repo-unaware sources like cumulative
// diffs that may not carry repository_name).
paths.add(path);
// Also add composite key (for repo-aware dedup when sources carry repo info).
if (repositoryName) paths.add(reviewFileKey({ path, repository_name: repositoryName }));
const scope = useRepositoryKeys ? repositoryName : undefined;
paths.add(reviewFileKey({ path, repository_name: scope }));
}
}
function collectUncommittedPaths(
statusByRepo: BuildReviewSourcesInput["statusByRepo"],
gitStatus: BuildReviewSourcesInput["gitStatus"],
useRepositoryKeys: boolean,
): Set<string> {
const paths = new Set<string>();
if (statusByRepo && statusByRepo.length > 0) {
@@ -164,19 +172,28 @@ function collectUncommittedPaths(
collectPathsFromFiles(
paths,
status.files as Record<string, UncommittedFile>,
repository_name || undefined,
repository_name,
useRepositoryKeys,
);
}
} else if (gitStatus?.files) {
collectPathsFromFiles(paths, gitStatus.files as Record<string, UncommittedFile>);
collectPathsFromFiles(
paths,
gitStatus.files as Record<string, UncommittedFile>,
useRepositoryKeys ? "" : undefined,
useRepositoryKeys,
);
}
return paths;
}
export type BuildReviewSourcesInput = {
gitStatus: { files?: Record<string, UncommittedFile> } | undefined;
gitStatus: { files?: Record<string, UncommittedFile>; is_submodule?: boolean } | undefined;
statusByRepo:
| Array<{ repository_name: string; status: { files?: Record<string, UncommittedFile> } }>
| Array<{
repository_name: string;
status: { files?: Record<string, UncommittedFile>; is_submodule?: boolean };
}>
| undefined;
cumulativeDiff: {
base_commit?: string;
@@ -204,20 +221,31 @@ type NormalizeReviewStatusSourcesInput = {
cumulativeRepositoryNames?: Iterable<string>;
};
function normalizeMultiRepoStatusByRepo(
input: NormalizeReviewStatusSourcesInput,
hasRootStatus: boolean,
) {
if (input.statusByRepo.length === 0) return undefined;
if (hasRootStatus || input.taskRepositoryCount > 1) return input.statusByRepo;
if (!input.gitStatus?.files) return input.statusByRepo;
return [{ repository_name: "", status: input.gitStatus }, ...input.statusByRepo];
}
export function normalizeReviewStatusSources(input: NormalizeReviewStatusSourcesInput) {
const namedStatuses = input.statusByRepo.filter((entry) => entry.repository_name !== "");
const useRepositoryKeys = isReviewMultiRepo(
input.taskRepositoryCount,
namedStatuses
input.statusByRepo
.map((entry) => entry.repository_name)
.concat(Array.from(input.cumulativeRepositoryNames ?? [])),
);
if (useRepositoryKeys) {
const hasRootStatus = input.statusByRepo.some((entry) => entry.repository_name === "");
return {
useRepositoryKeys: true,
prRepoName: input.resolvedPRRepoName,
normalizedGitStatus: input.gitStatus,
normalizedStatusByRepo: namedStatuses.length > 0 ? namedStatuses : undefined,
normalizedStatusByRepo: normalizeMultiRepoStatusByRepo(input, hasRootStatus),
};
}
return {
@@ -228,6 +256,69 @@ export function normalizeReviewStatusSources(input: NormalizeReviewStatusSources
};
}
function addUncommittedSources(
fileMap: Map<string, ReviewFile>,
statusByRepo: BuildReviewSourcesInput["statusByRepo"],
gitStatus: BuildReviewSourcesInput["gitStatus"],
useRepositoryKeys: boolean,
) {
if (statusByRepo && statusByRepo.length > 0) {
for (const { repository_name, status } of statusByRepo) {
if (status?.files) {
addUncommittedFiles(
fileMap,
status.files as Record<string, UncommittedFile>,
useRepositoryKeys ? repository_name : undefined,
status.is_submodule,
);
}
}
return;
}
if (gitStatus?.files) {
addUncommittedFiles(
fileMap,
gitStatus.files as Record<string, UncommittedFile>,
useRepositoryKeys ? "" : undefined,
gitStatus.is_submodule,
);
}
}
function addCommittedAndPRSources(
fileMap: Map<string, ReviewFile>,
input: BuildReviewSourcesInput,
uncommittedPaths: Set<string>,
) {
const { cumulativeDiff, prDiffFiles, prRepoName, useRepositoryKeys = true } = input;
if (cumulativeDiff?.files) {
addCumulativeFiles(
fileMap,
cumulativeDiff.files,
uncommittedPaths,
useRepositoryKeys,
cumulativeDiff.base_commit,
);
}
if (prDiffFiles && prDiffFiles.length > 0) {
addPRFiles(fileMap, prDiffFiles, uncommittedPaths, useRepositoryKeys ? prRepoName : undefined);
}
}
function sortReviewFiles(fileMap: Map<string, ReviewFile>): ReviewFile[] {
return 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);
});
}
function countReviewSources(files: ReviewFile[]): SourceCounts {
const sourceCounts: SourceCounts = { uncommitted: 0, committed: 0, pr: 0 };
for (const file of files) sourceCounts[file.source]++;
return sourceCounts;
}
/**
* Pure helper that merges the three diff sources into one sorted, deduped
* list and computes per-source counts. Uncommitted files write first under
@@ -239,55 +330,15 @@ export function normalizeReviewStatusSources(input: NormalizeReviewStatusSources
* `components/review/review-dialog.tsx`.
*/
export function buildReviewSources(input: BuildReviewSourcesInput): BuildReviewSourcesResult {
const {
gitStatus,
statusByRepo,
cumulativeDiff,
prDiffFiles,
prRepoName,
useRepositoryKeys = true,
} = input;
const { gitStatus, statusByRepo } = input;
const fileMap = new Map<string, ReviewFile>();
const useRepositoryKeys = input.useRepositoryKeys ?? true;
const uncommittedPaths = collectUncommittedPaths(statusByRepo, gitStatus);
if (statusByRepo && statusByRepo.length > 0) {
for (const { repository_name, status } of statusByRepo) {
if (status?.files) {
addUncommittedFiles(
fileMap,
status.files as Record<string, UncommittedFile>,
repository_name || undefined,
);
}
}
} else if (gitStatus?.files) {
addUncommittedFiles(fileMap, gitStatus.files as Record<string, UncommittedFile>);
}
if (cumulativeDiff?.files) {
addCumulativeFiles(
fileMap,
cumulativeDiff.files,
uncommittedPaths,
useRepositoryKeys,
cumulativeDiff.base_commit,
);
}
if (prDiffFiles && prDiffFiles.length > 0)
addPRFiles(fileMap, prDiffFiles, uncommittedPaths, prRepoName);
const allFiles = 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);
});
const sourceCounts: SourceCounts = { uncommitted: 0, committed: 0, pr: 0 };
for (const f of allFiles) sourceCounts[f.source]++;
return { allFiles, sourceCounts };
const uncommittedPaths = collectUncommittedPaths(statusByRepo, gitStatus, useRepositoryKeys);
addUncommittedSources(fileMap, statusByRepo, gitStatus, useRepositoryKeys);
addCommittedAndPRSources(fileMap, input, uncommittedPaths);
const allFiles = suppressAvailableGitlinkFiles(sortReviewFiles(fileMap));
return { allFiles, sourceCounts: countReviewSources(allFiles) };
}
export type UseReviewSourcesResult = {
@@ -1,5 +1,5 @@
import { describe, it, expect } from "vitest";
import { groupPathsByRepoName } from "./use-session-git";
import { groupPathsByRepoName, repositoryScopesForMutation } from "./use-session-git";
describe("groupPathsByRepoName", () => {
it("splits paths by repository_name, preserving insertion order per bucket", () => {
@@ -29,3 +29,27 @@ describe("groupPathsByRepoName", () => {
expect(out.get("only")).toEqual(["a.ts", "b.ts"]);
});
});
describe("repositoryScopesForMutation", () => {
it("excludes a file-only root when only named scopes have trackers", () => {
expect(
repositoryScopesForMutation(
[{ repository_name: undefined }, { repository_name: "vendor" }],
["vendor"],
),
).toEqual(["vendor"]);
});
it("keeps an available root and adds available ancestors", () => {
expect(
repositoryScopesForMutation(
[{ repository_name: "vendor/inner" }],
["", "vendor", "vendor/inner"],
),
).toEqual(["vendor/inner", "vendor", ""]);
});
it("preserves the legacy root fallback without per-repo status", () => {
expect(repositoryScopesForMutation([{ repository_name: undefined }], [])).toEqual([""]);
});
});
@@ -0,0 +1,73 @@
import { describe, expect, it } from "vitest";
import {
isRepositoryScopeAncestor,
repositoryScopesWithAvailableAncestors,
repositoryScopeWaves,
runRepositoryScopeWaves,
shouldSkipRepositoryScope,
} from "./use-session-git-repository-order";
const ROOT_SCOPE = "";
const OUTER_SCOPE = "vendor/outer";
const INNER_SCOPE = "vendor/outer/vendor/inner";
const OTHER_SCOPE = "vendor/other";
describe("repository scope mutation order", () => {
it("groups nested scopes into deepest-first waves and keeps siblings together", () => {
expect(repositoryScopeWaves([ROOT_SCOPE, OUTER_SCOPE, INNER_SCOPE, OTHER_SCOPE])).toEqual([
[INNER_SCOPE],
[OTHER_SCOPE, OUTER_SCOPE],
[ROOT_SCOPE],
]);
});
it("treats the workspace root as an ancestor of every named scope", () => {
expect(isRepositoryScopeAncestor(ROOT_SCOPE, OUTER_SCOPE)).toBe(true);
expect(isRepositoryScopeAncestor(OUTER_SCOPE, INNER_SCOPE)).toBe(true);
expect(isRepositoryScopeAncestor(OTHER_SCOPE, INNER_SCOPE)).toBe(false);
});
it("includes clean tracked parents without unrelated clean siblings", () => {
expect(
repositoryScopesWithAvailableAncestors(
[INNER_SCOPE],
[ROOT_SCOPE, OUTER_SCOPE, INNER_SCOPE, OTHER_SCOPE],
),
).toEqual([INNER_SCOPE, OUTER_SCOPE, ROOT_SCOPE]);
expect(repositoryScopesWithAvailableAncestors(["frontend"], ["frontend", "backend"])).toEqual([
"frontend",
]);
});
it("blocks only ancestors after a child failure", () => {
expect(shouldSkipRepositoryScope(ROOT_SCOPE, [OUTER_SCOPE])).toBe(true);
expect(shouldSkipRepositoryScope(OUTER_SCOPE, [INNER_SCOPE])).toBe(true);
expect(shouldSkipRepositoryScope(OTHER_SCOPE, [OUTER_SCOPE])).toBe(false);
expect(shouldSkipRepositoryScope(OUTER_SCOPE, [OTHER_SCOPE])).toBe(false);
});
it("preserves single-scope and sibling-only operations", () => {
expect(repositoryScopeWaves([ROOT_SCOPE])).toEqual([[ROOT_SCOPE]]);
expect(repositoryScopeWaves(["frontend", "backend"])).toEqual([["backend", "frontend"]]);
});
it("runs children before parents, blocks failed ancestors, and keeps siblings independent", async () => {
const calls: string[] = [];
const results = await runRepositoryScopeWaves(
[ROOT_SCOPE, OUTER_SCOPE, INNER_SCOPE, OTHER_SCOPE],
async (scope) => {
calls.push(scope);
return { success: scope !== INNER_SCOPE };
},
(scope, failedScopes) => ({
success: false,
skipped: `${scope} blocked by ${failedScopes.join(",")}`,
}),
);
expect(calls).toEqual([INNER_SCOPE, OTHER_SCOPE]);
expect(results.find((entry) => entry.repository_name === OUTER_SCOPE)?.skipped).toBe(true);
expect(results.find((entry) => entry.repository_name === ROOT_SCOPE)?.skipped).toBe(true);
expect(results.find((entry) => entry.repository_name === OTHER_SCOPE)?.skipped).toBe(false);
});
});
@@ -0,0 +1,96 @@
/** True when `ancestor` owns the gitlink recorded by `scope`. */
export function isRepositoryScopeAncestor(ancestor: string, scope: string): boolean {
if (ancestor === scope) return false;
if (ancestor === "") return scope !== "";
return scope.startsWith(`${ancestor}/`);
}
/**
* Adds only tracked repository scopes that are ancestors of changed scopes.
* Clean parents need to participate in stage-all commits so they can record
* the child gitlink after the child commit; unrelated clean siblings do not.
*/
export function repositoryScopesWithAvailableAncestors(
scopes: Iterable<string>,
availableScopes: Iterable<string>,
): string[] {
const requested = Array.from(new Set(scopes));
const available = new Set(availableScopes);
const result = new Set(requested);
for (const scope of requested) {
if (scope === "") continue;
const parts = scope.split("/");
for (let length = 1; length < parts.length; length += 1) {
const ancestor = parts.slice(0, length).join("/");
if (available.has(ancestor)) result.add(ancestor);
}
if (available.has("")) result.add("");
}
return Array.from(result);
}
/**
* Groups unique repository scopes into dependency-safe waves. The deepest
* repositories run first; scopes at the same depth are independent and may
* run in parallel. Empty scope is the real workspace root and therefore has
* depth zero.
*/
export function repositoryScopeWaves(scopes: Iterable<string>): string[][] {
const byDepth = new Map<number, Set<string>>();
for (const scope of new Set(scopes)) {
const depth = scope === "" ? 0 : scope.split("/").length;
const bucket = byDepth.get(depth) ?? new Set<string>();
bucket.add(scope);
byDepth.set(depth, bucket);
}
return Array.from(byDepth.entries())
.sort(([left], [right]) => right - left)
.map(([, scopesAtDepth]) => Array.from(scopesAtDepth).sort((a, b) => a.localeCompare(b)));
}
/** A failed child prevents an ancestor from recording a stale gitlink. */
export function shouldSkipRepositoryScope(scope: string, failedScopes: Iterable<string>): boolean {
for (const failedScope of failedScopes) {
if (isRepositoryScopeAncestor(scope, failedScope)) return true;
}
return false;
}
export type RepositoryScopeWaveResult<T> = {
repository_name: string;
result: T;
skipped: boolean;
};
/**
* Runs scope operations wave by wave and records blocked ancestors without
* invoking them. The caller supplies the skipped result so this helper stays
* independent of the Git operation wire shape.
*/
export async function runRepositoryScopeWaves<T extends { success: boolean }>(
scopes: Iterable<string>,
operation: (scope: string) => Promise<T>,
skippedResult: (scope: string, failedScopes: string[]) => T,
): Promise<RepositoryScopeWaveResult<T>[]> {
const failedScopes: string[] = [];
const results: RepositoryScopeWaveResult<T>[] = [];
for (const wave of repositoryScopeWaves(scopes)) {
const runnable = wave.filter((scope) => !shouldSkipRepositoryScope(scope, failedScopes));
const blocked = wave.filter((scope) => shouldSkipRepositoryScope(scope, failedScopes));
for (const scope of blocked) {
results.push({
repository_name: scope,
result: skippedResult(scope, failedScopes),
skipped: true,
});
}
const waveResults = await Promise.all(
runnable.map(async (scope) => ({ repository_name: scope, result: await operation(scope) })),
);
for (const entry of waveResults) {
results.push({ ...entry, skipped: false });
if (!entry.result.success) failedScopes.push(entry.repository_name);
}
}
return results;
}
@@ -0,0 +1,80 @@
import { describe, expect, it } from "vitest";
import type { FileInfo, GitStatusEntry } from "@/lib/state/slices/session-runtime/types";
import { deriveMultiRepoSummary } from "./use-session-git-summary";
type StatusByRepo = Parameters<typeof deriveMultiRepoSummary>[0];
const ROOT_SCOPE = "";
const OUTER_SCOPE = "vendor/outer";
const INNER_SCOPE = "vendor/outer/vendor/inner";
function status(overrides: Partial<GitStatusEntry> = {}): GitStatusEntry {
return {
branch: "main",
remote_branch: "origin/main",
modified: [],
added: [],
deleted: [],
untracked: [],
renamed: [],
ahead: 0,
behind: 0,
files: {},
timestamp: null,
...overrides,
};
}
function repoStatus(repository_name: string, overrides: Partial<GitStatusEntry> = {}) {
return { repository_name, status: status(overrides) } satisfies StatusByRepo[number];
}
function file(repository_name?: string, staged = false): FileInfo {
return { path: "README.md", status: "modified", staged, repository_name };
}
describe("deriveMultiRepoSummary", () => {
it("keeps the root control and status beside nested scopes", () => {
const result = deriveMultiRepoSummary(
[repoStatus(ROOT_SCOPE, { ahead: 1 }), repoStatus(OUTER_SCOPE), repoStatus(INNER_SCOPE)],
[file(ROOT_SCOPE, true), file(INNER_SCOPE)],
[INNER_SCOPE, OUTER_SCOPE, ROOT_SCOPE],
);
expect(result.repoNamesForControls).toEqual([ROOT_SCOPE, OUTER_SCOPE, INNER_SCOPE]);
expect(result.perRepoStatus).toEqual([
expect.objectContaining({ repository_name: ROOT_SCOPE, ahead: 1, hasStaged: true }),
expect.objectContaining({ repository_name: OUTER_SCOPE }),
expect.objectContaining({
repository_name: INNER_SCOPE,
hasUnstaged: true,
}),
]);
});
it("omits a bare workspace root when only named repositories have status", () => {
const result = deriveMultiRepoSummary(
[repoStatus("backend"), repoStatus("frontend", { ahead: 2 })],
[file("frontend")],
["frontend"],
);
expect(result.repoNamesForControls).toEqual(["backend", "frontend"]);
expect(result.perRepoStatus.map(({ repository_name }) => repository_name)).toEqual([
"backend",
"frontend",
]);
});
it("does not promote a file-only root into per-repo status", () => {
const result = deriveMultiRepoSummary(
[repoStatus(OUTER_SCOPE)],
[file(ROOT_SCOPE), file(OUTER_SCOPE)],
[ROOT_SCOPE, OUTER_SCOPE],
);
expect(result.repoNamesForControls).toEqual([OUTER_SCOPE]);
expect(result.perRepoStatus).toEqual([
expect.objectContaining({ repository_name: OUTER_SCOPE, hasUnstaged: true }),
]);
});
});
@@ -0,0 +1,70 @@
import { useMemo } from "react";
import type { FileInfo } from "@/lib/state/slices/session-runtime/types";
import { useSessionGitStatusByRepo } from "./use-session-git-status";
type RepositoryStatus = ReturnType<typeof useSessionGitStatusByRepo>[number];
export type RepositoryStatusSummary = {
repository_name: string;
branch: string | null;
ahead: number;
behind: number;
hasStaged: boolean;
hasUnstaged: boolean;
};
export function deriveMultiRepoSummary(
statusByRepo: RepositoryStatus[],
allFiles: FileInfo[],
reposInFiles: string[],
) {
const seen = new Set<string>();
for (const { repository_name } of statusByRepo) seen.add(repository_name);
for (const repositoryName of reposInFiles) seen.add(repositoryName);
const all = Array.from(seen).sort((a, b) => a.localeCompare(b));
const named = all.filter((repositoryName) => repositoryName !== "");
const hasRootStatus = statusByRepo.some((entry) => entry.repository_name === "");
const repoNamesForControls = hasRootStatus || named.length === 0 ? all : named;
if (statusByRepo.length === 0) {
return { repoNamesForControls, perRepoStatus: [] as RepositoryStatusSummary[] };
}
const stagedByRepo = new Map<string, boolean>();
const unstagedByRepo = new Map<string, boolean>();
for (const file of allFiles) {
const repositoryName = file.repository_name ?? "";
if (file.staged) stagedByRepo.set(repositoryName, true);
else unstagedByRepo.set(repositoryName, true);
}
const hasNamed = statusByRepo.some((status) => status.repository_name !== "");
const filtered =
!hasRootStatus && hasNamed
? statusByRepo.filter((status) => status.repository_name !== "")
: statusByRepo;
const perRepoStatus = filtered.map(({ repository_name, status }) => ({
repository_name,
branch: status?.branch ?? null,
ahead: status?.ahead ?? 0,
behind: status?.behind ?? 0,
hasStaged: stagedByRepo.get(repository_name) ?? false,
hasUnstaged: unstagedByRepo.get(repository_name) ?? false,
}));
return { repoNamesForControls, perRepoStatus };
}
/**
* Derives the repository names and status summaries used by Changes-panel
* repository controls. A bare multi-repo root has no real empty scope, while
* a Git root with submodules does and must retain it alongside its children.
*/
export function useMultiRepoSummary(
statusByRepo: ReturnType<typeof useSessionGitStatusByRepo>,
allFiles: FileInfo[],
reposInFiles: string[],
) {
return useMemo(
() => deriveMultiRepoSummary(statusByRepo, allFiles, reposInFiles),
[statusByRepo, allFiles, reposInFiles],
);
}
+108 -120
View File
@@ -15,6 +15,12 @@ import type {
GitOperationResult as RawGitOperationResult,
PRCreateResult,
} from "@/hooks/use-git-operations";
import { t } from "@/lib/i18n";
import {
repositoryScopesWithAvailableAncestors,
runRepositoryScopeWaves,
} from "./use-session-git-repository-order";
import { useMultiRepoSummary } from "./use-session-git-summary";
/**
* Per-repo result emitted by frontend-side fan-outs (commit, push, pull,
@@ -157,6 +163,34 @@ export function groupPathsByRepoName(
return buckets;
}
/**
* Returns mutation scopes that have a live status tracker. A file can still
* appear under the empty scope when the parent repository only reports a
* changed gitlink; that scope is not a runnable repository unless it is also
* present in statusByRepo. Legacy single-repository hydration has no per-repo
* list, so it keeps the historical empty-scope fallback.
*/
export function repositoryScopesForMutation(
allFiles: Pick<FileInfo, "repository_name">[],
availableScopes: Iterable<string>,
): string[] {
const available = Array.from(new Set(availableScopes));
if (available.length === 0) {
return repositoryScopesWithAvailableAncestors(
new Set(allFiles.map((file) => file.repository_name ?? "")),
available,
);
}
const availableSet = new Set(available);
const requested = new Set<string>();
for (const file of allFiles) {
const scope = file.repository_name ?? "";
if (availableSet.has(scope)) requested.add(scope);
}
return repositoryScopesWithAvailableAncestors(requested, available);
}
/**
* Builds the SessionGit's flat file list. For multi-repo workspaces it
* stamps each FileInfo with its repository_name so consumers can group;
@@ -239,37 +273,44 @@ function aggregatePerRepoResults(
};
}
/**
* Runs `op` against each repo in `repos`, collecting per-repo results.
* Continues past failures so partial-success state is surfaced (instead of
* stopping at the first error and leaving the user blind to repo A having
* already mutated).
*/
async function fanOutAcrossRepos(
async function fanOutAcrossRepositoryWaves(
repos: string[],
operation: string,
op: (repo: string) => Promise<RawGitOperationResult>,
): Promise<GitOperationResult> {
const perRepo: PerRepoOperationResult[] = [];
for (const repo of repos) {
try {
const r = await op(repo);
perRepo.push({
repository_name: repo,
success: r.success,
output: r.output,
error: r.error,
});
} catch (e) {
perRepo.push({
repository_name: repo,
success: false,
output: "",
error: e instanceof Error ? e.message : String(e),
});
}
}
return aggregatePerRepoResults(perRepo, operation);
const results = await runRepositoryScopeWaves(
repos,
async (repo) => {
try {
return await op(repo);
} catch (e) {
return {
success: false,
operation,
output: "",
error: e instanceof Error ? e.message : String(e),
};
}
},
(repo, failedScopes) => ({
success: false,
operation,
output: "",
error: t("common:gitScopeSkipped", {
scope: repo || t("common:workspace"),
failedScopes: failedScopes.join(", "),
}),
}),
);
return aggregatePerRepoResults(
results.map(({ repository_name, result }) => ({
repository_name,
success: result.success,
output: result.output,
error: result.error,
})),
operation,
);
}
/**
@@ -295,7 +336,7 @@ function useStageDispatch({
const stageAll = useCallback(
async (): Promise<GitOperationResult> => {
if (reposInFiles.length <= 1) return gitOps.stage(undefined, reposInFiles[0]);
return fanOutAcrossRepos(reposInFiles, "stage", (r) => gitOps.stage(undefined, r));
return fanOutAcrossRepositoryWaves(reposInFiles, "stage", (r) => gitOps.stage(undefined, r));
},
// eslint-disable-next-line react-hooks/exhaustive-deps -- stable fn ref
[reposInFiles, gitOps.stage],
@@ -303,7 +344,9 @@ function useStageDispatch({
const unstageAll = useCallback(
async (): Promise<GitOperationResult> => {
if (reposInFiles.length <= 1) return gitOps.unstage(undefined, reposInFiles[0]);
return fanOutAcrossRepos(reposInFiles, "unstage", (r) => gitOps.unstage(undefined, r));
return fanOutAcrossRepositoryWaves(reposInFiles, "unstage", (r) =>
gitOps.unstage(undefined, r),
);
},
// eslint-disable-next-line react-hooks/exhaustive-deps -- stable fn ref
[reposInFiles, gitOps.unstage],
@@ -320,32 +363,36 @@ function useStageDispatch({
amend: boolean = false,
repo?: string,
): Promise<GitOperationResult> => {
if (repo !== undefined) return gitOps.commit(message, stageAllOpt, amend, repo || undefined);
const reposWithStaged = Array.from(
new Set(stagedFiles.map((f) => f.repository_name).filter((n): n is string => Boolean(n))),
);
if (reposWithStaged.length === 0) return gitOps.commit(message, stageAllOpt, amend);
if (reposWithStaged.length === 1) {
return gitOps.commit(message, stageAllOpt, amend, reposWithStaged[0]);
if (repo !== undefined) return gitOps.commit(message, stageAllOpt, amend, repo);
const reposWithStaged = Array.from(new Set(stagedFiles.map((f) => f.repository_name ?? "")));
const reposToCommit = stageAllOpt ? reposInFiles : reposWithStaged;
if (reposToCommit.length === 0) return gitOps.commit(message, stageAllOpt, amend);
if (reposToCommit.length === 1) {
return gitOps.commit(message, stageAllOpt, amend, reposToCommit[0]);
}
return fanOutAcrossRepos(reposWithStaged, "commit", (r) =>
return fanOutAcrossRepositoryWaves(reposToCommit, "commit", (r) =>
gitOps.commit(message, stageAllOpt, amend, r),
);
},
// eslint-disable-next-line react-hooks/exhaustive-deps -- stable fn ref
[gitOps.commit, stagedFiles],
[gitOps.commit, reposInFiles, stagedFiles],
);
const runPerRepo = useCallback(
async (
paths: string[],
explicitRepo: string | undefined,
operation: string,
op: (paths: string[], repo: string | undefined) => Promise<GitOperationResult>,
): Promise<GitOperationResult> => {
if (explicitRepo !== undefined) return op(paths, explicitRepo || undefined);
if (explicitRepo !== undefined) return op(paths, explicitRepo);
const buckets = groupPathsByRepo(paths);
let last: GitOperationResult | undefined;
for (const [repo, repoPaths] of buckets) last = await op(repoPaths, repo || undefined);
return last as GitOperationResult;
if (buckets.size <= 1) {
const [repo, repoPaths] = buckets.entries().next().value as [string, string[]];
return op(repoPaths, repo);
}
return fanOutAcrossRepositoryWaves(Array.from(buckets.keys()), operation, (repo) =>
op(buckets.get(repo) ?? [], repo),
);
},
[groupPathsByRepo],
);
@@ -354,6 +401,7 @@ function useStageDispatch({
paths: string[],
repo: string | undefined,
op: (rp: string[], r: string | undefined) => Promise<GitOperationResult>,
operation: string,
) => {
// Bug 6: key pending entries by `repo::path` so an in-flight stage in
// repo B isn't cleared when repo A's status update lands. The consumer
@@ -367,7 +415,7 @@ function useStageDispatch({
return next;
});
try {
return await runPerRepo(paths, repo, op);
return await runPerRepo(paths, repo, operation, op);
} catch (err) {
setPendingStageFiles((prev) => {
const next = new Set(prev);
@@ -381,20 +429,20 @@ function useStageDispatch({
);
const stageFile = useCallback(
async (paths: string[], repo?: string) =>
wrapPending(paths, repo, (rp, r) => gitOps.stage(rp, r)),
wrapPending(paths, repo, (rp, r) => gitOps.stage(rp, r), "stage"),
// eslint-disable-next-line react-hooks/exhaustive-deps -- stable fn ref
[gitOps.stage, wrapPending],
);
const unstageFile = useCallback(
async (paths: string[], repo?: string) =>
wrapPending(paths, repo, (rp, r) => gitOps.unstage(rp, r)),
wrapPending(paths, repo, (rp, r) => gitOps.unstage(rp, r), "unstage"),
// eslint-disable-next-line react-hooks/exhaustive-deps -- stable fn ref
[gitOps.unstage, wrapPending],
);
const discard = useCallback(
async (paths?: string[], repo?: string) => {
if (!paths || paths.length === 0) return gitOps.discard(paths, repo);
return runPerRepo(paths, repo, (rp, r) => gitOps.discard(rp, r));
return runPerRepo(paths, repo, "discard", (rp, r) => gitOps.discard(rp, r));
},
// eslint-disable-next-line react-hooks/exhaustive-deps -- stable fn ref
[gitOps.discard, runPerRepo],
@@ -402,61 +450,6 @@ function useStageDispatch({
return { stageAll, unstageAll, commit, stageFile, unstageFile, discard };
}
/**
* Multi-repo summary for the per-repo Pull/Push/Commit controls. Returns the
* full list of repo names known to the session (even ones with no file
* changes) plus a per-repo `branch / ahead / behind / hasStaged / hasUnstaged`
* row for each. Empty for single-repo workspaces.
*/
function useMultiRepoSummary(
statusByRepo: ReturnType<typeof useSessionGitStatusByRepo>,
allFiles: FileInfo[],
reposInFiles: string[],
) {
// Include every repo known to the session, including the empty-name entry
// for single-repo. Components render the same per-repo group structure in
// both modes — the empty name routes ops to the workspace root, named
// entries route to their respective subdirectories.
//
// Defensive filter: when there are named entries (multi-repo), drop the
// empty entry. The bare task-root tracker is supposed to stay quiet for
// multi-repo (see workspace_tracker.go's gitIndexPath guard) but a stale
// entry can linger from older builds or a bugged code path; without this
// filter the dropdown would show an extra "Repository" / primary-name row
// alongside the real per-repo entries.
const repoNamesForControls = useMemo(() => {
const seen = new Set<string>();
for (const { repository_name } of statusByRepo) seen.add(repository_name);
for (const r of reposInFiles) seen.add(r);
const all = Array.from(seen).sort((a, b) => a.localeCompare(b));
const named = all.filter((r) => r !== "");
return named.length > 0 ? named : all;
}, [statusByRepo, reposInFiles]);
const perRepoStatus = useMemo(() => {
if (statusByRepo.length === 0) return [];
const stagedByRepo = new Map<string, boolean>();
const unstagedByRepo = new Map<string, boolean>();
for (const f of allFiles) {
const r = f.repository_name ?? "";
if (f.staged) stagedByRepo.set(r, true);
else unstagedByRepo.set(r, true);
}
const hasNamed = statusByRepo.some((s) => s.repository_name !== "");
const filtered = hasNamed ? statusByRepo.filter((s) => s.repository_name !== "") : statusByRepo;
return filtered.map(({ repository_name, status }) => ({
repository_name,
branch: status?.branch ?? null,
ahead: status?.ahead ?? 0,
behind: status?.behind ?? 0,
hasStaged: stagedByRepo.get(repository_name) ?? false,
hasUnstaged: unstagedByRepo.get(repository_name) ?? false,
}));
}, [statusByRepo, allFiles]);
return { repoNamesForControls, perRepoStatus };
}
/**
* Bug 6: clears pending stage markers per-repo, not globally. Each repo's
* status streams in independently as agentctl finishes per-repo ops; if we
@@ -518,14 +511,8 @@ type RemoteOpsArgs = {
* push. Pull/Rebase/Merge/Abort fan out across every repo unconditionally.
*/
function useRemoteOpsFanOut({ gitOps, repoNamesForControls, perRepoStatus }: RemoteOpsArgs) {
// Filter out the legacy empty-name entry; it represents the workspace root
// which isn't a real git repo in multi-repo task workspaces. We only want
// to iterate the named repos.
const namedRepos = useMemo(
() => repoNamesForControls.filter((r) => r !== ""),
[repoNamesForControls],
);
const isMultiRepo = namedRepos.length > 1;
const namedRepos = useMemo(() => repoNamesForControls, [repoNamesForControls]);
const isMultiRepo = repoNamesForControls.length > 1;
const aheadByRepo = useMemo(() => {
const m = new Map<string, number>();
for (const s of perRepoStatus) m.set(s.repository_name, s.ahead);
@@ -535,7 +522,7 @@ function useRemoteOpsFanOut({ gitOps, repoNamesForControls, perRepoStatus }: Rem
const pull = useCallback(
async (rebase = false, repo?: string): Promise<GitOperationResult> => {
if (repo !== undefined || !isMultiRepo) return gitOps.pull(rebase, repo);
return fanOutAcrossRepos(namedRepos, "pull", (r) => gitOps.pull(rebase, r));
return fanOutAcrossRepositoryWaves(namedRepos, "pull", (r) => gitOps.pull(rebase, r));
},
// eslint-disable-next-line react-hooks/exhaustive-deps -- stable fn ref
[gitOps.pull, isMultiRepo, namedRepos],
@@ -551,7 +538,7 @@ function useRemoteOpsFanOut({ gitOps, repoNamesForControls, perRepoStatus }: Rem
if (reposWithAhead.length === 0) {
return { success: true, operation: "push", output: "No commits to push" };
}
return fanOutAcrossRepos(reposWithAhead, "push", (r) => gitOps.push(options, r));
return fanOutAcrossRepositoryWaves(reposWithAhead, "push", (r) => gitOps.push(options, r));
},
// eslint-disable-next-line react-hooks/exhaustive-deps -- stable fn ref
[gitOps.push, isMultiRepo, namedRepos, aheadByRepo],
@@ -560,7 +547,7 @@ function useRemoteOpsFanOut({ gitOps, repoNamesForControls, perRepoStatus }: Rem
const rebase = useCallback(
async (baseBranch: string, repo?: string): Promise<GitOperationResult> => {
if (repo !== undefined || !isMultiRepo) return gitOps.rebase(baseBranch, repo);
return fanOutAcrossRepos(namedRepos, "rebase", (r) => gitOps.rebase(baseBranch, r));
return fanOutAcrossRepositoryWaves(namedRepos, "rebase", (r) => gitOps.rebase(baseBranch, r));
},
// eslint-disable-next-line react-hooks/exhaustive-deps -- stable fn ref
[gitOps.rebase, isMultiRepo, namedRepos],
@@ -569,7 +556,7 @@ function useRemoteOpsFanOut({ gitOps, repoNamesForControls, perRepoStatus }: Rem
const merge = useCallback(
async (baseBranch: string, repo?: string): Promise<GitOperationResult> => {
if (repo !== undefined || !isMultiRepo) return gitOps.merge(baseBranch, repo);
return fanOutAcrossRepos(namedRepos, "merge", (r) => gitOps.merge(baseBranch, r));
return fanOutAcrossRepositoryWaves(namedRepos, "merge", (r) => gitOps.merge(baseBranch, r));
},
// eslint-disable-next-line react-hooks/exhaustive-deps -- stable fn ref
[gitOps.merge, isMultiRepo, namedRepos],
@@ -578,7 +565,7 @@ function useRemoteOpsFanOut({ gitOps, repoNamesForControls, perRepoStatus }: Rem
const abort = useCallback(
async (operation: "merge" | "rebase", repo?: string): Promise<GitOperationResult> => {
if (repo !== undefined || !isMultiRepo) return gitOps.abort(operation, repo);
return fanOutAcrossRepos(namedRepos, "abort", (r) => gitOps.abort(operation, r));
return fanOutAcrossRepositoryWaves(namedRepos, "abort", (r) => gitOps.abort(operation, r));
},
// eslint-disable-next-line react-hooks/exhaustive-deps -- stable fn ref
[gitOps.abort, isMultiRepo, namedRepos],
@@ -622,10 +609,11 @@ function useFileDerivations(
return m;
}, [allFiles]);
const reposInFiles = useMemo(() => {
const seen = new Set<string>();
for (const f of allFiles) if (f.repository_name) seen.add(f.repository_name);
return Array.from(seen);
}, [allFiles]);
return repositoryScopesForMutation(
allFiles,
statusByRepo.map(({ repository_name }) => repository_name),
);
}, [allFiles, statusByRepo]);
const { repoNamesForControls, perRepoStatus } = useMultiRepoSummary(
statusByRepo,
allFiles,
@@ -723,14 +711,14 @@ export function useSessionGit(sessionId: string | null | undefined): SessionGit
// (single-repo). With paths, we route to the right repo per file.
stage: (paths?: string[], repo?: string) => {
if (paths && paths.length > 0) return stageFile(paths, repo);
if (repo) return gitOps.stage(undefined, repo);
if (repo !== undefined) return gitOps.stage(undefined, repo);
return stageAll();
},
stageFile,
stageAll,
unstage: (paths?: string[], repo?: string) => {
if (paths && paths.length > 0) return unstageFile(paths, repo);
if (repo) return gitOps.unstage(undefined, repo);
if (repo !== undefined) return gitOps.unstage(undefined, repo);
return unstageAll();
},
unstageFile,
+25 -2
View File
@@ -1,5 +1,9 @@
import { describe, expect, it } from "vitest";
import { getChangeRequestTerminology } from "./use-git-operations";
import { describe, expect, it, vi } from "vitest";
import {
buildGitOperationCallbacks,
getChangeRequestTerminology,
repositoryScopePayload,
} from "./use-git-operations";
describe("getChangeRequestTerminology", () => {
it("uses merge request terminology for GitLab", () => {
@@ -16,3 +20,22 @@ describe("getChangeRequestTerminology", () => {
});
});
});
describe("repository scope payloads", () => {
it("keeps an explicit empty repository name", () => {
expect(repositoryScopePayload("")).toEqual({ repo: "" });
expect(repositoryScopePayload(undefined)).toEqual({});
});
it("sends the root sentinel to scoped git operations", async () => {
const executeOperation = vi.fn() as unknown as Parameters<typeof buildGitOperationCallbacks>[0];
const operations = buildGitOperationCallbacks(executeOperation);
await operations.stage(undefined, "");
expect(executeOperation).toHaveBeenCalledWith("worktree.stage", {
paths: [],
repo: "",
});
});
});
+19 -14
View File
@@ -102,36 +102,41 @@ type ExecuteOperation = <T extends GitOperationResult>(
payload: Record<string, unknown>,
) => Promise<T>;
function buildGitOperationCallbacks(executeOperation: ExecuteOperation) {
/** Preserve an explicitly selected workspace-root scope (`repo === ""`). */
export function repositoryScopePayload(repo?: string): { repo?: string } {
return repo === undefined ? {} : { repo };
}
export function buildGitOperationCallbacks(executeOperation: ExecuteOperation) {
const pull = async (rebase = false, repo?: string) =>
executeOperation<GitOperationResult>("worktree.pull", {
rebase,
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
const push = async (options?: { force?: boolean; setUpstream?: boolean }, repo?: string) =>
executeOperation<GitOperationResult>("worktree.push", {
force: options?.force ?? false,
set_upstream: options?.setUpstream ?? false,
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
const rebase = async (baseBranch: string, repo?: string) =>
executeOperation<GitOperationResult>("worktree.rebase", {
base_branch: baseBranch,
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
const merge = async (baseBranch: string, repo?: string) =>
executeOperation<GitOperationResult>("worktree.merge", {
base_branch: baseBranch,
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
const abort = async (operation: "merge" | "rebase", repo?: string) =>
executeOperation<GitOperationResult>("worktree.abort", {
operation,
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
const commit = async (message: string, stageAll = true, amend = false, repo?: string) =>
@@ -139,44 +144,44 @@ function buildGitOperationCallbacks(executeOperation: ExecuteOperation) {
message,
stage_all: stageAll,
amend,
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
const stage = async (paths?: string[], repo?: string) =>
executeOperation<GitOperationResult>("worktree.stage", {
paths: paths ?? [],
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
const unstage = async (paths?: string[], repo?: string) =>
executeOperation<GitOperationResult>("worktree.unstage", {
paths: paths ?? [],
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
const discard = async (paths?: string[], repo?: string) =>
executeOperation<GitOperationResult>("worktree.discard", {
paths: paths ?? [],
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
const revertCommit = async (commitSHA: string, repo?: string) =>
executeOperation<GitOperationResult>("worktree.revert_commit", {
commit_sha: commitSHA,
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
const renameBranch = async (newName: string, repo?: string) =>
executeOperation<GitOperationResult>("worktree.rename_branch", {
new_name: newName,
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
const reset = async (commitSHA: string, mode: "soft" | "hard", repo?: string) =>
executeOperation<GitOperationResult>("worktree.reset", {
commit_sha: commitSHA,
mode,
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
const createPR = async (
@@ -191,7 +196,7 @@ function buildGitOperationCallbacks(executeOperation: ExecuteOperation) {
body,
base_branch: baseBranch ?? "",
draft: draft ?? true,
...(repo ? { repo } : {}),
...repositoryScopePayload(repo),
});
return {
+5 -1
View File
@@ -8,9 +8,13 @@ import type { ReviewSeverity, TaskReviewFinding, TaskReviewRun } from "@/lib/typ
* different repositories would share their findings.
*/
export function findingFileKey(finding: TaskReviewFinding): string {
const repositoryName =
finding.repository_name === "" && finding.repository_id === ""
? undefined
: finding.repository_name;
return reviewFileKey({
path: finding.file_path,
repository_name: finding.repository_name || undefined,
repository_name: repositoryName,
});
}
@@ -178,6 +178,21 @@ describe("switchEnvLayout — root fix for terminal/layout swapping", () => {
expect(setEnvLayout).not.toHaveBeenCalledWith("env-first", expect.anything());
});
it("prioritizes an explicit route layout over saved maximize state on first adoption", () => {
const api = makeMockApi();
const buildDefaultLayout = vi.fn();
vi.mocked(getEnvMaximizeState).mockReturnValue({
preMaximizeLayout: { columns: [] },
maximizedDockviewJson: { grid: {}, panels: {} },
});
useDockviewStore.setState({ api, currentLayoutEnvId: null, buildDefaultLayout });
useDockviewStore.getState().switchEnvLayout(null, "env-first", "session-Y", [], "plan");
expect(api.fromJSON).not.toHaveBeenCalled();
expect(buildDefaultLayout).toHaveBeenCalledWith(api, "plan");
});
it("does nothing when api is unset", () => {
useDockviewStore.setState({ api: null });
useDockviewStore.getState().switchEnvLayout("env-a", "env-b", null);
@@ -174,6 +174,18 @@ describe("performEnvSwitch", () => {
expect(params.buildDefault).toHaveBeenCalledWith(params.api);
});
it("keeps an explicit route layout on first environment adoption", () => {
// The task route can render ?layout=plan before the session's environment
// id arrives. First adoption must replay that explicit intent instead of
// replacing the plan preset with the effective default layout.
const params = makeParams({ oldEnvId: null, initialLayout: "plan" });
performEnvSwitch(params);
expect(params.buildDefault).toHaveBeenCalledWith(params.api, "plan");
expect(params.api.fromJSON).not.toHaveBeenCalled();
});
it("preserves the outgoing session panel's tab index when adding the new session on the fast path", () => {
// Regression: the fast-path used to call addPanel with only
// { referenceGroup }, so dockview appended the new session tab to the end
+20 -1
View File
@@ -90,8 +90,11 @@ export type EnvSwitchParams = {
currentSessionIds?: string[];
safeWidth: number;
safeHeight: number;
buildDefault: (api: DockviewApi) => void;
/** Build the effective default, optionally honoring a route layout intent. */
buildDefault: (api: DockviewApi, intentName?: string) => void;
getDefaultLayout: () => LayoutState;
/** Explicit layout from the task route, such as `?layout=plan`. */
initialLayout?: string | null;
};
/** Close non-session env-scoped panels before the new env's panels are restored. */
@@ -503,6 +506,14 @@ function addIncomingSessionPanel(
});
}
/** Apply a route layout intent that raced the first environment hydration. */
function applyInitialRouteLayout(params: EnvSwitchParams): LayoutGroupIds | null {
if (params.oldEnvId !== null || !params.initialLayout) return null;
params.buildDefault(params.api, params.initialLayout);
params.api.layout(params.safeWidth, params.safeHeight);
return applyLayoutFixups(params.api);
}
/**
* Switch the dockview layout between task environments.
*
@@ -532,6 +543,14 @@ export function performEnvSwitch(params: EnvSwitchParams): LayoutGroupIds {
});
}
// A task route can render before its session's environment id has hydrated.
// In that window onReady applies the explicit route intent (for example,
// ?layout=plan) to a temporary global layout. First adoption must replay the
// intent instead of restoring the env's saved/default layout over it.
// Later env switches intentionally ignore this one-shot route override.
const initialRouteLayout = applyInitialRouteLayout(params);
if (initialRouteLayout) return initialRouteLayout;
const fastResult = tryFastEnvSwitch(params);
if (fastResult) {
if (isDebug()) {
+12 -3
View File
@@ -209,6 +209,7 @@ type DockviewStore = {
newEnvId: string,
activeSessionId: string | null,
currentSessionIds?: string[],
initialLayout?: string | null,
) => void;
deferredPanelActions: DeferredPanelAction[];
queuePanelAction: (action: DeferredPanelAction) => void;
@@ -824,6 +825,7 @@ function buildEnvSwitchAction(set: StoreSet, get: StoreGet) {
newEnvId: string,
activeSessionId: string | null,
currentSessionIds: string[] = [],
initialLayout?: string | null,
) => {
const { api, currentLayoutEnvId, preMaximizeLayout } = get();
if (!api) {
@@ -872,7 +874,12 @@ function buildEnvSwitchAction(set: StoreSet, get: StoreGet) {
pinnedWidths: manualRightWidth === null ? new Map() : new Map([["right", manualRightWidth]]),
});
try {
if (restoreMaximizeFromStorage(api, newEnvId, set, activeSessionId, currentSessionIds))
const hasFirstAdoptionRouteLayout =
oldEnvId === null && currentLayoutEnvId === null && Boolean(initialLayout);
if (
!hasFirstAdoptionRouteLayout &&
restoreMaximizeFromStorage(api, newEnvId, set, activeSessionId, currentSessionIds)
)
return;
const measured = measureDockviewContainer(api);
const ids = performEnvSwitch({
@@ -883,8 +890,9 @@ function buildEnvSwitchAction(set: StoreSet, get: StoreGet) {
currentSessionIds,
safeWidth: measured.width,
safeHeight: measured.height,
buildDefault: (a) => get().buildDefaultLayout(a),
buildDefault: (a, intentName) => get().buildDefaultLayout(a, intentName),
getDefaultLayout: () => get().userDefaultLayout ?? getPresetLayout(get().defaultPreset),
initialLayout,
});
set(ids);
enforceFromStore(api, get);
@@ -1204,10 +1212,11 @@ export function performLayoutSwitch(
newEnvId: string,
activeSessionId: string | null,
currentSessionIds: string[] = [],
initialLayout?: string | null,
): void {
useDockviewStore
.getState()
.switchEnvLayout(oldEnvId, newEnvId, activeSessionId, currentSessionIds);
.switchEnvLayout(oldEnvId, newEnvId, activeSessionId, currentSessionIds, initialLayout);
}
/**
@@ -50,6 +50,7 @@ const COMPARABLE_FILE_FIELDS = [
"diff",
"diff_skip_reason",
"repository_name",
"is_submodule",
] as const;
function comparableFileInfo(file: FileInfo) {
@@ -63,6 +64,7 @@ function comparableFileInfo(file: FileInfo) {
diff: file.diff ?? "",
diff_skip_reason: file.diff_skip_reason ?? "",
repository_name: file.repository_name ?? "",
is_submodule: file.is_submodule ?? false,
};
}
@@ -96,6 +98,7 @@ function hasBranchSummaryChanged(existing: GitStatusEntry, incoming: GitStatusEn
existing.ahead !== incoming.ahead ||
existing.behind !== incoming.behind ||
(existing.repository_name ?? "") !== (incoming.repository_name ?? "") ||
existing.is_submodule !== incoming.is_submodule ||
existing.branch_additions !== incoming.branch_additions ||
existing.branch_deletions !== incoming.branch_deletions
);
@@ -63,4 +63,20 @@ describe("setGitStatus change reporting (single deep compare)", () => {
);
expect(changed).toBe(true);
});
it("returns true when only the submodule marker changes", () => {
store.getState().setGitStatus(SESSION, status({ is_submodule: false }));
expect(store.getState().setGitStatus(SESSION, status({ is_submodule: true }))).toBe(true);
});
it("returns false when the submodule marker is unchanged", () => {
store.getState().setGitStatus(SESSION, status({ is_submodule: true }));
expect(
store
.getState()
.setGitStatus(SESSION, status({ is_submodule: true, timestamp: "2026-05-28T00:01:00Z" })),
).toBe(false);
});
});
@@ -58,6 +58,8 @@ export type FileInfo = {
* per-repository headers.
*/
repository_name?: string;
/** True when the file belongs to an initialized Git submodule scope. */
is_submodule?: boolean;
};
export type GitStatusEntry = {
@@ -80,6 +82,8 @@ export type GitStatusEntry = {
* GitStatusState.byEnvironmentRepo.
*/
repository_name?: string;
/** True when this status belongs to an initialized Git submodule scope. */
is_submodule?: boolean;
};
export type GitStatusState = {
+2
View File
@@ -30,6 +30,8 @@ export type GitStatusData = {
* git status off this name so the changes panel can show all repos at once.
*/
repository_name?: string;
/** True when this status belongs to an initialized Git submodule scope. */
is_submodule?: boolean;
};
// Git commit data
+2
View File
@@ -514,6 +514,8 @@ export type PRDiffFile = {
deletions: number;
patch: string;
old_path?: string;
/** True when the PR file belongs to an initialized Git submodule scope. */
is_submodule?: boolean;
};
// PR commit info (from GitHub API)
@@ -193,6 +193,16 @@ describe("git-status WS handler — stale-while-revalidate", () => {
expect(invalidateCumulativeDiffCacheMock).toHaveBeenCalledTimes(1);
});
it("stores the status-level submodule marker", () => {
const handler = gitStatusHandler(store);
const event = statusUpdateEvent(STATUS_TIME_1);
event.status.is_submodule = true;
handler(gitEvent(event));
expect(store.getState().gitStatus.byEnvironmentId[SESSION].is_submodule).toBe(true);
});
it("invalidates cumulative diff when status diff content changes", () => {
const handler = gitStatusHandler(store);
+1
View File
@@ -45,6 +45,7 @@ function buildGitStatusEntry(event: GitStatusUpdateEvent): GitStatusEntry {
// Multi-repo workspaces tag each status with the repository it belongs to;
// setGitStatus routes the entry into byEnvironmentRepo accordingly.
repository_name: event.status.repository_name,
is_submodule: event.status.is_submodule,
};
}
+2
View File
@@ -49,6 +49,7 @@
"deleteSession": "Delete session",
"edit": "Edit",
"editTask": "Edit task",
"gitScopeSkipped": "Skipped {{scope}} because a child repository operation failed ({{failedScopes}}).",
"executors": "Executors",
"externalMcp": "External MCP",
"failedToLoadRoutine": "Failed to load routine",
@@ -98,6 +99,7 @@
"reloadTheApplicationToTryAgain": "Reload the application to try again.",
"requestFailed": "Request failed",
"rename": "Rename",
"reviewSubmoduleBoundary": "Submodule: {{scope}}",
"reset": "Reset",
"resetWatchChecking": "Checking how many tasks would be deleted…",
"resetWatchDeleteAllTasks": "This will delete every task previously created by the watch, including archived tasks. The watch's polling cursor is also cleared so the next check re-imports every currently-matching item. This cannot be undone.",
+2
View File
@@ -49,6 +49,7 @@
"deleteSession": "Ďēĺēţē śēśśĩōń",
"edit": "Ēďĩţ",
"editTask": "Ēďĩţ ţàśķ",
"gitScopeSkipped": "Śķĩƥƥēď {{scope}} ƀēćàũśē à ćĥĩĺď ŕēƥōśĩţōŕŷ ōƥēŕàţĩōń ƒàĩĺēď ({{failedScopes}}).",
"executors": "Ēxēćũţōŕś",
"externalMcp": "Ēxţēŕńàĺ ḾĆƤ",
"failedToLoadRoutine": "Ƒàĩĺēď ţō ĺōàď ŕōũţĩńē",
@@ -98,6 +99,7 @@
"reloadTheApplicationToTryAgain": "Ŕēĺōàď ţĥē àƥƥĺĩćàţĩōń ţō ţŕŷ àĝàĩń.",
"requestFailed": "Ŕēqũēśţ ƒàĩĺēď",
"rename": "Ŕēńàḿē",
"reviewSubmoduleBoundary": "Śũƀḿōďũĺē: {{scope}}",
"reset": "Ŕēśēţ",
"resetWatchChecking": "Ćĥēćķĩńĝ ĥōŵ ḿàńŷ ţàśķś ŵōũĺď ƀē ďēĺēţēď…",
"resetWatchDeleteAllTasks": "Ţĥĩś ŵĩĺĺ ďēĺēţē ēvēŕŷ ţàśķ ƥŕēvĩōũśĺŷ ćŕēàţēď ƀŷ ţĥē ŵàţćĥ, ĩńćĺũďĩńĝ àŕćĥĩvēď ţàśķś. Ţĥē ŵàţćĥ'ś ƥōĺĺĩńĝ ćũŕśōŕ ĩś àĺśō ćĺēàŕēď śō ţĥē ńēxţ ćĥēćķ ŕē-ĩḿƥōŕţś ēvēŕŷ ćũŕŕēńţĺŷ-ḿàţćĥĩńĝ ĩţēḿ. Ţĥĩś ćàńńōţ ƀē ũńďōńē.",
+2
View File
@@ -41,6 +41,7 @@
"dAgo": "{{diffDay}}天前",
"deleteSession": "删除会话",
"edit": "编辑",
"gitScopeSkipped": "已跳过 {{scope}},因为子仓库操作失败({{failedScopes}})。",
"executors": "执行器",
"externalMcp": "外部 MCP",
"hAgo": "{{diffHr}}小时前",
@@ -63,6 +64,7 @@
"prompts": "提示词",
"requestFailed": "请求失败",
"rename": "重命名",
"reviewSubmoduleBoundary": "子模块:{{scope}}",
"reset": "重置",
"resetWatchChecking": "正在检查将删除多少任务…",
"resetWatchDeleteAllTasks": "这会删除此前由此监控创建的所有任务,包括已归档任务。监控的轮询游标也会被清除,因此下次检查会重新导入当前所有匹配项。此操作无法撤销。",
@@ -0,0 +1,46 @@
# ADR-2026-08-05-nested-submodules-as-repository-scopes: Model Nested Submodules as Repository Scopes
**Status:** accepted
**Date:** 2026-08-05
**Area:** backend, frontend, protocol
## Context
Kandev initializes recursive Git submodules in task worktrees, but agentctl tracks only the parent repository or immediate sibling repositories. Review therefore receives the parent repository's gitlink change while the changed files inside initialized submodules remain invisible. Flattening those files into the parent repository would make them visible, but stage, discard, commit, base-resolution, and file-at-ref operations would still execute against the wrong Git repository.
## Decision
Initialized submodules are represented by the existing repository-scoped Git status, log, cumulative-diff, file, and mutation contracts.
- Each submodule tracker uses a task-workspace-relative, slash-delimited `repository_name`, such as `vendor/parser` or `frontend/vendor/parser`. The workspace root keeps the existing empty repository name. Immediate sibling task repositories keep their existing top-level names.
- A submodule's comparison anchor is the gitlink commit recorded by its parent repository at the parent's comparison anchor. Nested anchors are resolved recursively, so committed submodule work remains reviewable after an agentctl restart.
- Mixed root and named repository statuses are valid. Consumers must not discard the empty root status merely because named statuses exist.
- Review presents the repository-scope hierarchy as directories and marks submodule boundaries. When child file diffs are available, the parent gitlink-only row is suppressed from the review list.
- Git mutations that span nested scopes execute in dependency waves from deepest submodule to parent. Independent sibling scopes may execute in parallel within a wave. This is required for commit-all to create child commits before recording their new gitlinks in parent commits.
- Discovery and all resulting Git work use the existing class-aware subprocess admission system. An inaccessible or uninitialized submodule is omitted as a child scope; its parent repository remains usable and may still expose the gitlink change.
The existing routes and payload fields remain in place. The protocol change is semantic: `repository_name` may identify a nested path, and an empty root entry may coexist with named entries.
## Consequences
The current multi-repository infrastructure remains the single implementation path for repository-aware review and Git operations. Backend tracker discovery, aggregate API fan-out, frontend status normalization, review identities, and Git mutation ordering all need to understand a repository tree instead of assuming either one unnamed repository or several named siblings.
Each initialized submodule adds polling and diff work, but Git subprocess concurrency remains bounded and fair through the existing admission controller. Git-host pull requests remain repository-specific: showing and committing nested changes does not create or coordinate pull requests for submodule remotes.
## Alternatives Considered
### Flatten submodule files into the parent tracker
Rejected because Git treats a submodule as one gitlink. Parent-scoped stage, discard, ref lookup, and commit commands cannot correctly mutate files in the child repository.
### Run `git submodule foreach --recursive` for each Review request
Rejected because it would introduce a second parsing and aggregation path, would not provide live repository-scoped status events, and would still need custom routing for every mutation.
### Add a submodule-specific API and frontend state model
Rejected because the existing multi-repository contracts already carry repository-relative file paths, per-repository bases, comments, status, commits, and mutations. A parallel contract would duplicate those semantics and drift.
### Keep showing only gitlink SHAs
Rejected because it leaves reviewers unable to inspect or comment on the code changed inside the submodule, which is the user problem this decision addresses.
+1
View File
@@ -125,3 +125,4 @@ Read individual ADRs for full context. Create new ones via `/record decision` or
| 2026-08-04-remote-contribution-bindings | [Bind Remote Contributions to Target Repositories](2026-08-04-remote-contribution-bindings.md) | accepted | backend, protocol, security, GitHub, GitLab | 2026-08-04 |
| 2026-08-03-scope-and-merge-repository-secrets | [Scope and Merge Repository Secrets](2026-08-03-scope-and-merge-repository-secrets.md) | accepted | backend, frontend, security, protocol | 2026-08-03 |
| 2026-08-05-server-owned-quick-terminal-descriptors | [Server-Owned Quick Terminal Descriptors](2026-08-05-server-owned-quick-terminal-descriptors.md) | accepted | backend, frontend, protocol, security | 2026-08-05 |
| 2026-08-05-nested-submodules-as-repository-scopes | [Model Nested Submodules as Repository Scopes](2026-08-05-nested-submodules-as-repository-scopes.md) | accepted | backend, frontend, protocol | 2026-08-05 |
+108
View File
@@ -0,0 +1,108 @@
---
spec: docs/specs/ui/submodule-review.md
created: 2026-08-05
status: implemented
---
# Implementation Plan: Nested Submodule Review
## Overview
Extend agentctl's existing repository tracker graph so initialized submodules are repository-scoped children with stable comparison anchors, then include the mixed root/child graph in the existing status, log, and cumulative-diff fan-outs. Adapt frontend review normalization, hierarchy, and Git mutations to preserve the root alongside named children and execute dependent mutations from deepest scope to parent. Finish with real desktop/mobile Review coverage and public documentation.
The approach follows [ADR-2026-08-05-nested-submodules-as-repository-scopes](../../decisions/2026-08-05-nested-submodules-as-repository-scopes.md). It does not add persistence, new routes, or a submodule-specific state store.
---
## Backend
### Repository graph discovery
- Add a focused repository-graph helper beside `apps/backend/internal/agentctl/server/process/manager.go`. It discovers only initialized gitlinks declared by each tracked repository, validates every task-root-relative child path, and recurses through initialized nested submodules.
- Use classified Git lifecycle commands through `internal/common/subproc` to read gitlink paths and parent-tree commit IDs. Do not execute raw `git` commands or walk arbitrary nested directories.
- Represent every child with its task-root-relative repository name, absolute working directory, and comparison ref. For a top-level repository named `frontend`, `vendor/parser` becomes `frontend/vendor/parser`; for a Git workspace root it remains `vendor/parser`.
- Set each submodule tracker's base branch/ref to the gitlink SHA resolved from its parent's comparison tree. Build the root tracker plus child trackers even when the root itself is a Git repository.
- Teach construction, rebind, rescan, reconciliation, polling-mode propagation, subscriptions, file/content search, and teardown to retain a valid root tracker alongside child trackers. Preserve the current bare multi-repository task-root behavior.
### Review-facing Git fan-out
- Add a manager snapshot method for ordered Git repository scopes. It includes `""` only when the workspace root tracker owns a real Git repository and includes every named sibling/submodule scope in stable path order.
- Update `apps/backend/internal/agentctl/server/api/git.go` so status, log, and cumulative-diff aggregation use that scope list. The root and submodule results must coexist; per-file cumulative payloads keep repository-relative `path`, nested `repository_name`, and exact `base_ref`.
- Keep `RepoSubpaths` for callers that need named paths, but audit `workspace.go`, content search, LSP, and file search assumptions so adding a submodule does not make root file access disappear or cause a bare task root to be treated as a Git repository.
- Preserve partial-success behavior, deterministic merge order, request cancellation, and the shared class-aware Git admission bound.
---
## Frontend
### Repository-scope normalization
- Update `apps/web/hooks/domains/session/use-review-sources.ts`, `apps/web/components/task/use-review-dialog.ts`, and their helpers so a valid empty root status is retained when named submodule statuses exist.
- Derive an internal repository-scope hierarchy from task repository count plus slash-delimited `repository_name` values. A named scope below `""`, or below another named scope, is a submodule; top-level sibling task repositories remain workspace repositories.
- Keep review identities as `(repository_name, path)`. Suppress a parent file whose workspace-relative path exactly matches an available child scope, while retaining the gitlink row when no child file source is available.
- Apply the same source precedence and normalization in `buildReviewSources` and `buildAllFiles` so the Changes panel and expanded Review dialog cannot disagree.
### Review presentation
- Rework `buildFileTree` in `apps/web/components/review/types.ts` to compose one hierarchy from repository-scope paths plus repository-relative file paths. Preserve top-level multi-repository roots, pin submodule boundary nodes, and keep duplicate relative filenames isolated by the existing composite review key.
- Render a small submodule indicator in `review-file-tree.tsx` and the repository group/diff headers used on phone. Add new accessible copy through i18n and regenerate the pseudo locale.
- Preserve the current desktop split Review surface. On phone, reuse the existing full-height Review dialog and sticky diff headers; do not introduce a second navigator or desktop-only control.
### Nested Git mutation ordering
- Update `apps/web/hooks/domains/session/use-session-git.ts` to include the empty root repository in file derivations and control summaries when it is a real status source.
- Replace flat concurrent fan-out for dependency-sensitive operations with repository-depth waves. Stage/commit/discard operations that affect descendants run deepest scopes first; sibling scopes at the same depth may keep the existing parallel and partial-result behavior.
- For commit-all with unstaged nested work, select scopes from all changed files, run child `stage + commit` before parent `stage + commit`, and stop an ancestor chain after a child failure so a parent never records an unintended gitlink.
- Keep explicit single-repository operations and unrelated sibling multi-repository behavior unchanged.
---
## Tests
- **Repository discovery and baselines:** `apps/backend/internal/agentctl/server/process/manager_submodule_test.go` creates a parent repository with a nested initialized submodule graph and verifies tracker names, comparison refs, root retention, subscription events, rescan/rebind behavior, invalid-path rejection, and teardown.
- **Review API aggregation:** extend `apps/backend/internal/agentctl/server/api/git_multi_repo_review_test.go` to verify root plus direct/nested submodule status, commit log, cumulative file payloads, stable per-scope bases, duplicate relative paths, and partial failure.
- **Source normalization and tree hierarchy:** extend `apps/web/hooks/domains/session/use-review-sources.test.ts`, `apps/web/components/review/review-dialog.build-files.test.ts`, `apps/web/components/review/types.multi-repo.test.ts`, and `apps/web/components/review/review-file-tree.test.tsx` for mixed root/named entries, nested boundary nodes, gitlink suppression/fallback, duplicate filenames, and accessible boundary labels.
- **Mutation order:** add a focused pure-helper test beside `use-session-git-grouping.test.ts` that proves deepest-first waves, sibling grouping, root inclusion, child-failure ancestor blocking, and unchanged single/sibling behavior.
## E2E Tests
- **Desktop scenario:** `apps/web/e2e/tests/review/submodule-review.spec.ts` creates a disposable parent repository with direct and nested initialized submodules, changes parent and child files, opens Review, and verifies the hierarchy, submodule indicators, textual diffs, distinct duplicate paths, and absence of duplicate gitlink-only rows. It then exercises the commit-all flow and verifies child commits precede parent gitlink commits.
- **Mobile scenario:** `apps/web/e2e/tests/review/mobile-submodule-review.spec.ts` opens the same kind of task in the `mobile-chrome` project and verifies the submodule scope in the sticky header, textual diff access, touch-reachable review controls, viewport containment, and no document horizontal overflow.
- Use a shared sibling helper for disposable Git setup and cleanup. Seed preconditions through Git/API helpers and assert outcomes through the UI; do not mutate the worker-scoped seed repository permanently.
## Public documentation
- Update `docs/public/sessions-and-review.md` (how-to/explanation) to state that initialized nested submodule files participate in repository-aware Review and that submodule PRs remain separate repository workflows.
- Update the **Changes and cumulative Review** entry in `docs/public/feature-status.md` (reference) to mention nested initialized submodule support.
- Validate public docs with `node --test scripts/validate-public-docs.test.mjs` and `node scripts/validate-public-docs.mjs`.
## Verification Results
- Backend graph, aggregation, and mutation-support changes are implemented. `go test ./internal/agentctl/server/process -count=1` passed 584 tests after the post-rebase task-root rescan regression; the nested graph subset passed 4 tests; the nested API aggregation subset passed 3 tests. `make -C apps/backend test` passed, `make -C apps/backend lint` reported 0 issues, and `make -C apps/backend build` completed.
- Frontend source, hierarchy, and mutation tests passed: 9 Vitest files, 118 tests. `pnpm run typecheck`, `pnpm run lint`, `pnpm run i18n:check`, and `pnpm run i18n:ratchet` completed successfully.
- `pnpm run build:vite` completed. The managed desktop Chromium E2E passed 1 test and the managed `mobile-chrome` E2E passed 1 test; each fixture removes its disposable repository tree in `finally`.
- The host-mode SSH workspace-source E2E passed after the task-root rescan fix, and the six-test workflow-start-step suite passed after a fresh backend/Vite build.
- Public documentation validation passed 58 tests and validated 41 published pages.
## Implementation Waves And Parallel Candidates
The default execution order is sequential in the primary conversation. The frontend presentation and mutation tasks are parallel-safe after the backend contract is complete because they own disjoint files; using subagents still requires explicit user authorization.
Wave 1:
- [x] [Task 01: Discover nested repository scopes](task-01-discover-nested-repository-scopes.md)
- [x] [Task 02: Aggregate root and submodule Git data](task-02-aggregate-submodule-git-data.md)
Wave 2 (parallel candidates after Task 02; user authorization required):
- [x] [Task 03: Present submodule review hierarchy](task-03-present-submodule-review-hierarchy.md)
- [x] [Task 04: Order nested Git mutations](task-04-order-nested-git-mutations.md)
Wave 3:
- [x] [Task 05: Prove responsive submodule review](task-05-prove-submodule-review.md)
- [x] [Task 06: Document nested submodule review](task-06-document-submodule-review.md)
## Open Questions
None.
@@ -0,0 +1,62 @@
---
id: "01-discover-nested-repository-scopes"
title: "Discover nested repository scopes"
status: done
wave: 1
depends_on: []
plan: "plan.md"
spec: "../../specs/ui/submodule-review.md"
---
# Task 01: Discover nested repository scopes
## Acceptance
- Agentctl discovers every initialized direct and nested submodule as a tracker whose repository name is its task-root-relative path, while retaining a valid Git root tracker.
- Each submodule tracker compares from the gitlink SHA in its parent's comparison tree, including after manager reconstruction; invalid, uninitialized, or inaccessible children are skipped without disabling the parent.
- Construction, rescan, rebind, subscriptions, polling mode, search, and teardown preserve both existing bare multi-repository behavior and the new root-plus-children graph.
## Verification
```bash
cd apps/backend && go test ./internal/agentctl/server/process -run 'TestManager_.*Submodule|TestManager_.*RootAndChildren|TestManager_.*MultiRepo'
```
## Files likely touched
- `apps/backend/internal/agentctl/server/process/manager.go`
- `apps/backend/internal/agentctl/server/process/manager_rescan.go`
- `apps/backend/internal/agentctl/server/process/manager_submodules.go` (new)
- `apps/backend/internal/agentctl/server/process/workspace_tracker.go`
- `apps/backend/internal/agentctl/server/process/workspace_files.go`
- `apps/backend/internal/agentctl/server/process/workspace_content_search.go`
- `apps/backend/internal/agentctl/server/process/manager_submodule_test.go` (new)
- Existing manager multi-repository/rescan tests when expectations require amendment
## Dependencies
None.
## Parallelism
`sequential` — this establishes the repository graph and manager contract used by every later task.
## Inputs
- Spec sections **What**, **API surface**, and **Failure modes**.
- ADR `2026-08-05-nested-submodules-as-repository-scopes.md`.
- Existing immediate-sibling discovery in `manager.go`, graph reconciliation in `manager_rescan.go`, and hardened submodule setup in `internal/worktree/submodule.go`.
## Output contract
Report the discovered graph shape, base-ref derivation, invalid-child behavior, files changed, exact tests and counts, blockers/risks, and synchronized task/plan status.
## Results
Implemented recursive initialized-submodule discovery with slash-delimited task-root-relative scopes, parent gitlink comparison anchors, root-plus-child tracker retention, safe-path checks, rescan/reconcile preservation, and subscription/poll-mode propagation. Uninitialized or invalid children are skipped without losing the parent.
Verification:
- `go test ./internal/agentctl/server/process -run 'Test(ReconcileRepositories_PrunesRemovedTrackerAndPreservesSubscription|Manager_DiscoversNestedSubmoduleScopesWithStableAnchors|Manager_RescanDiscoversNewNestedSubmoduleAndRetainsRoot|Manager_SkipsUninitializedSubmoduleWithoutLosingRoot)' -count=1` — 4 passed.
- `go test ./internal/agentctl/server/process -count=1` — 584 passed, including the post-rebase task-root rescan regression.
- `make -C apps/backend test` — passed; `make -C apps/backend lint` — 0 issues.
@@ -0,0 +1,61 @@
---
id: "02-aggregate-submodule-git-data"
title: "Aggregate root and submodule Git data"
status: done
wave: 1
depends_on: ["01-discover-nested-repository-scopes"]
plan: "plan.md"
spec: "../../specs/ui/submodule-review.md"
---
# Task 02: Aggregate root and submodule Git data
## Acceptance
- Unpinned status, log, and cumulative-diff requests include a real workspace root plus all initialized submodule scopes in stable order, while bare multi-repository task roots remain excluded.
- Cumulative files carry nested `repository_name`, repository-relative `path`, and the parent-derived `base_ref`; one scope's failure preserves successful results according to the existing endpoint contract.
- Root file/ref access, LSP/search consumers, cancellation, deterministic merges, and class-aware Git admission retain their existing behavior when named submodule scopes exist.
## Verification
```bash
cd apps/backend && go test ./internal/agentctl/server/api -run 'Test.*SubmoduleReview|TestMultiRepoReview|TestGitStatusMulti'
cd apps/backend && go test ./internal/agentctl/server/process -run 'TestManager_.*RepoScopes|TestManager_RepoSubpaths'
```
## Files likely touched
- `apps/backend/internal/agentctl/server/process/manager.go`
- `apps/backend/internal/agentctl/server/api/git.go`
- `apps/backend/internal/agentctl/server/api/workspace.go`
- `apps/backend/internal/agentctl/server/api/git_multi_repo_review_test.go`
- `apps/backend/internal/agentctl/server/api/git_status_fresh_concurrency_test.go`
- `apps/backend/internal/agentctl/server/process/manager_multi_repo_stream_test.go`
- Runtime client or gateway status tests only if the unchanged JSON shape exposes a compatibility gap
## Dependencies
Task 01.
## Parallelism
`sequential` — it consumes and finalizes the manager contract introduced by Task 01.
## Inputs
- Spec **API surface** and partial-failure scenarios.
- Existing multi-repository fan-out in `api/git.go` and root-content guard in `api/workspace.go`.
- ADR `2026-08-02-class-aware-git-subprocess-admission.md`.
## Output contract
Report endpoint shapes, ordering, partial failures, root-access audit, files changed, exact tests and counts, blockers/risks, and synchronized task/plan status.
## Results
Implemented ordered root/direct/nested repository-scope fan-out for status, log, and cumulative diff, preserving repository-relative paths, nested names, stable parent-derived base refs, partial failures, and root file access. Bare task roots retain their existing named-only behavior.
Verification:
- `go test ./internal/agentctl/server/api -run 'Test(NestedSubmoduleReviewEndpointsIncludeRootAndStableChildBase|MultiRepoReviewEndpoints)' -count=1` — 3 passed.
- `make -C apps/backend test` — passed; `make -C apps/backend build` — completed.
@@ -0,0 +1,78 @@
---
id: "03-present-submodule-review-hierarchy"
title: "Present submodule review hierarchy"
status: done
wave: 2
depends_on: ["02-aggregate-submodule-git-data"]
plan: "plan.md"
spec: "../../specs/ui/submodule-review.md"
---
# Task 03: Present submodule review hierarchy
## Acceptance
- Review retains mixed root and named statuses, keeps `(repository_name, path)` identities distinct, and applies identical source precedence in the Changes panel and expanded dialog.
- Desktop builds the task-root directory hierarchy with pinned, accessible submodule boundary nodes; mobile sticky headers expose the same scope without adding a second navigator or horizontal page overflow.
- Parent gitlink rows are hidden only when matching child file diffs are available, and remain visible for unavailable children.
## Verification
```bash
cd apps && pnpm install --frozen-lockfile
cd apps && pnpm --filter @kandev/web test -- hooks/domains/session/use-review-sources.test.ts components/review/review-dialog.build-files.test.ts components/review/types.multi-repo.test.ts components/review/review-file-tree.test.tsx
cd apps/web && pnpm run typecheck
cd apps && pnpm --filter @kandev/web i18n:check
```
## Files likely touched
- `apps/web/hooks/domains/session/use-review-sources.ts`
- `apps/web/hooks/domains/session/use-review-sources.test.ts`
- `apps/web/components/task/use-review-dialog.ts`
- `apps/web/components/task/use-review-dialog.test.ts`
- `apps/web/components/review/review-dialog.tsx`
- `apps/web/components/review/review-dialog.build-files.test.ts`
- `apps/web/components/review/types.ts`
- `apps/web/components/review/types.multi-repo.test.ts`
- `apps/web/components/review/review-file-tree.tsx`
- `apps/web/components/review/review-file-tree.test.tsx`
- Repository/diff group header component and focused test identified during implementation
- `apps/web/src/locales/en/common.json`
- `apps/web/src/locales/pseudo/common.json`
## Dependencies
Task 02.
## Parallelism
`parallel-safe` with Task 04 after Task 02: this task owns Review source/presentation files and locale catalogs; Task 04 owns Git operation derivation/dispatch files.
## Mobile design contract
- Desktop outcome: the existing Review tree embeds submodule boundaries in the file hierarchy and the diff list remains the single scroll owner.
- Mobile entry point: the existing task Changes/Review control opens the existing full-height Review dialog.
- Nearest shipped exemplar: `apps/web/e2e/tests/review/mobile-review-multi-pr.spec.ts` for the focused phone Review surface and sticky repository context.
- Presentation: keep direct full-height Review; do not introduce a drawer because reviewing a dense diff is primary, deep content rather than a temporary choice.
- Shared logic: source identity, filtering, selection, review hashes, and comments remain common; only the already-existing desktop tree versus mobile sticky-header presentation differs.
- Mobile proof: Task 05 verifies the visible scope label, diff access, touch path, containment, and horizontal-overflow contract.
## Inputs
- Spec **What**, mobile scenario, duplicate-path scenario, and gitlink fallback.
- `/mobile-parity` full-height dense-content guidance and existing `ReviewDialogSurface` composition.
- Existing composite review keys and multi-repository tree tests.
## Output contract
Report hierarchy semantics, accessibility/i18n keys, mobile parity, files changed, exact tests and counts, rendered-check evidence or blocker, risks, and synchronized task/plan status.
## Results
Implemented mixed root/named source normalization, nested hierarchy construction, gitlink suppression with unavailable-child fallback, composite review identities, visible/accessibility-aware submodule markers, and localized desktop/mobile scope labels. Added a dedicated repository-summary helper to keep the Changes controls within lint thresholds.
Verification:
- Focused Vitest command covering the 9 changed Review/source/order files — 118 passed.
- `pnpm run typecheck` — passed; `pnpm run lint` — passed with zero warnings; `pnpm run i18n:check` and `pnpm run i18n:ratchet` — passed.
@@ -0,0 +1,59 @@
---
id: "04-order-nested-git-mutations"
title: "Order nested Git mutations"
status: done
wave: 2
depends_on: ["02-aggregate-submodule-git-data"]
plan: "plan.md"
spec: "../../specs/ui/submodule-review.md"
---
# Task 04: Order nested Git mutations
## Acceptance
- File derivation and controls retain a real empty root scope alongside named children and route duplicate relative paths by explicit repository identity.
- Dependency-sensitive multi-scope operations execute deepest repository paths first, preserve sibling parallelism and partial results, and do not attempt an ancestor after its child fails.
- Commit-all stages and commits changed child scopes before parents so parent commits record new gitlinks; explicit single-scope and sibling-only operations remain unchanged.
## Verification
```bash
cd apps && pnpm install --frozen-lockfile
cd apps && pnpm --filter @kandev/web test -- hooks/domains/session/use-session-git-grouping.test.ts hooks/domains/session/use-session-git-repository-order.test.ts
cd apps/web && pnpm run typecheck
```
## Files likely touched
- `apps/web/hooks/domains/session/use-session-git.ts`
- `apps/web/hooks/domains/session/use-session-git-grouping.test.ts`
- `apps/web/hooks/domains/session/use-session-git-repository-order.test.ts` (new)
- `apps/web/hooks/use-git-operations.ts` only if the existing single-scope primitive cannot express stage-plus-commit per wave
## Dependencies
Task 02.
## Parallelism
`parallel-safe` with Task 03 after Task 02: the owned operation/dispatch files are disjoint from Review presentation and locale files.
## Inputs
- Spec stage/discard/commit behavior and child-failure scenario.
- ADR deepest-first mutation invariant.
- Existing `fanOutAcrossRepos`, `aggregatePerRepoResults`, `pendingKey`, and per-repository stage/commit primitives.
## Output contract
Report wave ordering, failure propagation, files changed, exact tests and counts, blockers/risks, and synchronized task/plan status.
## Results
Implemented deepest-first repository-scope waves for stage, unstage, discard, and commit fan-out. Siblings remain independent, failed children block only their ancestors, and UI stage/commit controls disable while another operation is in flight to prevent overlapping mutations.
Verification:
- `use-session-git-repository-order.test.ts` and the surrounding focused Review/Git Vitest suite — 118 passed.
- `pnpm run typecheck` and `pnpm run lint` — passed.
@@ -0,0 +1,60 @@
---
id: "05-prove-submodule-review"
title: "Prove responsive submodule review"
status: done
wave: 3
depends_on: ["03-present-submodule-review-hierarchy", "04-order-nested-git-mutations"]
plan: "plan.md"
spec: "../../specs/ui/submodule-review.md"
---
# Task 05: Prove responsive submodule review
## Acceptance
- A real disposable parent/direct/nested submodule graph produces parent and child textual diffs in desktop Review with marked nested boundaries, isolated duplicate paths, and no duplicate gitlink row.
- The user-facing commit-all path commits children before parents and the resulting parent tree records each new child commit.
- Mobile Review exposes submodule scope and file diff through its existing touch path, stays viewport-contained, and has no document-level horizontal overflow.
## Verification
```bash
cd apps && pnpm install --frozen-lockfile
cd apps/web && pnpm e2e:run tests/review/submodule-review.spec.ts
cd apps/web && pnpm e2e:run --project mobile-chrome tests/review/mobile-submodule-review.spec.ts
```
## Files likely touched
- `apps/web/e2e/tests/review/submodule-review-helpers.ts` (new)
- `apps/web/e2e/tests/review/submodule-review.spec.ts` (new)
- `apps/web/e2e/tests/review/mobile-submodule-review.spec.ts` (new)
- Existing Review page object only when a reusable user action is missing
## Dependencies
Tasks 03 and 04.
## Parallelism
`sequential` — this is the integrated proof over the backend and both frontend tasks.
## Inputs
- Every spec scenario involving rendered Review, duplicate paths, commit ordering, and phone parity.
- `/e2e` managed runner, production-build, disposable-fixture, and UI-assertion guidance.
- `/mobile-parity` mobile-chrome naming, geometry, touch-target, and overflow requirements.
## Output contract
Report desktop/mobile test counts, discovered Playwright projects, commit/tree assertions, screenshots or other rendered evidence, disposable-repository cleanup, changed files, blockers/risks, and synchronized task/plan status.
## Results
Added disposable parent/outer/inner repository fixtures using the direct local executor, recursive initialization, UI assertions for duplicate `README.md` identities and gitlink suppression, deepest-first commit/tree checks, and mobile geometry/overflow assertions. Cleanup removes the per-test source tree in `finally`.
Verification:
- `pnpm run build:vite` — passed.
- `pnpm run e2e:run --host --no-build --project chromium tests/review/submodule-review.spec.ts` — 1 passed.
- `pnpm run e2e:run --host --no-build --project mobile-chrome tests/review/mobile-submodule-review.spec.ts` — 1 passed.
@@ -0,0 +1,58 @@
---
id: "06-document-submodule-review"
title: "Document nested submodule review"
status: done
wave: 3
depends_on: ["03-present-submodule-review-hierarchy"]
plan: "plan.md"
spec: "../../specs/ui/submodule-review.md"
---
# Task 06: Document nested submodule review
## Acceptance
- Public Review guidance explains that initialized direct and nested submodule files appear with repository scope and that unavailable children fall back to the gitlink.
- Public feature status names nested initialized submodule support without implying that Kandev creates or coordinates pull requests for submodule remotes.
- Public docs validation passes and internal spec/ADR/plan links resolve.
## Verification
```bash
node --test scripts/validate-public-docs.test.mjs
node scripts/validate-public-docs.mjs
rg -n "submodule|Nested Submodule Review" docs/public docs/specs docs/decisions docs/plans/submodule-review
```
## Files likely touched
- `docs/public/sessions-and-review.md`
- `docs/public/feature-status.md`
## Dependencies
Task 03 for final visible terminology.
## Parallelism
`parallel-safe` with Task 05 after Task 03 because public docs and E2E files are disjoint.
## Inputs
- Spec **What**, **Failure modes**, and **Out of scope**.
- ADR consequences for separate Git-host workflows.
- Existing public Review section and feature-status table.
## Output contract
Report public pages and Diataxis classifications, wording boundaries, exact validation results, files changed, blockers/risks, and synchronized task/plan status.
## Results
Updated the public Sessions and Review guide and Feature Status reference with initialized direct/nested submodule scope behavior, anchored comparisons, unavailable-child fallback, and separate submodule PR workflows.
Verification:
- `node --test scripts/validate-public-docs.test.mjs` — 58 passed.
- `node scripts/validate-public-docs.mjs` — 41 published pages validated.
- `rg -n "submodule|Nested Submodule Review" docs/public docs/specs docs/decisions docs/plans/submodule-review` — references present.
+1 -1
View File
@@ -61,7 +61,7 @@ Released builds can lag this page. Check **Settings > System > About** or `kande
| Capability | Status | Current boundary |
|---|---|---|
| Structured chat, tool activity, files, integrated PTYs, and session lifecycle | Dependency-bound | Panel availability follows the task environment, agent protocol, executor, and viewport. A terminal needs a materialized task environment. |
| Changes and cumulative Review | Supported | Supports staged, unstaged, commit, and multi-repository views; persisted reviewed hashes, stale detection, and anchored comments are implemented. Discard is destructive and permanent. |
| Changes and cumulative Review | Supported | Supports staged, unstaged, commit, multi-repository, and initialized direct/nested submodule views; repository-scoped paths, persisted reviewed hashes, stale detection, and anchored comments are implemented. Unavailable submodules retain their parent gitlink. Discard is destructive and permanent, and submodule pull requests remain separate repository workflows. |
| Agent-authored code walkthrough | Dependency-bound | One saved walkthrough per task is supported through task MCP. A new walkthrough replaces the old one; anchors can drift, and the backend does not prove that referenced file content is still current. |
| GitHub pull-request review, checks, merge, and task PR automation | Dependency-bound | Requires GitHub credentials and repository permissions. GitHub-only task-level controls apply to every linked PR and can auto-fix feedback, auto-merge, and notify agents when their review is requested or when a PR merges or closes. Lifecycle messages are factual, immutable server-owned templates with only a canonical PR URL as dynamic content. Review-request matching uses the task workspace's connected GitHub account and a quiet baseline; delivery uses an existing task session or queue and never creates a new session. Azure has a create-PR path only; other Git hosts do not get the full task PR panel. |
| GitLab merge-request review and task MR lifecycle notifications | Dependency-bound | Requires GitLab credentials and project permissions. GitLab has no auto-fix or auto-merge automation; task-level controls notify agents when their review is requested (assignment as a reviewer, since GitLab has no distinct review-request event), or when a linked MR merges or closes. Lifecycle messages are factual, immutable server-owned templates with only a canonical MR URL as dynamic content, delivered through the same durable queue as GitHub's. |
+3 -1
View File
@@ -161,7 +161,9 @@ Kandev hides the action instead of guessing when a repository is local-only, uns
## Review a diff
Select **Review** in the Changes header. Kandev builds a repository-aware file list by merging available uncommitted, cumulative committed, and linked-PR files. When a path occurs in more than one source, the uncommitted version wins deduplication.
Select **Review** in the Changes header. Kandev builds a repository-aware file list by merging available uncommitted, cumulative committed, and linked-PR files. Initialized direct and nested Git submodules appear under their task-workspace scopes, so a submodule's `README.md` remains distinct from the parent repository's `README.md`. When a path occurs in more than one source within the same repository, the uncommitted version wins deduplication.
Review compares each submodule with the gitlink commit recorded by its parent and marks the submodule boundaries in the file hierarchy and diff headers. If a declared submodule is unavailable or uninitialized, Kandev keeps the parent's gitlink change visible instead of hiding the only available evidence. Pull requests for submodule repositories remain separate repository workflows; Review does not create or coordinate them.
When a task has multiple linked pull requests, use the PR selector in the Changes diff header or Review toolbar to inspect one PR revision at a time. The selection is scoped to that task for the current app session. Switching PRs replaces only the remote PR contribution; uncommitted and committed sources keep their normal precedence. Selecting a file from a specific PR row opens that exact PR revision, even when a sibling PR changes the same path.
+1
View File
@@ -147,6 +147,7 @@ Subscription quota tracking and per-agent cheap-model profile routing.
| [acp-shell-command-output](ui/acp-shell-command-output.md) | shipped |
| [acp-model-configuration-summary](ui/acp-model-configuration-summary.md) | shipped |
| [review-file-status](ui/review-file-status.md) | building |
| [submodule-review](ui/submodule-review.md) | shipped |
| [review-markdown-preview](ui/review-markdown-preview.md) | draft |
| [sidebar-view-creation](ui/sidebar-view-creation.md) | shipped |
| [command-panel sidebar task reveal](ui/command-panel-sidebar-task-reveal.md) | draft |
+70
View File
@@ -0,0 +1,70 @@
---
status: shipped
created: 2026-08-05
owner: kandev
---
# Nested Submodule Review
Decision: [ADR-2026-08-05-nested-submodules-as-repository-scopes](../../decisions/2026-08-05-nested-submodules-as-repository-scopes.md)
## Why
Repositories that use Git submodules currently show only a changed gitlink commit in Changes and Review. A reviewer cannot inspect, comment on, or approve the actual files changed inside the submodule without leaving Kandev and reconstructing the nested diff manually.
## What
- Changes and Review include changed files from every initialized submodule present when the task repository graph is discovered, including recursively nested submodules.
- Parent-repository files and submodule files can appear in the same Review session. Each file keeps a repository-relative path and an unambiguous repository scope.
- Review renders submodule scopes in their task-workspace directory hierarchy and gives each submodule boundary a small visible and accessible indication. A nested submodule appears beneath its parent submodule rather than as an unrelated top-level repository.
- When Kandev can show the child files for a changed submodule, Review omits the parent's raw gitlink-only row. If the child is unavailable, Review retains the parent gitlink change instead of hiding the only evidence of the change.
- Uncommitted and committed submodule changes use the same source precedence, diff limits, reviewed hashes, stale detection, anchored comments, findings, filtering, status markers, and file-opening behavior as other Review files.
- Each submodule compares against the gitlink commit recorded by its parent at the parent's comparison anchor. The comparison does not expand to unrelated history from the submodule's default branch.
- Stage, unstage, discard, file-at-ref, and per-file editing actions execute in the file's owning repository scope.
- A commit-all operation that covers nested repositories commits the deepest changed submodules before their parents so every parent commit records the resulting child gitlink. Independent sibling repositories at the same depth may proceed in parallel.
- Desktop keeps the existing Review split surface and file tree. Phone Review keeps its existing full-height diff surface; submodule identity is visible in the sticky repository/diff header, so no required cue depends on the desktop-only file tree or hover.
## API surface
No route names or top-level payload shapes change.
- `GET /api/v1/git/status/multi?fresh=<bool>` returns one status entry for each tracked repository scope. A Git workspace root uses `repository_name = ""`; initialized submodules use their slash-delimited task-workspace-relative path.
- `GET /api/v1/git/log` and `GET /api/v1/git/cumulative-diff` include the workspace root and initialized submodule scopes when no `repo` query parameter pins one scope.
- Existing `repo=<subpath>` mutation and file routes accept the same slash-delimited submodule scope and continue to receive repository-relative file paths.
- Existing workspace-stream Git events use the same `repository_name` convention. Mixed empty and named repository entries are valid for one session.
For a multi-repository task, a top-level attached repository keeps its existing scope name, for example `frontend`; its nested submodule uses the combined task-root-relative scope, for example `frontend/vendor/parser`.
## Failure modes
| Condition | Observable behavior |
|---|---|
| A declared submodule is uninitialized, inaccessible, or its Git metadata is invalid | The child scope is skipped, the rest of the repository graph remains reviewable, and the parent gitlink change remains visible when changed. |
| One submodule status, log, or cumulative-diff request fails | Other repository scopes remain available and the existing per-repository partial-failure behavior applies. |
| A submodule comparison gitlink cannot be resolved from the parent comparison tree | Uncommitted child changes remain visible; committed-range collection follows the existing unresolved-base failure behavior for that scope without substituting another repository's base. |
| The same relative file path changes in a parent, sibling repository, and submodule | Review keeps separate identities and comments for every `(repository_name, path)` pair. |
| Commit-all fails in one child scope | Ancestor commits that depend on that child are not attempted; independent scopes report their own results through the existing partial-success UI. |
| A submodule is added or removed after agentctl's repository graph is built | It becomes part of nested review after the existing workspace rescan or session restart; automatic hot discovery of arbitrary graph changes is not required in this iteration. |
## Scenarios
- **GIVEN** a task repository with an initialized submodule containing an uncommitted file edit and a changed parent file, **WHEN** Review opens, **THEN** both textual diffs appear and the submodule file sits beneath a marked submodule boundary.
- **GIVEN** a submodule containing another initialized submodule, **WHEN** files change in both children, **THEN** Review shows both nested directory boundaries and each file's repository-relative diff.
- **GIVEN** a submodule commit made after the task comparison point, **WHEN** the session reloads or agentctl restarts and Review opens, **THEN** the committed file diff is computed from the parent-recorded gitlink commit and remains visible.
- **GIVEN** a parent gitlink change whose child file diffs are available, **WHEN** Review builds its file list, **THEN** it shows the child files without a duplicate raw gitlink-only row.
- **GIVEN** an unavailable submodule whose gitlink changed, **WHEN** Review opens, **THEN** the parent gitlink row remains visible and other repositories continue to render.
- **GIVEN** `README.md` changes in the parent and `README.md` changes in a nested submodule, **WHEN** the user reviews, comments on, stages, or discards either file, **THEN** the action and review state apply only to the selected repository scope.
- **GIVEN** staged changes in a nested submodule and its parent, **WHEN** the user commits all reviewed work, **THEN** Kandev commits the child first and the parent commit records the child's new commit ID.
- **GIVEN** a phone-sized viewport, **WHEN** the user opens Review for a submodule file, **THEN** the sticky diff header identifies the submodule scope and the file remains reviewable without document-level horizontal overflow.
## Out of scope
- Fetching or initializing a submodule that the existing hardened worktree setup could not initialize.
- Automatically creating, linking, pushing, or merging pull requests for submodule remotes. Those remain separate Git-host repository workflows.
- Treating arbitrary nested Git repositories that are not declared submodules as part of the parent repository graph.
- Automatically hot-reloading newly added or removed submodules without the existing workspace rescan or a session restart.
- Adding a new mobile file-tree navigator; phone Review keeps its current focused diff composition.
## Implementation plan
See [Nested Submodule Review plan](../../plans/submodule-review/plan.md).