Closed
Conversation
Signed-off-by: Shawn Hurley <shawn@hurley.page>
Signed-off-by: Shawn Hurley <shawn@hurley.page>
Signed-off-by: Shawn Hurley <shawn@hurley.page>
Signed-off-by: Shawn Hurley <shawn@hurley.page>
Signed-off-by: Shawn Hurley <shawn@hurley.page>
Signed-off-by: Shawn Hurley <shawn@hurley.page>
Signed-off-by: Shawn Hurley <shawn@hurley.page>
Signed-off-by: Shawn Hurley <shawn@hurley.page>
📝 WalkthroughWalkthroughRefactors execution to run the analyzer in-process (core.NewAnalyzer), replaces mutation-based command option APIs with option-returning builders, introduces an AddonReporter for progress events, changes NewInsights to accept in-memory RuleSets, updates dependencies and the runtime base image. Changes
Sequence DiagramsequenceDiagram
participant Addon as Addon
participant Analyzer as Analyzer Core
participant Parser as Rule Parser
participant Providers as Providers
participant Reporter as AddonReporter
Addon->>Analyzer: NewAnalyzer(options)
activate Analyzer
Addon->>Analyzer: ParseRules()
activate Parser
Parser->>Reporter: Report(RuleParsing)
Reporter->>Addon: Activity(rule count)
deactivate Parser
Addon->>Analyzer: ProviderStart()
activate Providers
Providers->>Reporter: Report(ProviderInit)
Reporter->>Addon: Activity(provider ready)
deactivate Providers
Addon->>Analyzer: Run()
Analyzer->>Providers: ExecuteRules()
Providers->>Reporter: Report(ExecutionProgress)
Reporter->>Addon: Activity(progress update)
Analyzer-->>Addon: results (in-memory)
Addon->>Addon: Marshal results to YAML / write insights.yaml
Addon->>Analyzer: Stop()
deactivate Analyzer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
jortel
added
do-not-merge/hold
![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 (4)
builder/builder_test.go (1)
152-169: Remove dead code - temp file creation is no longer needed.Since
NewInsightsnow accepts the in-memoryreportslice directly (line 169), the YAML marshaling and temp file operations (lines 152-167) are no longer used. This dead code should be removed to improve test clarity.♻️ Suggested fix
} - b, err := yaml.Marshal(report) - if err != nil { - t.Fatal(err) - } - f, err := os.CreateTemp("", "") - if err != nil { - t.Fatal(err) - } - defer func() { - _ = f.Close() - _ = os.Remove(f.Name()) - }() - _, err = f.WriteString(string(b)) - if err != nil { - t.Fatal(err) - } - builder, err := NewInsights(report) g.Expect(err).To(gomega.BeNil())Note: You may also be able to remove the
"os"and"gopkg.in/yaml.v2"imports if they're no longer used elsewhere in the file (thoughyamlis still used inTestDepsBuilder).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/builder_test.go` around lines 152 - 169, Remove the unused YAML marshaling and temp file creation in the test: delete the block that calls yaml.Marshal(report), os.CreateTemp, the defer that closes/removes the temp file, and the f.WriteString call since NewInsights(report) now accepts the in-memory report directly (see NewInsights). Also remove the now-unused "os" import and the yaml import if it is no longer referenced elsewhere in this test file (ensure TestDepsBuilder still needs yaml before deleting that import).progress/reporter.go (1)
11-17: Remove unused struct fields.The fields
events,closed, anddroppedEventsare initialized in the constructor (lines 68-70) but never used anywhere in theReportmethod or elsewhere. If these were intended for a different design (e.g., buffered channel processing), they should either be implemented or removed.♻️ Suggested fix
type AddonReporter struct { - events chan progress.Event - closed bool - droppedEvents atomic.Uint64 addon *adapter.Adapter lastReportedExecution *time.Time }And update the constructor:
func NewAddonReporter(addon *adapter.Adapter) *AddonReporter { return &AddonReporter{ - events: make(chan progress.Event), - closed: false, - droppedEvents: atomic.Uint64{}, - addon: addon, + addon: addon, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@progress/reporter.go` around lines 11 - 17, Remove the unused fields events, closed, and droppedEvents from the AddonReporter struct and stop initializing them in the constructor (the function that creates an *AddonReporter instance); update any constructor code that creates the channel or sets closed/droppedEvents (currently initializing events, closed, and droppedEvents) so it only sets addon and lastReportedExecution (and any other used fields), or alternatively implement their intended buffered-channel behavior in Report and related methods—choose removal if they're not used. Ensure references to events, closed, or droppedEvents are deleted from the codebase (constructor and any helper functions) to avoid unused-variable/compiler errors.builder/insight.go (1)
10-11: Consolidate duplicate imports with different aliases.Both lines import the same package (
github.com/konveyor/analyzer-lsp/output/v1/konveyor) but with different aliases:konveyoron line 10 andoutputon line 11. Consider using a single alias consistently throughout the file.♻️ Suggested fix
- "github.com/konveyor/analyzer-lsp/output/v1/konveyor" - output "github.com/konveyor/analyzer-lsp/output/v1/konveyor" + output "github.com/konveyor/analyzer-lsp/output/v1/konveyor"Then update line 23 to use
output.RuleSetinstead ofkonveyor.RuleSet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/insight.go` around lines 10 - 11, There are duplicate imports of "github.com/konveyor/analyzer-lsp/output/v1/konveyor" using two aliases (`konveyor` and `output`); remove one of the import lines and keep a single alias (pick `output` for consistency), then update all references that use `konveyor` (e.g., `konveyor.RuleSet`) to use `output` (e.g., `output.RuleSet`) and ensure the rest of the file consistently uses the chosen alias for that package.cmd/analyzer.go (1)
69-79: Usedeferfor file cleanup and checkClose()error.The current pattern has two issues:
- If a panic occurs between
os.Createandfile.Close(), the file handle leaks- The
Close()error is discarded, which could mask write failures on some filesystems (data may be buffered until close)Suggested fix using defer
file, cErr := os.Create(output) if cErr != nil { err = cErr return } + defer func() { + if closeErr := file.Close(); closeErr != nil && err == nil { + err = closeErr + } + }() _, wErr := file.Write(i) - file.Close() if wErr != nil { err = wErr return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/analyzer.go` around lines 69 - 79, The file creation/writing code in analyzer.go should use defer to close the file and capture Close() errors: after os.Create(output) (the variable file), immediately defer a closure that calls closeErr := file.Close() and if err == nil && closeErr != nil { err = closeErr } so we don't lose a close failure and we avoid leaking the handle on panic; keep the write call (file.Write(i)) and set err = wErr if it fails (preserving write errors over close errors by setting err only when nil in the deferred close).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/injector.go`:
- Around line 346-354: The call to addon.Identity.List() and its logging is
currently inside the loop over md.Resources causing repeated API calls and
duplicate addon.Activity("[IDENTITY] listed...") logs; move the call to
addon.Identity.List() and the subsequent for _, id := range ids {
addon.Activity(...) } block so it executes once before (or after) iterating
md.Resources (e.g., call addon.Identity.List() once, handle nErr, then log
identities via addon.Activity), and remove the per-resource invocation inside
the for _, resource := range md.Resources loop.
In `@cmd/mode.go`:
- Around line 51-60: In Mode.ToOption(), fix the misspelled log prefix by
replacing "[ANAYLZER]" with "[ANALYZER]" in both addon.Activity calls so the
activity messages logged when r.WithDeps is true or false match the rest of the
codebase (references: Mode.ToOption, r.WithDeps, addon.Activity,
core.WithAnalysisMode, provider.FullAnalysisMode,
provider.SourceOnlyAnalysisMode).
In `@cmd/scope.go`:
- Around line 41-45: The error handling branch for
core.WithIncidentSelector(selector) is inverted and calls the function twice;
call core.WithIncidentSelector(selector) once, store its returned error
(optErr), and if optErr == nil append the produced option to the options slice,
otherwise call addon.Activity("invalid label selector: %s, rules will not be
filtered", selector). Update the logic around the options variable and the
core.WithIncidentSelector(selector) call so valid selectors are appended and
invalid ones produce the log via addon.Activity.
In `@progress/reporter.go`:
- Around line 40-56: When event.Current == event.Total in the progress reporter,
you must call a.addon.Completed(event.Total) so the addon framework gets the
final completion update; update the block handling that condition (the branch
around event.Current/event.Total and a.lastReportedExecution) to call
a.addon.Completed(event.Total) before clearing a.lastReportedExecution and
returning, ensuring the final progress is reported (refer to a.addon.Completed,
a.addon.Activity, a.lastReportedExecution, and the event.Current/event.Total
checks).
- Around line 31-35: The current early-return logic in the
progress.StageProviderPrepare case is confusing and likely incorrect; change the
guard so we only report the provider prepare at the start of preparation (when
event.Current == 0 and there is a meaningful Total). Replace the existing
condition with a clear check such as: return early if event.Current != 0 ||
event.Total <= 0, then call a.addon.Activity("[ANALYZER] %s", event.Message).
This updates the StageProviderPrepare branch to only log at preparation start
and avoids treating Total==0 as the trigger.
---
Nitpick comments:
In `@builder/builder_test.go`:
- Around line 152-169: Remove the unused YAML marshaling and temp file creation
in the test: delete the block that calls yaml.Marshal(report), os.CreateTemp,
the defer that closes/removes the temp file, and the f.WriteString call since
NewInsights(report) now accepts the in-memory report directly (see NewInsights).
Also remove the now-unused "os" import and the yaml import if it is no longer
referenced elsewhere in this test file (ensure TestDepsBuilder still needs yaml
before deleting that import).
In `@builder/insight.go`:
- Around line 10-11: There are duplicate imports of
"github.com/konveyor/analyzer-lsp/output/v1/konveyor" using two aliases
(`konveyor` and `output`); remove one of the import lines and keep a single
alias (pick `output` for consistency), then update all references that use
`konveyor` (e.g., `konveyor.RuleSet`) to use `output` (e.g., `output.RuleSet`)
and ensure the rest of the file consistently uses the chosen alias for that
package.
In `@cmd/analyzer.go`:
- Around line 69-79: The file creation/writing code in analyzer.go should use
defer to close the file and capture Close() errors: after os.Create(output) (the
variable file), immediately defer a closure that calls closeErr := file.Close()
and if err == nil && closeErr != nil { err = closeErr } so we don't lose a close
failure and we avoid leaking the handle on panic; keep the write call
(file.Write(i)) and set err = wErr if it fails (preserving write errors over
close errors by setting err only when nil in the deferred close).
In `@progress/reporter.go`:
- Around line 11-17: Remove the unused fields events, closed, and droppedEvents
from the AddonReporter struct and stop initializing them in the constructor (the
function that creates an *AddonReporter instance); update any constructor code
that creates the channel or sets closed/droppedEvents (currently initializing
events, closed, and droppedEvents) so it only sets addon and
lastReportedExecution (and any other used fields), or alternatively implement
their intended buffered-channel behavior in Report and related methods—choose
removal if they're not used. Ensure references to events, closed, or
droppedEvents are deleted from the codebase (constructor and any helper
functions) to avoid unused-variable/compiler errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 922a4fce-f7f8-45fe-824a-5df8b651bf4a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
Dockerfilebuilder/builder_test.gobuilder/insight.gocmd/analyzer.gocmd/injector.gocmd/main.gocmd/mode.gocmd/rules.gocmd/scope.gocmd/settings.gogo.modprogress/reporter.go
Comment on lines
+346
to
+354
|
|
||
| ids, nErr := addon.Identity.List() | ||
| if nErr != nil { | ||
| err = nErr | ||
| return | ||
| } | ||
| for _, id := range ids { | ||
| addon.Activity("[IDENTITY] listed (id=%d), %s", id.ID, id.Name) | ||
| } |
There was a problem hiding this comment.
Identity listing is performed inside the resource loop, causing redundant API calls and log spam.
addon.Identity.List() is called on every iteration of the for _, resource := range md.Resources loop. Since the identity list doesn't change between iterations, this results in:
- Redundant API calls for each resource entry
- Duplicate
[IDENTITY] listedlog entries
If this logging is needed for debugging, consider moving it outside the loop or removing it entirely once debugging is complete.
Suggested fix: Move identity listing outside the loop
func (r *ResourceInjector) build(md *Metadata) (err error) {
application, err := addon.Task.Application()
if err != nil {
return
}
for _, resource := range md.Resources {
err = r.addDefaults(&resource)
}
+
+ ids, err := addon.Identity.List()
+ if err != nil {
+ return
+ }
+ for _, id := range ids {
+ addon.Activity("[IDENTITY] listed (id=%d), %s", id.ID, id.Name)
+ }
+
for _, resource := range md.Resources {
parsed := ParsedSelector{}
parsed.With(resource.Selector)
-
- ids, nErr := addon.Identity.List()
- if nErr != nil {
- err = nErr
- return
- }
- for _, id := range ids {
- addon.Activity("[IDENTITY] listed (id=%d), %s", id.ID, id.Name)
- }
-
switch strings.ToLower(parsed.kind) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/injector.go` around lines 346 - 354, The call to addon.Identity.List()
and its logging is currently inside the loop over md.Resources causing repeated
API calls and duplicate addon.Activity("[IDENTITY] listed...") logs; move the
call to addon.Identity.List() and the subsequent for _, id := range ids {
addon.Activity(...) } block so it executes once before (or after) iterating
md.Resources (e.g., call addon.Identity.List() once, handle nErr, then log
identities via addon.Activity), and remove the per-resource invocation inside
the for _, resource := range md.Resources loop.
Comment on lines
+51
to
60
| func (r *Mode) ToOption() (option core.AnalyzerOption) { | ||
| if r.WithDeps { | ||
| settings.Mode(provider.FullAnalysisMode) | ||
| addon.Activity("[ANAYLZER] using full analysis mode") | ||
| option = core.WithAnalysisMode(string(provider.FullAnalysisMode)) | ||
| } else { | ||
| settings.Mode(provider.SourceOnlyAnalysisMode) | ||
| addon.Activity("[ANAYLZER] using source analysis mode") | ||
| option = core.WithAnalysisMode(string(provider.SourceOnlyAnalysisMode)) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Typo: "ANAYLZER" should be "ANALYZER" in both activity log messages.
The log prefix is misspelled as [ANAYLZER] instead of [ANALYZER], which is inconsistent with other files (e.g., cmd/injector.go line 370 uses [ANALYZER]). This could cause issues when filtering or searching logs.
Suggested fix
func (r *Mode) ToOption() (option core.AnalyzerOption) {
if r.WithDeps {
- addon.Activity("[ANAYLZER] using full analysis mode")
+ addon.Activity("[ANALYZER] using full analysis mode")
option = core.WithAnalysisMode(string(provider.FullAnalysisMode))
} else {
- addon.Activity("[ANAYLZER] using source analysis mode")
+ addon.Activity("[ANALYZER] using source analysis mode")
option = core.WithAnalysisMode(string(provider.SourceOnlyAnalysisMode))
}
return
}📝 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.
Suggested change
| func (r *Mode) ToOption() (option core.AnalyzerOption) { | |
| if r.WithDeps { | |
| settings.Mode(provider.FullAnalysisMode) | |
| addon.Activity("[ANAYLZER] using full analysis mode") | |
| option = core.WithAnalysisMode(string(provider.FullAnalysisMode)) | |
| } else { | |
| settings.Mode(provider.SourceOnlyAnalysisMode) | |
| addon.Activity("[ANAYLZER] using source analysis mode") | |
| option = core.WithAnalysisMode(string(provider.SourceOnlyAnalysisMode)) | |
| } | |
| return | |
| } | |
| func (r *Mode) ToOption() (option core.AnalyzerOption) { | |
| if r.WithDeps { | |
| addon.Activity("[ANALYZER] using full analysis mode") | |
| option = core.WithAnalysisMode(string(provider.FullAnalysisMode)) | |
| } else { | |
| addon.Activity("[ANALYZER] using source analysis mode") | |
| option = core.WithAnalysisMode(string(provider.SourceOnlyAnalysisMode)) | |
| } | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/mode.go` around lines 51 - 60, In Mode.ToOption(), fix the misspelled log
prefix by replacing "[ANAYLZER]" with "[ANALYZER]" in both addon.Activity calls
so the activity messages logged when r.WithDeps is true or false match the rest
of the codebase (references: Mode.ToOption, r.WithDeps, addon.Activity,
core.WithAnalysisMode, provider.FullAnalysisMode,
provider.SourceOnlyAnalysisMode).
Comment on lines
+41
to
+45
| if optErr := core.WithIncidentSelector(selector); optErr != nil { | ||
| options = append(options, core.WithIncidentSelector(selector)) | ||
| } else { | ||
| addon.Activity("invalid label selector: %s, rules will not be filtered", selector) | ||
| } |
There was a problem hiding this comment.
Critical: Error handling logic is inverted.
The condition is backwards:
- When
optErr != nil(error occurred), the code appends the selector anyway (line 42) - When
optErr == nil(success), the code logs "invalid label selector" (line 44)
This will cause valid selectors to be logged as invalid and invalid selectors to be silently used.
🐛 Proposed fix
if selector != "" {
- if optErr := core.WithIncidentSelector(selector); optErr != nil {
- options = append(options, core.WithIncidentSelector(selector))
- } else {
+ if err := core.WithIncidentSelector(selector); err != nil {
addon.Activity("invalid label selector: %s, rules will not be filtered", selector)
+ } else {
+ options = append(options, core.WithIncidentSelector(selector))
}
}📝 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.
Suggested change
| if optErr := core.WithIncidentSelector(selector); optErr != nil { | |
| options = append(options, core.WithIncidentSelector(selector)) | |
| } else { | |
| addon.Activity("invalid label selector: %s, rules will not be filtered", selector) | |
| } | |
| if err := core.WithIncidentSelector(selector); err != nil { | |
| addon.Activity("invalid label selector: %s, rules will not be filtered", selector) | |
| } else { | |
| options = append(options, core.WithIncidentSelector(selector)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/scope.go` around lines 41 - 45, The error handling branch for
core.WithIncidentSelector(selector) is inverted and calls the function twice;
call core.WithIncidentSelector(selector) once, store its returned error
(optErr), and if optErr == nil append the produced option to the options slice,
otherwise call addon.Activity("invalid label selector: %s, rules will not be
filtered", selector). Update the logic around the options variable and the
core.WithIncidentSelector(selector) call so valid selectors are appended and
invalid ones produce the log via addon.Activity.
Comment on lines
+31
to
+35
| case progress.StageProviderPrepare: | ||
| if event.Current != 0 || event.Total != event.Current { | ||
| return | ||
| } | ||
| a.addon.Activity("[ANALYZER] %s", event.Message) |
There was a problem hiding this comment.
Logic appears inverted for StageProviderPrepare.
The condition event.Current != 0 || event.Total != event.Current returns early (skips reporting) when either Current is non-zero OR Total doesn't equal Current. Based on the AI summary ("reporting provider prepare only when Current == 0 and Total == Current"), this seems correct but the double-negative makes it hard to read. Additionally, if Current == 0 and Total == Current, that means Total == 0, which may not be the intended trigger condition.
Could you clarify the intended behavior? If the goal is to report only at the start of provider preparation, consider simplifying:
💡 Suggested clarification
case progress.StageProviderPrepare:
- if event.Current != 0 || event.Total != event.Current {
+ // Report only at the start (when Current == 0)
+ if event.Current != 0 {
return
}
a.addon.Activity("[ANALYZER] %s", event.Message)📝 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.
Suggested change
| case progress.StageProviderPrepare: | |
| if event.Current != 0 || event.Total != event.Current { | |
| return | |
| } | |
| a.addon.Activity("[ANALYZER] %s", event.Message) | |
| case progress.StageProviderPrepare: | |
| // Report only at the start (when Current == 0) | |
| if event.Current != 0 { | |
| return | |
| } | |
| a.addon.Activity("[ANALYZER] %s", event.Message) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@progress/reporter.go` around lines 31 - 35, The current early-return logic in
the progress.StageProviderPrepare case is confusing and likely incorrect; change
the guard so we only report the provider prepare at the start of preparation
(when event.Current == 0 and there is a meaningful Total). Replace the existing
condition with a clear check such as: return early if event.Current != 0 ||
event.Total <= 0, then call a.addon.Activity("[ANALYZER] %s", event.Message).
This updates the StageProviderPrepare branch to only log at preparation start
and avoids treating Total==0 as the trigger.
Comment on lines
+40
to
+56
| if event.Current == event.Total { | ||
| a.addon.Activity("[ANALYZER] processed %d rules out of %d", event.Current, event.Total) | ||
| // Just in case there is ever more than one analysis run per addon | ||
| a.lastReportedExecution = nil | ||
| return | ||
| } | ||
| if event.Current == 0 { | ||
| a.addon.Activity("[ANALYZER] starting to process %v rules", event.Total) | ||
| a.addon.Total(event.Total) | ||
| t := time.Now() | ||
| a.lastReportedExecution = &t | ||
| } else if a.lastReportedExecution == nil || time.Since(*a.lastReportedExecution) >= 5*time.Second { | ||
| a.addon.Activity("[ANALYZER] processed %d rules out of %d", event.Current, event.Total) | ||
| a.addon.Completed(event.Current) | ||
| t := time.Now() | ||
| a.lastReportedExecution = &t | ||
| } |
There was a problem hiding this comment.
Missing addon.Completed call when rule execution finishes.
When event.Current == event.Total (lines 40-44), the code reports the final count but doesn't call a.addon.Completed(event.Total) to signal completion to the addon framework. This may leave the progress indicator incomplete.
🐛 Suggested fix
case progress.StageRuleExecution:
if event.Total == 0 {
return
}
if event.Current == event.Total {
a.addon.Activity("[ANALYZER] processed %d rules out of %d", event.Current, event.Total)
+ a.addon.Completed(event.Current)
// Just in case there is ever more than one analysis run per addon
a.lastReportedExecution = nil
return
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@progress/reporter.go` around lines 40 - 56, When event.Current == event.Total
in the progress reporter, you must call a.addon.Completed(event.Total) so the
addon framework gets the final completion update; update the block handling that
condition (the branch around event.Current/event.Total and
a.lastReportedExecution) to call a.addon.Completed(event.Total) before clearing
a.lastReportedExecution and returning, ensuring the final progress is reported
(refer to a.addon.Completed, a.addon.Activity, a.lastReportedExecution, and the
event.Current/event.Total checks).
Signed-off-by: Jeff Ortel <jortel@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/analyzer.go`:
- Around line 123-125: The code currently marshals and logs settings.Configs
(around options = append(... core.WithProviderConfigs(settings.Configs) ...))
which can leak secrets injected by ProxySettings() (e.g., provider.Proxy
user:password@host) — remove the yaml.Marshal + addon.Activity call or replace
it with a sanitized summary: implement a sanitizer that redacts sensitive keys
(e.g., "password", "secret", "token", "api_key", "proxy", "authorization") from
settings.Configs and log only non-sensitive metadata (provider names, counts, or
a redacted map) before calling core.WithProviderConfigs; update the code paths
using settings.Configs (the options append and any logging near addon.Activity)
to call the sanitizer instead of marshaling the raw config.
In `@cmd/settings.go`:
- Around line 71-72: r.metadata(&extension) is being called and its result used
before checking err, and the second call to extension.Inject(...) swallows its
error, allowing partially-populated provider configs to be appended; fix by
immediately checking and returning the error from r.metadata(&extension) before
accessing md.Provider (so do not call r.hasProvider(&md.Provider) unless
metadata succeeded), and capture/return any error from extension.Inject(...)
instead of discarding it; apply the same changes to the other similar block (the
Inject/metadata sequence around the 83-85 area) so no partial provider is
appended on metadata or injection failure.
- Line 69: The current log.Info calls (e.g., the one referencing "extension" and
the places around lines referencing md mutation) are printing entire
extension/provider/init payloads (variables like extension, md, providerConfig,
initConfig), which can leak secrets; replace those Info logs to emit only stable
non-sensitive identifiers (e.g., extension name or ID, provider name, and
extension version) and do not log full maps or configs—remove or redact any
sensitive fields from md/providerConfig/initConfig before logging. Locate the
log.Info usages that mention "using extension to add to config" and the other
calls around the md mutation (including the ones at the referenced ranges) and
update them to log only those identifiers or a success/failure status rather
than full payloads.
- Around line 56-61: Remove the preflight call to addon.Identity.List() that
aborts AppendExtensions() on transient Hub errors: either delete the call and
the vars (ids, nErr) entirely, or make it best-effort by calling
addon.Identity.List() in a non-fatal way (capture error but do not assign to
err/return) and only log the result (e.g., log.Debug or log.Info) so
AppendExtensions() proceeds even if List() fails; reference the Identity.List()
invocation, the ids variable, and the surrounding AppendExtensions() flow when
making the change.
In `@go.mod`:
- Around line 58-59: Update the grpc dependency in go.mod by bumping the
google.golang.org/grpc module from v1.73.0 to v1.79.3 (or later) to address
GHSA-p77j-4mvh-x3m3, then run the Go tooling to propagate the change (e.g., go
get google.golang.org/grpc@v1.79.3 && go mod tidy) and verify go.sum updated; if
grpc is pulled in transitively, identify and update or replace the parent module
that pins google.golang.org/grpc so the resolved version is >= v1.79.3.
- Around line 46-50: The go.opentelemetry.io/otel/sdk dependency is pinned at
v1.35.0 and must be bumped to v1.40.0+ to mitigate GHSA-9h8m-3fm2-qjrq; update
the module declaration for go.opentelemetry.io/otel/sdk in go.mod (or add a
replace/require override) to v1.40.0 or later and then run module resolution (go
get ./... or go mod tidy) so the new version is applied project-wide; ensure the
change targets the go.opentelemetry.io/otel/sdk module name exactly so
transitive consumers are upgraded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3aafe450-d3a0-4538-928c-77ac112967a9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/analyzer.gocmd/injector.gocmd/settings.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/injector.go
Comment on lines
+123
to
+125
| options = append(options, core.WithProviderConfigs(settings.Configs)) | ||
| b, _ := yaml.Marshal(settings.Configs) | ||
| addon.Activity("[ANALYZER] options:\n%s", string(b)) |
There was a problem hiding this comment.
Don't post full provider configs to addon activity.
ProxySettings() runs immediately before this and can inject user:password@host into provider.Proxy. Marshalling settings.Configs here will leak proxy credentials and any provider init secrets into the addon activity log.
Safer logging
options = append(options, core.WithProviderConfigs(settings.Configs))
- b, _ := yaml.Marshal(settings.Configs)
- addon.Activity("[ANALYZER] options:\n%s", string(b))
+ addon.Activity("[ANALYZER] configured %d providers", len(settings.Configs))📝 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.
Suggested change
| options = append(options, core.WithProviderConfigs(settings.Configs)) | |
| b, _ := yaml.Marshal(settings.Configs) | |
| addon.Activity("[ANALYZER] options:\n%s", string(b)) | |
| options = append(options, core.WithProviderConfigs(settings.Configs)) | |
| addon.Activity("[ANALYZER] configured %d providers", len(settings.Configs)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/analyzer.go` around lines 123 - 125, The code currently marshals and logs
settings.Configs (around options = append(...
core.WithProviderConfigs(settings.Configs) ...)) which can leak secrets injected
by ProxySettings() (e.g., provider.Proxy user:password@host) — remove the
yaml.Marshal + addon.Activity call or replace it with a sanitized summary:
implement a sanitizer that redacts sensitive keys (e.g., "password", "secret",
"token", "api_key", "proxy", "authorization") from settings.Configs and log only
non-sensitive metadata (provider names, counts, or a redacted map) before
calling core.WithProviderConfigs; update the code paths using settings.Configs
(the options append and any logging near addon.Activity) to call the sanitizer
instead of marshaling the raw config.
Comment on lines
+56
to
+61
| ids, nErr := addon.Identity.List() | ||
| if nErr != nil { | ||
| err = nErr | ||
| return | ||
| } | ||
| log.Info("_______________________ Listed", "ids", ids) |
There was a problem hiding this comment.
Remove the Identity.List() preflight.
ids is only used for logging, but a transient Hub error here now aborts AppendExtensions() before any extension metadata is processed. If this was added for debugging, make it best-effort or drop it.
Minimal change
- ids, nErr := addon.Identity.List()
- if nErr != nil {
- err = nErr
- return
- }
- log.Info("_______________________ Listed", "ids", ids)📝 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.
Suggested change
| ids, nErr := addon.Identity.List() | |
| if nErr != nil { | |
| err = nErr | |
| return | |
| } | |
| log.Info("_______________________ Listed", "ids", ids) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/settings.go` around lines 56 - 61, Remove the preflight call to
addon.Identity.List() that aborts AppendExtensions() on transient Hub errors:
either delete the call and the vars (ids, nErr) entirely, or make it best-effort
by calling addon.Identity.List() in a non-fatal way (capture error but do not
assign to err/return) and only log the result (e.g., log.Debug or log.Info) so
AppendExtensions() proceeds even if List() fails; reference the Identity.List()
invocation, the ids variable, and the surrounding AppendExtensions() flow when
making the change.
| } | ||
|
|
||
| for _, extension := range addon.Extensions { | ||
| log.Info("using extension to add to config", "extension", extension) |
There was a problem hiding this comment.
Avoid logging full extension/provider payloads.
After injection mutates md in place, these Info calls dump the resolved extension map, provider config, and init config. That's a straightforward way for credentials or other sensitive settings to end up in runtime logs; log stable identifiers instead.
Also applies to: 73-73, 86-86, 227-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/settings.go` at line 69, The current log.Info calls (e.g., the one
referencing "extension" and the places around lines referencing md mutation) are
printing entire extension/provider/init payloads (variables like extension, md,
providerConfig, initConfig), which can leak secrets; replace those Info logs to
emit only stable non-sensitive identifiers (e.g., extension name or ID, provider
name, and extension version) and do not log full maps or configs—remove or
redact any sensitive fields from md/providerConfig/initConfig before logging.
Locate the log.Info usages that mention "using extension to add to config" and
the other calls around the md mutation (including the ones at the referenced
ranges) and update them to log only those identifiers or a success/failure
status rather than full payloads.
Comment on lines
71
to
72
| md, err = r.metadata(&extension) | ||
| if r.hasProvider(&md.Provider) { |
There was a problem hiding this comment.
Stop on metadata/self-injection errors.
r.metadata() is used before its error is checked, and the second Inject() call drops its error entirely. Either failure can append a partially populated provider config.
Suggested fix
var md *Metadata
md, err = r.metadata(&extension)
+ if err != nil {
+ return
+ }
if r.hasProvider(&md.Provider) {
log.Info("provider already found", "provider", &md.Provider)
continue
@@
rinjector := ResourceInjector{}
rinjector.Use(rinjector.asMap(md.Provider))
- rinjector.Inject(md)
+ if err = rinjector.Inject(md); err != nil {
+ return
+ }Also applies to: 83-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/settings.go` around lines 71 - 72, r.metadata(&extension) is being called
and its result used before checking err, and the second call to
extension.Inject(...) swallows its error, allowing partially-populated provider
configs to be appended; fix by immediately checking and returning the error from
r.metadata(&extension) before accessing md.Provider (so do not call
r.hasProvider(&md.Provider) unless metadata succeeded), and capture/return any
error from extension.Inject(...) instead of discarding it; apply the same
changes to the other similar block (the Inject/metadata sequence around the
83-85 area) so no partial provider is appended on metadata or injection failure.
Comment on lines
+46
to
+50
| go.opentelemetry.io/auto/sdk v1.1.0 // indirect | ||
| go.opentelemetry.io/otel v1.35.0 // indirect | ||
| go.opentelemetry.io/otel/exporters/jaeger v1.17.0 // indirect | ||
| go.opentelemetry.io/otel/metric v1.35.0 // indirect | ||
| go.opentelemetry.io/otel/sdk v1.35.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the first patched version of go.opentelemetry.io/otel/sdk for GHSA-9h8m-3fm2-qjrq / GO-2026-4394?
💡 Result:
v1.40.0
Citations:
- 1: GHSA-9h8m-3fm2-qjrq
- 2: https://osv.dev/vulnerability/GO-2026-4394
- 3: https://pkg.go.dev/vuln/GO-2026-4394
- 4: https://github.com/github/advisory-database/blob/main/advisories/github-reviewed/2026/02/GHSA-9h8m-3fm2-qjrq/GHSA-9h8m-3fm2-qjrq.json
- 5: https://nvd.nist.gov/vuln/detail/CVE-2026-24051
Upgrade otel/sdk to v1.40.0 or later.
The graph includes go.opentelemetry.io/otel/sdk v1.35.0, which is affected by GHSA-9h8m-3fm2-qjrq (PATH-hijacking issue). Bump the parent dependency or override this module to v1.40.0 or a later release.
🧰 Tools
🪛 OSV Scanner (2.3.3)
[HIGH] 50-50: go.opentelemetry.io/otel/sdk 1.35.0: OpenTelemetry Go SDK Vulnerable to Arbitrary Code Execution via PATH Hijacking in go.opentelemetry.io/otel/sdk
(GO-2026-4394)
[HIGH] 50-50: go.opentelemetry.io/otel/sdk 1.35.0: OpenTelemetry Go SDK Vulnerable to Arbitrary Code Execution via PATH Hijacking
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` around lines 46 - 50, The go.opentelemetry.io/otel/sdk dependency is
pinned at v1.35.0 and must be bumped to v1.40.0+ to mitigate
GHSA-9h8m-3fm2-qjrq; update the module declaration for
go.opentelemetry.io/otel/sdk in go.mod (or add a replace/require override) to
v1.40.0 or later and then run module resolution (go get ./... or go mod tidy) so
the new version is applied project-wide; ensure the change targets the
go.opentelemetry.io/otel/sdk module name exactly so transitive consumers are
upgraded.
Comment on lines
+58
to
+59
| google.golang.org/genproto/googleapis/rpc v0.0.0-20250324211829-b45e905df463 // indirect | ||
| google.golang.org/grpc v1.73.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the first patched version of google.golang.org/grpc for GHSA-p77j-4mvh-x3m3?
💡 Result:
The first patched version of google.golang.org/grpc for GHSA-p77j-4mvh-x3m3 is v1.79.3.
Citations:
- 1: GHSA-p77j-4mvh-x3m3
- 2: https://github.com/grpc/grpc-go/releases
- 3: https://github.com/grpc/grpc-go/releases/tag/v1.79.3
- 4: grpc: enforce strict path checking for incoming requests on the server grpc/grpc-go#8985
Upgrade google.golang.org/grpc to v1.79.3 or later to fix the auth-bypass vulnerability.
google.golang.org/grpc v1.73.0 is vulnerable to GHSA-p77j-4mvh-x3m3. Upgrade to v1.79.3 (the first patched release) or bump the transitive parent that pins it before cutting a new image.
🧰 Tools
🪛 OSV Scanner (2.3.3)
[CRITICAL] 59-59: google.golang.org/grpc 1.73.0: gRPC-Go has an authorization bypass via missing leading slash in :path
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` around lines 58 - 59, Update the grpc dependency in go.mod by bumping
the google.golang.org/grpc module from v1.73.0 to v1.79.3 (or later) to address
GHSA-p77j-4mvh-x3m3, then run the Go tooling to propagate the change (e.g., go
get google.golang.org/grpc@v1.79.3 && go mod tidy) and verify go.sum updated; if
grpc is pulled in transitively, identify and update or replace the parent module
that pins google.golang.org/grpc so the resolved version is >= v1.79.3.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores