From 4b780bbffa3d6fe73f13cc445bc60a8fe1931ec0 Mon Sep 17 00:00:00 2001 From: viettranx Date: Wed, 25 Mar 2026 16:03:31 +0700 Subject: [PATCH] refactor(workspace): extract layered resolver pipeline for workspace path computation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace inline workspace path computation in loop_context.go with composable WorkspaceLayer pipeline (tenant → team → project → user/chat). Each layer is a pure function that appends a path segment or is a no-op. - New workspace_resolver.go: ResolveWorkspace, TenantLayer, TeamLayer, ProjectLayer (future), UserChatLayer, SanitizePathSegment - 16 unit tests covering all layer combinations - Migrate loop_context.go, loop_history.go, team_tasks_mutations.go - Move sanitizePathSegment from agent to tools package (exported) - Zero behavior change — identical paths for all scenarios --- internal/agent/loop_context.go | 60 +++---- internal/agent/loop_history.go | 15 +- internal/agent/loop_utils.go | 13 -- internal/tools/team_tasks_mutations.go | 21 +-- internal/tools/workspace_resolver.go | 80 +++++++++ internal/tools/workspace_resolver_test.go | 200 ++++++++++++++++++++++ 6 files changed, 323 insertions(+), 66 deletions(-) create mode 100644 internal/tools/workspace_resolver.go create mode 100644 internal/tools/workspace_resolver_test.go diff --git a/internal/agent/loop_context.go b/internal/agent/loop_context.go index fab68581..039b75cb 100644 --- a/internal/agent/loop_context.go +++ b/internal/agent/loop_context.go @@ -97,17 +97,16 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup ctx = tools.WithTeamTaskID(ctx, req.TeamTaskID) } - // Per-user workspace isolation. - // Workspace path comes from user_agent_profiles (includes channel segment - // for cross-channel isolation). Cached in userWorkspaces to avoid repeated DB queries. + // --- Workspace resolution (layered pipeline) --- + // Layer order: tenant → team → project (future) → user/chat + // Two entry modes: solo agent (base = l.workspace) or team context (base = l.dataDir). + // Result is always a single folder set via WithToolWorkspace. + + // Solo agent workspace: resolve base from user profile or agent config. isTeamSession := bootstrap.IsTeamSession(req.SessionKey) if l.workspace != "" && req.UserID != "" { cachedWs, loaded := l.userWorkspaces.Load(req.UserID) if !loaded { - // First request for this user: get/create profile → returns stored workspace. - // Also seeds per-user context files on first chat. - // Team-dispatched sessions skip seeding — members process tasks with full - // capabilities, no bootstrap/user onboarding needed. ws := l.workspace if l.ensureUserFiles != nil && !isTeamSession { var err error @@ -117,7 +116,6 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup ws = l.workspace } } - // Expand ~ and convert to absolute for filesystem operations. ws = config.ExpandHome(ws) if !filepath.IsAbs(ws) { ws, _ = filepath.Abs(ws) @@ -125,10 +123,11 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup l.userWorkspaces.Store(req.UserID, ws) cachedWs = ws } - effectiveWorkspace := cachedWs.(string) - if !l.shouldShareWorkspace(req.UserID, req.PeerKind) { - effectiveWorkspace = filepath.Join(effectiveWorkspace, sanitizePathSegment(req.UserID)) - } + // Apply user isolation layer via pipeline. + shared := l.shouldShareWorkspace(req.UserID, req.PeerKind) + effectiveWorkspace := tools.ResolveWorkspace(cachedWs.(string), + tools.UserChatLayer(tools.SanitizePathSegment(req.UserID), shared), + ) if l.shouldShareMemory() { ctx = store.WithSharedMemory(ctx) } @@ -143,46 +142,41 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup ctx = tools.WithToolWorkspace(ctx, l.workspace) } - // Team workspace handling: - // - Dispatched task (req.TeamWorkspace set): override default workspace so - // relative paths resolve to team workspace. Agent workspace is accessible - // via ToolTeamWorkspace for absolute-path access. - // - Direct chat (auto-resolved): keep agent workspace as default, team - // workspace accessible via absolute path. + // Team workspace: dispatched task overrides default workspace. if req.TeamWorkspace != "" { if err := os.MkdirAll(req.TeamWorkspace, 0755); err != nil { slog.Warn("failed to create team workspace directory", "workspace", req.TeamWorkspace, "error", err) } ctx = tools.WithToolTeamWorkspace(ctx, req.TeamWorkspace) - ctx = tools.WithToolWorkspace(ctx, req.TeamWorkspace) // default for relative paths + ctx = tools.WithToolWorkspace(ctx, req.TeamWorkspace) } if req.TeamID != "" { ctx = tools.WithToolTeamID(ctx, req.TeamID) } - // Auto-resolve team workspace for agents not dispatched via team task. - // Lead agents default to team workspace (primary job is team coordination). - // Non-lead members keep own workspace; team workspace is accessible via absolute path. - // resolvedTeamSettings caches team settings from workspace resolution - // to avoid re-querying when checking slow_tool notification config. + // Team workspace: auto-resolve for agents with team membership (not dispatched). + // Lead agents default to team workspace; non-lead members keep own workspace. var resolvedTeamSettings json.RawMessage if req.TeamWorkspace == "" && l.teamStore != nil && l.agentUUID != uuid.Nil { if team, _ := l.teamStore.GetTeamForAgent(ctx, l.agentUUID); team != nil { resolvedTeamSettings = team.Settings - // Shared workspace: scope by teamID only. Isolated (default): scope by chatID too. wsChat := req.ChatID if wsChat == "" { wsChat = req.UserID } - if tools.IsSharedWorkspace(team.Settings) { - wsChat = "" + shared := tools.IsSharedWorkspace(team.Settings) + // Resolve team workspace via layered pipeline: tenant → team → user/chat. + wsDir := tools.ResolveWorkspace(l.dataDir, + tools.TenantLayer(store.TenantIDFromContext(ctx), store.TenantSlugFromContext(ctx)), + tools.TeamLayer(team.ID), + tools.UserChatLayer(wsChat, shared), + ) + if err := os.MkdirAll(wsDir, 0750); err != nil { + slog.Warn("failed to create team workspace directory", "workspace", wsDir, "error", err) } - tenantBase := config.TenantWorkspace(l.dataDir, store.TenantIDFromContext(ctx), store.TenantSlugFromContext(ctx)) - if wsDir, err := tools.WorkspaceDir(tenantBase, team.ID, wsChat); err == nil { - ctx = tools.WithToolTeamWorkspace(ctx, wsDir) - if team.LeadAgentID == l.agentUUID { - ctx = tools.WithToolWorkspace(ctx, wsDir) - } + ctx = tools.WithToolTeamWorkspace(ctx, wsDir) + if team.LeadAgentID == l.agentUUID { + ctx = tools.WithToolWorkspace(ctx, wsDir) } if req.TeamID == "" { ctx = tools.WithToolTeamID(ctx, team.ID.String()) diff --git a/internal/agent/loop_history.go b/internal/agent/loop_history.go index 9b6f002a..b6a84b3e 100644 --- a/internal/agent/loop_history.go +++ b/internal/agent/loop_history.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "log/slog" - "path/filepath" "strings" "sync" "time" @@ -88,13 +87,15 @@ func (l *Loop) buildMessages(ctx context.Context, history []providers.Message, s // When workspace sharing is enabled, show the base workspace without user subfolder. promptWorkspace := l.workspace if l.agentUUID != uuid.Nil && userID != "" && l.workspace != "" { + shared := l.shouldShareWorkspace(userID, peerKind) if cachedWs, ok := l.userWorkspaces.Load(userID); ok { - promptWorkspace = cachedWs.(string) - if !l.shouldShareWorkspace(userID, peerKind) { - promptWorkspace = filepath.Join(promptWorkspace, sanitizePathSegment(userID)) - } - } else if !l.shouldShareWorkspace(userID, peerKind) { - promptWorkspace = filepath.Join(l.workspace, sanitizePathSegment(userID)) + promptWorkspace = tools.ResolveWorkspace(cachedWs.(string), + tools.UserChatLayer(tools.SanitizePathSegment(userID), shared), + ) + } else { + promptWorkspace = tools.ResolveWorkspace(l.workspace, + tools.UserChatLayer(tools.SanitizePathSegment(userID), shared), + ) } } diff --git a/internal/agent/loop_utils.go b/internal/agent/loop_utils.go index aea25d6f..12685dd9 100644 --- a/internal/agent/loop_utils.go +++ b/internal/agent/loop_utils.go @@ -10,19 +10,6 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/tools" ) -// sanitizePathSegment makes a userID safe for use as a directory name. -// Replaces colons, spaces, and other unsafe chars with underscores. -func sanitizePathSegment(s string) string { - var b strings.Builder - for _, r := range s { - if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' || r == '_' { - b.WriteRune(r) - } else { - b.WriteByte('_') - } - } - return b.String() -} // scanWebToolResult checks web_fetch/web_search tool results for prompt injection patterns. // If detected, prepends a warning (doesn't block — may be false positive). diff --git a/internal/tools/team_tasks_mutations.go b/internal/tools/team_tasks_mutations.go index 049144ea..cf5c8d28 100644 --- a/internal/tools/team_tasks_mutations.go +++ b/internal/tools/team_tasks_mutations.go @@ -10,7 +10,6 @@ import ( "github.com/google/uuid" - "github.com/nextlevelbuilder/goclaw/internal/config" "github.com/nextlevelbuilder/goclaw/internal/store" "github.com/nextlevelbuilder/goclaw/internal/tracing" "github.com/nextlevelbuilder/goclaw/pkg/protocol" @@ -142,19 +141,15 @@ func (t *TeamTasksTool) executeCreate(ctx context.Context, args map[string]any) chatID := ToolChatIDFromCtx(ctx) - // Shared workspace: scope by teamID only. Isolated (default): scope by chatID too. - wsChat := chatID - if IsSharedWorkspace(team.Settings) { - wsChat = "" - } - - // Compute the team workspace directory (tenant-scoped) so member agents - // write files to the shared team folder instead of their own personal workspace. + // Compute team workspace via layered pipeline: tenant → team → user/chat. + shared := IsSharedWorkspace(team.Settings) taskMeta := make(map[string]any) - tenantBase := config.TenantWorkspace(t.manager.dataDir, store.TenantIDFromContext(ctx), store.TenantSlugFromContext(ctx)) - if teamWsDir, err := WorkspaceDir(tenantBase, team.ID, wsChat); err == nil { - taskMeta["team_workspace"] = teamWsDir - } + teamWsDir := ResolveWorkspace(t.manager.dataDir, + TenantLayer(store.TenantIDFromContext(ctx), store.TenantSlugFromContext(ctx)), + TeamLayer(team.ID), + UserChatLayer(chatID, shared), + ) + taskMeta["team_workspace"] = teamWsDir // Auto-collect media files from current run to team workspace. // When leader received files from user and creates a task, copy those // files to the team workspace so members can access them via read_file. diff --git a/internal/tools/workspace_resolver.go b/internal/tools/workspace_resolver.go new file mode 100644 index 00000000..d61cf61a --- /dev/null +++ b/internal/tools/workspace_resolver.go @@ -0,0 +1,80 @@ +package tools + +import ( + "path/filepath" + "strings" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/config" +) + +// WorkspaceLayer transforms a base path into a scoped path. +// Returns base unchanged if the layer is not applicable (no-op). +type WorkspaceLayer func(base string) string + +// ResolveWorkspace applies layers sequentially to produce the final workspace path. +// Each layer either appends a path segment or returns base unchanged (no-op). +func ResolveWorkspace(base string, layers ...WorkspaceLayer) string { + for _, layer := range layers { + base = layer(base) + } + return base +} + +// TenantLayer scopes to tenant subdirectory. +// Master tenant is a no-op (backward compat — returns base unchanged). +func TenantLayer(tenantID uuid.UUID, slug string) WorkspaceLayer { + return func(base string) string { + return config.TenantWorkspace(base, tenantID, slug) + } +} + +// TeamLayer scopes to team subdirectory: {base}/teams/{teamID}. +// Nil teamID is a no-op. +func TeamLayer(teamID uuid.UUID) WorkspaceLayer { + return func(base string) string { + if teamID == uuid.Nil { + return base + } + return filepath.Join(base, "teams", teamID.String()) + } +} + +// ProjectLayer scopes to project subdirectory: {base}/projects/{projectID}. +// Nil projectID is a no-op. Reserved for future use. +func ProjectLayer(projectID *uuid.UUID) WorkspaceLayer { + return func(base string) string { + if projectID == nil || *projectID == uuid.Nil { + return base + } + return filepath.Join(base, "projects", projectID.String()) + } +} + +// UserChatLayer scopes to per-user or per-chat subdirectory: {base}/{segment}. +// Empty segment or shared=true is a no-op. +// The segment should already be sanitized via SanitizePathSegment if it contains user input. +func UserChatLayer(segment string, shared bool) WorkspaceLayer { + return func(base string) string { + if shared || segment == "" { + return base + } + return filepath.Join(base, segment) + } +} + +// SanitizePathSegment makes a string safe for use as a directory name. +// Replaces colons, spaces, and other unsafe chars with underscores. +// Used to convert userIDs and chatIDs into safe filesystem path segments. +func SanitizePathSegment(s string) string { + var b strings.Builder + for _, r := range s { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' || r == '_' { + b.WriteRune(r) + } else { + b.WriteByte('_') + } + } + return b.String() +} diff --git a/internal/tools/workspace_resolver_test.go b/internal/tools/workspace_resolver_test.go new file mode 100644 index 00000000..134bda67 --- /dev/null +++ b/internal/tools/workspace_resolver_test.go @@ -0,0 +1,200 @@ +package tools + +import ( + "path/filepath" + "testing" + + "github.com/google/uuid" +) + +// masterTenantID mirrors store.MasterTenantID for tests. +var masterTenantID = uuid.MustParse("0193a5b0-7000-7000-8000-000000000001") + +func TestResolveWorkspace_EmptyLayers(t *testing.T) { + got := ResolveWorkspace("/data") + if got != "/data" { + t.Errorf("expected /data, got %s", got) + } +} + +func TestResolveWorkspace_TenantOnly(t *testing.T) { + tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002") + got := ResolveWorkspace("/data", TenantLayer(tid, "acme")) + want := filepath.Join("/data", "tenants", "acme") + if got != want { + t.Errorf("want %s, got %s", want, got) + } +} + +func TestResolveWorkspace_TenantMaster(t *testing.T) { + got := ResolveWorkspace("/data", TenantLayer(masterTenantID, "master")) + if got != "/data" { + t.Errorf("master tenant should be no-op, got %s", got) + } +} + +func TestResolveWorkspace_TenantTeam(t *testing.T) { + tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002") + teamID := uuid.MustParse("0193c000-0000-7000-8000-000000000003") + got := ResolveWorkspace("/data", + TenantLayer(tid, "acme"), + TeamLayer(teamID), + ) + want := filepath.Join("/data", "tenants", "acme", "teams", teamID.String()) + if got != want { + t.Errorf("want %s, got %s", want, got) + } +} + +func TestResolveWorkspace_TenantTeamShared(t *testing.T) { + tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002") + teamID := uuid.MustParse("0193c000-0000-7000-8000-000000000003") + got := ResolveWorkspace("/data", + TenantLayer(tid, "acme"), + TeamLayer(teamID), + UserChatLayer("", false), // shared → empty segment + ) + want := filepath.Join("/data", "tenants", "acme", "teams", teamID.String()) + if got != want { + t.Errorf("shared team should have no chat segment, want %s, got %s", want, got) + } +} + +func TestResolveWorkspace_TenantTeamIsolated(t *testing.T) { + tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002") + teamID := uuid.MustParse("0193c000-0000-7000-8000-000000000003") + chatID := "chat-abc-123" + got := ResolveWorkspace("/data", + TenantLayer(tid, "acme"), + TeamLayer(teamID), + UserChatLayer(chatID, false), + ) + want := filepath.Join("/data", "tenants", "acme", "teams", teamID.String(), chatID) + if got != want { + t.Errorf("want %s, got %s", want, got) + } +} + +func TestResolveWorkspace_TenantTeamProject(t *testing.T) { + tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002") + teamID := uuid.MustParse("0193c000-0000-7000-8000-000000000003") + projectID := uuid.MustParse("0193d000-0000-7000-8000-000000000004") + got := ResolveWorkspace("/data", + TenantLayer(tid, "acme"), + TeamLayer(teamID), + ProjectLayer(&projectID), + ) + want := filepath.Join("/data", "tenants", "acme", "teams", teamID.String(), "projects", projectID.String()) + if got != want { + t.Errorf("want %s, got %s", want, got) + } +} + +func TestResolveWorkspace_FullStack(t *testing.T) { + tid := uuid.MustParse("0193b000-0000-7000-8000-000000000002") + teamID := uuid.MustParse("0193c000-0000-7000-8000-000000000003") + projectID := uuid.MustParse("0193d000-0000-7000-8000-000000000004") + chatID := "chat-xyz" + got := ResolveWorkspace("/data", + TenantLayer(tid, "acme"), + TeamLayer(teamID), + ProjectLayer(&projectID), + UserChatLayer(chatID, false), + ) + want := filepath.Join("/data", "tenants", "acme", "teams", teamID.String(), "projects", projectID.String(), chatID) + if got != want { + t.Errorf("want %s, got %s", want, got) + } +} + +func TestResolveWorkspace_SoloAgent(t *testing.T) { + userID := SanitizePathSegment("user:telegram:12345") + got := ResolveWorkspace("/ws", + UserChatLayer(userID, false), + ) + want := filepath.Join("/ws", "user_telegram_12345") + if got != want { + t.Errorf("want %s, got %s", want, got) + } +} + +func TestResolveWorkspace_SoloAgentShared(t *testing.T) { + got := ResolveWorkspace("/ws", + UserChatLayer("user123", true), + ) + if got != "/ws" { + t.Errorf("shared should be no-op, got %s", got) + } +} + +func TestResolveWorkspace_SoloAgentProject(t *testing.T) { + projectID := uuid.MustParse("0193d000-0000-7000-8000-000000000004") + userID := SanitizePathSegment("user:slack:u1") + got := ResolveWorkspace("/ws", + ProjectLayer(&projectID), + UserChatLayer(userID, false), + ) + want := filepath.Join("/ws", "projects", projectID.String(), "user_slack_u1") + if got != want { + t.Errorf("want %s, got %s", want, got) + } +} + +func TestResolveWorkspace_NilProject(t *testing.T) { + got := ResolveWorkspace("/data", + ProjectLayer(nil), + ) + if got != "/data" { + t.Errorf("nil project should be no-op, got %s", got) + } +} + +func TestResolveWorkspace_NilTeam(t *testing.T) { + got := ResolveWorkspace("/data", + TeamLayer(uuid.Nil), + ) + if got != "/data" { + t.Errorf("nil team should be no-op, got %s", got) + } +} + +func TestResolveWorkspace_ZeroProject(t *testing.T) { + nilID := uuid.Nil + got := ResolveWorkspace("/data", + ProjectLayer(&nilID), + ) + if got != "/data" { + t.Errorf("zero project should be no-op, got %s", got) + } +} + +func TestResolveWorkspace_SharedTrue(t *testing.T) { + got := ResolveWorkspace("/data", + UserChatLayer("chat-123", true), + ) + if got != "/data" { + t.Errorf("shared=true should skip segment, got %s", got) + } +} + +func TestSanitizePathSegment(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"simple", "simple"}, + {"user:telegram:123", "user_telegram_123"}, + {"user@email.com", "user_email_com"}, + {"hello world", "hello_world"}, + {"a-b_c", "a-b_c"}, + {"", ""}, + {"café", "caf_"}, + {"../etc/passwd", "___etc_passwd"}, + } + for _, tt := range tests { + got := SanitizePathSegment(tt.input) + if got != tt.want { + t.Errorf("SanitizePathSegment(%q) = %q, want %q", tt.input, got, tt.want) + } + } +}