feat: Add agent invocation support to Conversation and PlannerEvent models #85
Conversation
…odels Add hierarchy support for agent invocations through conversation nesting: Data Model Changes: - Conversation: Add ParentConversationID, Depth, InvokedBy fields - PlannerEvent: Add AgentCompletion field and PlannerAgentCompletion struct - Support for "agent_completion" event type Features: - Conversations can now form parent-child hierarchies - Track nesting depth for limiting recursion - Link child conversations to parent tool invocations - Agent completion events to resume parent execution Testing: - Comprehensive tests for conversation hierarchy - Tests for agent completion event handling - Backward compatibility verified - Serialization/deserialization tests This enables stateless agent invocation where any conversation can invoke other conversations asynchronously via queue events.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
WalkthroughThis change extends the conversation and planner event models to support hierarchical conversation tracking. The Conversation struct receives three new fields (ParentConversationID, Depth, InvokedBy) to maintain parent-child relationships and invocation context. A new PlannerAgentCompletion struct is introduced to represent child agent completion events that resume parent execution, with fields capturing parent/child conversation IDs, tool use context, completion status, and timing. Supporting tests validate hierarchy chains, backward compatibility, serialization correctness, and event variant handling. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
pkg/model/model/conversation.go (2)
17-19: Add a small helper for clarity: Conversation.IsRoot().Avoid recomputing “root” across call sites; add a method that encodes the invariant ParentConversationID == "" && Depth == 0.
@@ type Conversation struct { registry.BaseModel baseTableModel @@ InvokedBy string `dynamodbav:"invokedBy,omitempty" json:"invokedBy,omitempty" desc:"Tool use ID that invoked this conversation for linking tool call to child conversation." example:"toolu_01A2B3C4D5E6F7G8H9I0J1K2L3M4N5"` } +// IsRoot reports whether this conversation is a top-level root. +func (c Conversation) IsRoot() bool { + return c.ParentConversationID == "" && c.Depth == 0 +}
18-18: Flag negative depth early (defensive).Depth should be >= 0. If you plan to validate models, consider adding a lightweight check (e.g., in a Validate() or in hooks) to guard negative values. Optional if enforced upstream.
pkg/model/model/conversation_test.go (3)
184-238: Broaden table: include a negative depth case.Add a case like depth = -1 to assert it’s rejected (once depth validation exists) or at least documented. Also consider using conv.IsRoot() once added.
@@ tests := []struct { @@ }{ @@ + { + name: "invalid negative depth", + parentConversationID: "parent-uuid-123", + depth: -1, + invokedBy: "toolu_neg", + wantValid: true, // flip to false if/when validation is added + }, } @@ - if got := conv.Valid(); got != tt.wantValid { + if got := conv.Valid(); got != tt.wantValid { t.Errorf("Conversation.Valid() = %v, want %v", got, tt.wantValid) }
254-285: Chain test: add a quick uniqueness/assert pair.Minor coverage boost: assert child/grandchild UUIDs/keys differ from parent and are non-empty.
@@ // Verify hierarchy assert.Equal(t, parent.UUID, child.ParentConversationID) assert.Equal(t, 1, child.Depth) assert.NotEmpty(t, child.InvokedBy) + assert.NotEmpty(t, child.UUID) + assert.NotEqual(t, parent.UUID, child.UUID) + assert.NotEmpty(t, child.Key) + assert.NotEqual(t, parent.Key, child.Key) @@ // Verify grandchild depth assert.Equal(t, 2, grandchild.Depth) assert.Equal(t, child.UUID, grandchild.ParentConversationID) + assert.NotEmpty(t, grandchild.UUID) + assert.NotEqual(t, child.UUID, grandchild.UUID)
286-323: Prefer using Conversation.IsRoot() in tests.Once the helper exists, use it for consistency and single-source of truth.
@@ - isRoot := conv.ParentConversationID == "" - assert.Equal(t, tt.wantIsRoot, isRoot) + assert.Equal(t, tt.wantIsRoot, conv.IsRoot())pkg/model/model/planner_event.go (2)
38-47: Avoid null for ToolsUsed; add omitempty (or always init to empty slice).Without omitempty, a nil slice serializes as null. Either always initialize to []string{} or mark omitempty. The tests initialize it; this change protects other producers.
type PlannerAgentCompletion struct { @@ - ToolsUsed []string `json:"toolsUsed" desc:"List of tools the child agent used."` + ToolsUsed []string `json:"toolsUsed,omitempty" desc:"List of tools the child agent used."` }
3-19: Define event type constants to prevent typos.Replace string literals like "agent_completion", "job_completion", "planner_execution" with constants.
package model +const ( + PlannerEventTypeAgentCompletion = "agent_completion" + PlannerEventTypeJobCompletion = "job_completion" + PlannerEventTypePlannerExec = "planner_execution" +)pkg/model/model/planner_event_test.go (3)
62-126: Use event type constants once added.Swap string literals for constants to avoid drift.
- Type: "planner_execution", + Type: PlannerEventTypePlannerExec, @@ - Type: "job_completion", + Type: PlannerEventTypeJobCompletion,
158-189: Also assert RFC3339 timestamp parses.Small guardrail for CompletedAt correctness.
@@ -import ( +import ( "encoding/json" "testing" + "time" ) @@ require.NoError(t, err) assert.Equal(t, "agent_completion", decoded.Type) assert.NotNil(t, decoded.AgentCompletion) assert.Nil(t, decoded.JobCompletion) assert.Nil(t, decoded.UserMessage) + _, terr := time.Parse(time.RFC3339, decoded.AgentCompletion.CompletedAt) + require.NoError(t, terr)
191-214: Test nil ToolsUsed serialization behavior.Add a case where ToolsUsed is left nil to ensure it’s either omitted or non-null per your chosen contract (after omitempty or defaulting).
@@ func TestPlannerAgentCompletion_RequiredFields(t *testing.T) { completion := PlannerAgentCompletion{ @@ CompletedAt: Now(), } @@ assert.NotEmpty(t, completion.CompletedAt) + + // Optional: verify behavior when ToolsUsed is nil + completionNil := PlannerAgentCompletion{ + ParentConversationID: "parent-123", + ChildConversationID: "child-456", + ParentToolUseID: "toolu_abc123", + Response: "Test", + Success: true, + CompletedAt: Now(), + } + data, err := json.Marshal(completionNil) + require.NoError(t, err) + // If omitempty is set, toolsUsed should be absent; otherwise assert it’s not null array depending on your contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
pkg/model/model/conversation.go(1 hunks)pkg/model/model/conversation_test.go(1 hunks)pkg/model/model/planner_event.go(2 hunks)pkg/model/model/planner_event_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
pkg/model/{model,attacksurface,beta}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/model/{model,attacksurface,beta}/**/*.go: Register every new model in its init() using registry.Registry.MustRegisterModel(&ModelName{})
Model structs must embed BaseAsset
All model fields must include json and desc struct tags for proper schema generation
Where applicable, include neo4j and example struct tags on model fields
Files:
pkg/model/model/planner_event_test.gopkg/model/model/conversation.gopkg/model/model/planner_event.gopkg/model/model/conversation_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use the standard Go testing framework (testing package); do not introduce additional test frameworks
Files:
pkg/model/model/planner_event_test.gopkg/model/model/conversation_test.go
🧬 Code graph analysis (2)
pkg/model/model/planner_event_test.go (2)
pkg/model/model/planner_event.go (4)
PlannerEvent(3-19)PlannerAgentCompletion(38-47)PlannerUserMessage(33-36)PlannerJobCompletion(21-31)pkg/model/model/globals.go (1)
Now(250-252)
pkg/model/model/conversation_test.go (1)
pkg/model/model/conversation.go (1)
NewConversation(57-64)
🔇 Additional comments (5)
pkg/model/model/conversation.go (1)
17-19: New hierarchy fields look correct and schema-friendly.Tags (dynamodbav/json/desc/example) are consistent and backward compatible via omitempty where appropriate. Nice.
pkg/model/model/conversation_test.go (1)
240-253: Backward-compat test reads well.Zero-values on new fields and overall validity confirmed. LGTM.
pkg/model/model/planner_event_test.go (3)
11-35: AgentCompletion success path: solid coverage.Round-trips and core invariants are asserted. LGTM.
37-60: Error path covered.Clear assertions on Success=false and Error content. LGTM.
1-214: Testing framework guideline check.Coding guideline says to use the standard testing package only. This file imports testify. Confirm repo policy; if the guideline applies here, we should migrate assertions to the stdlib in a follow-up.
As per coding guidelines
Add hierarchy support for agent invocations through conversation nesting:
Data Model Changes:
Features:
Testing:
This enables stateless agent invocation where any conversation can
invoke other conversations asynchronously via queue events.