refactor: Refactor message handlers into dedicated modules - #1332
Merged
Conversation
- extract Auto Run and Tabs domains (23 handlers total) into autoRun.ts and tabs.ts - add types.ts, shared.ts, and an index.ts barrel matching the MainPanel/ convention - extracted handlers take an explicit MessageHandlerContext instead of `this`, so they don't depend on class state - WebSocketMessageHandler.ts drops from 5,913 to 4,455 lines; pure code motion, no behavior change
…andler - Extracted the remaining 19 domain handlers into dedicated module files. - Reduced WebSocketMessageHandler.ts from 6,000+ down to 680 lines, leaving only core routing logic. - Refactored handlers to take an explicit MessageHandlerContext instead of `this`, with zero behavioral changes. - Validated with tsc, ESLint, Prettier, full test, and live end-to-end testing on the browser, desktop and CLI.
📝 WalkthroughWalkthroughIntroduces a typed WebSocket message-handler architecture with centralized dispatch, callback contracts, validation, error reporting, and handlers spanning sessions, tabs, commands, automation, marketplace, movement, notifications, Git, queues, plugins, statistics, and profiling. ChangesWebSocket handler architecture
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR reorganizes the WebSocket message-handling layer without establishing a behavioral change.
Confidence Score: 5/5The PR appears safe to merge because no concrete regression caused by the handler extraction was established. The refactor retains central message routing and callback-driven behavior while relocating domain implementations into dedicated modules, with no accepted build, protocol, state, or security-boundary failure attributable to the changed code. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Client[WebSocket client] --> Router[WebSocketMessageHandler]
Router --> Context[MessageHandlerContext]
Router --> Commands[Command handlers]
Router --> Sessions[Session and tab handlers]
Router --> Files[File tree and auto-run handlers]
Router --> Integrations[Git, Cue, plugins, marketplace]
Router --> UI[Notifications, movement, cadenza]
Commands --> Callbacks[MessageHandlerCallbacks]
Sessions --> Callbacks
Files --> Callbacks
Integrations --> Callbacks
UI --> Callbacks
Context --> Responses[WebSocket responses and error reporting]
Reviews (1): Last reviewed commit: "refactor: decompose remaining message ha..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (8)
src/main/web-server/handlers/messageHandlers/notifications.ts (1)
17-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
NOTIFY_FLASH_COLORSandNOTIFY_TOAST_COLORSare identical aliases ofNOTIFY_COLORS. Three names for one list adds indirection with no behavioral difference; useNOTIFY_COLORSdirectly unless the sets are expected to diverge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/web-server/handlers/messageHandlers/notifications.ts` around lines 17 - 27, Remove the redundant NOTIFY_FLASH_COLORS and NOTIFY_TOAST_COLORS aliases, and update their usages to reference NOTIFY_COLORS directly. Keep the existing color values and behavior unchanged.src/main/web-server/handlers/messageHandlers/commands.ts (1)
134-140: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnwrapped
error.messagein rejection handlers across the extracted modules. All of these.catchblocks assume the rejection value is anError; a non-Error rejection makeserror.messagethrow inside the handler, so the client receives no response and the failure surfaces as an unhandled rejection instead of a reported error.cue.tsalready normalizes witherror instanceof Error ? error : new Error(String(error)); apply that consistently.
src/main/web-server/handlers/messageHandlers/commands.ts#L134-L140: normalize the rejection before formatting (also lines 240-242 and 294-296).src/main/web-server/handlers/messageHandlers/cadenza.ts#L104-L108: normalize in thecadenzaViewcatch.src/main/web-server/handlers/messageHandlers/movement.ts#L192-L196: normalize in themovementViewcatch and the other three catches (lines 230, 269, 343).src/main/web-server/handlers/messageHandlers/notifications.ts#L204-L206: normalize in thenotifyToastandnotifyCenterFlashcatches (line 282).As per coding guidelines, "Do not silently swallow unexpected exceptions. Handle known recoverable errors explicitly, rethrow unexpected errors so Sentry can capture them".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/web-server/handlers/messageHandlers/commands.ts` around lines 134 - 140, Normalize rejection values before accessing error.message in the catches for commands.ts lines 134-140, 240-242, and 294-296; cadenza.ts lines 104-108; movement.ts lines 192-196, 230, 269, and 343; and notifications.ts lines 204-206 and 282. Reuse the cue.ts Error normalization pattern so all client responses and logs receive safe error details, while explicitly handling known recoverable errors and rethrowing unexpected exceptions for Sentry capture.Source: Coding guidelines
src/main/web-server/handlers/messageHandlers/groups.ts (1)
39-51: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
emojiis cast, not validated.
parentGroupIdgets strict runtime validation two lines below, butemojiis trusted blindly from the wire and forwarded intocreateGroup. A number or object will be persisted as-is.♻️ Proposed fix
- const emoji = message.emoji as string | undefined; + if (message.emoji !== undefined && typeof message.emoji !== 'string') { + ctx.sendError(client, 'Invalid emoji'); + return; + } + const emoji = message.emoji;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/web-server/handlers/messageHandlers/groups.ts` around lines 39 - 51, Validate message.emoji at runtime before passing it to createGroup, accepting only a string or undefined and rejecting other wire values with ctx.sendError(client, ...) followed by return. Update the emoji handling near requestedParentGroupId so createGroup receives only a trimmed or otherwise validated string value consistent with the existing input-validation pattern.src/main/web-server/handlers/messageHandlers/plugins.ts (1)
46-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winContribution-projection failure reports
success: truewith zero tools.An unexpected throw from
getContributions()is downgraded to a warn log and indistinguishable from "plugins off", so the bridge silently loses every tool. Report it via the Sentry utility so it is not invisible.As per coding guidelines: "Do not silently swallow unexpected exceptions... use the Sentry reporting utilities for intentional exception or event reporting".
♻️ Proposed fix
} catch (error) { const reason = error instanceof Error ? error.message : String(error); logger.warn(`[Web] plugins_list_tools failed: ${reason}`, LOG_CONTEXT); + captureException(error instanceof Error ? error : new Error(reason), { + extra: { area: 'web-server', handler: 'plugins_list_tools' }, + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/web-server/handlers/messageHandlers/plugins.ts` around lines 46 - 56, Update the getContributions() error path in the plugins_list_tools handler to report unexpected exceptions through the project’s Sentry utility in addition to retaining appropriate logging. Ensure the handler does not silently present a contribution-projection failure as a successful zero-tool response, while preserving the existing successful response behavior for valid empty results.Source: Coding guidelines
src/main/web-server/handlers/messageHandlers/settings.ts (1)
58-74: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate web setting values before persisting them.
set_settingallows the key, but the value is still cast toSettingValuebefore being sent through IPC and persisted by the genericsettings:sethandler. A web-originated{ key: 'fontSize', value: { } }would bypass any per-key type shape validation; add a per-key validator forALLOWED_SETTING_KEYSbefore returning success.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/web-server/handlers/messageHandlers/settings.ts` around lines 58 - 74, Validate the incoming value for each key in the set_setting handler before sending it through IPC or returning success. Add or reuse per-key validation associated with ALLOWED_SETTING_KEYS, including rejecting invalid object shapes such as an empty object for fontSize, and send the existing error response when validation fails.src/main/web-server/handlers/messageHandlers/marketplace.ts (1)
48-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the "(coderabbit feedback)" attribution from the doc comment.
Review-process artifacts do not belong in shipped source comments; the rationale itself is worth keeping.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/web-server/handlers/messageHandlers/marketplace.ts` around lines 48 - 52, Remove the “(coderabbit feedback)” attribution from the doc comment above the typed marketplace result failure handling, while preserving the explanation about clients otherwise missing the failure and timing out.src/main/web-server/handlers/messageHandlers/fileTree.ts (1)
109-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a named recursive
FileTreeNodetype instead of inlineany[]children.The inline shape is duplicated at Line 113 and Lines 146-151 and loses type safety on
children.♻️ Suggested type
+interface FileTreeNode { + name: string; + type: 'file' | 'folder'; + path: string; + children?: FileTreeNode[]; +} + async function buildFileTree( dirPath: string, maxDepth: number, currentDepth = 0 -): Promise< - Array<{ - name: string; - type: 'file' | 'folder'; - children?: Array<{ name: string; type: 'file' | 'folder'; children?: any[]; path: string }>; - path: string; - }> -> { +): Promise<FileTreeNode[]> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/web-server/handlers/messageHandlers/fileTree.ts` around lines 109 - 116, Define a named recursive FileTreeNode type for the file-tree shape, with children typed as FileTreeNode[] rather than any[]. Update the return type of the surrounding file-tree function and the duplicated node declarations around the recursive construction to reuse FileTreeNode, preserving the existing name, type, and path fields.src/main/web-server/handlers/messageHandlers/autoRun.ts (1)
519-532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne filename-safety predicate is implemented three times. The same rules (reject
.., backslashes, leading/, Windows drive prefixes) are duplicated instead of living in a single shared helper, so the rules can drift.
src/main/web-server/handlers/messageHandlers/autoRun.ts#L519-L532: replace the inline predicate with the shared validator.src/main/web-server/handlers/messageHandlers/playbooks.ts#L31-L34: import the shared validator inparsePlaybookDocumentsinstead of re-checking inline.- Promote
isValidFilename(src/main/web-server/handlers/messageHandlers/autoRun.ts#L28-L37) intosrc/main/web-server/handlers/messageHandlers/shared.tsas the canonical implementation.As per coding guidelines: "Before creating a new utility, helper, hook, component, type, or constant, check the relevant guide in
docs/agent-guides/and reuse or extend the canonical implementation instead of duplicating it."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/web-server/handlers/messageHandlers/autoRun.ts` around lines 519 - 532, Promote the existing isValidFilename helper from autoRun.ts into messageHandlers/shared.ts as the canonical validator, after checking the relevant agent guide. In autoRun.ts lines 519-532, replace the duplicated inline predicate with isValidFilename; in playbooks.ts lines 31-34, import and use the same shared validator in parsePlaybookDocuments. Preserve the existing rejection rules for traversal, backslashes, absolute paths, and Windows drive prefixes at both sites.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/web-server/handlers/messageHandlers/autoRun.ts`:
- Around line 70-72: Replace the bare error handling in autoRun.ts lines 70-72
and the catches in handleConfigureAutoRun, handleGetAutoRunDocs,
handleGetAutoRunDocument, handleSaveAutoRunDocument, and handleStopAutoRun with
ctx.reportHandlerError, preserving the appropriate handler context. In
fileTree.ts lines 44-46, update handleRefreshFileTree to use
ctx.reportHandlerError with sessionId and requestId instead of sending
error.message directly.
- Around line 103-116: Update the documents validation loop in the auto-run
handler to validate each non-empty doc.filename with the existing
isValidFilename validator before accepting it. Preserve the current object,
non-empty string, and resetOnCompletion checks, and send the established
filename validation error through ctx.sendError when validation fails.
In `@src/main/web-server/handlers/messageHandlers/commands.ts`:
- Around line 88-92: Update the prominent logging call in the web command
handler to avoid logging the full effectiveCommand; use the existing
50-character truncation approach or log only safe metadata such as command
length, while preserving the mode, session, image-count, and other non-sensitive
context.
In `@src/main/web-server/handlers/messageHandlers/contextOps.ts`:
- Around line 36-48: Standardize promise rejection handling across the listed
handlers by changing every affected catch callback to accept error as unknown
and route it through ctx.reportHandlerError instead of interpolating
error.message. Update mergeContext, transferContext, summarizeContext,
createGroup, renameGroup, deleteGroup, moveSessionToGroup, getGroupChats and the
other group-chat handlers, get_git_status, get_git_diff, setSetting,
getUsageDashboard, getAchievements, and generateDirectorNotesSynopsis; preserve
each handler’s existing response behavior while ensuring unexpected exceptions
reach Sentry. Affected sites:
src/main/web-server/handlers/messageHandlers/contextOps.ts:36-48,
groups.ts:73-75, groupChat.ts:32-34, git.ts:40-42, settings.ts:91-93, and
stats.ts:45-47.
In `@src/main/web-server/handlers/messageHandlers/cue.ts`:
- Around line 93-94: Validate and bound the limit value in the message handler
before passing it to getCueActivity: accept only a finite number within the
supported maximum, and fall back to 50 for invalid, nullish, non-numeric, or
oversized values. Keep the existing sessionId handling and ensure getCueActivity
never receives an unbounded limit.
In `@src/main/web-server/handlers/messageHandlers/fileTree.ts`:
- Around line 154-160: Update the entries filter in the file-tree sorting flow
to rely solely on the IGNORE set, removing the name-prefix exclusion so
legitimate dot-prefixed entries remain visible. Preserve the existing
directory-first and name-based sorting in the sorted pipeline.
In `@src/main/web-server/handlers/messageHandlers/git.ts`:
- Around line 53-67: Validate filePath in the git diff handler before invoking
ctx.callbacks.getGitDiff, rejecting missing or out-of-session paths through the
existing error response flow. Reuse the project’s established path-validation
logic to prevent traversal such as ../../etc/passwd, while preserving the
existing sessionId and callback checks.
In `@src/main/web-server/handlers/messageHandlers/groupChat.ts`:
- Around line 46-56: Validate every element of participantIds in the group-chat
handler before calling startGroupChat, rejecting arrays containing non-string
values while preserving the existing minimum-two-participants check and error
flow.
In `@src/main/web-server/handlers/messageHandlers/groups.ts`:
- Around line 162-174: Update the validation in the message handler around
sessionId and groupId to require groupId to be either a string or null,
rejecting values such as numbers and objects before calling moveSessionToGroup.
Preserve the existing missing-property error for messages without a groupId
field and keep the valid null case for ungrouped sessions.
In `@src/main/web-server/handlers/messageHandlers/marketplace.ts`:
- Around line 380-397: The targetFolderName validation around trimmedFolder must
reject a bare "." in addition to the existing traversal and path checks,
preventing imports from targeting the Auto Run root. Add the explicit "." guard
or reuse assertSafeTargetFolderName if it enforces the same single-subfolder
requirement, while preserving the existing failure response.
- Around line 20-46: Update isUntrustedLocalPath to reject scheme-prefixed
playbook paths, including http://, file://, and data: forms, alongside the
existing absolute-path, backslash, and traversal checks. Use a scheme-detection
guard that covers standard URI schemes before fetchDocument, fetchAsset, or
fetchReadme can construct the GitHub raw URL.
In `@src/main/web-server/handlers/messageHandlers/profiling.ts`:
- Around line 96-121: Constrain the client-supplied outputPath in the profiling
stop handler to a configured or app.getPath('downloads') capture root, rejecting
paths outside that root; require a .zip extension and reject an existing
destination before directory creation or finalizeCapture. Update the validation
around profiling_stop and preserve the existing stopProfiling/finalizeCapture
flow only for validated, non-colliding paths.
In `@src/main/web-server/handlers/messageHandlers/sessions.ts`:
- Around line 20-39: Update handleGetSessions so it sends a response when any
required callback is unavailable instead of silently returning. Add an else
branch that uses the existing ctx.send mechanism to return the module’s standard
error frame or an empty sessions_list, while preserving the current enrichment
behavior when all callbacks are present.
In `@src/main/web-server/handlers/messageHandlers/stats.ts`:
- Around line 90-120: Update handleStatsQuery and the runReadonlyStatsQuery
execution path to enforce a statement timeout for synchronous better-sqlite3
calls, using the statement interrupt mechanism to stop queries that exceed the
configured limit. Catch timeout failures as errors and send them through
ctx.sendError without sending stats_query_result, while preserving the existing
read-only checks, row cap, and successful response behavior.
In `@src/main/web-server/handlers/messageHandlers/tabs.ts`:
- Around line 223-234: Validate fromIndex and toIndex in the reorder_tab handler
before calling reorderTab, rejecting values that are not finite numbers in
addition to missing values. Preserve the existing missing-field error response
and ensure malformed client input cannot reach reorderTab; use the existing
handler validation pattern elsewhere in tabs.ts.
In `@src/main/web-server/handlers/messageHandlers/terminal.ts`:
- Around line 19-44: Update both terminal handlers around the validation
failures and successful write/resize responses to include the incoming
message.requestId on every terminal_write_result and terminal_resize_result
frame. Preserve the existing success, error, and sessionId fields while ensuring
all early-return and success paths echo the request identifier for client
correlation.
In `@src/main/web-server/handlers/messageHandlers/WebSocketMessageHandler.ts`:
- Around line 664-680: Remove the stale “Handle get_sessions message” doc
comment above handleGetAppInfo, leaving the handleGetAppInfo documentation
directly associated with that method. Do not remove or alter handleGetSessions
or other session-related code.
---
Nitpick comments:
In `@src/main/web-server/handlers/messageHandlers/autoRun.ts`:
- Around line 519-532: Promote the existing isValidFilename helper from
autoRun.ts into messageHandlers/shared.ts as the canonical validator, after
checking the relevant agent guide. In autoRun.ts lines 519-532, replace the
duplicated inline predicate with isValidFilename; in playbooks.ts lines 31-34,
import and use the same shared validator in parsePlaybookDocuments. Preserve the
existing rejection rules for traversal, backslashes, absolute paths, and Windows
drive prefixes at both sites.
In `@src/main/web-server/handlers/messageHandlers/commands.ts`:
- Around line 134-140: Normalize rejection values before accessing error.message
in the catches for commands.ts lines 134-140, 240-242, and 294-296; cadenza.ts
lines 104-108; movement.ts lines 192-196, 230, 269, and 343; and
notifications.ts lines 204-206 and 282. Reuse the cue.ts Error normalization
pattern so all client responses and logs receive safe error details, while
explicitly handling known recoverable errors and rethrowing unexpected
exceptions for Sentry capture.
In `@src/main/web-server/handlers/messageHandlers/fileTree.ts`:
- Around line 109-116: Define a named recursive FileTreeNode type for the
file-tree shape, with children typed as FileTreeNode[] rather than any[]. Update
the return type of the surrounding file-tree function and the duplicated node
declarations around the recursive construction to reuse FileTreeNode, preserving
the existing name, type, and path fields.
In `@src/main/web-server/handlers/messageHandlers/groups.ts`:
- Around line 39-51: Validate message.emoji at runtime before passing it to
createGroup, accepting only a string or undefined and rejecting other wire
values with ctx.sendError(client, ...) followed by return. Update the emoji
handling near requestedParentGroupId so createGroup receives only a trimmed or
otherwise validated string value consistent with the existing input-validation
pattern.
In `@src/main/web-server/handlers/messageHandlers/marketplace.ts`:
- Around line 48-52: Remove the “(coderabbit feedback)” attribution from the doc
comment above the typed marketplace result failure handling, while preserving
the explanation about clients otherwise missing the failure and timing out.
In `@src/main/web-server/handlers/messageHandlers/notifications.ts`:
- Around line 17-27: Remove the redundant NOTIFY_FLASH_COLORS and
NOTIFY_TOAST_COLORS aliases, and update their usages to reference NOTIFY_COLORS
directly. Keep the existing color values and behavior unchanged.
In `@src/main/web-server/handlers/messageHandlers/plugins.ts`:
- Around line 46-56: Update the getContributions() error path in the
plugins_list_tools handler to report unexpected exceptions through the project’s
Sentry utility in addition to retaining appropriate logging. Ensure the handler
does not silently present a contribution-projection failure as a successful
zero-tool response, while preserving the existing successful response behavior
for valid empty results.
In `@src/main/web-server/handlers/messageHandlers/settings.ts`:
- Around line 58-74: Validate the incoming value for each key in the set_setting
handler before sending it through IPC or returning success. Add or reuse per-key
validation associated with ALLOWED_SETTING_KEYS, including rejecting invalid
object shapes such as an empty object for fontSize, and send the existing error
response when validation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 193f8ac1-e534-4c47-99cd-c138df7fee90
📒 Files selected for processing (26)
src/main/web-server/handlers/messageHandlers.tssrc/main/web-server/handlers/messageHandlers/WebSocketMessageHandler.tssrc/main/web-server/handlers/messageHandlers/autoRun.tssrc/main/web-server/handlers/messageHandlers/cadenza.tssrc/main/web-server/handlers/messageHandlers/commands.tssrc/main/web-server/handlers/messageHandlers/contextOps.tssrc/main/web-server/handlers/messageHandlers/cue.tssrc/main/web-server/handlers/messageHandlers/fileTree.tssrc/main/web-server/handlers/messageHandlers/git.tssrc/main/web-server/handlers/messageHandlers/groupChat.tssrc/main/web-server/handlers/messageHandlers/groups.tssrc/main/web-server/handlers/messageHandlers/index.tssrc/main/web-server/handlers/messageHandlers/marketplace.tssrc/main/web-server/handlers/messageHandlers/movement.tssrc/main/web-server/handlers/messageHandlers/notifications.tssrc/main/web-server/handlers/messageHandlers/playbooks.tssrc/main/web-server/handlers/messageHandlers/plugins.tssrc/main/web-server/handlers/messageHandlers/profiling.tssrc/main/web-server/handlers/messageHandlers/queue.tssrc/main/web-server/handlers/messageHandlers/sessions.tssrc/main/web-server/handlers/messageHandlers/settings.tssrc/main/web-server/handlers/messageHandlers/shared.tssrc/main/web-server/handlers/messageHandlers/stats.tssrc/main/web-server/handlers/messageHandlers/tabs.tssrc/main/web-server/handlers/messageHandlers/terminal.tssrc/main/web-server/handlers/messageHandlers/types.ts
Comment on lines
+70
to
+72
| .catch((error) => { | ||
| ctx.sendError(client, `Failed to refresh auto-run docs: ${error.message}`); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Several promise catch blocks bypass ctx.reportHandlerError. These handlers report failures with a bare ctx.sendError(..., error.message), so unexpected exceptions never reach Sentry and non-Error rejections surface as undefined to the client. Sibling handlers in the same layer already use the shared reporter.
src/main/web-server/handlers/messageHandlers/autoRun.ts#L70-L72: switch this catch (and the equivalents inhandleConfigureAutoRun,handleGetAutoRunDocs,handleGetAutoRunDocument,handleSaveAutoRunDocument,handleStopAutoRun) toctx.reportHandlerError.src/main/web-server/handlers/messageHandlers/fileTree.ts#L44-L46: switch thehandleRefreshFileTreecatch toctx.reportHandlerErrorwith{ sessionId, requestId }.
As per coding guidelines: "Do not silently swallow unexpected exceptions... use the Sentry reporting utilities for intentional exception or event reporting."
📍 Affects 2 files
src/main/web-server/handlers/messageHandlers/autoRun.ts#L70-L72(this comment)src/main/web-server/handlers/messageHandlers/fileTree.ts#L44-L46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers/autoRun.ts` around lines 70 -
72, Replace the bare error handling in autoRun.ts lines 70-72 and the catches in
handleConfigureAutoRun, handleGetAutoRunDocs, handleGetAutoRunDocument,
handleSaveAutoRunDocument, and handleStopAutoRun with ctx.reportHandlerError,
preserving the appropriate handler context. In fileTree.ts lines 44-46, update
handleRefreshFileTree to use ctx.reportHandlerError with sessionId and requestId
instead of sending error.message directly.
Source: Coding guidelines
Comment on lines
+103
to
+116
| for (const doc of documents) { | ||
| if (typeof doc !== 'object' || doc === null) { | ||
| ctx.sendError(client, 'Each document must be an object'); | ||
| return; | ||
| } | ||
| if (typeof doc.filename !== 'string' || doc.filename.trim() === '') { | ||
| ctx.sendError(client, 'Each document must have a non-empty string filename'); | ||
| return; | ||
| } | ||
| if (doc.resetOnCompletion !== undefined && typeof doc.resetOnCompletion !== 'boolean') { | ||
| ctx.sendError(client, 'resetOnCompletion must be a boolean if provided'); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
documents[].filename is not path-validated here, unlike every sibling boundary.
handleGetAutoRunDocument/handleSaveAutoRunDocument gate filenames through isValidFilename, and parsePlaybookDocuments in playbooks.ts applies the same rules. This loop only checks non-empty string, so ../../secrets.md reaches configureAutoRun and is resolved against the Auto Run root during the run.
🛡️ Reuse the existing validator
if (typeof doc.filename !== 'string' || doc.filename.trim() === '') {
ctx.sendError(client, 'Each document must have a non-empty string filename');
return;
}
+ if (!isValidFilename(doc.filename)) {
+ ctx.sendError(client, 'Invalid document filename');
+ return;
+ }📝 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
| for (const doc of documents) { | |
| if (typeof doc !== 'object' || doc === null) { | |
| ctx.sendError(client, 'Each document must be an object'); | |
| return; | |
| } | |
| if (typeof doc.filename !== 'string' || doc.filename.trim() === '') { | |
| ctx.sendError(client, 'Each document must have a non-empty string filename'); | |
| return; | |
| } | |
| if (doc.resetOnCompletion !== undefined && typeof doc.resetOnCompletion !== 'boolean') { | |
| ctx.sendError(client, 'resetOnCompletion must be a boolean if provided'); | |
| return; | |
| } | |
| } | |
| for (const doc of documents) { | |
| if (typeof doc !== 'object' || doc === null) { | |
| ctx.sendError(client, 'Each document must be an object'); | |
| return; | |
| } | |
| if (typeof doc.filename !== 'string' || doc.filename.trim() === '') { | |
| ctx.sendError(client, 'Each document must have a non-empty string filename'); | |
| return; | |
| } | |
| if (!isValidFilename(doc.filename)) { | |
| ctx.sendError(client, 'Invalid document filename'); | |
| return; | |
| } | |
| if (doc.resetOnCompletion !== undefined && typeof doc.resetOnCompletion !== 'boolean') { | |
| ctx.sendError(client, 'resetOnCompletion must be a boolean if provided'); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers/autoRun.ts` around lines 103 -
116, Update the documents validation loop in the auto-run handler to validate
each non-empty doc.filename with the existing isValidFilename validator before
accepting it. Preserve the current object, non-empty string, and
resetOnCompletion checks, and send the established filename validation error
through ctx.sendError when validation fails.
Comment on lines
+88
to
+92
| // Log all web interface commands prominently | ||
| logger.info( | ||
| `[Web Command] Mode: ${mode} | Session: ${sessionId}${isAiMode ? ` | Claude: ${claudeId}` : ''} | Message: ${effectiveCommand} | Images: ${images?.length ?? 0}`, | ||
| LOG_CONTEXT | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Full command text is logged at info level. Line 43 deliberately truncates the command to 50 chars, and tabs.ts (handleNewAITabWithPrompt, line 571) logs only promptLength because prompts can carry secrets or PII. This log writes the entire user message verbatim, so the mitigation elsewhere is defeated for the same content.
🔒 Proposed fix
logger.info(
- `[Web Command] Mode: ${mode} | Session: ${sessionId}${isAiMode ? ` | Claude: ${claudeId}` : ''} | Message: ${effectiveCommand} | Images: ${images?.length ?? 0}`,
+ `[Web Command] Mode: ${mode} | Session: ${sessionId}${isAiMode ? ` | Claude: ${claudeId}` : ''} | MessageLength: ${effectiveCommand.length} | Images: ${images?.length ?? 0}`,
LOG_CONTEXT
);📝 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
| // Log all web interface commands prominently | |
| logger.info( | |
| `[Web Command] Mode: ${mode} | Session: ${sessionId}${isAiMode ? ` | Claude: ${claudeId}` : ''} | Message: ${effectiveCommand} | Images: ${images?.length ?? 0}`, | |
| LOG_CONTEXT | |
| ); | |
| // Log all web interface commands prominently | |
| logger.info( | |
| `[Web Command] Mode: ${mode} | Session: ${sessionId}${isAiMode ? ` | Claude: ${claudeId}` : ''} | MessageLength: ${effectiveCommand.length} | Images: ${images?.length ?? 0}`, | |
| LOG_CONTEXT | |
| ); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 88-91: Avoid logging sensitive data
Context: logger.info(
[Web Command] Mode: ${mode} | Session: ${sessionId}${isAiMode ? | Claude: ${claudeId} : ''} | Message: ${effectiveCommand} | Images: ${images?.length ?? 0},
LOG_CONTEXT
)
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers/commands.ts` around lines 88 -
92, Update the prominent logging call in the web command handler to avoid
logging the full effectiveCommand; use the existing 50-character truncation
approach or log only safe metadata such as command length, while preserving the
mode, session, image-count, and other non-sensitive context.
Source: Linters/SAST tools
Comment on lines
+36
to
+48
| ctx.callbacks | ||
| .mergeContext(sourceSessionId, targetSessionId) | ||
| .then((success) => { | ||
| ctx.send(client, { | ||
| type: 'merge_context_result', | ||
| success, | ||
| requestId: message.requestId, | ||
| timestamp: Date.now(), | ||
| }); | ||
| }) | ||
| .catch((error) => { | ||
| ctx.sendError(client, `Failed to merge context: ${error.message}`); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Inconsistent promise-rejection handling across the extracted handlers. Most .catch() blocks take an implicitly any error and interpolate error.message, which yields undefined for non-Error rejections and never reaches Sentry, while git.ts (get_git_branches, list_worktrees) and queue.ts in this same PR correctly use ctx.reportHandlerError/captureException. Standardize on (error: unknown) + ctx.reportHandlerError.
src/main/web-server/handlers/messageHandlers/contextOps.ts#L36-L48: route themergeContextrejection (and the siblingtransferContext/summarizeContextcatches) throughctx.reportHandlerError.src/main/web-server/handlers/messageHandlers/groups.ts#L73-L75: same forcreateGroup, plus therenameGroup,deleteGroup, andmoveSessionToGroupcatches.src/main/web-server/handlers/messageHandlers/groupChat.ts#L32-L34: same forgetGroupChatsand the four other group-chat catches.src/main/web-server/handlers/messageHandlers/git.ts#L40-L42: alignget_git_statusandget_git_diffwith thereportHandlerErrorusage already present in the lower half of the file.src/main/web-server/handlers/messageHandlers/settings.ts#L91-L93: same for thesetSettingrejection.src/main/web-server/handlers/messageHandlers/stats.ts#L45-L47: same forgetUsageDashboard,getAchievements, andgenerateDirectorNotesSynopsis.
As per coding guidelines: "Do not silently swallow unexpected exceptions. Handle known recoverable errors explicitly, rethrow unexpected errors so Sentry can capture them".
📍 Affects 6 files
src/main/web-server/handlers/messageHandlers/contextOps.ts#L36-L48(this comment)src/main/web-server/handlers/messageHandlers/groups.ts#L73-L75src/main/web-server/handlers/messageHandlers/groupChat.ts#L32-L34src/main/web-server/handlers/messageHandlers/git.ts#L40-L42src/main/web-server/handlers/messageHandlers/settings.ts#L91-L93src/main/web-server/handlers/messageHandlers/stats.ts#L45-L47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers/contextOps.ts` around lines 36 -
48, Standardize promise rejection handling across the listed handlers by
changing every affected catch callback to accept error as unknown and route it
through ctx.reportHandlerError instead of interpolating error.message. Update
mergeContext, transferContext, summarizeContext, createGroup, renameGroup,
deleteGroup, moveSessionToGroup, getGroupChats and the other group-chat
handlers, get_git_status, get_git_diff, setSetting, getUsageDashboard,
getAchievements, and generateDirectorNotesSynopsis; preserve each handler’s
existing response behavior while ensuring unexpected exceptions reach Sentry.
Affected sites:
src/main/web-server/handlers/messageHandlers/contextOps.ts:36-48,
groups.ts:73-75, groupChat.ts:32-34, git.ts:40-42, settings.ts:91-93, and
stats.ts:45-47.
Source: Coding guidelines
Comment on lines
+93
to
+94
| const sessionId = message.sessionId as string | undefined; | ||
| const limit = (message.limit as number) ?? 50; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
limit is neither type-checked nor bounded. ?? 50 only guards null/undefined, so a string or an arbitrarily large number reaches getCueActivity and can force an unbounded read.
🛡️ Proposed fix
- const limit = (message.limit as number) ?? 50;
+ const rawLimit = message.limit;
+ const limit =
+ typeof rawLimit === 'number' && Number.isInteger(rawLimit) && rawLimit > 0
+ ? Math.min(rawLimit, 500)
+ : 50;📝 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
| const sessionId = message.sessionId as string | undefined; | |
| const limit = (message.limit as number) ?? 50; | |
| const sessionId = message.sessionId as string | undefined; | |
| const rawLimit = message.limit; | |
| const limit = | |
| typeof rawLimit === 'number' && Number.isInteger(rawLimit) && rawLimit > 0 | |
| ? Math.min(rawLimit, 500) | |
| : 50; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers/cue.ts` around lines 93 - 94,
Validate and bound the limit value in the message handler before passing it to
getCueActivity: accept only a finite number within the supported maximum, and
fall back to 50 for invalid, nullish, non-numeric, or oversized values. Keep the
existing sessionId handling and ensure getCueActivity never receives an
unbounded limit.
Comment on lines
+20
to
+39
| export function handleGetSessions(ctx: MessageHandlerContext, client: WebClient): void { | ||
| if ( | ||
| ctx.callbacks.getSessions && | ||
| ctx.callbacks.getLiveSessionInfo && | ||
| ctx.callbacks.isSessionLive | ||
| ) { | ||
| const allSessions = ctx.callbacks.getSessions(); | ||
| // Enrich sessions with live info if available | ||
| const sessionsWithLiveInfo = allSessions.map((s) => { | ||
| const liveInfo = ctx.callbacks.getLiveSessionInfo!(s.id); | ||
| return { | ||
| ...s, | ||
| agentSessionId: liveInfo?.agentSessionId || s.agentSessionId, | ||
| liveEnabledAt: liveInfo?.enabledAt, | ||
| isLive: ctx.callbacks.isSessionLive!(s.id), | ||
| }; | ||
| }); | ||
| ctx.send(client, { type: 'sessions_list', sessions: sessionsWithLiveInfo }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Silent no-op when any of the three callbacks is unset. Every other handler in this module answers with an error frame; here the client gets nothing and waits for sessions_list indefinitely. Consider sending an error (or an empty list) in the else branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers/sessions.ts` around lines 20 -
39, Update handleGetSessions so it sends a response when any required callback
is unavailable instead of silently returning. Add an else branch that uses the
existing ctx.send mechanism to return the module’s standard error frame or an
empty sessions_list, while preserving the current enrichment behavior when all
callbacks are present.
Comment on lines
+90
to
+120
| export function handleStatsQuery( | ||
| ctx: MessageHandlerContext, | ||
| client: WebClient, | ||
| message: WebClientMessage | ||
| ): void { | ||
| const sql = message.sql as string | undefined; | ||
| const params = Array.isArray(message.params) ? (message.params as unknown[]) : []; | ||
|
|
||
| if (!sql || typeof sql !== 'string') { | ||
| ctx.sendError(client, 'Missing required "sql" string for stats_query'); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const result = runReadonlyStatsQuery(sql, params); | ||
| ctx.send(client, { | ||
| type: 'stats_query_result', | ||
| columns: result.columns, | ||
| rows: result.rows, | ||
| rowCount: result.rowCount, | ||
| truncated: result.truncated, | ||
| requestId: message.requestId, | ||
| timestamp: Date.now(), | ||
| }); | ||
| } catch (error) { | ||
| ctx.sendError( | ||
| client, | ||
| `Stats query failed: ${error instanceof Error ? error.message : String(error)}` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'readonly-query' src | xargs -r cat -nRepository: RunMaestro/Maestro
Length of output: 10045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package better-sqlite3 / sqlite3 versions =="
if [ -f package-lock.json ]; then
node - <<'JS'
const lock = require('./package-lock.json');
for (const name of ['better-sqlite3', 'sqlite3']) {
const v = lock.packages && lock.packages['node_modules/'+name] && lock.packages['node_modules/'+name].version;
const p = lock.dependencies && lock.dependencies[name] && lock.dependencies[name].version;
console.log(name, v || p || 'not found');
}
JS
elif [ -f pnpm-lock.yaml ]; then
rg -n "better-sqlite3|sqlite3:" pnpm-lock.yaml | head -40 || true
else
rg -n "better-sqlite3|sqlite3" package.json package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null | head -80 || true
fi
echo
echo "== relevant stats handler/source outline =="
fd -t f 'stats\.ts$|readonly-query\.ts$' src/main/web-server src/main/stats | while read -r f; do
echo "--- $f"
wc -l "$f"
done
echo
echo "== web-server handler imports/usages =="
fd -t f 'messageHandlers|stats\.ts$' src/main/web-server | while read -r f; do
echo "--- $f"
rg -n "handleStatsQuery|runReadonlyStatsQuery|stats_query|stats_query_result|MessageHandlerContext|WebClientMessage|WebClient" "$f" -C 3 || true
done
echo
echo "== stats module usages =="
rg -n "runReadonlyStatsQuery|MAX_ROWS|stats_query" src/main --glob '*.ts' --glob '!src/main/web-server/tests/**' -C 3Repository: RunMaestro/Maestro
Length of output: 9769
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect better-sqlite3 installed metadata if present =="
node - <<'JS'
const fs = require('fs');
for (const path of [
'node_modules/better-sqlite3/package.json',
'node_modules/sqlite3/package.json'
]) {
try {
const p = JSON.parse(fs.readFileSync(path, 'utf8'));
console.log(path);
console.log(` version=${p.version}`);
console.log(` repository=${JSON.stringify(p.repository)}`);
} catch (e) {
console.log(path, 'not loaded');
}
}
JS
echo
echo "== runtime semantics probe: synchronous JS vs async-await placeholder =="
node - <<'JS'
function blockingQuery() {
const start = Date.now();
while (Date.now() - start < 30) {}
return { rows: [1], rowCount: 1000000, truncated: true };
}
async function handleStatsQueryLike() {
const r = blockingQuery();
return r;
}
async function handleStatsQueryLikeWrappedTryCatch() {
try {
const r = blockingQuery();
return r;
} catch {}
}
async function handleStatsQueryLikeAsyncAbort() {
try {
await new Promise((resolve) => setTimeout(() => resolve(), 30));
return 1;
} catch {}
}
Promise.all([handleStatsQueryLike(), handleStatsQueryLikeWrappedTryCatch(), handleStatsQueryLikeAsyncAbort()]).then(() => {
console.log('done');
});
JS
echo
echo "== sqlite3 / better-sqlite3 sync behavior references from node_modules docs, if present =="
if [ -d node_modules/sqlite3 ]; then
fd -e md -t f . node_modules/sqlite3 | while read -r f; do
rg -n "synchronously|query\(|exec\(|each\(|run\(" "$f" | head -20 || true
done | head -100
fi
if [ -d node_modules/better-sqlite3 ]; then
fd -e md -t f . node_modules/better-sqlite3 | while read -r f; do
rg -n "synchronously|database locking|execute|prepare|Database|all\(" "$f" | head -40 || true
done | head -120
fiRepository: RunMaestro/Maestro
Length of output: 917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== handler context types =="
fd -t f 'types\.ts$' src/main/web-server/handlers | xargs -r cat -n
echo
echo "== message handler routing around stats =="
sed -n '480,545p' src/main/web-server/handlers/messageHandlers/WebSocketMessageHandler.ts | cat -n
echo
echo "== any existing query complexity/timeout guards =="
rg -n "timeout|abort|complexity|sql-timeout|row-limit|MAX_ROWS|sqlite3\.db_limit|SQLITE_LIMIT|SQLITE_TESTCTRL|handleStatsQuery|runReadonlyStatsQuery" src --glob '*.ts' --glob '!src/main/stats/tests/**' -C 2
echo
echo "== better-sqlite3 docs/package evidence =="
if [ -f node_modules/better-sqlite3/README.md ]; then
rg -n "synchronously|blocking|SQLITE_TIMEOUT|Database#timeout|prepare|all\\(|get\\(|run\\(" node_modules/better-sqlite3/README.md -C 2 | head -120
else
echo "README missing"
node - <<'JS'
const fs = require('fs');
console.log('files:', fs.readdirSync('node_modules/better-sqlite3').slice(0,50));
JS
fiRepository: RunMaestro/Maestro
Length of output: 50375
Guard stats_query execution before the synchronous SQLite call.
runReadonlyStatsQuery() only enforces read-only access and caps returned rows; it still uses synchronous stmt.all(...) via better-sqlite3, so an expensive web-client SQL query can block the Electron main process. Add a statement timeout / interrupt and fail clients on timeout before sending stats_query_result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers/stats.ts` around lines 90 - 120,
Update handleStatsQuery and the runReadonlyStatsQuery execution path to enforce
a statement timeout for synchronous better-sqlite3 calls, using the statement
interrupt mechanism to stop queries that exceed the configured limit. Catch
timeout failures as errors and send them through ctx.sendError without sending
stats_query_result, while preserving the existing read-only checks, row cap, and
successful response behavior.
Comment on lines
+223
to
+234
| const sessionId = message.sessionId as string; | ||
| const fromIndex = message.fromIndex as number; | ||
| const toIndex = message.toIndex as number; | ||
| logger.info( | ||
| `[Web] Received reorder_tab message: session=${sessionId}, from=${fromIndex}, to=${toIndex}`, | ||
| LOG_CONTEXT | ||
| ); | ||
|
|
||
| if (!sessionId || fromIndex == null || toIndex == null) { | ||
| ctx.sendError(client, 'Missing sessionId, fromIndex, or toIndex'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
fromIndex/toIndex are not type-validated. Only a == null check is applied, so a string or NaN from the client reaches reorderTab and can silently mis-order or throw downstream. Other handlers in this file reject malformed optional fields explicitly (see lines 485-496).
🛡️ Proposed fix
- if (!sessionId || fromIndex == null || toIndex == null) {
- ctx.sendError(client, 'Missing sessionId, fromIndex, or toIndex');
+ if (
+ !sessionId ||
+ !Number.isInteger(fromIndex) ||
+ !Number.isInteger(toIndex) ||
+ fromIndex < 0 ||
+ toIndex < 0
+ ) {
+ ctx.sendError(client, 'Missing or invalid sessionId, fromIndex, or toIndex');
return;
}📝 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
| const sessionId = message.sessionId as string; | |
| const fromIndex = message.fromIndex as number; | |
| const toIndex = message.toIndex as number; | |
| logger.info( | |
| `[Web] Received reorder_tab message: session=${sessionId}, from=${fromIndex}, to=${toIndex}`, | |
| LOG_CONTEXT | |
| ); | |
| if (!sessionId || fromIndex == null || toIndex == null) { | |
| ctx.sendError(client, 'Missing sessionId, fromIndex, or toIndex'); | |
| return; | |
| } | |
| const sessionId = message.sessionId as string; | |
| const fromIndex = message.fromIndex as number; | |
| const toIndex = message.toIndex as number; | |
| logger.info( | |
| `[Web] Received reorder_tab message: session=${sessionId}, from=${fromIndex}, to=${toIndex}`, | |
| LOG_CONTEXT | |
| ); | |
| if ( | |
| !sessionId || | |
| !Number.isInteger(fromIndex) || | |
| !Number.isInteger(toIndex) || | |
| fromIndex < 0 || | |
| toIndex < 0 | |
| ) { | |
| ctx.sendError(client, 'Missing or invalid sessionId, fromIndex, or toIndex'); | |
| return; | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 225-228: Avoid logging sensitive data
Context: logger.info(
[Web] Received reorder_tab message: session=${sessionId}, from=${fromIndex}, to=${toIndex},
LOG_CONTEXT
)
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers/tabs.ts` around lines 223 - 234,
Validate fromIndex and toIndex in the reorder_tab handler before calling
reorderTab, rejecting values that are not finite numbers in addition to missing
values. Preserve the existing missing-field error response and ensure malformed
client input cannot reach reorderTab; use the existing handler validation
pattern elsewhere in tabs.ts.
Comment on lines
+19
to
+44
| if (!sessionId || typeof data !== 'string') { | ||
| ctx.send(client, { | ||
| type: 'terminal_write_result', | ||
| success: false, | ||
| error: 'Missing sessionId or data', | ||
| }); | ||
| return; | ||
| } | ||
| if (client.subscribedSessionId !== sessionId) { | ||
| ctx.send(client, { | ||
| type: 'terminal_write_result', | ||
| success: false, | ||
| error: 'Not subscribed to this session', | ||
| }); | ||
| return; | ||
| } | ||
| if (!ctx.callbacks.writeToTerminal) { | ||
| ctx.send(client, { | ||
| type: 'terminal_write_result', | ||
| success: false, | ||
| error: 'writeToTerminal not available', | ||
| }); | ||
| return; | ||
| } | ||
| const success = ctx.callbacks.writeToTerminal(sessionId, data); | ||
| ctx.send(client, { type: 'terminal_write_result', success, sessionId }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Error and success frames drop requestId. Every other handler echoes message.requestId so clients can correlate responses; here neither the failure nor the success frame includes it, so a request/response client times out even though the write/resize succeeded.
🔧 Proposed fix (apply to all frames in both handlers)
- const success = ctx.callbacks.writeToTerminal(sessionId, data);
- ctx.send(client, { type: 'terminal_write_result', success, sessionId });
+ const success = ctx.callbacks.writeToTerminal(sessionId, data);
+ ctx.send(client, {
+ type: 'terminal_write_result',
+ success,
+ sessionId,
+ requestId: message.requestId,
+ });Also applies to: 58-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers/terminal.ts` around lines 19 -
44, Update both terminal handlers around the validation failures and successful
write/resize responses to include the incoming message.requestId on every
terminal_write_result and terminal_resize_result frame. Preserve the existing
success, error, and sessionId fields while ensuring all early-return and success
paths echo the request identifier for client correlation.
Comment on lines
+664
to
+680
| /** | ||
| * Handle get_sessions message - request updated sessions list | ||
| */ | ||
| /** | ||
| * Handle get_app_info message - report the running desktop app's version and the | ||
| * git commit hash it was built from (for `maestro-cli version`). commitHash is '' | ||
| * when the build couldn't determine it (see utils/build-info.ts). | ||
| */ | ||
| private handleGetAppInfo(client: WebClient, message: WebClientMessage): void { | ||
| this.send(client, { | ||
| type: 'app_info', | ||
| requestId: message.requestId, | ||
| version: app.getVersion(), | ||
| commitHash: getCommitHash(), | ||
| platform: process.platform, | ||
| }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale doc comment left over from extraction.
The comment "Handle get_sessions message - request updated sessions list" (664-667) no longer describes any code here - handleGetSessions was moved to sessions.ts. It's now dangling directly above the unrelated handleGetAppInfo doc block, which is confusing for future readers.
As per path instructions, "After refactoring, identify unreachable or newly unused code and ask for approval before removing it."
🧹 Proposed fix
- /**
- * Handle get_sessions message - request updated sessions list
- */
/**
* Handle get_app_info message - report the running desktop app's version and the
* git commit hash it was built from (for `maestro-cli version`). commitHash is ''
* when the build couldn't determine it (see utils/build-info.ts).
*/
private handleGetAppInfo(client: WebClient, message: WebClientMessage): void {📝 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
| /** | |
| * Handle get_sessions message - request updated sessions list | |
| */ | |
| /** | |
| * Handle get_app_info message - report the running desktop app's version and the | |
| * git commit hash it was built from (for `maestro-cli version`). commitHash is '' | |
| * when the build couldn't determine it (see utils/build-info.ts). | |
| */ | |
| private handleGetAppInfo(client: WebClient, message: WebClientMessage): void { | |
| this.send(client, { | |
| type: 'app_info', | |
| requestId: message.requestId, | |
| version: app.getVersion(), | |
| commitHash: getCommitHash(), | |
| platform: process.platform, | |
| }); | |
| } | |
| /** | |
| * Handle get_app_info message - report the running desktop app's version and the | |
| * git commit hash it was built from (for `maestro-cli version`). commitHash is '' | |
| * when the build couldn't determine it (see utils/build-info.ts). | |
| */ | |
| private handleGetAppInfo(client: WebClient, message: WebClientMessage): void { | |
| this.send(client, { | |
| type: 'app_info', | |
| requestId: message.requestId, | |
| version: app.getVersion(), | |
| commitHash: getCommitHash(), | |
| platform: process.platform, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers/WebSocketMessageHandler.ts`
around lines 664 - 680, Remove the stale “Handle get_sessions message” doc
comment above handleGetAppInfo, leaving the handleGetAppInfo documentation
directly associated with that method. Do not remove or alter handleGetSessions
or other session-related code.
Source: Path instructions
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 completes the refactoring of
WebSocketMessageHandler.tsby extracting all remaining 19 domain handlers (85 handlers total) into dedicated files undermessageHandlers/.Key Changes
MessageHandlerContextpattern instead ofthis.WebSocketMessageHandler.tsdown from 6000+ to 600 lines, leaving strictly core routing logic, callbacks, and error reporting on the class.tsc(3 configs), ESLint, and Prettier.Summary by CodeRabbit