Skip to content

Extract RPC command handlers from AppDelegate#148

Open
dhilgaertner wants to merge 1 commit intomainfrom
feature/crow-138-extract-rpc-handlers
Open

Extract RPC command handlers from AppDelegate#148
dhilgaertner wants to merge 1 commit intomainfrom
feature/crow-138-extract-rpc-handlers

Conversation

@dhilgaertner
Copy link
Copy Markdown
Contributor

Summary

  • Extract 17 inline RPC handler closures from AppDelegate.startSocketServer() into focused files under Sources/Crow/App/RPCHandlers/, grouped by domain
  • Each file exports a factory function returning [String: CommandRouter.Handler] that AppDelegate merges at startup
  • AppDelegate.swift shrinks from 941 to 427 LOC with zero behavior changes

New files

File Handlers
RPCError.swift Error enum (moved from AppDelegate)
SessionHandlers.swift new/rename/select/list/get/set-status/delete session, set-ticket
WorktreeHandlers.swift add/list worktrees
TerminalHandlers.swift new/list/close terminal, send
LinkHandlers.swift add/list links
HookHandlers.swift hook-event

Closes #138

Test plan

  • swift build compiles without errors or new warnings
  • CrowIPC tests pass (32/32)
  • CrowCore tests pass (101/101)
  • Manual: launch app, verify crow list-sessions, crow new-session, crow delete-session work via CLI

🤖 Generated with Claude Code

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>
@dhilgaertner dhilgaertner requested a review from dgershman as a code owner April 9, 2026 05:43
@dhilgaertner dhilgaertner deleted the feature/crow-138-extract-rpc-handlers branch April 16, 2026 02:48
@dhilgaertner dhilgaertner restored the feature/crow-138-extract-rpc-handlers branch April 16, 2026 02:51
@dhilgaertner dhilgaertner reopened this Apr 16, 2026
@dhilgaertner dhilgaertner deleted the feature/crow-138-extract-rpc-handlers branch April 16, 2026 02:53
@dhilgaertner dhilgaertner restored the feature/crow-138-extract-rpc-handlers branch April 16, 2026 04:17
@dhilgaertner dhilgaertner reopened this Apr 16, 2026
Copy link
Copy Markdown
Collaborator

@dgershman dgershman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code & Security Review

Critical Issues

None found.

Security Review

Strengths:

  • Path traversal validation (Validation.isPathWithinRoot) preserved in WorktreeHandlers.swift:20-27 and TerminalHandlers.swift:19-21
  • Session name validation (Validation.isValidSessionName) preserved in SessionHandlers.swift:14-15,30-32
  • Manager session deletion guard preserved in SessionHandlers.swift:105
  • Managed terminal close guard preserved in TerminalHandlers.swift:70-72
  • @Sendable closures correctly use await MainActor.run for all AppState mutations, 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 scan
  • RPCError properly extracted to its own file since it's shared across all handler files
  • The merge strategy { _, n in n } in startSocketServer means later registrations silently win on key collision — this is fine since all handler keys are unique, but worth noting
  • HookHandlers.swift is the largest file (194 LOC) due to the event state machine — reasonable to keep together since the cases are tightly coupled
  • TerminalHandlers.swift:25 references AppDelegate.resolveClaudeInCommand which 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.

Copy link
Copy Markdown
Collaborator

@dgershman dgershman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code & Security Review

Critical Issues

None found.

Security Review

Strengths:

  • All path validation (isPathWithinRoot) preserved in WorktreeHandlers.swift:20-27 and TerminalHandlers.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
  • RPCError enum properly conforms to RPCErrorCoded for 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., hookHandlers doesn't take store since it doesn't persist)
  • AppDelegate.startSocketServer at :374-391 is now a clear merge-and-wire method, easy to audit
  • TerminalHandlers.swift:25 references AppDelegate.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 in AppDelegate.swift:376-380 silently 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 TerminalHandlersAppDelegate 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extract RPC command handlers from AppDelegate

2 participants