From a0315a558a1ed9be759f12ec1e7ef1f596e15605 Mon Sep 17 00:00:00 2001 From: Levi Neely Date: Mon, 10 Aug 2026 20:49:22 +0200 Subject: [PATCH] refactor: move toolsrv spawn/process management to toolclient Move spawn.go from toolsrv/ to cmd/olliesrv/internal/toolclient/: - Process, ProcessKeeper, Spawn, SpawnRemote now in toolclient package - toolsrv/ now only contains client code (Dial, Conn, etc.) This clarifies the architecture: - toolsrv/ = client SDK for connecting to toolsrv - cmd/toolsrv/ = the toolsrv server - cmd/olliesrv/internal/toolclient/ = olliesrv's toolsrv process management Removed ProcessKeeper tests from toolsrv integration tests since they now belong to toolclient. --- cmd/olliesrv/internal/session/session.go | 5 +- cmd/olliesrv/internal/session/setup.go | 31 +++++------ .../olliesrv/internal/toolclient}/spawn.go | 9 ++-- cmd/toolsrv/integration_test.go | 51 ------------------- 4 files changed, 24 insertions(+), 72 deletions(-) rename {toolsrv => cmd/olliesrv/internal/toolclient}/spawn.go (97%) diff --git a/cmd/olliesrv/internal/session/session.go b/cmd/olliesrv/internal/session/session.go index 30bf35b..c023e03 100644 --- a/cmd/olliesrv/internal/session/session.go +++ b/cmd/olliesrv/internal/session/session.go @@ -10,6 +10,7 @@ import ( "time" "ollie/cmd/olliesrv/internal/agent" + "ollie/cmd/olliesrv/internal/toolclient" olog "ollie/log" "ollie/paths" "ollie/toolsrv" @@ -26,8 +27,8 @@ type Session struct { Cancel context.CancelFunc // Tool server lifecycle (set during setup/teardown, not raced) - Proc *toolsrv.Process - Keeper *toolsrv.ProcessKeeper + Proc *toolclient.Process + Keeper *toolclient.ProcessKeeper Remote string // Agent state diff --git a/cmd/olliesrv/internal/session/setup.go b/cmd/olliesrv/internal/session/setup.go index a10b3b9..b49840f 100644 --- a/cmd/olliesrv/internal/session/setup.go +++ b/cmd/olliesrv/internal/session/setup.go @@ -5,6 +5,7 @@ import ( "fmt" "ollie/cmd/olliesrv/internal/agent" + "ollie/cmd/olliesrv/internal/toolclient" "ollie/paths" "ollie/cmd/olliesrv/internal/prompts" "ollie/toolsrv" @@ -13,8 +14,8 @@ import ( // SessionInfra holds the tool server infrastructure for a session. // Created by SetupToolServer and used by both CreateAgent and restoreSession. type SessionInfra struct { - Proc *toolsrv.Process - Keeper *toolsrv.ProcessKeeper + Proc *toolclient.Process + Keeper *toolclient.ProcessKeeper ToolsConn *toolsrv.Conn NewToolServer func() *toolsrv.Conn Platform string @@ -36,8 +37,8 @@ type ToolServerConfig struct { // InfraConfig describes existing infrastructure to reuse. type InfraConfig struct { - Proc *toolsrv.Process - Keeper *toolsrv.ProcessKeeper + Proc *toolclient.Process + Keeper *toolclient.ProcessKeeper ToolsConn *toolsrv.Conn } @@ -76,15 +77,15 @@ func SetupToolServer(cfg ToolServerConfig) (*SessionInfra, error) { IsGitRepo: paths.IsGitRepo(cfg.CWD), }, nil } - var proc *toolsrv.Process - var keeper *toolsrv.ProcessKeeper + var proc *toolclient.Process + var keeper *toolclient.ProcessKeeper var newToolServer func() *toolsrv.Conn var err error var platform string var isGitRepo bool if cfg.RemoteTarget != "" { - proc, err = toolsrv.SpawnRemote(cfg.Ctx, toolsrv.RemoteConfig{ + proc, err = toolclient.SpawnRemote(cfg.Ctx, toolclient.RemoteConfig{ SSHTarget: cfg.RemoteTarget, CWD: cfg.CWD, SessionID: cfg.SessionID, @@ -93,8 +94,8 @@ func SetupToolServer(cfg ToolServerConfig) (*SessionInfra, error) { if err != nil { return nil, fmt.Errorf("remote spawn: %w", err) } - keeper = toolsrv.NewProcessKeeper(cfg.Ctx, proc, func(ctx context.Context) (*toolsrv.Process, error) { - return toolsrv.SpawnRemote(ctx, toolsrv.RemoteConfig{ + keeper = toolclient.NewProcessKeeper(cfg.Ctx, proc, func(ctx context.Context) (*toolclient.Process, error) { + return toolclient.SpawnRemote(ctx, toolclient.RemoteConfig{ SSHTarget: cfg.RemoteTarget, CWD: cfg.CWD, SessionID: cfg.SessionID, @@ -111,19 +112,19 @@ func SetupToolServer(cfg ToolServerConfig) (*SessionInfra, error) { platform = proc.Info.Platform isGitRepo = proc.Info.IsGitRepo } else { - var dialOpts []toolsrv.Option + var dialOpts []toolclient.Option if cfg.Yolo { - dialOpts = append(dialOpts, toolsrv.WithYolo()) + dialOpts = append(dialOpts, toolclient.WithYolo()) } if cfg.SessionID != "" { - dialOpts = append(dialOpts, toolsrv.WithSessionID(cfg.SessionID)) + dialOpts = append(dialOpts, toolclient.WithSessionID(cfg.SessionID)) } - proc, err = toolsrv.Spawn(cfg.Ctx, cfg.CWD, dialOpts...) + proc, err = toolclient.Spawn(cfg.Ctx, cfg.CWD, dialOpts...) if err != nil { return nil, fmt.Errorf("local spawn: %w", err) } - keeper = toolsrv.NewProcessKeeper(cfg.Ctx, proc, func(ctx context.Context) (*toolsrv.Process, error) { - return toolsrv.Spawn(ctx, cfg.CWD, dialOpts...) + keeper = toolclient.NewProcessKeeper(cfg.Ctx, proc, func(ctx context.Context) (*toolclient.Process, error) { + return toolclient.Spawn(ctx, cfg.CWD, dialOpts...) }) newToolServer = func() *toolsrv.Conn { conn, err := keeper.Dial() diff --git a/toolsrv/spawn.go b/cmd/olliesrv/internal/toolclient/spawn.go similarity index 97% rename from toolsrv/spawn.go rename to cmd/olliesrv/internal/toolclient/spawn.go index 9264241..a7754ce 100644 --- a/toolsrv/spawn.go +++ b/cmd/olliesrv/internal/toolclient/spawn.go @@ -1,5 +1,5 @@ // spawn.go - Process spawning and lifecycle management for toolsrv. -package toolsrv +package toolclient import ( "bufio" @@ -18,6 +18,7 @@ import ( "time" "ollie/paths" + toolsrvclient "ollie/toolsrv" ) // Process represents a running toolsrv process. @@ -314,13 +315,13 @@ func (pk *ProcessKeeper) SetContext(ctx context.Context) { } // Dial connects to the managed process, respawning if necessary. -func (pk *ProcessKeeper) Dial() (*Conn, error) { +func (pk *ProcessKeeper) Dial() (*toolsrvclient.Conn, error) { pk.mu.Lock() defer pk.mu.Unlock() // Try to connect to existing process if pk.proc != nil { - conn, err := Dial(pk.proc.SocketPath, pk.proc.Secret) + conn, err := toolsrvclient.Dial(pk.proc.SocketPath, pk.proc.Secret) if err == nil { // Save the secret if this was first connection if pk.proc.Secret == "" { @@ -337,7 +338,7 @@ func (pk *ProcessKeeper) Dial() (*Conn, error) { return nil, fmt.Errorf("respawn: %w", err) } pk.proc = proc - conn, err := Dial(proc.SocketPath, proc.Secret) + conn, err := toolsrvclient.Dial(proc.SocketPath, proc.Secret) if err != nil { return nil, err } diff --git a/cmd/toolsrv/integration_test.go b/cmd/toolsrv/integration_test.go index bb33df0..bd0ce4d 100644 --- a/cmd/toolsrv/integration_test.go +++ b/cmd/toolsrv/integration_test.go @@ -379,41 +379,6 @@ func TestIntegration_ToolExecutionError(t *testing.T) { t.Logf("Error tool result: %+v", toolResult) } -func TestIntegration_ProcessKeeperReconnect(t *testing.T) { - socketPath, cleanup := startTestServer(t) - defer cleanup() - - secret := "test-secret-keeper" - - // Create a process manually (simulating what Spawn returns) - proc := &toolsrv.Process{ - SocketPath: socketPath, - Secret: secret, - } - - // Create keeper without respawn (we're testing reconnect, not respawn) - keeper := toolsrv.NewProcessKeeper(context.Background(), proc, nil) - - // First dial - conn1, err := keeper.Dial() - if err != nil { - t.Fatalf("first Dial failed: %v", err) - } - token := conn1.Token() - conn1.Close() - - // Second dial should reconnect with same secret - conn2, err := keeper.Dial() - if err != nil { - t.Fatalf("second Dial failed: %v", err) - } - defer conn2.Close() - - if conn2.Token() != token { - t.Errorf("token changed after reconnect: %q -> %q", token, conn2.Token()) - } -} - func TestIntegration_ConcurrentConnections(t *testing.T) { socketPath, cleanup := startTestServer(t) defer cleanup() @@ -499,22 +464,6 @@ func TestIntegration_ProcessKeeperRespawn(t *testing.T) { t.Error("old secret should not work on new server") } - // Verify ProcessKeeper handles this correctly - proc := &toolsrv.Process{ - SocketPath: socketPath2, - Secret: secret2, - } - keeper := toolsrv.NewProcessKeeper(context.Background(), proc, nil) - - conn4, err := keeper.Dial() - if err != nil { - t.Fatalf("keeper Dial failed: %v", err) - } - if conn4.Token() != token2 { - t.Error("keeper should reconnect with same token") - } - conn4.Close() - t.Logf("Successfully verified: old_token=%s new_token=%s", token1[:8], token2[:8]) _ = cwd // silence unused warning }