refactor: session resumption cleanup and AgentInstance → AgentExecution rename (#23)

* feat: session resumption with ACP session ID support

- Add useSessionResumption hook for automatic session recovery on page reload
- Fix ACPSessionID not being set on AgentInstance in Launch() and createInstance()
- Add task.session.status and task.session.resume WebSocket actions
- Fix shell terminal retry logic for resumed sessions
- Fix taskSessionState reading from Zustand store instead of local state
- Remove dead code: unused stream handling functions in lifecycle/manager.go
- Fix test mock interfaces (CleanupStaleInstanceByTaskID, NewManager signature)
- Add enhanced logging for session resumption debugging
- Simplify page-client.tsx state merging logic

* refactor: remove unused AgentController (dead code)

- Delete agent_controller.go - entirely unused duplicate of Manager methods
- Remove agentController field from Manager struct
- Remove AgentController initialization in NewManager

The AgentController duplicated StartAgentProcess, initializeACPSession, and
PromptAgent from Manager but was never called. All agent control operations
go through Manager's own methods.

* refactor: remove unused MemoryRepository (dead code)

- Delete memory.go - entire MemoryRepository implementation unused
  (replaced by SQLite repository)
- Delete memory_test.go - tests for removed MemoryRepository

Found via 'deadcode' tool analysis.

* refactor: consolidate agentctl packages into server/client structure

Reorganize the confusing dual agentctl packages:
- internal/agentctl/* → internal/agentctl/server/
- internal/agent/agentctl/* → internal/agentctl/client/

Structure is now:
  internal/agentctl/
  ├── client/      - HTTP client for talking to agentctl servers
  │   ├── client.go
  │   ├── standalone.go
  │   ├── acp.go
  │   └── launcher/
  ├── server/      - agentctl server implementation (runs in containers)
  │   ├── api/
  │   ├── adapter/
  │   ├── acp/
  │   ├── config/
  │   ├── instance/
  │   ├── process/
  │   └── shell/
  └── types/       - Shared types used by both client and server

Also consolidated shared types (GitStatusUpdate, FileInfo, etc.) from
process/types.go into the shared types/ package.

* refactor: rename AgentInstance to AgentExecution for clarity

Rename AgentInstance -> AgentExecution across the entire codebase to better
reflect what it represents: an agent actively working on a task, not just
a generic 'instance'.

Changes:
- AgentInstance struct -> AgentExecution
- InstanceStore -> ExecutionStore (new file: execution_store.go)
- RecoveredInstance -> RecoveredExecution
- All method names: GetInstance* -> GetExecution*, ListInstances -> ListExecutions, etc.
- All parameters: instanceID -> executionID
- All local variables: instance -> execution
- DTOs and JSON tags: agent_instance_id -> agent_execution_id
- Database columns: agent_instance_id -> agent_execution_id

Also consolidates duplicate prompt logic:
- Merge sendInitialPrompt and SendPrompt into single SendPrompt method
- Add validateStatus parameter to differentiate initial vs follow-up prompts

Also extracts ACP session management to session.go:
- Move initializeACPSession logic to SessionManager.InitializeAndPrompt
- Move PromptAgent logic to SessionManager.SendPrompt
- Manager now delegates to SessionManager for all prompt operations

Also removes duplicate event publishing:
- Remove 9 duplicate m.publish*() methods from manager.go
- Use existing EventPublisher methods via m.eventPublisher.Publish*()

Also extracts types to dedicated file:
- Move AgentExecution, LaunchRequest, PromptResult, etc. to types.go

Net reduction: ~600 lines removed from manager.go through deduplication
and better separation of concerns.

* fix(backend): Docker runtime now creates instance via ControlServer

The Docker runtime was incorrectly creating an agentctl.Client pointing
directly to port 9999 (the ControlServer port), then trying to call
instance-specific APIs like /api/v1/agent/configure which only exist
on per-instance servers.

Fixed by making Docker runtime follow the same flow as standalone mode:
- LaunchContainer now uses StandaloneCtl to create an instance via the
  control API, then creates Client pointing to the instance port
- RecoverInstances queries the ControlServer to get the instance port
- Added waitForHealth helper to wait for agentctl to be ready

Also improved HTTP error handling in agentctl client:
- All client methods now check HTTP status codes before JSON decoding
- Better error messages include status code and response body

* fix: restore session resumption imports and variables

* fix: add sessionId prop back to TaskLayout
This commit is contained in:
João Salavisa
2026-01-16 22:14:19 +00:00
committed by GitHub
parent 5085169696
commit 2e108b040f
69 changed files with 3280 additions and 4278 deletions
@@ -9,21 +9,11 @@ import (
"github.com/kandev/kandev/internal/agent/docker"
"github.com/kandev/kandev/internal/agent/registry"
"github.com/kandev/kandev/internal/common/config"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/events/bus"
v1 "github.com/kandev/kandev/pkg/api/v1"
)
// testAgentConfig returns a default AgentConfig for testing (docker mode)
func testAgentConfig() config.AgentConfig {
return config.AgentConfig{
Runtime: "docker",
StandaloneHost: "localhost",
StandalonePort: 9999,
}
}
// MockDockerClient implements a mock for the docker.Client for testing
type MockDockerClient struct {
CreateContainerFn func(ctx context.Context, cfg docker.ContainerConfig) (string, error)
@@ -135,30 +125,54 @@ func newTestRegistry() *registry.Registry {
return reg
}
func TestNewManager(t *testing.T) {
// MockCredentialsManager implements CredentialsManager for testing
type MockCredentialsManager struct{}
func (m *MockCredentialsManager) GetCredentialValue(ctx context.Context, key string) (string, error) {
return "", nil
}
// MockProfileResolver implements ProfileResolver for testing
type MockProfileResolver struct{}
func (m *MockProfileResolver) ResolveProfile(ctx context.Context, profileID string) (*AgentProfileInfo, error) {
return &AgentProfileInfo{
ProfileID: profileID,
ProfileName: "Test Profile",
AgentID: "augment-agent",
AgentName: "auggie",
Model: "claude-sonnet-4-20250514",
}, nil
}
// newTestManager creates a Manager for testing with mock dependencies
func newTestManager() *Manager {
log := newTestLogger()
reg := newTestRegistry()
eventBus := &MockEventBus{}
credsMgr := &MockCredentialsManager{}
profileResolver := &MockProfileResolver{}
// Pass nil for runtime and containerManager - tests don't need them
return NewManager(reg, eventBus, nil, nil, credsMgr, profileResolver, log)
}
mgr := NewManager(nil, reg, eventBus, testAgentConfig(), log)
func TestNewManager(t *testing.T) {
mgr := newTestManager()
if mgr == nil {
t.Fatal("expected non-nil manager")
}
if len(mgr.instances) != 0 {
t.Errorf("expected empty instances, got %d", len(mgr.instances))
if len(mgr.ListExecutions()) != 0 {
t.Errorf("expected empty executions, got %d", len(mgr.ListExecutions()))
}
}
func TestManager_GetInstance(t *testing.T) {
log := newTestLogger()
reg := newTestRegistry()
eventBus := &MockEventBus{}
mgr := NewManager(nil, reg, eventBus, testAgentConfig(), log)
func TestManager_GetExecution(t *testing.T) {
mgr := newTestManager()
// Manually add an instance for testing
instance := &AgentInstance{
ID: "test-instance-id",
// Manually add an execution for testing
execution := &AgentExecution{
ID: "test-execution-id",
TaskID: "test-task-id",
AgentProfileID: "test-agent",
ContainerID: "container-123",
@@ -166,36 +180,29 @@ func TestManager_GetInstance(t *testing.T) {
StartedAt: time.Now(),
}
mgr.mu.Lock()
mgr.instances[instance.ID] = instance
mgr.byTask[instance.TaskID] = instance.ID
mgr.byContainer[instance.ContainerID] = instance.ID
mgr.mu.Unlock()
mgr.executionStore.Add(execution)
// Test GetInstance
got, found := mgr.GetInstance("test-instance-id")
// Test GetExecution
got, found := mgr.GetExecution("test-execution-id")
if !found {
t.Fatal("expected to find instance")
t.Fatal("expected to find execution")
}
if got.ID != instance.ID {
t.Errorf("expected ID %q, got %q", instance.ID, got.ID)
if got.ID != execution.ID {
t.Errorf("expected ID %q, got %q", execution.ID, got.ID)
}
// Test not found
_, found = mgr.GetInstance("non-existent")
_, found = mgr.GetExecution("non-existent")
if found {
t.Error("expected not to find instance")
t.Error("expected not to find execution")
}
}
func TestManager_GetInstanceByTaskID(t *testing.T) {
log := newTestLogger()
reg := newTestRegistry()
eventBus := &MockEventBus{}
mgr := NewManager(nil, reg, eventBus, testAgentConfig(), log)
func TestManager_GetExecutionByTaskID(t *testing.T) {
mgr := newTestManager()
instance := &AgentInstance{
ID: "test-instance-id",
execution := &AgentExecution{
ID: "test-execution-id",
TaskID: "test-task-id",
AgentProfileID: "test-agent",
ContainerID: "container-123",
@@ -203,35 +210,29 @@ func TestManager_GetInstanceByTaskID(t *testing.T) {
StartedAt: time.Now(),
}
mgr.mu.Lock()
mgr.instances[instance.ID] = instance
mgr.byTask[instance.TaskID] = instance.ID
mgr.mu.Unlock()
mgr.executionStore.Add(execution)
// Test GetInstanceByTaskID
got, found := mgr.GetInstanceByTaskID("test-task-id")
// Test GetExecutionByTaskID
got, found := mgr.GetExecutionByTaskID("test-task-id")
if !found {
t.Fatal("expected to find instance")
t.Fatal("expected to find execution")
}
if got.TaskID != instance.TaskID {
t.Errorf("expected TaskID %q, got %q", instance.TaskID, got.TaskID)
if got.TaskID != execution.TaskID {
t.Errorf("expected TaskID %q, got %q", execution.TaskID, got.TaskID)
}
// Test not found
_, found = mgr.GetInstanceByTaskID("non-existent")
_, found = mgr.GetExecutionByTaskID("non-existent")
if found {
t.Error("expected not to find instance")
t.Error("expected not to find execution")
}
}
func TestManager_GetInstanceByContainerID(t *testing.T) {
log := newTestLogger()
reg := newTestRegistry()
eventBus := &MockEventBus{}
mgr := NewManager(nil, reg, eventBus, testAgentConfig(), log)
func TestManager_GetExecutionByContainerID(t *testing.T) {
mgr := newTestManager()
instance := &AgentInstance{
ID: "test-instance-id",
execution := &AgentExecution{
ID: "test-execution-id",
TaskID: "test-task-id",
AgentProfileID: "test-agent",
ContainerID: "container-123",
@@ -239,74 +240,61 @@ func TestManager_GetInstanceByContainerID(t *testing.T) {
StartedAt: time.Now(),
}
mgr.mu.Lock()
mgr.instances[instance.ID] = instance
mgr.byContainer[instance.ContainerID] = instance.ID
mgr.mu.Unlock()
mgr.executionStore.Add(execution)
// Test GetInstanceByContainerID
got, found := mgr.GetInstanceByContainerID("container-123")
// Test GetExecutionByContainerID
got, found := mgr.GetExecutionByContainerID("container-123")
if !found {
t.Fatal("expected to find instance")
t.Fatal("expected to find execution")
}
if got.ContainerID != instance.ContainerID {
t.Errorf("expected ContainerID %q, got %q", instance.ContainerID, got.ContainerID)
if got.ContainerID != execution.ContainerID {
t.Errorf("expected ContainerID %q, got %q", execution.ContainerID, got.ContainerID)
}
// Test not found
_, found = mgr.GetInstanceByContainerID("non-existent")
_, found = mgr.GetExecutionByContainerID("non-existent")
if found {
t.Error("expected not to find instance")
t.Error("expected not to find execution")
}
}
func TestManager_ListInstances(t *testing.T) {
log := newTestLogger()
reg := newTestRegistry()
eventBus := &MockEventBus{}
mgr := NewManager(nil, reg, eventBus, testAgentConfig(), log)
func TestManager_ListExecutions(t *testing.T) {
mgr := newTestManager()
// Empty list
list := mgr.ListInstances()
list := mgr.ListExecutions()
if len(list) != 0 {
t.Errorf("expected empty list, got %d", len(list))
}
// Add instances
mgr.mu.Lock()
mgr.instances["instance-1"] = &AgentInstance{ID: "instance-1", TaskID: "task-1", Status: v1.AgentStatusRunning}
mgr.instances["instance-2"] = &AgentInstance{ID: "instance-2", TaskID: "task-2", Status: v1.AgentStatusCompleted}
mgr.mu.Unlock()
// Add executions
mgr.executionStore.Add(&AgentExecution{ID: "execution-1", TaskID: "task-1", Status: v1.AgentStatusRunning})
mgr.executionStore.Add(&AgentExecution{ID: "execution-2", TaskID: "task-2", Status: v1.AgentStatusCompleted})
list = mgr.ListInstances()
list = mgr.ListExecutions()
if len(list) != 2 {
t.Errorf("expected 2 instances, got %d", len(list))
t.Errorf("expected 2 executions, got %d", len(list))
}
}
func TestManager_UpdateStatus(t *testing.T) {
log := newTestLogger()
reg := newTestRegistry()
eventBus := &MockEventBus{}
mgr := NewManager(nil, reg, eventBus, testAgentConfig(), log)
mgr := newTestManager()
instance := &AgentInstance{
ID: "test-instance-id",
execution := &AgentExecution{
ID: "test-execution-id",
TaskID: "test-task-id",
Status: v1.AgentStatusRunning,
}
mgr.mu.Lock()
mgr.instances[instance.ID] = instance
mgr.mu.Unlock()
mgr.executionStore.Add(execution)
// Test UpdateStatus
err := mgr.UpdateStatus("test-instance-id", v1.AgentStatusCompleted)
err := mgr.UpdateStatus("test-execution-id", v1.AgentStatusCompleted)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
got, _ := mgr.GetInstance("test-instance-id")
got, _ := mgr.GetExecution("test-execution-id")
if got.Status != v1.AgentStatusCompleted {
t.Errorf("expected status %v, got %v", v1.AgentStatusCompleted, got.Status)
}
@@ -314,34 +302,29 @@ func TestManager_UpdateStatus(t *testing.T) {
// Test not found
err = mgr.UpdateStatus("non-existent", v1.AgentStatusCompleted)
if err == nil {
t.Error("expected error for non-existent instance")
t.Error("expected error for non-existent execution")
}
}
func TestManager_UpdateProgress(t *testing.T) {
log := newTestLogger()
reg := newTestRegistry()
eventBus := &MockEventBus{}
mgr := NewManager(nil, reg, eventBus, testAgentConfig(), log)
mgr := newTestManager()
instance := &AgentInstance{
ID: "test-instance-id",
execution := &AgentExecution{
ID: "test-execution-id",
TaskID: "test-task-id",
Status: v1.AgentStatusRunning,
Progress: 0,
}
mgr.mu.Lock()
mgr.instances[instance.ID] = instance
mgr.mu.Unlock()
mgr.executionStore.Add(execution)
// Test UpdateProgress
err := mgr.UpdateProgress("test-instance-id", 50)
err := mgr.UpdateProgress("test-execution-id", 50)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
got, _ := mgr.GetInstance("test-instance-id")
got, _ := mgr.GetExecution("test-execution-id")
if got.Progress != 50 {
t.Errorf("expected progress 50, got %d", got.Progress)
}
@@ -349,18 +332,15 @@ func TestManager_UpdateProgress(t *testing.T) {
// Test not found
err = mgr.UpdateProgress("non-existent", 50)
if err == nil {
t.Error("expected error for non-existent instance")
t.Error("expected error for non-existent execution")
}
}
func TestManager_MarkCompleted_Success(t *testing.T) {
log := newTestLogger()
reg := newTestRegistry()
eventBus := &MockEventBus{}
mgr := NewManager(nil, reg, eventBus, testAgentConfig(), log)
mgr := newTestManager()
instance := &AgentInstance{
ID: "test-instance-id",
execution := &AgentExecution{
ID: "test-execution-id",
TaskID: "test-task-id",
AgentProfileID: "test-agent",
ContainerID: "container-123",
@@ -369,17 +349,15 @@ func TestManager_MarkCompleted_Success(t *testing.T) {
Progress: 50,
}
mgr.mu.Lock()
mgr.instances[instance.ID] = instance
mgr.mu.Unlock()
mgr.executionStore.Add(execution)
// Mark as completed successfully (exit code 0)
err := mgr.MarkCompleted("test-instance-id", 0, "")
err := mgr.MarkCompleted("test-execution-id", 0, "")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
got, _ := mgr.GetInstance("test-instance-id")
got, _ := mgr.GetExecution("test-execution-id")
if got.Status != v1.AgentStatusCompleted {
t.Errorf("expected status %v, got %v", v1.AgentStatusCompleted, got.Status)
}
@@ -395,13 +373,10 @@ func TestManager_MarkCompleted_Success(t *testing.T) {
}
func TestManager_MarkCompleted_Failure(t *testing.T) {
log := newTestLogger()
reg := newTestRegistry()
eventBus := &MockEventBus{}
mgr := NewManager(nil, reg, eventBus, testAgentConfig(), log)
mgr := newTestManager()
instance := &AgentInstance{
ID: "test-instance-id",
execution := &AgentExecution{
ID: "test-execution-id",
TaskID: "test-task-id",
AgentProfileID: "test-agent",
ContainerID: "container-123",
@@ -409,17 +384,15 @@ func TestManager_MarkCompleted_Failure(t *testing.T) {
StartedAt: time.Now(),
}
mgr.mu.Lock()
mgr.instances[instance.ID] = instance
mgr.mu.Unlock()
mgr.executionStore.Add(execution)
// Mark as failed
err := mgr.MarkCompleted("test-instance-id", 1, "process failed")
err := mgr.MarkCompleted("test-execution-id", 1, "process failed")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
got, _ := mgr.GetInstance("test-instance-id")
got, _ := mgr.GetExecution("test-execution-id")
if got.Status != v1.AgentStatusFailed {
t.Errorf("expected status %v, got %v", v1.AgentStatusFailed, got.Status)
}
@@ -432,58 +405,45 @@ func TestManager_MarkCompleted_Failure(t *testing.T) {
}
func TestManager_MarkCompleted_NotFound(t *testing.T) {
log := newTestLogger()
reg := newTestRegistry()
eventBus := &MockEventBus{}
mgr := NewManager(nil, reg, eventBus, testAgentConfig(), log)
mgr := newTestManager()
err := mgr.MarkCompleted("non-existent", 0, "")
if err == nil {
t.Error("expected error for non-existent instance")
t.Error("expected error for non-existent execution")
}
}
func TestManager_RemoveInstance(t *testing.T) {
log := newTestLogger()
reg := newTestRegistry()
eventBus := &MockEventBus{}
mgr := NewManager(nil, reg, eventBus, testAgentConfig(), log)
func TestManager_RemoveExecution(t *testing.T) {
mgr := newTestManager()
instance := &AgentInstance{
ID: "test-instance-id",
execution := &AgentExecution{
ID: "test-execution-id",
TaskID: "test-task-id",
ContainerID: "container-123",
}
mgr.mu.Lock()
mgr.instances[instance.ID] = instance
mgr.byTask[instance.TaskID] = instance.ID
mgr.byContainer[instance.ContainerID] = instance.ID
mgr.mu.Unlock()
mgr.executionStore.Add(execution)
// Remove instance
mgr.RemoveInstance("test-instance-id")
// Remove execution
mgr.RemoveExecution("test-execution-id")
// Verify it's gone from all maps
if _, found := mgr.GetInstance("test-instance-id"); found {
t.Error("instance should be removed from instances map")
if _, found := mgr.GetExecution("test-execution-id"); found {
t.Error("execution should be removed from executions map")
}
if _, found := mgr.GetInstanceByTaskID("test-task-id"); found {
t.Error("instance should be removed from byTask map")
if _, found := mgr.GetExecutionByTaskID("test-task-id"); found {
t.Error("execution should be removed from byTask map")
}
if _, found := mgr.GetInstanceByContainerID("container-123"); found {
t.Error("instance should be removed from byContainer map")
if _, found := mgr.GetExecutionByContainerID("container-123"); found {
t.Error("execution should be removed from byContainer map")
}
// Remove non-existent should not panic
mgr.RemoveInstance("non-existent")
mgr.RemoveExecution("non-existent")
}
func TestManager_StartStop(t *testing.T) {
log := newTestLogger()
reg := newTestRegistry()
eventBus := &MockEventBus{}
mgr := NewManager(nil, reg, eventBus, testAgentConfig(), log)
mgr := newTestManager()
ctx := context.Background()