🐛 adding updates to get lang ext working #960
Conversation
Co-authored-by: Shawn Hurley <[email protected]> Signed-off-by: Savitha Raghunathan <[email protected]> Co-authored-by: Shawn Hurley <[email protected]>
WalkthroughAdds an exported ServerCapabilities.AssumeWorks flag and an early-return in Supports; sets AssumeWorks=true for service clients created with a provided RPC connection; marks providers initialized when a language-server pipe is present; adds capability-evaluation logging. Changes
Sequence Diagram(s)sequenceDiagram
participant ProviderInit as Provider Init
participant ClientCtor as NewLSPServiceClientBase
participant SC as ServerCapabilities
participant CapCheck as Supports(method)
rect rgb(220,240,220)
note over ProviderInit,ClientCtor: Using provided RPC connection / LanguageServerPipe present
ProviderInit->>ClientCtor: NewLSPServiceClientBase(rpc)
ClientCtor->>SC: sc.ServerCapabilities.AssumeWorks = true
ProviderInit->>ProviderInit: Initialized = (LanguageServerPipe != "")
end
rect rgb(240,240,220)
note over CapCheck: Capability evaluation
CapCheck->>SC: Supports("workspace/symbol" / ...)
alt SC.AssumeWorks == true
SC-->>CapCheck: return true (early exit)
else
SC->>SC: perform switch-based capability checks
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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: 2
🧹 Nitpick comments (4)
provider/grpc/provider.go (1)
195-197: PipeName ⇒ Initialized: confirm end-to-end contractSetting Initialized=true when PipeName is provided assumes the downstream will supply an already-usable RPC and that we can safely skip the LSP initialize/initialized handshake. This matches server.go wiring but tightly couples behavior to AssumeWorks. Please verify all providers using PipeName actually deliver a ready RPC and won’t require initialize. Also consider redacting proxy fields from the nearby config log to avoid leaking credentials.
lsp/base_service_client/base_capabilities.go (1)
43-43: Reduce verbosity and avoid logging raw payloadsLogging raw info bytes at Info can be noisy and leak data. Log at a higher verbosity and only the size/preview.
Apply this diff:
- sc.Log.Info("Evaluate Call", "cap", cap, "info", info) + sc.Log.V(5).Info("Evaluate Call", "cap", cap, "info_len", len(info))lsp/base_service_client/base_service_client.go (2)
216-218: AssumeWorks on provided RPC: pair with scoped Supports behaviorSetting AssumeWorks for provided RPC is reasonable given the tech debt (RPC treated as initialized), but only safe if Supports does not globally bypass checks. Please adopt the scoped allowlist in protocol/extensions.go so we don’t call unsafe methods. Based on learnings.
359-359: Lower log level for server capability checkThis log will fire per query; make it verbose-only to avoid log noise.
- sc.Log.Info("server caps", "supports", sc.ServerCapabilities.Supports("workspace/symbol")) + sc.Log.V(5).Info("server caps", "supports", sc.ServerCapabilities.Supports("workspace/symbol"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lsp/base_service_client/base_capabilities.go(1 hunks)lsp/base_service_client/base_service_client.go(2 hunks)lsp/protocol/extensions.go(1 hunks)lsp/protocol/tsprotocol.go(1 hunks)provider/grpc/provider.go(1 hunks)provider/server.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T16:25:40.903Z
Learnt from: shawn-hurley
Repo: konveyor/analyzer-lsp PR: 860
File: lsp/base_service_client/base_service_client.go:212-214
Timestamp: 2025-10-21T16:25:40.903Z
Learning: In `lsp/base_service_client/base_service_client.go`, when an RPC connection is provided via `c.RPC` (not nil), it is treated as already initialized and no handler setup is performed. This is known technical debt that needs to be addressed in the future to ensure LSP notifications like `textDocument/publishDiagnostics` are properly processed.
Applied to files:
lsp/base_service_client/base_service_client.goprovider/server.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:
provider/server.go
🧬 Code graph analysis (1)
lsp/base_service_client/base_service_client.go (1)
lsp/protocol/tsprotocol.go (1)
ServerCapabilities(3976-4085)
⏰ 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 (macos-latest, mac)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
🔇 Additional comments (1)
provider/server.go (1)
250-251: Initialized derived from LanguageServerPipe: OK, but validate assumptionsDeriving Initialized from LanguageServerPipe is consistent with the gRPC client and the base client path that skips initialize when RPC is provided. Please validate this doesn’t regress servers that expect initialize before any request on a new connection. Based on learnings.
| if c.AssumeWorks { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Do not globally bypass capability checks; narrow the scope
Global early-return on AssumeWorks defeats the guard that prevents sending unsupported methods to servers that may crash (as noted in the file header). This is a high‑risk regression.
Constrain the bypass to an explicit, minimal allowlist (e.g., only the method you need to unblock), and fall back to existing switch logic otherwise. Example:
- if c.AssumeWorks {
- return true
- }
+ if c.AssumeWorks {
+ switch method {
+ case "workspace/symbol":
+ return true
+ }
+ }If more methods are required, make the list configurable per provider rather than blanket enabling.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lsp/protocol/extensions.go around lines 8-10, the code currently returns true
unconditionally when c.AssumeWorks is set which bypasses all capability checks;
change this to only bypass for a minimal allowlist of explicit method names (or
provider-configurable list) by checking if the requested method is in that
allowlist and return true only in that case, otherwise fall through to the
existing switch/capability logic so unsupported methods are still guarded
against.
| // Defines the capabilities provided by a language | ||
| // server. | ||
| type ServerCapabilities struct { | ||
| AssumeWorks bool |
There was a problem hiding this comment.
Avoid modifying generated types; prevent accidental JSON serialization
Adding AssumeWorks to a generated struct (“DO NOT EDIT”) is fragile and will be lost on regen. It also serializes as “AssumeWorks”, diverging from the LSP schema.
Minimal mitigation (still in generated file) to prevent wire breakage:
- AssumeWorks bool
+ AssumeWorks bool `json:"-"` // konveyor extension; not part of LSP, do not serializePrefer keeping extensions outside generated types (wrapper or helper predicate) to survive regeneration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AssumeWorks bool | |
| AssumeWorks bool `json:"-"` // konveyor extension; not part of LSP, do not serialize |
…angauge server Signed-off-by: Shawn Hurley <[email protected]>
2 participants
Signed-off-by: Savitha Raghunathan [email protected]
Co-authored-by: Shawn Hurley [email protected]
Summary by CodeRabbit