fix: resolve duplicated tenant-path segment in team shared-workspace resolution (#1322)

Root cause: internal/agent/loop_context.go re-applied tenant scoping
(TenantLayer/TenantID) on top of l.dataDir, which was already
tenant-scoped upstream in internal/agent/resolver.go, producing paths
like /app/workspace/tenants/<slug>/tenants/<slug>/teams/<teamID>
instead of the correct /app/workspace/tenants/<slug>/teams/<teamID>.

Also consolidates all previously-duplicated tenant-path-joining logic
(config.TenantDataDir, config.TenantWorkspace, tools.TenantLayer,
workspace.tenantPath) into a single canonical config.TenantScopedDir
function in internal/config/tenant_paths.go, with all other
implementations now delegating to it.

Co-authored-by: Bruno Clermont <bruno.clermont@gmail.com>

internal/agent/loop_context.go

@@ -212,9 +212,11 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup
 		ctx = tools.WithToolTeamID(ctx, req.TeamID)
 		// Team root for dispatched tasks: resolve the UserChatLayer-stripped root
 		// so the dispatched agent can still read peer-scoped files in the same team.
+		// l.dataDir is already tenant-scoped (see resolver.go: config.TenantDataDir),
+		// so TenantLayer must NOT be reapplied here — doing so double-joins the
+		// tenant segment (tenants/<slug>/tenants/<slug>/teams/<id>).
 		if teamUUID, err := uuid.Parse(req.TeamID); err == nil && l.dataDir != "" {
 			teamRoot := tools.ResolveWorkspace(l.dataDir,
-				tools.TenantLayer(store.TenantIDFromContext(ctx), store.TenantSlugFromContext(ctx)),
 				tools.TeamLayer(teamUUID),
 			)
 			ctx = tools.WithToolTeamRoot(ctx, teamRoot)
@@ -244,9 +246,10 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup
 				wsChat = req.UserID
 			}
 			shared := tools.IsSharedWorkspace(team.Settings)
-			// Resolve team workspace via layered pipeline: tenant → team → user/chat.
+			// Resolve team workspace via layered pipeline: team → user/chat.
+			// l.dataDir is already tenant-scoped (see resolver.go: config.TenantDataDir) —
+			// do NOT reapply TenantLayer here, it would double-join the tenant segment.
 			wsDir := tools.ResolveWorkspace(l.dataDir,
-				tools.TenantLayer(store.TenantIDFromContext(ctx), store.TenantSlugFromContext(ctx)),
 				tools.TeamLayer(team.ID),
 				tools.UserChatLayer(wsChat, shared),
 			)
@@ -264,7 +267,6 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup
 			// the same team. Writes still default to wsDir above; team root only
 			// widens the allowed-prefix set for path boundary checks.
 			teamRoot := tools.ResolveWorkspace(l.dataDir,
-				tools.TenantLayer(store.TenantIDFromContext(ctx), store.TenantSlugFromContext(ctx)),
 				tools.TeamLayer(team.ID),
 			)
 			ctx = tools.WithToolTeamRoot(ctx, teamRoot)
@@ -294,12 +296,14 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup
 			// Filesystem path segment must use agent_key, not UUID — matches
 			// the v2 path in loop_pipeline_callbacks.go and the session_key
 			// anchor. See docs/agent-identity-conventions.md.
-			AgentID:    l.id,
-			AgentType:  l.agentType,
-			UserID:     req.UserID,
-			ChatID:     req.ChatID,
-			TenantID:   store.TenantIDFromContext(ctx).String(),
-			TenantSlug: store.TenantSlugFromContext(ctx),
+			AgentID:   l.id,
+			AgentType: l.agentType,
+			UserID:    req.UserID,
+			ChatID:    req.ChatID,
+			// TenantID/TenantSlug intentionally left empty: l.dataDir (BaseDir below)
+			// is already tenant-scoped (see resolver.go: config.TenantDataDir). Setting
+			// TenantID here would make resolveTeam/resolvePersonal reapply tenantPath()
+			// and double-join the tenant segment (tenants/<slug>/tenants/<slug>/...).
 			PeerKind:   req.PeerKind,
 			TeamID:     teamIDPtr,
 			TeamConfig: teamWSConfig,

internal/config/tenant_paths.go

@@ -11,45 +11,46 @@ import (
 // Duplicated here to avoid an import cycle (store imports config).
 var masterTenantID = uuid.MustParse("0193a5b0-7000-7000-8000-000000000001")
 
+// TenantScopedDir is the single canonical implementation for joining a base
+// directory with a tenant-scoped "tenants/{slug-or-id}" subdirectory.
+// It is the sole source of truth for this join: TenantDataDir and
+// TenantWorkspace (uuid.UUID-typed call sites) delegate to it, and
+// internal/workspace's team/personal workspace resolution (string-typed
+// tenant identifiers, not always parseable as uuid.UUID) delegates to it too.
+//
+// Master tenant (tenantIDStr == "" or the master sentinel) returns base
+// unchanged (backward compat).
+// Empty tenantSlug (transient DB lookup failure) would otherwise resolve to
+// the tenants/ parent dir, granting cross-tenant access — falls back to an
+// ID-based path instead.
+// Includes defense-in-depth path traversal protection against a malicious slug.
+func TenantScopedDir(base, tenantIDStr, tenantSlug string) string {
+	if tenantIDStr == "" || tenantIDStr == masterTenantID.String() {
+		return base
+	}
+	if tenantSlug == "" {
+		return filepath.Join(base, "tenants", tenantIDStr)
+	}
+	result := filepath.Join(base, "tenants", tenantSlug)
+	tenantsBase := filepath.Join(base, "tenants") + string(filepath.Separator)
+	if !strings.HasPrefix(result+string(filepath.Separator), tenantsBase) {
+		return filepath.Join(base, "tenants", tenantIDStr)
+	}
+	return result
+}
+
 // TenantDataDir returns the data directory root for a tenant.
 // Master tenant returns dataDir unchanged (backward compat).
 // Other tenants return dataDir/tenants/{slug}/.
 func TenantDataDir(dataDir string, tenantID uuid.UUID, tenantSlug string) string {
-	if tenantID == masterTenantID {
-		return dataDir
-	}
-	// Empty slug (transient DB lookup failure) would resolve to the tenants/ parent dir,
-	// granting cross-tenant access. Fall back to UUID-based path instead.
-	if tenantSlug == "" {
-		return filepath.Join(dataDir, "tenants", tenantID.String())
-	}
-	result := filepath.Join(dataDir, "tenants", tenantSlug)
-	// Defense-in-depth: prevent path traversal via malicious slug.
-	tenantsBase := filepath.Join(dataDir, "tenants") + string(filepath.Separator)
-	if !strings.HasPrefix(result+string(filepath.Separator), tenantsBase) {
-		return filepath.Join(dataDir, "tenants", tenantID.String())
-	}
-	return result
+	return TenantScopedDir(dataDir, tenantID.String(), tenantSlug)
 }
 
 // TenantWorkspace returns the workspace root for a tenant.
 // Master tenant returns workspace unchanged (backward compat).
 // Other tenants return workspace/tenants/{slug}/.
 func TenantWorkspace(workspace string, tenantID uuid.UUID, tenantSlug string) string {
-	if tenantID == masterTenantID {
-		return workspace
-	}
-	// Empty slug (transient DB lookup failure) would resolve to the tenants/ parent dir,
-	// granting cross-tenant access. Fall back to UUID-based path instead.
-	if tenantSlug == "" {
-		return filepath.Join(workspace, "tenants", tenantID.String())
-	}
-	result := filepath.Join(workspace, "tenants", tenantSlug)
-	tenantsBase := filepath.Join(workspace, "tenants") + string(filepath.Separator)
-	if !strings.HasPrefix(result+string(filepath.Separator), tenantsBase) {
-		return filepath.Join(workspace, "tenants", tenantID.String())
-	}
-	return result
+	return TenantScopedDir(workspace, tenantID.String(), tenantSlug)
 }
 
 // TenantTeamDir returns the team workspace directory for a tenant.

internal/config/tenant_paths_test.go

@@ -0,0 +1,78 @@
+package config
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/google/uuid"
+)
+
+func TestTenantScopedDir_MasterTenant(t *testing.T) {
+	got := TenantScopedDir("/data", masterTenantID.String(), "master")
+	if got != "/data" {
+		t.Errorf("master tenant should be no-op, got %s", got)
+	}
+}
+
+func TestTenantScopedDir_EmptyTenantID(t *testing.T) {
+	got := TenantScopedDir("/data", "", "acme")
+	if got != "/data" {
+		t.Errorf("empty tenantID should be no-op, got %s", got)
+	}
+}
+
+func TestTenantScopedDir_UsesSlug(t *testing.T) {
+	tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002")
+	got := TenantScopedDir("/data", tid.String(), "acme")
+	want := filepath.Join("/data", "tenants", "acme")
+	if got != want {
+		t.Errorf("got %s, want %s", got, want)
+	}
+}
+
+func TestTenantScopedDir_EmptySlugFallsBackToID(t *testing.T) {
+	tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002")
+	got := TenantScopedDir("/data", tid.String(), "")
+	want := filepath.Join("/data", "tenants", tid.String())
+	if got != want {
+		t.Errorf("got %s, want %s", got, want)
+	}
+}
+
+func TestTenantScopedDir_TraversalDefense(t *testing.T) {
+	tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002")
+	got := TenantScopedDir("/data", tid.String(), "../../etc")
+	want := filepath.Join("/data", "tenants", tid.String())
+	if got != want {
+		t.Errorf("malicious slug should fall back to ID-based path, got %s, want %s", got, want)
+	}
+}
+
+// TestTenantDataDir_TenantWorkspace_DelegateToCanonical pins that both
+// exported wrappers produce identical output shapes via the single
+// canonical TenantScopedDir implementation — no independent join logic.
+func TestTenantDataDir_TenantWorkspace_DelegateToCanonical(t *testing.T) {
+	tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002")
+
+	dataDir := TenantDataDir("/data", tid, "acme")
+	wantDataDir := filepath.Join("/data", "tenants", "acme")
+	if dataDir != wantDataDir {
+		t.Errorf("TenantDataDir = %s, want %s", dataDir, wantDataDir)
+	}
+
+	workspace := TenantWorkspace("/ws", tid, "acme")
+	wantWorkspace := filepath.Join("/ws", "tenants", "acme")
+	if workspace != wantWorkspace {
+		t.Errorf("TenantWorkspace = %s, want %s", workspace, wantWorkspace)
+	}
+}
+
+func TestTenantTeamDir(t *testing.T) {
+	tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002")
+	teamID := uuid.MustParse("0193c000-0000-7000-8000-000000000003")
+	got := TenantTeamDir("/data", tid, "acme", teamID)
+	want := filepath.Join("/data", "tenants", "acme", "teams", teamID.String())
+	if got != want {
+		t.Errorf("got %s, want %s", got, want)
+	}
+}

internal/tools/workspace_resolver_test.go

@@ -2,6 +2,7 @@ package tools
 
 import (
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/google/uuid"
@@ -177,6 +178,31 @@ func TestResolveWorkspace_SharedTrue(t *testing.T) {
 	}
 }
 
+// TestResolveWorkspace_TenantScopedBaseDir_NoDoubleJoin pins a regression:
+// when the base dir passed in is ALREADY tenant-scoped (as internal/agent's
+// Loop.dataDir is — see resolver.go's config.TenantDataDir call), applying
+// TeamLayer alone must produce the tenant segment exactly once, matching the
+// path the web UI file-upload handler writes to (internal/http/files.go via
+// config.TenantWorkspace). Regression: an earlier version of internal/agent's
+// loop_context.go re-applied TenantLayer on top of this already-scoped base,
+// producing "tenants/acme/tenants/acme/teams/<id>".
+func TestResolveWorkspace_TenantScopedBaseDir_NoDoubleJoin(t *testing.T) {
+	teamID := uuid.MustParse("0193c000-0000-7000-8000-000000000003")
+	alreadyTenantScopedBase := filepath.Join("/data", "tenants", "acme")
+
+	got := ResolveWorkspace(alreadyTenantScopedBase,
+		TeamLayer(teamID),
+	)
+
+	want := filepath.Join("/data", "tenants", "acme", "teams", teamID.String())
+	if got != want {
+		t.Errorf("want %s, got %s", want, got)
+	}
+	if n := strings.Count(got, "tenants"+string(filepath.Separator)+"acme"); n != 1 {
+		t.Errorf("expected tenant segment to appear exactly once, got %d occurrences in %q", n, got)
+	}
+}
+
 func TestSanitizePathSegment(t *testing.T) {
 	tests := []struct {
 		input string

internal/workspace/resolver_impl.go

@@ -7,6 +7,8 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+
+	"github.com/nextlevelbuilder/goclaw/internal/config"
 )
 
 // masterTenantID is the sentinel UUID for the master/default tenant.
@@ -123,22 +125,13 @@ func (r *defaultResolver) resolvePersonal(p ResolveParams) *WorkspaceContext {
 
 // tenantPath returns tenant-scoped directory.
 // Master tenant returns base dir directly (backward compat with v2).
-// Uses slug when available (matches config.TenantWorkspace), falls back to UUID.
+// Uses slug when available (matches config.TenantWorkspace), falls back to
+// tenantID. Sanitizes both identifiers (workspace tenant IDs/slugs are not
+// always valid UUIDs, unlike config's uuid.UUID-typed call sites) before
+// delegating the actual join+traversal-defense logic to the single canonical
+// implementation, config.TenantScopedDir.
 func tenantPath(base, tenantID, tenantSlug string) string {
-	if tenantID == "" || tenantID == masterTenantID {
-		return base
-	}
-	segment := tenantSlug
-	if segment == "" {
-		segment = tenantID
-	}
-	result := filepath.Join(base, "tenants", sanitizeSegment(segment))
-	// Path traversal defense: ensure result stays under tenants/ base
-	tenantsBase := filepath.Join(base, "tenants") + string(filepath.Separator)
-	if !strings.HasPrefix(result+string(filepath.Separator), tenantsBase) {
-		return filepath.Join(base, "tenants", sanitizeSegment(tenantID))
-	}
-	return result
+	return config.TenantScopedDir(base, sanitizeSegment(tenantID), sanitizeSegment(tenantSlug))
 }
 
 // userChatSegment returns the isolation segment: chatID for group, userID for direct.

internal/workspace/resolver_impl_test.go

@@ -122,6 +122,46 @@ func TestResolve_TeamShared(t *testing.T) {
 	}
 }
 
+// TestResolve_TeamShared_TenantScopedBaseDir_NoDoubleJoin pins a regression:
+// callers whose BaseDir is already tenant-scoped (e.g. internal/agent's
+// Loop.dataDir, built via config.TenantDataDir) must pass an empty
+// TenantID/TenantSlug so resolveTeam's tenantPath() call is a no-op. An
+// earlier bug passed both an already-scoped BaseDir AND a non-empty
+// TenantID/TenantSlug into this same call, producing
+// ".../tenants/acme/tenants/acme/teams/<id>" for the agent-facing path,
+// while the web UI upload handler (config.TenantWorkspace, single join)
+// correctly produced ".../tenants/acme/teams/<id>".
+func TestResolve_TeamShared_TenantScopedBaseDir_NoDoubleJoin(t *testing.T) {
+	rawBase := t.TempDir()
+	alreadyTenantScopedBase := filepath.Join(rawBase, "tenants", "acme")
+	r := NewResolver()
+	teamID := "team-abc"
+
+	wc, err := r.Resolve(context.Background(), ResolveParams{
+		AgentID:    "agent-1",
+		AgentType:  "open",
+		UserID:     "user-1",
+		ChatID:     "chat-1",
+		TenantID:   "", // intentionally empty: BaseDir is already tenant-scoped
+		TenantSlug: "",
+		PeerKind:   "direct",
+		TeamID:     &teamID,
+		TeamConfig: &TeamWorkspaceConfig{WorkspaceScope: "shared"},
+		BaseDir:    alreadyTenantScopedBase,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	want := filepath.Join(rawBase, "tenants", "acme", "teams", "team-abc")
+	if wc.ActivePath != want {
+		t.Errorf("ActivePath = %q, want %q", wc.ActivePath, want)
+	}
+	if n := strings.Count(wc.ActivePath, filepath.Join("tenants", "acme")); n != 1 {
+		t.Errorf("expected tenant segment to appear exactly once, got %d occurrences in %q", n, wc.ActivePath)
+	}
+}
+
 func TestResolve_TeamIsolated(t *testing.T) {
 	base := t.TempDir()
 	r := NewResolver()