fix: group chat process cleanup, Codex cost tracking, moderator @mention enforcement#437
Open
openasocket wants to merge 36 commits intoRunMaestro:mainfrom
Open
fix: group chat process cleanup, Codex cost tracking, moderator @mention enforcement#437openasocket wants to merge 36 commits intoRunMaestro:mainfrom
openasocket wants to merge 36 commits intoRunMaestro:mainfrom
Conversation
Enable group chat to spawn new agent instances by type when no matching session exists in the sidebar. When a user or moderator @mentions an agent name that doesn't match an existing session, the system now falls back to matching against AGENT_DEFINITIONS and spawns a fresh instance with default configuration. - Add addFreshParticipant() to group-chat-agent.ts (clean spawn, no session overrides) - Add agent type fallback in routeUserMessage() and routeModeratorResponse() in group-chat-router.ts - Add groupChat:addFreshParticipant IPC handler and preload exposure - Update test to include new IPC handler channel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add "+" button in participants panel header and AddParticipantModal with two modes: "Use existing session" (inherits config) and "Create fresh agent" (clean defaults). Wires modal through modalStore, App.tsx, AppModals, and useGroupChatHandlers. Adds missing addFreshParticipant type to global.d.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…arsing Add skipBatchForReadOnly guard in buildAgentArgs to prevent --dangerously-bypass-approvals-and-sandbox from conflicting with --sandbox read-only when Codex runs as a read-only moderator. Handle item.completed type:"error" events in Codex output parser (e.g., account warnings, sandbox violations) instead of silently swallowing them as system events. Replace OpenCode-specific error patterns in Codex with proper OpenAI patterns: model not found, model access denied, stream disconnection, and thread not found. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rticipant categories Add agents:getAvailable IPC endpoint that returns installed, non-hidden agent types. Enhance GroupChatInput mention dropdown to show three categories: existing participants (disabled), available sessions, and agent types that can be spawned fresh (with "New" badge). Section headers appear when multiple categories are present. Keyboard navigation skips disabled participant items. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Added 6 test cases in a new describe('addFreshParticipant') block:
- spawns fresh agent with default config (no session overrides)
- generates unique session ID with group-chat- prefix
- rejects duplicate participant names
- requires moderator to be active
- throws if agent is not available
- uses os.homedir() as default cwd
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…outer
Add 6 tests in describe('agent type fallback') covering: session-first
resolution (backward compat), agent type fallback when no session exists,
fresh participant default cwd (os.homedir()), ignoring unavailable agents,
both ID and name mention matching via mentionMatches(), and moderator
response fallback. Mocked getVisibleAgentDefinitions via vi.mock with
importActual passthrough.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 13 tests covering rendering, session filtering (terminal exclusion, already-added exclusion), fresh agent selection, existing session selection, error handling, and modal close. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Input Add 5 tests covering agent type items with "New" badge, participant disabled state, session/agent-type deduplication, and dropdown filtering for the enhanced @mention autocomplete feature. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Updated 7 generic error messages across group-chat-agent.ts and group-chat-router.ts to provide actionable guidance: agent unavailable now suggests installing or using a different agent, spawn failures suggest checking configuration, and moderator-not-active suggests restarting the moderator. Updated corresponding test assertions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…oup chat Add validation module (validation.ts) with UUID, participant name, message content, base64 image, custom args, and env vars validators. Apply validation at the top of every group chat IPC handler. Add defense-in-depth path containment check to getGroupChatDir(). 45 new unit tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prevent state corruption from concurrent operations: - Chat-level locks prevent delete/update during message processing - Synthesis guards prevent double-trigger from duplicate exit handlers - Pending participants merge instead of overwrite across rounds - Spawn failure resets participant UI state to idle Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…roup chat - Add explicit null guards for processManager/agentDetector in groupChat IPC handlers (sendToModerator, addParticipant, sendToParticipant, addFreshParticipant) replacing silent ?? undefined coalescing with thrown errors - Add killAllModerators() to group-chat-moderator.ts that kills all active moderator processes and clears session tracking maps - Wire group chat cleanup (clearAllParticipantSessionsGlobal + killAllModerators) into the quit handler's performCleanup() to prevent zombie processes on app exit - Add SSH wrapping to spawnModeratorSynthesis() matching the pattern used in routeUserMessage() so synthesis runs remotely on SSH-configured moderators - Add tests for killAllModerators and quit handler group chat cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…g in group chat Wrap untrusted content (chat history, user messages, agent responses) in XML-style boundary tags to prevent prompt injection attacks in moderator and participant prompts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… remove debug logging
- Wrap deleteGroupChatHistoryEntry() and clearGroupChatHistory() in
enqueueWrite() to prevent read-modify-write race conditions on
concurrent history operations
- Add missing fields to resetGroupChatState() (moderatorUsage,
groupChatExecutionQueue, groupChatStagedImages, groupChatReadOnlyMode)
to prevent stale state when switching chats
- Remove ~100 console.log('[GroupChat:Debug]') statements from
group-chat-router.ts, replacing key state transitions with
logger.debug() and converting console.error/warn to logger equivalents
- Remove 11 debug console.log/warn statements from groupChat.ts IPC
handler, replace emitParticipantState warning with logger.warn()
- Add TODO comment for unimplemented image embedding in sendToModerator
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nt buffer loss When Codex uses tool calls during a moderator turn, the first agent_message sets resultEmitted=true after flushing. If a second agent_message arrives (the final one with @mentions), the subsequent usage event skips the flush because resultEmitted is already true, leaving stale reasoning text in the buffer. Reset the flag on each new agent_message so the latest text is always flushed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ew agent_message Covers the scenario where Codex tool calls cause an interim buffer flush, then a second agent_message with @mentions arrives. Verifies that resultEmitted is reset so the subsequent usage event flushes the updated text (preventing stale reasoning from being routed instead of the real response). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ngs) Moderator and participant processes now resume prior agent sessions instead of rebuilding full prompts each turn. On resume, only the new message/delegation is sent (~200-400 tokens vs ~2,200 tokens full prompt). Falls back to full prompt when no stored session ID or agent lacks resume support. Exit listener detects session_not_found errors and clears stored session ID for automatic fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…y sandbox bypass, improve moderator @mention rules - Add new wrapped format support for Codex CLI v0.103.0+ (task_started, agent_reasoning, agent_message, token_count, tool_call, tool_result events) - Fix agent-args readOnlyMode to only strip sandbox-bypass flags, preserving --skip-git - Enhance moderator system prompt with explicit @mention routing rules and agent addition flow - Add conductorProfile callback for group chat conductor customization - Add groupChatLock mock to exit-listener tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-json output Adds 8 new tests to StdoutHandler.test.ts verifying the outputParser path for session-id emission, which was previously untested (all mocks returned null). Includes 3 integration tests using real ClaudeOutputParser. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…resume Codex wrapped format (v0.103.0+) `task_started` events were generating synthetic `codex-0-<timestamp>` session IDs. These IDs can't be used with `codex exec resume` — instead of erroring, Codex silently starts a new session with no prior context (verified manually on v0.103.0). Real session IDs come from `thread.started` events (legacy format), which Codex v0.103.0 uses by default. Removing synthetic ID generation ensures: - Legacy format: real thread_id UUIDs are captured and resume works - Wrapped format: no session ID stored, safe fallback to full prompt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… participant) Adds 4 tests verifying that Claude Code and Codex agents resume independently in group chat with their distinct resume arg formats: - Claude: --resume <session_id> - Codex: resume <session_id> (no -- prefix) Tests cover: independent resume args, batch mode prefixes, synthesis round isolation, and full-prompt-to-resume lifecycle transitions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…on resume Automates the previously manual "app restart" test checklist item with 4 new tests verifying session IDs survive disk persistence and resume works after simulated restart: single-agent moderator+participant resume, synthesis resume, listGroupChats enumeration, and mixed Claude+Codex restart flow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4 new tests verify resume args (`--resume <sessionId>`) pass through the SSH wrapper correctly for moderator routing, participant delegation, synthesis spawning, and the negative case (first turn without resume). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…isplay, moderator @mention enforcement Three fixes for group chat reliability: 1. Process cleanup on delete: killModerator() and clearAllParticipantSessions() used exact session ID matching but batch mode spawns processes with timestamp suffixes. Added killByPrefix() to ProcessManager so delete kills all matching processes instead of silently failing (root cause of zombie processes and y-flood). 2. Codex session ID and cost: Restored display-only synthetic session IDs (codex-0-*) so UI shows an ID instead of "pending". Resume logic skips these prefixed IDs. Added OpenAI model pricing table to calculate cost from token counts since Codex CLI doesn't report cost directly. 3. Moderator prompt: Added mandatory @mention enforcement section and delegation strategy to prevent moderator from discussing agent tasks without actually routing messages. Updated synthesis prompt for critical analysis and fusion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds group-chat concurrency guards, input validation, fresh-participant spawn API, batch moderator termination, router/quit wiring for group-chat cleanup, preload/IPCs, UI modal and autocomplete enhancements, Codex parser/stdout updates, and extensive tests across those areas. Changes
Sequence Diagram(s)sequenceDiagram
participant QuitHandler
participant GroupChatAgent
participant GroupChatModerator
participant ProcessManager
participant Logger
QuitHandler->>Logger: log("running group-chat cleanup")
QuitHandler->>GroupChatAgent: clearAllParticipantSessionsGlobal?()
alt clearAllParticipantSessionsGlobal provided
GroupChatAgent-->>QuitHandler: cleared sessions
else
QuitHandler-->>QuitHandler: skip
end
QuitHandler->>GroupChatModerator: killAllModerators?(processManager)
alt killAllModerators provided
GroupChatModerator->>ProcessManager: killByPrefix(prefix)
ProcessManager-->>GroupChatModerator: killedCount
GroupChatModerator-->>QuitHandler: done
else
GroupChatModerator-->>QuitHandler: cleared session map only
end
QuitHandler->>ProcessManager: killAll()
ProcessManager-->>QuitHandler: all process kill results
QuitHandler->>Logger: log("quit cleanup complete")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes three critical group chat issues: Process cleanup on delete
Codex session ID and cost tracking
Moderator @mention enforcement
Additional improvements
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User sends message to group chat] --> B{Acquire chat lock}
B -->|Lock failed| C[Error: Chat busy]
B -->|Lock acquired| D[Validate input]
D --> E{Auto-add @mentioned agents?}
E -->|Session exists| F[Add from existing session]
E -->|Agent type only| G[Add fresh agent instance]
E -->|No match| H[Continue]
F --> H
G --> H
H --> I[Spawn moderator batch process]
I --> J[Moderator processes message]
J --> K{Contains @mentions?}
K -->|Yes| L[Route to participants]
K -->|No| M[Direct response to user]
L --> N[Spawn participant batch processes]
N --> O{All responses received?}
O -->|No| P[Wait for responses]
O -->|Yes| Q{Synthesis guard check}
Q -->|Already running| R[Skip duplicate synthesis]
Q -->|Not running| S[Mark synthesis started]
S --> T[Spawn synthesis process]
T --> U[Moderator fuses responses]
U --> V[Clear synthesis guard]
V --> M
M --> W[Release chat lock]
X[Delete group chat] --> Y{Check lock}
Y -->|Locked| Z[Error: Chat busy]
Y -->|Unlocked| AA[Kill moderator by prefix]
AA --> AB[Kill all participants by prefix]
AB --> AC[Delete chat data]
style B fill:#fff3cd
style Q fill:#fff3cd
style AA fill:#d1ecf1
style AB fill:#d1ecf1
Last reviewed commit: f7b1265 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/main/ipc/handlers/groupChat.test.ts (1)
100-132:⚠️ Potential issue | 🟠 MajorType the mock to include
killByPrefix.The object literal includes
killByPrefix, but the declared type formockProcessManagerdoesn’t, which will fail TypeScript’s excess property checks.✅ Proposed fix
let mockProcessManager: { spawn: ReturnType<typeof vi.fn>; write: ReturnType<typeof vi.fn>; kill: ReturnType<typeof vi.fn>; + killByPrefix: ReturnType<typeof vi.fn>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/groupChat.test.ts` around lines 100 - 132, The mockProcessManager declared type is missing the killByPrefix property while the object literal sets it; update the mockProcessManager type to include killByPrefix (e.g., add killByPrefix: ReturnType<typeof vi.fn>) so the declaration matches the object created in beforeEach and TypeScript no longer errors on excess/missing properties for mockProcessManager.
🧹 Nitpick comments (13)
src/renderer/global.d.ts (1)
1776-1786: Parameter order differs fromaddParticipant.
addParticipantuses(id, name, agentId, cwd)(lines 1765-1770) whileaddFreshParticipantuses(id, agentId, name, cwd). This inconsistency could cause confusion for callers. If intentional (perhaps to emphasize that agentId is more central for fresh spawns), consider documenting this difference. Otherwise, aligning the parameter order would improve API consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/global.d.ts` around lines 1776 - 1786, The two APIs have inconsistent parameter order: addParticipant(id, name, agentId, cwd) vs addFreshParticipant(id, agentId, name, cwd); change addFreshParticipant’s declaration and any implementations/callsites to use the same order as addParticipant (id, name, agentId, cwd) to keep the API consistent, and update any documentation/tests that rely on the old order; if the original order was intentional, instead add a comment/docstring to addFreshParticipant clarifying the different ordering.src/main/preload/groupChat.ts (1)
115-116: Consider consistency: addFreshParticipant argument order differs from addParticipant.The preload method
addParticipantuses(id, name, agentId, cwd)whileaddFreshParticipantuses(id, agentId, name, cwd). While the handler correctly receives and reorders these arguments before invoking the actual function, the inconsistent parameter ordering in the preload API is confusing for developers. Consider aligning both methods to use the same argument order for better API consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/preload/groupChat.ts` around lines 115 - 116, The preload API is inconsistent: addParticipant(id, name, agentId, cwd) vs addFreshParticipant(id, agentId, name, cwd); change addFreshParticipant to the same parameter order as addParticipant (id, name, agentId, cwd), update its ipcRenderer.invoke call to pass arguments in that order, and update the corresponding IPC handler for 'groupChat:addFreshParticipant' (or the code that reorders its args before calling the underlying function) so it expects and forwards (id, name, agentId, cwd) consistently with addParticipant.src/main/ipc/handlers/groupChat.ts (2)
770-771: Incorrect validator used forentryId.
validateGroupChatId(entryId)validates thatentryIdis a UUID, which is correct if entry IDs are UUIDs. However, the function name is misleading since it's being used for a history entry ID, not a group chat ID. Consider creating a separatevalidateEntryIdfunction or a more genericvalidateUUIDfunction for clarity.Proposed refactor to use a generic UUID validator
In
src/main/group-chat/validation.ts, add:+/** + * Validate that a value is a valid UUID v4. + */ +export function validateUUID(value: unknown, fieldName = 'value'): string { + if (typeof value !== 'string' || !UUID_V4_REGEX.test(value)) { + throw new Error(`Invalid ${fieldName}: must be a valid UUID`); + } + return value; +}Then in the handler:
- validateGroupChatId(entryId); + validateUUID(entryId, 'entry ID');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/groupChat.ts` around lines 770 - 771, The code incorrectly reuses validateGroupChatId for the history entry ID; add a clear UUID validator (e.g., validateEntryId or validateUUID) in the group-chat validation module and replace the validateGroupChatId(entryId) call with the new validator; update any imports/usages in the handler (groupChatId = validateGroupChatId(groupChatId); validateEntryId(entryId) or validateUUID(entryId)) so intent is clear and validators are not misleading.
440-441: Acknowledged TODO for image embedding.The TODO comment about image embedding in moderator prompts is clear. Consider tracking this in an issue if not already done.
Would you like me to help create an issue to track the image embedding feature for moderator prompts?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/groupChat.ts` around lines 440 - 441, Create a tracked issue for implementing image embedding in moderator prompts: describe the current state (images saved separately via the groupChat:saveImage handler and a TODO in the async moderator prompt handler with signature async (id: string, message: string, images?: string[], readOnly?: boolean)), list acceptance criteria (embed images inline in moderator prompt payloads, preserve image ordering and alt text, fallback to separate storage when embedding fails), include required API changes (modify the moderator prompt handler to accept/serialize embedded image data or URLs), add tests and UI/UX notes, and tag with a feature/enhancement label and assign or link to the relevant maintainer for prioritization.src/main/group-chat/group-chat-lock.ts (1)
75-82: Consider consolidatingforceReleaseChatLockwithreleaseChatLock.Both functions have identical implementations (calling
chatLocks.delete(chatId)). The semantic distinction ("unconditional" vs regular release) doesn't manifest in the code. Consider whether one function with clear documentation would suffice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/group-chat-lock.ts` around lines 75 - 82, Both forceReleaseChatLock and releaseChatLock simply call chatLocks.delete(chatId), so consolidate by removing one of them (preferably keep releaseChatLock for the simpler name) and update all references to the removed symbol to call the retained function; adjust JSDoc on the retained function to document unconditional behavior if needed and run/update any callers/tests that referenced forceReleaseChatLock to use releaseChatLock instead.src/main/group-chat/validation.ts (2)
95-109: Consider validating base64 format, not just size.The function validates type and size but doesn't verify the string is valid base64. Invalid base64 will fail later during decode. A regex check could catch malformed input earlier.
🛡️ Add base64 format validation
+const BASE64_REGEX = /^[A-Za-z0-9+/]*={0,2}$/; + export function validateBase64Image(data: unknown, maxSizeBytes = 20 * 1024 * 1024): string { if (typeof data !== 'string') { throw new Error('Invalid image data: must be a string'); } + + // Strip data URL prefix if present (e.g., "data:image/png;base64,") + const base64Data = data.includes(',') ? data.split(',')[1] : data; + + if (!BASE64_REGEX.test(base64Data)) { + throw new Error('Invalid image data: not valid base64'); + } // Approximate decoded size: base64 uses ~4/3 ratio - const estimatedSize = data.length * 0.75; + const estimatedSize = base64Data.length * 0.75;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/validation.ts` around lines 95 - 109, The validateBase64Image function currently only checks type and estimated size but doesn't confirm the string is valid base64; add a format validation step in validateBase64Image that verifies the input matches a base64 pattern (optionally allowing data URI prefix removal) and rejects malformed strings before size checking, or attempt a safe decode (e.g., try Buffer.from(..., 'base64') and ensure re-encoding matches) to catch invalid base64 and throw a clear error message when validation fails.
10-10: UUID validation regex allows UUID v1-5, not just v4.The
UUID_V4_REGEXpattern matches any valid UUID format (v1-v5) since it doesn't validate the version nibble. While this works, the name is misleading.🧹 Either rename to UUID_REGEX or add version validation
-const UUID_V4_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; +// Matches standard UUID format (any version) +const UUID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;Or for strict v4 validation:
-const UUID_V4_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; +// Strict UUID v4: version nibble = 4, variant nibble = 8/9/a/b +const UUID_V4_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/validation.ts` at line 10, The constant UUID_V4_REGEX currently matches any UUID version because it doesn't validate the version nibble; update the code so the name and behavior align: either rename UUID_V4_REGEX to UUID_REGEX (and update any references) if you intend to accept all UUID versions, or change the regex to enforce v4 specifically by requiring the version character to be "4" (e.g., the 13th hex digit) and adjust the constant name to remain UUID_V4_REGEX; locate and edit the UUID_V4_REGEX declaration in validation.ts and update usages accordingly.src/renderer/components/AddParticipantModal.tsx (2)
108-129: Minor: Early returns don't resetisSubmittingstate.The early returns on lines 114, 116, and 119 don't reset
isSubmittingtofalse. WhilecanSubmitshould prevent reaching these paths, if logic changes, the button could remain permanently disabled.🧹 Add isSubmitting reset to early returns
try { if (mode === 'existing') { - if (!selectedSessionId) return; + if (!selectedSessionId) { + setIsSubmitting(false); + return; + } const session = sessions.find((s) => s.id === selectedSessionId); - if (!session) return; + if (!session) { + setIsSubmitting(false); + return; + } onAddExisting(session.id, session.name, session.toolType, session.cwd); } else { - if (!selectedAgentId) return; + if (!selectedAgentId) { + setIsSubmitting(false); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AddParticipantModal.tsx` around lines 108 - 129, In handleSubmit ensure isSubmitting is reset on the early-return paths: before each early return in the 'existing' branch when selectedSessionId is falsy or session not found, and before the early return in the 'fresh' branch when selectedAgentId is falsy, call setIsSubmitting(false) (and optionally setError with a helpful message) so the component doesn't remain stuck in submitting state; update the closure that defines handleSubmit to call setIsSubmitting(false) right before those return points (symbols: handleSubmit, setIsSubmitting, selectedSessionId, sessions, selectedAgentId, AGENT_TILES, onAddExisting, onAddFresh).
22-22: Unused prop:groupChatIdis declared but never referenced.The
groupChatIdprop is defined inAddParticipantModalPropsand destructured in the component parameters but is never used in the component body.🧹 Remove unused prop
interface AddParticipantModalProps { theme: Theme; isOpen: boolean; - groupChatId: string; sessions: Session[]; participants: GroupChatParticipant[]; onClose: () => void; onAddExisting: (sessionId: string, name: string, agentId: string, cwd: string) => void; onAddFresh: (agentId: string, name: string) => void; } export function AddParticipantModal({ theme, isOpen, - groupChatId, sessions, participants,Also applies to: 30-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AddParticipantModal.tsx` at line 22, Remove the unused prop "groupChatId": delete it from the AddParticipantModalProps interface and from the AddParticipantModal component's parameter destructuring, and remove any other references or propTypes/defaults named groupChatId in that component. Ensure the component compiles after removing the identifier and update any callers only if they were passing groupChatId (remove the argument) to avoid unused prop warnings.src/main/parsers/codex-output-parser.ts (2)
45-71: Pricing data is hardcoded and will become stale.Model pricing changes frequently. Consider externalizing this to a config file or adding a comment with the last-updated date and a reminder to verify periodically.
The comment on line 37-38 mentions "updated 2026-02-21" which is helpful. Consider also adding a TODO or linking to the source URL for easier verification:
/** * OpenAI model pricing per million tokens (USD) - * Source: https://openai.com/api/pricing/ (updated 2026-02-21) + * Source: https://openai.com/api/pricing/ + * Last verified: 2026-02-21 + * TODO: Consider moving to external config or fetching dynamically */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/parsers/codex-output-parser.ts` around lines 45 - 71, The MODEL_PRICING constant contains hardcoded rates that will go stale; either move this object into a configuration source (e.g., JSON/YAML loaded at runtime) and update code that references MODEL_PRICING to read from that config, or at minimum add a clear metadata comment above MODEL_PRICING with the last-updated date, a TODO to verify pricing regularly, and a link to the authoritative pricing URL so maintainers can refresh values; refer to the MODEL_PRICING symbol in src/main/parsers/codex-output-parser.ts when making changes.
76-86: Prefix matching order may cause incorrect pricing for new model variants.The prefix matching iterates in insertion order, so
gpt-5will match beforegpt-5.1for any model starting withgpt-5that isn't an exact match. For example, a hypotheticalgpt-5.1-codex-turbowould matchgpt-5pricing instead ofgpt-5.1-codex.🔧 Sort prefixes by length (longest first) for correct matching
function getModelPricing(model: string): ModelPricing { if (MODEL_PRICING[model]) { return MODEL_PRICING[model]; } - for (const [prefix, pricing] of Object.entries(MODEL_PRICING)) { + // Sort by prefix length descending to match most specific first + const sortedEntries = Object.entries(MODEL_PRICING).sort( + ([a], [b]) => b.length - a.length + ); + for (const [prefix, pricing] of sortedEntries) { if (model.startsWith(prefix)) { return pricing; } } return MODEL_PRICING['default']; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/parsers/codex-output-parser.ts` around lines 76 - 86, getModelPricing may return the wrong tier because prefix iteration uses insertion order; change the fallback loop that iterates Object.entries(MODEL_PRICING) to first collect the prefixes (excluding exact matches), sort them by descending length, then iterate that sorted list and use model.startsWith(prefix) to find the most specific match; keep the existing exact lookup MODEL_PRICING[model] and final default return the same.src/renderer/components/AppModals.tsx (1)
1541-1557: Consider extracting the IIFE into a local variable for clarity.The immediately-invoked function expression (IIFE) pattern inside JSX works but is less readable than computing the value before the return. This is a minor style suggestion.
♻️ Optional refactor
+ const addParticipantGroupChat = showAddParticipantModal + ? groupChats.find((c) => c.id === showAddParticipantModal) + : null; + return ( <> {/* ... other modals ... */} {/* --- ADD PARTICIPANT MODAL --- */} - {showAddParticipantModal && (() => { - const addParticipantGroupChat = groupChats.find( - (c) => c.id === showAddParticipantModal - ); - return ( - <AddParticipantModal - theme={theme} - isOpen={!!showAddParticipantModal} - groupChatId={showAddParticipantModal} - sessions={sessions} - participants={addParticipantGroupChat?.participants || []} - onClose={onCloseAddParticipantModal} - onAddExisting={onAddExistingParticipant} - onAddFresh={onAddFreshParticipant} - /> - ); - })()} + {showAddParticipantModal && ( + <AddParticipantModal + theme={theme} + isOpen={!!showAddParticipantModal} + groupChatId={showAddParticipantModal} + sessions={sessions} + participants={addParticipantGroupChat?.participants || []} + onClose={onCloseAddParticipantModal} + onAddExisting={onAddExistingParticipant} + onAddFresh={onAddFreshParticipant} + /> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AppModals.tsx` around lines 1541 - 1557, Compute the JSX for the add-participant modal before the component return instead of using the IIFE: read showAddParticipantModal and find the matching group chat with groupChats.find(...) (store into a local addParticipantGroupChat variable), build the <AddParticipantModal ... /> element (setting isOpen based on !!showAddParticipantModal and passing groupChatId, participants from addParticipantGroupChat?.participants || [], sessions, onCloseAddParticipantModal, onAddExistingParticipant, onAddFreshParticipant), store that element in a local variable (e.g., addParticipantModalElement) and then render it in JSX with {showAddParticipantModal && addParticipantModalElement}; this removes the inline IIFE while preserving all props and behavior.src/main/group-chat/group-chat-router.ts (1)
501-503: Empty catch blocks silently swallow all errors, not just "unavailable" agents.The empty
catch { }blocks will suppress all exceptions including unexpected ones (e.g., network errors, parsing failures). Consider logging at debug level for observability.🔧 Proposed fix
} catch { - // Skip unavailable agents + // Skip unavailable agents - expected when agent is not installed + logger.debug(`Agent ${def.id} not available, skipping`, LOG_CONTEXT); }Also applies to: 1341-1344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/group-chat-router.ts` around lines 501 - 503, Replace the empty catch that contains the comment "// Skip unavailable agents" with a catch that captures the error (e.g., catch (err)) and logs it at debug level instead of swallowing it, then continue skipping the agent; e.g., call an existing debug logger (logger.debug or this.logger.debug) with a concise message and the error/context (agent id or index) so unexpected failures are observable. Apply the same change to the other empty catch at the second occurrence (the block around lines 1341-1344).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/group-chat/group-chat-moderator.ts`:
- Around line 280-296: The killAllModerators function currently calls
processManager.kill(sessionId) but activeModeratorSessions holds prefix IDs so
timestamp-suffixed processes won’t be terminated; change the call to
processManager.killByPrefix(sessionId) when processManager is provided, keep the
powerManager.removeBlockReason(`groupchat:${groupChatId}`) and the
session/sessionActivity clears, and update the killAllModerators unit tests to
expect killByPrefix to be invoked for each sessionId prefix instead of kill.
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 1108-1109: The synthesis error paths call clearSynthesisInProgress
but never release the chat lock, leaving the conversation locked; update each
synthesis failure branch (the code paths that currently call
clearSynthesisInProgress at the synthesis spawn/error locations tied to
routeModeratorResponse and the synthesis flow) to also call
releaseChatLock(groupChatId) before returning/throwing so the chat lock is
always released; specifically add releaseChatLock(groupChatId) alongside
clearSynthesisInProgress in the error handlers found around the synthesis
spawn/error sites (the branches referenced in your review at the lines that call
clearSynthesisInProgress) to guarantee locks are freed on all failure paths.
In `@src/main/process-manager/ProcessManager.ts`:
- Around line 194-208: killByPrefix currently increments killed for every
sessionId that startsWith(prefix) even though this.kill(sessionId) can return
false; change the logic to call this.kill(sessionId) and only increment killed
when that call returns a truthy/success value. Update the loop in killByPrefix
(referencing killByPrefix, this.kill, this.processes, and sessionId) so it
checks the boolean result of this.kill(sessionId) and increments killed only on
success, then return the accurate killed count.
In `@src/renderer/components/AddParticipantModal.tsx`:
- Around line 76-80: The catch block in AddParticipantModal currently only logs
failures to console (inside the try/catch around agent detection) so users still
see "No agents available"; update the catch to set a user-facing error state
(e.g., setAgentDetectionError or setDetectionError) and leave
setIsDetecting(false) in finally, and also forward the exception to your error
monitoring (e.g., captureException or Sentry.captureException) so it’s recorded;
then update the component render logic that shows "No agents available" to
prefer displaying the new error state message when present.
In `@src/renderer/components/GroupChatRightPanel.tsx`:
- Line 10: The "Add Participant" button in the GroupChatRightPanel component
lacks keyboard focus handling; update the element that renders the Plus
icon/button inside GroupChatRightPanel to include tabIndex={0} (or tabIndex={-1}
if it should be programmatically focusable only), add the "outline-none" class
to match renderer focus rules, and add an accessible label via aria-label (e.g.,
aria-label="Add participant"); apply the same pattern (tabIndex + outline-none
and optional aria-label) to the other interactive elements referenced in this
file (the ones rendering PanelRightClose and other similar buttons).
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 658-677: The parameter sessionId in handleAddExistingParticipant
is unused; either remove it from the function signature and any callers (keeping
the function to derive id from useGroupChatStore.getState().activeGroupChatId),
or if the caller should supply the session id, replace the local id assignment
with const id = sessionId and ensure callers pass the sessionId; update
references to useGroupChatStore, window.maestro.groupChat.addParticipant, and
useModalStore.getState().closeModal('addGroupChatParticipant') accordingly so
the function signature and implementation remain consistent.
---
Outside diff comments:
In `@src/__tests__/main/ipc/handlers/groupChat.test.ts`:
- Around line 100-132: The mockProcessManager declared type is missing the
killByPrefix property while the object literal sets it; update the
mockProcessManager type to include killByPrefix (e.g., add killByPrefix:
ReturnType<typeof vi.fn>) so the declaration matches the object created in
beforeEach and TypeScript no longer errors on excess/missing properties for
mockProcessManager.
---
Nitpick comments:
In `@src/main/group-chat/group-chat-lock.ts`:
- Around line 75-82: Both forceReleaseChatLock and releaseChatLock simply call
chatLocks.delete(chatId), so consolidate by removing one of them (preferably
keep releaseChatLock for the simpler name) and update all references to the
removed symbol to call the retained function; adjust JSDoc on the retained
function to document unconditional behavior if needed and run/update any
callers/tests that referenced forceReleaseChatLock to use releaseChatLock
instead.
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 501-503: Replace the empty catch that contains the comment "//
Skip unavailable agents" with a catch that captures the error (e.g., catch
(err)) and logs it at debug level instead of swallowing it, then continue
skipping the agent; e.g., call an existing debug logger (logger.debug or
this.logger.debug) with a concise message and the error/context (agent id or
index) so unexpected failures are observable. Apply the same change to the other
empty catch at the second occurrence (the block around lines 1341-1344).
In `@src/main/group-chat/validation.ts`:
- Around line 95-109: The validateBase64Image function currently only checks
type and estimated size but doesn't confirm the string is valid base64; add a
format validation step in validateBase64Image that verifies the input matches a
base64 pattern (optionally allowing data URI prefix removal) and rejects
malformed strings before size checking, or attempt a safe decode (e.g., try
Buffer.from(..., 'base64') and ensure re-encoding matches) to catch invalid
base64 and throw a clear error message when validation fails.
- Line 10: The constant UUID_V4_REGEX currently matches any UUID version because
it doesn't validate the version nibble; update the code so the name and behavior
align: either rename UUID_V4_REGEX to UUID_REGEX (and update any references) if
you intend to accept all UUID versions, or change the regex to enforce v4
specifically by requiring the version character to be "4" (e.g., the 13th hex
digit) and adjust the constant name to remain UUID_V4_REGEX; locate and edit the
UUID_V4_REGEX declaration in validation.ts and update usages accordingly.
In `@src/main/ipc/handlers/groupChat.ts`:
- Around line 770-771: The code incorrectly reuses validateGroupChatId for the
history entry ID; add a clear UUID validator (e.g., validateEntryId or
validateUUID) in the group-chat validation module and replace the
validateGroupChatId(entryId) call with the new validator; update any
imports/usages in the handler (groupChatId = validateGroupChatId(groupChatId);
validateEntryId(entryId) or validateUUID(entryId)) so intent is clear and
validators are not misleading.
- Around line 440-441: Create a tracked issue for implementing image embedding
in moderator prompts: describe the current state (images saved separately via
the groupChat:saveImage handler and a TODO in the async moderator prompt handler
with signature async (id: string, message: string, images?: string[], readOnly?:
boolean)), list acceptance criteria (embed images inline in moderator prompt
payloads, preserve image ordering and alt text, fallback to separate storage
when embedding fails), include required API changes (modify the moderator prompt
handler to accept/serialize embedded image data or URLs), add tests and UI/UX
notes, and tag with a feature/enhancement label and assign or link to the
relevant maintainer for prioritization.
In `@src/main/parsers/codex-output-parser.ts`:
- Around line 45-71: The MODEL_PRICING constant contains hardcoded rates that
will go stale; either move this object into a configuration source (e.g.,
JSON/YAML loaded at runtime) and update code that references MODEL_PRICING to
read from that config, or at minimum add a clear metadata comment above
MODEL_PRICING with the last-updated date, a TODO to verify pricing regularly,
and a link to the authoritative pricing URL so maintainers can refresh values;
refer to the MODEL_PRICING symbol in src/main/parsers/codex-output-parser.ts
when making changes.
- Around line 76-86: getModelPricing may return the wrong tier because prefix
iteration uses insertion order; change the fallback loop that iterates
Object.entries(MODEL_PRICING) to first collect the prefixes (excluding exact
matches), sort them by descending length, then iterate that sorted list and use
model.startsWith(prefix) to find the most specific match; keep the existing
exact lookup MODEL_PRICING[model] and final default return the same.
In `@src/main/preload/groupChat.ts`:
- Around line 115-116: The preload API is inconsistent: addParticipant(id, name,
agentId, cwd) vs addFreshParticipant(id, agentId, name, cwd); change
addFreshParticipant to the same parameter order as addParticipant (id, name,
agentId, cwd), update its ipcRenderer.invoke call to pass arguments in that
order, and update the corresponding IPC handler for
'groupChat:addFreshParticipant' (or the code that reorders its args before
calling the underlying function) so it expects and forwards (id, name, agentId,
cwd) consistently with addParticipant.
In `@src/renderer/components/AddParticipantModal.tsx`:
- Around line 108-129: In handleSubmit ensure isSubmitting is reset on the
early-return paths: before each early return in the 'existing' branch when
selectedSessionId is falsy or session not found, and before the early return in
the 'fresh' branch when selectedAgentId is falsy, call setIsSubmitting(false)
(and optionally setError with a helpful message) so the component doesn't remain
stuck in submitting state; update the closure that defines handleSubmit to call
setIsSubmitting(false) right before those return points (symbols: handleSubmit,
setIsSubmitting, selectedSessionId, sessions, selectedAgentId, AGENT_TILES,
onAddExisting, onAddFresh).
- Line 22: Remove the unused prop "groupChatId": delete it from the
AddParticipantModalProps interface and from the AddParticipantModal component's
parameter destructuring, and remove any other references or propTypes/defaults
named groupChatId in that component. Ensure the component compiles after
removing the identifier and update any callers only if they were passing
groupChatId (remove the argument) to avoid unused prop warnings.
In `@src/renderer/components/AppModals.tsx`:
- Around line 1541-1557: Compute the JSX for the add-participant modal before
the component return instead of using the IIFE: read showAddParticipantModal and
find the matching group chat with groupChats.find(...) (store into a local
addParticipantGroupChat variable), build the <AddParticipantModal ... /> element
(setting isOpen based on !!showAddParticipantModal and passing groupChatId,
participants from addParticipantGroupChat?.participants || [], sessions,
onCloseAddParticipantModal, onAddExistingParticipant, onAddFreshParticipant),
store that element in a local variable (e.g., addParticipantModalElement) and
then render it in JSX with {showAddParticipantModal &&
addParticipantModalElement}; this removes the inline IIFE while preserving all
props and behavior.
In `@src/renderer/global.d.ts`:
- Around line 1776-1786: The two APIs have inconsistent parameter order:
addParticipant(id, name, agentId, cwd) vs addFreshParticipant(id, agentId, name,
cwd); change addFreshParticipant’s declaration and any implementations/callsites
to use the same order as addParticipant (id, name, agentId, cwd) to keep the API
consistent, and update any documentation/tests that rely on the old order; if
the original order was intentional, instead add a comment/docstring to
addFreshParticipant clarifying the different ordering.
Comment on lines
76
to
80
| } catch (err) { | ||
| console.error('Failed to detect agents:', err); | ||
| } finally { | ||
| setIsDetecting(false); | ||
| } |
There was a problem hiding this comment.
Error is logged but user receives no feedback.
When agent detection fails, the error is logged to console but the user sees "No agents available" which is misleading. Per coding guidelines, exceptions should either bubble up to Sentry or be handled with user-facing feedback.
🛠️ Suggested fix: Set error state on detection failure
} catch (err) {
- console.error('Failed to detect agents:', err);
+ setError('Failed to detect available agents. Please try again.');
} finally {
setIsDetecting(false);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (err) { | |
| console.error('Failed to detect agents:', err); | |
| } finally { | |
| setIsDetecting(false); | |
| } | |
| } catch (err) { | |
| setError('Failed to detect available agents. Please try again.'); | |
| } finally { | |
| setIsDetecting(false); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/AddParticipantModal.tsx` around lines 76 - 80, The
catch block in AddParticipantModal currently only logs failures to console
(inside the try/catch around agent detection) so users still see "No agents
available"; update the catch to set a user-facing error state (e.g.,
setAgentDetectionError or setDetectionError) and leave setIsDetecting(false) in
finally, and also forward the exception to your error monitoring (e.g.,
captureException or Sentry.captureException) so it’s recorded; then update the
component render logic that shows "No agents available" to prefer displaying the
new error state message when present.
…-chat-router spawnModeratorSynthesis() had 5 error paths that called clearSynthesisInProgress() but did NOT call releaseChatLock(), causing the chat lock to be held forever if synthesis failed (deadlocking conversation until the 5-min stale timeout). Added releaseChatLock(groupChatId) to every synthesis error path: - Chat not found - Moderator not active - No moderator session ID - Agent not available - Spawn failure (catch block) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rators activeModeratorSessions stores session ID prefixes, not exact IDs. Batch-mode moderators have timestamp-suffixed IDs, so kill() by prefix was needed (matching the existing killModerator pattern). Updated tests accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t test The mockProcessManager object already had killByPrefix in its value but the TypeScript type declaration was missing it, causing a type mismatch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n error, a11y, unused param - Fix killByPrefix to only count successful kills (ProcessManager.ts) - Show user-facing error on agent detection failure (AddParticipantModal.tsx) - Add tabIndex, outline-none, aria-label to Add Participant button (GroupChatRightPanel.tsx) - Remove unused sessionId parameter from handleAddExistingParticipant - Expand GenericProcessManager interface with missing spawn options and killByPrefix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Both functions now consistently use (id, name, agentId, cwd) parameter order across preload, IPC handler, type declarations, and renderer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…easeChatLock) forceReleaseChatLock was functionally identical to releaseChatLock (both unconditionally call chatLocks.delete) and had zero production callers. Removed the dead function and its unit tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dal handleSubmit Early returns in handleSubmit (no selectedSessionId, session not found, no selectedAgentId) left isSubmitting=true, permanently disabling the Add button until the modal was closed and reopened. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/ipc/handlers/groupChat.ts (1)
774-780:⚠️ Potential issue | 🟡 MinorAssign the return value from validation of
entryId.Line 776 calls
validateGroupChatId(entryId)but discards the return value, whereas line 775 correctly assigns the validatedgroupChatId. Entry IDs are generated as UUID v4 strings (viauuidv4()inaddGroupChatHistoryEntry), so the validation is technically appropriate, but the return value should be assigned toentryIdfor consistency:entryId = validateGroupChatId(entryId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/groupChat.ts` around lines 774 - 780, The handler's validation call discards the validated value for entryId; update the async handler that takes (groupChatId, entryId) so that you assign the result of validateGroupChatId back to entryId (just like groupChatId = validateGroupChatId(groupChatId)), ensuring you call validateGroupChatId for entryId and reassign it before calling logger.debug and deleteGroupChatHistoryEntry(groupChatId, entryId); keep the same function names (validateGroupChatId, logger.debug, deleteGroupChatHistoryEntry) and behavior.
🧹 Nitpick comments (1)
src/renderer/hooks/groupChat/useGroupChatHandlers.ts (1)
653-690: Parameter order inconsistency between add participant handlers.
handleAddExistingParticipanttakes(name, agentId, cwd)whilehandleAddFreshParticipanttakes(agentId, name). This inconsistency could lead to confusion or bugs when calling these handlers.Consider unifying the parameter order for both handlers to improve API consistency:
♻️ Suggested refactor for consistent parameter order
-const handleAddFreshParticipant = useCallback(async (agentId: string, name: string) => { +const handleAddFreshParticipant = useCallback(async (name: string, agentId: string) => { const { activeGroupChatId } = useGroupChatStore.getState(); const id = activeGroupChatId; if (!id) return; try { - await window.maestro.groupChat.addFreshParticipant(id, name, agentId); + await window.maestro.groupChat.addFreshParticipant(id, name, agentId); useModalStore.getState().closeModal('addGroupChatParticipant');Note: If changing the signature, ensure all callers (e.g., AddParticipantModal) are updated accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts` around lines 653 - 690, Change the parameter order to be consistent across handlers by making handleAddExistingParticipant accept (agentId: string, name: string, cwd: string) to match handleAddFreshParticipant's agentId-first convention; update the call inside handleAddExistingParticipant to pass agentId and name in the correct positions when calling window.maestro.groupChat.addParticipant(id, name, agentId, cwd) (or adjust to the maestro API if it expects agentId first), and update any callers (e.g., AddParticipantModal and any other invocations) and related types/signatures so all usages of handleAddExistingParticipant and handleAddFreshParticipant use the same (agentId, name[, cwd]) ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/handlers/groupChat.ts`:
- Around line 547-585: The IPC handler 'groupChat:addFreshParticipant' validates
id and name but omits validating agentId at the boundary; update the handler to
validate agentId before calling addFreshParticipant—either call the existing
validateAgentId(agentId) helper (if available) or perform an explicit check
using the agentDetector (e.g., call agentDetector.getAgent(agentId) or
hasAgent(agentId) inside the IPC handler and throw a clear error if missing).
Apply the same pattern to the analogous 'addParticipant' handler so all handlers
that accept agentId validate it consistently before delegating to
addFreshParticipant/addParticipant.
---
Outside diff comments:
In `@src/main/ipc/handlers/groupChat.ts`:
- Around line 774-780: The handler's validation call discards the validated
value for entryId; update the async handler that takes (groupChatId, entryId) so
that you assign the result of validateGroupChatId back to entryId (just like
groupChatId = validateGroupChatId(groupChatId)), ensuring you call
validateGroupChatId for entryId and reassign it before calling logger.debug and
deleteGroupChatHistoryEntry(groupChatId, entryId); keep the same function names
(validateGroupChatId, logger.debug, deleteGroupChatHistoryEntry) and behavior.
---
Nitpick comments:
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 653-690: Change the parameter order to be consistent across
handlers by making handleAddExistingParticipant accept (agentId: string, name:
string, cwd: string) to match handleAddFreshParticipant's agentId-first
convention; update the call inside handleAddExistingParticipant to pass agentId
and name in the correct positions when calling
window.maestro.groupChat.addParticipant(id, name, agentId, cwd) (or adjust to
the maestro API if it expects agentId first), and update any callers (e.g.,
AddParticipantModal and any other invocations) and related types/signatures so
all usages of handleAddExistingParticipant and handleAddFreshParticipant use the
same (agentId, name[, cwd]) ordering.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/__tests__/main/group-chat/group-chat-lock.test.tssrc/__tests__/main/group-chat/group-chat-moderator.test.tssrc/__tests__/main/ipc/handlers/groupChat.test.tssrc/main/group-chat/group-chat-lock.tssrc/main/group-chat/group-chat-moderator.tssrc/main/group-chat/group-chat-router.tssrc/main/ipc/handlers/groupChat.tssrc/main/preload/groupChat.tssrc/main/process-manager/ProcessManager.tssrc/renderer/components/AddParticipantModal.tsxsrc/renderer/components/AppModals.tsxsrc/renderer/components/GroupChatRightPanel.tsxsrc/renderer/global.d.tssrc/renderer/hooks/groupChat/useGroupChatHandlers.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/process-manager/ProcessManager.ts
- src/renderer/global.d.ts
- src/tests/main/ipc/handlers/groupChat.test.ts
- src/tests/main/group-chat/group-chat-lock.test.ts
- src/main/group-chat/group-chat-lock.ts
- src/renderer/components/AddParticipantModal.tsx
- src/renderer/components/AppModals.tsx
- src/main/group-chat/group-chat-moderator.ts
Comment on lines
+547
to
+585
| // Add a fresh participant (by agent type, no session overrides) | ||
| ipcMain.handle( | ||
| 'groupChat:addFreshParticipant', | ||
| withIpcErrorLogging( | ||
| handlerOpts('addFreshParticipant'), | ||
| async ( | ||
| id: string, | ||
| name: string, | ||
| agentId: string, | ||
| cwd?: string | ||
| ): Promise<GroupChatParticipant> => { | ||
| id = validateGroupChatId(id); | ||
| validateParticipantName(name); | ||
| const processManager = getProcessManager(); | ||
| if (!processManager) { | ||
| throw new Error('Process manager not initialized'); | ||
| } | ||
|
|
||
| const agentDetector = getAgentDetector(); | ||
| if (!agentDetector) { | ||
| throw new Error('Agent detector not initialized'); | ||
| } | ||
| const agentConfigValues = getAgentConfig?.(agentId) || {}; | ||
|
|
||
| logger.info(`Adding fresh participant ${name} (${agentId}) to ${id}`, LOG_CONTEXT); | ||
| const participant = await addFreshParticipant( | ||
| id, | ||
| name, | ||
| agentId, | ||
| processManager, | ||
| cwd || os.homedir(), | ||
| agentDetector, | ||
| agentConfigValues | ||
| ); | ||
| logger.info(`Added fresh participant: ${name}`, LOG_CONTEXT); | ||
| return participant; | ||
| } | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if agentId validation exists elsewhere
rg -n 'validateAgentId|validate.*agentId' src/main/Repository: RunMaestro/Maestro
Length of output: 44
🏁 Script executed:
# Check what validation functions are available
rg -n 'export.*function.*validate|export const.*validate' src/main/ -A 2 | head -50Repository: RunMaestro/Maestro
Length of output: 1463
🏁 Script executed:
# Look at other handlers in the groupChat.ts file that use agentId to see if they validate it
rg -B 5 -A 10 "agentId:" src/main/ipc/handlers/groupChat.ts | head -100Repository: RunMaestro/Maestro
Length of output: 1605
🏁 Script executed:
# Check the addFreshParticipant function implementation to see how it handles agentId
fd -t f 'groupChat' src/main/ --exec grep -l 'addFreshParticipant' {} \;Repository: RunMaestro/Maestro
Length of output: 126
🏁 Script executed:
# Search for agentId parameter usage in handlers to see validation patterns
rg -n 'agentId.*string' src/main/ipc/handlers/ -B 3 -A 3 | head -80Repository: RunMaestro/Maestro
Length of output: 5313
🏁 Script executed:
# Find the actual addFreshParticipant function implementation (not the handler)
rg -n "function addFreshParticipant|const addFreshParticipant|export.*addFreshParticipant" src/main/ -A 15Repository: RunMaestro/Maestro
Length of output: 1202
🏁 Script executed:
# Look at the full addParticipant handler to compare validation
rg -B 2 -A 30 "groupChat:addParticipant" src/main/ipc/handlers/groupChat.tsRepository: RunMaestro/Maestro
Length of output: 1019
🏁 Script executed:
# Check if agentId is validated anywhere in the agents or group-chat modules
rg -n "getAgentDetector.*getAgent|agent.*not.*found|invalid.*agent" src/main/group-chat/ -B 2 -A 2Repository: RunMaestro/Maestro
Length of output: 44
🏁 Script executed:
# Check the addParticipant function implementation to see how it handles agentId
rg -n "function addParticipant|const addParticipant" src/main/group-chat/ -A 30 | head -80Repository: RunMaestro/Maestro
Length of output: 4773
🏁 Script executed:
# Look for where agentId is actually used in the addParticipant/addFreshParticipant flow
rg -n "agentDetector.*agentId|agentId.*agentDetector" src/main/group-chat/ -B 3 -A 3Repository: RunMaestro/Maestro
Length of output: 1838
🏁 Script executed:
# Check if agentId being passed to getAgent or similar methods validates it
rg -n "getAgent\(|getAgent<|agentDetector\.get" src/main/group-chat/ -B 2 -A 5Repository: RunMaestro/Maestro
Length of output: 6064
Missing agentId validation at IPC boundary in addFreshParticipant handler.
The handler validates id and name at the boundary but does not validate agentId. While the downstream addParticipant function calls agentDetector.getAgent(agentId) which throws if the agent is unavailable, explicit validation at the IPC boundary would provide consistent behavior with other parameters and clearer error messages. Note that the similar addParticipant handler has the same pattern—this appears to be a systemic issue affecting multiple handlers that accept agentId.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/handlers/groupChat.ts` around lines 547 - 585, The IPC handler
'groupChat:addFreshParticipant' validates id and name but omits validating
agentId at the boundary; update the handler to validate agentId before calling
addFreshParticipant—either call the existing validateAgentId(agentId) helper (if
available) or perform an explicit check using the agentDetector (e.g., call
agentDetector.getAgent(agentId) or hasAgent(agentId) inside the IPC handler and
throw a clear error if missing). Apply the same pattern to the analogous
'addParticipant' handler so all handlers that accept agentId validate it
consistently before delegating to addFreshParticipant/addParticipant.
The component accepted groupChatId as a prop but never used it — all participant operations delegate through callbacks that read activeGroupChatId from the store. Also fixed a pre-existing test assertion that still expected the removed sessionId argument. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…refix matching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…hat-router Replace silent catch blocks in routeUserMessage and spawnModeratorSynthesis with logger.debug() calls that log the agent ID and error for debuggability. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
resetGroupChatState should only clear active chat fields (activeGroupChatId, messages, state, participants, error). Staged images are user input that should not be lost when closing a chat. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ent detection Removes the unused `err` parameter from the catch block in the agent detection useEffect to eliminate ESLint no-unused-vars warning. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/parsers/codex-output-parser.ts (1)
151-162: Apply the same longest-prefix-first sorting here for consistency.
getModelPricing(lines 80-85) now sorts prefixes longest-first to ensure correct matching, butgetModelContextWindowstill iterates unsorted. This could produce incorrect results if a model likegpt-5.2-foomatchesgpt-5(200000) beforegpt-5.2(400000) due to object iteration order.♻️ Proposed fix for consistency
function getModelContextWindow(model: string): number { // Try exact match first if (MODEL_CONTEXT_WINDOWS[model]) { return MODEL_CONTEXT_WINDOWS[model]; } // Try prefix match (e.g., "gpt-4o-2024-11-20" matches "gpt-4o") - for (const [prefix, size] of Object.entries(MODEL_CONTEXT_WINDOWS)) { + const sorted = Object.entries(MODEL_CONTEXT_WINDOWS).sort((a, b) => b[0].length - a[0].length); + for (const [prefix, size] of sorted) { if (model.startsWith(prefix)) { return size; } } return MODEL_CONTEXT_WINDOWS['default']; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/parsers/codex-output-parser.ts` around lines 151 - 162, The getModelContextWindow function can match shorter prefixes before longer ones due to unordered iteration of MODEL_CONTEXT_WINDOWS; modify getModelContextWindow to iterate the MODEL_CONTEXT_WINDOWS keys sorted longest-first (e.g., sort Object.keys(MODEL_CONTEXT_WINDOWS) by descending length) and then return the size for the first prefix where model.startsWith(prefix), falling back to MODEL_CONTEXT_WINDOWS['default'] if none match, mirroring the longest-prefix-first logic used in getModelPricing.src/renderer/components/AppModals.tsx (1)
1539-1555: Consider extracting group chat lookup for consistency.The IIFE pattern works but differs from the style used for other group chat lookups (lines 1467-1481). Consider extracting the lookup for consistency.
♻️ Optional refactor for consistency
const infoGroupChat = activeGroupChatId ? groupChats.find((c) => c.id === activeGroupChatId) : null; + const addParticipantGroupChat = showAddParticipantModal + ? groupChats.find((c) => c.id === showAddParticipantModal) + : null; + return ( <> ... {/* --- ADD PARTICIPANT MODAL --- */} - {showAddParticipantModal && - (() => { - const addParticipantGroupChat = groupChats.find((c) => c.id === showAddParticipantModal); - return ( - <AddParticipantModal - theme={theme} - isOpen={!!showAddParticipantModal} - sessions={sessions} - participants={addParticipantGroupChat?.participants || []} - onClose={onCloseAddParticipantModal} - onAddExisting={onAddExistingParticipant} - onAddFresh={onAddFreshParticipant} - /> - ); - })()} + {showAddParticipantModal && ( + <AddParticipantModal + theme={theme} + isOpen={!!showAddParticipantModal} + sessions={sessions} + participants={addParticipantGroupChat?.participants || []} + onClose={onCloseAddParticipantModal} + onAddExisting={onAddExistingParticipant} + onAddFresh={onAddFreshParticipant} + /> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AppModals.tsx` around lines 1539 - 1555, The add-participant modal currently uses an IIFE to find the group chat (const addParticipantGroupChat = groupChats.find((c) => c.id === showAddParticipantModal)) which is inconsistent with other lookups; pull that lookup out above the JSX (e.g. compute addParticipantGroupChat when showAddParticipantModal is truthy) and then render <AddParticipantModal ... participants={addParticipantGroupChat?.participants || []} isOpen={!!showAddParticipantModal} onClose={onCloseAddParticipantModal} onAddExisting={onAddExistingParticipant} onAddFresh={onAddFreshParticipant} /> directly without the IIFE to match the style used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/AddParticipantModal.tsx`:
- Around line 106-136: The handleSubmit function currently calls
onAddExisting/onAddFresh synchronously and never resets isSubmitting on success;
change it to await the potentially async callbacks (await onAddExisting(...) /
await onAddFresh(...)) inside the try block and ensure setIsSubmitting(false) is
called in a finally block so isSubmitting is reset whether the call succeeds or
throws; keep the existing setError handling in the catch (using err instanceof
Error) and retain the early-return checks for selectedSessionId/selectedAgentId
and session/tile lookups.
---
Nitpick comments:
In `@src/main/parsers/codex-output-parser.ts`:
- Around line 151-162: The getModelContextWindow function can match shorter
prefixes before longer ones due to unordered iteration of MODEL_CONTEXT_WINDOWS;
modify getModelContextWindow to iterate the MODEL_CONTEXT_WINDOWS keys sorted
longest-first (e.g., sort Object.keys(MODEL_CONTEXT_WINDOWS) by descending
length) and then return the size for the first prefix where
model.startsWith(prefix), falling back to MODEL_CONTEXT_WINDOWS['default'] if
none match, mirroring the longest-prefix-first logic used in getModelPricing.
In `@src/renderer/components/AppModals.tsx`:
- Around line 1539-1555: The add-participant modal currently uses an IIFE to
find the group chat (const addParticipantGroupChat = groupChats.find((c) => c.id
=== showAddParticipantModal)) which is inconsistent with other lookups; pull
that lookup out above the JSX (e.g. compute addParticipantGroupChat when
showAddParticipantModal is truthy) and then render <AddParticipantModal ...
participants={addParticipantGroupChat?.participants || []}
isOpen={!!showAddParticipantModal} onClose={onCloseAddParticipantModal}
onAddExisting={onAddExistingParticipant} onAddFresh={onAddFreshParticipant} />
directly without the IIFE to match the style used elsewhere.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/__tests__/renderer/components/AddParticipantModal.test.tsxsrc/main/group-chat/group-chat-router.tssrc/main/parsers/codex-output-parser.tssrc/renderer/components/AddParticipantModal.tsxsrc/renderer/components/AppModals.tsxsrc/renderer/stores/groupChatStore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/stores/groupChatStore.ts
Comment on lines
+106
to
+136
| const handleSubmit = useCallback(async () => { | ||
| setError(null); | ||
| setIsSubmitting(true); | ||
|
|
||
| try { | ||
| if (mode === 'existing') { | ||
| if (!selectedSessionId) { | ||
| setIsSubmitting(false); | ||
| return; | ||
| } | ||
| const session = sessions.find((s) => s.id === selectedSessionId); | ||
| if (!session) { | ||
| setIsSubmitting(false); | ||
| return; | ||
| } | ||
| onAddExisting(session.name, session.toolType, session.cwd); | ||
| } else { | ||
| if (!selectedAgentId) { | ||
| setIsSubmitting(false); | ||
| return; | ||
| } | ||
| const tile = AGENT_TILES.find((t) => t.id === selectedAgentId); | ||
| const name = tile?.name || selectedAgentId; | ||
| onAddFresh(selectedAgentId, name); | ||
| } | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : 'Failed to add participant'); | ||
| setIsSubmitting(false); | ||
| return; | ||
| } | ||
| }, [mode, selectedSessionId, selectedAgentId, sessions, onAddExisting, onAddFresh]); |
There was a problem hiding this comment.
isSubmitting is never reset after successful submission.
When onAddExisting or onAddFresh complete successfully (without throwing), isSubmitting remains true. The modal relies on the parent closing it via onClose, but if the parent doesn't close the modal immediately, the Add button stays disabled.
Additionally, if onAddExisting/onAddFresh are async functions that return a Promise, the current synchronous try/catch won't catch their rejections.
🛠️ Suggested fix: Reset isSubmitting and handle async callbacks
const handleSubmit = useCallback(async () => {
setError(null);
setIsSubmitting(true);
try {
if (mode === 'existing') {
if (!selectedSessionId) {
setIsSubmitting(false);
return;
}
const session = sessions.find((s) => s.id === selectedSessionId);
if (!session) {
setIsSubmitting(false);
return;
}
- onAddExisting(session.name, session.toolType, session.cwd);
+ await Promise.resolve(onAddExisting(session.name, session.toolType, session.cwd));
} else {
if (!selectedAgentId) {
setIsSubmitting(false);
return;
}
const tile = AGENT_TILES.find((t) => t.id === selectedAgentId);
const name = tile?.name || selectedAgentId;
- onAddFresh(selectedAgentId, name);
+ await Promise.resolve(onAddFresh(selectedAgentId, name));
}
+ // Parent is expected to close the modal on success
} catch (err) {
setError(err instanceof Error ? err.message : 'Failed to add participant');
setIsSubmitting(false);
- return;
}
}, [mode, selectedSessionId, selectedAgentId, sessions, onAddExisting, onAddFresh]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/AddParticipantModal.tsx` around lines 106 - 136, The
handleSubmit function currently calls onAddExisting/onAddFresh synchronously and
never resets isSubmitting on success; change it to await the potentially async
callbacks (await onAddExisting(...) / await onAddFresh(...)) inside the try
block and ensure setIsSubmitting(false) is called in a finally block so
isSubmitting is reset whether the call succeeds or throws; keep the existing
setError handling in the catch (using err instanceof Error) and retain the
early-return checks for selectedSessionId/selectedAgentId and session/tile
lookups.
Contributor
Author
Review Fixes AppliedAll conflict resolutions and review items from CodeRabbit and code review have been addressed across 12 commits. Summary below. Critical Fix
Major Fixes
Minor Fixes
Nitpick Fixes
Additional Fix
Verification
|
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.
Summary
killModerator()andclearAllParticipantSessions()silently failed because batch mode spawns processes with timestamp-suffixed session IDs while the active sessions map stores prefix-only IDs. AddedkillByPrefix()toProcessManager— fixes zombie processes and the observedy\nflood after delete.codex-0-*) so participant cards show an ID instead of "pending". Resume logic skips these prefixed IDs. Added OpenAI model pricing table (GPT-4o through GPT-5.3-codex) to calculate cost from token counts since Codex CLI doesn't report cost directly.## MANDATORY: Always @mention agents when delegatingsection to moderator system prompt. Prevents the moderator from discussing agent tasks without actually routing messages via @mentions. Updated synthesis prompt for critical analysis and fusion instead of simple concatenation.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests