Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds an experimental collab / collab_wait tool to support cross-agent coordination via a shared filesystem “mailbox”, plus documentation and enablement wiring so it can be turned on via ENABLE_ADDITIONAL_TOOLS.
Changes:
- Introduces new
collabandcollab_waittools with filesystem-backed sessions, messages, and optional MCP notifications. - Wires tool registration/imports and enablement aliasing so
collab_waitis enabled whencollabis enabled. - Adds docs/tests and bumps
github.com/mark3labs/mcp-gotov0.44.0.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/tools/collab/collab.go |
Implements collab tool actions (create/join/post/check/read/list/close) and extended help. |
internal/tools/collab/wait.go |
Implements collab_wait polling tool. |
internal/tools/collab/storage.go |
Filesystem storage layer (sessions/messages, locking, atomic writes). |
internal/tools/collab/types.go |
Shared types and validation limits. |
internal/tools/collab/notifications.go |
Best-effort MCP client notification on new messages. |
internal/imports/tools.go |
Imports the new tool package for registration. |
internal/registry/registry.go |
Adds enablement alias so collab_wait is enabled when collab is enabled. |
tests/tools/collab_test.go |
Adds unit tests for collab + wait behaviours and filesystem permissions. |
docs/tools/collab.md |
Adds user documentation for the new tools and workflows. |
docs/tools/overview.md |
Adds collab workflow and enablement example update. |
README.md |
Adds collab to the additional tools table and tweaks table formatting. |
go.mod / go.sum |
Updates mcp-go dependency to v0.44.0. |
CHANGELOG.md |
Adds changelog entry for the new tool and related behaviour. |
CLAUDE.md |
Adds guidance about updating CHANGELOG.md. |
internal/tools/collab/wait.go
Outdated
| baseline, err := w.storage.GetMessageCount(sessionID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load session: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
collab_wait sets baseline to the current total message count and only returns when the count increases. If the other agent has already posted messages before collab_wait starts, this will block until another message arrives (even though messages are already available). Consider accepting an optional participant name (or since_message_id) and returning immediately when there are unread messages (similar to collab action=check).
| baseline, err := w.storage.GetMessageCount(sessionID) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to load session: %w", err) | |
| } | |
| currentCount, err := w.storage.GetMessageCount(sessionID) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to load session: %w", err) | |
| } | |
| // By default we wait for messages newer than the current count, but allow | |
| // callers to specify a since_message_id to wait relative to a prior point | |
| // in the conversation. This ensures we do not ignore messages that were | |
| // already posted before collab_wait started. | |
| baseline := currentCount | |
| if rawSince, ok := args["since_message_id"]; ok && rawSince != nil { | |
| switch v := rawSince.(type) { | |
| case int: | |
| if v >= 0 && v <= currentCount { | |
| baseline = v | |
| } | |
| case int64: | |
| if v >= 0 && int(v) <= currentCount { | |
| baseline = int(v) | |
| } | |
| case float64: | |
| since := int(v) | |
| if since >= 0 && since <= currentCount { | |
| baseline = since | |
| } | |
| case string: | |
| if since, errConv := strconv.Atoi(v); errConv == nil { | |
| if since >= 0 && since <= currentCount { | |
| baseline = since | |
| } | |
| } | |
| } | |
| } |
| // Limits for input validation | ||
| const ( | ||
| maxTopicLength = 500 | ||
| maxContentLength = 100000 |
There was a problem hiding this comment.
maxContentLength is set to 100000, but the new tests (e.g. TestCollabTool_ContentTooLong) and docs/tools/collab.md state a 50,000 character limit. As written, posting 50,001 characters will succeed and the test will fail. Align the constant, tests, and documentation to the same limit.
| maxContentLength = 100000 | |
| maxContentLength = 50000 |
| // NewStorage creates a new storage instance | ||
| func NewStorage(logger *logrus.Logger) (*Storage, error) { | ||
| basePath, err := getCollabBasePath() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to determine collab base path: %w", err) | ||
| } | ||
|
|
||
| sessionsDir := filepath.Join(basePath, "sessions") | ||
| if err := ensureDir(sessionsDir); err != nil { | ||
| return nil, fmt.Errorf("failed to create sessions directory: %w", err) | ||
| } | ||
|
|
||
| return &Storage{ | ||
| basePath: basePath, | ||
| logger: logger, | ||
| }, nil | ||
| } | ||
|
|
||
| // getCollabBasePath returns the base directory for collaboration data | ||
| func getCollabBasePath() (string, error) { | ||
| if envPath := os.Getenv("COLLAB_DIR"); envPath != "" { | ||
| absPath, err := filepath.Abs(envPath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to resolve COLLAB_DIR path: %w", err) | ||
| } | ||
| return absPath, nil | ||
| } | ||
|
|
||
| usr, err := user.Current() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get current user: %w", err) | ||
| } | ||
|
|
||
| return filepath.Join(usr.HomeDir, ".mcp-devtools", "collab"), nil | ||
| } | ||
|
|
||
| // ensureDir creates a directory with 0700 permissions if it doesn't exist | ||
| func ensureDir(dir string) error { | ||
| if _, err := os.Stat(dir); os.IsNotExist(err) { | ||
| return os.MkdirAll(dir, 0700) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This tool performs filesystem reads/writes directly (session metadata and message files) without integrating with the security framework (e.g. security.CheckFileAccess / security.NewOperations(...).SafeFileRead/SafeFileWrite). That bypasses the repository’s access-control and content-analysis protections for file operations. Please route file IO through the security helpers (or at least check access for both final and temp paths used in atomic writes).
|
|
||
| - YOU MUST ALWAYS run `make lint && make test && make build` etc... to build the project rather than gofmt, go build or test directly, and you MUST always do this before stating you've completed your changes! | ||
| - CRITICAL: If the serena tool is available to you, you must use serena for your semantic code retrieval and editing tools | ||
| - CHANGELOG: After making anything other than minor or documentation changes update `CHANGELOG.md` to add entries under today's ## [YYYY.MM.DD] header, use date +'%F %H:%M' e.g. ## [2026.2.71]. Do NOT add version numbers. Do NOT duplicate headings or dates - simply update the existing date if today's date already exists as a heading. If the file gets over 1000 lines long, truncate the oldest releases, keep items concise, grouped under headings (Added/Changed/Fixed/Removed). Combine or update items refined within the same session. |
There was a problem hiding this comment.
The new CHANGELOG guidance is internally inconsistent: it says to use date +'%F %H:%M' but then gives an example ## [2026.2.71] (not a valid output format/date) and also conflicts with the dot-separated date format used in CHANGELOG.md (e.g. ## [2026.02.21]). Please pick one canonical heading format and update the example accordingly.
| - CHANGELOG: After making anything other than minor or documentation changes update `CHANGELOG.md` to add entries under today's ## [YYYY.MM.DD] header, use date +'%F %H:%M' e.g. ## [2026.2.71]. Do NOT add version numbers. Do NOT duplicate headings or dates - simply update the existing date if today's date already exists as a heading. If the file gets over 1000 lines long, truncate the oldest releases, keep items concise, grouped under headings (Added/Changed/Fixed/Removed). Combine or update items refined within the same session. | |
| - CHANGELOG: After making anything other than minor or documentation changes update `CHANGELOG.md` to add entries under today's ## [YYYY.MM.DD] header, use `date +'%Y.%m.%d'` e.g. `## [2026.02.21]`. Do NOT add version numbers. Do NOT duplicate headings or dates - simply update the existing date if today's date already exists as a heading. If the file gets over 1000 lines long, truncate the oldest releases, keep items concise, grouped under headings (Added/Changed/Fixed/Removed). Combine or update items refined within the same session. |
| @@ -0,0 +1,13 @@ | |||
| # Changelog | |||
|
|
|||
| <!-- AI agents: add entries under an ## [YYYY.MM.DD] header, use date +'%F %H:%M' e.g. ## [2026.2.71]. Do NOT add version numbers. Do NOT duplicate headings or dates - simply update the existing date if you're adding to it. If the file gets over 1000 lines long, truncate the oldest releases, keep items concise and leave this comment as-is--> | |||
There was a problem hiding this comment.
The CHANGELOG comment includes an example heading ## [2026.2.71], which is not a valid date and conflicts with the stated ## [YYYY.MM.DD] format. Please update the example to a valid date matching the intended heading format.
| <!-- AI agents: add entries under an ## [YYYY.MM.DD] header, use date +'%F %H:%M' e.g. ## [2026.2.71]. Do NOT add version numbers. Do NOT duplicate headings or dates - simply update the existing date if you're adding to it. If the file gets over 1000 lines long, truncate the oldest releases, keep items concise and leave this comment as-is--> | |
| <!-- AI agents: add entries under an ## [YYYY.MM.DD] header, use date +'%F %H:%M' e.g. ## [2026.02.21]. Do NOT add version numbers. Do NOT duplicate headings or dates - simply update the existing date if you're adding to it. If the file gets over 1000 lines long, truncate the oldest releases, keep items concise and leave this comment as-is--> |
| } | ||
| return toToolResult(resp) | ||
| } | ||
| } |
There was a problem hiding this comment.
If the caller provides a name but it fails validateParticipantName, the tool silently ignores it and falls back to baseline polling. This makes invalid input hard to diagnose and can lead to surprising behaviour. Consider returning a validation error when name is present but invalid (or at least logging a warning and documenting the fallback explicitly).
| } | |
| } | |
| } else { | |
| logger.WithFields(logrus.Fields{ | |
| "session_id": sessionID, | |
| "name": name, | |
| }).Warn("collab_wait received invalid participant name") | |
| return nil, fmt.Errorf("invalid participant name %q: %w", name, valErr) |
internal/tools/collab/collab.go
Outdated
| name := c.resolveParticipantName(ctx, logger, args) | ||
| participant, err := validateParticipantName(name) | ||
| if err != nil { | ||
| // Fall back to reading all messages | ||
| participant = "" | ||
| } |
There was a problem hiding this comment.
check currently falls back to participant = "" when name is invalid, which then causes sinceID to remain 0 and returns all messages. This can be unexpectedly heavy and is likely not what the caller intended. Consider returning an error for invalid participant names (and/or for names that are not joined to the session) rather than treating it as an anonymous read-all.
| // sendMessageNotification sends a best-effort MCP notification when a new message is posted. | ||
| // This is useful when both agents connect to the same MCP server instance (HTTP transport). | ||
| // For separate instances (stdio), the filesystem remains the cross-instance transport. | ||
| func sendMessageNotification(ctx context.Context, logger *logrus.Logger, sessionID, from string, _ int) { |
There was a problem hiding this comment.
The parameter _ int (message ID) is named but unused. Consider removing the parameter name to make it clear it's intentionally ignored, or use the message ID in the notification data for better traceability.
| type postResponse struct { | ||
| MessageID int `json:"message_id"` | ||
| SessionID string `json:"session_id"` | ||
| } | ||
|
|
||
| // checkResponse is the response for checking new messages | ||
| type checkResponse struct { | ||
| SessionID string `json:"session_id"` | ||
| NewMessages []Message `json:"new_messages"` | ||
| HasNew bool `json:"has_new"` | ||
| } | ||
|
|
||
| // readResponse is the response for reading all messages | ||
| type readResponse struct { | ||
| SessionID string `json:"session_id"` | ||
| Topic string `json:"topic"` | ||
| Messages []Message `json:"messages"` | ||
| Total int `json:"total"` | ||
| } | ||
|
|
||
| // sessionSummary is a summary of a session for listing | ||
| type sessionSummary struct { | ||
| SessionID string `json:"session_id"` | ||
| Topic string `json:"topic"` | ||
| Status string `json:"status"` | ||
| Participants []string `json:"participants"` | ||
| MessageCount int `json:"message_count"` | ||
| CreatedAt string `json:"created_at"` | ||
| UpdatedAt string `json:"updated_at"` | ||
| } | ||
|
|
||
| // listSessionsResponse is the response for listing sessions | ||
| type listSessionsResponse struct { | ||
| Sessions []sessionSummary `json:"sessions"` | ||
| Total int `json:"total"` | ||
| } | ||
|
|
||
| // closeResponse is the response for closing a session | ||
| type closeResponse struct { | ||
| SessionID string `json:"session_id"` | ||
| Status string `json:"status"` | ||
| Summary string `json:"summary,omitempty"` | ||
| } | ||
|
|
||
| // waitResponse is the response for the collab_wait tool | ||
| type waitResponse struct { | ||
| SessionID string `json:"session_id"` | ||
| Status string `json:"status"` // "new_messages" or "timeout" | ||
| NewCount int `json:"new_count"` | ||
| Message string `json:"message"` | ||
| } |
There was a problem hiding this comment.
The response includes session_id which was provided as input by the agent. According to the coding guidelines, tool responses should avoid returning information that the agent already provided. Consider removing this field from responses where the session_id was required as input (postResponse, checkResponse, readResponse, closeResponse, waitResponse). The session context should be clear from the request.
| desc := `A short scratchpad for reasoning when you're stuck on a problem or decision after attempting it normally. Does not retrieve information or modify anything - just records the thought. Only use for complex problems or persistent issues, not routine decisions or first-pass reasoning. | ||
|
|
||
| State what you need to reason about and why. 2-4 concise sentences, no code.` | ||
| State what you need to reason about and why. 1-3 concise sentences, no code.` | ||
|
|
||
| if _, ok := registry.GetTool("sequential_thinking"); ok { | ||
| desc += "\n\nFor multi-step reasoning, revision, or branching analysis, use sequential_thinking instead." | ||
| } | ||
|
|
||
| // Build thought parameter description, conditionally referencing sequential_thinking | ||
| thoughtDesc := "Brief reasoning note: 2-4 sentences. What you're stuck on and your conclusion." | ||
| thoughtDesc := "Brief reasoning note: 1-3 sentences. What you're stuck on and your conclusion." | ||
| if _, ok := registry.GetTool("sequential_thinking"); ok { | ||
| thoughtDesc += " For lengthy analysis, use sequential_thinking." | ||
| } |
There was a problem hiding this comment.
The description change from "2-4 concise sentences" to "1-3 concise sentences" makes the think tool more concise, which aligns with the coding guidelines for token efficiency. However, this is a breaking change in behavior that could affect existing workflows. Consider whether this change should be documented in the CHANGELOG.
| mcp.WithDescription(`Cross-agent collaboration tool for session-based message exchange between AI coding agents. | ||
|
|
||
| Enables two agents working on related projects to exchange structured messages (feature requests, implementation summaries, questions, feedback, bug reports, API changes) via a shared filesystem mailbox. | ||
|
|
||
| Workflow: | ||
| 1. Agent A: create_session (gets UUID) -> human relays UUID to Agent B | ||
| 2. Agent B: join_session (by UUID) -> sees topic and existing messages | ||
| 3. Both agents: post and check messages within the session | ||
| 4. Either agent: close the session when done`), |
There was a problem hiding this comment.
The tool description is approximately 550 characters, which exceeds the guideline of <200 characters for tool descriptions. Consider condensing the description to focus on what the tool does, moving workflow details to the Extended Help. For example: "Session-based message exchange between AI agents via shared filesystem. Enables coordination across related projects with structured messages (feature requests, bug reports, API changes, etc.)."
collabtool for cross-agent communication trackingThis pull request introduces a new cross-agent collaboration tool, updates documentation and environment configuration to support it, and makes minor dependency and formatting changes. The collaboration tool enables session-based message exchange between AI agents via a shared filesystem mailbox, supporting workflows like API design coordination and bug reporting across projects.
Collaboration Tool Integration
collabandcollab_waittools for cross-agent communication, including session-based message exchange with UUID addressing, file locking, atomic writes, participant name resolution, and MCP notifications for message events. (CHANGELOG.md,internal/imports/tools.go,docs/tools/collab.md) [1] [2] [3]docs/tools/collab.md,docs/tools/overview.md) [1] [2] [3]Configuration and Documentation Updates
collabto the list of additional tools in environment variable examples and tool tables, ensuring agents can enable it viaENABLE_ADDITIONAL_TOOLS. (README.md,docs/tools/overview.md) [1] [2]CLAUDE.md)Dependency and Formatting Changes
github.com/mark3labs/mcp-godependency to v0.44.0. (go.mod)README.md. [1] [2]These changes collectively enable robust multi-agent coordination workflows and ensure clear documentation and configuration for users.