Conversation
WalkthroughDependencies updated across multiple modules including AWS SDK, Kubernetes client, OpenTelemetry, and various libraries. OpenSearch log search enhanced with scroll pagination and JSON field preprocessing capabilities. Pretty-printing methods systematically added to model types for text rendering via the clicky library. Changes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
🧹 Nitpick comments (2)
logs/opensearch/search.go (1)
24-30: Interface return type mismatch with implementation.
RawClientMixindeclaresGetRawClient() anybut the implementation returns*opensearch.Client. While Go allows this (concrete types satisfyany), it's inconsistent. Consider whether the interface should be more specific or if the implementation should returnanyfor true interface compliance.-type RawClientMixin interface { - GetRawClient() any -} - -func (t *Searcher) GetRawClient() *opensearch.Client { +type RawClientMixin interface { + GetRawClient() *opensearch.Client +} + +func (t *Searcher) GetRawClient() *opensearch.Client {models/components.go (1)
133-162: Unusedoptsparameter.The
opts interface{}parameter is declared but never used within the method body. Consider removing it if not needed, or document its intended purpose if it's reserved for future use.-func (c Component) PrettyRow(opts interface{}) map[string]api.Text { +func (c Component) PrettyRow() map[string]api.Text {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
go.mod(13 hunks)logs/opensearch/search.go(5 hunks)logs/opensearch/search_test.go(1 hunks)logs/opensearch/types.go(3 hunks)models/canary.go(1 hunks)models/checks.go(4 hunks)models/common.go(4 hunks)models/components.go(2 hunks)models/config.go(6 hunks)models/integrations.go(2 hunks)models/job_history.go(2 hunks)models/permission.go(2 hunks)models/playbooks.go(5 hunks)types/resource_selector.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Only add comments if really necessary; do not add comments that simply explain the code, except for function comments which are considered good practice in Go
Files:
models/job_history.gomodels/permission.gomodels/components.gologs/opensearch/search_test.gomodels/checks.gomodels/integrations.gotypes/resource_selector.gologs/opensearch/types.gomodels/playbooks.gomodels/config.gomodels/common.gologs/opensearch/search.gomodels/canary.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Always usegithub.com/onsi/gomegapackage for assertions in test files
When using gomega with native go tests, useg := gomega.NewWithT(t)approach
Files:
logs/opensearch/search_test.go
🧠 Learnings (2)
📚 Learning: 2025-11-24T18:22:22.382Z
Learnt from: CR
Repo: flanksource/duty PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:22:22.382Z
Learning: Applies to tests/rls_test.go : Add comprehensive RLS test cases to tests/rls_test.go covering access granted scenarios, access denied scenarios, and edge cases
Applied to files:
logs/opensearch/search_test.go
📚 Learning: 2025-11-24T18:22:22.382Z
Learnt from: CR
Repo: flanksource/duty PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T18:22:22.382Z
Learning: Applies to models/**/*.go : Database model structs are defined in models/ directory
Applied to files:
models/checks.gomodels/common.go
🧬 Code graph analysis (10)
models/job_history.go (1)
models/components.go (1)
Text(560-565)
models/permission.go (1)
models/components.go (1)
Text(560-565)
logs/opensearch/search_test.go (3)
logs/opensearch/search.go (1)
Searcher(18-22)logs/config.go (1)
FieldMappingConfig(8-16)logs/opensearch/types.go (3)
Response(76-81)HitsInfo(47-51)SearchHit(83-90)
models/checks.go (3)
models/components.go (1)
Text(560-565)models/common.go (1)
Severity(25-25)types/duration.go (1)
Duration(9-11)
models/integrations.go (1)
models/components.go (1)
Text(560-565)
types/resource_selector.go (1)
relationship_selector.go (1)
Lookup(19-23)
models/playbooks.go (1)
models/components.go (1)
Text(560-565)
models/common.go (2)
models/components.go (1)
Text(560-565)relationship_selector.go (1)
Lookup(19-23)
logs/opensearch/search.go (3)
logs/opensearch/types.go (4)
Backend(13-17)Request(20-24)ScrollRequest(37-40)Response(76-81)logs/config.go (1)
FieldMappingConfig(8-16)logs/logs.go (1)
LogResult(93-96)
models/canary.go (1)
relationship_selector.go (1)
Lookup(19-23)
🪛 GitHub Actions: Lint
models/config.go
[error] 824-824: golangci-lint (staticcheck): QF1003: could use tagged switch on c.Status.
🪛 GitHub Actions: Test
logs/opensearch/search_test.go
[error] 1-1: Ginkgo test suite failed for 'opensearch': there were failures detected in the suite and a hint that no test suites were bootstrapped ("Found no test suites, did you forget to run "ginkgo bootstrap"?").
logs/opensearch/types.go
[error] 1-1: Ginkgo test suite failed for 'opensearch': there were failures detected in the suite and a hint that no test suites were bootstrapped ("Found no test suites, did you forget to run "ginkgo bootstrap"?").
logs/opensearch/search.go
[error] 1-1: Ginkgo test suite failed for 'opensearch': there were failures detected in the suite and a hint that no test suites were bootstrapped ("Found no test suites, did you forget to run "ginkgo bootstrap"?").
🪛 GitHub Check: lint
models/config.go
[failure] 824-824:
QF1003: could use tagged switch on c.Status (staticcheck)
🪛 OSV Scanner (2.3.0)
go.mod
[CRITICAL] 38-38: github.com/go-git/go-git/v5 5.12.0: Clients vulnerable to DoS via maliciously crafted Git server replies in github.com/go-git/go-git
(GO-2025-3367)
[CRITICAL] 38-38: github.com/go-git/go-git/v5 5.12.0: Argument Injection via the URL field in github.com/go-git/go-git
(GO-2025-3368)
[CRITICAL] 38-38: github.com/go-git/go-git/v5 5.12.0: go-git clients vulnerable to DoS via maliciously crafted Git server replies
[CRITICAL] 38-38: github.com/go-git/go-git/v5 5.12.0: go-git has an Argument Injection via the URL field
🔇 Additional comments (24)
logs/opensearch/types.go (2)
26-40: LGTM! Well-documented scroll types.The
ScrollOptionsandScrollRequesttypes are cleanly structured with appropriate field documentation.
80-80: LGTM!The
ScrollIDfield correctly uses the OpenSearch JSON tag_scroll_idfor proper deserialization.logs/opensearch/search_test.go (2)
150-173: LGTM - good in-place modification test.This test correctly validates that
preprocessJSONFieldsmodifies the map in place and preserves unrelated fields. However, it should also be refactored to use gomega assertions for consistency.
175-242: Integration test for parseSearchResponse is comprehensive.Good coverage of JSON field preprocessing and label flattening. Note that the
metadata@inputtest data (lines 190, 224-228) will fail due to the@inputsuffix not being handled in the implementation.logs/opensearch/search.go (5)
37-41: LGTM - Conditional verbose logging.Good practice to only enable the transport logger when the context logger is sufficiently verbose.
131-178: LGTM - Well-implemented scroll search with sensible defaults.The scroll implementation correctly:
- Provides default size (1000) and timeout (1 minute)
- Validates the required index field
- Handles error responses with body content
- Returns the scroll ID for subsequent calls
180-212: LGTM - ScrollNext correctly continues scroll context.Proper handling of scroll continuation with timeout refresh.
214-234: LGTM - ClearScroll properly cleans up resources.Good practice to provide a method for explicitly clearing scroll contexts to free server-side resources.
261-292: LGTM - Clean response parsing with field mapping.The
parseSearchResponsemethod correctly:
- Pre-allocates the slice with appropriate capacity
- Falls back to defaults when no mapping config is provided
- Preprocesses JSON fields before mapping
- Logs warnings for mapping errors without failing the entire operation
models/permission.go (1)
46-67: LGTM!The
Pretty()method correctly handles all definedPermissionSubjectTypeconstants with appropriate emoji icons and includes a sensible default case for unknown types. The implementation follows the same pattern used consistently across the codebase.models/job_history.go (1)
30-49: LGTM!The
Pretty()method provides appropriate visual representations for allJobStatusconstants with intuitive icons and color styling. The implementation is clean and consistent with the pattern used in other model files.models/integrations.go (1)
19-32: LGTM!The
Pretty()method provides clear visual representation for allIntegrationTypeconstants with semantically appropriate icons and styling. The implementation is consistent with the project-wide pattern.models/canary.go (1)
165-171: LGTM!The
Lookupmethod signature update to use named return values(value string, exists bool)improves readability and aligns with the consistent pattern applied across other matchers in this PR.models/components.go (1)
101-131: LGTM!The
Pretty()method provides a comprehensive textual representation of a Component with proper nil-checking for optional fields likeHealth. The label iteration is acceptable for display purposes.models/common.go (3)
35-50: LGTM!The
Severity.Pretty()method correctly maps all severity levels to appropriate styling with escalating visual prominence (uppercase bold red for critical, down to plain gray for info). The default case handles edge cases gracefully.
61-74: LGTM!The
Health.Pretty()method provides intuitive visual indicators (✓, ✗, !, ?) with appropriate color coding for each health state. The implementation is clean and consistent.
178-180: LGTM!The
Lookupmethod signature update to named return values maintains consistency with other matcher implementations in the codebase.types/resource_selector.go (3)
679-692: LGTM!The new
Lookupmethod onGenericFieldMatchercorrectly mirrors theGetmethod's type handling logic while adding the existence check. The JSON marshaling fallback for non-string values is consistent with the existing pattern.
707-711: LGTM!Clean implementation using named return values with idiomatic bare return.
726-732: LGTM!The
Lookupmethod correctly handles themap[string]anytype with proper existence checking and consistent string conversion viafmt.Sprintf.models/checks.go (1)
7-8: LGTM! Clean implementation of Pretty methods.The Pretty and PrettyRow methods provide consistent UI rendering with appropriate icons, styling, and conditional formatting. The code handles nil values properly and follows Go conventions.
Also applies to: 26-35, 83-131, 328-392
models/playbooks.go (1)
9-10: LGTM! Consistent Pretty method implementation.The Pretty and PrettyRow methods for playbooks follow the same patterns established in other model files. Time duration calculations and nil pointer handling are correct.
Also applies to: 42-67, 84-105, 161-212, 321-362
models/config.go (2)
9-10: LGTM! Well-structured Pretty methods.The Pretty and PrettyRow implementations for AnalysisType, RelatedConfigDirection, ConfigItem, ConfigChange, and ConfigAnalysis provide consistent UI rendering with appropriate icons and styling.
Also applies to: 74-99, 108-117, 206-259, 694-732, 809-854
488-499: LGTM! Improved type safety.The updated
Lookupmethod now properly handles missing keys and usesfmt.Sprintfto safely convert values to strings, which is more robust than direct casting.
| github.com/go-git/go-git/v5 v5.16.3 | ||
| github.com/flanksource/is-healthy v1.0.82 | ||
| github.com/glebarez/sqlite v1.7.0 | ||
| github.com/go-git/go-git/v5 v5.12.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
go-git v5 vulnerabilities GO-2025-3367 GO-2025-3368 GHSA-r9px-m959-cxf4 GHSA-v725-9546-7q7m
💡 Result:
Summary — go-git v5 vulnerabilities (GO-2025-3367, GO-2025-3368 / GHSA-r9px-m959-cxf4, GHSA-v725-9546-7q7m)
-
GO-2025-3367 (alias CVE-2025-21614 / GHSA-r9px-m959-cxf4): denial-of-service via specially crafted Git server replies; affects go-git v4 and v5 before v5.13.0. Severity: High (CVSS 7.5). Mitigation: upgrade to v5.13.0; otherwise restrict use to trusted Git servers. [1][2]
-
GO-2025-3368 (alias CVE-2025-21613 / GHSA-v725-9546-7q7m): argument-injection via the URL field when using the file transport (shells out to git), allowing attacker-controlled git-upload-pack flags; affects go-git < v5.13.0. Mitigation: upgrade to v5.13.0; or validate/restrict URL inputs if you cannot upgrade. Severity: Critical per advisory. [3][4]
Quick action steps
- If using github.com/go-git/go-git/v5, upgrade to v5.13.0. [1][3]
- If you cannot upgrade immediately: only use trusted Git servers (for DO S) and validate/restrict URL fields (for argument injection). [1][3]
- Track advisories and CVE entries for further updates (CVE-2025-21614, CVE-2025-21613). [2][4]
Sources
[1] GitHub Advisory GHSA-r9px-m959-cxf4 (go-git DoS).
[2] OSV / NVD / pkg.go.dev entries for GO-2025-3367 / CVE-2025-21614.
[3] GitHub Advisory GHSA-v725-9546-7q7m (argument injection).
[4] GO vulnerability entry for GO-2025-3368 / CVE-2025-21613.
🌐 Web query:
github.com/go-git/go-git/v5 latest version security patch 2025
💡 Result:
The go-git security advisories show the fixes were released in v5.13.0 (published Jan 5–6, 2025). Upgrade if you run < 5.13.0 — both the argument-injection (CVE-2025-21613) and DoS (CVE-2025-21614) advisories list 5.13.0 as the patched version. [1][2]
Sources:
[1] GitHub Advisory GHSA-v725-9546-7q7m (Argument injection) — patched in 5.13.0.
[2] GitHub Advisory GHSA-r9px-m959-cxf4 (DoS) — patched in 5.13.0.
Upgrade go-git to v5.13.0 to address critical security vulnerabilities.
The current version v5.12.0 is vulnerable to:
- CVE-2025-21614 (GO-2025-3367, GHSA-r9px-m959-cxf4): Denial-of-service via specially crafted Git server replies (Severity: High, CVSS 7.5)
- CVE-2025-21613 (GO-2025-3368, GHSA-v725-9546-7q7m): Argument injection via URL field when using file transport, enabling attacker-controlled flags (Severity: Critical)
Update to v5.13.0 (released Jan 5–6, 2025) which patches both vulnerabilities.
🧰 Tools
🪛 OSV Scanner (2.3.0)
[CRITICAL] 38-38: github.com/go-git/go-git/v5 5.12.0: Clients vulnerable to DoS via maliciously crafted Git server replies in github.com/go-git/go-git
(GO-2025-3367)
[CRITICAL] 38-38: github.com/go-git/go-git/v5 5.12.0: Argument Injection via the URL field in github.com/go-git/go-git
(GO-2025-3368)
[CRITICAL] 38-38: github.com/go-git/go-git/v5 5.12.0: go-git clients vulnerable to DoS via maliciously crafted Git server replies
[CRITICAL] 38-38: github.com/go-git/go-git/v5 5.12.0: go-git has an Argument Injection via the URL field
🤖 Prompt for AI Agents
In go.mod around line 38, the project pins github.com/go-git/go-git/v5 to
v5.12.0 which contains critical security vulnerabilities; update the dependency
version to v5.13.0 in go.mod and run go mod tidy (and optionally go get
github.com/go-git/go-git/v5@v5.13.0) to refresh the go.sum so the new version is
recorded and dependency checks pass.
| package opensearch | ||
|
|
||
| import ( | ||
| "reflect" | ||
| "testing" | ||
|
|
||
| dutyContext "github.com/flanksource/duty/context" | ||
| "github.com/flanksource/duty/logs" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use gomega for test assertions per coding guidelines.
The tests use reflect.DeepEqual and t.Errorf for assertions. As per coding guidelines, test files should use github.com/onsi/gomega with the g := gomega.NewWithT(t) approach. This is also likely contributing to the pipeline failure mentioning Ginkgo test suite issues.
Example refactor for one test case:
import (
- "reflect"
"testing"
dutyContext "github.com/flanksource/duty/context"
"github.com/flanksource/duty/logs"
+ "github.com/onsi/gomega"
)
func TestPreprocessJSONFields(t *testing.T) {
+ g := gomega.NewWithT(t)
// ... in the test loop:
- if !reflect.DeepEqual(input, tt.expected) {
- t.Errorf("preprocessJSONFields() got = %v, want %v", input, tt.expected)
- }
+ g.Expect(input).To(gomega.Equal(tt.expected))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Test
[error] 1-1: Ginkgo test suite failed for 'opensearch': there were failures detected in the suite and a hint that no test suites were bootstrapped ("Found no test suites, did you forget to run "ginkgo bootstrap"?").
🤖 Prompt for AI Agents
In logs/opensearch/search_test.go around lines 1 to 9, the tests use
reflect.DeepEqual and t.Errorf for assertions which violates the coding
guidelines and breaks the Ginkgo/Gomega test suite; replace these with Gomega
style assertions by importing "github.com/onsi/gomega", creating g :=
gomega.NewWithT(t) in each test, and then replace reflect.DeepEqual checks and
t.Errorf calls with g.Expect(actual).To(gomega.Equal(expected)) (or other
appropriate Gomega matchers). Also update imports to remove unused reflect and
ensure gomega is added.
| { | ||
| name: "valid JSON in @input field", | ||
| input: map[string]any{ | ||
| "message": "test message", | ||
| "payload@input": `{"users": ["alice", "bob"], "active": true}`, | ||
| "other_field": "normal value", | ||
| }, | ||
| expected: map[string]any{ | ||
| "message": "test message", | ||
| "payload@input": map[string]any{"users": []any{"alice", "bob"}, "active": true}, | ||
| "other_field": "normal value", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Test expects @input suffix handling, but implementation only handles @json.
The test case expects payload@input to be unmarshalled into a map, but preprocessJSONFields in search.go (line 241) only checks for the @json suffix. This test will fail.
Either update the implementation to also handle @input suffix, or remove these test cases if @input handling isn't intended.
If @input handling is intended, apply this fix to search.go:
- if !strings.HasSuffix(key, "@json") {
+ if !strings.HasSuffix(key, "@json") && !strings.HasSuffix(key, "@input") {
continue
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In logs/opensearch/search.go around line 241, preprocessJSONFields currently
only treats keys with the "@json" suffix as JSON to unmarshal; update it to also
recognize the "@input" suffix (e.g., treat keys ending with "@input" same as
"@json") so the test case for "payload@input" succeeds: adjust the suffix check
to accept both "@json" and "@input", remove the suffix appropriately before
unmarshalling, and leave other keys unchanged; ensure error handling and the
resulting map types remain the same as existing "@json" behavior.
| // preprocessJSONFields attempts to unmarshal JSON from fields ending with @json | ||
| // It modifies the input map in place, replacing string values with unmarshalled JSON where possible | ||
| func preprocessJSONFields(source map[string]any) { | ||
| for key, value := range source { | ||
| // Check if field name ends with @json | ||
| if !strings.HasSuffix(key, "@json") { | ||
| continue | ||
| } | ||
|
|
||
| // Only attempt to unmarshal string values | ||
| strValue, ok := value.(string) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| // Attempt to unmarshal the JSON string | ||
| var jsonValue any | ||
| if err := json.Unmarshal([]byte(strValue), &jsonValue); err == nil { | ||
| // Successfully unmarshalled, replace the value | ||
| source[key] = jsonValue | ||
| } | ||
| // On error, leave the original string value unchanged (treat as text) | ||
| } | ||
| } |
There was a problem hiding this comment.
Implementation only handles @json suffix, but tests expect @input too.
The function comment mentions "fields ending with @JSON" but the test file includes test cases for @input suffix fields (e.g., payload@input, metadata@input). The implementation at line 241 only checks for @json.
-// preprocessJSONFields attempts to unmarshal JSON from fields ending with @json
+// preprocessJSONFields attempts to unmarshal JSON from fields ending with @json or @input
// It modifies the input map in place, replacing string values with unmarshalled JSON where possible
func preprocessJSONFields(source map[string]any) {
for key, value := range source {
- // Check if field name ends with @json
- if !strings.HasSuffix(key, "@json") {
+ // Check if field name ends with @json or @input
+ if !strings.HasSuffix(key, "@json") && !strings.HasSuffix(key, "@input") {
continue
}🤖 Prompt for AI Agents
In logs/opensearch/search.go around lines 236 to 259, the function only checks
for the "@json" suffix but tests expect it to treat fields with "@input" the
same way; update the suffix check to handle both "@json" and "@input" (e.g., if
strings.HasSuffix(key, "@json") || strings.HasSuffix(key, "@input")), keep the
same behavior of only unmarshalling string values and replacing them when
json.Unmarshal succeeds, and update the function comment to document that both
"@json" and "@input" suffixes are processed.
| statusStyle := "text-blue-600" | ||
| if c.Status == "resolved" { | ||
| statusStyle = "text-green-600" | ||
| } else if c.Status == "silenced" { | ||
| statusStyle = "text-gray-500" | ||
| } | ||
| t = t.AddText(" ").Add(clicky.Text(c.Status, statusStyle).Wrap("[", "]")) | ||
| } |
There was a problem hiding this comment.
Use tagged switch for status checking.
The linter (staticcheck QF1003) and the pipeline both suggest using a tagged switch instead of an if-else chain for checking c.Status.
Apply this diff:
- if c.Status != "" {
- statusStyle := "text-blue-600"
- if c.Status == "resolved" {
- statusStyle = "text-green-600"
- } else if c.Status == "silenced" {
- statusStyle = "text-gray-500"
- }
- t = t.AddText(" ").Add(clicky.Text(c.Status, statusStyle).Wrap("[", "]"))
- }
+ if c.Status != "" {
+ var statusStyle string
+ switch c.Status {
+ case "resolved":
+ statusStyle = "text-green-600"
+ case "silenced":
+ statusStyle = "text-gray-500"
+ default:
+ statusStyle = "text-blue-600"
+ }
+ t = t.AddText(" ").Add(clicky.Text(c.Status, statusStyle).Wrap("[", "]"))
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Lint
[error] 824-824: golangci-lint (staticcheck): QF1003: could use tagged switch on c.Status.
🪛 GitHub Check: lint
[failure] 824-824:
QF1003: could use tagged switch on c.Status (staticcheck)
🤖 Prompt for AI Agents
In models/config.go around lines 823 to 830, replace the current if-else chain
that sets statusStyle based on c.Status with a tagged switch on c.Status; create
a switch c.Status { case "resolved": statusStyle = "text-green-600"; case
"silenced": statusStyle = "text-gray-500"; default: statusStyle =
"text-blue-600" } so behavior remains identical but uses a switch as the linter
requests.
1 participant
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.