Feat/Refactor: Server Client architecture and API#2455
Conversation
… direct app access
…hind CRUSH_CLIENT_SERVER env var
There was a problem hiding this comment.
Pull request overview
This PR refactors Crush into a client/server architecture with a REST API (served over Unix socket / named pipe by default) and introduces a new Workspace abstraction so the TUI/CLI can operate against either an in-process workspace or a remote server-managed workspace.
Changes:
- Add Swagger/OpenAPI generation (swag annotations + Taskfile task) and expose API docs under
/v1/docs. - Introduce
internal/workspace.Workspaceand an in-process implementation (AppWorkspace) to unify frontend interactions across local and client/server modes. - Adjust UI/CLI/server/client plumbing (version info, SSE events, LSP state surfacing, build metadata).
Reviewed changes
Copilot reviewed 72 out of 75 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Taskfile.yaml | Adds swag task to generate OpenAPI specs from annotations. |
| main.go | Adds Swagger annotations for API metadata/base path. |
| internal/workspace/workspace.go | Introduces the frontend-facing Workspace interface and shared types. |
| internal/workspace/app_workspace.go | Implements Workspace by delegating to in-process app.App. |
| internal/version/version.go | Adds build metadata (Commit) alongside Version. |
| internal/ui/util/util.go | Removes logging side-effect from ReportError. |
| internal/ui/model/ui.go | Switches UI to use com.Workspace instead of com.App for multiple operations. |
| internal/ui/model/header.go | Updates header LSP diagnostic display to consume Workspace state. |
| internal/cmd/run.go | Updates non-interactive run behavior and client/server connection handling. |
| internal/client/client.go | Adds/adjusts client calls, including VersionInfo. |
| internal/server/server.go | Wires server routes and implements server lifecycle methods. |
| internal/server/events.go | Wraps internal pubsub events into SSE payload envelopes using proto types. |
| internal/proto/agent.go | Defines AgentEvent/AgentEventType used in SSE/API payloads. |
| internal/backend/agent.go | Implements server-side agent operations for workspaces. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ErrAgentNotInitialized | ||
| } | ||
|
|
||
| _, err = ws.AgentCoordinator.Run(ctx, msg.SessionID, msg.Prompt) |
There was a problem hiding this comment.
Backend.SendMessage drops AgentMessage.Attachments when invoking AgentCoordinator.Run, so client/server mode cannot send file/image attachments even though the API accepts them. Pass msg.Attachments through to Run (or explicitly reject attachments with a clear error if not supported).
| _, err = ws.AgentCoordinator.Run(ctx, msg.SessionID, msg.Prompt) | |
| _, err = ws.AgentCoordinator.Run(ctx, msg.SessionID, msg.Prompt, msg.Attachments) |
| // ListenAndServe starts the server and begins accepting connections. | ||
| func (s *Server) ListenAndServe() error { | ||
| if s.ln != nil { | ||
| return fmt.Errorf("server already started") | ||
| } | ||
| ln, err := listen(s.network, s.Addr) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to listen on %s: %w", s.Addr, err) | ||
| } | ||
| return s.Serve(ln) | ||
| } | ||
|
|
||
| func (s *Server) closeListener() { | ||
| if s.ln != nil { | ||
| s.ln.Close() | ||
| s.ln = nil | ||
| } | ||
| } | ||
|
|
||
| // Close force closes all listeners and connections. | ||
| func (s *Server) Close() error { | ||
| defer func() { s.closeListener() }() | ||
| return s.h.Close() | ||
| } | ||
|
|
||
| // Shutdown gracefully shuts down the server without interrupting active | ||
| // connections. | ||
| func (s *Server) Shutdown(ctx context.Context) error { | ||
| defer func() { s.closeListener() }() | ||
| return s.h.Shutdown(ctx) | ||
| } |
There was a problem hiding this comment.
ListenAndServe creates a local listener but never assigns it to s.ln. As a result, the "server already started" guard never triggers and Close()/Shutdown() won't actually close the active listener via closeListener(). Store the listener in s.ln before serving (and clear it after Serve returns).
| // VersionInfo retrieves the server's version information. | ||
| func (c *Client) VersionInfo(ctx context.Context) (*proto.VersionInfo, error) { | ||
| var vi proto.VersionInfo | ||
| rsp, err := c.get(ctx, "version", nil, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer rsp.Body.Close() | ||
| if err := json.NewDecoder(rsp.Body).Decode(&vi); err != nil { | ||
| return nil, err | ||
| } | ||
| return &vi, nil |
There was a problem hiding this comment.
Client.VersionInfo calls c.get(ctx, "version", ...) without a leading slash, unlike other endpoints (e.g., "/health"). This can produce incorrect request paths depending on how sendReq joins paths. Use a consistent absolute path (e.g., "/version").
| // Cancel on SIGINT or SIGTERM. | ||
| ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, os.Kill) | ||
| defer cancel() | ||
|
|
There was a problem hiding this comment.
signal.NotifyContext includes os.Kill, but SIGKILL cannot be trapped/handled, so this entry is ineffective and can be misleading. Replace with syscall.SIGTERM (and keep os.Interrupt) to support graceful shutdown on typical termination signals.
| availDetailWidth := width - leftPadding - rightPadding - lipgloss.Width(b.String()) - minHeaderDiags - diagToDetailsSpacing | ||
| lspErrorCount := 0 | ||
| for _, info := range h.com.Workspace.LSPGetStates() { | ||
| lspErrorCount += info.DiagnosticCount | ||
| } | ||
| details := renderHeaderDetails( | ||
| h.com, | ||
| session, | ||
| h.com.App.LSPManager.Clients(), | ||
| lspErrorCount, | ||
| detailsOpen, |
There was a problem hiding this comment.
header.go sums LSPClientInfo.DiagnosticCount into lspErrorCount, but DiagnosticCount is the total number of diagnostics (all severities), not errors. This will render non-error diagnostics with the error icon/count. Either rename the variable/UI label to reflect "diagnostics", or compute the error-only count via LSPGetDiagnosticCounts(...).Error.
| // AgentEventType represents the type of agent event. | ||
| type AgentEventType string | ||
|
|
||
| const ( | ||
| AgentEventTypeError AgentEventType = "error" | ||
| AgentEventTypeResponse AgentEventType = "response" | ||
| AgentEventTypeSummarize AgentEventType = "summarize" | ||
| ) |
There was a problem hiding this comment.
proto.AgentEventType only defines "error"/"response"/"summarize", but SSE wrapping casts notify.Type values like "agent_finished" into AgentEventType. This makes the exported type constants incomplete/misleading and can break clients that validate against the declared enum. Either add constants for the notification event types you emit, or use a dedicated proto type for notify.Notification events (and/or document that AgentEventType is an open set).
| case pubsub.Event[notify.Notification]: | ||
| return envelope(pubsub.PayloadTypeAgentEvent, pubsub.Event[proto.AgentEvent]{ | ||
| Type: e.Type, | ||
| Payload: proto.AgentEvent{ | ||
| SessionID: e.Payload.SessionID, | ||
| SessionTitle: e.Payload.SessionTitle, | ||
| Type: proto.AgentEventType(e.Payload.Type), | ||
| }, | ||
| }) |
There was a problem hiding this comment.
SSE wrapping of notify.Notification builds proto.AgentEvent without setting the required Message field (no omitempty), so JSON will include an empty/zero "message" object for these notifications. Consider using a dedicated proto payload for agent notifications, or make AgentEvent.Message optional (pointer or omitempty) so notification-only events don't emit misleading fields.
andreynering
left a comment
andreynering
left a comment
There was a problem hiding this comment.
Good for an initial iteraction!
There are a couple bugs, but since it's opt-in and experimental, we can merge to avoid conflicts and continue working on it.
Screenshots
This change is a big and important one. It refactors Crush into a server/client architecture that can be used to interact with the agent loop via API. The server can manage multiple workspace sessions. A workspace is where Crush finds a
.crushdirectory and a database (the data directory).Right now, you can enable this feature using the
CRUSH_CLIENT_SERVER=1flag. Having this flag on will make Crush spin a detached server process, if there isn't any, before displaying the TUI. The server is an REST HTTP server that runs on a Unix socket by default. It can bind to whatever network type. You can control that via the-Hcommand line flag. The default iscrush -H unix:///tmp/crush-$(id -u).sock. To use a TCP connection, you can docrush -H tcp://localhost:3456. There is an OpenAPI endpoint at/v1/docsthat gives a high overview of the API and how it looks like.To run the server independently without a client or TUI, you can use
crush server. That too accepts a-Hflag to bind the server to a specific network address.Fixes: #976