From 8ef36b3184dfd0d52f5ba9243507eeeb1540f3ad Mon Sep 17 00:00:00 2001 From: David Schovanec Date: Wed, 18 Feb 2026 05:23:39 +0100 Subject: [PATCH] feat: Phase 4 usability and interface improvements 14 improvements across CLI, MCP, and daemon targeting better UX, correctness, and developer ergonomics per consolidated code review plan. - Show partial output on timeout instead of discarding (exec, read MCP) - Add cursor+snapshot/follow mutual exclusion validation (CLI + MCP) - Include cursor positions in info response - Snapshot no longer updates global read position (peek semantics) - Idempotent create with --if-not-exists / if_not_exists parameter - Stable list ordering by creation time - Better daemon startup error messages with socket path and hints - MCP settle_ms: 0 now means "don't wait" (use *int to distinguish) - Improved kill command help explaining destructive behavior - SocketPath() now returns error instead of silently failing - MemoryStorage.LoadMeta deep copies Cursors map and StoppedAt pointer - Search display respects --strip-ansi flag (was always stripping) - Remove redundant flock from FileStorage (Go mutex is sufficient) - Update stale docs: remove --raw references, update TUI guidance - Add quick-start examples to root --help --- .claude/skills/shelli-auto-detector/SKILL.md | 4 +- README.md | 10 +-- cmd/create.go | 29 ++++--- cmd/exec.go | 6 +- cmd/info.go | 6 ++ cmd/kill.go | 9 +- cmd/read.go | 12 ++- cmd/root.go | 9 +- cmd/search.go | 18 +++- internal/daemon/client.go | 81 +++++++++++------- internal/daemon/server.go | 37 ++++++-- internal/daemon/storage_file.go | 6 -- internal/daemon/storage_memory.go | 10 +++ internal/mcp/tools.go | 88 +++++++++++++++----- 14 files changed, 228 insertions(+), 97 deletions(-) diff --git a/.claude/skills/shelli-auto-detector/SKILL.md b/.claude/skills/shelli-auto-detector/SKILL.md index deb712c..8eb022b 100644 --- a/.claude/skills/shelli-auto-detector/SKILL.md +++ b/.claude/skills/shelli-auto-detector/SKILL.md @@ -209,8 +209,8 @@ shelli read openclaw --strip-ansi | `npm test` | Bash | Exits with status | | `npm run dev` + "watch for errors" | shelli | Long-running | | `openclaw tui` | shelli | TUI with two-step submit | -| `vim file.txt` | Bash (not shelli) | Full-screen TUI, use `sed`/`Edit` | -| `htop` | Bash (not shelli) | Full-screen TUI, use `ps aux` | +| `vim file.txt` | shelli (--tui) | Full-screen TUI, use TUI mode with snapshot | +| `htop` | shelli (--tui) | Full-screen TUI, use TUI mode with snapshot | ## Proactive Suggestions diff --git a/README.md b/README.md index 2342729..203240c 100644 --- a/README.md +++ b/README.md @@ -398,7 +398,7 @@ shelli daemon --stopped-ttl 1h ## Escape Sequences -When using `send --raw`, escape sequences are interpreted: +When using `send`, escape sequences are always interpreted: | Sequence | Character | Description | |----------|-----------|-------------| @@ -425,16 +425,16 @@ When using `send --raw`, escape sequences are interpreted: ```bash # Interrupt a long-running command -shelli send myshell "\x03" --raw +shelli send myshell "\x03" # Send EOF to close stdin -shelli send myshell "\x04" --raw +shelli send myshell "\x04" # Tab completion -shelli send myshell "doc\t" --raw +shelli send myshell "doc\t" # Answer a yes/no prompt without newline, then send newline -shelli send myshell "y" --raw +shelli send myshell "y" shelli send myshell "" # just newline ``` diff --git a/cmd/create.go b/cmd/create.go index 247df23..0f96571 100644 --- a/cmd/create.go +++ b/cmd/create.go @@ -16,13 +16,14 @@ var createCmd = &cobra.Command{ } var ( - createCmdFlag string - createJsonFlag bool - createEnvFlag []string - createCwdFlag string - createColsFlag int - createRowsFlag int - createTUIFlag bool + createCmdFlag string + createJsonFlag bool + createEnvFlag []string + createCwdFlag string + createColsFlag int + createRowsFlag int + createTUIFlag bool + createIfNotExistsFlag bool ) func init() { @@ -33,6 +34,7 @@ func init() { createCmd.Flags().IntVar(&createColsFlag, "cols", 80, "Terminal columns") createCmd.Flags().IntVar(&createRowsFlag, "rows", 24, "Terminal rows") createCmd.Flags().BoolVar(&createTUIFlag, "tui", false, "Enable TUI mode (auto-truncate buffer on frame boundaries)") + createCmd.Flags().BoolVar(&createIfNotExistsFlag, "if-not-exists", false, "Return existing session if already running instead of error") } func runCreate(cmd *cobra.Command, args []string) error { @@ -44,12 +46,13 @@ func runCreate(cmd *cobra.Command, args []string) error { } data, err := client.Create(name, daemon.CreateOptions{ - Command: createCmdFlag, - Env: createEnvFlag, - Cwd: createCwdFlag, - Cols: createColsFlag, - Rows: createRowsFlag, - TUIMode: createTUIFlag, + Command: createCmdFlag, + Env: createEnvFlag, + Cwd: createCwdFlag, + Cols: createColsFlag, + Rows: createRowsFlag, + TUIMode: createTUIFlag, + IfNotExists: createIfNotExistsFlag, }) if err != nil { return err diff --git a/cmd/exec.go b/cmd/exec.go index 0c74f20..5f3d4cf 100644 --- a/cmd/exec.go +++ b/cmd/exec.go @@ -3,6 +3,7 @@ package cmd import ( "encoding/json" "fmt" + "os" "strings" "github.com/schovi/shelli/internal/ansi" @@ -74,7 +75,10 @@ func runExec(cmd *cobra.Command, args []string) error { TimeoutSec: execTimeoutFlag, }) if err != nil { - return err + if result == nil || result.Output == "" { + return err + } + fmt.Fprintf(os.Stderr, "Warning: %v\n", err) } output := result.Output diff --git a/cmd/info.go b/cmd/info.go index 45863e4..2bff2cc 100644 --- a/cmd/info.go +++ b/cmd/info.go @@ -54,6 +54,12 @@ func runInfo(cmd *cobra.Command, args []string) error { fmt.Printf("Buffer: %d bytes\n", info.BytesBuffered) fmt.Printf("ReadPos: %d\n", info.ReadPosition) fmt.Printf("Size: %dx%d\n", info.Cols, info.Rows) + if len(info.Cursors) > 0 { + fmt.Printf("Cursors:\n") + for name, pos := range info.Cursors { + fmt.Printf(" %s: %d\n", name, pos) + } + } } return nil } diff --git a/cmd/kill.go b/cmd/kill.go index d8070ed..efa33a9 100644 --- a/cmd/kill.go +++ b/cmd/kill.go @@ -17,8 +17,13 @@ func init() { var killCmd = &cobra.Command{ Use: "kill ", Short: "Kill a session", - Args: cobra.ExactArgs(1), - RunE: runKill, + Long: `Kill a session: terminates the process (if running) and permanently deletes all stored output. + +To stop a session but keep output accessible for later reading, use 'stop' instead. + +This is a destructive operation and cannot be undone.`, + Args: cobra.ExactArgs(1), + RunE: runKill, } func runKill(cmd *cobra.Command, args []string) error { diff --git a/cmd/read.go b/cmd/read.go index 1806152..d765f28 100644 --- a/cmd/read.go +++ b/cmd/read.go @@ -89,6 +89,10 @@ func runRead(cmd *cobra.Command, args []string) error { return fmt.Errorf("--wait and --settle are mutually exclusive") } + if readCursorFlag != "" && (readSnapshotFlag || readFollowFlag) { + return fmt.Errorf("--cursor cannot be combined with --snapshot or --follow") + } + if readSnapshotFlag { if readFollowFlag || readAllFlag || hasWait { return fmt.Errorf("--snapshot cannot be combined with --follow, --all, or --wait") @@ -136,9 +140,13 @@ func runRead(cmd *cobra.Command, args []string) error { output = daemon.LimitLines(output, headLines, tailLines) } if readCursorFlag != "" { - client.ReadWithCursor(name, "new", readCursorFlag, 0, 0) + if _, _, advErr := client.ReadWithCursor(name, "new", readCursorFlag, 0, 0); advErr != nil { + return fmt.Errorf("advance cursor: %w", advErr) + } } else { - client.Read(name, "new", 0, 0) + if _, _, advErr := client.Read(name, "new", 0, 0); advErr != nil { + return fmt.Errorf("advance read position: %w", advErr) + } } } } else { diff --git a/cmd/root.go b/cmd/root.go index 42ea9fe..66378bd 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -9,7 +9,14 @@ import ( var rootCmd = &cobra.Command{ Use: "shelli", Short: "Shell Interactive - session manager for AI agents", - Long: `shelli (Shell Interactive) enables AI agents to interact with persistent interactive shell sessions (REPLs, SSH, database CLIs, etc.)`, + Long: `shelli (Shell Interactive) enables AI agents to interact with persistent interactive shell sessions (REPLs, SSH, database CLIs, etc.) + +Quick start: + shelli create myshell # Start a shell session + shelli exec myshell "echo hello" # Run command and get output + shelli read myshell # Read new output + shelli stop myshell # Stop (keeps output) + shelli kill myshell # Kill (deletes everything)`, } func Execute() { diff --git a/cmd/search.go b/cmd/search.go index d092501..9b40879 100644 --- a/cmd/search.go +++ b/cmd/search.go @@ -95,13 +95,25 @@ func runSearch(cmd *cobra.Command, args []string) error { startLine := match.LineNumber - len(match.Before) for j, line := range match.Before { - fmt.Printf("%4d: %s\n", startLine+j, ansi.Strip(line)) + display := line + if searchStripAnsiFlag { + display = ansi.Strip(line) + } + fmt.Printf("%4d: %s\n", startLine+j, display) } - fmt.Printf(">%3d: %s\n", match.LineNumber, ansi.Strip(match.Line)) + display := match.Line + if searchStripAnsiFlag { + display = ansi.Strip(match.Line) + } + fmt.Printf(">%3d: %s\n", match.LineNumber, display) for j, line := range match.After { - fmt.Printf("%4d: %s\n", match.LineNumber+1+j, ansi.Strip(line)) + display := line + if searchStripAnsiFlag { + display = ansi.Strip(line) + } + fmt.Printf("%4d: %s\n", match.LineNumber+1+j, display) } } diff --git a/internal/daemon/client.go b/internal/daemon/client.go index 951894d..e790cfe 100644 --- a/internal/daemon/client.go +++ b/internal/daemon/client.go @@ -50,7 +50,16 @@ func (c *Client) EnsureDaemon() error { } } - return fmt.Errorf("daemon failed to start") + sockPath := "" + if sp, err := SocketPath(); err == nil { + sockPath = sp + } + if sockPath != "" { + if _, err := os.Stat(sockPath); err == nil { + return fmt.Errorf("daemon failed to start within %s. Stale socket found at %s. Try: rm %s && shelli daemon", DaemonStartTimeout, sockPath, sockPath) + } + } + return fmt.Errorf("daemon failed to start within %s. Socket: %s. Try: rm %s && shelli daemon", DaemonStartTimeout, sockPath, sockPath) } func (c *Client) Ping() bool { @@ -59,12 +68,13 @@ func (c *Client) Ping() bool { } type CreateOptions struct { - Command string - Env []string - Cwd string - Cols int - Rows int - TUIMode bool + Command string + Env []string + Cwd string + Cols int + Rows int + TUIMode bool + IfNotExists bool } func (c *Client) Create(name string, opts CreateOptions) (map[string]interface{}, error) { @@ -73,14 +83,15 @@ func (c *Client) Create(name string, opts CreateOptions) (map[string]interface{} } resp, err := c.send(Request{ - Action: "create", - Name: name, - Command: opts.Command, - Env: opts.Env, - Cwd: opts.Cwd, - Cols: opts.Cols, - Rows: opts.Rows, - TUIMode: opts.TUIMode, + Action: "create", + Name: name, + Command: opts.Command, + Env: opts.Env, + Cwd: opts.Cwd, + Cols: opts.Cols, + Rows: opts.Rows, + TUIMode: opts.TUIMode, + IfNotExists: opts.IfNotExists, }) if err != nil { return nil, err @@ -241,17 +252,18 @@ type SearchResponse struct { } type InfoResponse struct { - Name string `json:"name"` - State string `json:"state"` - PID int `json:"pid"` - Command string `json:"command"` - CreatedAt string `json:"created_at"` - StoppedAt string `json:"stopped_at,omitempty"` - BytesBuffered int64 `json:"bytes_buffered"` - ReadPosition int64 `json:"read_position"` - Cols int `json:"cols"` - Rows int `json:"rows"` - Uptime float64 `json:"uptime_seconds,omitempty"` + Name string `json:"name"` + State string `json:"state"` + PID int `json:"pid"` + Command string `json:"command"` + CreatedAt string `json:"created_at"` + StoppedAt string `json:"stopped_at,omitempty"` + BytesBuffered int64 `json:"bytes_buffered"` + ReadPosition int64 `json:"read_position"` + Cols int `json:"cols"` + Rows int `json:"rows"` + Uptime float64 `json:"uptime_seconds,omitempty"` + Cursors map[string]int64 `json:"cursors,omitempty"` } func (c *Client) Clear(name string) error { @@ -385,6 +397,7 @@ type ExecOptions struct { SettleMs int WaitPattern string TimeoutSec int + SettleSet bool } type ExecResult struct { @@ -404,7 +417,7 @@ func (c *Client) Exec(name string, opts ExecOptions) (*ExecResult, error) { } settleMs := opts.SettleMs - if opts.WaitPattern == "" && settleMs == 0 { + if opts.WaitPattern == "" && settleMs == 0 && !opts.SettleSet { settleMs = 500 } @@ -423,17 +436,25 @@ func (c *Client) Exec(name string, opts ExecOptions) (*ExecResult, error) { SizeFunc: func() (int, error) { return c.Size(name) }, }, ) + + result := &ExecResult{Input: opts.Input, Output: output, Position: pos} if err != nil { - return nil, err + return result, err } - return &ExecResult{Input: opts.Input, Output: output, Position: pos}, nil + return result, nil } func (c *Client) send(req Request) (*Response, error) { - sockPath := SocketPath() + var sockPath string if c.customSocketPath != "" { sockPath = c.customSocketPath + } else { + var err error + sockPath, err = SocketPath() + if err != nil { + return nil, err + } } conn, err := net.Dial("unix", sockPath) if err != nil { diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 0094319..4f17e30 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -10,6 +10,7 @@ import ( "path/filepath" "regexp" "runtime/debug" + "sort" "strings" "sync" "syscall" @@ -160,9 +161,12 @@ func (s *Server) recoverSessions() error { return nil } -func SocketPath() string { - homeDir, _ := os.UserHomeDir() - return filepath.Join(homeDir, ".shelli", "shelli.sock") +func SocketPath() (string, error) { + homeDir, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("get home dir: %w", err) + } + return filepath.Join(homeDir, ".shelli", "shelli.sock"), nil } func (s *Server) socketPath() string { @@ -276,9 +280,10 @@ type Request struct { Env []string `json:"env,omitempty"` Cwd string `json:"cwd,omitempty"` TUIMode bool `json:"tui_mode,omitempty"` - Snapshot bool `json:"snapshot,omitempty"` - SettleMs int `json:"settle_ms,omitempty"` - TimeoutSec int `json:"timeout_sec,omitempty"` + Snapshot bool `json:"snapshot,omitempty"` + SettleMs int `json:"settle_ms,omitempty"` + TimeoutSec int `json:"timeout_sec,omitempty"` + IfNotExists bool `json:"if_not_exists,omitempty"` } type Response struct { @@ -354,7 +359,15 @@ func (s *Server) handleCreate(req Request) Response { s.mu.Lock() defer s.mu.Unlock() - if _, exists := s.handles[req.Name]; exists { + if h, exists := s.handles[req.Name]; exists { + if req.IfNotExists && h.state == StateRunning { + return Response{Success: true, Data: map[string]interface{}{ + "name": h.name, + "pid": h.pid, + "command": h.command, + "existed": true, + }} + } return Response{Success: false, Error: fmt.Sprintf("session %q already exists", req.Name)} } @@ -553,6 +566,10 @@ func (s *Server) handleList() Response { result = append(result, info) } + sort.Slice(result, func(i, j int) bool { + return result[i].CreatedAt < result[j].CreatedAt + }) + return Response{Success: true, Data: result} } @@ -780,8 +797,6 @@ func (s *Server) handleSnapshot(req Request) Response { } totalLen := int64(len(output)) - meta.ReadPos = totalLen - storage.SaveMeta(req.Name, meta) return Response{Success: true, Data: map[string]interface{}{ "output": result, @@ -1030,6 +1045,10 @@ func (s *Server) handleInfo(req Request) Response { result["uptime_seconds"] = time.Since(h.createdAt).Seconds() } + if len(meta.Cursors) > 0 { + result["cursors"] = meta.Cursors + } + return Response{Success: true, Data: result} } diff --git a/internal/daemon/storage_file.go b/internal/daemon/storage_file.go index 2da33c0..20d082d 100644 --- a/internal/daemon/storage_file.go +++ b/internal/daemon/storage_file.go @@ -8,7 +8,6 @@ import ( "path/filepath" "strings" "sync" - "syscall" ) type FileStorage struct { @@ -41,11 +40,6 @@ func (s *FileStorage) Append(session string, data []byte) error { } defer f.Close() - if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil { // #nosec G115 -- f.Fd() returns a valid file descriptor, overflow not possible - return fmt.Errorf("lock file: %w", err) - } - defer syscall.Flock(int(f.Fd()), syscall.LOCK_UN) // #nosec G115 - if _, err := f.Write(data); err != nil { return fmt.Errorf("write output: %w", err) } diff --git a/internal/daemon/storage_memory.go b/internal/daemon/storage_memory.go index f41cb36..63a4d2e 100644 --- a/internal/daemon/storage_memory.go +++ b/internal/daemon/storage_memory.go @@ -138,6 +138,16 @@ func (s *MemoryStorage) LoadMeta(session string) (*SessionMeta, error) { return nil, fmt.Errorf("session %q not found", session) } copied := *meta + if meta.Cursors != nil { + copied.Cursors = make(map[string]int64, len(meta.Cursors)) + for k, v := range meta.Cursors { + copied.Cursors[k] = v + } + } + if meta.StoppedAt != nil { + t := *meta.StoppedAt + copied.StoppedAt = &t + } return &copied, nil } diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index c7e5603..444c490 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -61,6 +61,10 @@ var createSchema = map[string]interface{}{ "type": "boolean", "description": "Enable TUI mode for apps like vim, htop. Auto-truncates buffer on frame boundaries to reduce storage.", }, + "if_not_exists": map[string]interface{}{ + "type": "boolean", + "description": "If true, return existing running session instead of error when session already exists.", + }, }, "required": []string{"name"}, } @@ -309,13 +313,14 @@ func (r *ToolRegistry) Call(name string, args json.RawMessage) (*CallToolResult, } type CreateArgs struct { - Name string `json:"name"` - Command string `json:"command"` - Env []string `json:"env"` - Cwd string `json:"cwd"` - Cols int `json:"cols"` - Rows int `json:"rows"` - TUI bool `json:"tui"` + Name string `json:"name"` + Command string `json:"command"` + Env []string `json:"env"` + Cwd string `json:"cwd"` + Cols int `json:"cols"` + Rows int `json:"rows"` + TUI bool `json:"tui"` + IfNotExists bool `json:"if_not_exists"` } func (r *ToolRegistry) callCreate(args json.RawMessage) (*CallToolResult, error) { @@ -325,12 +330,13 @@ func (r *ToolRegistry) callCreate(args json.RawMessage) (*CallToolResult, error) } data, err := r.client.Create(a.Name, daemon.CreateOptions{ - Command: a.Command, - Env: a.Env, - Cwd: a.Cwd, - Cols: a.Cols, - Rows: a.Rows, - TUIMode: a.TUI, + Command: a.Command, + Env: a.Env, + Cwd: a.Cwd, + Cols: a.Cols, + Rows: a.Rows, + TUIMode: a.TUI, + IfNotExists: a.IfNotExists, }) if err != nil { return nil, err @@ -345,7 +351,7 @@ func (r *ToolRegistry) callCreate(args json.RawMessage) (*CallToolResult, error) type ExecArgs struct { Name string `json:"name"` Input string `json:"input"` - SettleMs int `json:"settle_ms"` + SettleMs *int `json:"settle_ms"` WaitPattern string `json:"wait_pattern"` TimeoutSec int `json:"timeout_sec"` StripAnsi bool `json:"strip_ansi"` @@ -357,7 +363,7 @@ func (r *ToolRegistry) callExec(args json.RawMessage) (*CallToolResult, error) { return nil, fmt.Errorf("parse args: %w", err) } - if a.WaitPattern != "" && a.SettleMs > 0 { + if a.WaitPattern != "" && a.SettleMs != nil && *a.SettleMs > 0 { return nil, fmt.Errorf("wait_pattern and settle_ms are mutually exclusive") } @@ -365,14 +371,36 @@ func (r *ToolRegistry) callExec(args json.RawMessage) (*CallToolResult, error) { return nil, fmt.Errorf("input is required") } + settleMs := 0 + if a.SettleMs != nil { + settleMs = *a.SettleMs + } + result, err := r.client.Exec(a.Name, daemon.ExecOptions{ Input: a.Input, - SettleMs: a.SettleMs, + SettleMs: settleMs, WaitPattern: a.WaitPattern, TimeoutSec: a.TimeoutSec, + SettleSet: a.SettleMs != nil, }) if err != nil { - return nil, err + if result == nil || result.Output == "" { + return nil, err + } + output := result.Output + if a.StripAnsi { + output = ansi.Strip(output) + } + data, _ := json.MarshalIndent(map[string]interface{}{ + "input": result.Input, + "output": output, + "position": result.Position, + "warning": err.Error(), + }, "", " ") + return &CallToolResult{ + Content: []ContentBlock{{Type: "text", Text: string(data)}}, + IsError: true, + }, nil } output := result.Output @@ -519,6 +547,10 @@ func (r *ToolRegistry) callRead(args json.RawMessage) (*CallToolResult, error) { return nil, fmt.Errorf("all cannot be combined with wait_pattern or settle_ms") } + if a.Cursor != "" && a.Snapshot { + return nil, fmt.Errorf("cursor and snapshot are mutually exclusive") + } + if a.Snapshot { if a.All { return nil, fmt.Errorf("snapshot and all are mutually exclusive") @@ -572,15 +604,21 @@ func (r *ToolRegistry) callRead(args json.RawMessage) (*CallToolResult, error) { SizeFunc: func() (int, error) { return r.client.Size(a.Name) }, }, ) + + var warning string if err != nil { - return nil, err + if output == "" { + return nil, err + } + warning = err.Error() } - // Advance read cursor past returned content (matches CLI behavior) - if a.Cursor != "" { - r.client.ReadWithCursor(a.Name, "new", a.Cursor, 0, 0) - } else { - r.client.Read(a.Name, "new", 0, 0) + if warning == "" { + if a.Cursor != "" { + r.client.ReadWithCursor(a.Name, "new", a.Cursor, 0, 0) + } else { + r.client.Read(a.Name, "new", 0, 0) + } } if a.Head > 0 || a.Tail > 0 { @@ -595,9 +633,13 @@ func (r *ToolRegistry) callRead(args json.RawMessage) (*CallToolResult, error) { "output": output, "position": pos, } + if warning != "" { + result["warning"] = warning + } data, _ := json.MarshalIndent(result, "", " ") return &CallToolResult{ Content: []ContentBlock{{Type: "text", Text: string(data)}}, + IsError: warning != "", }, nil }