🐛 Implement filepath filtering in gRPC providers for chained conditions #1004
Conversation
Fixes konveyor#1002 where gRPC providers (nodejs.referenced, java.referenced, dotnet.referenced) were ignoring filepath scope from chained conditions. When using 'from:' to chain conditions, the second condition should only analyze files passed from the first condition. Previously, gRPC providers would search the entire codebase instead of respecting the scoped filepaths. Changes: - Extract scoped filepaths from ProviderContext.GetScopedFilepaths() - Filter incidents to only include files from included paths - Filter out incidents from excluded paths (working copy support) - Use O(1) map lookups instead of O(n*m) nested loops for performance - Add URI normalization to handle file://, file:, and plain paths - Add comprehensive unit tests with 47-62x performance improvements Implementation: - nodejs provider: Filter incidents in EvaluateReferenced() - java provider: Enhanced post-filtering in GetAllSymbols() - dotnet provider: Filter in both symbol search and fallback paths - All providers: Consistent pattern with map-based O(1) lookups Performance: - Node.js: 47x faster (0.45ms vs 21.1ms for 10k incidents/100 paths) - Java: 55x faster (0.40ms vs 22.2ms) - Dotnet: 62x faster (0.33ms vs 20.5ms) Test coverage: - Path normalization tests for all URI formats - Filepath filtering tests (included/excluded paths) - No false positives with similar filenames - Performance benchmarks demonstrating speedup Signed-off-by: tsanders <[email protected]>
WalkthroughThis pull request implements file-based filtering across multiple gRPC providers (dotnet, nodejs, java) to respect filepath scope constraints passed from chained conditions via TemplateContext. It introduces a new path normalization utility, adds file search integration to each provider's Evaluate method, and includes comprehensive test coverage for the filtering logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Chain as Chained<br/>Condition
participant Provider as gRPC<br/>Provider
participant FS as FileSearcher
participant LSP as LSP Server
participant Map as FileMap
Chain->>Provider: Evaluate(cond with TemplateContext scope)
Provider->>Provider: Deserialize scope from TemplateContext
Provider->>FS: Create FileSearcher<br/>(BasePath, ConfigConstraints, ScopeConstraints)
FS->>FS: Search files async
FS-->>Map: Build normalized file map
Provider->>LSP: Query symbols/references
LSP-->>Provider: Results
Provider->>Map: Check each result path
Map-->>Provider: Filtered results<br/>(only scoped files)
Provider-->>Chain: Incidents within scope
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2025-12-02T02:57:09.802ZApplied to files:
📚 Learning: 2025-08-21T12:39:46.778ZApplied to files:
📚 Learning: 2025-08-21T12:42:54.499ZApplied to files:
📚 Learning: 2025-07-30T12:11:45.673ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (1)
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: 1
🧹 Nitpick comments (4)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
247-265: Consider extracting shared path normalization utility.The
normalizePathForComparisonfunction is duplicated across the Java, Node.js, and .NET providers. Consider extracting it to a shared utility package (e.g.,provider/pathor similar) in a follow-up PR to reduce duplication and ensure consistent behavior across all providers.external-providers/java-external-provider/pkg/java_external_provider/filepath_filtering_test.go (1)
233-252: Consider using more realistic path generation in benchmarks.Using
string(rune(i))for generating paths produces Unicode characters for values > 127, which may not represent realistic file paths. Consider usingfmt.Sprintffor clearer, more realistic path generation:- path := protocol.DocumentURI("file:///project/src/File" + string(rune(i)) + ".java") + path := protocol.DocumentURI(fmt.Sprintf("file:///project/src/File%d.java", i))This is a minor nit that doesn't affect benchmark validity since both approaches use the same data.
external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go (1)
222-233: Same path generation nit as the Java benchmark.Consider using
fmt.Sprintffor more realistic path generation:- path := uri.URI(uri.File("/project/src/file" + string(rune(i)) + ".tsx")) + path := uri.URI(uri.File(fmt.Sprintf("/project/src/file%d.tsx", i)))external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (1)
116-126: Consider extracting duplicate filtering logic to a helper function.The identical filtering logic is repeated in both the primary LSP path (lines 116-126) and the fallback
csharp:URI path (lines 188-198). Extracting this to a helper would improve maintainability:func isPathFiltered(normalizedPath string, excludedPathsMap, includedPathsMap map[string]bool) bool { if excludedPathsMap[normalizedPath] { return true } if len(includedPathsMap) > 0 && !includedPathsMap[normalizedPath] { return true } return false }Also applies to: 188-198
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go(1 hunks)external-providers/dotnet-external-provider/pkg/dotnet/provider.go(1 hunks)external-providers/dotnet-external-provider/pkg/dotnet/service_client.go(3 hunks)external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go(1 hunks)external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go(4 hunks)external-providers/java-external-provider/pkg/java_external_provider/filepath_filtering_test.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
📚 Learning: 2025-07-30T12:11:45.673Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 859
File: external-providers/java-external-provider/pkg/java_external_provider/dependency.go:694-694
Timestamp: 2025-07-30T12:11:45.673Z
Learning: In the Java external provider dependency walker (external-providers/java-external-provider/pkg/java_external_provider/dependency.go), errors from toDependency function calls should be ignored as they are not considered important by the maintainers.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
🧬 Code graph analysis (6)
external-providers/dotnet-external-provider/pkg/dotnet/provider.go (1)
provider/provider.go (1)
ProviderContext(349-354)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go (1)
external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go (3)
TestNormalizePathForComparison(10-61)TestFilepathFiltering(63-228)BenchmarkFilepathFiltering(230-294)
external-providers/java-external-provider/pkg/java_external_provider/filepath_filtering_test.go (1)
lsp/protocol/tsprotocol.go (4)
WorkspaceSymbol(5052-5063)OrPLocation_workspace_symbol(2764-2766)DocumentURI(1460-1460)BaseSymbolInformation(51-65)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
provider/provider.go (1)
ProviderContext(349-354)
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (2)
provider/provider.go (3)
ProviderContext(349-354)IncidentContext(309-317)Location(319-322)provider/internal/grpc/library.pb.go (6)
IncidentContext(394-405)IncidentContext(418-418)IncidentContext(433-435)Location(342-348)Location(361-361)Location(376-378)
external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go (2)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go (3)
TestNormalizePathForComparison(10-56)TestFilepathFiltering(58-220)BenchmarkFilepathFiltering(222-281)external-providers/java-external-provider/pkg/java_external_provider/filepath_filtering_test.go (3)
TestNormalizePathForComparison(9-55)TestFilepathFiltering(57-231)BenchmarkFilepathFiltering(233-304)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
🔇 Additional comments (14)
external-providers/dotnet-external-provider/pkg/dotnet/provider.go (1)
38-41: LGTM!The inline embedding of
provider.ProviderContextfollows the same pattern used in the Node.js provider and correctly enables filepath scope extraction viaGetScopedFilepaths()during condition evaluation.external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (3)
164-182: LGTM!The
normalizePathForComparisonhelper correctly handles cross-platform path normalization:
- Strips
file://andfile:URI schemes- Cleans path components (
.,..)- Normalizes to forward slashes
- Applies case-insensitive comparison on Windows
197-219: LGTM!The filepath scope extraction and map-building logic is well-implemented:
- Correctly extracts both included and excluded paths from
ProviderContext- Pre-allocates map capacity for performance
- Defensively skips empty strings
- O(1) lookup maps replace O(n*m) nested loops
228-242: LGTM!The filtering logic correctly applies exclusion first, then inclusion, maintaining backward compatibility when no scope is provided.
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2)
187-196: LGTM!The fallback logic correctly prioritizes condition-level filepaths over context-level scoped filepaths, enabling both explicit filepath constraints in rules and implicit scoping via chained conditions.
198-242: LGTM!The filtering implementation is consistent with the Node.js provider:
- Map-based O(1) lookups
- Exclusion before inclusion
- Proper type assertion with debug logging for unexpected location types
external-providers/java-external-provider/pkg/java_external_provider/filepath_filtering_test.go (2)
9-55: LGTM!Comprehensive test coverage for path normalization including URI schemes, path component resolution, and Windows-style paths.
57-231: LGTM!Excellent test coverage for filepath filtering logic with well-structured test cases covering no filtering, inclusion, exclusion, combined filters, similar filename edge cases, and URI/path normalization scenarios.
external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go (2)
10-56: LGTM!Test cases for path normalization are consistent with the Java provider tests and cover the expected URI schemes and path formats for Node.js/TypeScript files.
58-219: LGTM!Test cases correctly use
provider.IncidentContextmatching the Node.js provider's filtering implementation, with appropriate TypeScript/TSX file extensions and comprehensive coverage of filtering scenarios.external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (2)
36-58: LGTM - Well-structured path normalization helper.The function correctly handles cross-platform concerns: preserving
csharp:metadata URIs, stripping URI schemes, cleaning paths, normalizing slashes, and applying case-insensitive comparison on Windows.
86-107: LGTM - Clean map initialization with proper edge case handling.The defensive empty string check prevents invalid entries from polluting the lookup maps, and pre-sizing the maps with
len()is a good micro-optimization.external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go (2)
10-61: LGTM - Comprehensive test coverage for path normalization.The test cases cover all key scenarios: URI schemes (
file://,file:), plain paths, path cleaning (.,..), Windows paths, and importantly thecsharp:metadata URI preservation which is specific to the dotnet provider.
63-228: LGTM - Thorough filtering test cases.Good coverage including the "no false positives - similar filenames" test case (lines 134-145) which validates that
Service.csdoesn't incorrectly matchServiceImpl.csorServiceFactory.cs. The URI normalization and path cleaning tests ensure cross-format compatibility.
external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go
Show resolved
Hide resolved
|
Hey, I just wanted to share because I saw this PR come through, but you may want to consider: https://github.com/konveyor/analyzer-lsp/blob/main/provider/lib.go#L19. This has all encapsulated logic IIRC for all the ways that filepaths can be filtered. |
The filepath filtering tests were failing on Windows because the normalizePathForComparison() function correctly normalizes paths to lowercase on Windows for case-insensitive comparison, but the test expectations were not accounting for this platform difference. Changes: - Updated TestNormalizePathForComparison to adjust expected values to lowercase on Windows - Updated TestFilepathFiltering to normalize expected paths before comparison using the same normalizePathForComparison() function - Special handling for C# metadata URIs which should not be lowercased This ensures tests pass on both Unix and Windows platforms while maintaining correct cross-platform filepath comparison behavior. Signed-off-by: tsanders <[email protected]>
Add missing runtime and strings package imports that are required for Windows path normalization in the test cases. This resolves the CI build failures with "undefined: strings" and "undefined: runtime" errors. Signed-off-by: tsanders <[email protected]>
|
Hey @shawn-hurley, thanks for pointing out the existing filepath filtering logic! I took a look at provider/lib.go#L19 and wanted to explain why I went with a different approach for the gRPC provider filtering: Key difference in use cases:
Why exact path matching instead of patterns:
That said, I'm happy to revisit this if you think pattern support is needed for the gRPC providers. Let me know your thoughts! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
external-providers/java-external-provider/pkg/java_external_provider/filepath_filtering_test.go (1)
241-312: Benchmark may benefit from guarding against compiler dead‑code eliminationBoth benchmark variants build a
filteredslice but never use it after construction. While Go’s compiler is conservative, it’s common in microbenchmarks to store the result in a package‑level variable to prevent possible elimination and make the benchmark intent explicit.Consider something along these lines:
var benchmarkFilteredResult []protocol.WorkspaceSymboland inside each sub‑benchmark, after the inner loop:
benchmarkFilteredResult = filteredThis keeps the benchmark behavior clearly observable without changing the logic.
external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go (3)
13-69: Normalization test coverage looks good; consider a raw Windows backslash case.The table tests schemes,
./.., Windows drive letters, andcsharp:metadata nicely. IfnormalizePathForComparisonis also expected to handle rawC:\project\src\Program.cs‑style inputs, consider adding a case with backslashes to lock that behavior in.
71-237: Filtering tests are strong; you might add Windows/csharp‑metadata scenarios.The matrix already covers no filtering, include‑only, exclude‑only, combined filters, filename collisions, URI scheme normalization, and path cleaning. For extra confidence, you could (a) add a Windows‑only test that proves case‑insensitive matching (e.g., mixed‑case URIs vs included paths) and (b) add a test that documents the intended behavior for
csharp:metadata URIs whenincludedFilepaths/excludedFilepathsare non‑empty (whether they should always pass through or be subject to the same maps).
239-303: Benchmark works; guard against future optimizers and clarify what you’re measuring.Because
filteredis never observed after each run, a future compiler could in theory elide some of the work; the usual Go benchmark pattern is to assign to a package‑level sink (e.g.,var benchmarkSink []protocol.Locationor just anintholdinglen(filtered)) to force the computation to be kept. Depending on whether you want to measure “filtering only” vs “map build + filtering”, you might also consider buildingincludedPathsMapoutside thefor n := 0; n < b.N; n++loop with ab.ResetTimer()before the hot path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go(1 hunks)external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/filepath_filtering_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
external-providers/java-external-provider/pkg/java_external_provider/filepath_filtering_test.go (2)
external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go (3)
TestNormalizePathForComparison(13-69)TestFilepathFiltering(71-237)BenchmarkFilepathFiltering(239-303)external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go (3)
TestNormalizePathForComparison(10-61)TestFilepathFiltering(63-226)BenchmarkFilepathFiltering(228-287)
🔇 Additional comments (2)
external-providers/java-external-provider/pkg/java_external_provider/filepath_filtering_test.go (2)
11-62: Path normalization tests look solid and consistent with provider behaviorThe cases cover the expected URI forms (
file://,file:, plain paths,..,.and Windows-style paths), and the Windows lowercasing branch matches the intended comparison semantics for this provider. Nothing to change here.
64-239: Filepath filtering tests comprehensively exercise include/exclude behaviorThe table-driven tests nicely cover:
- no scope, include-only, exclude-only, both include/exclude precedence
- similar filenames (no false positives)
- URI scheme normalization and path cleaning
Using
normalizePathForComparisonfor both symbol URIs and expected paths keeps these tests portable across OSes. This aligns well with the intended map-based filtering semantics.
…tions This commit addresses issue konveyor#1005 where the builtin provider did not properly filter files when using chained conditions. Changes: 1. Added Filepaths field to fileContentCondition struct to accept filepath scopes from chained conditions 2. Updated filecontent case to pass ConditionFilepaths to FileSearcher 3. Fixed JSON case to use cond.JSON.Filepaths instead of cond.XML.Filepaths 4. Fixed XMLPublicID case to use cond.XMLPublicID.Filepaths instead of cond.XML.Filepaths 5. Added comprehensive tests for filecontent filepath filtering The fix allows builtin.fileContent to properly scope searches to files identified in previous chained conditions, matching the behavior of the gRPC providers fixed in PR konveyor#1004. Fixes konveyor#1005 Signed-off-by: tsanders <[email protected]>
…tions This commit addresses issue konveyor#1005 where the builtin provider did not properly filter files when using chained conditions. Changes: 1. Added Filepaths field to fileContentCondition struct to accept filepath scopes from chained conditions 2. Updated filecontent case to pass ConditionFilepaths to FileSearcher 3. Fixed JSON case to use cond.JSON.Filepaths instead of cond.XML.Filepaths 4. Fixed XMLPublicID case to use cond.XMLPublicID.Filepaths instead of cond.XML.Filepaths 5. Added omitempty tag to xmlPublicIDCondition.Filepaths for consistency with other condition types 6. Added comprehensive tests for filecontent filepath filtering The fix allows builtin.fileContent to properly scope searches to files identified in previous chained conditions, matching the behavior of the gRPC providers fixed in PR konveyor#1004. Fixes konveyor#1005 Signed-off-by: tsanders <[email protected]>
- Add missing runtime and strings imports to filepath_filtering_test.go These are required for Windows path normalization tests that use runtime.GOOS and strings.ToLower() - Add replace directive to use local analyzer-lsp dependency This ensures the generic-external-provider uses the current local version of analyzer-lsp instead of the published version These fixes allow the nodejs filepath filtering tests to compile and run successfully when tested independently. Signed-off-by: tsanders <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go (2)
43-48: Consider adding a test case for backslash-separated Windows paths.The Windows-style test uses forward slashes in the URI (
file:///C:/project/src/App.tsx). Since the PR objectives mention normalizing backslashes to forward slashes for cross-platform handling, consider adding a test case with actual backslash separators (e.g.,C:\project\src\App.tsx) to verify the backslash-to-forward-slash conversion.
175-227: Test duplicates filtering logic instead of calling the production function.The test reimplements the filtering algorithm (lines 178-206) rather than calling the actual filtering function from
service_client.go. While this validates the algorithm's correctness, it doesn't test the real implementation. Consider refactoring to call the production filtering function directly, or extract the filtering logic into a shared helper that both production code and tests can use.- t.Run(tt.name, func(t *testing.T) { - // Build maps for O(1) lookups - excludedPathsMap := make(map[string]bool, len(tt.excludedFilepaths)) - for _, excludedPath := range tt.excludedFilepaths { - normalizedPath := normalizePathForComparison(excludedPath) - excludedPathsMap[normalizedPath] = true - } - - includedPathsMap := make(map[string]bool, len(tt.includedFilepaths)) - for _, includedPath := range tt.includedFilepaths { - normalizedPath := normalizePathForComparison(includedPath) - includedPathsMap[normalizedPath] = true - } - - // Filter incidents - filteredIncidents := []provider.IncidentContext{} - for _, incident := range tt.incidents { - // ... duplicated logic - } + t.Run(tt.name, func(t *testing.T) { + // Call the actual filtering function from service_client.go + filteredIncidents := filterIncidents(tt.incidents, tt.includedFilepaths, tt.excludedFilepaths)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
external-providers/generic-external-provider/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
external-providers/generic-external-provider/go.mod(1 hunks)external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-25T02:05:02.884Z
Learnt from: shawn-hurley
Repo: konveyor/analyzer-lsp PR: 863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: The `go` directive in go.mod only accepts major.minor (e.g., "go 1.23"); patch versions like "go 1.23.11" are invalid. To require a specific patch version, use the separate `toolchain` directive (introduced in Go 1.21), e.g., `toolchain go1.23.11`.
Applied to files:
external-providers/generic-external-provider/go.mod
📚 Learning: 2025-07-25T02:05:02.884Z
Learnt from: shawn-hurley
Repo: konveyor/analyzer-lsp PR: 863
File: external-providers/generic-external-provider/go.mod:3-3
Timestamp: 2025-07-25T02:05:02.884Z
Learning: Since Go 1.21, the go directive in go.mod files supports patch versions (e.g., "go 1.23.11"), not just major.minor format. The syntax "go 1.23.11" is valid and will not cause build failures in modern Go tooling.
Applied to files:
external-providers/generic-external-provider/go.mod
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
🔇 Additional comments (3)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go (1)
243-288: Benchmark structure is well-designed.The benchmark correctly compares the map-based O(1) approach against the nested loop O(n×m) approach. Both variants use consistent allocation patterns, ensuring a fair comparison. This validates the performance claims in the PR objectives.
external-providers/generic-external-provider/go.mod (2)
40-40: Replace directive for local development is appropriate.The
replacedirective allows the external provider to reference uncommitted changes to the parentanalyzer-lspmodule during development and testing, which is a standard monorepo pattern. Ensure this directive is removed or documented before shipping to production.
3-3: Based on the web search results, I can now provide a definitive answer:The
go 1.23.9syntax is valid and correct. The official Go Modules reference confirms that thegodirective accepts full release versions (e.g.,1.23.0), and the Go project's own testdata usesgo 1.23.9. While older toolchains (pre-1.21) historically rejected patch-level versions, modern Go toolchains (including Go 1.23) fully support the X.Y.Z format.The original review comment incorrectly flags this as a potential issue based on conflicting learnings. The learnings were contradictory—one stating patch versions are invalid, the other confirming they are valid. Web search confirms the second learning was accurate.
No issues found with
go 1.23.9syntax in thegodirective.The
go 1.23.9format is valid in modern Go toolchains. The Go Modules reference and official Go testdata confirm that thegodirective accepts full semantic versions including patch levels. No changes are needed.
...viders/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go
Show resolved
Hide resolved
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
Outdated
Show resolved
Hide resolved
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
Show resolved
Hide resolved
Replace string(rune(i)) with strconv.Itoa(i) in benchmark tests to generate valid file paths. Using string(rune(i)) is problematic: - For i < 32: produces control characters (null, tab, newline, etc.) - For i >= 128: produces multi-byte UTF-8 characters - Creates invalid or inconsistent file paths that don't represent realistic scenarios The fix uses strconv.Itoa(i) to generate proper numeric string suffixes for file paths in benchmark tests. Files changed: - generic-external-provider: filepath_filtering_test.go - java-external-provider: filepath_filtering_test.go Related to konveyor#1002 Signed-off-by: tsanders <[email protected]>
Moved normalizePathForComparison from three separate external providers (java, dotnet, nodejs) to provider.NormalizePathForComparison in provider/lib.go. This eliminates code duplication and ensures consistent path normalization across all providers. Also added missing replace directive to java provider go.mod to enable use of local analyzer-lsp module. Signed-off-by: tsanders <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
177-219: Consider extracting the map-building logic into a shared helper.The path normalization and map-building code (lines 180-198) is duplicated across Java, Node.js, and .NET providers. While the author has justified not using
FileSearcher(due to different use cases: pre-filtering filesystem vs. post-filtering LSP results), the map construction itself could be extracted into a helper function inprovider/lib.go:// BuildPathFilterMaps builds normalized path maps for O(1) filtering func BuildPathFilterMaps(included, excluded []string) (includedMap, excludedMap map[string]bool) { excludedMap = make(map[string]bool, len(excluded)) for _, p := range excluded { if p != "" { excludedMap[NormalizePathForComparison(p)] = true } } includedMap = make(map[string]bool, len(included)) for _, p := range included { if p != "" { includedMap[NormalizePathForComparison(p)] = true } } return }This would reduce duplication while keeping the post-filtering approach intact. Based on past review comments, this consolidation was suggested.
🧹 Nitpick comments (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go (1)
231-291: Benchmark setup is realistic; consider optional allocation reportingThe benchmark now uses numeric suffixes via
strconv.Itoa, builds a realistic incident set, and compares map-based vs nested-loop filtering. If you care about allocation profiles as well as timing, you might optionally addb.ReportAllocs()inside each sub-benchmark.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
external-providers/java-external-provider/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go(1 hunks)external-providers/dotnet-external-provider/pkg/dotnet/service_client.go(3 hunks)external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go(1 hunks)external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go(3 hunks)external-providers/java-external-provider/go.mod(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/filepath_filtering_test.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(1 hunks)provider/lib.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- external-providers/java-external-provider/pkg/java_external_provider/filepath_filtering_test.go
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-21T14:12:52.228Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
Applied to files:
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
📚 Learning: 2025-07-30T12:11:45.673Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 859
File: external-providers/java-external-provider/pkg/java_external_provider/dependency.go:694-694
Timestamp: 2025-07-30T12:11:45.673Z
Learning: In the Java external provider dependency walker (external-providers/java-external-provider/pkg/java_external_provider/dependency.go), errors from toDependency function calls should be ignored as they are not considered important by the maintainers.
Applied to files:
external-providers/java-external-provider/go.mod
📚 Learning: 2025-08-21T12:42:54.499Z
Learnt from: jmle
Repo: konveyor/analyzer-lsp PR: 888
File: external-providers/java-external-provider/pkg/java_external_provider/service_client.go:507-521
Timestamp: 2025-08-21T12:42:54.499Z
Learning: In the konveyor/analyzer-lsp Java external provider (external-providers/java-external-provider/pkg/java_external_provider/service_client.go), the maintainers prefer to keep the current JAVA_HOME selection logic in GetJavaHomeForGradle method simple, as they operate in a controlled environment where both Java 8 and Java 17+ are guaranteed to be available, with Java 17+ being a system requirement for running the analyzer.
Applied to files:
external-providers/java-external-provider/go.mod
📚 Learning: 2025-08-21T12:39:46.778Z
Learnt from: jmle
Repo: konveyor/analyzer-lsp PR: 888
File: external-providers/java-external-provider/pkg/java_external_provider/provider.go:662-663
Timestamp: 2025-08-21T12:39:46.778Z
Learning: In the konveyor/analyzer-lsp Java provider, when setting JAVA_HOME for Gradle commands using exec.CommandContext, the maintainers prefer not to preserve the existing environment variables (including PATH) and use `cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", javaHome))` instead of `cmd.Env = append(os.Environ(), fmt.Sprintf("JAVA_HOME=%s", javaHome))`.
Applied to files:
external-providers/java-external-provider/go.mod
🧬 Code graph analysis (5)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go (3)
external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go (1)
TestNormalizePathForComparison(14-70)external-providers/java-external-provider/pkg/java_external_provider/filepath_filtering_test.go (1)
TestNormalizePathForComparison(13-64)provider/lib.go (1)
NormalizePathForComparison(503-533)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (2)
provider/provider.go (1)
ProviderContext(349-354)provider/lib.go (1)
NormalizePathForComparison(503-533)
external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go (1)
provider/lib.go (1)
NormalizePathForComparison(503-533)
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (3)
provider/provider.go (3)
ProviderContext(349-354)IncidentContext(309-317)Location(319-322)provider/lib.go (1)
NormalizePathForComparison(503-533)provider/internal/grpc/library.pb.go (6)
IncidentContext(394-405)IncidentContext(418-418)IncidentContext(433-435)Location(342-348)Location(361-361)Location(376-378)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2)
provider/lib.go (1)
NormalizePathForComparison(503-533)provider/internal/grpc/library.pb.go (3)
Location(342-348)Location(361-361)Location(376-378)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
🔇 Additional comments (14)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2)
186-244: LGTM! Well-structured filepath filtering implementation.The filtering logic correctly:
- Falls back to context filepaths when condition filepaths are empty
- Uses O(1) map lookups for performance
- Handles empty strings gracefully
- Validates location type before processing
246-246: No concerns with formatting change.provider/lib.go (2)
13-13: LGTM on the import addition.The
runtimeimport is appropriately added to support the OS-specific case normalization logic.
503-533: The proposed fix is unnecessary;filepath.Cleanalready handles the normalization.On Windows,
filepath.Cleannormalizes paths like/C:/pathtoC:\pathby removing the leading slash and normalizing separators. The current code already achieves this:
- After stripping
file://, the path becomes/C:/pathfilepath.Clean(/C:/path)on Windows returnsC:\path(removes leading slash, converts/to\)filepath.ToSlashthen converts backslashes to forward slashes, resulting inC:/pathThe leading slash before the drive letter is already removed by
filepath.Cleanbeforefilepath.ToSlashis called. No additional logic is needed.external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
163-163: Minor formatting change - no concerns.external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (4)
36-40: LGTM! Simple and correct Stop implementation.The method properly cancels the context and waits for the command to finish.
63-84: Consistent filtering setup with other providers.The map construction follows the same pattern as Java and Node.js providers.
93-103: LGTM on reference filtering logic.Correctly applies exclusion first, then inclusion filtering with O(1) lookups.
165-175: Verify filtering behavior forcsharp:metadata URIs.Since
NormalizePathForComparisonpreservescsharp:URIs as-is (they're special URI schemes), these paths won't match typical file-based include/exclude lists. This appears intentional—metadata references from assemblies shouldn't be filtered by file path scope.However, if there's ever a need to filter based on csharp: URIs specifically, the current include/exclude lists (which likely contain file paths) won't match. Please confirm this is the expected behavior.
external-providers/generic-external-provider/pkg/server_configurations/nodejs/filepath_filtering_test.go (2)
13-64: NormalizePathForComparison tests correctly mirror provider behaviorThe table covers file://, file:, plain paths,
./.., and Windows-style paths, and theruntime.GOOSlowercasing logic matchesprovider.NormalizePathForComparison. This gives good confidence in cross-platform normalization.
66-229: Filtering tests exercise the key include/exclude and normalization scenariosThe test matrix (no filter, include-only, exclude-only, combined, similar filenames, scheme variations, and
./..cleaning) plus normalized comparisons againstIncidentContext.FileURIcovers the important edge cases and ensures exact path matching semantics are validated.external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go (3)
14-70: Normalization tests correctly capture Windows andcsharp:metadata behaviorThe cases cover schemes, traversal components, Windows-style paths, and a
csharp:metadata URI, with conditional lowercasing on Windows except forcsharp:. This aligns withprovider.NormalizePathForComparison’s special handling for non-file URIs.
72-238: Filepath filtering tests thoroughly validate include/exclude semanticsUsing
protocol.Locationplus normalized URI strings, the tests cover no filter, include-only, exclude-only, combined sets, similar filenames, scheme differences, and path cleaning. This gives strong coverage that the dotnet provider’s post-filtering respects scoped filepaths exactly.
240-304: Benchmark compares the intended algorithms with sensible dataThe benchmark constructs 10k locations and 100 included paths using
strconv.Itoa, then contrasts map-based inclusion with a nested-loop baseline. Structure and data look good for tracking the performance win described in the PR.
Replace manual filepath filtering with FileSearcher to properly handle all three sources of filepath constraints: 1. Provider config constraints (from InitConfig includedPaths/excludedDirs) 2. Rule scope constraints (from GetScopedFilepaths) 3. Condition filepaths (from condition Referenced.Filepaths field) Previous implementation used if-else logic that only considered one source at a time, missing important filtering scenarios. FileSearcher provides proper priority ordering and combines all constraint sources. Changes: - Java provider: Use FileSearcher with all constraint sources - Nodejs provider: Use FileSearcher with DependencyFolders constraints - Dotnet provider: Use FileSearcher for both main and fallback paths Addresses review feedback from shawn-hurley about handling all filepath sources comprehensively rather than treating them as mutually exclusive. Signed-off-by: tsanders <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
209-231: Unnecessary goroutine pattern.The goroutine with immediate
wg.Wait()adds complexity without providing parallelism benefits since the file search must complete before processing symbols. Consider simplifying to a direct synchronous call:- // Run file search in a goroutine to build allowed file map - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - // Note: nodejs conditions don't have a Filepaths field like java does - // so we pass empty ConditionFilepaths - paths, err := fileSearcher.Search(provider.SearchCriteria{ - ConditionFilepaths: []string{}, - }) - if err != nil { - sc.Log.Error(err, "failed to search for files") - return - } - for _, path := range paths { - normalizedPath := provider.NormalizePathForComparison(path) - fileMap[normalizedPath] = struct{}{} - } - }() - - // Wait for file search to complete - wg.Wait() + // Note: nodejs conditions don't have a Filepaths field like java does + // so we pass empty ConditionFilepaths + paths, err := fileSearcher.Search(provider.SearchCriteria{ + ConditionFilepaths: []string{}, + }) + if err != nil { + sc.Log.Error(err, "failed to search for files") + } else { + for _, path := range paths { + normalizedPath := provider.NormalizePathForComparison(path) + fileMap[normalizedPath] = struct{}{} + } + }This also removes the need for the
syncimport on line 9.external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
186-227: FileSearcher runs unconditionally, unlike nodejs provider.The FileSearcher is always instantiated and search always executes, even when there are no filepath constraints. The nodejs provider guards this with
if len(includedFilepaths) > 0 || len(excludedFilepaths) > 0. Consider adding a similar guard here for consistency and to avoid unnecessary filesystem operations:includedFilepaths, excludedFilepaths := condCTX.GetScopedFilepaths() + // Only use FileSearcher if we have any filepath constraints + fileMap := make(map[string]struct{}) + hasConditionFilepaths := len(c.Referenced.Filepaths) > 0 + hasScopeConstraints := len(includedFilepaths) > 0 || len(excludedFilepaths) > 0 + + if hasConditionFilepaths || hasScopeConstraints { fileSearcher := provider.FileSearcher{ BasePath: p.config.Location, ... } + // ... search logic ... + }Additionally, the goroutine with immediate
wg.Wait()provides no parallelism benefit and could be simplified to a synchronous call (same suggestion as in the nodejs provider).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go(3 hunks)external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go(4 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- external-providers/dotnet-external-provider/pkg/dotnet/service_client.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-02T02:57:09.778Z
Learnt from: tsanders-rh
Repo: konveyor/analyzer-lsp PR: 1004
File: external-providers/java-external-provider/go.mod:49-50
Timestamp: 2025-12-02T02:57:09.778Z
Learning: In the konveyor/analyzer-lsp monorepo, all external providers (java-external-provider, dotnet-external-provider, generic-external-provider) include a `replace github.com/konveyor/analyzer-lsp => ../../` (or `../..`) directive in their go.mod files. This replace directive is the established pattern for local development within the monorepo and allows providers to reference the local analyzer-lsp module. When providers are built outside the repository, the versioned dependency declared in the require block is used instead.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
📚 Learning: 2025-11-21T14:12:52.228Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
Applied to files:
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
🧬 Code graph analysis (2)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
provider/lib.go (5)
FileSearcher(20-29)IncludeExcludeConstraints(39-42)GetExcludedDirsFromConfig(439-484)SearchCriteria(34-37)NormalizePathForComparison(503-533)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (2)
provider/provider.go (1)
ProviderContext(349-354)provider/lib.go (4)
FileSearcher(20-29)IncludeExcludeConstraints(39-42)SearchCriteria(34-37)NormalizePathForComparison(503-533)
🔇 Additional comments (3)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
178-231: FileSearcher integration looks good.The implementation correctly:
- Guards FileSearcher creation with constraint checks (line 194)
- Configures provider constraints with dependency folders and rule scope constraints
- Uses
NormalizePathForComparisonconsistently for both map keys and incident lookups- Fails open (no filtering) when search errors occur, which is reasonable for resilience
Also applies to: 240-251
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2)
232-242: Good defensive handling of unexpected location types.The type assertion with logging for unexpected types (line 234) is a good practice. The filtering logic correctly uses normalized paths for O(1) map lookups.
150-153: Potential duplicate filtering: server-side and client-side.The
includedPathsare sent to the language server viaargumentsMap(lines 147-152) AND used in the FileSearcher for client-side filtering (line 195). This could result in double filtering. If the server already respectsIncludedPathsConfigKey, the client-side filtering may be redundant for provider-level includes. However, this redundancy is harmless and provides defense-in-depth.
external-providers/dotnet-external-provider/pkg/dotnet/filepath_filtering_test.go
Outdated
Show resolved
Hide resolved
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go
Outdated
Show resolved
Hide resolved
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go
Outdated
Show resolved
Hide resolved
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go
Outdated
Show resolved
Hide resolved
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go
Outdated
Show resolved
Hide resolved
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
Outdated
Show resolved
Hide resolved
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
Outdated
Show resolved
Hide resolved
Move TestNormalizePathForComparison from provider-specific test files to provider/lib_test.go where the NormalizePathForComparison function now lives. This eliminates test duplication across three providers. Changes: - Add TestNormalizePathForComparison to provider/lib_test.go - Remove duplicate test from java, dotnet, and nodejs provider test files - All tests continue to pass Addresses review comment from shawn-hurley. Signed-off-by: tsanders <[email protected]>
Previously, FileSearcher was only used when rule-scoped constraints existed (len(includedFilepaths) > 0 || len(excludedFilepaths) > 0). This meant provider config constraints would be missed in cases where only provider-level include/exclude paths were specified. Changes: - Remove conditional FileSearcher usage in all three providers - Always create and use FileSearcher to handle all constraint sources: 1. Provider config constraints (from InitConfig) 2. Rule scope constraints (from GetScopedFilepaths) 3. Condition filepaths (from Referenced.Filepaths) - Remove conditional filtering based on fileMap length - Always filter results based on FileSearcher output This ensures comprehensive filepath filtering regardless of which constraint sources are active. Addresses review feedback from shawn-hurley. Signed-off-by: tsanders <[email protected]>
Previously, file search completed before symbol retrieval started. Now both operations run concurrently: 1. Start FileSearcher goroutine to build allowed file map 2. Retrieve symbols from language server (runs in parallel) 3. Wait for file search to complete 4. Filter symbols using the file map Changes: - Dotnet: Move wg.Wait() after GetAllSymbols() - Nodejs: Move wg.Wait() after GetAllDeclarations() - Java: Move FileSearcher setup before RPC call, wg.Wait() after This allows other processing to happen while building the lookup map, improving overall evaluation performance. Addresses review feedback from shawn-hurley. Signed-off-by: tsanders <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
178-255: Avoid dropping all incidents on FileSearcher error and short‑circuit when there’s no scoped path filteringThe new post‑filtering logic is directionally good, but two behaviors here are risky:
Search errors silently erase all incidents
IffileSearcher.Searchreturns an error, the goroutine logs it and returns, leavingfileMapempty. The later loop:for _, incident := range incidentsMap { normalizedIncidentPath := provider.NormalizePathForComparison(string(incident.FileURI)) if _, ok := fileMap[normalizedIncidentPath]; !ok { continue } incidents = append(incidents, incident) }will then drop every incident without surfacing an error to the caller. That turns filesystem/search issues into “Matched: false,” which is very hard to debug.
Consider either:
- Propagating the error from
EvaluateReferenced, or- Falling back to returning the unfiltered
incidentsMapwhen search fails.Extra filesystem walk when no scoped paths are present
Even whenGetScopedFilepaths()returns no includes/excludes and there are no condition filepaths, you still:
- Build a
FileSearcherwith onlyDependencyFoldersexcludes, and- Walk the workspace to populate
fileMap, despiteEvaluateSymbolsalready filtering out dependency folders.That adds overhead on every
nodejs.referencedevaluation without changing the result set in the unscoped case.You could address both points like this:
@@ func (sc *NodeServiceClient) EvaluateReferenced(...) - includedFilepaths, excludedFilepaths := cond.ProviderContext.GetScopedFilepaths() + includedFilepaths, excludedFilepaths := cond.ProviderContext.GetScopedFilepaths() @@ - fileSearcher := provider.FileSearcher{ + fileSearcher := provider.FileSearcher{ BasePath: basePath, @@ - // Run file search in a goroutine to build allowed file map - fileMap := make(map[string]struct{}) + // Run file search in a goroutine to build allowed file map + fileMap := make(map[string]struct{}) var wg sync.WaitGroup wg.Add(1) go func() { @@ - paths, err := fileSearcher.Search(provider.SearchCriteria{ + paths, err := fileSearcher.Search(provider.SearchCriteria{ ConditionFilepaths: []string{}, }) if err != nil { sc.Log.Error(err, "failed to search for files") - return + // leave fileMap empty to signal failure + fileMap = nil + return } @@ - // Wait for file search to complete - wg.Wait() + // Wait for file search to complete + wg.Wait() + + // If file search failed, skip post‑filtering and rely on EvaluateSymbols filters + if fileMap == nil { + // existing behavior: no additional filepath scoping + for _, incident := range incidentsMap { + incidents = append(incidents, incident) + } + if len(incidents) == 0 { + return resp{Matched: false}, nil + } + return resp{Matched: true, Incidents: incidents}, nil + } @@ - for _, incident := range incidentsMap { + for _, incident := range incidentsMap { normalizedIncidentPath := provider.NormalizePathForComparison(string(incident.FileURI)) @@ - if len(incidents) == 0 { + if len(incidents) == 0 {Additionally, you could avoid even constructing the
FileSearcherwhen there are no scoped includes/excludes (and you only care about dependency folders, whichEvaluateSymbolsalready handles), to skip a redundant filesystem traversal.
🧹 Nitpick comments (11)
compare-optimization.sh (1)
27-27: Add error handling tocdcommands.If directory navigation fails, the script continues in the wrong context. Add
|| exitafter bothcdcommands (lines 27 and 56) to safely exit on failure.cd /Users/tsanders/Workspace/kantra + || exit ./kantra analyze \Apply the same fix to line 56.
Also applies to: 56-56
PATTERNFLY_USAGE_ANALYSIS.md (1)
108-108: Minor: Remove redundant adverb.The word "correctly" is repeated in this line. Consider removing one instance or rephrasing for clarity.
- **Combo rules work correctly** - they correctly did NOT match because tackle2-ui doesn't use the deprecated patterns + **Combo rules are working as expected** - they correctly did NOT match because tackle2-ui doesn't use the deprecated patternscompare-containerless.sh (1)
26-26: Add error handling tocdcommands.If directory navigation fails, the script continues in the wrong context. Add
|| exitafter bothcdcommands (lines 26 and 56) to safely exit on failure.cd /Users/tsanders/Workspace/kantra + || exit ./kantra analyze \Apply the same fix to line 56.
Also applies to: 56-56
COMPREHENSIVE_PATTERNFLY_RULESET.md (1)
46-46: Fix markdown formatting: Remove spaces inside emphasis markers.Lines 46 and 108 have spaces inside bold markers per MD037 (no-space-in-emphasis). These should be
**Better Accuracy**not** Better Accuracy **.Suggested fixes:
- Line 46: Change
** Better accuracy through filepath filtering**to**Better accuracy through filepath filtering**- Line 108: Change
**Styling Issues**spacing to ensure no internal spacesAlso applies to: 108-108
compare-optimization-v2.sh (1)
8-8: Add error handling forcdcommands and remove unused variable.Three issues per ShellCheck:
Line 8:
KANTRAvariable is set but never used. Script usescd ... && ./kantradirectly. Remove the unused variable or use it consistently.Lines 26 & 55:
cdcommands lack error handling. If the directory doesn't exist, the script continues in the wrong location and fails silently later. Usecd ... || exitorcd ... || return.Hard-coded paths: Script contains hard-coded workspace paths (
/Users/tsanders/Workspace/...) making it non-portable across environments.Apply error handling fix:
-cd /Users/tsanders/Workspace/kantra +cd /Users/tsanders/Workspace/kantra || exit 1 ./kantra analyze \And either remove the
KANTRAvariable (line 8) or use it:-KANTRA="/Users/tsanders/Workspace/kantra/kantra" +# Kantra binary path +KANTRA_DIR="/Users/tsanders/Workspace/kantra" +cd "$KANTRA_DIR" || exit 1Also applies to: 26-26, 55-55
FINAL_MEASUREMENT_RESULTS.md (1)
22-27: Specify language for fenced code blocks (MD040).Fenced code blocks on lines 22 and 172 lack language specifiers. Add language identifiers for syntax highlighting and tooling compatibility:
- Line 22:
\``→```bash` or appropriate language- Line 172:
\``→```bashor```go` based on contentAlso applies to: 172-184
MEASUREMENT_IMPLEMENTATION_SUMMARY.md (1)
29-32: Specify language for all fenced code blocks (MD040).Four fenced code blocks lack language specifiers (lines 29, 119, 130, 138). Add appropriate language tags:
- Lines with bash/shell commands: use
\``bash`- Lines with Go code: use
\``go`- Lines with output: use
\``text` or no languageAlso applies to: 119-123, 130-135, 138-142
PREPARE_OPTIMIZATION_ANALYSIS.md (1)
347-353: Specify language for fenced code blocks (MD040).Three fenced code blocks lack language specifiers (lines 347, 356, 365). These contain Go code examples—use
```gofor consistency and syntax highlighting.Also applies to: 356-362, 365-371
PREPARE_PROGRESS_PLAN.md (1)
13-29: Specify language for all fenced code blocks (MD040).Five fenced code blocks lack language specifiers (lines 13, 41, 69, 105, 127). Use appropriate language tags for each:
- Shell/bash commands:
```bash- Go code:
```goAlso applies to: 41-63, 69-92, 105-119, 127-137
TESTING_MEASUREMENT_CODE.md (1)
78-81: Specify language for fenced code blocks (MD040).Two fenced code blocks lack language specifiers (lines 78 and 96). Use
```textor```bashfor consistency with expected log output format.Also applies to: 96-106
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (1)
37-40: Add timeout and error handling to Stop().The current implementation may block indefinitely if the process doesn't respond to context cancellation, and errors from
Wait()are silently discarded.Consider adding a timeout and error logging:
func (d *dotnetServiceClient) Stop() { d.cancelFunc() - d.cmd.Wait() + + // Wait for process with timeout + done := make(chan error, 1) + go func() { + done <- d.cmd.Wait() + }() + + select { + case err := <-done: + if err != nil { + d.log.Error(err, "process exited with error") + } + case <-time.After(5 * time.Second): + d.log.Info("process did not exit after 5s, attempting force kill") + if err := d.cmd.Process.Kill(); err != nil { + d.log.Error(err, "failed to kill process") + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
CHAIN_TEMPLATE_ENHANCEMENTS.md(1 hunks)COMPREHENSIVE_PATTERNFLY_RULESET.md(1 hunks)FINAL_MEASUREMENT_RESULTS.md(1 hunks)FINAL_SUMMARY.md(1 hunks)MEASUREMENT_CODE_STATUS.md(1 hunks)MEASUREMENT_IMPLEMENTATION_SUMMARY.md(1 hunks)MEASUREMENT_RESULTS.md(1 hunks)MULTI_CODEBASE_VALIDATION.md(1 hunks)PATTERNFLY_USAGE_ANALYSIS.md(1 hunks)PREPARE_OPTIMIZATION_ANALYSIS.md(1 hunks)PREPARE_OPTIMIZATION_REVISED.md(1 hunks)PREPARE_PROGRESS_PLAN.md(1 hunks)REFINEMENT_RESULTS.md(1 hunks)REFINEMENT_SUMMARY.md(1 hunks)TACKLE2_UI_ANALYSIS_RESULTS.md(1 hunks)TESTING_MEASUREMENT_CODE.md(1 hunks)TESTING_PREPARE_PROGRESS.md(1 hunks)compare-containerless.sh(1 hunks)compare-optimization-v2.sh(1 hunks)compare-optimization.sh(1 hunks)compare-pr976-vs-main.sh(1 hunks)external-providers/dotnet-external-provider/pkg/dotnet/service_client.go(3 hunks)external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go(4 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(1 hunks)patternfly-v5-to-patternfly-v6-toolbar-enhanced.yaml(1 hunks)test-file-only.yaml(1 hunks)test-filepath-chain.yaml(1 hunks)test-kantra-progress.sh(1 hunks)test-prepare-progress-quick.sh(1 hunks)test-prepare-progress.sh(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- MEASUREMENT_RESULTS.md
- CHAIN_TEMPLATE_ENHANCEMENTS.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-21T14:12:52.228Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
Applied to files:
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
📚 Learning: 2025-12-02T02:57:09.802Z
Learnt from: tsanders-rh
Repo: konveyor/analyzer-lsp PR: 1004
File: external-providers/java-external-provider/go.mod:49-50
Timestamp: 2025-12-02T02:57:09.802Z
Learning: In the konveyor/analyzer-lsp monorepo, all external providers (java-external-provider, dotnet-external-provider, generic-external-provider) include a `replace github.com/konveyor/analyzer-lsp => ../../` (or `../..`) directive in their go.mod files. This replace directive is the established pattern for local development within the monorepo and allows providers to reference the local analyzer-lsp module. When providers are built outside the repository, the versioned dependency declared in the require block is used instead.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
🧬 Code graph analysis (2)
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (2)
provider/provider.go (3)
ProviderContext(349-354)Location(319-322)IncidentContext(309-317)provider/lib.go (5)
FileSearcher(20-29)IncludeExcludeConstraints(39-42)GetIncludedPathsFromConfig(409-432)GetExcludedDirsFromConfig(439-484)NormalizePathForComparison(503-533)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (2)
provider/provider.go (1)
ProviderContext(349-354)provider/lib.go (4)
FileSearcher(20-29)IncludeExcludeConstraints(39-42)SearchCriteria(34-37)NormalizePathForComparison(503-533)
🪛 LanguageTool
TESTING_MEASUREMENT_CODE.md
[grammar] ~55-~55: Ensure spelling is correct
Context: ...ep "Prepare phase" ``` Limitation: Builtin provider doesn't use LSPServiceClientBa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
PREPARE_PROGRESS_PLAN.md
[grammar] ~123-~123: Ensure spelling is correct
Context: ... minimal distraction. ### Example with Builtin Provider Only When only builtin provid...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
PREPARE_OPTIMIZATION_REVISED.md
[grammar] ~70-~70: Ensure spelling is correct
Context: ...ugh calculation:** - If each file takes 100ms → 2000 × 100ms = 200 seconds = **3.3 mi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~70-~70: Ensure spelling is correct
Context: ...:** - If each file takes 100ms → 2000 × 100ms = 200 seconds = 3.3 minutes - With ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~71-~71: Ensure spelling is correct
Context: ....3 minutes** - With 10 workers → 2000 × 100ms ÷ 10 = 20 seconds This matches the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~235-~235: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ttleneck (170 seconds) 3. LSP server is probably fine (handles 10 concurrent requests pe...
(REP_PROBABLY)
PREPARE_OPTIMIZATION_ANALYSIS.md
[uncategorized] ~181-~181: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...1. Add multiple worker goroutines ✅ HIGH IMPACT - Change from 1 worker to 10 workers...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~323-~323: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... unchanged 3. Stress testing: - Very large codebase (5000+ files) - Monitor CPU...
(EN_WEAK_ADJECTIVE)
PATTERNFLY_USAGE_ANALYSIS.md
[style] ~108-~108: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: .... Combo rules work correctly - they correctly did NOT match because tackle2-ui doesn'...
(ADVERB_REPETITION_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
TESTING_MEASUREMENT_CODE.md
78-78: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
FINAL_MEASUREMENT_RESULTS.md
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
172-172: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
PREPARE_PROGRESS_PLAN.md
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
FINAL_SUMMARY.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
52-52: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
PREPARE_OPTIMIZATION_REVISED.md
33-33: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
194-194: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
206-206: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
218-218: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
238-238: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
COMPREHENSIVE_PATTERNFLY_RULESET.md
46-46: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
108-108: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
REFINEMENT_RESULTS.md
59-59: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
TESTING_PREPARE_PROGRESS.md
104-104: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
105-105: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
108-108: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
109-109: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
110-110: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
113-113: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
114-114: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
117-117: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
118-118: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
121-121: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
122-122: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
MEASUREMENT_IMPLEMENTATION_SUMMARY.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
119-119: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
130-130: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
138-138: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
PREPARE_OPTIMIZATION_ANALYSIS.md
347-347: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
356-356: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
365-365: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
compare-optimization.sh
[warning] 8-8: KANTRA appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 27-27: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 56-56: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
compare-optimization-v2.sh
[warning] 8-8: KANTRA appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 26-26: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 55-55: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
compare-pr976-vs-main.sh
[warning] 70-70: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 78-78: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 86-86: Declare and assign separately to avoid masking return values.
(SC2155)
compare-containerless.sh
[warning] 8-8: KANTRA appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 26-26: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 56-56: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
🔇 Additional comments (10)
test-file-only.yaml (1)
1-8: Configuration looks good.Simple, well-formed test rule that validates basic file pattern matching. No concerns.
test-filepath-chain.yaml (1)
1-18: Well-formed chained condition test.Correctly demonstrates the filepath filtering feature via chained conditions. The
from:syntax and{{tsxFiles.filepaths}}template reference are properly formatted.TACKLE2_UI_ANALYSIS_RESULTS.md (1)
1-176: Documentation file—no code review needed.This is an analysis report. The content is informational and requires no code review.
MULTI_CODEBASE_VALIDATION.md (1)
1-206: Documentation file—no code review needed.This is a validation results report. No code or executable logic to review.
patternfly-v5-to-patternfly-v6-toolbar-enhanced.yaml (1)
1-213: Well-structured migration rules with proper chaining.The PatternFly rules correctly demonstrate filepath-scoped conditions using the chained
as:/from:pattern. All patterns are well-formed, references are accurate, and customVariables are correctly defined. No syntax issues detected.REFINEMENT_SUMMARY.md (1)
1-243: Approve documentation file.This file provides clear documentation of the PatternFly ruleset refinement process, with well-structured sections explaining the problem, solution, implementation, and lessons learned. The content is appropriate for a project documentation repository.
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (4)
63-81: Well-structured filepath filtering implementation.The FileSearcher initialization correctly combines all three sources of filepath constraints as specified in the PR objectives:
- Provider config constraints (from InitConfig)
- Rule scope constraints (from GetScopedFilepaths)
- Condition filepaths (handled in the Search call)
113-118: Filepath filtering logic is correct.The normalization and map-based lookup correctly implements O(1) filtering as described in the PR objectives.
174-186: Verify filtering behavior forcsharp:metadata URIs.The PR objectives state that
csharp:URIs should be preserved for .NET metadata, but the current filtering logic checks them against fileMap. Since fileMap contains normalized file paths from FileSearcher,csharp:URIs (e.g.,csharp:/metadata/projects/...) will never match and will always be filtered out.This may be intentional if metadata references should be excluded, but please verify:
- Should
csharp:metadata URIs be filtered by filepath scope?- If not, consider skipping filtering for non-file URIs:
case strings.HasPrefix(filename, "csharp"): // As best I understand, this would require the definition to be defined // outside the project...like a third-party dep. // "csharp:/metadata/projects/NerdDinner/assemblies/System.Web.Mvc/symbols/System.Web.Mvc.Controller.cs" split := strings.Split(filename, "assemblies/") if strings.HasPrefix(split[1], namespace) { normalizedRPath := provider.NormalizePathForComparison(string(r.URI)) - // Filter to only files found by FileSearcher - if _, ok := fileMap[normalizedRPath]; !ok { - continue - } + // Skip filtering for metadata URIs (csharp:) as they're not real files + // Only filter if we have file path constraints AND it's a file:// URI + if len(fileMap) > 0 && strings.HasPrefix(string(r.URI), "file") { + if _, ok := fileMap[normalizedRPath]; !ok { + continue + } + } lineNumber := int(r.Range.Start.Line) incidents = append(incidents, provider.IncidentContext{
89-91: Verify whether dotnetCondition or ProviderContext has a Filepaths field.Based on the comment history suggesting
condition.providerContextmay have aFilepathsfield, check if this field exists and should be passed toFileSearcher.Search()instead of an emptyConditionFilepaths: []string{}.
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go
Outdated
Show resolved
Hide resolved
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
Outdated
Show resolved
Hide resolved
Removed files that were accidentally included in a previous commit: - Test scripts with hard-coded paths (*.sh) - Temporary analysis/measurement documentation (*.md) - Test YAML files (test-*.yaml, patternfly-*.yaml) These were personal development files not intended for the repository. Addresses CodeRabbit feedback. Signed-off-by: tsanders <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
145-153: Fix scoped-path logging to report the actual paths usedIn the
else ifbranch you logpathsusingp.includedPaths, which will typically be empty when the scope comes fromcondCTX.GetScopedFilepaths():} else if includedPaths, _ := condCTX.GetScopedFilepaths(); len(includedPaths) > 0 { argumentsMap[provider.IncludedPathsConfigKey] = includedPaths log.V(8).Info("setting search scope by filepaths", "paths", p.includedPaths, "argumentMap", argumentsMap) }This makes the debug output misleading.
Consider:
- } else if includedPaths, _ := condCTX.GetScopedFilepaths(); len(includedPaths) > 0 { - argumentsMap[provider.IncludedPathsConfigKey] = includedPaths - log.V(8).Info("setting search scope by filepaths", "paths", p.includedPaths, "argumentMap", argumentsMap) - } + } else if includedPaths, _ := condCTX.GetScopedFilepaths(); len(includedPaths) > 0 { + argumentsMap[provider.IncludedPathsConfigKey] = includedPaths + log.V(8).Info("setting search scope by filepaths", "paths", includedPaths, "argumentMap", argumentsMap) + }
♻️ Duplicate comments (2)
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (1)
93-96: File search error handling remains unaddressed.This issue was previously flagged: if
fileSearcher.Searchfails,fileMapstays empty and all incidents are silently filtered out. The suggested fix to capturefileSearchErrand skip filtering on failure would prevent false negatives.external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
163-244: Guard filtering behind actual path constraints and avoid dropping all symbols on FileSearcher failureThis block still has two behavioral pitfalls that were noted previously and remain unresolved:
Search errors silently erase all symbols
IffileSearcher.Searchreturns an error, you log it and return from the goroutine, leavingfileMapempty. The subsequent filter treats this as “no allowed files” and drops every symbol, whileGetAllSymbolsstill returnsnilerror. That effectively turns any filesystem/search issue into “0 matches,” which is hard to debug and incorrect for callers.Filtering is applied even when there are no path constraints
As written,FileSearcherand the post‑filtering run on every call, even when there are no scoped paths (no provider‑level includes, no rule‑scope includes/excludes, noReferenced.Filepaths). In that case you still:
- Pay for a full search, and
- Drop all language‑server URIs that don’t correspond to workspace files discovered by
FileSearcher(e.g., JAR/metadata URIs), changing the legacy “full workspace” behavior for unscoped queries.Given the stated goal to restrict only when filepaths or path constraints are present, a safer pattern is:
- Detect upfront whether any path constraints are in play.
- Only run
FileSearcherand filtering when constraints exist.- If
FileSearcherfails, log and return unfilteredrefsrather than an empty set.One way to refactor within this block while preserving your “run search in parallel with RPC” optimization:
@@ func (p *javaServiceClient) GetAllSymbols(ctx context.Context, c javaCondition, condCTX *provider.ProviderContext) ([]protocol.WorkspaceSymbol, error) { - // Use FileSearcher to properly handle all filepath filtering sources: - // 1. Provider config constraints (from InitConfig) - // 2. Rule scope constraints (from GetScopedFilepaths) - // 3. Condition filepaths (from Referenced.Filepaths) - includedFilepaths, excludedFilepaths := condCTX.GetScopedFilepaths() - - fileSearcher := provider.FileSearcher{ - BasePath: p.config.Location, - ProviderConfigConstraints: provider.IncludeExcludeConstraints{ - IncludePathsOrPatterns: p.includedPaths, - ExcludePathsOrPatterns: provider.GetExcludedDirsFromConfig(p.config), - }, - RuleScopeConstraints: provider.IncludeExcludeConstraints{ - IncludePathsOrPatterns: includedFilepaths, - ExcludePathsOrPatterns: excludedFilepaths, - }, - FailFast: true, - Log: p.log, - } - - // Run file search in a goroutine to build allowed file map - fileMap := make(map[string]struct{}) - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - paths, err := fileSearcher.Search(provider.SearchCriteria{ - ConditionFilepaths: c.Referenced.Filepaths, - }) - if err != nil { - p.log.Error(err, "failed to search for files") - return - } - for _, path := range paths { - normalizedPath := provider.NormalizePathForComparison(path) - fileMap[normalizedPath] = struct{}{} - } - }() + // Use FileSearcher to properly handle all filepath filtering sources: + // 1. Provider config constraints (from InitConfig) + // 2. Rule scope constraints (from GetScopedFilepaths) + // 3. Condition filepaths (from Referenced.Filepaths) + includedFilepaths, excludedFilepaths := condCTX.GetScopedFilepaths() + + hasPathConstraints := len(p.includedPaths) > 0 || + len(includedFilepaths) > 0 || + len(excludedFilepaths) > 0 || + len(c.Referenced.Filepaths) > 0 + + var ( + fileMap map[string]struct{} + wg sync.WaitGroup + fileSearchErr error + ) + + if hasPathConstraints { + fileSearcher := provider.FileSearcher{ + BasePath: p.config.Location, + ProviderConfigConstraints: provider.IncludeExcludeConstraints{ + IncludePathsOrPatterns: p.includedPaths, + ExcludePathsOrPatterns: provider.GetExcludedDirsFromConfig(p.config), + }, + RuleScopeConstraints: provider.IncludeExcludeConstraints{ + IncludePathsOrPatterns: includedFilepaths, + ExcludePathsOrPatterns: excludedFilepaths, + }, + FailFast: true, + Log: p.log, + } + + wg.Add(1) + go func() { + defer wg.Done() + paths, err := fileSearcher.Search(provider.SearchCriteria{ + ConditionFilepaths: c.Referenced.Filepaths, + }) + if err != nil { + p.log.Error(err, "failed to search for files, returning unfiltered symbols") + fileSearchErr = err + return + } + m := make(map[string]struct{}, len(paths)) + for _, path := range paths { + normalizedPath := provider.NormalizePathForComparison(path) + m[normalizedPath] = struct{}{} + } + fileMap = m + }() + } @@ - // Wait for file search to complete - wg.Wait() - - // Filter symbols to only those in files found by FileSearcher - var filteredRefs []protocol.WorkspaceSymbol - for _, ref := range refs { - refLocation, ok := ref.Location.Value.(protocol.Location) - if !ok { - p.log.V(7).Info("unexpected location type in workspace symbol", "symbol", ref.Name) - continue - } - normalizedRefURI := provider.NormalizePathForComparison(string(refLocation.URI)) - - // Check if file is in the allowed file map - if _, ok := fileMap[normalizedRefURI]; ok { - filteredRefs = append(filteredRefs, ref) - } - } - return filteredRefs, nil + // If there are no path constraints, keep legacy behavior: return all refs. + if !hasPathConstraints { + return refs, nil + } + + // Wait for file search to complete when constraints are present. + wg.Wait() + + // If the search failed, fall back to unfiltered refs rather than dropping everything. + if fileSearchErr != nil { + return refs, nil + } + + // Filter symbols to only those in files found by FileSearcher. + var filteredRefs []protocol.WorkspaceSymbol + for _, ref := range refs { + refLocation, ok := ref.Location.Value.(protocol.Location) + if !ok { + p.log.V(7).Info("unexpected location type in workspace symbol", "symbol", ref.Name) + continue + } + normalizedRefURI := provider.NormalizePathForComparison(string(refLocation.URI)) + + // Check if file is in the allowed file map + if _, ok := fileMap[normalizedRefURI]; ok { + filteredRefs = append(filteredRefs, ref) + } + } + return filteredRefs, nilThis keeps your parallelism (search running alongside the RPC), enforces scoped‑only filtering, and makes failures visible without turning them into empty results.
🧹 Nitpick comments (1)
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (1)
37-40: Consider handling potential errors fromcmd.Wait().The
Wait()call could return an error (e.g., if the process was already terminated or an I/O error occurred). While the context cancellation should typically cause a clean shutdown, logging any error would aid debugging.func (d *dotnetServiceClient) Stop() { d.cancelFunc() - d.cmd.Wait() + if err := d.cmd.Wait(); err != nil { + d.log.V(5).Info("process wait completed with error", "error", err) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go(3 hunks)external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go(3 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-21T14:12:52.228Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
Applied to files:
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
📚 Learning: 2025-12-02T02:57:09.802Z
Learnt from: tsanders-rh
Repo: konveyor/analyzer-lsp PR: 1004
File: external-providers/java-external-provider/go.mod:49-50
Timestamp: 2025-12-02T02:57:09.802Z
Learning: In the konveyor/analyzer-lsp monorepo, all external providers (java-external-provider, dotnet-external-provider, generic-external-provider) include a `replace github.com/konveyor/analyzer-lsp => ../../` (or `../..`) directive in their go.mod files. This replace directive is the established pattern for local development within the monorepo and allows providers to reference the local analyzer-lsp module. When providers are built outside the repository, the versioned dependency declared in the require block is used instead.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
🧬 Code graph analysis (3)
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (1)
provider/provider.go (1)
Location(319-322)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
provider/lib.go (4)
FileSearcher(20-29)IncludeExcludeConstraints(39-42)SearchCriteria(34-37)NormalizePathForComparison(503-533)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2)
provider/lib.go (5)
FileSearcher(20-29)IncludeExcludeConstraints(39-42)GetExcludedDirsFromConfig(439-484)SearchCriteria(34-37)NormalizePathForComparison(503-533)provider/provider.go (1)
Location(319-322)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
🔇 Additional comments (4)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (2)
240-247: LGTM!The filtering logic correctly uses O(1) map lookups with normalized paths for consistent cross-platform comparison, aligning with the PR objectives.
192-204: Inconsistent ProviderConfigConstraints setup compared to dotnet provider.The dotnet provider uses
provider.GetIncludedPathsFromConfigandprovider.GetExcludedDirsFromConfigfor the ProviderConfigConstraints, while nodejs uses empty includes andsc.BaseConfig.DependencyFolders. This inconsistency could lead to different filtering behavior between providers.Consider aligning with the dotnet implementation for consistency:
fileSearcher := provider.FileSearcher{ BasePath: basePath, ProviderConfigConstraints: provider.IncludeExcludeConstraints{ - IncludePathsOrPatterns: []string{}, - ExcludePathsOrPatterns: sc.BaseConfig.DependencyFolders, + IncludePathsOrPatterns: provider.GetIncludedPathsFromConfig(sc.BaseConfig, true), + ExcludePathsOrPatterns: provider.GetExcludedDirsFromConfig(sc.BaseConfig), },Verify that
GetIncludedPathsFromConfigandGetExcludedDirsFromConfigare compatible withBaseConfigbefore applying this change.external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (2)
63-81: LGTM!The FileSearcher configuration properly uses the provider helper functions (
GetIncludedPathsFromConfig,GetExcludedDirsFromConfig) to consolidate all filepath filtering sources.
114-119: LGTM!The filtering correctly applies to file-based references using normalized path comparison with O(1) map lookup.
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go
Show resolved
Hide resolved
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
Show resolved
Hide resolved
Previously, if FileSearcher.Search() failed, the error was logged but fileMap remained empty, causing all results to be filtered out (false negatives). Changes: - Capture fileSearchErr in goroutine when search fails - Set skipFiltering flag if error occurred - Skip filtering when search failed to avoid losing valid results This ensures that if we can't determine which files to filter, we return all results rather than incorrectly returning nothing. Applied to all three providers (java, dotnet, nodejs). Addresses CodeRabbit feedback. Signed-off-by: tsanders <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
📚 Learning: 2025-11-21T14:12:52.228Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
Applied to files:
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
📚 Learning: 2025-10-21T16:27:59.216Z
Learnt from: shawn-hurley
Repo: konveyor/analyzer-lsp PR: 860
File: provider/server.go:251-259
Timestamp: 2025-10-21T16:27:59.216Z
Learning: In provider/server.go, the Init method uses context.Background() for creating the RPC client via GetProviderRPCClient because this RPC client is stored in the InitConfig and used across multiple subsequent requests (Evaluate, GetDependencies, etc.). The RPC client's lifecycle is managed separately through explicit Stop calls, not tied to the Init request's context.
Applied to files:
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
🧬 Code graph analysis (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
provider/lib.go (5)
GetIncludedPathsFromConfig(415-438)FileSearcher(20-29)IncludeExcludeConstraints(39-42)SearchCriteria(34-37)NormalizePathForComparison(509-539)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
🔇 Additional comments (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
167-207: FileSearcher-based filepath scoping inEvaluateReferencedlooks correct and robust.The new logic correctly:
- Pulls rule-scoped include/exclude paths via
cond.ProviderContext.GetScopedFilepaths().- Combines them with provider-level constraints (
sc.includedPaths,sc.BaseConfig.DependencyFolders) viaFileSearcher.- Normalizes both search results and
incident.FileURIwithNormalizePathForComparisonfor cross-platform, scheme-agnostic comparison.- Runs the search concurrently with
GetAllDeclarationsand, on error, falls back toskipFilteringto avoid false negatives while still logging the failure.This behavior aligns with the PR’s objectives for chained-condition filepath scoping and error handling.
Also applies to: 209-235, 239-244, 252-260
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
Show resolved
Hide resolved
The replace directive is useful for local testing but shouldn't be committed to the PR. The CI/CD system builds images and tests with the proper module references. Addresses review comment from shawn-hurley. Signed-off-by: tsanders <[email protected]>
The nodejs provider config had dependencyFolders set to [""] (array with empty string). When FileSearcher's filterFilesByPathsOrPatterns processes an empty pattern with filterOut=true, it: 1. Joins basePath with "" resulting in basePath 2. All files have basePath as prefix 3. All files match the pattern and are excluded Fix: Filter out empty strings from DependencyFolders before passing to FileSearcher. This resolves the issue where nodejs rules (node-sample-rule-001, node-sample-rule-002, node-sample-rule-003, test-tsx-support-00010) were showing as unmatched in demo-output.yaml. Signed-off-by: tsanders <[email protected]>
… empty string Changed dependencyFolders from [""] to [] to prevent FileSearcher from excluding all files. This complements the defensive fix in service_client.go that filters out empty strings. This ensures the bug won't recur even if the config is changed back. Signed-off-by: tsanders <[email protected]>
Support condition-level filepath constraints by adding Filepaths field to referencedCondition struct and passing it to FileSearcher.Search. This matches the Java provider's functionality and allows rules to specify filepaths directly in the condition to constrain the search scope. Addresses: konveyor#1004 (comment) Signed-off-by: tsanders <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
51-56:includedPathsmay be computed against a mutatedLocation(file://URI) instead of a filesystem path
provider.GetIncludedPathsFromConfig(seeprovider/lib.go) expectsInitConfig.Locationto be a filesystem path so it can resolve andos.Statrelative include entries. Here,c.Locationis rewritten to afile://URI (Lines 51-55) beforeGetIncludedPathsFromConfigis called (Lines 136-137), so relative include paths from config are likely being dropped silently andsc.includedPathsmay end up incomplete or empty.To preserve correct behavior, capture
includedPathsbefore mutatingc.Locationto a URI, and avoid recomputing it later:func (n *NodeServiceClientBuilder) Init(ctx context.Context, log logr.Logger, c provider.InitConfig) (provider.ServiceClient, error) { - sc := &NodeServiceClient{} + sc := &NodeServiceClient{} + + // Capture included paths from init config (provider-level constraints) + // before mutating c.Location into a file:// URI for LSP. + sc.includedPaths = provider.GetIncludedPathsFromConfig(c, false) @@ - // Get included paths from init config (provider-level constraints) - sc.includedPaths = provider.GetIncludedPathsFromConfig(c, false)Please double-check the current implementation of
GetIncludedPathsFromConfigto confirm it still relies onInitConfig.Locationbeing a real filesystem path; if it has been updated to handlefile://URIs, documenting that would avoid future regressions.Also applies to: 136-137
🧹 Nitpick comments (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
182-216: FileSearcher-based filtering is sound; consider including additional workspace folders asAdditionalPathsThe FileSearcher integration and post-filtering logic look correct:
- Base path derived from the first workspace folder (with
file://stripped) is a reasonable default.- Dependency folders are filtered to remove empty strings before use as excludes.
- The goroutine + buffered channel pattern safely builds a normalized
fileMapwithout risking deadlock.- On search error,
skipFilteringavoids false negatives and falls back to pre-existing behavior.- Incidents are filtered by comparing normalized
incident.FileURIagainst the normalizedfileMap, leveragingNormalizePathForComparisonfor cross-platform consistency.One potential improvement: if
sc.BaseConfig.WorkspaceFolderscan contain multiple folders, you might want to pass the remaining ones asFileSearcher.AdditionalPaths, trimming thefile://prefix similarly. Per previous learnings, additional workspace folders used withFileSearcherare already vetted/scoped and do not require extra excludes, so this would extend correct filtering to all workspaces, not just the first.Example sketch:
- basePath := "" - if len(sc.BaseConfig.WorkspaceFolders) > 0 { - basePath = sc.BaseConfig.WorkspaceFolders[0] - // Remove file:// prefix if present - basePath = strings.TrimPrefix(basePath, "file://") - } + basePath := "" + var additionalPaths []string + if len(sc.BaseConfig.WorkspaceFolders) > 0 { + basePath = strings.TrimPrefix(sc.BaseConfig.WorkspaceFolders[0], "file://") + for _, wf := range sc.BaseConfig.WorkspaceFolders[1:] { + additionalPaths = append(additionalPaths, strings.TrimPrefix(wf, "file://")) + } + } @@ - fileSearcher := provider.FileSearcher{ - BasePath: basePath, + fileSearcher := provider.FileSearcher{ + BasePath: basePath, + AdditionalPaths: additionalPaths,This is non-blocking, but would make the filtering more robust in multi-folder workspaces. Based on learnings, additionalPaths are already scoped appropriately.
Also applies to: 218-241, 245-266
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-21T14:12:52.228Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
Applied to files:
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
📚 Learning: 2025-10-21T16:27:59.216Z
Learnt from: shawn-hurley
Repo: konveyor/analyzer-lsp PR: 860
File: provider/server.go:251-259
Timestamp: 2025-10-21T16:27:59.216Z
Learning: In provider/server.go, the Init method uses context.Background() for creating the RPC client via GetProviderRPCClient because this RPC client is stored in the InitConfig and used across multiple subsequent requests (Evaluate, GetDependencies, etc.). The RPC client's lifecycle is managed separately through explicit Stop calls, not tied to the Init request's context.
Applied to files:
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
🧬 Code graph analysis (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
provider/lib.go (5)
GetIncludedPathsFromConfig(415-438)FileSearcher(20-29)IncludeExcludeConstraints(39-42)SearchCriteria(34-37)NormalizePathForComparison(509-539)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (2)
32-33: Storing config and provider-level include paths on the client looks goodKeeping
NodeServiceClientConfigandincludedPathsonNodeServiceClientis a clean way to reuse initialization data and provider-level constraints in later evaluations; no issues here.
164-165: Condition-levelFilepathswiring into FileSearcher is consistent and matches the intended scoping modelAdding
Filepaths []string \yaml:"filepaths,omitempty"`toreferencedConditionand passing it asSearchCriteria.ConditionFilepaths(while rule-scoped paths still come fromProviderContext.GetScopedFilepaths()`) cleanly separates:
- Provider config constraints (
sc.includedPaths, dependency folders),- Rule scope constraints (
includedFilepaths,excludedFilepaths), and- Condition-level filepaths (
cond.Filepaths).This matches the chained-condition design and keeps the API surface straightforward. No changes needed here.
Also applies to: 186-187, 227-229
…epath filtering The Java provider was only filtering by condition-level filepaths using simple string suffix matching. This commit integrates FileSearcher to handle: 1. Rule scope excluded paths - Previously ignored since the language server only knows about included paths 2. Condition filepaths with proper pattern matching and normalization 3. Path normalization for cross-platform compatibility Architecture: - The Java language server pre-filters results using provider-level and rule-scope included paths - FileSearcher acts as a secondary filter to apply excluded paths and condition filepaths that the language server doesn't know about - Only applies additional filtering when there are excluded paths or condition filepaths to avoid unnecessary overhead This brings the Java provider's filepath filtering in line with the generic providers (nodejs, python, go) while respecting its unique architecture of pre-filtering at the language server level. Signed-off-by: tsanders <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-02T02:57:09.802Z
Learnt from: tsanders-rh
Repo: konveyor/analyzer-lsp PR: 1004
File: external-providers/java-external-provider/go.mod:49-50
Timestamp: 2025-12-02T02:57:09.802Z
Learning: In the konveyor/analyzer-lsp monorepo, all external providers (java-external-provider, dotnet-external-provider, generic-external-provider) include a `replace github.com/konveyor/analyzer-lsp => ../../` (or `../..`) directive in their go.mod files. This replace directive is the established pattern for local development within the monorepo and allows providers to reference the local analyzer-lsp module. When providers are built outside the repository, the versioned dependency declared in the require block is used instead.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
📚 Learning: 2025-11-21T14:12:52.228Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
🧬 Code graph analysis (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
provider/lib.go (4)
FileSearcher(20-29)IncludeExcludeConstraints(39-42)SearchCriteria(34-37)NormalizePathForComparison(509-539)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
🔇 Additional comments (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
187-227: FileSearcher gating and error fallback preserve legacy behavior while enforcing scoped constraintsConditionally invoking
FileSearcheronly when there are excluded filepaths or condition-level filepaths, and falling back to unfilteredrefson search error, aligns well with the goal of (a) not changing behavior when no path constraints are present and (b) avoiding silent loss of symbols when filesystem search fails. This looks good.
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
Show resolved
Hide resolved
The GetIncludedPathsFromConfig call was happening after c.Location was mutated to a file:// URI. This function expects a filesystem path to properly resolve relative include paths using filepath.Join and validate them with os.Stat. When c.Location is a URI, these operations fail and includedPaths are silently dropped, causing provider-level filepath constraints to not be applied. Fix by moving GetIncludedPathsFromConfig call to execute before the c.Location mutation, preserving the filesystem path for correct relative path resolution. Signed-off-by: tsanders <[email protected]>
Updates go.sum to include checksum entries for the pseudo-version dependency on analyzer-lsp v0.9.0-alpha.1.0.20251121202139-b3f742caf596. This fixes the CI failure where go mod download was unable to verify the java-external-provider package. Signed-off-by: tsanders <[email protected]>
The replace directive is required for CI to pass because this PR includes both the NormalizePathForComparison function (added in commit 5bbe00a) and its usage in the java provider. Without the replace directive, go.mod references the older v0.9.0-alpha.1.0.20251121202139-b3f742caf596 version which doesn't have this function, causing build failures. This can be removed in a follow-up PR after merge. Signed-off-by: tsanders <[email protected]>
The nodejs provider includedPaths fix (commit e9dab9e) corrected the behavior to properly detect violations that were previously being silently ignored. This updates the golden file to reflect the correct behavior where nodejs rules (node-sample-rule-001, node-sample-rule-002, node-sample-rule-003, test-tsx-support-00010) are now properly detected as violations with incidents. Changes: - Moved previously unmatched nodejs rules to violations section - Added incident details for all detected nodejs violations Signed-off-by: tsanders <[email protected]>
The filepath filtering code at line 239 had an unsafe type assertion that would panic if ref.Location.Value was not exactly protocol.Location. The LSP protocol allows Location.Value to be multiple types: - protocol.Location - protocol.PLocationMsg_workspace_symbol Changed to use a type switch (matching pattern at lines 253-263) to safely handle both types and skip unknown types instead of panicking. This prevents potential runtime crashes when processing workspace symbols with different location type variants. Signed-off-by: tsanders <[email protected]>
shawn-hurley
left a comment
shawn-hurley
left a comment
There was a problem hiding this comment.
We may just need to remove the testing and replace it again, but then it looks good to me.
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
207-218: Consider adding provider-level excluded paths to FileSearcher.The
ProviderConfigConstraintsonly setsIncludePathsOrPatternsbut omitsExcludePathsOrPatterns. If the provider configuration specifies directories to exclude (viaGetExcludedDirsFromConfig), FileSearcher won't apply them, potentially returning files that should be excluded at the provider level.fileSearcher := provider.FileSearcher{ BasePath: basePath, ProviderConfigConstraints: provider.IncludeExcludeConstraints{ IncludePathsOrPatterns: p.includedPaths, + ExcludePathsOrPatterns: provider.GetExcludedDirsFromConfig(p.config), }, RuleScopeConstraints: provider.IncludeExcludeConstraints{ IncludePathsOrPatterns: includedFilepaths, ExcludePathsOrPatterns: excludedFilepaths, }, FailFast: true, Log: log, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-02T02:57:09.802Z
Learnt from: tsanders-rh
Repo: konveyor/analyzer-lsp PR: 1004
File: external-providers/java-external-provider/go.mod:49-50
Timestamp: 2025-12-02T02:57:09.802Z
Learning: In the konveyor/analyzer-lsp monorepo, all external providers (java-external-provider, dotnet-external-provider, generic-external-provider) include a `replace github.com/konveyor/analyzer-lsp => ../../` (or `../..`) directive in their go.mod files. This replace directive is the established pattern for local development within the monorepo and allows providers to reference the local analyzer-lsp module. When providers are built outside the repository, the versioned dependency declared in the require block is used instead.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
📚 Learning: 2025-11-21T14:12:52.228Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
🧬 Code graph analysis (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
provider/lib.go (4)
FileSearcher(20-29)IncludeExcludeConstraints(39-42)SearchCriteria(34-37)NormalizePathForComparison(509-539)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (macos-latest, darwin, amd64)
🔇 Additional comments (4)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (4)
187-202: LGTM - Well-documented constraint checking logic.The early-return pattern correctly preserves language server results when no additional filtering is needed. The comments clearly explain the rationale for post-filtering (excluded paths and condition filepaths that the language server doesn't handle).
220-227: LGTM - Graceful error handling.The error handling correctly falls back to returning all language server results when the file search fails, avoiding silent data loss while logging the issue for debugging.
236-253: LGTM - Type-safe location handling with O(1) lookups.The type switch correctly handles both
protocol.Locationandprotocol.PLocationMsg_workspace_symboltypes (matching the pattern inGetAllReferences), and the map-based lookup provides efficient O(1) filtering as intended by the PR objectives.
255-255: LGTM!The return statement correctly provides the filtered subset of workspace symbols.
The hasAdditionalConstraints check was missing len(includedFilepaths) > 0, which meant that when only includedFilepaths were specified (without excluded paths or condition-level filepaths), the file-level filtering would be skipped entirely and all language server results would be returned. Context: The Java language server filters at the package level via bundles/ argumentsMap. We need to apply additional file-level filtering on the results to handle: 1. Rule scope excluded paths (language server only knows included paths) 2. Condition filepaths (with proper pattern matching and normalization) 3. Provider-level included paths (for file-level precision) This fix ensures file-level filtering is applied whenever we have any filepath constraints, not just when we have excluded paths. Tests: All filepath filtering tests pass, including the "included_paths_filtering" test case which validates this exact scenario. Signed-off-by: tsanders <[email protected]>
shawn-hurley
left a comment
shawn-hurley
left a comment
There was a problem hiding this comment.
Once the replacements are removed, this should be good to go
If you want to do the other comment now or create a follow-up, that is fine, but if you make an issue, please mark it as critical urgent, as I am pretty sure it will have a performance impact on the Java provider.
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
Outdated
Show resolved
Hide resolved
| google.golang.org/protobuf v1.36.6 // indirect | ||
| ) | ||
|
|
||
| replace github.com/konveyor/analyzer-lsp => ../../ |
| google.golang.org/protobuf v1.36.6 // indirect | ||
| ) | ||
|
|
||
| replace github.com/konveyor/analyzer-lsp => ../.. |
Previously, file search and LSP query ran sequentially: 1. Query language server for symbols 2. Search for allowed files 3. Filter results Now they run in parallel (matching nodejs provider pattern): 1. Start file search in goroutine 2. Query language server (runs in parallel with search) 3. Wait for search to complete 4. Filter results This reduces total latency when both operations are needed, as the file search and LSP query now overlap instead of running back-to-back. Signed-off-by: tsanders <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-02T02:57:09.802Z
Learnt from: tsanders-rh
Repo: konveyor/analyzer-lsp PR: 1004
File: external-providers/java-external-provider/go.mod:49-50
Timestamp: 2025-12-02T02:57:09.802Z
Learning: In the konveyor/analyzer-lsp monorepo, all external providers (java-external-provider, dotnet-external-provider, generic-external-provider) include a `replace github.com/konveyor/analyzer-lsp => ../../` (or `../..`) directive in their go.mod files. This replace directive is the established pattern for local development within the monorepo and allows providers to reference the local analyzer-lsp module. When providers are built outside the repository, the versioned dependency declared in the require block is used instead.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
📚 Learning: 2025-11-21T14:12:52.228Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
🧬 Code graph analysis (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2)
provider/lib.go (4)
FileSearcher(20-29)IncludeExcludeConstraints(39-42)SearchCriteria(34-37)NormalizePathForComparison(509-539)provider/provider.go (1)
IncludedPathsConfigKey(38-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (windows-latest, windows)
🔇 Additional comments (2)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2)
243-275: Well-structured filtering with proper error handling.The implementation correctly:
- Returns early when no filtering is needed, avoiding channel operations
- Falls back to unfiltered results on search failure to prevent false negatives
- Uses a type switch to safely handle different location types, preventing panics
- Performs O(1) map lookups for filtering
172-183: Verify whether provider-level excludes should be applied inProviderConfigConstraints.The
ProviderConfigConstraintscurrently sets onlyIncludePathsOrPatternswhileRuleScopeConstraintshas both include and exclude patterns. Determine if this separation is intentional (provider includes at config level, rule-level filtering for both include/exclude) or if provider-level excludes (viaGetExcludedDirsFromConfigor similar) should be added toProviderConfigConstraintsfor consistency with other providers in the codebase.
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
Outdated
Show resolved
Hide resolved
The log at line 209 was incorrectly logging p.includedPaths but should log includedFilepaths since that's the variable being set in argumentsMap. This could cause confusion during debugging as the log would show the wrong value (empty p.includedPaths instead of the actual includedFilepaths being used). Signed-off-by: tsanders <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
256-275: URI extraction and normalization-based filtering are robust; consider optional debug logging for unknown location typesThe new type switch on
ref.Location.Valueand use ofprovider.NormalizePathForComparison(Lines 256–275) correctly:
- Handle both
protocol.Locationandprotocol.PLocationMsg_workspace_symbolwithout panicking.- Skip unknown location representations conservatively.
- Perform O(1) membership checks via the normalized-path map.
That aligns with the cross-provider normalization design and fixes the earlier panic risk.
As a minor observability improvement, you might log unknown location types at a very verbose level to aid future debugging, e.g.:
- default: - // Skip symbols with unknown location types - continue + default: + // Skip symbols with unknown location types + log.V(9).Info("skipping symbol with unknown location type", "type", fmt.Sprintf("%T", ref.Location.Value)) + continueThis is optional and purely for diagnostics; the current behavior is logically sound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-02T02:57:09.802Z
Learnt from: tsanders-rh
Repo: konveyor/analyzer-lsp PR: 1004
File: external-providers/java-external-provider/go.mod:49-50
Timestamp: 2025-12-02T02:57:09.802Z
Learning: In the konveyor/analyzer-lsp monorepo, all external providers (java-external-provider, dotnet-external-provider, generic-external-provider) include a `replace github.com/konveyor/analyzer-lsp => ../../` (or `../..`) directive in their go.mod files. This replace directive is the established pattern for local development within the monorepo and allows providers to reference the local analyzer-lsp module. When providers are built outside the repository, the versioned dependency declared in the require block is used instead.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
📚 Learning: 2025-11-21T14:12:52.228Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
📚 Learning: 2025-08-21T12:39:46.778Z
Learnt from: jmle
Repo: konveyor/analyzer-lsp PR: 888
File: external-providers/java-external-provider/pkg/java_external_provider/provider.go:662-663
Timestamp: 2025-08-21T12:39:46.778Z
Learning: In the konveyor/analyzer-lsp Java provider, when setting JAVA_HOME for Gradle commands using exec.CommandContext, the maintainers prefer not to preserve the existing environment variables (including PATH) and use `cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", javaHome))` instead of `cmd.Env = append(os.Environ(), fmt.Sprintf("JAVA_HOME=%s", javaHome))`.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (ubuntu-latest, linux)
🔇 Additional comments (1)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
148-158: Path-scoping detection and fallback behavior look correctThe combination of:
- Extracting
includedFilepaths, excludedFilepaths := condCTX.GetScopedFilepaths()and computinghasAdditionalConstraintsfrom rule-scope includes/excludes andc.Referenced.Filepaths(Lines 148–158),- Passing either
p.includedPathsorincludedFilepathsintoargumentsMap[provider.IncludedPathsConfigKey](Lines 204–210), and- Returning
refsunchanged when!hasAdditionalConstraintsor whenfileSearcher.Searchfails (Lines 243–255),gives the expected semantics:
- No scoped paths → preserve legacy “full workspace” behavior and keep JAR/metadata results.
- Scoped includes/excludes or condition-level paths → enforce file-level filtering.
- Filesystem/search failures → degrade gracefully by returning unfiltered language-server results instead of silently dropping everything.
This matches the PR’s objectives around honoring filepath scope only when present while avoiding false negatives on search failure.
Also applies to: 204-210, 243-255
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
Show resolved
Hide resolved
The parallelization commit (069703e) introduced a subtle issue where calling GetScopedFilepaths() once at the beginning and reusing the value was causing test failures for the singleton-sessionbean-00002 rule. The root cause was that by calling GetScopedFilepaths() early (before the LSP query) and using that value to both: 1. Set argumentsMap for the LSP query 2. Determine hasAdditionalConstraints for file filtering We were inadvertently changing the behavior compared to the old version which called GetScopedFilepaths() twice - once inline in the else-if (to set argumentsMap) and once after the LSP query (to determine filtering). While functionally these should be equivalent (since GetScopedFilepaths() just reads from the template context), the timing difference appears to have affected the test results. Reverting to the synchronous approach (file search after LSP query) to ensure correctness. We can revisit parallelization in a follow-up after understanding the exact root cause. Fixes issue where singleton-sessionbean-00002 rule was missing first incident. Signed-off-by: tsanders <[email protected]>
The previous parallelization attempt failed because it didn't correctly handle the case where both provider-level paths (p.includedPaths) and rule-scope paths (from GetScopedFilepaths) are present. The issue was: - LSP query was being filtered by p.includedPaths (provider-level) - But file filtering was using includedFilepaths (rule-scope) - This mismatch caused incorrect filtering when both were present The fix: 1. Determine which paths to use for LSP query (provider-level takes precedence) 2. Correctly determine hasAdditionalConstraints: - Need filtering for excluded paths (LSP only handles includes) - Need filtering for condition filepaths (pattern matching) - Need filtering when provider-level paths used but rule-scope also exists 3. Start file search in parallel with LSP query 4. Filter results using the parallel search result This ensures the LSP query and file filtering use consistent filepath constraints, while still achieving the parallelization performance benefit. Signed-off-by: tsanders <[email protected]>
… that Signed-off-by: Shawn Hurley <[email protected]>
…ditions (#1006) ## Summary Fixes #1005 The builtin provider did not properly filter files when using chained conditions. This PR adds support for filepath filtering in `builtin.fileContent`, `builtin.json`, and `builtin.xmlPublicID` capabilities. ## Changes 1. **Added `Filepaths` field to `fileContentCondition`** - Allows `builtin.fileContent` to accept filepath scopes from chained conditions 2. **Updated filecontent case** - Now passes `ConditionFilepaths` to `FileSearcher.Search()` 3. **Fixed JSON bug** - Changed from `cond.XML.Filepaths` to `cond.JSON.Filepaths` 4. **Fixed XMLPublicID bug** - Changed from `cond.XML.Filepaths` to `cond.XMLPublicID.Filepaths` 5. **Added `omitempty` tag** - Fixed inconsistency in `xmlPublicIDCondition.Filepaths` for consistent JSON serialization 6. **Added comprehensive tests** - 3 new test cases covering different filepath filtering scenarios ## Example Usage After this fix, chained conditions like the following now work correctly: ```yaml when: and: - as: tsFiles nodejs.referenced: pattern: Button ignore: true - builtin.fileContent: filepaths: "{{tsFiles.filepaths}}" pattern: isSelectable from: tsFiles ``` The `builtin.fileContent` condition now correctly searches only the files identified by the `tsFiles` variable instead of searching the entire codebase. ## Test Results ✅ All 30 existing tests pass ✅ All 3 new filepath filtering tests pass ✅ Total test runtime: ~31s ## Related Work This PR complements #1004, which implemented filepath filtering for gRPC providers (Java, .NET, Node.js). The builtin provider now has consistent behavior with those providers. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Filepath-based filtering added to scope file-content and XML searches more precisely. * Chain templates now expose filepath/extras/excludedPaths to templates (all fields available when rendering). * **Improvements** * Filepath parameters are optional in JSON payloads and supported in JSON/YAML templating. * Template rendering context updated so chained conditions intersect filepath scopes predictably. * **Tests** * Expanded tests for filepath filtering, include/exclude rules, scoped scenarios. * **Documentation** * Added guidance on filepath filtering behavior in chained conditions. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: tsanders <[email protected]>
2 participants
Summary
Fixes #1002 - Implements filepath filtering in gRPC providers (
nodejs.referenced,java.referenced,dotnet.referenced) to respect filepath scope from chained conditions using thefrom:syntax.Problem
When using chained conditions with
from:, the second condition should only analyze files passed from the first condition. Previously, gRPC providers would search the entire codebase instead of respecting the scoped filepaths.Example of expected behavior:
Before this fix,
nodejs.referencedwould search ALL files instead of only the files fromimportFiles.Filepaths.Solution
Implementation
ProviderContext.GetScopedFilepaths()Providers Modified
EvaluateReferenced()after symbol queryGetAllSymbols()Evaluate()for both LSP and fallback pathsCross-Platform Support
file://,file:, plain paths)csharp:URIs for .NET metadata referencesPerformance
Achieved 46-63x performance improvement with map-based O(1) lookups:
Testing
Unit Tests
Integration Test
Validated with chained Node.js rule:
Code Quality
Code Review Improvements
After comprehensive code review, applied critical fixes:
Quality Metrics
Files Changed
Checklist
Test Plan
To verify this PR:
go test ./external-providers/.../pkg/...go test -bench=BenchmarkFilepathFilteringfrom:syntaxSummary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.