Add kai-analyzer-rpc target#35
Open
fabianvf wants to merge 13 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a kai-analyzer RPC target and end-to-end test flow: new CI workflows and Makefile targets, local socket/named-pipe RPC target implementation, rule preparation helpers, parser normalization aware of workDir, and validator changes for RPC-specific relaxed comparisons and subset semantics. Changes
Sequence DiagramsequenceDiagram
participant CLI as Koncur CLI
participant Target as Kai RPC Target
participant FS as File System
participant Server as kai-analyzer (binary)
participant Providers as Provider Containers
CLI->>Target: Execute(test)
Target->>FS: prepareInput, prepareRules, generate provider-config
Target->>Target: startServer(pipePath, rules, providerConfig)
Target->>Server: launch binary (server pipe)
Server->>Providers: request/coordinate provider analysis
Target->>Server: connectToServer (LSP JSON-RPC)
Server-->>Target: readiness notification
Target->>Server: RPC Analyze(AnalyzeArgs)
Providers-->>Server: analysis responses
Server-->>Target: AnalyzeResponse (Rulesets)
Target->>FS: write output/output.yaml
Target->>Server: terminate / cleanup
Target-->>CLI: ExecutionResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
pkg/cli/config.go (2)
372-376: Minor inconsistency in handling empty optional fields.The
binaryPathis assigned to the config even when empty (line 376), whereas other config creators (e.g.,createKantraConfigat lines 201-203) only assign non-empty values. Consider aligning the behavior:♻️ Suggested fix for consistency
binaryPath, err := prompt.Run() if err != nil { return nil, err } - kaiRPCConfig.BinaryPath = binaryPath + if binaryPath != "" { + kaiRPCConfig.BinaryPath = binaryPath + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/config.go` around lines 372 - 376, The code assigns binaryPath to kaiRPCConfig.BinaryPath even when empty; update the logic in the function that prompts for binaryPath (the block that calls prompt.Run() and sets kaiRPCConfig.BinaryPath) to only set kaiRPCConfig.BinaryPath when binaryPath is non-empty (e.g., if binaryPath != "" ) to match the pattern used in createKantraConfig and keep optional fields consistent.
391-395: Same inconsistency withlogFileassignment.Similar to
binaryPath,logFileis assigned even when empty. For consistency with other config creators:♻️ Suggested fix
logFile, err := prompt.Run() if err != nil { return nil, err } - kaiRPCConfig.LogFile = logFile + if logFile != "" { + kaiRPCConfig.LogFile = logFile + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/config.go` around lines 391 - 395, The code assigns kaiRPCConfig.LogFile unconditionally from prompt.Run() even when empty; change the logic after calling prompt.Run() (where logFile is returned) to only set kaiRPCConfig.LogFile when logFile is non-empty (e.g., if logFile != "" { kaiRPCConfig.LogFile = logFile }) so it matches the existing pattern used for binaryPath and other optional fields.Makefile (2)
1-1: Consider adding standardtestandalltargets.The static analysis tool flags missing conventional Makefile targets. While not required, adding them as aliases could improve discoverability:
♻️ Optional: Add conventional targets
-.PHONY: help kind-create kind-delete hub-install hub-uninstall hub-forward hub-status test-hub test-kai-rpc clean build +.PHONY: all test help kind-create kind-delete hub-install hub-uninstall hub-forward hub-status test-hub test-kai-rpc clean build + +all: build ## Default target: build the binary + +test: test-hub test-kai-rpc ## Run all tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, The Makefile lacks conventional "test" and "all" targets; add two phony aliases—create a "test" target that depends on the existing "test-hub" (or aggregates other test targets) and an "all" target that depends on "build" (or the primary build target), and update the .PHONY list to include "test" and "all" so static analysis and users can discover these conventional entry points (refer to existing targets "test-hub" and "build" when wiring the dependencies).
290-291: Consider a readiness check instead of fixed sleep.The 5-second sleep may be insufficient in slow environments or CI runners. Consider polling the provider endpoints for readiness:
♻️ Example readiness check
`@echo` "Waiting for providers to be ready..." - `@sleep` 5 + `@for` i in $$(seq 1 30); do \ + nc -z localhost $(JAVA_PROVIDER_PORT) 2>/dev/null && break || sleep 1; \ + if [ $$i -eq 30 ]; then echo "Warning: java provider may not be ready"; fi; \ + doneAlternatively, increase the sleep duration or add a note that users may need to wait longer in certain environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 290 - 291, Replace the fixed "@sleep 5" wait with a readiness check that polls provider endpoints instead of sleeping; update the Makefile target (the lines containing '@echo "Waiting for providers to be ready..."' and '@sleep 5') to loop and probe the providers' health/readiness URLs until they respond OK or a timeout is reached (or, if simpler, increase the sleep and add a comment advising users to extend the timeout in slow CI). Ensure the polling uses a sensible interval and overall timeout so the target fails deterministically if providers never become ready.pkg/parser/parser_test.go (1)
318-318: Add one test case that uses a non-emptyworkDir.Both updated call sites pass
"", so the newly introduced workdir-aware normalization path is still untested.Also applies to: 467-467
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/parser_test.go` at line 318, The existing tests call NormalizeRuleSets with an empty workDir ("") leaving the workdir-aware branch untested; add a new test case in pkg/parser/parser_test.go that invokes NormalizeRuleSets(tt.input, tt.testDir, "<non-empty-workdir>") (use a representative relative path like "workdir" or a temp dir string), set the expected normalized result for that workDir in the test table, and include assertions for both the returned result and err; update both test locations that currently pass "" to include this additional case so the workdir-aware normalization path exercised by NormalizeRuleSets is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/kai-rpc.yaml:
- Around line 57-64: The step "Create maven settings file" uses an invalid
GitHub Actions context `${{ runner.home }}` in the `path` input for
s4u/maven-settings-action; replace that with a valid context such as `${{
runner.temp }}` (or `${{ env.HOME }}`) so the `path` becomes e.g. `${{
runner.temp }}/.m2/settings.xml`, updating the `path` field in the action block
to use `${{ runner.temp }}` to ensure the settings file is written to a valid
runner directory.
In `@pkg/cli/generate.go`:
- Line 376: The call to parser.NormalizeRuleSets is passing an empty string for
the workDir, which disables workdir-aware normalization and can leak temp paths;
change the third argument from "" to the actual execution work directory
variable (e.g., workDir) so that parser.NormalizeRuleSets(rulesets, testDir,
workDir) is used, ensuring normalization uses the real workDir.
In `@pkg/targets/kai_rpc.go`:
- Around line 324-330: The unbuffered done channel can cause the goroutine that
sends cmd.Wait() to block if the timeout branch wins; change the cleanup to use
a buffered channel or close the channel so the send cannot block (e.g., make
done a buffered channel of size 1 or ensure the goroutine consumes/closess
safely) when invoking cmd.Wait() in the anonymous goroutine and selecting with
time.After(5 * time.Second) and cmd.Process.Kill().
- Around line 227-241: The parent process currently keeps the log file
descriptor `f` open after setting `cmd.Stdout`/`cmd.Stderr`, leaking descriptors
across runs; change the flow around `cmd.Start()` in the function that creates
`logFile` so that on Start error you still call `f.Close()` (already present)
and on successful start you also call `f.Close()` before returning `cmd`—i.e.,
ensure `f.Close()` is invoked after `cmd.Start()` returns successfully (using
the existing `f` variable and `cmd`/`cmd.Start()`), so the child inherits the FD
but the parent does not keep it open.
- Around line 250-281: The wait loop that checks for the socket file (using
pipePath and deadline) can expire without returning an error, allowing the code
to proceed to the dialing/codec setup with conn == nil; modify that logic to
detect when the deadline was reached (or the loop exited without finding the
socket) and return a hard error (with context, e.g., mentioning pipePath and
timeout) before attempting net.Dial/codec creation; update the function that
contains these loops so that after the file-wait loop completes without finding
the socket (and before the dialing loop that uses net.Dial and conn) it returns
an explicit error (or wraps ctx.Err if canceled) instead of continuing.
In `@pkg/validator/kai_rpc_validator.go`:
- Around line 181-184: The code indexes into
slices.Sorted(maps.Keys(faildFields))[len(faildFields)-1] which will panic if
faildFields is empty; update the incident-matching logic (the block that builds
ValidationError using faildFields, e.g., where ValidationError.Message is set
with i.URI, lineNumberOrZero(i.LineNumber), and fieldError.String()) to first
check if len(faildFields) == 0 and handle that case (for example, set a fallback
message like "no matching fields" or include the full list of expected/actual
field names) instead of indexing; apply the same guard in both places where
faildFields is used so the code no longer assumes the map is non-empty before
calling slices.Sorted/mapping to the last element.
In `@testdata/examples/target-kai-rpc.yaml`:
- Around line 9-11: The config's defaultRulesDir value points to
".koncur/rulesets/stable" but the Makefile target download-rulesets clones
konveyor/rulesets into ".koncur/rulesets"; update the defaultRulesDir key to
".koncur/rulesets" (change the value of defaultRulesDir in target-kai-rpc.yaml)
so it matches the repository location unless you intentionally expect a "stable"
subdirectory; ensure the string exactly reads ".koncur/rulesets".
---
Nitpick comments:
In `@Makefile`:
- Line 1: The Makefile lacks conventional "test" and "all" targets; add two
phony aliases—create a "test" target that depends on the existing "test-hub" (or
aggregates other test targets) and an "all" target that depends on "build" (or
the primary build target), and update the .PHONY list to include "test" and
"all" so static analysis and users can discover these conventional entry points
(refer to existing targets "test-hub" and "build" when wiring the dependencies).
- Around line 290-291: Replace the fixed "@sleep 5" wait with a readiness check
that polls provider endpoints instead of sleeping; update the Makefile target
(the lines containing '@echo "Waiting for providers to be ready..."' and '@sleep
5') to loop and probe the providers' health/readiness URLs until they respond OK
or a timeout is reached (or, if simpler, increase the sleep and add a comment
advising users to extend the timeout in slow CI). Ensure the polling uses a
sensible interval and overall timeout so the target fails deterministically if
providers never become ready.
In `@pkg/cli/config.go`:
- Around line 372-376: The code assigns binaryPath to kaiRPCConfig.BinaryPath
even when empty; update the logic in the function that prompts for binaryPath
(the block that calls prompt.Run() and sets kaiRPCConfig.BinaryPath) to only set
kaiRPCConfig.BinaryPath when binaryPath is non-empty (e.g., if binaryPath != ""
) to match the pattern used in createKantraConfig and keep optional fields
consistent.
- Around line 391-395: The code assigns kaiRPCConfig.LogFile unconditionally
from prompt.Run() even when empty; change the logic after calling prompt.Run()
(where logFile is returned) to only set kaiRPCConfig.LogFile when logFile is
non-empty (e.g., if logFile != "" { kaiRPCConfig.LogFile = logFile }) so it
matches the existing pattern used for binaryPath and other optional fields.
In `@pkg/parser/parser_test.go`:
- Line 318: The existing tests call NormalizeRuleSets with an empty workDir ("")
leaving the workdir-aware branch untested; add a new test case in
pkg/parser/parser_test.go that invokes NormalizeRuleSets(tt.input, tt.testDir,
"<non-empty-workdir>") (use a representative relative path like "workdir" or a
temp dir string), set the expected normalized result for that workDir in the
test table, and include assertions for both the returned result and err; update
both test locations that currently pass "" to include this additional case so
the workdir-aware normalization path exercised by NormalizeRuleSets is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68c62362-e32a-431d-b359-e88be1c498fe
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
.github/workflows/ci.yaml.github/workflows/kai-rpc.yaml.github/workflows/permerge-ci.yamlMakefilego.modpkg/cli/config.gopkg/cli/generate.gopkg/cli/run.gopkg/config/target_config.gopkg/parser/parser.gopkg/parser/parser_test.gopkg/targets/kai_rpc.gopkg/targets/target.gopkg/targets/util.gopkg/validator/base_validator.gopkg/validator/kai_rpc_validator.gopkg/validator/tackle2_hub_validator.gopkg/validator/validator.gotestdata/examples/provider-settings-kai-rpc.yamltestdata/examples/target-kai-rpc.yaml
| // Uses yaml.v2 to match analyzer-lsp's marshalling behavior and avoid circular reference issues | ||
| func saveFilteredOutput(rulesets []konveyor.RuleSet, path string, testDir string) error { | ||
| rulesets, err := parser.NormalizeRuleSets(rulesets, testDir) | ||
| rulesets, err := parser.NormalizeRuleSets(rulesets, testDir, "") |
There was a problem hiding this comment.
Pass the real execution workDir to normalization, not an empty string.
At Line 376, using "" bypasses the new workdir-aware normalization path and can preserve machine-specific temp paths in generated expectations.
🔧 Proposed fix
- if err := saveFilteredOutput(filteredOutput, expectedOutputFile, testDirPath); err != nil {
+ if err := saveFilteredOutput(filteredOutput, expectedOutputFile, testDirPath, result.WorkDir); err != nil {
color.Red(" ✗ Failed to save filtered output: %v", err)
failCount++
continue
}-func saveFilteredOutput(rulesets []konveyor.RuleSet, path string, testDir string) error {
- rulesets, err := parser.NormalizeRuleSets(rulesets, testDir, "")
+func saveFilteredOutput(rulesets []konveyor.RuleSet, path string, testDir string, workDir string) error {
+ rulesets, err := parser.NormalizeRuleSets(rulesets, testDir, workDir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cli/generate.go` at line 376, The call to parser.NormalizeRuleSets is
passing an empty string for the workDir, which disables workdir-aware
normalization and can leak temp paths; change the third argument from "" to the
actual execution work directory variable (e.g., workDir) so that
parser.NormalizeRuleSets(rulesets, testDir, workDir) is used, ensuring
normalization uses the real workDir.
This is the first iteration of kai-rpc support using a TDD approach. The implementation compiles but needs testing against actual kai-analyzer. Changes: - Update KaiRPCConfig to use Unix socket-based config (BinaryPath, ProviderConfigPath, LogFile, Verbosity) instead of Host/Port - Implement KaiRPCTarget with full Execute() flow: - Clone git repos for app and rules - Generate provider config with source location - Start kai-analyzer server process - Connect via Unix socket with LSP-style codec - Call analysis_engine.Analyze RPC method - Write response rulesets to output.yaml - Add simplified LSPCodec for client-side JSON-RPC communication - Update CLI config wizard for new fields - Add cenkalti/rpc2 dependency Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
The builtin provider's file search uses ".git" as an excluded directory
pattern. When .git was removed from cloned repos, stat() failed and the
pattern fell through to regexp.Compile(".git") where "." matches any
character, causing it to match "/git" in "github.com" absolute paths
and incorrectly excluding ALL files from analysis.
Fix: clean .git contents but keep empty directory so stat() succeeds
and the correct directory-based exclusion path is used. Also update
kai-analyzer's analyzer-lsp dependency to v0.9.0 which adds anchoring
for other literal exclusion patterns (vendor, build, target, etc.).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
…zation Add a kai-rpc-specific validator that is strict by default, with only three documented relaxations tied to root-caused upstream bugs in kai-analyzer-rpc: 1. Skip missing rulesets that contain only tags/insights (discovery rulesets not returned by RPC - analyzer.go:323 only returns violation cache) 2. One-directional unmatched comparison (discovery rules separated into discoveryRulesets, their unmatched status never returned) 3. Tolerate nil Effort/Links on violations (incident cache only stores Description/Category/Labels - analyzer.go:365-369) Also: - Fix URI normalization by stripping workDir from incident URIs (kai-rpc runs locally with absolute paths vs container-relative /source/ paths) - Extract compareViolationsUsing helper to eliminate 3x copy-paste across base, tackleHub, and kaiRpc validators - Hoist regexp.MustCompile and filepath.Abs to avoid per-incident overhead - Add draft upstream issues for kai-analyzer-rpc bugs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
- Delegate compareViolationDetails to baseValidator, only override link error suppression when cache drops violation details (77 → 13 lines) - Remove draft issue files from git tracking (keep locally) - Remove dead filepath.IsAbs guards on already-absolute paths - Simplify .git cleanup to RemoveAll + MkdirAll - Trim verbose comments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
Wire kai-rpc.yaml reusable workflow into ci.yaml and permerge-ci.yaml, matching the pattern used by kantra and hub. Adds missing download-rulesets step and CONTAINER_RUNTIME=docker for the test target. Maven support is deferred (TODO) — tests requiring Maven settings will be skipped until the maven mirror plumbing is added. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
Two panics in CI: 1. kai-analyzer crashes on startup because log file path is relative but cmd.Dir changes the working directory. Fix: ensure workDir is absolute at the top of Execute(). 2. The rpc2 client readLoop goroutine panics with nil pointer when the connection dies (server crashed). Fix: recover in the goroutine. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
…support Validator changes: - Broaden skipMissingRuleset to skip any missing ruleset (handles missing providers like dotnet and discovery rulesets not returned by RPC) - Add compareSkipped one-directional check (cache doesn't store skipped status) - Add compareTags one-directional check (discovery tags not fully returned) - Extract compareStringsSubset helper for all three CI changes: - Add skip_maven input to kai-rpc workflow (default true, matching kantra/hub) - Add Maven settings setup using local HTTP server + Docker bridge gateway - Pass skip_maven: false in pre-merge CI to run full test suite Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
Add compareInsights to the comparer interface so insights (discovery metadata) can be validated independently from violations. The base implementation delegates to compareViolations (strict). kaiRpcValidator overrides it one-directional — missing expected insights are tolerated since discovery insights live in a separate cache never returned by RPC. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
… unsupported tests - Add c-sharp-provider container to start/stop-kai-providers Makefile targets - Add csharp provider entry to provider-settings-kai-rpc.yaml - Fix URI normalization for git repos with subdirectory paths (e.g., repo#branch/testdata/nerd-dinner) by stripping the subpath component - Add ErrUnsupported sentinel error so targets can signal tests to skip (e.g., binary analysis on kai-rpc) instead of failing - Update CI workflow to log c-sharp-provider on failure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
Revert sourceSubPath plumbing from shared parser/CLI code — it broke kantra and hub targets. Instead, handle the kantra/kai-rpc URI format mismatch with suffix matching in kaiRpcValidator.incidentsMatch, which needs no external data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/parser/parser.go (1)
167-176:⚠️ Potential issue | 🟠 MajorTrim path roots instead of doing global string replacement.
The new
workDirstripping repeats the same pitfall astestDir:strings.ReplaceAllwill happily rewrite/tmp/workspace/...when the root is/tmp/work, and it can remove the same substring again later in the path. Please switch these branches to a leading-prefix/path-boundary check so normalization does not manufacture invalid URIs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/parser.go` around lines 167 - 176, The code currently uses strings.ReplaceAll to strip testDir and workDir from fileName which can remove interior occurrences and produce invalid URIs; update the logic in the parser function that manipulates fileName so it only trims a leading path root match: normalize testDir and workDir to use slash form (as already done), then check if fileName has the testDir or workDir as a leading prefix followed by either a path separator or end-of-string and only then remove that prefix (preserving the separator semantics), instead of calling strings.ReplaceAll; adjust both the testDir and workDir branches that currently use strings.ReplaceAll(fileName, ...) accordingly and keep using the fact that NormalizeRuleSets pre-resolved workDir to an absolute, slash-normalized form.
🧹 Nitpick comments (2)
pkg/parser/parser_test.go (1)
467-467: Consider adding test coverage for non-emptyworkDirscenarios.The existing tests cover
testDirnormalization well, but with the newworkDirparameter supporting kai-rpc's local execution paths, consider adding test cases that verifyworkDirstripping behavior (e.g., when URIs contain the work directory prefix).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/parser_test.go` at line 467, Add unit tests to pkg/parser/parser_test.go that exercise normalizeIncident with a non-empty workDir to verify it correctly strips workDir prefixes from URIs; specifically, add test cases where tt.incident contains URIs prefixed by the workDir and call normalizeIncident(tt.incident, tt.testDir, workDir) (reference normalizeIncident) and assert the normalized result has the workDir removed while preserving existing testDir behavior. Ensure you include at least one positive case (workDir present) and one negative case (no workDir prefix) to validate both strip and no-op paths.Makefile (1)
378-379: Consider a readiness probe instead of fixed sleep.The
sleep 5may be insufficient in slower CI environments or excessive in fast ones. Consider polling the provider's health endpoint for more reliable readiness detection.♻️ Example readiness check
`@echo` "Waiting for providers to be ready..." `@for` i in $$(seq 1 30); do \ curl -s http://localhost:$(JAVA_PROVIDER_PORT)/health >/dev/null 2>&1 && break || sleep 1; \ if [ $$i -eq 30 ]; then echo "Timeout waiting for java provider"; exit 1; fi; \ done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 378 - 379, Replace the fixed "sleep 5" in the Makefile after the "Waiting for providers to be ready..." echo with a readiness poll that hits the provider health endpoint (use the JAVA_PROVIDER_PORT variable) in a loop with a reasonable timeout and exit non‑zero on timeout; this should curl or wget the /health path repeatedly (sleeping briefly between attempts), break when successful, and print a clear timeout error like "Timeout waiting for java provider" before exiting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/config.go`:
- Around line 378-385: The prompt currently labeled "Provider config path
(required)" does not validate input and allows empty values; update the prompt
creation in config.go to enforce a non-empty value by supplying a Validate
function on the promptui.Prompt (or wrap prompt.Run() in a loop) so it rejects
empty input and reprompts until a non-empty providerConfigPath is returned, then
assign the validated value to kaiRPCConfig.ProviderConfigPath; reference the
prompt variable and kaiRPCConfig.ProviderConfigPath to locate where to add the
validation.
In `@pkg/targets/kai_rpc.go`:
- Around line 205-225: prepareInput and the provider config generation can leave
local Application paths relative, so when startServer sets cmd.Dir = workDir the
analyzer sees them relative to the temp work dir instead of the original
fixture; ensure any Application path fields are converted to absolute paths
before writing the provider config. Modify prepareInput (or the code that writes
the providerConfig prior to startServer) to detect relative local paths and
resolve them against the original source root (or cwd used to produce the input)
so the values in the provider config are absolute; apply the same fix to the
other similar block referenced around the 390-407 region so all local-path
analyses point at the correct source tree when kai-analyzer is started.
- Around line 415-421: The codec currently overwrites rpc.Request.Seq with its
internal counter and only partially protects writes causing request/response
mismatch and corrupted frames; update the LSPCodec.WriteRequest implementation
to use req.Seq as-is (do not set c.seq into req.Seq) and serialize all write
operations to the shared bufio.Writer by holding c.seqLock (or a dedicated write
mutex) for the entire duration of composing and flushing the header and body
(i.e., acquire the mutex before writing header, payload, and calling Flush and
release after Flush) so that concurrent writes cannot interleave; keep c.seq
only if used for internal purposes but do not mutate req.Seq.
In `@pkg/validator/kai_rpc_validator.go`:
- Around line 65-87: The compareInsights function currently only iterates over
actual and thus silently tolerates any missing expected insights; update
compareInsights to also iterate expected and for each key missing from actual
(i.e., present in expected but not in actual) append a ValidationError unless
the insight is a known discovery-cache-only type; use the same ValidationError
shape (Path: fmt.Sprintf("/%s", key), Message: "Missing expected insight: %s",
Actual: nil) and reuse compareViolationDetails for present keys; reference the
compareInsights method, the expected/actual maps of konveyor.Violation, the
ValidationError type, and compareViolationDetails when implementing the
missing-key check.
- Around line 19-25: The skipMissingRuleset implementation currently returns
true unconditionally, allowing ValidateFiles to ignore all missing rulesets;
change kaiRpcValidator.skipMissingRuleset to only return true for the specific
discovery/provider-dependent rulesets (e.g., ones with names/IDs that represent
discovery caches or language-specific providers) and return false for everything
else so ValidateFiles still fails on genuinely missing rulesets; implement this
by checking the incoming konveyor.RuleSet's identifying field(s) (name/ID/type)
against a whitelist/set of known-skippable ruleset identifiers (or by consulting
the validator's provider status if available) inside skipMissingRuleset and use
that check to decide true/false.
---
Outside diff comments:
In `@pkg/parser/parser.go`:
- Around line 167-176: The code currently uses strings.ReplaceAll to strip
testDir and workDir from fileName which can remove interior occurrences and
produce invalid URIs; update the logic in the parser function that manipulates
fileName so it only trims a leading path root match: normalize testDir and
workDir to use slash form (as already done), then check if fileName has the
testDir or workDir as a leading prefix followed by either a path separator or
end-of-string and only then remove that prefix (preserving the separator
semantics), instead of calling strings.ReplaceAll; adjust both the testDir and
workDir branches that currently use strings.ReplaceAll(fileName, ...)
accordingly and keep using the fact that NormalizeRuleSets pre-resolved workDir
to an absolute, slash-normalized form.
---
Nitpick comments:
In `@Makefile`:
- Around line 378-379: Replace the fixed "sleep 5" in the Makefile after the
"Waiting for providers to be ready..." echo with a readiness poll that hits the
provider health endpoint (use the JAVA_PROVIDER_PORT variable) in a loop with a
reasonable timeout and exit non‑zero on timeout; this should curl or wget the
/health path repeatedly (sleeping briefly between attempts), break when
successful, and print a clear timeout error like "Timeout waiting for java
provider" before exiting.
In `@pkg/parser/parser_test.go`:
- Line 467: Add unit tests to pkg/parser/parser_test.go that exercise
normalizeIncident with a non-empty workDir to verify it correctly strips workDir
prefixes from URIs; specifically, add test cases where tt.incident contains URIs
prefixed by the workDir and call normalizeIncident(tt.incident, tt.testDir,
workDir) (reference normalizeIncident) and assert the normalized result has the
workDir removed while preserving existing testDir behavior. Ensure you include
at least one positive case (workDir present) and one negative case (no workDir
prefix) to validate both strip and no-op paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6401ca8-c623-442b-870d-d9797ee8a465
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
.github/workflows/ci.yaml.github/workflows/kai-rpc.yaml.github/workflows/permerge-ci.yamlMakefilego.modpkg/cli/config.gopkg/cli/generate.gopkg/cli/run.gopkg/config/git_url_test.gopkg/config/target_config.gopkg/parser/parser.gopkg/parser/parser_test.gopkg/targets/factory_test.gopkg/targets/kai_rpc.gopkg/targets/kantra.gopkg/targets/kantra_test.gopkg/targets/tackle_hub_test.gopkg/targets/target.gopkg/targets/util.gopkg/validator/base_validator.gopkg/validator/kai_rpc_validator.gopkg/validator/tackle2_hub_validator.gopkg/validator/validator.gotestdata/examples/provider-settings-kai-rpc.yamltestdata/examples/target-kai-rpc.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- testdata/examples/target-kai-rpc.yaml
- .github/workflows/permerge-ci.yaml
- testdata/examples/provider-settings-kai-rpc.yaml
- pkg/cli/run.go
- .github/workflows/kai-rpc.yaml
Comment on lines
+378
to
+385
| prompt = promptui.Prompt{ | ||
| Label: "Provider config path (required)", | ||
| } | ||
| providerConfigPath, err := prompt.Run() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| kaiRPCConfig.Host = host | ||
| kaiRPCConfig.ProviderConfigPath = providerConfigPath |
There was a problem hiding this comment.
Validate that required ProviderConfigPath is not empty.
The prompt labels this field as "required" but doesn't validate user input. If the user presses Enter without providing a value, providerConfigPath will be empty, leading to runtime failures when the config is used.
🛡️ Proposed fix to add validation
prompt = promptui.Prompt{
- Label: "Provider config path (required)",
+ Label: "Provider config path (required)",
+ Validate: func(input string) error {
+ if input == "" {
+ return fmt.Errorf("provider config path is required")
+ }
+ return nil
+ },
}
providerConfigPath, err := prompt.Run()
if err != nil {
return nil, err
}📝 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
| prompt = promptui.Prompt{ | |
| Label: "Provider config path (required)", | |
| } | |
| providerConfigPath, err := prompt.Run() | |
| if err != nil { | |
| return nil, err | |
| } | |
| kaiRPCConfig.Host = host | |
| kaiRPCConfig.ProviderConfigPath = providerConfigPath | |
| prompt = promptui.Prompt{ | |
| Label: "Provider config path (required)", | |
| Validate: func(input string) error { | |
| if input == "" { | |
| return fmt.Errorf("provider config path is required") | |
| } | |
| return nil | |
| }, | |
| } | |
| providerConfigPath, err := prompt.Run() | |
| if err != nil { | |
| return nil, err | |
| } | |
| kaiRPCConfig.ProviderConfigPath = providerConfigPath |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cli/config.go` around lines 378 - 385, The prompt currently labeled
"Provider config path (required)" does not validate input and allows empty
values; update the prompt creation in config.go to enforce a non-empty value by
supplying a Validate function on the promptui.Prompt (or wrap prompt.Run() in a
loop) so it rejects empty input and reprompts until a non-empty
providerConfigPath is returned, then assign the validated value to
kaiRPCConfig.ProviderConfigPath; reference the prompt variable and
kaiRPCConfig.ProviderConfigPath to locate where to add the validation.
Comment on lines
+205
to
+225
| func (k *KaiRPCTarget) startServer(ctx context.Context, pipePath, rules, providerConfig, workDir string) (*exec.Cmd, error) { | ||
| args := []string{ | ||
| "--server-pipe", pipePath, | ||
| "--rules", rules, | ||
| "--provider-config", providerConfig, | ||
| } | ||
| // Log file path must be absolute since cmd.Dir changes the working directory | ||
| var logFilePath string | ||
| if k.logFile != "" { | ||
| logFilePath = k.logFile | ||
| } else { | ||
| logFilePath = filepath.Join(workDir, "kai-analyzer.log") | ||
| } | ||
| if !filepath.IsAbs(logFilePath) { | ||
| logFilePath, _ = filepath.Abs(logFilePath) | ||
| } | ||
| args = append(args, "--log-file", logFilePath) | ||
| args = append(args, "--verbosity", strconv.Itoa(k.verbosity)) | ||
|
|
||
| cmd := exec.CommandContext(ctx, k.binaryPath, args...) | ||
| cmd.Dir = workDir |
There was a problem hiding this comment.
Resolve relative application paths before writing provider config.
For local inputs, prepareInput returns analysis.Application unchanged. Since Line 225 later starts kai-analyzer with cmd.Dir = workDir, a relative application path in the generated provider config resolves against the temp work directory instead of the test fixture, so local-path analyses will point at the wrong source tree.
Also applies to: 390-407
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/targets/kai_rpc.go` around lines 205 - 225, prepareInput and the provider
config generation can leave local Application paths relative, so when
startServer sets cmd.Dir = workDir the analyzer sees them relative to the temp
work dir instead of the original fixture; ensure any Application path fields are
converted to absolute paths before writing the provider config. Modify
prepareInput (or the code that writes the providerConfig prior to startServer)
to detect relative local paths and resolve them against the original source root
(or cwd used to produce the input) so the values in the provider config are
absolute; apply the same fix to the other similar block referenced around the
390-407 region so all local-path analyses point at the correct source tree when
kai-analyzer is started.
Comment on lines
+415
to
+421
| type LSPCodec struct { | ||
| reader *bufio.Reader | ||
| writer *bufio.Writer | ||
| ctx context.Context | ||
| log interface{ Info(string, ...interface{}) } | ||
| seq uint64 | ||
| seqLock sync.Mutex |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For github.com/cenkalti/rpc2, what does Request.Seq represent, and does Codec.WriteRequest need to be safe for concurrent use?
💡 Result:
Request.Seq represents the sequence number chosen by the client. It is a uint64 field in the Request struct, used to match requests with their corresponding responses. Codec.WriteRequest needs to be safe for concurrent use by multiple goroutines, as this is a standard requirement for rpc.Codec implementations in Go's net/rpc package, which rpc2 forks.
Citations:
- 1: https://pkg.go.dev/g.yxqyang.asia/contiv/netplugin/Godeps/_workspace/src/github.com/cenkalti/rpc2
- 2: https://pkg.go.dev/github.com/rxjh-emu/server/share/rpc
🏁 Script executed:
# First, locate the file and check its size
find . -name "kai_rpc.go" -type fRepository: konveyor/koncur
Length of output: 83
🏁 Script executed:
# Read the area around lines 415-421 to see the LSPCodec definition
sed -n '410,430p' pkg/targets/kai_rpc.goRepository: konveyor/koncur
Length of output: 727
🏁 Script executed:
# Read the WriteRequest method implementation around lines 526-551
sed -n '520,560p' pkg/targets/kai_rpc.goRepository: konveyor/koncur
Length of output: 974
🏁 Script executed:
# Find ReadResponse method
rg -n "ReadResponse" pkg/targets/kai_rpc.go -A 20Repository: konveyor/koncur
Length of output: 691
🏁 Script executed:
# Also check what "Also applies to: 526-551" refers to - look for other methods
rg -n "func \(c \*LSPCodec\)" pkg/targets/kai_rpc.goRepository: konveyor/koncur
Length of output: 459
🏁 Script executed:
# Look at ReadHeader to see how it handles responses and seq matching
sed -n '454,516p' pkg/targets/kai_rpc.goRepository: konveyor/koncur
Length of output: 1677
🏁 Script executed:
# Check WriteResponse implementation to see if it also has the same issue
sed -n '555,575p' pkg/targets/kai_rpc.goRepository: konveyor/koncur
Length of output: 279
Use req.Seq and serialize codec writes.
rpc.Request.Seq is the sequence number chosen by the client, and the codec contract requires WriteRequest to be safe for concurrent use. This implementation replaces the ID with its own internal counter and writes through a shared bufio.Writer without a write lock. This causes two issues:
- Request/response mismatch: The codec sends a different sequence number than what the caller expects to correlate with responses.
- Data corruption: Concurrent calls to
WriteRequestcan interleave writes to the buffered writer (theseqLockis released at line 530 before writes at lines 540–548), causing protocol frames to become corrupted.
The fix is to use req.Seq directly and protect all writer calls with a single mutex.
Suggested fix
type LSPCodec struct {
reader *bufio.Reader
writer *bufio.Writer
ctx context.Context
log interface{ Info(string, ...interface{}) }
- seq uint64
- seqLock sync.Mutex
+ writeMu sync.Mutex
// Current response being read
currentResponse *lspResponse
}
@@
func (c *LSPCodec) WriteRequest(req *rpc.Request, v interface{}) error {
- c.seqLock.Lock()
- c.seq++
- seq := c.seq
- c.seqLock.Unlock()
+ c.writeMu.Lock()
+ defer c.writeMu.Unlock()
+ seq := req.Seq
request := lspRequest{
Method: req.Method,
Params: []interface{}{v},
ID: &seq,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/targets/kai_rpc.go` around lines 415 - 421, The codec currently
overwrites rpc.Request.Seq with its internal counter and only partially protects
writes causing request/response mismatch and corrupted frames; update the
LSPCodec.WriteRequest implementation to use req.Seq as-is (do not set c.seq into
req.Seq) and serialize all write operations to the shared bufio.Writer by
holding c.seqLock (or a dedicated write mutex) for the entire duration of
composing and flushing the header and body (i.e., acquire the mutex before
writing header, payload, and calling Flush and release after Flush) so that
concurrent writes cannot interleave; keep c.seq only if used for internal
purposes but do not mutate req.Seq.
Comment on lines
+19
to
+25
| // skipMissingRuleset skips any expected ruleset absent from actual output. | ||
| // Rulesets may be missing because: (1) discovery rulesets are stored in a separate | ||
| // cache that's never included in the RPC response, or (2) the required provider | ||
| // isn't running (e.g. dotnet for C# tests). | ||
| func (k *kaiRpcValidator) skipMissingRuleset(_ konveyor.RuleSet) bool { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Don't globally ignore missing rulesets.
ValidateFiles calls this hook for every expected ruleset miss. Returning true unconditionally means a truncated or even empty kai-rpc result can pass without any “missing ruleset” failures, which makes the validator much weaker than the comment above suggests. Please scope this to the specific discovery/provider-dependent rulesets that are known to be absent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/validator/kai_rpc_validator.go` around lines 19 - 25, The
skipMissingRuleset implementation currently returns true unconditionally,
allowing ValidateFiles to ignore all missing rulesets; change
kaiRpcValidator.skipMissingRuleset to only return true for the specific
discovery/provider-dependent rulesets (e.g., ones with names/IDs that represent
discovery caches or language-specific providers) and return false for everything
else so ValidateFiles still fails on genuinely missing rulesets; implement this
by checking the incoming konveyor.RuleSet's identifying field(s) (name/ID/type)
against a whitelist/set of known-skippable ruleset identifiers (or by consulting
the validator's provider status if available) inside skipMissingRuleset and use
that check to decide true/false.
Comment on lines
+65
to
+87
| // compareInsights validates actual ⊆ expected: returned insights are checked strictly, | ||
| // but missing expected insights are tolerated (discovery insights live in the discovery | ||
| // cache which is never included in the RPC response). | ||
| func (k *kaiRpcValidator) compareInsights(expected, actual map[string]konveyor.Violation) []ValidationError { | ||
| var errors []ValidationError | ||
| for key, act := range actual { | ||
| exp, exists := expected[key] | ||
| if !exists { | ||
| errors = append(errors, ValidationError{ | ||
| Path: fmt.Sprintf("/%s", key), | ||
| Message: fmt.Sprintf("Unexpected insight found: %s", key), | ||
| Actual: act, | ||
| }) | ||
| continue | ||
| } | ||
| detailErrors := k.compareViolationDetails(exp, act) | ||
| for i := range detailErrors { | ||
| detailErrors[i].Path = fmt.Sprintf("/%s%s", key, detailErrors[i].Path) | ||
| } | ||
| errors = append(errors, detailErrors...) | ||
| } | ||
| return errors | ||
| } |
There was a problem hiding this comment.
This makes every missing insight optional.
The loop only walks actual, so once a ruleset exists any expected insight that disappears from the RPC output is silently accepted. That is broader than the documented discovery-cache exception and can hide regressions in non-discovery insights too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/validator/kai_rpc_validator.go` around lines 65 - 87, The compareInsights
function currently only iterates over actual and thus silently tolerates any
missing expected insights; update compareInsights to also iterate expected and
for each key missing from actual (i.e., present in expected but not in actual)
append a ValidationError unless the insight is a known discovery-cache-only
type; use the same ValidationError shape (Path: fmt.Sprintf("/%s", key),
Message: "Missing expected insight: %s", Actual: nil) and reuse
compareViolationDetails for present keys; reference the compareInsights method,
the expected/actual maps of konveyor.Violation, the ValidationError type, and
compareViolationDetails when implementing the missing-key check.
…ules - Fix runner.home → env.HOME in kai-rpc workflow (invalid GH Actions context) - Fix goroutine leak: use buffered done channel in cleanup, drain after kill - Fix potential nil conn: return hard error if socket never appears - Fix FD leak: close log file in parent after cmd.Start() - Fix conn/client leak in connectToServer on timeout/cancel paths - Guard empty faildFields map to prevent index panic in validator - Fix defaultRulesDir path to match Makefile download-rulesets target - Extract shared PrepareRules into util.go (was duplicated in kantra/kai-rpc) - Use else-if for File/Git in PrepareRules to prevent double-adding rules - Remove unused pending/pendingLock from LSPCodec - Replace slices.Sorted()[len-1] with slices.Max() for clarity - Update tests for CustomRule type and new KaiRPCConfig fields Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
shawn-hurley
approved these changes
Mar 25, 2026
shawn-hurley
left a comment
shawn-hurley
left a comment
Collaborator
There was a problem hiding this comment.
Generally I am in favor of this and if it is passing CI I am good to merge to get testing and we can choose to do any of the comments as follow-ons.
One thing that I will note, if we were able to download the provider binaries and the neccesary bits they needed, then we wouldn't need containers and this could run much closer (I think?) to the way that kai would run today. It would allow us to test on mac / windows / linux which might be nice.
I don't know the right way off the top of head how to get all of that so totally ok with a follow up, Just wanted to bring it up to see if you had thoughts.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result, err := NormalizeRuleSets(tt.input, tt.testDir) | ||
| result, err := NormalizeRuleSets(tt.input, tt.testDir, "") |
Collaborator
There was a problem hiding this comment.
nit: Should we add a test for the working directory?
| } | ||
|
|
||
| // Generate unique pipe path for this execution | ||
| pipePath := filepath.Join(os.TempDir(), fmt.Sprintf("kai-rpc-%d.sock", time.Now().UnixNano())) |
Collaborator
There was a problem hiding this comment.
Do we want this to run on Windows as well? We need to handle named pipes if that is the case.
| } | ||
|
|
||
| // LSPCodec implements the LSP-style JSON-RPC codec for client-side communication | ||
| type LSPCodec struct { |
Collaborator
There was a problem hiding this comment.
Should we be able to import/go mod these things? (should we??)
It may simplify if there is a client in kai_analyzer_rpc to re-use that, then rebuild one?
|
|
||
| // PrepareRules handles rules that may be Git URLs or local paths. | ||
| // Returns a list of prepared rule paths ready for use by any target. | ||
| func PrepareRules(ctx context.Context, rules []config.CustomRule, testDir, workDir string) ([]string, error) { |
Collaborator
There was a problem hiding this comment.
+100 Thank your for making this!
Add platform-specific pipe handling via build-tagged files so kai-rpc works on both Unix (domain sockets) and Windows (named pipes via go-winio). Extract waitAndDial per platform to avoid dead code paths and optimize retry behavior for each transport. Additional review fixes: - Keep log file handle open until process exits (Windows compat) - Log signalGracefulShutdown errors instead of discarding - Add workDir test case to TestNormalizeRuleSets - Add TODO for upstream kai-analyzer codec import (blocked by module path mismatch) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/targets/tackle_hub_test.go (1)
625-641:⚠️ Potential issue | 🟠 MajorThe test now diverges from
prepareRulesForHub.
TackleHubTarget.prepareRulesForHubinpkg/targets/tackle_hub.gostill rejects mixed local+Git rules and more than one Git repo, but this simulated loop accepts both. The new cases can therefore pass even though the real method would return an error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/targets/tackle_hub_test.go` around lines 625 - 641, The simulated loop in the test diverges from TackleHubTarget.prepareRulesForHub by allowing mixed local+Git rules and multiple Git repos; update the test's simulation to mirror prepareRulesForHub: when iterating test.Analysis.Rules (the same loop that builds taskData.Rules.repositories and taskData.Rules.rules), enforce the same validations used by prepareRulesForHub—reject if both any rule.File and any rule.Git are present (mixed local+Git) and reject if more than one distinct Git repository is added—so the test either asserts the expected error or uses prepareRulesForHub directly to build taskData, referencing the prepareRulesForHub function and the taskData.Rules.repositories / taskData.Rules.rules fields to locate where to apply the checks.
♻️ Duplicate comments (4)
pkg/validator/kai_rpc_validator.go (2)
19-25:⚠️ Potential issue | 🟠 MajorThis still suppresses every missing ruleset.
A truncated or empty kai-rpc result can skip the top-level “missing ruleset” failures entirely. Please scope this to the specific discovery/provider-dependent rulesets that are known to be absent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validator/kai_rpc_validator.go` around lines 19 - 25, The current skipMissingRuleset implementation always returns true, which hides all missing-ruleset failures; change it to only skip known discovery/provider-dependent rulesets by checking the incoming konveyor.RuleSet (use its ID or Name) against an explicit whitelist (e.g., discovery rulesets and provider-specific ones like dotnet) stored on kaiRpcValidator (or a package-level const slice) and return true only when it matches; otherwise return false so genuine missing-ruleset errors are reported (update the function skipMissingRuleset to reference the whitelist and use RuleSet.ID/Name for matching).
65-87:⚠️ Potential issue | 🟠 MajorMissing expected insights are still silently accepted.
Once a ruleset exists, this only validates
actual. Any expected insight that disappears from the RPC output is treated as okay, including non-discovery insights.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validator/kai_rpc_validator.go` around lines 65 - 87, The current compareInsights only iterates actual and ignores any expected keys that are missing; change compareInsights to also iterate expected and for any key present in expected but missing in actual append a ValidationError (Path "/<key>", Message "Expected insight missing: <key>", Actual nil/zero) so missing non-discovery insights are reported; when doing this, still tolerate discovery-cache insights by checking a discovery marker on the expected violation (e.g., exp.Source == "discovery" or exp.IsDiscovery boolean on konveyor.Violation) and skip adding the missing-error for those entries; keep existing logic in compareInsights and compareViolationDetails unchanged and use the same ValidationError struct.pkg/targets/kai_rpc.go (2)
108-109:⚠️ Potential issue | 🟠 MajorResolve local
analysis.Applicationto an absolute path before writing provider config.For local inputs, returning
analysis.Applicationas-is can point Kai to the wrong source when it runs withcmd.Dir = workDir. This should be normalized beforegenerateProviderConfig.🔧 Proposed fix
-func (k *KaiRPCTarget) prepareInput(ctx context.Context, analysis *config.AnalysisConfig, workDir string) (string, error) { +func (k *KaiRPCTarget) prepareInput(ctx context.Context, analysis *config.AnalysisConfig, workDir, testDir string) (string, error) { @@ - // Local path - return analysis.Application, nil + // Local path: resolve against testDir and normalize to absolute path + localPath := analysis.Application + if !filepath.IsAbs(localPath) { + localPath = filepath.Join(testDir, localPath) + } + absPath, err := filepath.Abs(localPath) + if err != nil { + return "", fmt.Errorf("failed to resolve application path %q: %w", analysis.Application, err) + } + return absPath, nil }- inputPath, err := k.prepareInput(ctx, &test.Analysis, workDir) + inputPath, err := k.prepareInput(ctx, &test.Analysis, workDir, test.GetTestDir())Also applies to: 362-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/targets/kai_rpc.go` around lines 108 - 109, The local analysis.Application path must be resolved to an absolute path before writing provider config so Kai won't be pointed to the wrong source when running with cmd.Dir = workDir; update where k.prepareInput(...) returns or where generateProviderConfig(...) is called (and the code paths around prepareInput and generateProviderConfig) to detect local inputs (analysis.Application) and normalize them to an absolute path (e.g., using filepath.Abs or joining with workDir then cleaning) before passing into generateProviderConfig (apply the same fix in the other occurrence around lines 362-378).
389-395:⚠️ Potential issue | 🔴 CriticalUse
req.Seqand serialize the full writer path inWriteRequest.
WriteRequestcurrently rewrites sequence IDs and releases the mutex before writing/flushing a sharedbufio.Writer, which can cause request/response mismatch and interleaved frames under concurrency.🔧 Proposed fix
type LSPCodec struct { reader *bufio.Reader writer *bufio.Writer ctx context.Context log interface{ Info(string, ...interface{}) } - seq uint64 - seqLock sync.Mutex + writeMu sync.Mutex @@ func (c *LSPCodec) WriteRequest(req *rpc.Request, v interface{}) error { - c.seqLock.Lock() - c.seq++ - seq := c.seq - c.seqLock.Unlock() + c.writeMu.Lock() + defer c.writeMu.Unlock() + seq := req.Seq request := lspRequest{ Method: req.Method, Params: []interface{}{v}, ID: &seq, JSONRPC: "2.0", } @@ if _, err := c.writer.Write(body); err != nil { return err } return c.writer.Flush() }For github.com/cenkalti/rpc2 v1.0.4, does the client codec require WriteRequest to be safe for concurrent use, and is rpc.Request.Seq expected to be preserved by the codec for response correlation?Also applies to: 500-525
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/targets/kai_rpc.go` around lines 389 - 395, The current WriteRequest in LSPCodec rewrites req.Seq and releases seqLock before writing/flushing the shared bufio.Writer, causing interleaved frames under concurrency; change it to preserve and use req.Seq (do not overwrite request.Seq with c.seq), and serialize the entire write path by holding seqLock (or a dedicated writer lock) for the full duration of encoding, writing and flushing to c.writer so only one goroutine touches the shared writer at a time; keep any internal c.seq counter separate (if still needed) but do not assign it into rpc.Request.Seq, and ensure WriteRequest uses c.writer and c.writer.Flush while the lock is held.
🧹 Nitpick comments (2)
pkg/parser/parser_test.go (2)
316-346: Strengthen URI assertion to avoidContainsfalse positives.
Current checks can pass even if extra unexpected path parts remain. Consider asserting the full normalized URI for tighter regression detection.Suggested test hardening
validate: func(t *testing.T, result []konveyor.RuleSet) { incident := result[0].Violations["rule1"].Incidents[0] uriStr := string(incident.URI) - if strings.Contains(uriStr, "/tmp/koncur-work-123") { + if strings.Contains(uriStr, "/tmp/koncur-work-123") { t.Errorf("Expected workDir to be stripped from URI, got: %s", uriStr) } - if !strings.Contains(uriStr, "/source/example-1/src/main/java/App.java") { - t.Errorf("Expected path after workDir to be preserved, got: %s", uriStr) + expectedURI := "file:///source/example-1/src/main/java/App.java" + if uriStr != expectedURI { + t.Errorf("Expected normalized URI %s, got: %s", expectedURI, uriStr) } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/parser_test.go` around lines 316 - 346, The test's validate closure in the "workDir normalization" case uses contains checks that can miss extra unexpected path fragments; instead compute the exact expected normalized URI (e.g. "file:///source/example-1/src/main/java/App.java") and assert equality against uriStr (from incident := result[0].Violations["rule1"].Incidents[0]; uriStr := string(incident.URI)) using a single equality check and a clear t.Errorf when they differ to ensure the workDir stripping logic is precisely validated.
500-500: Add at least one directnormalizeIncidenttest with non-emptyworkDir.
Right now all table cases pass"", so the new parameter’s branch is only indirectly covered viaTestNormalizeRuleSets.Suggested coverage extension
tests := []struct { name string incident konveyor.Incident testDir string + workDir string expectedURI uri.URI expectError bool }{ + { + name: "file URI strips workDir", + incident: konveyor.Incident{ + URI: uri.URI("file:///tmp/koncur-work-123/source/app/main.go"), + Message: "Test message", + }, + testDir: "", + workDir: "/tmp/koncur-work-123", + expectedURI: uri.URI("file:///source/app/main.go"), + expectError: false, + }, // existing cases... } @@ - result, err := normalizeIncident(tt.incident, tt.testDir, "") + result, err := normalizeIncident(tt.incident, tt.testDir, tt.workDir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/parser_test.go` at line 500, Add a direct unit test for normalizeIncident that passes a non-empty workDir string to exercise the new branch in normalizeIncident; update or add a table case in parser_test.go (the test that calls normalizeIncident, where result, err := normalizeIncident(tt.incident, tt.testDir, "")) to use a non-empty third argument (e.g., "tmp/workdir" or an os.TempDir path), assert expected output and no error (or specific error behavior) for that case, and ensure any filesystem-dependent expectations (file paths, resolved relative paths) are adjusted or created within the test so it runs deterministically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/kai-rpc.yaml:
- Around line 26-30: The checkout step "Checkout kai repo" currently uses
actions/checkout@v6 to pull repository: konveyor/kai without pinning to a
specific ref; update that step to add a ref field (a commit SHA or tag) so the
workflow checks out an explicit ref instead of the repo's default branch—modify
the step that uses actions/checkout@v6 with repository: konveyor/kai and path:
kai to include ref: <commit-sha-or-tag>.
In `@pkg/targets/kai_rpc_unix.go`:
- Around line 20-47: The function waitAndDial may return (nil, nil) if the
socket never appears and the dial loop never sets err; update waitAndDial to
return a real timeout error instead of nil by setting err =
context.DeadlineExceeded (or a descriptive fmt.Errorf("timed out waiting for
socket %s", path)) when the deadline passes and no connection/error was
produced; ensure this assignment occurs after the initial existence-wait loop
and before the final return so callers receive a non-nil error when deadline is
exceeded.
In `@pkg/targets/kai_rpc.go`:
- Around line 266-269: The callback registered in client.Handle("started", ...)
closes startedCh unguarded which will panic if the "started" notification
arrives multiple times; modify the handler to avoid closing startedCh more than
once by introducing a sync.Once (e.g., startedOnce.Do(func(){ close(startedCh)
})) or replace the close with a non-closing signal send (e.g., a single-value
channel send with select to avoid blocking), updating the
client.Handle("started", ...) closure to use startedOnce or the send-guard so
duplicate notifications no longer cause a panic.
In `@pkg/targets/util.go`:
- Around line 99-105: The cleanup of the .git directory currently ignores errors
from os.RemoveAll and os.MkdirAll and then logs success via log.Info; change
this so failures are propagated instead of silenced: check the error returned by
os.RemoveAll(filepath.Join(absCloneDir, ".git")) and return or wrap that error,
then check the error from os.MkdirAll(gitDir, 0755) and return or wrap that
error before logging success; update the surrounding function signature to
return an error if needed and ensure callers handle it.
- Around line 24-40: The loop preparing rules doesn't validate CustomRule inputs
and silently accepts invalid combinations; add validation in the loop over rules
to require exactly one non-empty source: if both rule.File and rule.Git are nil,
if both are non-nil, or if rule.File.FilePath == "" (after trimming) or
rule.Git.GitRepo == "" (after trimming) then return a descriptive error (include
the rule index and offending values). Place this guard at the start of the for
i, rule := range rules loop in util.go so invalid rules fail fast; keep the
existing behavior for handling absolute vs joined file paths and the
CloneGitRepository call (using rule.Git.GetCompents and rule.Git.GitRepo) after
the validation.
---
Outside diff comments:
In `@pkg/targets/tackle_hub_test.go`:
- Around line 625-641: The simulated loop in the test diverges from
TackleHubTarget.prepareRulesForHub by allowing mixed local+Git rules and
multiple Git repos; update the test's simulation to mirror prepareRulesForHub:
when iterating test.Analysis.Rules (the same loop that builds
taskData.Rules.repositories and taskData.Rules.rules), enforce the same
validations used by prepareRulesForHub—reject if both any rule.File and any
rule.Git are present (mixed local+Git) and reject if more than one distinct Git
repository is added—so the test either asserts the expected error or uses
prepareRulesForHub directly to build taskData, referencing the
prepareRulesForHub function and the taskData.Rules.repositories /
taskData.Rules.rules fields to locate where to apply the checks.
---
Duplicate comments:
In `@pkg/targets/kai_rpc.go`:
- Around line 108-109: The local analysis.Application path must be resolved to
an absolute path before writing provider config so Kai won't be pointed to the
wrong source when running with cmd.Dir = workDir; update where
k.prepareInput(...) returns or where generateProviderConfig(...) is called (and
the code paths around prepareInput and generateProviderConfig) to detect local
inputs (analysis.Application) and normalize them to an absolute path (e.g.,
using filepath.Abs or joining with workDir then cleaning) before passing into
generateProviderConfig (apply the same fix in the other occurrence around lines
362-378).
- Around line 389-395: The current WriteRequest in LSPCodec rewrites req.Seq and
releases seqLock before writing/flushing the shared bufio.Writer, causing
interleaved frames under concurrency; change it to preserve and use req.Seq (do
not overwrite request.Seq with c.seq), and serialize the entire write path by
holding seqLock (or a dedicated writer lock) for the full duration of encoding,
writing and flushing to c.writer so only one goroutine touches the shared writer
at a time; keep any internal c.seq counter separate (if still needed) but do not
assign it into rpc.Request.Seq, and ensure WriteRequest uses c.writer and
c.writer.Flush while the lock is held.
In `@pkg/validator/kai_rpc_validator.go`:
- Around line 19-25: The current skipMissingRuleset implementation always
returns true, which hides all missing-ruleset failures; change it to only skip
known discovery/provider-dependent rulesets by checking the incoming
konveyor.RuleSet (use its ID or Name) against an explicit whitelist (e.g.,
discovery rulesets and provider-specific ones like dotnet) stored on
kaiRpcValidator (or a package-level const slice) and return true only when it
matches; otherwise return false so genuine missing-ruleset errors are reported
(update the function skipMissingRuleset to reference the whitelist and use
RuleSet.ID/Name for matching).
- Around line 65-87: The current compareInsights only iterates actual and
ignores any expected keys that are missing; change compareInsights to also
iterate expected and for any key present in expected but missing in actual
append a ValidationError (Path "/<key>", Message "Expected insight missing:
<key>", Actual nil/zero) so missing non-discovery insights are reported; when
doing this, still tolerate discovery-cache insights by checking a discovery
marker on the expected violation (e.g., exp.Source == "discovery" or
exp.IsDiscovery boolean on konveyor.Violation) and skip adding the missing-error
for those entries; keep existing logic in compareInsights and
compareViolationDetails unchanged and use the same ValidationError struct.
---
Nitpick comments:
In `@pkg/parser/parser_test.go`:
- Around line 316-346: The test's validate closure in the "workDir
normalization" case uses contains checks that can miss extra unexpected path
fragments; instead compute the exact expected normalized URI (e.g.
"file:///source/example-1/src/main/java/App.java") and assert equality against
uriStr (from incident := result[0].Violations["rule1"].Incidents[0]; uriStr :=
string(incident.URI)) using a single equality check and a clear t.Errorf when
they differ to ensure the workDir stripping logic is precisely validated.
- Line 500: Add a direct unit test for normalizeIncident that passes a non-empty
workDir string to exercise the new branch in normalizeIncident; update or add a
table case in parser_test.go (the test that calls normalizeIncident, where
result, err := normalizeIncident(tt.incident, tt.testDir, "")) to use a
non-empty third argument (e.g., "tmp/workdir" or an os.TempDir path), assert
expected output and no error (or specific error behavior) for that case, and
ensure any filesystem-dependent expectations (file paths, resolved relative
paths) are adjusted or created within the test so it runs deterministically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 307416c5-0358-4366-bedf-7aa1aab2db4d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
.github/workflows/kai-rpc.yamlgo.modpkg/config/git_url_test.gopkg/parser/parser_test.gopkg/targets/factory_test.gopkg/targets/kai_rpc.gopkg/targets/kai_rpc_unix.gopkg/targets/kai_rpc_windows.gopkg/targets/kantra.gopkg/targets/kantra_test.gopkg/targets/tackle_hub_test.gopkg/targets/util.gopkg/validator/kai_rpc_validator.gopkg/validator/tackle2_hub_validator.gotestdata/examples/target-kai-rpc.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/targets/factory_test.go
- pkg/targets/kantra.go
- go.mod
- testdata/examples/target-kai-rpc.yaml
- pkg/validator/tackle2_hub_validator.go
Comment on lines
+816
to
824
| // Simulate rule preparation without actually cloning repositories | ||
| preparedRules := make([]string, 0, len(tt.analysis.Rules)) | ||
| for i, rule := range tt.analysis.Rules { | ||
| // Check if we have parsed Git components for this rule | ||
| if i < len(tt.analysis.RulesGitComponents) && tt.analysis.RulesGitComponents[i] != nil { | ||
| if rule.Git != nil { | ||
| // Simulate a cloned path | ||
| preparedRules = append(preparedRules, fmt.Sprintf("testwork/rules-%d", i)) | ||
| } else { | ||
| // Local path - use as-is | ||
| preparedRules = append(preparedRules, rule) | ||
| } else if rule.File != nil { | ||
| preparedRules = append(preparedRules, rule.File.FilePath) | ||
| } |
There was a problem hiding this comment.
This test harness no longer matches PrepareRules.
PrepareRules in pkg/targets/util.go returns the cloned subdirectory for Git rules with #ref/path, but this loop always emits testwork/rules-%d. The new cases for ...#main/java and ...#v1.0/rules therefore assert a path the real implementation would not return.
Comment on lines
+24
to
+40
| for i, rule := range rules { | ||
| if rule.File != nil { | ||
| if filepath.IsAbs(rule.File.FilePath) { | ||
| preparedRules = append(preparedRules, rule.File.FilePath) | ||
| } else { | ||
| preparedRules = append(preparedRules, filepath.Join(testDir, rule.File.FilePath)) | ||
| } | ||
| } else if rule.Git != nil { | ||
| components := rule.Git.GetCompents() | ||
| log.Info("Cloning rules repository", "rule", rule.Git.GitRepo) | ||
| cloneName := fmt.Sprintf("rules-%d", i) | ||
| clonedPath, err := CloneGitRepository(ctx, components, workDir, cloneName) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to clone rules repository %s: %w", rule.Git.GitRepo, err) | ||
| } | ||
| preparedRules = append(preparedRules, clonedPath) | ||
| } |
There was a problem hiding this comment.
Validate each CustomRule before preparing it.
{} is silently skipped, {file: {filePath: ""}} resolves to testDir, and {file: ..., git: ...} silently prefers the file path. Those config mistakes change which rules run instead of failing fast. Require exactly one non-empty source per rule.
Suggested guard
for i, rule := range rules {
+ switch {
+ case rule.File != nil && rule.Git != nil:
+ return nil, fmt.Errorf("rule %d must set only one of file or git", i)
+ case rule.File != nil:
+ if rule.File.FilePath == "" {
+ return nil, fmt.Errorf("rule %d has an empty file path", i)
+ }
+ case rule.Git != nil:
+ if rule.Git.GitRepo == "" {
+ return nil, fmt.Errorf("rule %d has an empty git repo", i)
+ }
+ default:
+ return nil, fmt.Errorf("rule %d must set either file or git", i)
+ }
+
if rule.File != nil {
if filepath.IsAbs(rule.File.FilePath) {
preparedRules = append(preparedRules, rule.File.FilePath)
} else {
preparedRules = append(preparedRules, filepath.Join(testDir, rule.File.FilePath))📝 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
| for i, rule := range rules { | |
| if rule.File != nil { | |
| if filepath.IsAbs(rule.File.FilePath) { | |
| preparedRules = append(preparedRules, rule.File.FilePath) | |
| } else { | |
| preparedRules = append(preparedRules, filepath.Join(testDir, rule.File.FilePath)) | |
| } | |
| } else if rule.Git != nil { | |
| components := rule.Git.GetCompents() | |
| log.Info("Cloning rules repository", "rule", rule.Git.GitRepo) | |
| cloneName := fmt.Sprintf("rules-%d", i) | |
| clonedPath, err := CloneGitRepository(ctx, components, workDir, cloneName) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to clone rules repository %s: %w", rule.Git.GitRepo, err) | |
| } | |
| preparedRules = append(preparedRules, clonedPath) | |
| } | |
| for i, rule := range rules { | |
| switch { | |
| case rule.File != nil && rule.Git != nil: | |
| return nil, fmt.Errorf("rule %d must set only one of file or git", i) | |
| case rule.File != nil: | |
| if rule.File.FilePath == "" { | |
| return nil, fmt.Errorf("rule %d has an empty file path", i) | |
| } | |
| case rule.Git != nil: | |
| if rule.Git.GitRepo == "" { | |
| return nil, fmt.Errorf("rule %d has an empty git repo", i) | |
| } | |
| default: | |
| return nil, fmt.Errorf("rule %d must set either file or git", i) | |
| } | |
| if rule.File != nil { | |
| if filepath.IsAbs(rule.File.FilePath) { | |
| preparedRules = append(preparedRules, rule.File.FilePath) | |
| } else { | |
| preparedRules = append(preparedRules, filepath.Join(testDir, rule.File.FilePath)) | |
| } | |
| } else if rule.Git != nil { | |
| components := rule.Git.GetCompents() | |
| log.Info("Cloning rules repository", "rule", rule.Git.GitRepo) | |
| cloneName := fmt.Sprintf("rules-%d", i) | |
| clonedPath, err := CloneGitRepository(ctx, components, workDir, cloneName) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to clone rules repository %s: %w", rule.Git.GitRepo, err) | |
| } | |
| preparedRules = append(preparedRules, clonedPath) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/targets/util.go` around lines 24 - 40, The loop preparing rules doesn't
validate CustomRule inputs and silently accepts invalid combinations; add
validation in the loop over rules to require exactly one non-empty source: if
both rule.File and rule.Git are nil, if both are non-nil, or if
rule.File.FilePath == "" (after trimming) or rule.Git.GitRepo == "" (after
trimming) then return a descriptive error (include the rule index and offending
values). Place this guard at the start of the for i, rule := range rules loop in
util.go so invalid rules fail fast; keep the existing behavior for handling
absolute vs joined file paths and the CloneGitRepository call (using
rule.Git.GetCompents and rule.Git.GitRepo) after the validation.
…tests - Return real timeout error from waitAndDial instead of (nil, nil) - Guard startedCh close with sync.Once to prevent panic on duplicate notifications - Propagate .git cleanup errors in CloneGitRepository - Add kai_ref workflow input for pinning konveyor/kai version - Tighten URI assertion in workDir normalization test to exact equality - Add normalizeIncident test case exercising workDir stripping Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/targets/kai_rpc.go (2)
377-379:⚠️ Potential issue | 🟠 MajorResolve relative local paths to absolute before returning.
When
analysis.Applicationis a relative path, it's returned unchanged. Sincekai-analyzerruns withcmd.Dir = workDir(line 225), the path in the provider config will resolve against the temp work directory instead of the original test fixture location.🔧 Proposed fix
// Local path - return analysis.Application, nil + appPath := analysis.Application + if !filepath.IsAbs(appPath) { + absPath, err := filepath.Abs(appPath) + if err != nil { + return "", fmt.Errorf("failed to resolve absolute application path: %w", err) + } + appPath = absPath + } + return appPath, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/targets/kai_rpc.go` around lines 377 - 379, The returned analysis.Application may be a relative path and will be interpreted relative to the analyzer's working dir; before returning, detect if analysis.Application is relative and convert it to an absolute path (using the original fixture/workDir context) so callers get a canonical path; update the return path logic that currently returns analysis.Application unchanged to call the path resolution (use filepath.IsAbs + filepath.Abs or join with the fixture base) so the provider config receives an absolute path.
500-527:⚠️ Potential issue | 🔴 CriticalUse
req.Seqand hold the lock during writes to prevent corruption.The codec has two issues that remain from previous review:
Request/response mismatch: The internal
seqcounter (line 504) is used instead ofreq.Seq(line 510). Therpc2library usesreq.Seqto correlate responses with pending calls, so using a different sequence number will cause response mismatches.Data corruption from concurrent writes:
seqLockis released at line 505, but the sharedbufio.Writeris written to at lines 520-526 without any synchronization. ConcurrentWriteRequestcalls can interleave their writes, corrupting the protocol frames.🔧 Proposed fix
type LSPCodec struct { reader *bufio.Reader writer *bufio.Writer ctx context.Context log interface{ Info(string, ...interface{}) } - seq uint64 - seqLock sync.Mutex + writeMu sync.Mutex // Current response being read currentResponse *lspResponse }// WriteRequest writes a JSON-RPC request func (c *LSPCodec) WriteRequest(req *rpc.Request, v interface{}) error { - c.seqLock.Lock() - c.seq++ - seq := c.seq - c.seqLock.Unlock() + c.writeMu.Lock() + defer c.writeMu.Unlock() request := lspRequest{ Method: req.Method, Params: []interface{}{v}, - ID: &seq, + ID: &req.Seq, JSONRPC: "2.0", } body, err := json.Marshal(request)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/targets/kai_rpc.go` around lines 500 - 527, Replace use of the internal counter and the unlocked write with a locked, req.Seq-based write: in LSPCodec.WriteRequest use req.Seq (not c.seq) as the JSON-RPC ID and hold c.seqLock for the entire sequence of writing the header, body and calling c.writer.Flush() to prevent concurrent goroutine interleaving; keep the lock only around the write/flush operations (and the seq read) so no other goroutine can write to c.writer while this request is being serialized.
🧹 Nitpick comments (3)
pkg/parser/parser_test.go (1)
482-492: Add one boundary case to prevent accidental over-stripping.Consider a negative test where the URI starts with a similar prefix (e.g.
/tmp/koncur-work-1234/...) andworkDiris/tmp/koncur-work-123, asserting no strip occurs.Proposed test addition
+ { + name: "workDir does not strip similar prefix", + incident: konveyor.Incident{ + URI: uri.URI("file:///tmp/koncur-work-1234/source/example-1/src/main/java/App.java"), + Message: "Test message", + }, + testDir: "", + workDir: "/tmp/koncur-work-123", + expectedURI: uri.URI("file:///tmp/koncur-work-1234/source/example-1/src/main/java/App.java"), + expectError: false, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/parser_test.go` around lines 482 - 492, Add a negative boundary test alongside the existing "workDir stripping" case that ensures similar-but-longer prefixes are not stripped: create a test case (e.g. name "workDir no-overstrip") with konveyor.Incident{URI: uri.URI("file:///tmp/koncur-work-1234/source/..."), Message: "..."} and workDir set to "/tmp/koncur-work-123", then assert expectedURI equals the original full URI and expectError is false; this verifies the stripping logic that examines incident.URI against workDir does exact-prefix or path-boundary checks and does not remove "/tmp/koncur-work-123" from "/tmp/koncur-work-1234".pkg/targets/kai_rpc.go (2)
529-533:WriteResponseno-op may be intentional but consider adding context.The empty implementation is acceptable for a client-only codec, but the comment could be more explicit about why server requests don't need responses (e.g., all server→client messages are notifications).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/targets/kai_rpc.go` around lines 529 - 533, Update the comment on LSPCodec.WriteResponse to explicitly state why the method is intentionally a no-op: explain that this codec is client-only and server→client messages are treated as notifications (no response expected), so the implementation intentionally returns nil; reference the LSPCodec type and the WriteResponse(resp *rpc.Response, v interface{}) method to make the intent clear for future readers.
274-277: Silent panic recovery may obscure connection errors.Recovering and discarding all panics from
client.Run()could hide legitimate connection failures. Consider logging recovered panics at debug level for troubleshooting.🔍 Suggested improvement
// Start client in background (recover from panics if connection dies) go func() { - defer func() { recover() }() + defer func() { + if r := recover(); r != nil { + log.V(1).Info("RPC client panic recovered", "panic", r) + } + }() client.Run() }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/targets/kai_rpc.go` around lines 274 - 277, The anonymous goroutine currently swallows all panics from client.Run() via defer func() { recover() }(), which hides connection errors; change the defer to capture the recovered value (r := recover()) and, if non-nil, log it at debug level (e.g., debugLogger.Debugf("recovered panic from client.Run(): %v", r)) so recovered panics are recorded for troubleshooting; keep the goroutine and call to client.Run() but replace the silent recover with a guarded recover+debug-log using your existing logger variable (e.g., logger, processLogger or rpcLogger).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/targets/kai_rpc.go`:
- Around line 218-220: The filepath.Abs call that sets logFilePath must handle
its returned error instead of discarding it; change the assignment to capture
(absPath, err := filepath.Abs(logFilePath)) and if err != nil handle it
consistently (e.g., return the error from the surrounding function or log and
exit) so logFilePath cannot remain relative on failure—apply the same pattern
used to fix Execute; update the code around the logFilePath filepath.Abs call in
pkg/targets/kai_rpc.go accordingly.
- Around line 101-103: The call to filepath.Abs ignores its error, leaving
workDir potentially relative; change the code around the workDir normalization
so that after calling filepath.Abs(workDir) you check the returned error and
handle it (e.g., return or propagate an error from the current function or log
and fail) instead of discarding it; update the branch that uses filepath.IsAbs
and filepath.Abs to assign the absolute path only when err == nil and produce a
clear error if Abs fails so workDir remains guaranteed absolute.
---
Duplicate comments:
In `@pkg/targets/kai_rpc.go`:
- Around line 377-379: The returned analysis.Application may be a relative path
and will be interpreted relative to the analyzer's working dir; before
returning, detect if analysis.Application is relative and convert it to an
absolute path (using the original fixture/workDir context) so callers get a
canonical path; update the return path logic that currently returns
analysis.Application unchanged to call the path resolution (use filepath.IsAbs +
filepath.Abs or join with the fixture base) so the provider config receives an
absolute path.
- Around line 500-527: Replace use of the internal counter and the unlocked
write with a locked, req.Seq-based write: in LSPCodec.WriteRequest use req.Seq
(not c.seq) as the JSON-RPC ID and hold c.seqLock for the entire sequence of
writing the header, body and calling c.writer.Flush() to prevent concurrent
goroutine interleaving; keep the lock only around the write/flush operations
(and the seq read) so no other goroutine can write to c.writer while this
request is being serialized.
---
Nitpick comments:
In `@pkg/parser/parser_test.go`:
- Around line 482-492: Add a negative boundary test alongside the existing
"workDir stripping" case that ensures similar-but-longer prefixes are not
stripped: create a test case (e.g. name "workDir no-overstrip") with
konveyor.Incident{URI: uri.URI("file:///tmp/koncur-work-1234/source/..."),
Message: "..."} and workDir set to "/tmp/koncur-work-123", then assert
expectedURI equals the original full URI and expectError is false; this verifies
the stripping logic that examines incident.URI against workDir does exact-prefix
or path-boundary checks and does not remove "/tmp/koncur-work-123" from
"/tmp/koncur-work-1234".
In `@pkg/targets/kai_rpc.go`:
- Around line 529-533: Update the comment on LSPCodec.WriteResponse to
explicitly state why the method is intentionally a no-op: explain that this
codec is client-only and server→client messages are treated as notifications (no
response expected), so the implementation intentionally returns nil; reference
the LSPCodec type and the WriteResponse(resp *rpc.Response, v interface{})
method to make the intent clear for future readers.
- Around line 274-277: The anonymous goroutine currently swallows all panics
from client.Run() via defer func() { recover() }(), which hides connection
errors; change the defer to capture the recovered value (r := recover()) and, if
non-nil, log it at debug level (e.g., debugLogger.Debugf("recovered panic from
client.Run(): %v", r)) so recovered panics are recorded for troubleshooting;
keep the goroutine and call to client.Run() but replace the silent recover with
a guarded recover+debug-log using your existing logger variable (e.g., logger,
processLogger or rpcLogger).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b92fae87-8601-4c67-8d8f-05f753a6b270
📒 Files selected for processing (5)
.github/workflows/kai-rpc.yamlpkg/parser/parser_test.gopkg/targets/kai_rpc.gopkg/targets/kai_rpc_unix.gopkg/targets/util.go
✅ Files skipped from review due to trivial changes (1)
- pkg/targets/kai_rpc_unix.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/kai-rpc.yaml
- pkg/targets/util.go
Comment on lines
+101
to
+103
| if !filepath.IsAbs(workDir) { | ||
| workDir, _ = filepath.Abs(workDir) | ||
| } |
There was a problem hiding this comment.
Handle the error from filepath.Abs.
If filepath.Abs fails, workDir remains unchanged (possibly relative), which could cause subtle issues since the comment at line 96 states it "must be absolute."
🔧 Proposed fix
if !filepath.IsAbs(workDir) {
- workDir, _ = filepath.Abs(workDir)
+ absWorkDir, err := filepath.Abs(workDir)
+ if err != nil {
+ return nil, fmt.Errorf("failed to resolve absolute work directory: %w", err)
+ }
+ workDir = absWorkDir
}📝 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 !filepath.IsAbs(workDir) { | |
| workDir, _ = filepath.Abs(workDir) | |
| } | |
| if !filepath.IsAbs(workDir) { | |
| absWorkDir, err := filepath.Abs(workDir) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to resolve absolute work directory: %w", err) | |
| } | |
| workDir = absWorkDir | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/targets/kai_rpc.go` around lines 101 - 103, The call to filepath.Abs
ignores its error, leaving workDir potentially relative; change the code around
the workDir normalization so that after calling filepath.Abs(workDir) you check
the returned error and handle it (e.g., return or propagate an error from the
current function or log and fail) instead of discarding it; update the branch
that uses filepath.IsAbs and filepath.Abs to assign the absolute path only when
err == nil and produce a clear error if Abs fails so workDir remains guaranteed
absolute.
Comment on lines
+218
to
+220
| if !filepath.IsAbs(logFilePath) { | ||
| logFilePath, _ = filepath.Abs(logFilePath) | ||
| } |
There was a problem hiding this comment.
Handle the error from filepath.Abs.
Same issue as in Execute - if filepath.Abs fails, logFilePath may remain relative.
🔧 Proposed fix
if !filepath.IsAbs(logFilePath) {
- logFilePath, _ = filepath.Abs(logFilePath)
+ absPath, err := filepath.Abs(logFilePath)
+ if err != nil {
+ return nil, nil, fmt.Errorf("failed to resolve absolute log file path: %w", err)
+ }
+ logFilePath = absPath
}📝 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 !filepath.IsAbs(logFilePath) { | |
| logFilePath, _ = filepath.Abs(logFilePath) | |
| } | |
| if !filepath.IsAbs(logFilePath) { | |
| absPath, err := filepath.Abs(logFilePath) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("failed to resolve absolute log file path: %w", err) | |
| } | |
| logFilePath = absPath | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/targets/kai_rpc.go` around lines 218 - 220, The filepath.Abs call that
sets logFilePath must handle its returned error instead of discarding it; change
the assignment to capture (absPath, err := filepath.Abs(logFilePath)) and if err
!= nil handle it consistently (e.g., return the error from the surrounding
function or log and exit) so logFilePath cannot remain relative on failure—apply
the same pattern used to fix Execute; update the code around the logFilePath
filepath.Abs call in pkg/targets/kai_rpc.go accordingly.
shawn-hurley
enabled auto-merge (squash)
April 27, 2026 15:57
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.
closes #35
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor