Extract RPC command handlers from AppDelegate#148
Open
dhilgaertner wants to merge 1 commit intomainfrom
Open
Conversation
Move 17 inline RPC handler closures out of AppDelegate.startSocketServer() into focused files under RPCHandlers/, grouped by domain: sessions, worktrees, terminals, links, and hook events. Each file exports a factory function returning [String: CommandRouter.Handler] that AppDelegate merges at startup. AppDelegate shrinks from 941 to 427 LOC with no behavior changes. Closes #138 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dgershman
approved these changes
Apr 16, 2026
Collaborator
dgershman
left a comment
There was a problem hiding this comment.
Code & Security Review
Critical Issues
None found.
Security Review
Strengths:
- Path traversal validation (
Validation.isPathWithinRoot) preserved inWorktreeHandlers.swift:20-27andTerminalHandlers.swift:19-21 - Session name validation (
Validation.isValidSessionName) preserved inSessionHandlers.swift:14-15,30-32 - Manager session deletion guard preserved in
SessionHandlers.swift:105 - Managed terminal close guard preserved in
TerminalHandlers.swift:70-72 @Sendableclosures correctly useawait MainActor.runfor allAppStatemutations, maintaining the concurrency safety model
Concerns:
- None — the extracted code is a faithful 1:1 move of the original handlers with no behavioral changes
Code Quality
- Clean decomposition: 17 handlers split into 6 files grouped by domain, each with a factory function returning
[String: CommandRouter.Handler] AppDelegate.startSocketServer()now just merges handler dictionaries — easy to scanRPCErrorproperly extracted to its own file since it's shared across all handler files- The merge strategy
{ _, n in n }instartSocketServermeans later registrations silently win on key collision — this is fine since all handler keys are unique, but worth noting HookHandlers.swiftis the largest file (194 LOC) due to the event state machine — reasonable to keep together since the cases are tightly coupledTerminalHandlers.swift:25referencesAppDelegate.resolveClaudeInCommandwhich crosses back into AppDelegate — acceptable since it's a static utility, though it could be extracted to a standalone function in a future pass
Summary Table
| Priority | Issue |
|---|---|
| Green | Consider extracting resolveClaudeInCommand out of AppDelegate since handler code now depends on it |
| Green | Merge conflict strategy { _, n in n } is safe but could use a comment noting keys are expected to be unique |
Recommendation: Approve — this is a clean, mechanical extraction that cuts AppDelegate from 941 to 427 LOC with no behavioral changes. Security validations are preserved, concurrency model is maintained, and the handler grouping is logical.
dgershman
approved these changes
Apr 20, 2026
Collaborator
dgershman
left a comment
There was a problem hiding this comment.
Code & Security Review
Critical Issues
None found.
Security Review
Strengths:
- All path validation (
isPathWithinRoot) preserved inWorktreeHandlers.swift:20-27andTerminalHandlers.swift:19-21 - Input validation (session name length/control chars) preserved in
SessionHandlers.swift:14-15,30-31 - Manager session deletion guard preserved in
SessionHandlers.swift:105 RPCErrorenum properly conforms toRPCErrorCodedfor structured error codes
Concerns:
- None. The security boundaries are faithfully reproduced from the original inline handlers.
Code Quality
- Clean mechanical extraction — each factory function takes only the dependencies it needs (e.g.,
hookHandlersdoesn't takestoresince it doesn't persist) AppDelegate.startSocketServerat:374-391is now a clear merge-and-wire method, easy to auditTerminalHandlers.swift:25referencesAppDelegate.resolveClaudeInCommand— this couples the handler file back to AppDelegate. Minor, and fine for a pure extraction PR, but worth noting for a future pass.- The
{ _, n in n }merge strategy inAppDelegate.swift:376-380silently drops duplicate keys. This is correct since each handler file owns distinct command names, but a debug assertion on conflicts could catch future mistakes.
Summary Table
| Priority | Issue |
|---|---|
| Green | TerminalHandlers → AppDelegate coupling via resolveClaudeInCommand — consider moving to a shared utility in a follow-up |
| Green | handlers.merge silently drops duplicates — consider a debug-only duplicate-key assertion |
Recommendation: Approve — this is a clean, behavior-preserving refactor that cuts AppDelegate from 941 to 427 LOC. All security checks, MainActor serialization, and persistence logic are faithfully preserved. The two green items are minor and don't block merge.
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
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
AppDelegate.startSocketServer()into focused files underSources/Crow/App/RPCHandlers/, grouped by domain[String: CommandRouter.Handler]that AppDelegate merges at startupAppDelegate.swiftshrinks from 941 to 427 LOC with zero behavior changesNew files
RPCError.swiftSessionHandlers.swiftWorktreeHandlers.swiftTerminalHandlers.swiftLinkHandlers.swiftHookHandlers.swiftCloses #138
Test plan
swift buildcompiles without errors or new warningscrow list-sessions,crow new-session,crow delete-sessionwork via CLI🤖 Generated with Claude Code