Conversation
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Analysis Results: Has Go, TypeScript, or Python Changes: Yes Findings• Major architectural changes: Introduction of Recommendations• Verify backward compatibility: Cloud resource changes remove significant code - ensure dependent systems handle the simplified structure |
WalkthroughCentralizes Merge/Visit for cloud resources by delegating AWS/GCP/Azure updates to an enhanced CloudResource. Introduces OriginationData (Capability, AttackSurface, Origins) embedded in Metadata and CloudResource with Merge/Visit methods to manage Status, Visited, TTL, Properties, and origination slices. Adds Defaulted() for cloud resources to initialize Origins and AttackSurface. Updates tests to cover OriginationData, CloudResource, AWS/Azure/GCP defaulting and integration; message tests switch from KSUID to UUIDv7; conversation tests drop Source/TTL assertions. ✨ Finishing touches
🧪 Generate unit tests
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/model/model/message.go (2)
47-51: Handle uuid.NewV7() error to avoid zero UUIDs.Ignoring the error can yield an all-zero UUID on failure, breaking uniqueness and keys derived from it.
func (m *Message) Defaulted() { m.Timestamp = Now() m.TTL = Future(24 * 30) if m.MessageID == "" { - id, _ := uuid.NewV7() - m.MessageID = id.String() + if id, err := uuid.NewV7(); err == nil { + m.MessageID = id.String() + } else { + // Fallback to v4 to preserve uniqueness if v7 generation fails + m.MessageID = uuid.New().String() + } } }
58-63: Also handle uuid.NewV7() error inside hook.Same risk path as Defaulted(); ensure the hook can’t produce a zero UUID on entropy failure.
if m.Key == "" { if m.MessageID == "" { - id, _ := uuid.NewV7() - m.MessageID = id.String() + if id, err := uuid.NewV7(); err == nil { + m.MessageID = id.String() + } else { + m.MessageID = uuid.New().String() + } } m.Key = "#message#" + m.ConversationID + "#" + m.MessageID }
🧹 Nitpick comments (12)
pkg/model/model/message.go (2)
21-21: Update Key example to reflect UUIDv7 (KSUID residue).The Key example still shows a KSUID suffix. Since MessageID is now UUIDv7, the example should match to avoid confusion.
- Key string `dynamodbav:"key" json:"key" desc:"Unique key for the message." example:"#message#550e8400-e29b-41d4-a716-446655440000#1sB5tZfLipTVWQWHVKnDFS6kFRK"` + Key string `dynamodbav:"key" json:"key" desc:"Unique key for the message." example:"#message#550e8400-e29b-41d4-a716-446655440000#01890ea2-0ba7-7c93-9d2e-07b4f24f8633"`
56-63: Optional: Skip key generation if ConversationID is empty.Prevents malformed keys like “#message##”. Consider early return when ConversationID == "".
pkg/model/model/message_test.go (6)
59-62: Strengthen: Assert UUID version is v7, not just “a UUID”.Add a version check to ensure we’re generating v7 IDs.
- // Verify UUIDv7 format - _, err := uuid.Parse(msg.MessageID) - assert.NoError(t, err, "MessageID should be a valid UUID") + // Verify UUIDv7 format + parsed, err := uuid.Parse(msg.MessageID) + assert.NoError(t, err, "MessageID should be a valid UUID") + assert.Equal(t, uuid.Version(7), parsed.Version(), "MessageID should be a UUIDv7")
80-89: Strengthen: Validate v7 in key-derived MessageID.Also assert version 7 for the ID extracted from the key.
// Verify it's a valid UUID - _, err := uuid.Parse(messageIDFromKey) - assert.NoError(t, err) + parsed, err := uuid.Parse(messageIDFromKey) + assert.NoError(t, err) + assert.Equal(t, uuid.Version(7), parsed.Version())
195-221: Reduce flakiness: increase delay and/or assert v7.1 ms sleep can be flaky on some CI clocks. Also assert v7 for clarity.
- // Create messages with small delays to ensure ordering + // Create messages with small delays to ensure ordering for i := 0; i < 5; i++ { msg := NewMessage(conversationID, "user", "Message "+string(rune('A'+i)), username) messages = append(messages, msg) - parsed, err := uuid.Parse(msg.MessageID) + parsed, err := uuid.Parse(msg.MessageID) require.NoError(t, err) - uuids = append(uuids, parsed) + require.Equal(t, uuid.Version(7), parsed.Version()) + uuids = append(uuids, parsed) // Small delay to ensure different timestamps - time.Sleep(1 * time.Millisecond) + time.Sleep(5 * time.Millisecond) } // Verify UUIDs are in ascending order (chronological for UUIDv7) for i := 1; i < len(uuids); i++ { // UUIDv7 has timestamp in most significant bits, so string comparison works assert.True(t, uuids[i-1].String() <= uuids[i].String(), "UUID %d should be <= UUID %d chronologically", i-1, i) }
233-244: Minor: Prefer comparing parsed UUIDs over strings.Comparing strings works but parsing once in the sort avoids repeated conversions and is more explicit.
- // Sort by MessageID (UUID) - this should put them back in chronological order - sort.Slice(shuffled, func(i, j int) bool { - return shuffled[i].MessageID < shuffled[j].MessageID - }) + // Sort by parsed UUID (chronological for UUIDv7) + sort.Slice(shuffled, func(i, j int) bool { + ui, _ := uuid.Parse(shuffled[i].MessageID) + uj, _ := uuid.Parse(shuffled[j].MessageID) + return ui.String() < uj.String() + }) // Sort original messages by UUID too for comparison originalSorted := make([]Message, len(messages)) copy(originalSorted, messages) sort.Slice(originalSorted, func(i, j int) bool { - return originalSorted[i].MessageID < originalSorted[j].MessageID + ui, _ := uuid.Parse(originalSorted[i].MessageID) + uj, _ := uuid.Parse(originalSorted[j].MessageID) + return ui.String() < uj.String() })
271-274: Strengthen: Also assert UUIDv7 version.- _, err := uuid.Parse(msg.MessageID) - assert.NoError(t, err) + parsed, err := uuid.Parse(msg.MessageID) + assert.NoError(t, err) + assert.Equal(t, uuid.Version(7), parsed.Version())
364-367: Strengthen: Assert UUIDv7 version in security tests.- _, err := uuid.Parse(msg.MessageID) - assert.NoError(t, err) + parsed, err := uuid.Parse(msg.MessageID) + assert.NoError(t, err) + assert.Equal(t, uuid.Version(7), parsed.Version())pkg/model/model/origination_data_test.go (1)
141-161: Replace manual bubble sort with sort.Strings.The manual sorting implementation is inefficient and verbose.
Use the standard library:
+import ( + "reflect" + "sort" + "testing" +) - // Sort slices for comparison since order might vary due to map iteration - for i := range tt.initial.Capability { - for j := range tt.initial.Capability { - if i < j && tt.initial.Capability[i] > tt.initial.Capability[j] { - tt.initial.Capability[i], tt.initial.Capability[j] = tt.initial.Capability[j], tt.initial.Capability[i] - } - } - } - for i := range tt.initial.AttackSurface { - for j := range tt.initial.AttackSurface { - if i < j && tt.initial.AttackSurface[i] > tt.initial.AttackSurface[j] { - tt.initial.AttackSurface[i], tt.initial.AttackSurface[j] = tt.initial.AttackSurface[j], tt.initial.AttackSurface[i] - } - } - } - for i := range tt.initial.Origins { - for j := range tt.initial.Origins { - if i < j && tt.initial.Origins[i] > tt.initial.Origins[j] { - tt.initial.Origins[i], tt.initial.Origins[j] = tt.initial.Origins[j], tt.initial.Origins[i] - } - } - } + // Sort slices for comparison since order might vary due to map iteration + sort.Strings(tt.initial.Capability) + sort.Strings(tt.initial.AttackSurface) + sort.Strings(tt.initial.Origins)pkg/model/model/cloud_resource.go (1)
148-168: Consider documenting Visit return type mismatch.Visit doesn't return an error but AWS/GCP/Azure Visit methods do. This asymmetry might cause confusion.
Either:
- Add error return to CloudResource.Visit for consistency
- Document why CloudResource.Visit doesn't need error handling
- Update resource-specific Visit methods to not return errors
pkg/model/model/base_asset.go (2)
214-224: Consider nil-safe merge for defensive programming.While the current implementation works, consider making Merge more defensive against nil receivers.
func (o *OriginationData) Merge(other OriginationData) { + if o == nil { + return + } if other.Origins != nil { o.Origins = other.Origins }
226-244: Consider using slices.Sort for deterministic output.The Visit method correctly deduplicates slices but produces non-deterministic ordering due to map key iteration.
For more predictable test results and debugging:
+import "slices" // Already imported func (o *OriginationData) Visit(other OriginationData) { seen := make(map[string]bool) for _, s := range append(o.Origins, other.Origins...) { seen[s] = true } o.Origins = slices.Collect(maps.Keys(seen)) + slices.Sort(o.Origins) seen = make(map[string]bool) for _, s := range append(o.AttackSurface, other.AttackSurface...) { seen[s] = true } o.AttackSurface = slices.Collect(maps.Keys(seen)) + slices.Sort(o.AttackSurface) seen = make(map[string]bool) for _, s := range append(o.Capability, other.Capability...) { seen[s] = true } o.Capability = slices.Collect(maps.Keys(seen)) + slices.Sort(o.Capability) }
📜 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 (11)
pkg/model/model/aws_resource.go(1 hunks)pkg/model/model/azure_resource.go(1 hunks)pkg/model/model/base_asset.go(1 hunks)pkg/model/model/base_asset_test.go(5 hunks)pkg/model/model/cloud_resource.go(2 hunks)pkg/model/model/cloud_resource_test.go(2 hunks)pkg/model/model/conversation_test.go(0 hunks)pkg/model/model/gcp_resource.go(1 hunks)pkg/model/model/message.go(2 hunks)pkg/model/model/message_test.go(11 hunks)pkg/model/model/origination_data_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/model/model/conversation_test.go
🧰 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/aws_resource.gopkg/model/model/base_asset_test.gopkg/model/model/gcp_resource.gopkg/model/model/message_test.gopkg/model/model/cloud_resource.gopkg/model/model/origination_data_test.gopkg/model/model/cloud_resource_test.gopkg/model/model/message.gopkg/model/model/azure_resource.gopkg/model/model/base_asset.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/base_asset_test.gopkg/model/model/message_test.gopkg/model/model/origination_data_test.gopkg/model/model/cloud_resource_test.go
🧠 Learnings (1)
📚 Learning: 2025-08-30T00:29:52.572Z
Learnt from: CR
PR: praetorian-inc/tabularium#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:29:52.572Z
Learning: Applies to pkg/model/{model,attacksurface,beta}/**/*.go : Model structs must embed BaseAsset
Applied to files:
pkg/model/model/base_asset.go
🧬 Code graph analysis (8)
pkg/model/model/aws_resource.go (1)
pkg/model/model/cloud_resource.go (1)
CloudResource(27-50)
pkg/model/model/base_asset_test.go (1)
pkg/model/model/base_asset.go (2)
Metadata(183-206)OriginationData(208-212)
pkg/model/model/gcp_resource.go (1)
pkg/model/model/cloud_resource.go (1)
CloudResource(27-50)
pkg/model/model/message_test.go (1)
pkg/model/model/message.go (2)
Message(17-30)NewMessage(74-84)
pkg/model/model/cloud_resource.go (1)
pkg/model/model/base_asset.go (1)
OriginationData(208-212)
pkg/model/model/origination_data_test.go (1)
pkg/model/model/base_asset.go (1)
OriginationData(208-212)
pkg/model/model/cloud_resource_test.go (3)
pkg/model/model/cloud_resource.go (1)
CloudResource(27-50)pkg/model/model/base_asset.go (1)
OriginationData(208-212)pkg/model/model/aws_resource.go (1)
NewAWSResource(27-48)
pkg/model/model/azure_resource.go (1)
pkg/model/model/cloud_resource.go (1)
CloudResource(27-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-code-action / claude-code-action
🔇 Additional comments (19)
pkg/model/model/message.go (2)
26-26: LGTM: MessageID docs and example align with UUIDv7.The field’s description and example reflect the new ordering semantics.
41-41: LGTM: Updated description accurately reflects UUIDv7 ordering.Matches the implementation and tests.
pkg/model/model/message_test.go (3)
11-11: LGTM: Add uuid import for parsing/validation.
43-45: LGTM: Description expectation updated to UUIDv7.
174-175: LGTM: Valid() semantics accept missing username.Matches implementation (Valid checks conversationID, role, content).
If any API layer requires Username, ensure upstream validation enforces it.
pkg/model/model/aws_resource.go (1)
108-109: LGTM! Clean delegation to CloudResource.The refactor properly delegates merge logic to the embedded CloudResource, maintaining clean separation of concerns.
pkg/model/model/gcp_resource.go (2)
94-95: LGTM! Consistent delegation pattern.Clean delegation to CloudResource.Merge following the same pattern as AWS.
102-103: Same error handling issue as AWSResource.
CloudResource.Visitcall doesn't propagate errors, matching the same issue in AWSResource.pkg/model/model/origination_data_test.go (1)
164-184: Apply same sorting simplification here.Replace with:
- // Sort expected for comparison too - for i := range tt.expected.Capability { - for j := range tt.expected.Capability { - if i < j && tt.expected.Capability[i] > tt.expected.Capability[j] { - tt.expected.Capability[i], tt.expected.Capability[j] = tt.expected.Capability[j], tt.expected.Capability[i] - } - } - } - for i := range tt.expected.AttackSurface { - for j := range tt.expected.AttackSurface { - if i < j && tt.expected.AttackSurface[i] > tt.expected.AttackSurface[j] { - tt.expected.AttackSurface[i], tt.expected.AttackSurface[j] = tt.expected.AttackSurface[j], tt.expected.AttackSurface[i] - } - } - } - for i := range tt.expected.Origins { - for j := range tt.expected.Origins { - if i < j && tt.expected.Origins[i] > tt.expected.Origins[j] { - tt.expected.Origins[i], tt.expected.Origins[j] = tt.expected.Origins[j], tt.expected.Origins[i] - } - } - } + // Sort expected for comparison too + sort.Strings(tt.expected.Capability) + sort.Strings(tt.expected.AttackSurface) + sort.Strings(tt.expected.Origins)pkg/model/model/cloud_resource.go (2)
131-146: Well-structured Merge implementation.Good defensive programming with nil checks and map initialization. The overwrite semantics are clear and appropriate for merge operations.
148-168: Clean Visit implementation with proper additive semantics.The Visit method correctly implements additive behavior - only updating missing properties and preserving TTL when other.TTL is zero.
pkg/model/model/azure_resource.go (2)
121-122: LGTM! Consistent delegation.Follows the same clean delegation pattern as AWS and GCP resources.
129-130: Same Visit error handling pattern.Consistent with AWS/GCP - doesn't propagate CloudResource.Visit errors.
pkg/model/model/cloud_resource_test.go (1)
619-671: LGTM! Well-structured test for OriginationData merge behavior.The test correctly validates that CloudResource.Merge properly delegates to OriginationData.Merge, replacing slices rather than appending them as expected for merge operations.
pkg/model/model/base_asset_test.go (2)
198-213: LGTM! Tests correctly updated for OriginationData nesting.The test properly validates that AttackSurface is now accessed via OriginationData.AttackSurface after the refactoring.
648-661: LGTM! Origins field access correctly updated.Tests properly reflect the new OriginationData structure for Origins field access.
pkg/model/model/base_asset.go (3)
205-206: LGTM! Clean embedding of OriginationData.Good design choice to embed OriginationData directly in Metadata, providing cleaner field access while maintaining the benefits of encapsulation.
208-212: LGTM! Well-documented OriginationData struct.Clear field definitions with appropriate Neo4j tags and helpful examples.
246-254: LGTM! Clean delegation to OriginationData methods.Good refactoring - Metadata now cleanly delegates slice operations to OriginationData while maintaining field updates separately.
| a.CloudResource.Visit(&other.CloudResource) | ||
| return nil |
There was a problem hiding this comment.
Visit method doesn't return the error from CloudResource.
The CloudResource.Visit method is called but any potential error it might return is not being propagated. While the current implementation might not return errors, this could lead to silent failures if the implementation changes.
Consider capturing and returning any error from the delegation:
- a.CloudResource.Visit(&other.CloudResource)
- return nil
+ return a.CloudResource.Visit(&other.CloudResource)Note: If CloudResource.Visit doesn't return an error, this AWS method signature should be updated to match for consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| a.CloudResource.Visit(&other.CloudResource) | |
| return nil | |
| func (a *AWSResource) Visit(other *AWSResource) error { | |
| - a.CloudResource.Visit(&other.CloudResource) | |
| return a.CloudResource.Visit(&other.CloudResource) | |
| } |
🤖 Prompt for AI Agents
In pkg/model/model/aws_resource.go around lines 116-117, the call
a.CloudResource.Visit(&other.CloudResource) ignores any error the delegated
Visit might return; capture its return value and propagate it (e.g., err :=
a.CloudResource.Visit(&other.CloudResource); if err != nil { return err } return
nil). If CloudResource.Visit currently has no error return, update its signature
to return error and adjust all callers accordingly so the AWS Visit can forward
that error.
| func TestCloudResource_OriginationDataVisit(t *testing.T) { | ||
| t.Run("CloudResource should visit OriginationData correctly", func(t *testing.T) { | ||
| resource1 := &CloudResource{ | ||
| Key: "test1", | ||
| Name: "test1", | ||
| Provider: "aws", | ||
| ResourceType: "instance", | ||
| Status: "active", | ||
| TTL: 0, // Uninitialized | ||
| Properties: map[string]any{"key1": "value1"}, | ||
| OriginationData: OriginationData{ | ||
| Capability: []string{"dns"}, | ||
| AttackSurface: []string{"internal"}, | ||
| Origins: []string{"dns"}, | ||
| }, | ||
| } | ||
|
|
||
| resource2 := &CloudResource{ | ||
| Key: "test2", | ||
| Name: "test2", | ||
| Provider: "aws", | ||
| ResourceType: "instance", | ||
| Status: "visited", | ||
| TTL: 12345, | ||
| Properties: map[string]any{"key2": "value2"}, | ||
| OriginationData: OriginationData{ | ||
| Capability: []string{"amazon", "portscan"}, | ||
| AttackSurface: []string{"external"}, | ||
| Origins: []string{"amazon", "ipv4"}, | ||
| }, | ||
| } | ||
|
|
||
| resource1.Visit(resource2) | ||
|
|
||
| if resource1.Status != "visited" { | ||
| t.Errorf("Expected status 'visited', got '%s'", resource1.Status) | ||
| } | ||
| if resource1.TTL != 12345 { | ||
| t.Errorf("Expected TTL 12345, got %d", resource1.TTL) | ||
| } | ||
|
|
||
| if resource1.Properties["key1"] != "value1" { | ||
| t.Errorf("Existing property should be preserved") | ||
| } | ||
| if resource1.Properties["key2"] != "value2" { | ||
| t.Errorf("New property should be added") | ||
| } | ||
| sortStringSlice := func(s []string) { | ||
| for i := range s { | ||
| for j := range s { | ||
| if i < j && s[i] > s[j] { | ||
| s[i], s[j] = s[j], s[i] | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| sortStringSlice(resource1.Capability) | ||
| sortStringSlice(resource1.AttackSurface) | ||
| sortStringSlice(resource1.Origins) | ||
|
|
||
| expectedCapability := []string{"amazon", "dns", "portscan"} | ||
| expectedAttackSurface := []string{"external", "internal"} | ||
| expectedOrigins := []string{"amazon", "dns", "ipv4"} | ||
|
|
||
| sortStringSlice(expectedCapability) | ||
| sortStringSlice(expectedAttackSurface) | ||
| sortStringSlice(expectedOrigins) | ||
|
|
||
| if !reflect.DeepEqual(resource1.Capability, expectedCapability) { | ||
| t.Errorf("Capability visit failed: expected %v, got %v", expectedCapability, resource1.Capability) | ||
| } | ||
| if !reflect.DeepEqual(resource1.AttackSurface, expectedAttackSurface) { | ||
| t.Errorf("AttackSurface visit failed: expected %v, got %v", expectedAttackSurface, resource1.AttackSurface) | ||
| } | ||
| if !reflect.DeepEqual(resource1.Origins, expectedOrigins) { | ||
| t.Errorf("Origins visit failed: expected %v, got %v", expectedOrigins, resource1.Origins) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Fix the inefficient bubble sort implementation.
The custom sorting function is an O(n²) bubble sort that's missing the swapped flag optimization and has incorrect loop bounds.
Replace the inefficient sorting with Go's standard library:
- sortStringSlice := func(s []string) {
- for i := range s {
- for j := range s {
- if i < j && s[i] > s[j] {
- s[i], s[j] = s[j], s[i]
- }
- }
- }
- }
-
- sortStringSlice(resource1.Capability)
- sortStringSlice(resource1.AttackSurface)
- sortStringSlice(resource1.Origins)
-
- expectedCapability := []string{"amazon", "dns", "portscan"}
- expectedAttackSurface := []string{"external", "internal"}
- expectedOrigins := []string{"amazon", "dns", "ipv4"}
-
- sortStringSlice(expectedCapability)
- sortStringSlice(expectedAttackSurface)
- sortStringSlice(expectedOrigins)
+ import "sort" // Add to imports at top of file
+
+ sort.Strings(resource1.Capability)
+ sort.Strings(resource1.AttackSurface)
+ sort.Strings(resource1.Origins)
+
+ expectedCapability := []string{"amazon", "dns", "portscan"}
+ expectedAttackSurface := []string{"external", "internal"}
+ expectedOrigins := []string{"amazon", "dns", "ipv4"}
+
+ sort.Strings(expectedCapability)
+ sort.Strings(expectedAttackSurface)
+ sort.Strings(expectedOrigins)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/model/model/cloud_resource_test.go around lines 673 to 752, the test
defines a custom O(n²) bubble-sort with wrong loop bounds; replace that custom
sorter by calling sort.Strings(...) on resource1.Capability,
resource1.AttackSurface, resource1.Origins and on expectedCapability,
expectedAttackSurface, expectedOrigins, remove the custom sortStringSlice
function and its usages, and add the "sort" package to the file imports.
| func TestAWSResource_OriginationDataIntegration(t *testing.T) { | ||
| t.Run("AWSResource should use CloudResource OriginationData merge/visit", func(t *testing.T) { | ||
| // Create AWS resources with OriginationData | ||
| resource1, err := NewAWSResource( | ||
| "arn:aws:ec2:us-east-1:123456789012:instance/i-1234567890abcdef0", | ||
| "123456789012", | ||
| AWSEC2Instance, | ||
| map[string]any{"InstanceId": "i-1234567890abcdef0"}, | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create AWSResource: %v", err) | ||
| } | ||
|
|
||
| // Set some origination data | ||
| resource1.OriginationData = OriginationData{ | ||
| Capability: []string{"amazon"}, | ||
| Origins: []string{"aws-account"}, | ||
| } | ||
|
|
||
| resource2, err := NewAWSResource( | ||
| "arn:aws:ec2:us-east-1:123456789012:instance/i-abcdef1234567890", | ||
| "123456789012", | ||
| AWSEC2Instance, | ||
| map[string]any{"InstanceId": "i-abcdef1234567890"}, | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create second AWSResource: %v", err) | ||
| } | ||
|
|
||
| resource2.OriginationData = OriginationData{ | ||
| Capability: []string{"portscan"}, | ||
| Origins: []string{"discovery"}, | ||
| } | ||
|
|
||
| resource1.Merge(&resource2) | ||
|
|
||
| expectedCapability := []string{"portscan"} | ||
| if !reflect.DeepEqual(resource1.Capability, expectedCapability) { | ||
| t.Errorf("AWS resource merge failed: expected %v, got %v", expectedCapability, resource1.Capability) | ||
| } | ||
|
|
||
| resource1.OriginationData.Capability = []string{"amazon"} | ||
| err = resource1.Visit(&resource2) | ||
| if err != nil { | ||
| t.Fatalf("Visit failed: %v", err) | ||
| } | ||
| sortStringSlice := func(s []string) { | ||
| for i := range s { | ||
| for j := range s { | ||
| if i < j && s[i] > s[j] { | ||
| s[i], s[j] = s[j], s[i] | ||
| } | ||
| } | ||
| } | ||
| } | ||
| sortStringSlice(resource1.Capability) | ||
| expectedCapability = []string{"amazon", "portscan"} | ||
| sortStringSlice(expectedCapability) | ||
|
|
||
| if !reflect.DeepEqual(resource1.Capability, expectedCapability) { | ||
| t.Errorf("AWS resource visit failed: expected %v, got %v", expectedCapability, resource1.Capability) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Duplicate inefficient sorting function.
Same bubble sort implementation issue as above.
Replace with standard library sort:
- sortStringSlice := func(s []string) {
- for i := range s {
- for j := range s {
- if i < j && s[i] > s[j] {
- s[i], s[j] = s[j], s[i]
- }
- }
- }
- }
- sortStringSlice(resource1.Capability)
+ sort.Strings(resource1.Capability)
expectedCapability = []string{"amazon", "portscan"}
- sortStringSlice(expectedCapability)
+ sort.Strings(expectedCapability)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestAWSResource_OriginationDataIntegration(t *testing.T) { | |
| t.Run("AWSResource should use CloudResource OriginationData merge/visit", func(t *testing.T) { | |
| // Create AWS resources with OriginationData | |
| resource1, err := NewAWSResource( | |
| "arn:aws:ec2:us-east-1:123456789012:instance/i-1234567890abcdef0", | |
| "123456789012", | |
| AWSEC2Instance, | |
| map[string]any{"InstanceId": "i-1234567890abcdef0"}, | |
| ) | |
| if err != nil { | |
| t.Fatalf("Failed to create AWSResource: %v", err) | |
| } | |
| // Set some origination data | |
| resource1.OriginationData = OriginationData{ | |
| Capability: []string{"amazon"}, | |
| Origins: []string{"aws-account"}, | |
| } | |
| resource2, err := NewAWSResource( | |
| "arn:aws:ec2:us-east-1:123456789012:instance/i-abcdef1234567890", | |
| "123456789012", | |
| AWSEC2Instance, | |
| map[string]any{"InstanceId": "i-abcdef1234567890"}, | |
| ) | |
| if err != nil { | |
| t.Fatalf("Failed to create second AWSResource: %v", err) | |
| } | |
| resource2.OriginationData = OriginationData{ | |
| Capability: []string{"portscan"}, | |
| Origins: []string{"discovery"}, | |
| } | |
| resource1.Merge(&resource2) | |
| expectedCapability := []string{"portscan"} | |
| if !reflect.DeepEqual(resource1.Capability, expectedCapability) { | |
| t.Errorf("AWS resource merge failed: expected %v, got %v", expectedCapability, resource1.Capability) | |
| } | |
| resource1.OriginationData.Capability = []string{"amazon"} | |
| err = resource1.Visit(&resource2) | |
| if err != nil { | |
| t.Fatalf("Visit failed: %v", err) | |
| } | |
| sortStringSlice := func(s []string) { | |
| for i := range s { | |
| for j := range s { | |
| if i < j && s[i] > s[j] { | |
| s[i], s[j] = s[j], s[i] | |
| } | |
| } | |
| } | |
| } | |
| sortStringSlice(resource1.Capability) | |
| expectedCapability = []string{"amazon", "portscan"} | |
| sortStringSlice(expectedCapability) | |
| if !reflect.DeepEqual(resource1.Capability, expectedCapability) { | |
| t.Errorf("AWS resource visit failed: expected %v, got %v", expectedCapability, resource1.Capability) | |
| } | |
| }) | |
| } | |
| func TestAWSResource_OriginationDataIntegration(t *testing.T) { | |
| t.Run("AWSResource should use CloudResource OriginationData merge/visit", func(t *testing.T) { | |
| // Create AWS resources with OriginationData | |
| resource1, err := NewAWSResource( | |
| "arn:aws:ec2:us-east-1:123456789012:instance/i-1234567890abcdef0", | |
| "123456789012", | |
| AWSEC2Instance, | |
| map[string]any{"InstanceId": "i-1234567890abcdef0"}, | |
| ) | |
| if err != nil { | |
| t.Fatalf("Failed to create AWSResource: %v", err) | |
| } | |
| // Set some origination data | |
| resource1.OriginationData = OriginationData{ | |
| Capability: []string{"amazon"}, | |
| Origins: []string{"aws-account"}, | |
| } | |
| resource2, err := NewAWSResource( | |
| "arn:aws:ec2:us-east-1:123456789012:instance/i-abcdef1234567890", | |
| "123456789012", | |
| AWSEC2Instance, | |
| map[string]any{"InstanceId": "i-abcdef1234567890"}, | |
| ) | |
| if err != nil { | |
| t.Fatalf("Failed to create second AWSResource: %v", err) | |
| } | |
| resource2.OriginationData = OriginationData{ | |
| Capability: []string{"portscan"}, | |
| Origins: []string{"discovery"}, | |
| } | |
| resource1.Merge(&resource2) | |
| expectedCapability := []string{"portscan"} | |
| if !reflect.DeepEqual(resource1.Capability, expectedCapability) { | |
| t.Errorf("AWS resource merge failed: expected %v, got %v", expectedCapability, resource1.Capability) | |
| } | |
| resource1.OriginationData.Capability = []string{"amazon"} | |
| err = resource1.Visit(&resource2) | |
| if err != nil { | |
| t.Fatalf("Visit failed: %v", err) | |
| } | |
| // Replace custom bubble‐sort with stdlib | |
| sort.Strings(resource1.Capability) | |
| expectedCapability = []string{"amazon", "portscan"} | |
| sort.Strings(expectedCapability) | |
| if !reflect.DeepEqual(resource1.Capability, expectedCapability) { | |
| t.Errorf("AWS resource visit failed: expected %v, got %v", expectedCapability, resource1.Capability) | |
| } | |
| }) | |
| } |
🤖 Prompt for AI Agents
In pkg/model/model/cloud_resource_test.go around lines 754 to 817 there is a
duplicated custom bubble-sort function used to sort string slices; replace both
uses by calling the standard library sort.Strings on the slices (e.g.,
sort.Strings(resource1.Capability) and sort.Strings(expectedCapability)), remove
the custom sortStringSlice function, and add the "sort" import to the test
file's import block.
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Analysis Results: Has Go, TypeScript, or Python Changes: Yes Findings• Major architectural changes: Introduction of Recommendations• Verify backward compatibility: Cloud resource changes remove significant code - ensure dependent systems handle the simplified structure |
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Analysis Results: Has Go, TypeScript, or Python Changes: Yes Findings• Major architectural changes: Introduction of Recommendations• Verify backward compatibility: Cloud resource changes remove significant code - ensure dependent systems handle the simplified structure |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/model/model/aws_resource.go (1)
33-47: Initialize Properties when nil to satisfy tests and avoid nil-map pitfalls.TestNewAWSResource("successful creation with nil properties") expects non-nil Properties. Initialize to an empty map when input is nil.
Outside this range, update NewAWSResource:
// inside NewAWSResource before constructing r props := properties if props == nil { props = map[string]any{} } r := AWSResource{ CloudResource: CloudResource{ Name: name, DisplayName: parsedARN.Resource, Provider: AWSProvider, Properties: props, // use non-nil map ResourceType: CloudResourceType(rtype), Region: parsedARN.Region, AccountRef: accountRef, }, }pkg/model/model/azure_resource.go (1)
77-99: Support both []string and []any for IP lists.Current code only handles []any. Real inputs may be []string; otherwise IPs are silently dropped.
Refactor GetIPs to accept both:
func (a *AzureResource) GetIPs() []string { ips := make([]string, 0) addIPs := func(v any) { switch vv := v.(type) { case []string: for _, s := range vv { if s != "" { ips = append(ips, s) } } case []any: for _, it := range vv { if s, ok := it.(string); ok && s != "" { ips = append(ips, s) } } } } if v, ok := a.Properties["privateIPs"]; ok { addIPs(v) } if v, ok := a.Properties["publicIPs"]; ok { addIPs(v) } return ips }
🧹 Nitpick comments (3)
pkg/model/model/aws_resource.go (1)
50-53: Defaulted should not clobber existing values.Guard assignments so existing Origins/AttackSurface aren’t overwritten during merges or re-defaulting.
Apply this diff:
func (a *AWSResource) Defaulted() { - a.Origins = []string{"amazon"} - a.AttackSurface = []string{"cloud"} + if len(a.Origins) == 0 { + a.Origins = []string{"amazon"} + } + if len(a.AttackSurface) == 0 { + a.AttackSurface = []string{"cloud"} + } }pkg/model/model/azure_resource.go (2)
25-42: Initialize Properties when nil to avoid nil-map issues.Mirror the AWS fix: ensure Properties is an empty map when input is nil.
Outside this range, in NewAzureResource:
props := properties if props == nil { props = map[string]any{} } r := AzureResource{ CloudResource: CloudResource{ Name: name, Provider: "azure", Properties: props, ResourceType: CloudResourceType(rtype), AccountRef: accountRef, }, }
55-59: Defaulted should not clobber existing values.Set defaults only when fields are empty.
Apply this diff:
func (a *AzureResource) Defaulted() { - a.Origins = []string{"azure"} - a.AttackSurface = []string{"cloud"} + if len(a.Origins) == 0 { + a.Origins = []string{"azure"} + } + if len(a.AttackSurface) == 0 { + a.AttackSurface = []string{"cloud"} + } }
📜 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 (6)
pkg/model/model/aws_resource.go(2 hunks)pkg/model/model/aws_resource_test.go(2 hunks)pkg/model/model/azure_resource.go(2 hunks)pkg/model/model/azure_resource_test.go(2 hunks)pkg/model/model/gcp_resource.go(2 hunks)pkg/model/model/gcp_resource_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/model/model/gcp_resource.go
🧰 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/aws_resource_test.gopkg/model/model/gcp_resource_test.gopkg/model/model/azure_resource_test.gopkg/model/model/azure_resource.gopkg/model/model/aws_resource.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/aws_resource_test.gopkg/model/model/gcp_resource_test.gopkg/model/model/azure_resource_test.go
🧬 Code graph analysis (5)
pkg/model/model/aws_resource_test.go (2)
pkg/model/model/aws_resource.go (2)
AWSResource(17-19)NewAWSResource(27-48)pkg/model/model/cloud_resource.go (1)
CloudResource(27-50)
pkg/model/model/gcp_resource_test.go (2)
pkg/model/model/gcp_resource.go (2)
GCPResource(15-17)NewGCPResource(24-43)pkg/model/model/cloud_resource.go (1)
CloudResource(27-50)
pkg/model/model/azure_resource_test.go (3)
pkg/model/model/azure_resource.go (2)
AzureResource(15-18)NewAzureResource(25-42)pkg/model/model/cloud_resource.go (1)
CloudResource(27-50)pkg/model/model/cloud_resource_type.go (1)
AzureVM(32-32)
pkg/model/model/azure_resource.go (1)
pkg/model/model/cloud_resource.go (1)
CloudResource(27-50)
pkg/model/model/aws_resource.go (1)
pkg/model/model/cloud_resource.go (1)
CloudResource(27-50)
🪛 GitHub Actions: Go
pkg/model/model/aws_resource_test.go
[error] 1-1: Tests failed in package 'model'. See go test output for details.
pkg/model/model/gcp_resource_test.go
[error] 1-1: Tests failed in package 'model'. See go test output for details.
pkg/model/model/azure_resource_test.go
[error] 1-1: Tests failed in package 'model'. See go test output for details.
pkg/model/model/azure_resource.go
[error] 1-1: Tests failed in package 'model'. See go test output for details.
pkg/model/model/aws_resource.go
[error] 1-1: Tests failed in package 'model'. See go test output for details.
🔇 Additional comments (11)
pkg/model/model/azure_resource_test.go (3)
6-6: Test framework policy: confirm testify usage.Coding guideline states: use the standard testing package; avoid additional test frameworks. These tests import/testify. Confirm if exceptions apply; otherwise, migrate to stdlib assertions.
As per coding guidelines
Also applies to: 10-10
509-519: LGTM: verifies Defaulted fields on NewAzureResource.Asserts Origins ["azure"] and AttackSurface ["cloud"] successfully.
521-561: LGTM: direct Defaulted() behavior covered.Good coverage for both direct call and constructor path.
pkg/model/model/aws_resource_test.go (2)
334-343: LGTM: Defaulted fields validated for AWS.Confirms Origins ["amazon"] and AttackSurface ["cloud"] after NewAWSResource.
928-967: LGTM: direct Defaulted() behavior for AWS.Direct invocation test is clear and mirrors constructor expectations.
pkg/model/model/gcp_resource_test.go (3)
8-8: Test framework policy: confirm testify usage.Guidelines require using testing only; these tests import/testify. Confirm repo policy or plan a follow-up to remove testify.
As per coding guidelines
444-453: LGTM: Defaulted fields validated for GCP constructor.Origins ["gcp"] and AttackSurface ["cloud"] checks look good.
455-495: LGTM: direct Defaulted() behavior for GCP.Covers direct call and constructor path consistently.
pkg/model/model/aws_resource.go (1)
116-123: Propagate potential error from CloudResource.Visit (repeat).You’re ignoring the delegated Visit’s error. Forward it for correctness and future-proofing.
If CloudResource.Visit returns error, apply:
func (a *AWSResource) Visit(otherModel any) error { @@ - a.CloudResource.Visit(&other.CloudResource) - return nil + return a.CloudResource.Visit(&other.CloudResource) }If it doesn’t currently return error, consider updating its signature and callers to enable propagation.
#!/bin/bash # Verify CloudResource.Visit signature and all Visit implementations rg -n -C2 -g '!**/vendor/**' -P 'func\s*\(\s*[^)]*\*CloudResource[^)]*\)\s*Visit\s*\(' rg -n -C2 -g '!**/vendor/**' -P 'func\s*\(\s*[^)]*\)\s*Visit\s*\(' pkg/model/model | sed -n '1,200p'pkg/model/model/azure_resource.go (2)
121-127: LGTM: Merge delegates to CloudResource.Keeps behavior centralized and consistent across providers.
129-136: No error to propagate: CloudResource.Visit has no error return, so there’s nothing to forward.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 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 (3)
pkg/model/model/aws_resource.go(2 hunks)pkg/model/model/azure_resource.go(2 hunks)pkg/model/model/gcp_resource.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/gcp_resource.gopkg/model/model/azure_resource.gopkg/model/model/aws_resource.go
🧬 Code graph analysis (3)
pkg/model/model/gcp_resource.go (1)
pkg/model/model/cloud_resource.go (1)
CloudResource(27-50)
pkg/model/model/azure_resource.go (1)
pkg/model/model/cloud_resource.go (1)
CloudResource(27-50)
pkg/model/model/aws_resource.go (1)
pkg/model/model/cloud_resource.go (1)
CloudResource(27-50)
🔇 Additional comments (3)
pkg/model/model/aws_resource.go (2)
117-124: Propagate error from delegated Visit and guard nil.The error from a.CloudResource.Visit(...) is dropped, causing silent failures; also suffers the same typed-nil issue as Merge.
Apply:
func (a *AWSResource) Visit(otherModel any) error { other, ok := otherModel.(*AWSResource) - if !ok { + if !ok || other == nil { return fmt.Errorf("expected *AWSResource, got %T", otherModel) } - a.CloudResource.Visit(&other.CloudResource) - return nil + return a.CloudResource.Visit(&other.CloudResource) }
50-54: Ordering safe as-is; CloudResource.Defaulted doesn’t touch Origins or AttackSurface. It only sets Status, Created and Visited, so your AWS defaults won’t be clobbered.pkg/model/model/gcp_resource.go (1)
45-49: Make Defaulted idempotent and preserve base defaults: call CloudResource.Defaulted() first, then append “gcp” to Origins and “cloud” to AttackSurface only if missing to avoid overwrites or duplicates. Manually verify CloudResource.Defaulted()’s implementation to ensure it doesn’t reset those slices.
| func (a *AWSResource) Merge(otherModel any) { | ||
| other, ok := otherModel.(*AWSResource) | ||
| if !ok { | ||
| return | ||
| } | ||
| a.Status = other.Status | ||
| a.Visited = other.Visited | ||
|
|
||
| // Safely copy properties with nil checks | ||
| if a.Properties == nil { | ||
| a.Properties = make(map[string]any) | ||
| } | ||
| if other.Properties != nil { | ||
| maps.Copy(a.Properties, other.Properties) | ||
| } | ||
| a.CloudResource.Merge(&other.CloudResource) | ||
| } |
There was a problem hiding this comment.
Nil-pointer panic risk when otherModel is a typed nil.
Type assertion on a nil *AWSResource yields ok=true with other==nil; accessing other.CloudResource will panic. Add a nil check.
Apply:
func (a *AWSResource) Merge(otherModel any) {
other, ok := otherModel.(*AWSResource)
- if !ok {
+ if !ok || other == nil {
return
}
a.CloudResource.Merge(&other.CloudResource)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (a *AWSResource) Merge(otherModel any) { | |
| other, ok := otherModel.(*AWSResource) | |
| if !ok { | |
| return | |
| } | |
| a.Status = other.Status | |
| a.Visited = other.Visited | |
| // Safely copy properties with nil checks | |
| if a.Properties == nil { | |
| a.Properties = make(map[string]any) | |
| } | |
| if other.Properties != nil { | |
| maps.Copy(a.Properties, other.Properties) | |
| } | |
| a.CloudResource.Merge(&other.CloudResource) | |
| } | |
| func (a *AWSResource) Merge(otherModel any) { | |
| other, ok := otherModel.(*AWSResource) | |
| if !ok || other == nil { | |
| return | |
| } | |
| a.CloudResource.Merge(&other.CloudResource) | |
| } |
🤖 Prompt for AI Agents
In pkg/model/model/aws_resource.go around lines 109 to 115, after performing the
type assertion other, ok := otherModel.(*AWSResource) add a nil-pointer guard:
if other == nil { return } so that a typed nil *AWSResource does not lead to a
panic when accessing other.CloudResource; keep the existing ok check and then
check other for nil before calling a.CloudResource.Merge(&other.CloudResource).
| func (a *AzureResource) Defaulted() { | ||
| a.Origins = []string{"azure"} | ||
| a.AttackSurface = []string{"cloud"} | ||
| a.CloudResource.Defaulted() | ||
| } |
There was a problem hiding this comment.
Keep provider defaults after delegating to the cloud base.
CloudResource.Defaulted() now populates Origins and AttackSurface with its own defaults. By setting the Azure-specific values before calling into the base, you risk them being overwritten (current implementation of CloudResource.Defaulted() replaces empty slices). Flip the order so the base defaults run first and then layer on the provider-specific entries.
func (a *AzureResource) Defaulted() {
- a.Origins = []string{"azure"}
- a.AttackSurface = []string{"cloud"}
- a.CloudResource.Defaulted()
+ a.CloudResource.Defaulted()
+ a.Origins = appendUnique(a.Origins, "azure")
+ a.AttackSurface = appendUnique(a.AttackSurface, "cloud")
}(Use the same helper you introduced in cloud_resource.go to keep the slices deduplicated.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (a *AzureResource) Defaulted() { | |
| a.Origins = []string{"azure"} | |
| a.AttackSurface = []string{"cloud"} | |
| a.CloudResource.Defaulted() | |
| } | |
| func (a *AzureResource) Defaulted() { | |
| a.CloudResource.Defaulted() | |
| a.Origins = appendUnique(a.Origins, "azure") | |
| a.AttackSurface = appendUnique(a.AttackSurface, "cloud") | |
| } |
🤖 Prompt for AI Agents
In pkg/model/model/azure_resource.go around lines 55 to 59, the Azure Defaulted
method sets provider-specific Origins and AttackSurface before calling
CloudResource.Defaulted which can overwrite them; change the order to call
a.CloudResource.Defaulted() first, then append or merge the provider-specific
entries ("azure" into Origins and "cloud" into AttackSurface) using the same
deduplication helper introduced in cloud_resource.go so existing base defaults
are preserved and slices remain unique.
| func (a *GCPResource) Merge(otherModel any) { | ||
| other, ok := otherModel.(*GCPResource) | ||
| if !ok { | ||
| return | ||
| } | ||
| a.Status = other.Status | ||
| a.Visited = other.Visited | ||
|
|
||
| // Safely copy properties with nil checks | ||
| if a.Properties == nil { | ||
| a.Properties = make(map[string]any) | ||
| } | ||
| if other.Properties != nil { | ||
| maps.Copy(a.Properties, other.Properties) | ||
| } | ||
| a.CloudResource.Merge(&other.CloudResource) | ||
| } |
There was a problem hiding this comment.
Guard against typed-nil input to prevent panic in Merge
Type assertion succeeds on a typed-nil *GCPResource; dereferencing other.CloudResource would panic. Add a nil check.
Apply this diff:
func (a *GCPResource) Merge(otherModel any) {
other, ok := otherModel.(*GCPResource)
- if !ok {
+ if !ok {
return
}
- a.CloudResource.Merge(&other.CloudResource)
+ if other == nil {
+ return
+ }
+ a.CloudResource.Merge(&other.CloudResource)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (a *GCPResource) Merge(otherModel any) { | |
| other, ok := otherModel.(*GCPResource) | |
| if !ok { | |
| return | |
| } | |
| a.Status = other.Status | |
| a.Visited = other.Visited | |
| // Safely copy properties with nil checks | |
| if a.Properties == nil { | |
| a.Properties = make(map[string]any) | |
| } | |
| if other.Properties != nil { | |
| maps.Copy(a.Properties, other.Properties) | |
| } | |
| a.CloudResource.Merge(&other.CloudResource) | |
| } | |
| func (a *GCPResource) Merge(otherModel any) { | |
| other, ok := otherModel.(*GCPResource) | |
| if !ok { | |
| return | |
| } | |
| if other == nil { | |
| return | |
| } | |
| a.CloudResource.Merge(&other.CloudResource) | |
| } |
🤖 Prompt for AI Agents
In pkg/model/model/gcp_resource.go around lines 95 to 101, the type assertion
can succeed on a typed-nil *GCPResource which will cause a panic when accessing
fields; after the assertion add a nil check (if other == nil { return }) before
calling a.CloudResource.Merge(&other.CloudResource) so the method safely returns
when a typed-nil is passed.
| func (a *GCPResource) Visit(otherModel any) error { | ||
| other, ok := otherModel.(*GCPResource) | ||
| if !ok { | ||
| return fmt.Errorf("expected *GCPResource, got %T", otherModel) | ||
| } | ||
| a.Visited = other.Visited | ||
| a.Status = other.Status | ||
|
|
||
| // Safely copy properties with nil checks | ||
| if a.Properties == nil { | ||
| a.Properties = make(map[string]any) | ||
| } | ||
| if other.Properties != nil { | ||
| maps.Copy(a.Properties, other.Properties) | ||
| } | ||
|
|
||
| // Fix TTL update logic: update if other has a valid TTL | ||
| if other.TTL != 0 { | ||
| a.TTL = other.TTL | ||
| } | ||
| a.CloudResource.Visit(&other.CloudResource) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Propagate CloudResource.Visit error and guard nil to avoid panic
Currently the error is dropped and a typed-nil other would panic. Return the error and handle nil.
Apply this diff:
func (a *GCPResource) Visit(otherModel any) error {
other, ok := otherModel.(*GCPResource)
if !ok {
return fmt.Errorf("expected *GCPResource, got %T", otherModel)
}
- a.CloudResource.Visit(&other.CloudResource)
- return nil
+ if other == nil {
+ return fmt.Errorf("nil *GCPResource provided to Visit")
+ }
+ return a.CloudResource.Visit(&other.CloudResource)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (a *GCPResource) Visit(otherModel any) error { | |
| other, ok := otherModel.(*GCPResource) | |
| if !ok { | |
| return fmt.Errorf("expected *GCPResource, got %T", otherModel) | |
| } | |
| a.Visited = other.Visited | |
| a.Status = other.Status | |
| // Safely copy properties with nil checks | |
| if a.Properties == nil { | |
| a.Properties = make(map[string]any) | |
| } | |
| if other.Properties != nil { | |
| maps.Copy(a.Properties, other.Properties) | |
| } | |
| // Fix TTL update logic: update if other has a valid TTL | |
| if other.TTL != 0 { | |
| a.TTL = other.TTL | |
| } | |
| a.CloudResource.Visit(&other.CloudResource) | |
| return nil | |
| } | |
| func (a *GCPResource) Visit(otherModel any) error { | |
| other, ok := otherModel.(*GCPResource) | |
| if !ok { | |
| return fmt.Errorf("expected *GCPResource, got %T", otherModel) | |
| } | |
| if other == nil { | |
| return fmt.Errorf("nil *GCPResource provided to Visit") | |
| } | |
| return a.CloudResource.Visit(&other.CloudResource) | |
| } |
🤖 Prompt for AI Agents
In pkg/model/model/gcp_resource.go around lines 103 to 110, the method drops the
error from CloudResource.Visit and will panic if other is a typed nil; update
the type-assertion check to ensure other != nil after asserting (*GCPResource),
and propagate the Visit error by returning it instead of discarding it (e.g. if
ok and other != nil call return a.CloudResource.Visit(&other.CloudResource) and
otherwise return the existing fmt.Errorf). Ensure the function returns the error
from CloudResource.Visit so callers can handle it.
1 participant
No description provided.