✨ Refactor to use the the provider config to initialize providers #889
Conversation
* Forces the use of providers that are using sockets. * New flow is analyzer-lsp server -> provider (GRPC over Socket) -> jsonrpc (lsp protocol) over Socket. * This is working locally for me w/o vscode extensions Signed-off-by: Shawn Hurley <[email protected]>
shawn-hurley
requested review from
djzager,
pranavgaikwad and
savitharaghunathan
and removed request for
djzager
WalkthroughThe PR refactors the analyzer to a server-oriented RPC model driven by a provider configuration file, replaces source-directory/pipe flags with server-pipe and provider-config, removes legacy Java internal provider implementations, and updates dependencies (Go toolchain and gRPC/protobuf-related packages). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Main as main()
participant Server as kairpc.Server
participant Pipe as Connection (pipe)
participant Config as ProviderConfig
participant PipeAnalyzer as PipeAnalyzer
participant Engine as AnalysisEngine
Client->>Main: start with --server-pipe & --provider-config
Main->>Server: NewServer(providerConfigFile)
Main->>Server: Accept(server-pipe)
Note over Server: OnConnect registered (sends "started" notification)
Pipe->>Server: connect
Server->>Config: Load(providerConfigFile)
Config-->>Server: provider definitions
Server->>PipeAnalyzer: NewPipeAnalyzer(rules, providerConfigFile)
PipeAnalyzer->>Config: GetConfig(providerConfigFile)
PipeAnalyzer->>PipeAnalyzer: Initialize providers (including builtin injection)
PipeAnalyzer->>Engine: Create engine with initialized providers
Engine-->>PipeAnalyzer: ready
Server->>Pipe: Send "started" notification
Pipe-->>Server: Ack
Note over Server,PipeAnalyzer: Ready to accept analysis requests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
kai_analyzer_rpc/pkg/service/analyzer.go (2)
281-287: Fix data race on discoveryCache read.Reads are unsynchronized while writes are mutex-protected. Copy under lock before passing to scope to avoid racy access.
Apply this diff:
- scopes = append(scopes, scope.NewDiscoveryRuleScope(a.Logger, a.discoveryCache)) + a.discoveryCacheMutex.Lock() + cacheCopy := append([]konveyor.RuleSet(nil), a.discoveryCache...) + a.discoveryCacheMutex.Unlock() + scopes = append(scopes, scope.NewDiscoveryRuleScope(a.Logger, cacheCopy))
262-265: Correct logr key/value usage.Second argument must be a key, not a bare value.
- a.Logger.Info("Current cache len", a.cache.Len()) + a.Logger.Info("Current cache len", "len", a.cache.Len())kai_analyzer_rpc/pkg/service/pipe_analyzer.go (1)
1-1: Fix build tag.
// go: build(with space) is ignored. Either remove it or use the correct form. This file should likely be cross-platform.-// go: build linux || darwin || freebsd || openbsd || netbsd || solaris || dragonfly || plan9 +// (removed incorrect build tag)kai_analyzer_rpc/pkg/rpc/server_windows.go (1)
55-58: Fail fast on analyzer init failure (panic rather than return).Match non-Windows behavior to avoid a half-alive server. Based on learnings.
- if err != nil { - s.log.Error(err, "unable to create analyzer service") - return - } + if err != nil { + s.log.Error(err, "unable to create analyzer service; exiting") + panic(err) + }
🧹 Nitpick comments (7)
kai_analyzer_rpc/pkg/service/analyzer.go (1)
340-346: Reduce incident log verbosity to avoid leaking content and excessive logs.Logging entire incidents can include code/text and bloat logs.
- a.Logger.Info("here update cache incident", "incident", i) + a.Logger.V(6).Info("update cache incident", "uri", i.URI.Filename())kai_analyzer_rpc/pkg/service/pipe_analyzer.go (3)
28-29: Avoid logging full provider configs.Configs may include paths/addresses. Log names/count instead.
- l.Info("got provider config", "config", providerConfigFile, "configs", configs) + l.Info("got provider config", "config", providerConfigFile, "providers", len(configs))
110-125: Track all created provider clients for Stop().
initedProvidersonly includesneededProviders; clients created but unused won’t be stopped. Prefer storingproviders.- initedProviders: neededProviders, + initedProviders: providers,
19-26: Ensure cancelFunc() on all error returns.Prevent context/resource leaks by canceling on early exit.
Example pattern:
-func NewPipeAnalyzer(ctx context.Context, ... ) (Analyzer, error) { - ctx, cancelFunc := context.WithCancel(ctx) +func NewPipeAnalyzer(ctx context.Context, ... ) (Analyzer, error) { + ctx, cancelFunc := context.WithCancel(ctx) + var retErr error + defer func() { + if retErr != nil { + cancelFunc() + } + }() ... - if err != nil { - return nil, err - } + if err != nil { + retErr = err + return nil, err + }Or explicitly call
cancelFunc()before eachreturn nil, err.Also applies to: 45-50, 57-60, 68-71, 79-85, 87-96
kai_analyzer_rpc/go.mod (1)
10-13: Align OTel exporter/core versions.Exporter is v1.34.0 while core is v1.35.0. Consider bumping exporter to 1.35.0 to avoid subtle incompatibilities.
- go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.34.0 + go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.35.0kai_analyzer_rpc/pkg/rpc/server.go (1)
49-51: Fix log wording.We’re listening, not dialing.
- s.log.Info("dialing connection connections", "pipe", pipePath) + s.log.Info("listening for connections", "pipe", pipePath)kai_analyzer_rpc/pkg/rpc/server_windows.go (1)
29-31: Remove or use initService.
initServiceis unused; consider removing to reduce clutter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
kai_analyzer_rpc/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
kai_analyzer_rpc/go.mod(2 hunks)kai_analyzer_rpc/main.go(3 hunks)kai_analyzer_rpc/pkg/rpc/server.go(3 hunks)kai_analyzer_rpc/pkg/rpc/server_windows.go(3 hunks)kai_analyzer_rpc/pkg/service/analyzer.go(4 hunks)kai_analyzer_rpc/pkg/service/pipe_analyzer.go(3 hunks)kai_analyzer_rpc/provider/java/internal_java_provider.go(0 hunks)kai_analyzer_rpc/provider/java/internal_java_provider_windows.go(0 hunks)
💤 Files with no reviewable changes (2)
- kai_analyzer_rpc/provider/java/internal_java_provider.go
- kai_analyzer_rpc/provider/java/internal_java_provider_windows.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-10T22:53:20.874Z
Learnt from: pranavgaikwad
PR: konveyor/kai#864
File: kai_analyzer_rpc/pkg/rpc/server.go:58-61
Timestamp: 2025-09-10T22:53:20.874Z
Learning: In kai_analyzer_rpc/pkg/rpc/server.go, panic on analyzer service initialization failure is intentional to ensure the process ends when the core service cannot be created, following a fail-fast approach.
Applied to files:
kai_analyzer_rpc/pkg/rpc/server_windows.gokai_analyzer_rpc/main.go
📚 Learning: 2025-09-10T22:52:55.595Z
Learnt from: pranavgaikwad
PR: konveyor/kai#864
File: kai_analyzer_rpc/pkg/service/analyzer.go:374-379
Timestamp: 2025-09-10T22:52:55.595Z
Learning: In kai_analyzer_rpc/pkg/service/analyzer.go, the analyzer only receives file paths from within the workspace, so path normalization before cache operations is not needed as paths are already consistently formatted.
Applied to files:
kai_analyzer_rpc/pkg/service/analyzer.go
🧬 Code graph analysis (4)
kai_analyzer_rpc/pkg/rpc/server_windows.go (2)
kai_analyzer_rpc/pkg/rpc/server.go (2)
NewServer(31-41)Server(20-29)kai_analyzer_rpc/pkg/service/pipe_analyzer.go (1)
NewPipeAnalyzer(19-127)
kai_analyzer_rpc/pkg/rpc/server.go (2)
kai_analyzer_rpc/pkg/rpc/server_windows.go (2)
NewServer(33-45)Server(21-31)kai_analyzer_rpc/pkg/service/pipe_analyzer.go (1)
NewPipeAnalyzer(19-127)
kai_analyzer_rpc/pkg/service/pipe_analyzer.go (2)
kai_analyzer_rpc/pkg/service/analyzer.go (1)
Analyzer(31-35)kai_analyzer_rpc/pkg/service/cache.go (1)
NewIncidentsCache(27-33)
kai_analyzer_rpc/main.go (3)
kai_analyzer_rpc/pkg/rpc/server.go (1)
NewServer(31-41)kai_analyzer_rpc/pkg/rpc/server_windows.go (1)
NewServer(33-45)kai_analyzer_rpc/pkg/rpc/client/client.go (1)
Client(9-11)
⏰ 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). (7)
- GitHub Check: Run e2e test (ubuntu-22.04-arm, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (macos-13, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (windows-latest, cmd, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (ubuntu-24.04, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (macos-latest, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Trunk Check Runner
- GitHub Check: Check Types on PR Runner (ubuntu-latest, 3.12, x64)
🔇 Additional comments (4)
kai_analyzer_rpc/go.mod (1)
3-3: CI/build environment properly supports Go 1.23.9.The CI workflow (build-and-push-binaries.yml:180) uses
actions/setup-go@v5withgo-version: "1.23"andcheck-latest: true, which automatically fetches the latest 1.23.x patch available. This satisfies the requirement specified in kai_analyzer_rpc/go.mod. No container images or other toolchain constraints that would conflict with this configuration were found.kai_analyzer_rpc/main.go (3)
55-55: LGTM!The updated logging statement correctly includes the new configuration parameters, which will be helpful for debugging server startup issues.
79-79: LGTM!The
NewServercall correctly uses the updated signature with theproviderConfigFileparameter, and properly dereferences the flag pointers.
80-87: Verify error handling strategy for startup notification.The
OnConnectcallback logs notification failures but doesn't take further action. While this may be appropriate if the notification is purely informational, consider whether a failure to notify the client of server readiness should prevent the server from accepting requests.If notification failure should be non-fatal (as currently implemented), the code is correct. However, if clients depend on receiving the "started" notification to know the server is ready, you may want to consider returning or logging this as a more severe issue.
| serverPipe := flag.String("server-pipe", "", "Path to the pipe to use for bi-directional communication") | ||
| providerConfigFile := flag.String("provider-config", "", "Path the provider config file") |
There was a problem hiding this comment.
Add validation for required flags.
The server-pipe and provider-config flags appear to be required for server operation, but there's no validation to ensure they're provided. Without validation, empty values could lead to unclear runtime errors when Accept() or the server initialization attempts to use these paths.
Consider adding validation after flag.Parse():
flag.Parse()
+ if *serverPipe == "" {
+ slog.Error("server-pipe flag is required")
+ os.Exit(1)
+ }
+ if *providerConfigFile == "" {
+ slog.Error("provider-config flag is required")
+ os.Exit(1)
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In kai_analyzer_rpc/main.go around lines 23 to 24, both the server-pipe and
provider-config flags are treated as required but not validated; after
flag.Parse() add checks that *serverPipe and *providerConfigFile are non-empty
and if either is empty log a clear error (including which flag is missing and
the expected purpose), print flag.Usage() or usage hint, and exit with non-zero
status (e.g., log.Fatalf or fmt.Fprintln(os.Stderr, ...); os.Exit(1)) so the
program fails fast with a helpful message instead of producing unclear runtime
errors later.
| s.Accept(*pipePath) | ||
| } | ||
| }) | ||
| s.Accept(*serverPipe) |
There was a problem hiding this comment.
Add error handling for Accept call.
The Accept() call has no error handling, which could lead to silent failures if the server cannot start (e.g., invalid pipe path, permission issues, pipe already in use). Based on learnings, fail-fast is preferred when core services cannot be initialized.
Apply this diff to add error handling:
- s.Accept(*serverPipe)
+ if err := s.Accept(*serverPipe); err != nil {
+ l.Error(err, "Failed to accept connections on server pipe")
+ os.Exit(1)
+ }📝 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.
| s.Accept(*serverPipe) | |
| if err := s.Accept(*serverPipe); err != nil { | |
| l.Error(err, "Failed to accept connections on server pipe") | |
| os.Exit(1) | |
| } |
🤖 Prompt for AI Agents
In kai_analyzer_rpc/main.go around line 88, the call to s.Accept(*serverPipe)
lacks error handling; capture its returned error into a variable, check it
immediately, and fail-fast on error (log the error with context and exit the
process) so the service doesn't continue silently when the pipe cannot be opened
or accepted.
| a.Logger.Info("updated rulesets", "discovery", len(a.discoveryRulesets), "violations", len(a.violationRulesets)) | ||
| } |
There was a problem hiding this comment.
Log lengths for the rule sets actually used.
Use dRulesets/vRulesets, not the struct fields.
- a.Logger.Info("updated rulesets", "discovery", len(a.discoveryRulesets), "violations", len(a.violationRulesets))
+ a.Logger.Info("updated rulesets", "discovery", len(dRulesets), "violations", len(vRulesets))Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In kai_analyzer_rpc/pkg/service/analyzer.go around lines 236 to 237, the Info
log call uses the struct fields a.discoveryRulesets and a.violationRulesets
instead of the local variables that represent the actual rule sets in use
(dRulesets and vRulesets); change the log to use len(dRulesets) for "discovery"
and len(vRulesets) for "violations" so the logged lengths reflect the rule sets
actually used.
| if builtinClient, ok := neededProviders["builtin"]; ok { | ||
| if _, err = builtinClient.ProviderInit(ctx, builtinConfigs); err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| providers["builtin"] = builtinClient | ||
| _, err = builtinClient.ProviderInit(ctx, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Blocker: compile error in builtin init else-branch.
builtinClient is undefined in the else scope. Fetch it from providers.
- } else {
- providers["builtin"] = builtinClient
- _, err = builtinClient.ProviderInit(ctx, nil)
- if err != nil {
- return nil, err
- }
- }
+ } else {
+ builtinClient, ok := providers["builtin"]
+ if !ok {
+ return nil, fmt.Errorf("builtin provider not configured")
+ }
+ if _, err = builtinClient.ProviderInit(ctx, nil); err != nil {
+ return nil, err
+ }
+ }🤖 Prompt for AI Agents
In kai_analyzer_rpc/pkg/service/pipe_analyzer.go around lines 86 to 96, the else
branch references an undefined builtinClient; replace that with a lookup from
the providers map: retrieve builtinClient, check existence (return an error if
missing), then call builtinClient.ProviderInit(ctx, nil). Do not reuse the
neededProviders variable in this branch and ensure providers["builtin"] is only
set if you actually have a valid builtinClient to assign.
savitharaghunathan
left a comment
savitharaghunathan
left a comment
There was a problem hiding this comment.
@shawn-hurley Just a clarification: these changes dont include the addition of main pipe communication with the konveyor core? is that going to be on a followup?
savitharaghunathan
left a comment
savitharaghunathan
left a comment
There was a problem hiding this comment.
visiack
the changes make sense to me.
Signed-off-by: Shawn Hurley <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
kai_analyzer_rpc/pkg/service/pipe_analyzer.go (1)
87-97: Blocker: undefined variable in builtin initialization.The else branch (lines 91-97) references
builtinClientwhich is undefined in this scope. The variable only exists within the if-branch (line 87). This code must retrievebuiltinClientfrom theprovidersmap.This is the same critical issue previously flagged and remains unresolved.
Apply this fix to retrieve builtinClient from the providers map:
- if builtinClient, ok := neededProviders["builtin"]; ok { - if _, err = builtinClient.ProviderInit(ctx, builtinConfigs); err != nil { - return nil, err - } - } else { - providers["builtin"] = builtinClient - _, err = builtinClient.ProviderInit(ctx, nil) - if err != nil { - return nil, err - } - } + if builtinClient, ok := neededProviders["builtin"]; ok { + if _, err = builtinClient.ProviderInit(ctx, builtinConfigs); err != nil { + return nil, err + } + } else { + builtinClient, ok := providers["builtin"] + if !ok { + return nil, fmt.Errorf("builtin provider not found in providers map") + } + if _, err = builtinClient.ProviderInit(ctx, nil); err != nil { + return nil, err + } + }
🧹 Nitpick comments (1)
kai_analyzer_rpc/pkg/service/pipe_analyzer.go (1)
126-126: Remove commented-out code.The commented line appears to be legacy code and should be removed to improve code cleanliness.
contextLines: contextLines, rules: rules, - //updateConditionProvider: updateProviderConditionToUseNewRPClientParseRules, }, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kai_analyzer_rpc/pkg/service/pipe_analyzer.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
kai_analyzer_rpc/pkg/service/pipe_analyzer.go (2)
kai_analyzer_rpc/pkg/service/analyzer.go (1)
Analyzer(31-35)kai_analyzer_rpc/pkg/service/cache.go (1)
NewIncidentsCache(27-33)
⏰ 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). (7)
- GitHub Check: Run e2e test (ubuntu-24.04, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (macos-13, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (windows-latest, cmd, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (macos-latest, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Run e2e test (ubuntu-22.04-arm, bash, ChatOpenAI, kai-test-generation)
- GitHub Check: Check Types on PR Runner (ubuntu-latest, 3.12, x64)
- GitHub Check: Trunk Check Runner
🔇 Additional comments (5)
kai_analyzer_rpc/pkg/service/pipe_analyzer.go (5)
38-40: LGTM!The validation correctly enforces the new socket-based architecture requirement that all providers must use pipes for LSP communication.
52-62: LGTM!The fallback logic correctly ensures the builtin provider is always available by constructing it from locations collected from other provider configurations.
74-86: LGTM!The provider initialization correctly separates builtin from other providers and collects additional configs to pass to the builtin provider, maintaining proper initialization order.
131-184: LGTM!The parseRules function correctly accumulates and returns the providers needed by the parsed rules, properly integrating with the new provider initialization flow.
20-21: ****The context cancellation on line 21 is not premature. The
ProviderInitcalls (lines 80, 88, 93) are synchronous and complete beforeNewPipeAnalyzerreturns, so canceling the context at function exit is proper cleanup. A fresh context is separately created for the engine on line 99.However, there is an actual bug in the
elsebranch (lines 92-93): the variablebuiltinClientis undefined. It's only created conditionally in lines 54-58 whenprovidersdoesn't contain "builtin", but theelsebranch at line 91 assumes it exists regardless. This will cause a runtime panic if theelsebranch is reached.Likely an incorrect or invalid review comment.
2 participants
@djzager @savitharaghunathan If you could take a look
Summary by CodeRabbit
Chores
Refactor
Chores / Breaking Change