Merged
Conversation
Code Review - Group Chat Support (PR #39)Overall AssessmentThis is an excellent, well-architected PR that implements a sophisticated multi-agent collaboration feature. The code quality is very high with comprehensive test coverage (147+ unit tests + 6 integration tests), clear separation of concerns, and thoughtful design patterns. ✅ StrengthsArchitecture & Design
Code Quality
🔍 Security Concerns1. Command Injection Risk (High Priority) The moderator spawning uses chat.moderatorAgentId directly without validation. Recommendation: Add validation to ensure moderatorAgentId matches known agent IDs from agent-detector.ts 2. File Path Traversal Risk (Medium Priority) Path construction should be validated to prevent traversal attacks. Recommendation: Add path validation to ensure resolved paths stay within expected directories. 3. Image Data Validation (Medium Priority) Images are loaded without size/format validation. Recommendation: Add file size limits (max 10MB) and validate file types. 🐛 Potential Bugs4. Race Condition in Message Queueing (Medium Priority) Recommendation: Use useReducer for complex state transitions. 5. Memory Leak in Session Tracking (Low Priority) activeModeratorSessions map is never cleaned up if processes crash. Recommendation: Add periodic cleanup and process exit handlers. 6. Uncaught Promise Rejections (Medium Priority) Recommendation: Wrap critical operations in try-catch blocks. 🎯 Code Quality7. Magic Strings (Low Priority) 8. Input Sanitization (Medium Priority) 📦 RecommendationsHigh Priority (Before Merge)
Medium Priority (Follow-up PR) SummaryThis PR represents outstanding work with thoughtful architecture and comprehensive testing. The security concerns are typical for a feature of this complexity and should be straightforward to address. Recommendation: Approve with minor revisions. Address high-priority security items before merge. Great job! 🎉 Files reviewed: 59 files (+7680 lines, -460 lines) |
Add pipe-delimited log format for Group Chat feature: - escapeContent: escapes newlines and pipes for safe storage - unescapeContent: reverses escaping to restore original content - appendToLog: appends timestamped messages to log file - readLog: parses log file into structured messages All 24 unit tests pass covering: - Content escaping/unescaping for newlines and pipes - Log append with ISO 8601 timestamps - Log parsing with edge cases (empty files, image refs, whitespace) - Round-trip verification for data integrity
Add group-chat-storage.ts with CRUD operations for group chats:
- createGroupChat: Creates directory structure with metadata, log, images dir
- loadGroupChat: Loads group chat by ID, returns null if not found
- listGroupChats: Lists all group chats from config directory
- deleteGroupChat: Removes group chat and all associated data
- updateGroupChat: Updates metadata with automatic updatedAt timestamp
- addParticipantToChat/removeParticipantFromChat: Participant management
- getParticipant: Lookup participant by name
Storage structure:
~/Library/Application Support/Maestro/group-chats/{id}/
- metadata.json: GroupChat metadata
- chat.log: Pipe-delimited message log
- images/: Directory for image attachments
Test coverage: 20 tests covering all storage operations
- Add group-chat-moderator.ts with spawnModerator, sendToModerator, killModerator, and getModeratorSessionId functions - Implement IProcessManager interface for dependency injection - Add MODERATOR_SYSTEM_PROMPT for moderator initialization - Create comprehensive test suite with 19 tests covering: - Spawning in read-only mode (3.1) - System prompt introduction (3.2) - Message routing and logging (3.3) - Session termination (3.4) - Session ID retrieval (3.5) - Include edge case tests for error handling and multi-chat support
…4.5) Add group-chat-agent.ts with functions for managing participants: - addParticipant: Creates session and adds to chat - sendToParticipant: Routes messages to specific agents - removeParticipant: Kills session and removes from chat - Helper functions for session tracking 31 comprehensive tests covering all participant scenarios.
Add group-chat-router.ts with: - extractMentions: Finds @mentions in text matching known participants - routeUserMessage: Routes user messages to moderator and logs - routeModeratorResponse: Forwards to mentioned agents via @mentions - routeAgentResponse: Logs agent responses and notifies moderator Comprehensive test suite with 35 tests covering: - @mention extraction with ordering, duplicates, unknown mentions - Case sensitivity and partial name matching - Message routing between user, moderator, and participants - Log persistence and session management - Error handling for non-existent chats/participants
- Add integration test infrastructure in group-chat-test-utils.ts - Helper functions for agent selection, response waiting, cleanup - Support for SKIP_INTEGRATION_TESTS environment variable - Add 6 integration tests in group-chat.integration.test.ts - 6.1: Basic moderator response - 6.2: Two agents collaborate on addition task - 6.3: Agents reference chat log for context - 6.4: Moderator handles non-existent participant - 6.5: Chat log persists across restart - 6.6: Mixed agent types work together - Add vitest.integration.config.ts for integration tests - 3 minute timeout per test - Sequential execution to avoid agent conflicts - Add npm scripts: test:integration, test:integration:watch
- Fix escapeContent to escape backslashes first (per spec order) - Fix unescapeContent to use single-pass regex for correct handling of escaped backslash followed by 'n' vs escaped newline - Add saveImage function for storing images with uuid-based filenames - Add comprehensive tests for backslash handling and image saving
…r Prompt) Phase 2 (Storage) was already implemented in a previous commit. This commit: - Adds the moderator system prompt (group-chat-moderator.md) - Exports the prompt from src/prompts/index.ts - Fixes unused imports in group-chat-agent.ts and group-chat-router.ts to resolve TypeScript build errors
- Fix loadGroupChat to handle empty/invalid JSON files gracefully by returning null instead of throwing SyntaxError - Fix test isolation in listGroupChats test by using relative comparison instead of expecting absolute empty state - All 147 group-chat tests now pass Phase 4-6 implementations verified complete: - Moderator management (spawn, send, kill, getSessionId) - Agent management (add, send, remove, getSessionId) - Message routing (extractMentions, routeUser, routeModerator, routeAgent)
- Created src/main/ipc/handlers/groupChat.ts with full IPC handler implementation: - Storage handlers: create, list, load, delete, rename - Chat log handlers: appendMessage, getMessages, saveImage - Moderator handlers: startModerator, sendToModerator, stopModerator, getModeratorSessionId - Participant handlers: addParticipant, sendToParticipant, removeParticipant - Event emission helpers for message and state change notifications - Uses withIpcErrorLogging wrapper for consistent error handling - Updated src/main/ipc/handlers/index.ts: - Added registerGroupChatHandlers import and export - Added GroupChatHandlerDependencies type export - Registered handlers in registerAllHandlers() with ProcessManager type casting - Added window.maestro.groupChat API to preload.ts: - Full API surface with 14 methods matching IPC handlers - Event subscriptions: onMessage, onStateChange - Complete TypeScript type definitions in MaestroAPI interface All 147 group chat tests continue to pass.
- Create shared/group-chat-types.ts with GroupChat, GroupChatParticipant, GroupChatMessage, and GroupChatState type definitions - Re-export group chat types from renderer/types/index.ts - Add group chat state variables to App.tsx: - groupChats / setGroupChats - activeGroupChatId / setActiveGroupChatId - groupChatMessages / setGroupChatMessages - groupChatState / setGroupChatState
Created GroupChatList component with: - Collapsible section header with count badge - "New" button to create group chats - List of group chats with click-to-open - Context menu for Rename/Delete actions - Integration with SessionList.tsx below Ungrouped section All 11,134 tests pass.
- Create NewGroupChatModal.tsx using Modal UI component pattern - Agent detection and grid selection with AGENT_TILES - Auto-select first available supported agent - FormInput for chat name with validation - Create DeleteGroupChatModal.tsx for deletion confirmation - Destructive styling on Delete button - Warning about permanent deletion - Create RenameGroupChatModal.tsx for renaming group chats - Pre-fills current name, validates changes - FormInput with Enter key submission - Add modal priorities in modalPriorities.ts: DELETE_GROUP_CHAT: 660, NEW_GROUP_CHAT: 650, RENAME_GROUP_CHAT: 640, GROUP_CHAT_INFO: 630 - Export AGENT_TILES, AgentLogo, AgentTile from AgentSelectionScreen for reuse in NewGroupChatModal
Created four new components for the Group Chat main view: - GroupChatPanel.tsx: Container composing header, messages, and input - GroupChatHeader.tsx: Header with name, participant count, info/close buttons - GroupChatMessages.tsx: Message display with color-coded participants, auto-scroll - GroupChatInput.tsx: Text input with @mention autocomplete, Enter-to-send
…-6.2) Add the participants panel components for the right sidebar: - GroupChatParticipants.tsx: Container component for displaying all participants - ParticipantCard.tsx: Expandable card showing participant status, agent type, context usage bar, session ID (copy-able), and last activity info
Create GroupChatInfoOverlay.tsx component for displaying group chat metadata. The component: - Uses Modal UI component for consistent layer stack integration - Displays chat ID, creation time, log path, images directory - Shows moderator agent and session information - Lists all participant sessions with copy-to-clipboard buttons - Includes "Open in Finder" button using shell.openExternal - Supports backdrop click to close
- Add modal state for group chat modals (new, delete, rename, info overlay) - Load group chats on mount alongside sessions and groups - Add event listeners for real-time message and state updates - Implement handlers for open, close, create, delete, rename, send - Add conditional rendering for GroupChatPanel and GroupChatParticipants - Add all Group Chat modal components - Pass group chat props to SessionList for left panel integration - Fix unused variable warnings in groupChat IPC handler
…Palette (9.1-9.2) - Add newGroupChat shortcut (Cmd+Shift+G) to DEFAULT_SHORTCUTS - Add keyboard handler in useMainKeyboardHandler.ts to trigger NewGroupChatModal - Add setShowNewGroupChatModal to keyboard handler context in App.tsx - Add Group Chat commands to QuickActionsModal (New Group Chat, Close Group Chat) - Add props for group chat handlers (onNewGroupChat, onCloseGroupChat, activeGroupChatId)
Found expired OAuth token, attempting refresh... Token refresh successful ## CHANGES - Updated context window documentation for GPT-5.2 model (200K tokens) 🚀 - Fixed callback declaration order to prevent state reference issues 🔧 - Replaced async useEffect with synchronous state broadcasting for mobile 📱 - Added sessionId to Auto Run synopsis for better history tracking 📊 - Introduced updateBatchStateAndBroadcast wrapper for immediate updates ⚡ - Enhanced mobile client state synchronization without render delays 🏃 - Fixed React Strict Mode compatibility for state updates 🛡️ - Improved real-time batch processing state propagation 🔄 - Removed dependency on React's render cycle for broadcasts 🎯 - Ensured immediate state updates reach web interface clients 🌐
- Added read-only mode support for group chat messages 📖 - Implemented message queueing when moderator is busy 📬 - Added drag-and-drop reordering for queued messages 🔄 - Enabled @mention autocomplete for all Maestro agents 🤖 - Added keyboard navigation for mention suggestions ⌨️ - Synchronized staged images between group chat and composer 🖼️ - Added group chat statistics to info overlay 📊 - Fixed CSS conflicts between prose containers 🎨 - Added auto-expand when new group chat is created ✨ - Improved group chat UI with better visual indicators 💫
…b project! However, I don't see any input provided after "INPUT:" in your message. Could you please share the changelog, commit history, or release notes that you'd like me to summarize? This could include: - Git commit messages - Pull request descriptions - A changelog file - Release notes - Or any other documentation about what changed since the last release Once you provide that information, I'll create an exciting CHANGES section with 10-word bullets and relevant emojis as requested!
Add environment variable that allows agent hooks to differentiate between new sessions and resumed sessions. Since Maestro spawns a new process per message (batch mode), session start hooks fire on every turn. This env var lets users conditionally skip hooks. - Set MAESTRO_SESSION_RESUMED=1 when --resume or --session flags present - Document in README with example hook guard - Add note in Agent Provider UI linking to documentation Fixes #42 Co-authored-by: TQ White II (@tqwhite)
Add the ability to configure custom environment variables per-agent in the "Create New Agent" and "Edit Agent" modals. Environment variables are stored alongside custom CLI arguments and are passed to the agent process when spawned. Changes: - Add IPC handlers for set/get/getAll custom environment variables - Expose env var APIs through preload.ts - Add UI section in NewInstanceModal with key-value pair management - Update process-manager to apply custom env vars when spawning agents Closes #41 Hat tip to @tqwhite for the feature request!
Found expired OAuth token, attempting refresh... Token refresh successful I'd be happy to help you create a clean update summary for your Github project! However, I don't see any input provided after "INPUT:" in your message. Could you please share the changelog, commit history, or release notes that you'd like me to summarize? This could include: - Git commit messages - Pull request descriptions - A changelog file - Release notes - Or any other documentation about what changed since the last release Once you provide that information, I'll create an exciting CHANGES section with clean 10-word bullets and relevant emojis as requested!
- Create shared AgentConfigPanel component for reuse between NewInstanceModal and wizard - Add "Customize" button to each supported agent tile in wizard - Implement expand/collapse view transition with fade animation - Allow users to configure custom path, arguments, environment variables, and agent-specific settings directly in wizard - Refactor NewInstanceModal to use shared AgentConfigPanel component - Update test mocks for new lucide-react icons (Settings, ArrowLeft, Plus, Trash2) - Add missing IPC mock methods (getCustomPath, getCustomEnvVars, setCustomEnvVars) Closes #37
pedramamini
force-pushed
the
group-chat
branch
from
598ee54 to
d9ebe1c
Compare
Pull Request Review: Group Chat SupportThis is a substantial feature addition implementing multi-agent collaboration. Overall well-structured with comprehensive testing. Critical Issues1. Path Traversal Vulnerability (src/main/group-chat/group-chat-storage.ts:84-107)
2. Stale Session Cleanup Race (src/main/group-chat/group-chat-moderator.ts:65-88)
3. Unbounded Memory Growth (src/main/group-chat/group-chat-router.ts:116-119)
Security ConcernsUnsafe File Deletion (src/main/group-chat/group-chat-storage.ts:239-242)
Bugs1. Race Condition in Participant Response (src/main/group-chat/group-chat-router.ts:220-236)
2. Silent Failure in Mention Forwarding (src/main/group-chat/group-chat-router.ts:174-188)
3. Case-Sensitive Participant Check (src/main/group-chat/group-chat-storage.ts:290)
4. Missing Error Handler (src/renderer/components/GroupChatInput.tsx:256-277)
Performance
Strengths✅ Excellent architecture with clean separation RecommendationsBefore Merge (Critical):
High Priority:
Rating: 7.5/10
Version bump 0.9.1 → 0.10.0 is appropriate. Great feature overall - address security issues before merge! |
- Updated app icons with fresh new design aesthetic! 🎨 - Added markdown rendering support for group chat messages 📝 - Implemented toggle between formatted and plain text views 👁️ - Added copy-to-clipboard functionality for chat messages 📋 - Improved phase generation logic for better document detection 🔍 - Enhanced disk document validation with task counting checks ✅ - Fixed Claude Code file output handling in wizard mode 🛠️ - Added keyboard shortcut (⌘E) for markdown view toggle ⌨️ - Improved UI with hover effects on message action buttons ✨ - Better handling of documents written directly to disk 💾
…"type":"error","error":{"type":"rate_limit_error","message":"This request would exceed your account's rate limit. Please try again later."},"request_id":"req_011CWHZ4TBc4wsjeoh5KdWfA"}
…packager Add comprehensive debug package system for generating sanitized support bundles: Phase 1: Backend Data Collectors (src/main/debug-package/) - index.ts: Main orchestrator with generateDebugPackage() function - collectors/system.ts: OS, hardware, and Electron app version info - collectors/settings.ts: Sanitized settings (API keys redacted, paths replaced with ~) - collectors/agents.ts: Agent configurations and capabilities - collectors/external-tools.ts: Shells, git, gh CLI, cloudflared status - collectors/sessions.ts: Session metadata (no conversation content) - collectors/processes.ts: Active process info from ProcessManager - collectors/logs.ts: Recent system logs with level counts - collectors/errors.ts: Session error states and error log counts - collectors/web-server.ts: Server status, tunnel state (no URLs/tokens) - collectors/storage.ts: Storage paths (sanitized) and sizes - collectors/group-chats.ts: Group chat metadata (no messages) - collectors/batch-state.ts: Auto Run state from sessions Phase 2: Zip Packager - packager.ts: Creates timestamped zip using archiver with README.md Privacy guarantees: - NO API keys, tokens, or passwords - NO conversation content - NO personal file contents - All paths sanitized (usernames replaced with ~)
…load API - Created src/main/ipc/handlers/debug.ts with createPackage and previewPackage handlers - Integrated debug handlers into registerAllHandlers with full dependency injection - Added debug API to preload.ts with TypeScript types for renderer access - Handlers use save dialog for user-selected output location
Add "Create Debug Package" action to the QuickActions modal (Command K menu) with keyboard shortcut Alt+Meta+D. The action shows toast notifications for progress, success, and error states. Changes: - Add createDebugPackage shortcut to DEFAULT_SHORTCUTS (Alt+Meta+D) - Add action to QuickActionsModal with toast notifications - Add keyboard handler in useMainKeyboardHandler.ts - Add handleCreateDebugPackage function in App.tsx - Update test mocks for useToast and window.maestro.debug
Add DebugPackageModal component for enhanced debug package generation UX: - Category selection with checkboxes to include/exclude data types - Size estimates for each category - Privacy notice explaining what is not included - Generation progress indicator with states: idle/generating/success/error - Success view with path and "Show in Finder" button - Error view with error message display - Uses Modal and ModalFooter UI components - Added MODAL_PRIORITIES.DEBUG_PACKAGE (605) Updated Command K integration: - Opens modal instead of calling API directly - Keyboard shortcut (Alt+Meta+D) now opens modal - Added setDebugPackageModalOpen prop to QuickActionsModal
Create comprehensive test suite with 31 tests covering all 11 collectors: - System info collection (OS, hardware, app versions) - Settings sanitization (API keys, tokens, passwords redacted) - Session metadata extraction (no conversation content) - Agent configuration collection (paths sanitized) - Process info collection - Logs and errors collection - Batch/Auto Run state extraction - External tools availability - Web server state - Storage paths and sizes - Group chat metadata (no messages) Tests verify proper sanitization of sensitive data and that no private information leaks through the debug package collectors.
Add comprehensive unit tests for the debug package packager: - 14 tests covering zip file creation and content validation - Verifies correct timestamp-based filename format - Tests file inclusion (README.md and all JSON files) - Validates JSON content parsing and formatting - Tests handling of undefined values, empty contents - Verifies maximum compression is applied - Tests special characters and complex nested structures - Uses execFileSync with unzip CLI for content extraction (bypasses jsdom environment issues with AdmZip)
Add comprehensive test suite for debug package sanitization utilities: - Path sanitization: home directory replacement, Windows/Unix handling, edge cases (null, undefined, numbers), special characters, spaces - API key redaction: 14+ sensitive key patterns (apiKey, authToken, password, secret, credential, accessToken, refreshToken, privateKey) with case-insensitive and naming convention support - Environment variable filtering: custom env vars/args masking, path-based env variables sanitization - Nested object and array handling for deep structures - Comprehensive integration tests ensuring no sensitive data leaks 62 tests covering all sanitization functionality.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements Group Chat, a feature that enables orchestrated multi-agent collaboration through a moderator-driven architecture. Users can create group chats that spawn a hidden "moderator" AI session running in read-only mode, which coordinates multiple participant AI agents (each with their own dedicated session) to work on tasks collaboratively. The backend (src/main/group-chat-*.ts) provides storage for chat metadata, a pipe-delimited chat log format, and image attachments, along with IPC handlers for message routing between the user, moderator, and participant agents. The frontend adds a Group Chats section to the left sidebar with create/rename/delete functionality, a full-width chat panel with color-coded messages by sender, @mention autocomplete for participants, a typing/working indicator based on state (idle/moderator-thinking/agent-working), and a right panel showing participant status cards with context usage, session IDs, and last activity. The feature includes keyboard shortcuts (Cmd+Shift+G for new group chat), Command Palette integration, and an info overlay displaying all session IDs and file paths. Real-time updates are handled via event listeners for message and state change events from the main process.