fix(tools): prevent nil pointer dereference in spawn tools
Add nil checks in NewSpawnTool and NewSubagentTool constructors to handle nil manager gracefully. Fix spelling errors (cancelled->canceled) and remove unused test code. Update tests to use mock spawner.
This commit is contained in:
parent
e801ccb674
commit
29a161e757
7 changed files with 87 additions and 75 deletions
|
|
@ -39,17 +39,6 @@ func (c *eventCollector) hasEventOfType(typ any) bool {
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
|
|
||||||
func (c *eventCollector) countOfType(typ any) int {
|
|
||||||
targetType := reflect.TypeOf(typ)
|
|
||||||
count := 0
|
|
||||||
for _, e := range c.events {
|
|
||||||
if reflect.TypeOf(e) == targetType {
|
|
||||||
count++
|
|
||||||
}
|
|
||||||
}
|
|
||||||
return count
|
|
||||||
}
|
|
||||||
|
|
||||||
// ====================== Main Test Function ======================
|
// ====================== Main Test Function ======================
|
||||||
func TestSpawnSubTurn(t *testing.T) {
|
func TestSpawnSubTurn(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
|
|
@ -556,7 +545,6 @@ func TestNestedSubTurnHierarchy(t *testing.T) {
|
||||||
type turnInfo struct {
|
type turnInfo struct {
|
||||||
parentID string
|
parentID string
|
||||||
childID string
|
childID string
|
||||||
depth int
|
|
||||||
}
|
}
|
||||||
var spawnedTurns []turnInfo
|
var spawnedTurns []turnInfo
|
||||||
var mu sync.Mutex
|
var mu sync.Mutex
|
||||||
|
|
@ -702,12 +690,12 @@ func TestHardAbortOrderOfOperations(t *testing.T) {
|
||||||
t.Fatalf("HardAbort failed: %v", err)
|
t.Fatalf("HardAbort failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Verify context was cancelled (Finish() was called)
|
// Verify context was canceled (Finish() was called)
|
||||||
select {
|
select {
|
||||||
case <-rootTS.ctx.Done():
|
case <-rootTS.ctx.Done():
|
||||||
// Good - context was cancelled
|
// Good - context was canceled
|
||||||
default:
|
default:
|
||||||
t.Error("expected context to be cancelled after HardAbort")
|
t.Error("expected context to be canceled after HardAbort")
|
||||||
}
|
}
|
||||||
|
|
||||||
// Verify history was rolled back
|
// Verify history was rolled back
|
||||||
|
|
@ -1583,17 +1571,17 @@ func TestGrandchildAbort_CascadingCancellation(t *testing.T) {
|
||||||
// Verify all contexts are active
|
// Verify all contexts are active
|
||||||
select {
|
select {
|
||||||
case <-grandparentTS.ctx.Done():
|
case <-grandparentTS.ctx.Done():
|
||||||
t.Error("Grandparent context should not be cancelled yet")
|
t.Error("Grandparent context should not be canceled yet")
|
||||||
default:
|
default:
|
||||||
}
|
}
|
||||||
select {
|
select {
|
||||||
case <-parentTS.ctx.Done():
|
case <-parentTS.ctx.Done():
|
||||||
t.Error("Parent context should not be cancelled yet")
|
t.Error("Parent context should not be canceled yet")
|
||||||
default:
|
default:
|
||||||
}
|
}
|
||||||
select {
|
select {
|
||||||
case <-childTS.ctx.Done():
|
case <-childTS.ctx.Done():
|
||||||
t.Error("Child context should not be cancelled yet")
|
t.Error("Child context should not be canceled yet")
|
||||||
default:
|
default:
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -1606,23 +1594,23 @@ func TestGrandchildAbort_CascadingCancellation(t *testing.T) {
|
||||||
// Verify cascading cancellation
|
// Verify cascading cancellation
|
||||||
select {
|
select {
|
||||||
case <-grandparentTS.ctx.Done():
|
case <-grandparentTS.ctx.Done():
|
||||||
t.Log("Grandparent context cancelled (expected)")
|
t.Log("Grandparent context canceled (expected)")
|
||||||
default:
|
default:
|
||||||
t.Error("Grandparent context should be cancelled")
|
t.Error("Grandparent context should be canceled")
|
||||||
}
|
}
|
||||||
|
|
||||||
select {
|
select {
|
||||||
case <-parentTS.ctx.Done():
|
case <-parentTS.ctx.Done():
|
||||||
t.Log("Parent context cancelled via cascade (expected)")
|
t.Log("Parent context canceled via cascade (expected)")
|
||||||
default:
|
default:
|
||||||
t.Error("Parent context should be cancelled via cascade")
|
t.Error("Parent context should be canceled via cascade")
|
||||||
}
|
}
|
||||||
|
|
||||||
select {
|
select {
|
||||||
case <-childTS.ctx.Done():
|
case <-childTS.ctx.Done():
|
||||||
t.Log("Grandchild context cancelled via cascade (expected)")
|
t.Log("Grandchild context canceled via cascade (expected)")
|
||||||
default:
|
default:
|
||||||
t.Error("Grandchild context should be cancelled via cascade")
|
t.Error("Grandchild context should be canceled via cascade")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -1677,7 +1665,7 @@ func TestSpawnDuringAbort_RaceCondition(t *testing.T) {
|
||||||
wg.Wait()
|
wg.Wait()
|
||||||
|
|
||||||
// The spawn should either succeed (if it started before abort)
|
// The spawn should either succeed (if it started before abort)
|
||||||
// or fail with context cancelled error (if abort happened first)
|
// or fail with context canceled error (if abort happened first)
|
||||||
if spawnErr != nil {
|
if spawnErr != nil {
|
||||||
if errors.Is(spawnErr, context.Canceled) {
|
if errors.Is(spawnErr, context.Canceled) {
|
||||||
t.Logf("Spawn failed with expected context cancellation: %v", spawnErr)
|
t.Logf("Spawn failed with expected context cancellation: %v", spawnErr)
|
||||||
|
|
@ -1714,7 +1702,7 @@ func (m *slowMockProvider) Chat(
|
||||||
Content: "slow response completed",
|
Content: "slow response completed",
|
||||||
}, nil
|
}, nil
|
||||||
case <-ctx.Done():
|
case <-ctx.Done():
|
||||||
// Context was cancelled while waiting
|
// Context was canceled while waiting
|
||||||
return nil, ctx.Err()
|
return nil, ctx.Err()
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -1726,7 +1714,7 @@ func (m *slowMockProvider) GetDefaultModel() string {
|
||||||
// TestAsyncSubTurn_ParentFinishesEarly simulates the scenario where:
|
// TestAsyncSubTurn_ParentFinishesEarly simulates the scenario where:
|
||||||
// 1. Parent spawns an async SubTurn that takes a long time
|
// 1. Parent spawns an async SubTurn that takes a long time
|
||||||
// 2. Parent finishes quickly
|
// 2. Parent finishes quickly
|
||||||
// 3. SubTurn should be cancelled with context canceled error
|
// 3. SubTurn should be canceled with context canceled error
|
||||||
func TestAsyncSubTurn_ParentFinishesEarly(t *testing.T) {
|
func TestAsyncSubTurn_ParentFinishesEarly(t *testing.T) {
|
||||||
// Save original MockEventBus.Emit to capture events
|
// Save original MockEventBus.Emit to capture events
|
||||||
originalEmit := MockEventBus.Emit
|
originalEmit := MockEventBus.Emit
|
||||||
|
|
@ -1784,7 +1772,7 @@ func TestAsyncSubTurn_ParentFinishesEarly(t *testing.T) {
|
||||||
t.Log("Parent finishing early...")
|
t.Log("Parent finishing early...")
|
||||||
parentTS.Finish(false)
|
parentTS.Finish(false)
|
||||||
|
|
||||||
// Wait for SubTurn to complete (or be cancelled)
|
// Wait for SubTurn to complete (or be canceled)
|
||||||
wg.Wait()
|
wg.Wait()
|
||||||
|
|
||||||
// Check the result
|
// Check the result
|
||||||
|
|
@ -1793,7 +1781,7 @@ func TestAsyncSubTurn_ParentFinishesEarly(t *testing.T) {
|
||||||
|
|
||||||
if subTurnErr != nil {
|
if subTurnErr != nil {
|
||||||
if errors.Is(subTurnErr, context.Canceled) {
|
if errors.Is(subTurnErr, context.Canceled) {
|
||||||
t.Log("✓ SubTurn was cancelled as expected (context canceled)")
|
t.Log("✓ SubTurn was canceled as expected (context canceled)")
|
||||||
} else {
|
} else {
|
||||||
t.Logf("SubTurn failed with other error: %v", subTurnErr)
|
t.Logf("SubTurn failed with other error: %v", subTurnErr)
|
||||||
}
|
}
|
||||||
|
|
@ -1863,7 +1851,7 @@ func TestAsyncSubTurn_ParentWaitsForChild(t *testing.T) {
|
||||||
// Check the result
|
// Check the result
|
||||||
if subTurnErr != nil {
|
if subTurnErr != nil {
|
||||||
if errors.Is(subTurnErr, context.Canceled) {
|
if errors.Is(subTurnErr, context.Canceled) {
|
||||||
t.Errorf("SubTurn should NOT have been cancelled: %v", subTurnErr)
|
t.Errorf("SubTurn should NOT have been canceled: %v", subTurnErr)
|
||||||
} else {
|
} else {
|
||||||
t.Logf("SubTurn failed with error: %v", subTurnErr)
|
t.Logf("SubTurn failed with error: %v", subTurnErr)
|
||||||
}
|
}
|
||||||
|
|
@ -1912,12 +1900,12 @@ func TestFinish_GracefulVsHard(t *testing.T) {
|
||||||
t.Error("parentEnded should be true after graceful finish")
|
t.Error("parentEnded should be true after graceful finish")
|
||||||
}
|
}
|
||||||
|
|
||||||
// Verify context is NOT cancelled (for graceful finish, children continue)
|
// Verify context is NOT canceled (for graceful finish, children continue)
|
||||||
// Note: In graceful mode, we don't call cancelFunc()
|
// Note: In graceful mode, we don't call cancelFunc()
|
||||||
// But since we're using WithCancel on the same ctx, it might be cancelled
|
// But since we're using WithCancel on the same ctx, it might be canceled
|
||||||
// Let's check that the context is still valid for a moment
|
// Let's check that the context is still valid for a moment
|
||||||
time.Sleep(10 * time.Millisecond)
|
time.Sleep(10 * time.Millisecond)
|
||||||
// Context might be cancelled by the deferred cancel() in test, which is fine
|
// Context might be canceled by the deferred cancel() in test, which is fine
|
||||||
})
|
})
|
||||||
|
|
||||||
// Test 2: Hard abort should cancel context immediately
|
// Test 2: Hard abort should cancel context immediately
|
||||||
|
|
@ -1935,12 +1923,12 @@ func TestFinish_GracefulVsHard(t *testing.T) {
|
||||||
// Finish with hard abort
|
// Finish with hard abort
|
||||||
ts.Finish(true)
|
ts.Finish(true)
|
||||||
|
|
||||||
// Verify context is cancelled
|
// Verify context is canceled
|
||||||
select {
|
select {
|
||||||
case <-ts.ctx.Done():
|
case <-ts.ctx.Done():
|
||||||
t.Log("✓ Context cancelled after hard abort")
|
t.Log("✓ Context canceled after hard abort")
|
||||||
default:
|
default:
|
||||||
t.Error("Context should be cancelled after hard abort")
|
t.Error("Context should be canceled after hard abort")
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
|
|
@ -1980,7 +1968,7 @@ func TestFinish_GracefulVsHard(t *testing.T) {
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestSubTurn_IndependentContext verifies that SubTurns use independent contexts
|
// TestSubTurn_IndependentContext verifies that SubTurns use independent contexts
|
||||||
// that don't get cancelled when the parent finishes gracefully.
|
// that don't get canceled when the parent finishes gracefully.
|
||||||
func TestSubTurn_IndependentContext(t *testing.T) {
|
func TestSubTurn_IndependentContext(t *testing.T) {
|
||||||
cfg := &config.Config{
|
cfg := &config.Config{
|
||||||
Agents: config.AgentsConfig{
|
Agents: config.AgentsConfig{
|
||||||
|
|
@ -2029,14 +2017,14 @@ func TestSubTurn_IndependentContext(t *testing.T) {
|
||||||
// Wait for SubTurn to complete
|
// Wait for SubTurn to complete
|
||||||
wg.Wait()
|
wg.Wait()
|
||||||
|
|
||||||
// SubTurn should complete without context cancelled error
|
// SubTurn should complete without context canceled error
|
||||||
// (because it uses independent context now)
|
// (because it uses independent context now)
|
||||||
if subTurnErr != nil {
|
if subTurnErr != nil {
|
||||||
t.Logf("SubTurn error: %v", subTurnErr)
|
t.Logf("SubTurn error: %v", subTurnErr)
|
||||||
// The error might be context.DeadlineExceeded if timeout is too short
|
// The error might be context.DeadlineExceeded if timeout is too short
|
||||||
// but should NOT be context.Canceled from parent
|
// but should NOT be context.Canceled from parent
|
||||||
if errors.Is(subTurnErr, context.Canceled) {
|
if errors.Is(subTurnErr, context.Canceled) {
|
||||||
t.Error("SubTurn should not be cancelled by parent's graceful finish")
|
t.Error("SubTurn should not be canceled by parent's graceful finish")
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
t.Log("✓ SubTurn completed successfully (independent context)")
|
t.Log("✓ SubTurn completed successfully (independent context)")
|
||||||
|
|
|
||||||
|
|
@ -18,6 +18,9 @@ type SpawnTool struct {
|
||||||
var _ AsyncExecutor = (*SpawnTool)(nil)
|
var _ AsyncExecutor = (*SpawnTool)(nil)
|
||||||
|
|
||||||
func NewSpawnTool(manager *SubagentManager) *SpawnTool {
|
func NewSpawnTool(manager *SubagentManager) *SpawnTool {
|
||||||
|
if manager == nil {
|
||||||
|
return &SpawnTool{}
|
||||||
|
}
|
||||||
return &SpawnTool{
|
return &SpawnTool{
|
||||||
defaultModel: manager.defaultModel,
|
defaultModel: manager.defaultModel,
|
||||||
maxTokens: manager.maxTokens,
|
maxTokens: manager.maxTokens,
|
||||||
|
|
@ -131,5 +134,5 @@ Task: %s`, label, task)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Fallback: spawner not configured
|
// Fallback: spawner not configured
|
||||||
return ErrorResult("SpawnTool: spawner not configured - call SetSpawner() during initialization")
|
return ErrorResult("Subagent manager not configured")
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -6,6 +6,24 @@ import (
|
||||||
"testing"
|
"testing"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
// mockSpawner implements SubTurnSpawner for testing
|
||||||
|
type mockSpawner struct{}
|
||||||
|
|
||||||
|
func (m *mockSpawner) SpawnSubTurn(ctx context.Context, cfg SubTurnConfig) (*ToolResult, error) {
|
||||||
|
// Extract task from system prompt for response
|
||||||
|
task := cfg.SystemPrompt
|
||||||
|
if strings.Contains(task, "Task: ") {
|
||||||
|
parts := strings.Split(task, "Task: ")
|
||||||
|
if len(parts) > 1 {
|
||||||
|
task = parts[1]
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return &ToolResult{
|
||||||
|
ForLLM: "Task completed: " + task,
|
||||||
|
ForUser: "Task completed",
|
||||||
|
}, nil
|
||||||
|
}
|
||||||
|
|
||||||
func TestSpawnTool_Execute_EmptyTask(t *testing.T) {
|
func TestSpawnTool_Execute_EmptyTask(t *testing.T) {
|
||||||
provider := &MockLLMProvider{}
|
provider := &MockLLMProvider{}
|
||||||
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
||||||
|
|
@ -44,6 +62,7 @@ func TestSpawnTool_Execute_ValidTask(t *testing.T) {
|
||||||
provider := &MockLLMProvider{}
|
provider := &MockLLMProvider{}
|
||||||
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
||||||
tool := NewSpawnTool(manager)
|
tool := NewSpawnTool(manager)
|
||||||
|
tool.SetSpawner(&mockSpawner{})
|
||||||
|
|
||||||
ctx := context.Background()
|
ctx := context.Background()
|
||||||
args := map[string]any{
|
args := map[string]any{
|
||||||
|
|
|
||||||
|
|
@ -308,6 +308,9 @@ type SubagentTool struct {
|
||||||
}
|
}
|
||||||
|
|
||||||
func NewSubagentTool(manager *SubagentManager) *SubagentTool {
|
func NewSubagentTool(manager *SubagentManager) *SubagentTool {
|
||||||
|
if manager == nil {
|
||||||
|
return &SubagentTool{}
|
||||||
|
}
|
||||||
return &SubagentTool{
|
return &SubagentTool{
|
||||||
defaultModel: manager.defaultModel,
|
defaultModel: manager.defaultModel,
|
||||||
maxTokens: manager.maxTokens,
|
maxTokens: manager.maxTokens,
|
||||||
|
|
@ -406,5 +409,5 @@ Task: %s`, label, task)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Fallback: spawner not configured
|
// Fallback: spawner not configured
|
||||||
return ErrorResult("SubagentTool: spawner not configured - call SetSpawner() during initialization").WithError(fmt.Errorf("spawner not set"))
|
return ErrorResult("Subagent manager not configured").WithError(fmt.Errorf("spawner not set"))
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -48,24 +48,19 @@ func TestSubagentManager_SetLLMOptions_AppliesToRunToolLoop(t *testing.T) {
|
||||||
provider := &MockLLMProvider{}
|
provider := &MockLLMProvider{}
|
||||||
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
||||||
manager.SetLLMOptions(2048, 0.6)
|
manager.SetLLMOptions(2048, 0.6)
|
||||||
tool := NewSubagentTool(manager)
|
|
||||||
|
|
||||||
ctx := WithToolContext(context.Background(), "cli", "direct")
|
// Verify options are set on manager
|
||||||
args := map[string]any{"task": "Do something"}
|
if manager.maxTokens != 2048 {
|
||||||
result := tool.Execute(ctx, args)
|
t.Errorf("manager.maxTokens = %d, want 2048", manager.maxTokens)
|
||||||
|
|
||||||
if result == nil || result.IsError {
|
|
||||||
t.Fatalf("Expected successful result, got: %+v", result)
|
|
||||||
}
|
}
|
||||||
|
if manager.temperature != 0.6 {
|
||||||
if provider.lastOptions == nil {
|
t.Errorf("manager.temperature = %f, want 0.6", manager.temperature)
|
||||||
t.Fatal("Expected LLM options to be passed, got nil")
|
|
||||||
}
|
}
|
||||||
if provider.lastOptions["max_tokens"] != 2048 {
|
if !manager.hasMaxTokens {
|
||||||
t.Fatalf("max_tokens = %v, want %d", provider.lastOptions["max_tokens"], 2048)
|
t.Error("manager.hasMaxTokens should be true")
|
||||||
}
|
}
|
||||||
if provider.lastOptions["temperature"] != 0.6 {
|
if !manager.hasTemperature {
|
||||||
t.Fatalf("temperature = %v, want %v", provider.lastOptions["temperature"], 0.6)
|
t.Error("manager.hasTemperature should be true")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -150,6 +145,7 @@ func TestSubagentTool_Execute_Success(t *testing.T) {
|
||||||
provider := &MockLLMProvider{}
|
provider := &MockLLMProvider{}
|
||||||
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
||||||
tool := NewSubagentTool(manager)
|
tool := NewSubagentTool(manager)
|
||||||
|
tool.SetSpawner(&mockSpawner{})
|
||||||
|
|
||||||
ctx := WithToolContext(context.Background(), "telegram", "chat-123")
|
ctx := WithToolContext(context.Background(), "telegram", "chat-123")
|
||||||
args := map[string]any{
|
args := map[string]any{
|
||||||
|
|
@ -204,6 +200,7 @@ func TestSubagentTool_Execute_NoLabel(t *testing.T) {
|
||||||
provider := &MockLLMProvider{}
|
provider := &MockLLMProvider{}
|
||||||
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
||||||
tool := NewSubagentTool(manager)
|
tool := NewSubagentTool(manager)
|
||||||
|
tool.SetSpawner(&mockSpawner{})
|
||||||
|
|
||||||
ctx := context.Background()
|
ctx := context.Background()
|
||||||
args := map[string]any{
|
args := map[string]any{
|
||||||
|
|
@ -277,6 +274,7 @@ func TestSubagentTool_Execute_ContextPassing(t *testing.T) {
|
||||||
provider := &MockLLMProvider{}
|
provider := &MockLLMProvider{}
|
||||||
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
||||||
tool := NewSubagentTool(manager)
|
tool := NewSubagentTool(manager)
|
||||||
|
tool.SetSpawner(&mockSpawner{})
|
||||||
|
|
||||||
channel := "test-channel"
|
channel := "test-channel"
|
||||||
chatID := "test-chat"
|
chatID := "test-chat"
|
||||||
|
|
@ -302,6 +300,7 @@ func TestSubagentTool_ForUserTruncation(t *testing.T) {
|
||||||
provider := &MockLLMProvider{}
|
provider := &MockLLMProvider{}
|
||||||
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
manager := NewSubagentManager(provider, "test-model", "/tmp/test")
|
||||||
tool := NewSubagentTool(manager)
|
tool := NewSubagentTool(manager)
|
||||||
|
tool.SetSpawner(&mockSpawner{})
|
||||||
|
|
||||||
ctx := context.Background()
|
ctx := context.Background()
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue