From 8d7259f637dd1c83fb563c7c890ef70d8e1e3319 Mon Sep 17 00:00:00 2001 From: viettranx Date: Mon, 16 Mar 2026 20:05:42 +0700 Subject: [PATCH] feat(tools): add team workspace context + WorkspaceInterceptor + file tools access - Add WithToolTeamWorkspace/ToolTeamWorkspaceFromCtx context key for team workspace path (accessible but not necessarily default) - Create WorkspaceInterceptor for team-specific write validation (RBAC, quota, blocked extensions, event broadcasting) - File tools (read_file, write_file, list_files, edit) allow access to team workspace via allowedWithTeamWorkspace() helper - read_file/list_files hint team workspace path when file not found - Registry detects empty tool call args and returns actionable hint (DashScope/Qwen large-output truncation workaround) --- internal/tools/context_keys.go | 20 +++- internal/tools/edit.go | 3 +- internal/tools/filesystem.go | 24 ++++- internal/tools/filesystem_list.go | 9 +- internal/tools/filesystem_write.go | 28 ++++- internal/tools/registry.go | 17 +++ internal/tools/workspace_interceptor.go | 134 ++++++++++++++++++++++++ 7 files changed, 227 insertions(+), 8 deletions(-) create mode 100644 internal/tools/workspace_interceptor.go diff --git a/internal/tools/context_keys.go b/internal/tools/context_keys.go index 077a595c..79bca100 100644 --- a/internal/tools/context_keys.go +++ b/internal/tools/context_keys.go @@ -224,8 +224,8 @@ func MemoryConfigFromCtx(ctx context.Context) *config.MemoryConfig { const ctxTeamID toolContextKey = "tool_team_id" -// WithToolTeamID injects the dispatching team's ID into context so workspace -// tools (workspace_read, workspace_write, team_tasks, team_message) resolve +// WithToolTeamID injects the dispatching team's ID into context so team +// tools (team_tasks, team_message) and the WorkspaceInterceptor resolve // the correct team when the agent belongs to multiple teams. func WithToolTeamID(ctx context.Context, teamID string) context.Context { return context.WithValue(ctx, ctxTeamID, teamID) @@ -237,6 +237,22 @@ func ToolTeamIDFromCtx(ctx context.Context) string { return v } +// --- Team workspace path (accessible but not default) --- + +const ctxTeamWorkspace toolContextKey = "tool_team_workspace" + +// WithToolTeamWorkspace stores the team shared workspace directory path. +// File tools allow access to this path even when restrict_to_workspace is true. +func WithToolTeamWorkspace(ctx context.Context, dir string) context.Context { + return context.WithValue(ctx, ctxTeamWorkspace, dir) +} + +// ToolTeamWorkspaceFromCtx returns the team shared workspace directory path. +func ToolTeamWorkspaceFromCtx(ctx context.Context) string { + v, _ := ctx.Value(ctxTeamWorkspace).(string) + return v +} + // --- Team task ID propagation (delegation origin → workspace tools) --- const ctxTeamTaskID toolContextKey = "tool_team_task_id" diff --git a/internal/tools/edit.go b/internal/tools/edit.go index 4807af19..ddbff5db 100644 --- a/internal/tools/edit.go +++ b/internal/tools/edit.go @@ -160,7 +160,8 @@ func (t *EditTool) Execute(ctx context.Context, args map[string]any) *Result { if workspace == "" { workspace = t.workspace } - resolved, err := resolvePath(path, workspace, effectiveRestrict(ctx, t.restrict)) + allowed := allowedWithTeamWorkspace(ctx, nil) + resolved, err := resolvePathWithAllowed(path, workspace, effectiveRestrict(ctx, t.restrict), allowed) if err != nil { return ErrorResult(err.Error()) } diff --git a/internal/tools/filesystem.go b/internal/tools/filesystem.go index 64f51196..18d8f329 100644 --- a/internal/tools/filesystem.go +++ b/internal/tools/filesystem.go @@ -145,7 +145,8 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *Result if workspace == "" { workspace = t.workspace } - resolved, err := resolvePathWithAllowed(path, workspace, effectiveRestrict(ctx, t.restrict), t.allowedPrefixes) + allowed := allowedWithTeamWorkspace(ctx, t.allowedPrefixes) + resolved, err := resolvePathWithAllowed(path, workspace, effectiveRestrict(ctx, t.restrict), allowed) if err != nil { return ErrorResult(err.Error()) } @@ -155,7 +156,13 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *Result data, err := os.ReadFile(resolved) if err != nil { - return ErrorResult(fmt.Sprintf("failed to read file: %v", err)) + msg := fmt.Sprintf("failed to read file: %v", err) + if os.IsNotExist(err) { + if teamWs := ToolTeamWorkspaceFromCtx(ctx); teamWs != "" && !strings.HasPrefix(resolved, teamWs) { + msg += fmt.Sprintf("\nHint: file may be in the team workspace. Try: read_file(path=\"%s/%s\")", teamWs, path) + } + } + return ErrorResult(msg) } return SilentResult(string(data)) @@ -183,6 +190,19 @@ func (t *ReadFileTool) getFsBridge(ctx context.Context, sandboxKey string) (*san return sandbox.NewFsBridge(sb.ID(), "/workspace"), nil } +// allowedWithTeamWorkspace returns the allowed prefixes with team workspace appended +// if present in context. Thread-safe: creates a new slice per request. +func allowedWithTeamWorkspace(ctx context.Context, base []string) []string { + teamWs := ToolTeamWorkspaceFromCtx(ctx) + if teamWs == "" { + return base + } + out := make([]string, len(base)+1) + copy(out, base) + out[len(base)] = teamWs + return out +} + // resolvePathWithAllowed is like resolvePath but also allows paths under extra prefixes. func resolvePathWithAllowed(path, workspace string, restrict bool, allowedPrefixes []string) (string, error) { resolved, err := resolvePath(path, workspace, restrict) diff --git a/internal/tools/filesystem_list.go b/internal/tools/filesystem_list.go index c847f62d..d96414db 100644 --- a/internal/tools/filesystem_list.go +++ b/internal/tools/filesystem_list.go @@ -88,7 +88,8 @@ func (t *ListFilesTool) Execute(ctx context.Context, args map[string]any) *Resul if workspace == "" { workspace = t.workspace } - resolved, err := resolvePath(path, workspace, effectiveRestrict(ctx, t.restrict)) + allowed := allowedWithTeamWorkspace(ctx, nil) + resolved, err := resolvePathWithAllowed(path, workspace, effectiveRestrict(ctx, t.restrict), allowed) if err != nil { return ErrorResult(err.Error()) } @@ -99,7 +100,11 @@ func (t *ListFilesTool) Execute(ctx context.Context, args map[string]any) *Resul entries, err := os.ReadDir(resolved) if err != nil { if os.IsNotExist(err) { - return SilentResult(fmt.Sprintf("Directory does not exist: %s", path)) + msg := fmt.Sprintf("Directory does not exist: %s", path) + if teamWs := ToolTeamWorkspaceFromCtx(ctx); teamWs != "" && !strings.HasPrefix(resolved, teamWs) { + msg += fmt.Sprintf("\nHint: try the team workspace path: list_files(path=\"%s/%s\")", teamWs, path) + } + return SilentResult(msg) } return ErrorResult(fmt.Sprintf("failed to list directory: %v", err)) } diff --git a/internal/tools/filesystem_write.go b/internal/tools/filesystem_write.go index cfbc7179..973e45f8 100644 --- a/internal/tools/filesystem_write.go +++ b/internal/tools/filesystem_write.go @@ -20,6 +20,7 @@ type WriteFileTool struct { contextFileIntc *ContextFileInterceptor // nil = no virtual FS routing memIntc *MemoryInterceptor // nil = no memory routing groupWriterCache *store.GroupWriterCache // nil = no group write restriction + workspaceIntc *WorkspaceInterceptor // nil = no team workspace validation } // DenyPaths adds path prefixes that write_file must reject. @@ -42,6 +43,11 @@ func (t *WriteFileTool) SetGroupWriterCache(c *store.GroupWriterCache) { t.groupWriterCache = c } +// SetWorkspaceInterceptor enables team workspace validation and event broadcasting. +func (t *WriteFileTool) SetWorkspaceInterceptor(intc *WorkspaceInterceptor) { + t.workspaceIntc = intc +} + func NewWriteFileTool(workspace string, restrict bool) *WriteFileTool { return &WriteFileTool{workspace: workspace, restrict: restrict} } @@ -128,7 +134,8 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *Resul if workspace == "" { workspace = t.workspace } - resolved, err := resolvePath(path, workspace, effectiveRestrict(ctx, t.restrict)) + allowed := allowedWithTeamWorkspace(ctx, nil) + resolved, err := resolvePathWithAllowed(path, workspace, effectiveRestrict(ctx, t.restrict), allowed) if err != nil { return ErrorResult(err.Error()) } @@ -136,6 +143,21 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *Resul return ErrorResult(err.Error()) } + // Team workspace validation + delete-on-empty. + if t.workspaceIntc != nil { + isDelete, intcErr := t.workspaceIntc.HandleWrite(ctx, resolved, content) + if intcErr != nil { + return ErrorResult(intcErr.Error()) + } + if isDelete { + if err := os.Remove(resolved); err != nil && !os.IsNotExist(err) { + return ErrorResult(fmt.Sprintf("failed to delete file: %v", err)) + } + t.workspaceIntc.AfterWrite(ctx, resolved, "delete") + return SilentResult(fmt.Sprintf("File deleted: %s", path)) + } + } + if err := os.MkdirAll(filepath.Dir(resolved), 0755); err != nil { return ErrorResult(fmt.Sprintf("failed to create directory: %v", err)) } @@ -144,6 +166,10 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *Resul return ErrorResult(fmt.Sprintf("failed to write file: %v", err)) } + if t.workspaceIntc != nil { + t.workspaceIntc.AfterWrite(ctx, resolved, "write") + } + result := SilentResult(fmt.Sprintf("File written: %s (%d bytes)", path, len(content))) result.Deliverable = content if deliver { diff --git a/internal/tools/registry.go b/internal/tools/registry.go index 95c789f5..ef383e1d 100644 --- a/internal/tools/registry.go +++ b/internal/tools/registry.go @@ -2,8 +2,10 @@ package tools import ( "context" + "fmt" "log/slog" "maps" + "strings" "sync" "time" @@ -136,6 +138,21 @@ func (r *Registry) ExecuteWithContext(ctx context.Context, name string, args map } } + // Detect empty tool call arguments — typically caused by providers truncating + // or dropping arguments when output is too large (e.g. DashScope with long content). + // Give the model an actionable hint instead of a confusing "X is required" error. + if len(args) == 0 { + if params := tool.Parameters(); params != nil { + if req, ok := params["required"].([]string); ok && len(req) > 0 { + return ErrorResult(fmt.Sprintf( + "Tool call had empty arguments (required: %s). "+ + "This usually means your previous response was too long for the API to include tool parameters. "+ + "Try again with shorter content — split into smaller parts if needed.", + strings.Join(req, ", "))) + } + } + } + start := time.Now() result := tool.Execute(ctx, args) duration := time.Since(start) diff --git a/internal/tools/workspace_interceptor.go b/internal/tools/workspace_interceptor.go new file mode 100644 index 00000000..1034f9dc --- /dev/null +++ b/internal/tools/workspace_interceptor.go @@ -0,0 +1,134 @@ +package tools + +import ( + "context" + "fmt" + "log/slog" + "os" + "path/filepath" + "strings" + + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/pkg/protocol" +) + +// WorkspaceInterceptor validates writes and broadcasts events for team workspace files. +// When no team context is active (ToolTeamIDFromCtx returns ""), all methods are no-ops. +type WorkspaceInterceptor struct { + teamMgr *TeamToolManager +} + +func NewWorkspaceInterceptor(mgr *TeamToolManager) *WorkspaceInterceptor { + return &WorkspaceInterceptor{teamMgr: mgr} +} + +// HandleWrite validates a file write in team workspace context. +// Returns (true, nil) if the write should be treated as a delete (empty content). +// Returns (false, nil) to proceed with normal write. +// Returns (_, error) to block the write. +func (w *WorkspaceInterceptor) HandleWrite(ctx context.Context, path string, content string) (isDelete bool, err error) { + if w == nil { + return false, nil + } + teamIDStr := ToolTeamIDFromCtx(ctx) + if teamIDStr == "" { + return false, nil // Not in team context + } + + // Only apply team validation when path is inside the team workspace. + teamWs := ToolTeamWorkspaceFromCtx(ctx) + if teamWs == "" || !strings.HasPrefix(filepath.Clean(path), filepath.Clean(teamWs)) { + return false, nil // Write is to agent's own workspace, not team workspace + } + + // Resolve team and role for RBAC. Fail-open: if resolution fails (DB issue, + // corrupt cache), allow the write but log a warning for observability. + team, agentID, err := w.teamMgr.resolveTeam(ctx) + if err != nil { + slog.Warn("workspace: team resolution failed, skipping validation", "team", teamIDStr, "error", err) + return false, nil + } + role, err := w.teamMgr.resolveTeamRole(ctx, team, agentID) + if err != nil { + slog.Warn("workspace: role resolution failed, skipping validation", "team", teamIDStr, "error", err) + return false, nil + } + + // Empty content = delete. + if content == "" { + if role == store.TeamRoleReviewer { + return false, fmt.Errorf("reviewers cannot delete workspace files") + } + return true, nil + } + + // RBAC: reviewer cannot write. + if role == store.TeamRoleReviewer { + return false, fmt.Errorf("reviewers cannot write to the workspace") + } + + // Blocked extensions. + ext := strings.ToLower(filepath.Ext(path)) + if blockedExtensions[ext] { + return false, fmt.Errorf("executable file type %q is not allowed", ext) + } + + // File size limit (10MB). + if len(content) > maxFileSizeBytes { + return false, fmt.Errorf("file exceeds max size (10MB)") + } + + // Quota: count files in team workspace scope. + wsDir := teamWs + if wsDir != "" { + entries, err := os.ReadDir(wsDir) + if err != nil { + slog.Warn("workspace: quota check ReadDir failed", "dir", wsDir, "error", err) + } + fileCount := 0 + for _, e := range entries { + if !e.IsDir() { + fileCount++ + } + } + // Only check when creating new file (not updating existing). + if _, statErr := os.Stat(path); os.IsNotExist(statErr) { + if fileCount >= maxFilesPerScope { + return false, fmt.Errorf("workspace file limit reached (%d/%d)", fileCount, maxFilesPerScope) + } + } + } + + return false, nil +} + +// AfterWrite broadcasts a workspace file change event. +func (w *WorkspaceInterceptor) AfterWrite(ctx context.Context, path string, action string) { + if w == nil { + return + } + teamIDStr := ToolTeamIDFromCtx(ctx) + if teamIDStr == "" { + return + } + // Only broadcast for writes inside team workspace. + teamWs := ToolTeamWorkspaceFromCtx(ctx) + if teamWs == "" || !strings.HasPrefix(filepath.Clean(path), filepath.Clean(teamWs)) { + return + } + + fileName := filepath.Base(path) + chatID := ToolChatIDFromCtx(ctx) + if chatID == "" { + chatID = store.UserIDFromContext(ctx) + } + + w.teamMgr.broadcastTeamEvent(protocol.EventWorkspaceFileChanged, map[string]string{ + "team_id": teamIDStr, + "channel": "", + "chat_id": chatID, + "file_name": fileName, + "action": action, + }) + slog.Debug("workspace: file changed", "team", teamIDStr, "file", fileName, "action", action) +}