Add optional LSP enrichment for observe get-card and hover capability#84
Merged
Add optional LSP enrichment for observe get-card and hover capability#84
Conversation
… get-card This adds support for LSP hover enrichment of symbol cards returned by `observe get-card` at detail level semantic or higher. When the semantic backend is available and the LSP language server supports hover, the handler calls `textDocument/hover` to populate the card's `lsp` field with hover documentation, type information, and deprecation status. The feature is gated by capability negotiation and degrades gracefully if LSP is unavailable (not registered, capability denied, or request failure). In that case, the card retains the Tree-sitter-only extraction with provenance `tree_sitter_degraded_semantic`. Key changes include: - Adding a Hover capability to weaver-lsp-host with protocol and trait support. - Extending the `LspHost` with a `hover` method. - Creating an enrichment module in weaverd to perform hover-based enrichment. - Updating `observe get-card` handler and router to attempt enrichment. - Adding unit tests and updating BDD tests to verify proper behaviour and degradation. - Updating documentation to reflect enrichment and degradation behaviours. Closes #7.1.3. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Reviewer's GuideEnable optional LSP hover-based enrichment for observe get-card when semantic detail is requested, by adding a Hover capability to the LSP host, introducing an enrichment module in weaverd that calls textDocument/hover via backends, updating get_card/router signatures and provenance handling, and extending tests and docs accordingly. Sequence diagram for optional LSP hover enrichment in observe_get_cardsequenceDiagram
actor User
participant CLI as observe_get-card_CLI
participant Daemon as weaverd_DomainRouter
participant Handler as observe_get_card_handle
participant Extractor as TreeSitter_extractor
participant Backends as FusionBackends_Semantic
participant Provider as SemanticBackendProvider
participant Host as LspHost
participant Ls as LanguageServer
User->>CLI: invoke observe get-card --detail semantic
CLI->>Daemon: CommandRequest(get-card, detail=Semantic)
Daemon->>Handler: handle(request, writer, backends)
Handler->>Extractor: extract_symbol_card(detail=Semantic)
Extractor-->>Handler: SymbolCard(tree_sitter_degraded_semantic)
alt detail >= Semantic
Handler->>Handler: apply_lsp_enrichment(card, backends)
Handler->>Backends: ensure_started(Semantic)
alt backend_start_ok
Backends->>Provider: with_lsp_host_mut(...)
Provider->>Host: initialize(language)
Host-->>Provider: init_result
Provider->>Host: hover(language, HoverParams)
alt hover_success
Host->>Ls: textDocument/hover
Ls-->>Host: Hover
Host-->>Provider: Some(Hover)
Provider-->>Handler: Ok(Some(Hover))
Handler->>Handler: parse_hover_response(Hover)
Handler->>Handler: set card.lsp and provenance lsp_hover
else hover_failure_or_none
Host-->>Provider: None_or_error
Provider-->>Handler: error_or_None
Handler->>Handler: keep degraded card
end
else backend_start_failed
Backends-->>Handler: Err
Handler->>Handler: keep degraded card
end
else detail < Semantic
Handler->>Handler: skip LSP enrichment
end
Handler-->>Daemon: DispatchResult(status, GetCardResponse_Success)
Daemon-->>CLI: response_envelope
CLI-->>User: symbol_card JSON (enriched or degraded)
Sequence diagram for provenance update after successful LSP enrichmentsequenceDiagram
participant Handler as observe_get_card_handle
participant Enrich as apply_lsp_enrichment
participant Card as SymbolCard
Handler->>Enrich: apply_lsp_enrichment(&mut card, backends)
Enrich->>Enrich: try_lsp_enrichment(card, backends)
Enrich-->>Handler: EnrichmentOutcome::Enriched
Handler->>Card: remove tree_sitter_degraded_semantic from provenance.sources
alt provenance_missing_lsp_hover
Handler->>Card: push lsp_hover into provenance.sources
else already_has_lsp_hover
Handler->>Card: keep existing lsp_hover entry
end
Handler-->>Handler: return GetCardResponse::Success with updated provenance
Class diagram for LSP hover capability and enrichment typesclassDiagram
class CapabilityKind {
<<enum>>
Definition
References
Diagnostics
CallHierarchy
Hover
}
class ServerCapabilitySet {
-bool definition
-bool references
-bool diagnostics
-bool call_hierarchy
-bool hover
+new(definition bool, references bool, diagnostics bool) ServerCapabilitySet
+with_call_hierarchy(self ServerCapabilitySet, supported bool) ServerCapabilitySet
+with_hover(self ServerCapabilitySet, supported bool) ServerCapabilitySet
+supports_definition(self ServerCapabilitySet) bool
+supports_references(self ServerCapabilitySet) bool
+supports_diagnostics(self ServerCapabilitySet) bool
+supports_call_hierarchy(self ServerCapabilitySet) bool
+supports_hover(self ServerCapabilitySet) bool
}
class HostOperation {
<<enum>>
Definition
References
Diagnostics
PrepareCallHierarchy
IncomingCalls
OutgoingCalls
Hover
}
class LanguageServer {
<<interface>>
+initialize() Result~ServerCapabilitySet,LanguageServerError~
+goto_definition(params GotoDefinitionParams) Result~Option~GotoDefinitionResponse~,LanguageServerError~
+references(params ReferenceParams) Result~Option~Vec~Location~~,LanguageServerError~
+diagnostics(params DidOpenTextDocumentParams) Result~Option~Vec~Diagnostic~~,LanguageServerError~
+prepare_call_hierarchy(params CallHierarchyPrepareParams) Result~Option~Vec~CallHierarchyItem~~,LanguageServerError~
+incoming_calls(params CallHierarchyIncomingCallsParams) Result~Option~Vec~CallHierarchyIncomingCall~~,LanguageServerError~
+outgoing_calls(params CallHierarchyOutgoingCallsParams) Result~Option~Vec~CallHierarchyOutgoingCall~~,LanguageServerError~
+hover(params HoverParams) Result~Option~Hover~,LanguageServerError~
}
class ProcessLanguageServer {
+initialize() Result~ServerCapabilitySet,LanguageServerError~
+goto_definition(params GotoDefinitionParams) Result~Option~GotoDefinitionResponse~,LanguageServerError~
+hover(params HoverParams) Result~Option~Hover~,LanguageServerError~
}
class DocStubServer {
+initialize() Result~ServerCapabilitySet,LanguageServerError~
+hover(params HoverParams) Result~Option~Hover~,LanguageServerError~
}
class LspHost {
+hover(&mut self, language Language, params HoverParams) Result~Option~Hover~,LspHostError~
}
class CardLanguage {
<<enum>>
Rust
Python
TypeScript
Other
}
class LspInfo {
+String hover
+String type_info
+bool deprecated
+String source
}
class Provenance {
+Vec~String~ sources
}
class SymbolRefRangePosition {
+u32 line
+u32 column
}
class SymbolRefRange {
+SymbolRefRangePosition start
+SymbolRefRangePosition end
}
class SymbolRef {
+String uri
+SymbolRefRange range
+CardLanguage language
}
class SymbolCard {
+SymbolRef symbol_ref
+LspInfo lsp
+Provenance provenance
}
class BackendKind {
<<enum>>
Semantic
}
class FusionBackends_SemanticBackendProvider {
+ensure_started(kind BackendKind) Result~(),BackendError~
+provider(self FusionBackends_SemanticBackendProvider) &SemanticBackendProvider
}
class SemanticBackendProvider {
+with_lsp_host_mut(self SemanticBackendProvider, f FnOnce) Result~Option~Hover~,LspHostError~
}
class EnrichmentOutcome {
<<enum>>
Enriched
Degraded
}
class EnrichModule {
+try_lsp_enrichment(card &mut SymbolCard, backends &mut FusionBackends_SemanticBackendProvider) EnrichmentOutcome
+parse_hover_response(hover &Hover) LspInfo
+extract_hover_text(contents &HoverContents) String
+extract_type_hint(hover_text &str) String
+to_lsp_language(card_lang CardLanguage) Option~Language~
}
LanguageServer <|.. ProcessLanguageServer
LanguageServer <|.. DocStubServer
CapabilityKind --> ServerCapabilitySet
ServerCapabilitySet ..> LspHost
HostOperation ..> LspHost
SymbolRef --> SymbolRefRange
SymbolRefRange --> SymbolRefRangePosition
SymbolCard --> SymbolRef
SymbolCard --> LspInfo
SymbolCard --> Provenance
CardLanguage --> SymbolRef
FusionBackends_SemanticBackendProvider --> SemanticBackendProvider
EnrichModule --> SymbolCard
EnrichModule --> FusionBackends_SemanticBackendProvider
EnrichModule --> LspInfo
EnrichModule --> EnrichmentOutcome
EnrichModule --> CardLanguage
LspHost --> LanguageServer
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughAdd optional LSP hover enrichment for observe get-card: negotiate position encoding and hover capability, route textDocument/hover via LspHost, parse Hover into card.lsp (hover text, type info, deprecated flag, source), degrade gracefully when unavailable, add enrichment wiring, tests, stubs and documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User
participant Dispatcher as "Observe Dispatcher"
participant Handler as "get_card Handler"
participant Enricher as "Enrich Module"
participant Backends as "FusionBackends"
participant LSPHost as "LspHost"
participant LangServer as "Language Server"
User->>Dispatcher: observe get-card (detail: Semantic)
Dispatcher->>Handler: handle(request, writer, backends)
Handler->>Handler: Build SymbolCard (Tree-sitter)
alt detail >= Semantic
Handler->>Enricher: try_lsp_enrichment(card, source, backends)
Enricher->>Backends: ensure_started / select provider
Backends->>LSPHost: obtain host for language
Enricher->>LSPHost: initialise language server
Enricher->>LSPHost: hover(HoverParams)
LSPHost->>LangServer: textDocument/hover request
LangServer-->>LSPHost: Hover response
LSPHost-->>Enricher: Option<Hover>
alt Hover received
Enricher->>Enricher: parse_hover_response -> LspInfo
Enricher-->>Handler: Enriched
Handler->>Handler: set card.lsp and add provenance "lsp_hover"
else No hover or error
Enricher-->>Handler: Degraded
Handler->>Handler: set provenance "tree_sitter_degraded_semantic"
end
end
Handler->>Dispatcher: return DispatchResult (card)
Dispatcher-->>User: Response with card
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66e9a86e24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/weaverd/src/dispatch/observe/get_card.rs (1)
263-347: 🛠️ Refactor suggestion | 🟠 MajorAdd handler-level coverage for the semantic path.
These changed tests still only assert
StructureorMinimalrequests. The enrichment call at Line 55-Line 56 and the provenance rewrite at Line 81-Line 87 need one semantic success case and one semantic degraded case here, or the handler wiring can regress unnoticed.As per coding guidelines, "Write unit and behavioural tests for new functionality; run both before and after making any change."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/observe/get_card.rs` around lines 263 - 347, The test suite in get_card.rs lacks handler-level coverage for the semantic path: add two tests that call handle() with DetailLevel::Semantic — one where the semantic backend returns a successful enrichment (assert status success and that the response includes the enrichment/provenance fields) and one where the semantic backend degrades (assert a structured degraded response/refusal and that provenance rewrite logic still runs). Reuse the existing helpers (make_request, ResponseWriter, response_payload, FusionBackends<SemanticBackendProvider>) and mirror the pattern used in handle_returns_success_for_supported_rust_symbol and the refusal cases so the enrichment call and provenance rewrite (currently exercised at the handler level) are validated. Ensure one test asserts presence of semantic "enrichment" content and provenance fields on success and the other asserts a degraded/partial semantic indication on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaver-lsp-host/src/adapter/trait_impl.rs`:
- Line 66: The current check let hover_supported = caps.hover_provider.is_some()
incorrectly treats explicit false as supported; replace it by pattern-matching
caps.hover_provider to only treat Simple(true) or Options(_) as supported.
Update the assignment to set hover_supported by matching caps.hover_provider
against Some(HoverProviderCapability::Simple(true)) |
Some(HoverProviderCapability::Options(_)), ensuring textDocument/hover is only
routed when genuine support is advertised.
In `@crates/weaver-lsp-host/src/capability.rs`:
- Line 35: The capability mapping Self::Hover => "observe.get-card-hover" was
added but the key is missing from the "Current capability keys" section; update
the documentation by adding "observe.get-card-hover" to that section (alongside
the other capability keys) and include a short description matching the Hover
capability semantics so docs stay consistent with the enum mapping.
In `@crates/weaverd/src/dispatch/observe/enrich.rs`:
- Around line 170-269: The tests only exercise parse_hover_response and
to_lsp_language but omit exercising the orchestration logic in
try_lsp_enrichment; add unit/behavioral tests that invoke try_lsp_enrichment (or
the public entry that calls it) to cover backend startup, LSP initialization, a
successful hover response, and degraded/failed cases. Implement these tests in
the same tests mod using mocks/fakes or a spawned test LSP process to simulate:
(1) successful startup + hover returning markup/markedstring and assert
enrichment returns expected source/type_info/deprecated flags, and (2) failure
paths (LSP init failure, missing hover, or backend not available) and assert the
function returns the degraded outcome; reference try_lsp_enrichment and
parse_hover_response so the test hooks target the real orchestration and parsing
code.
In
`@docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md`:
- Around line 25-28: The API examples in the completed plan are using the wrong
field name and an inaccurate function signature; update the `get-card` example
and any other examples showing the LSP card so the LSP payload uses `type_info`
instead of `type`, and ensure provenance lists still include `"tree_sitter"` and
`"lsp_hover"`; also correct the function signature shown in the docs to match
the shipped implementation (remove or adjust the generic `BackendProvider`
parameter so the signature text matches the actual exported function) — search
for occurrences of `get-card`, `lsp`, `type` (replace with `type_info`), and the
signature that references `BackendProvider` and make them consistent with the
locked schema and released API.
---
Outside diff comments:
In `@crates/weaverd/src/dispatch/observe/get_card.rs`:
- Around line 263-347: The test suite in get_card.rs lacks handler-level
coverage for the semantic path: add two tests that call handle() with
DetailLevel::Semantic — one where the semantic backend returns a successful
enrichment (assert status success and that the response includes the
enrichment/provenance fields) and one where the semantic backend degrades
(assert a structured degraded response/refusal and that provenance rewrite logic
still runs). Reuse the existing helpers (make_request, ResponseWriter,
response_payload, FusionBackends<SemanticBackendProvider>) and mirror the
pattern used in handle_returns_success_for_supported_rust_symbol and the refusal
cases so the enrichment call and provenance rewrite (currently exercised at the
handler level) are validated. Ensure one test asserts presence of semantic
"enrichment" content and provenance fields on success and the other asserts a
degraded/partial semantic indication on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc0bdc52-8590-483c-969c-78d0b956480f
📒 Files selected for processing (17)
crates/weaver-lsp-host/docs/language_server_trait.mdcrates/weaver-lsp-host/src/adapter/trait_impl.rscrates/weaver-lsp-host/src/capability.rscrates/weaver-lsp-host/src/doc_support.rscrates/weaver-lsp-host/src/errors.rscrates/weaver-lsp-host/src/host.rscrates/weaver-lsp-host/src/server.rscrates/weaver-lsp-host/src/tests/behaviour.rscrates/weaver-lsp-host/src/tests/support/recording_server.rscrates/weaver-lsp-host/src/tests/unit.rscrates/weaverd/src/dispatch/observe/enrich.rscrates/weaverd/src/dispatch/observe/get_card.rscrates/weaverd/src/dispatch/observe/mod.rscrates/weaverd/src/dispatch/router.rsdocs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.mddocs/roadmap.mddocs/users-guide.md
- Implement optional enrichment of SymbolCards with LSP hover information in the observe get-card semantic detail level. - Populate SymbolCard.lsp using textDocument/hover when supported by the LSP server. - Introduce graceful degradation when LSP initialization or hover request fails. - Add thorough unit tests for LSP enrichment, including stub language server implementations. - Refactor observe::enrich tests to a dedicated file for clarity. - Update users-guide and execplan docs to describe new observe.get-card-hover capability controlling this behavior. - Modify weaver-lsp-host to correctly respect hover capability support according to LSP specification. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaverd/src/dispatch/observe/enrich.rs`:
- Around line 56-59: The match on uri_str.parse() currently returns
EnrichmentOutcome::Degraded silently; update the Err arm to log the failure
using the debug! macro (consistent with other degradation paths) by including a
message like "failed to parse URI", the offending uri_str and the parse error,
then return EnrichmentOutcome::Degraded; target the match around uri_str.parse()
that produces uri and ensure the debug! call precedes the return so failures are
visible for debugging.
- Around line 108-119: parse_hover_response currently lowercases hover_text with
to_ascii_lowercase() just to run contains("deprecated"), causing an unnecessary
allocation; change the deprecated detection to a case-insensitive substring
check that avoids allocating (e.g., use a compiled case-insensitive regex like
Regex::new("(?i)deprecated") or implement an ASCII case-insensitive search using
sliding-window comparison with eq_ignore_ascii_case) and replace the deprecated
= hover_text.to_ascii_lowercase().contains("deprecated") line accordingly in the
parse_hover_response function so LspInfo.hover remains unallocated.
- Around line 161-168: The match in to_lsp_language currently uses a wildcard
arm that returns None, which hides unhandled CardLanguage variants; replace the
`_ => None` with explicit arms for every CardLanguage variant (e.g., add
explicit branches for any remaining variants such as Java, Go, Unknown, etc.)
and return the appropriate Option<Language> for each or None explicitly per
variant so the compiler will force you to update this function when new
CardLanguage variants are added; reference the to_lsp_language function and the
CardLanguage and Language enums when making the change.
- Around line 140-150: The helper extract_type_hint currently returns an owned
String causing an unnecessary allocation; change its signature to fn
extract_type_hint(hover_text: &str) -> &str, return the trimmed &str slice found
by lines().find(...).map(|s| s.trim()).unwrap_or("") (remove .to_string()), and
update its caller in parse_hover_response to accept the &str result (remove any
.to_string()/.clone() uses or String assumptions) so the lifetime of the
returned slice is tied to the input hover_text; ensure any downstream code
expecting String is adjusted to borrow or call to_string() only when ownership
is required.
In `@crates/weaverd/src/dispatch/observe/get_card.rs`:
- Around line 76-89: The contains call in apply_lsp_enrichment creates a
temporary String for the membership check; replace
card.provenance.sources.contains(&String::from("lsp_hover")) with an
iterator-based check that compares &String to &str without allocation, e.g.
card.provenance.sources.iter().any(|s| s == "lsp_hover"), and keep the push of
"lsp_hover" unchanged.
In `@crates/weaverd/src/dispatch/observe/test_support.rs`:
- Around line 18-23: The StubLanguageServer struct declares a hover_error field
that is never set and makes the error branch in StubLanguageServer::hover()
unreachable; either remove the hover_error field or add a constructor that sets
it (e.g., failing_hover) to exercise the hover-error path. Locate
StubLanguageServer and implement a new constructor (e.g.,
StubLanguageServer::failing_hover or similar) that initializes capabilities,
hover, initialize_error and sets hover_error = Some(...) so tests can trigger
the error path in hover(), or delete hover_error and the hover error branch if
you don't intend to test that behavior.
- Around line 155-159: Add tempfile to crates/weaverd's dev-dependencies and
change test_config to produce a unique socket path per test: modify the function
test_config to create a tempfile::TempDir, build the socket path from
dir.path().join("socket.sock"), construct Config with daemon_socket:
SocketEndpoint::unix(socket_path) and return both (Config, TempDir) so callers
keep the TempDir for the test lifetime; update call sites to accept the
additional TempDir return value.
In
`@docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md`:
- Line 231: Update the comment text in the constructor that currently reads
"initialises `hover: false` for backwards compatibility." to use en-GB-oxendict
spelling "initializes" by replacing "initialises" with "initializes" in the
constructor comment or documentation string where `hover: false` is described;
target the constructor comment mentioning `hover` to make the change.
- Line 148: Replace the British-Oxford spelling error "serialisation" with
"serialization" in the document (specifically in the line containing
"serialisation. Date/Author: 2026-03-15, agent."); update any other occurrences
of "-isation" to "-ization" to comply with the repository's en-GB-oxendict
preference, and run a quick grep across the docs for "isation" to ensure
consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9537f9f-e61e-4454-a5b4-92c23e188d38
📒 Files selected for processing (10)
crates/weaver-lsp-host/src/adapter/trait_impl.rscrates/weaverd/src/dispatch/observe/enrich.rscrates/weaverd/src/dispatch/observe/enrich_tests.rscrates/weaverd/src/dispatch/observe/get_card.rscrates/weaverd/src/dispatch/observe/get_card_tests.rscrates/weaverd/src/dispatch/observe/mod.rscrates/weaverd/src/dispatch/observe/test_support.rscrates/weaverd/src/semantic_provider/mod.rsdocs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.mddocs/users-guide.md
This change implements optional hover enrichment for `weaver observe get-card --detail semantic` by querying the language server via the `textDocument/hover` LSP request. - Introduce `get_hover` helper to safely handle LSP server initialization and hover requests with graceful degradation. - Parse hover response to extract hover text, type hint, and deprecation status from structured markers. - Improve deprecation detection to avoid false positives from unstructured mentions. - Add unit tests for hover recording and deprecation logic. - Update documentation to describe new LSP enrichment behavior and degradation. If LSP is unavailable or errors occur, the enrichment degrades gracefully to tree-sitter-only data, recording explicit provenance for consumers. Refs: docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md (3)
150-150:⚠️ Potential issue | 🟡 MinorUse en-GB-oxendict spelling: "serialization" not "serialisation".
The repository mandates -ize/-ization spellings per en-GB-oxendict.
📝 Proposed fix
- separates the enrichment concern from request parsing and response - serialisation. Date/Author: 2026-03-15, agent. + separates the enrichment concern from request parsing and response + serialization. Date/Author: 2026-03-15, agent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md` at line 150, Replace the British spelling "serialisation" with the repository-mandated en-GB-oxendict "-ize" form "serialization" in the document string "serialisation. Date/Author: 2026-03-15, agent." (search for that exact phrase) so the file uses "serialization" consistently throughout; update any other occurrences of "serialisation" in this document to "serialization".
26-29: 🧹 Nitpick | 🔵 TrivialClarify the field naming in the API example.
Line 28 documents a
typeattribute, whilst the constraints at lines 44-46 note that the Rust field istype_infobut serializes to JSON key"type". Add a parenthetical clarification to avoid confusion for readers cross-referencing the Rust code.📝 Proposed clarification
server supports hover returns a card with a populated `lsp` field containing - `hover`, `type`, `deprecated`, and `source` attributes, and provenance + `hover`, `type` (Rust field: `type_info`), `deprecated`, and `source` attributes, and provenance sources including both `"tree_sitter"` and `"lsp_hover"`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md` around lines 26 - 29, Clarify the API example by adding a parenthetical note that the JSON key "type" corresponds to the Rust struct field type_info (i.e., Rust field is named type_info but serializes as "type"); update the sentence describing the populated lsp field in the get-card example (which lists hover, type, deprecated, and source) to include this parenthetical so readers cross-referencing the Rust code (e.g., the type_info field) understand the mapping.
233-233:⚠️ Potential issue | 🟡 MinorUse en-GB-oxendict spelling: "initializes" not "initialises".
📝 Proposed fix
-constructor initialises `hover: false` for backwards compatibility. +constructor initializes `hover: false` for backwards compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md` at line 233, Update the inline comment that reads "constructor initialises `hover: false` for backwards compatibility." to use the en-GB-oxendict spelling "initializes" instead of "initialises"; locate the comment near the class constructor that sets hover: false (search for the constructor and the `hover: false` initializer) and change the single word to "initializes".crates/weaverd/src/dispatch/observe/enrich.rs (2)
63-66: 🧹 Nitpick | 🔵 TrivialLog the URI parse failure for debugging.
Silent degradation here makes troubleshooting difficult. Add a
debug!log consistent with other degradation paths in this function.♻️ Proposed refactor
let uri = match uri_str.parse() { Ok(u) => u, - Err(_) => return EnrichmentOutcome::Degraded, + Err(e) => { + debug!( + target: DISPATCH_TARGET, + uri = %uri_str, + error = %e, + "LSP enrichment degraded: invalid URI" + ); + return EnrichmentOutcome::Degraded; + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/observe/enrich.rs` around lines 63 - 66, The URI parse failure currently returns EnrichmentOutcome::Degraded silently; update the match in enrich.rs that parses uri_str (the let uri = match uri_str.parse() { ... }) to log the error with a debug! call before returning EnrichmentOutcome::Degraded, following the same message style and context used by other degradation branches in this function so it’s easier to debug URI parsing failures.
181-188:⚠️ Potential issue | 🟠 MajorReplace wildcard with exhaustive match for
CardLanguagevariants.The
_ => Nonearm silently swallows newCardLanguagevariants. Exhaustive matching forces explicit handling when the enum is extended, preventing accidental omissions.Run the following script to identify all
CardLanguagevariants:#!/bin/bash # Description: List all CardLanguage variants to verify exhaustive coverage. rg -nP --type rust 'enum\s+CardLanguage' -A 30 | head -50🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/observe/enrich.rs` around lines 181 - 188, The match in to_lsp_language currently uses a wildcard arm that masks any new CardLanguage variants; replace the `_ => None` arm with an exhaustive match listing every CardLanguage variant so each is handled explicitly—map each CardLanguage variant to the appropriate Language::... where applicable and for variants that should not map, return None explicitly (e.g., CardLanguage::X => None). This forces the compiler to break when CardLanguage is extended and ensures you update to_lsp_language accordingly; update the match to enumerate all CardLanguage variants and their corresponding Language outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md`:
- Line 111: The article uses the incorrect indefinite article "an" before
"rstest" — find the exact phrase "an rstest fixture" in the document and change
it to "a rstest fixture" (i.e., replace "an rstest fixture" with "a rstest
fixture") to correct the pronunciation-based article usage.
- Line 352: The documentation uses en-GB spelling "serialisation" in the
sentence "After successful Tree-sitter extraction but before serialisation,
when" — update that word to the en-GB-oxendict preferred American spelling
"serialization" throughout this document (search for "serialisation"), ensuring
any occurrences in headings, body text, and captions (e.g., the sentence
referencing Tree-sitter extraction) are replaced with "serialization".
- Line 473: Replace every occurrence of the British-spelled word "serialised"
with the requested "serialized" in this document (including headings, inline
text and code blocks) — locate the exact token "serialised" near the phrase
"re-run the stage's validation." and perform a global find-and-replace for that
token and its derivatives to ensure consistency with en-GB-oxendict spelling.
---
Duplicate comments:
In `@crates/weaverd/src/dispatch/observe/enrich.rs`:
- Around line 63-66: The URI parse failure currently returns
EnrichmentOutcome::Degraded silently; update the match in enrich.rs that parses
uri_str (the let uri = match uri_str.parse() { ... }) to log the error with a
debug! call before returning EnrichmentOutcome::Degraded, following the same
message style and context used by other degradation branches in this function so
it’s easier to debug URI parsing failures.
- Around line 181-188: The match in to_lsp_language currently uses a wildcard
arm that masks any new CardLanguage variants; replace the `_ => None` arm with
an exhaustive match listing every CardLanguage variant so each is handled
explicitly—map each CardLanguage variant to the appropriate Language::... where
applicable and for variants that should not map, return None explicitly (e.g.,
CardLanguage::X => None). This forces the compiler to break when CardLanguage is
extended and ensures you update to_lsp_language accordingly; update the match to
enumerate all CardLanguage variants and their corresponding Language outcomes.
In
`@docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md`:
- Line 150: Replace the British spelling "serialisation" with the
repository-mandated en-GB-oxendict "-ize" form "serialization" in the document
string "serialisation. Date/Author: 2026-03-15, agent." (search for that exact
phrase) so the file uses "serialization" consistently throughout; update any
other occurrences of "serialisation" in this document to "serialization".
- Around line 26-29: Clarify the API example by adding a parenthetical note that
the JSON key "type" corresponds to the Rust struct field type_info (i.e., Rust
field is named type_info but serializes as "type"); update the sentence
describing the populated lsp field in the get-card example (which lists hover,
type, deprecated, and source) to include this parenthetical so readers
cross-referencing the Rust code (e.g., the type_info field) understand the
mapping.
- Line 233: Update the inline comment that reads "constructor initialises
`hover: false` for backwards compatibility." to use the en-GB-oxendict spelling
"initializes" instead of "initialises"; locate the comment near the class
constructor that sets hover: false (search for the constructor and the `hover:
false` initializer) and change the single word to "initializes".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61ddb173-497f-4071-ae08-fad85bf078d9
📒 Files selected for processing (4)
crates/weaver-lsp-host/src/tests/support/recording_server.rscrates/weaverd/src/dispatch/observe/enrich.rscrates/weaverd/src/dispatch/observe/enrich_tests.rsdocs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md
Owner
Author
|
@coderabbitai have the following now been resolved? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 24640
|
… enrichment Improve diagnostics in try_lsp_enrichment by logging the URI parse error with debug level, aiding in debugging degraded hover enrichment outcomes without changing behavior. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaverd/src/dispatch/observe/enrich.rs`:
- Around line 79-82: The Position.character is being set directly from
start.column (a Tree-sitter byte offset) but LSP expects UTF-16 code unit
offsets; convert start.column to UTF-16 code units before constructing Position.
Locate the Position construction in enrich.rs and call the existing conversion
helper implemented in crates/weaver-plugin-rust-analyzer/src/lsp/text_edits.rs
(the UTF-16 conversion utility there) to translate the byte column to a UTF-16
code unit index, then assign that value to Position.character instead of
start.column so hover requests use correct offsets.
In
`@docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md`:
- Around line 333-334: The doc text currently says deprecation is detected by
"searching for `deprecated` in the hover text (case-insensitive)"; update this
single sentence to accurately reflect the shipped logic by describing that we
detect a structured deprecation marker in the hover payload (e.g., a dedicated
"deprecated" field or marker token) rather than a generic case-insensitive
substring search of hover text—replace the phrase mentioning a substring search
and the backticked `deprecated` with wording that says "detects a structured
deprecation marker in the hover payload (e.g., a dedicated 'deprecated' field or
marker token)".
- Around line 197-213: Add a short, descriptive caption above the table at the
position indicated (above the list of types and files) explaining what the table
shows (for example: "Mapping of key types and symbols to their defining files"),
so the table is self-describing and guideline-compliant; place the caption
immediately before the table header in
docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md
and ensure it clearly refers to the listed items such as SymbolCard, LspInfo,
DetailLevel, Provenance, provenance_sources(), CapabilityKind,
ServerCapabilitySet, LanguageServer trait, LspHost, HostOperation,
get_card::handle(), route_observe(), SemanticBackendProvider and the BDD
test/feature entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0ad00fe-00fa-412f-8e12-0e277496b2ab
📒 Files selected for processing (2)
crates/weaverd/src/dispatch/observe/enrich.rsdocs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Code Duplicationcrates/weaverd/src/dispatch/observe/enrich_tests.rs: What lead to degradation?The module contains 4 functions with similar structure: detects_deprecation_in_hover_text,ignores_unstructured_deprecated_mentions_in_hover_text,try_lsp_enrichment_degrades_when_hover_is_missing,try_lsp_enrichment_degrades_when_initialization_fails Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
…chment doc - Document known limitation regarding UTF-16 conversion for hover positions using Tree-sitter byte offsets, which impacts multi-byte UTF-8 characters. - Clarify detection of deprecation in hover response by identifying structured deprecation markers. - Slight rephrasing and formatting improvements for better context and orientation in the observe get-card documentation. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md (1)
209-225:⚠️ Potential issue | 🟡 MinorAdd a caption directly above the table.
Insert a short caption line immediately before the header so the table is self-describing and guideline-compliant.
📝 Proposed fix
Key types and locations across the three crates involved in the `observe get-card` operation: +Table 1. Mapping of key types and symbols to defining files. + | Type | File | | ------------------------- | ------------------------------------------------- | | `SymbolCard`, `LspInfo` | `crates/weaver-cards/src/card.rs` |As per coding guidelines, "Caption every table and every diagram in documentation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md` around lines 209 - 225, Add a one-line caption immediately above the Markdown table header in docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md so the table is self-describing; insert a short descriptive caption (e.g., "Table: Key types and locations referenced in this plan") on the line directly before the header row (before the line that starts with "| Type") to satisfy the "Caption every table" guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md`:
- Around line 197-204: Reword the two bullet points to present them as
pre-change baseline context (e.g., start with "Before 7.1.3...") instead of
present-tense assertions: explicitly state that prior to the change,
crates/weaver-lsp-host managed per-language LSP servers and supported
Definition, References, Diagnostics, CallHierarchy (but not Hover), and that
crates/weaverd's handler at crates/weaverd/src/dispatch/observe/get_card.rs only
received (request, writer) and required the router at
crates/weaverd/src/dispatch/router.rs to be updated to pass backends so the
handler could attempt LSP enrichment; keep references to the exact symbols
(weaver-lsp-host, weaverd, get_card.rs, router.rs, backends, Hover) so readers
can locate the relevant code.
---
Duplicate comments:
In
`@docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md`:
- Around line 209-225: Add a one-line caption immediately above the Markdown
table header in
docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md
so the table is self-describing; insert a short descriptive caption (e.g.,
"Table: Key types and locations referenced in this plan") on the line directly
before the header row (before the line that starts with "| Type") to satisfy the
"Caption every table" guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb01d85e-6240-46c9-8d44-40403ce6717e
📒 Files selected for processing (1)
docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md
…lization - Advertise UTF-8 position encoding preference in LSP InitializeParams - Log a debug message when server declines UTF-8 negotiation - Allows usage of byte offsets as character offsets for LSP features This improves correctness for non-ASCII files without requiring major refactoring. Also includes minor optimizations and test refactors: - Use iter().any() to avoid String allocation in provenance checks - Refactor hover enrichment tests for clarity and reuse - Update test helpers to return temp directory for ephemeral socket Refs #docs:execplans: implement-optional-lsp-enrichment-for-observe-get-card Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. crates/weaverd/src/dispatch/observe/enrich_tests.rs Comment on file fn try_lsp_enrichment_degrades_when_initialization_fails() {
let server = StubLanguageServer::failing_initialize(
ServerCapabilitySet::new(false, false, false).with_hover(true),
"boom",
);
let (outcome, backends, card) = run_enrichment_with_server(server);
assert_eq!(outcome, EnrichmentOutcome::Degraded);
assert!(backends.is_started(BackendKind::Semantic));
assert!(card.lsp.is_none());
}❌ New issue: Code Duplication |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaverd/src/dispatch/observe/enrich_test_utils.rs`:
- Around line 176-214: The function formatting in run_non_ascii_enrichment and
the SymbolCard construction has drifted from rustfmt; run rustfmt by executing
`cargo fmt --all` (or `make check-fmt` to validate), update the file
crates/weaverd/src/dispatch/observe/enrich_test_utils.rs with the formatted
output for run_non_ascii_enrichment, the SymbolCard literal
(symbol_ref/SourceRange/SourcePosition fields) and any nearby assertions, and
commit the changes so CI `cargo fmt --check` passes.
- Around line 131-133: Replace the forbidden #[allow(dead_code)] on the
test_symbol_card_with_pos function with a narrowly scoped #[expect(dead_code,
reason = "...")] attribute; remove the #[allow] line and add #[expect(dead_code,
reason = "used in tests only/keeps helper available for encoding tests")]
immediately above the pub(crate) fn test_symbol_card_with_pos declaration so the
lint is explicitly expected and scoped to that function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 228d5633-d24a-4a22-bb8b-5fc834cb3f6f
📒 Files selected for processing (2)
crates/weaverd/src/dispatch/observe/enrich_test_utils.rscrates/weaverd/src/dispatch/observe/enrich_tests.rs
Formatted the function parameter lists and struct field initializations for better readability. Changed #[allow(dead_code)] to #[expect(dead_code, reason = "keeps helper available for encoding tests")] to clarify why the code is kept despite being unused. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaverd/src/dispatch/observe/enrich_test_utils.rs`:
- Around line 187-222: Replace the duplicated inline SymbolCard literal with the
shared fixture builder: call test_symbol_card_with_pos(0, 12, 0, 15) to create
the card (e.g., let mut card = test_symbol_card_with_pos(0, 12, 0, 15);) instead
of constructing SymbolCard manually; keep the card mutable and preserve any
subsequent modifications to fields like etag/provenance if present after
creation.
- Around line 179-239: The helper run_non_ascii_enrichment currently returns
(EnrichmentOutcome, u32) but unconditionally asserts enrichment success; change
the contract to return only the UTF-16 character offset (u32) and enforce
success: call try_lsp_enrichment(&mut card, source, &mut backends), assert the
result equals EnrichmentOutcome::Enriched (use the enum variant
EnrichmentOutcome::Enriched) before continuing, remove the returned
EnrichmentOutcome from the tuple and from all call sites, and keep the rest of
the logic (locking hover_params_ref and extracting position.character) unchanged
so the function now returns only the character offset.
- Around line 42-46: The test helper check_utf16_offset currently performs a
lossy cast of byte_col (usize) to u32 when calling byte_col_to_utf16; replace
the raw cast with a checked conversion (e.g. using u32::try_from(byte_col) or
u32::try_from(byte_col as usize).expect("...")) so oversized byte_col values
cause an explicit panic/failure instead of silent truncation—update the call in
check_utf16_offset to convert byte_col to u32 via try_from and assert/expect on
the conversion result before passing it to byte_col_to_utf16.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88b69bcc-d115-46e2-9fa2-fae9c9133722
📒 Files selected for processing (1)
crates/weaverd/src/dispatch/observe/enrich_test_utils.rs
…rove test utils - run_non_ascii_enrichment now returns just the character offset and asserts success internally - Replaced manual SymbolCard construction with test_symbol_card_with_pos helper - Removed redundant assertion and dead code annotation - Updated tests to match new run_non_ascii_enrichment signature - Added explicit u32 conversion for byte_col in check_utf16_offset Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaverd/src/dispatch/observe/enrich_test_utils.rs`:
- Around line 195-204: The code creates an unnecessary intermediate binding
`character` before returning it, triggering Clippy's let_and_return lint; remove
the `let character = params.text_document_position_params.position.character;`
binding and return `params.text_document_position_params.position.character`
directly (operate on the already-locked `hover_params_ref`, `hover_params`, and
`params` values and replace the final `character` return with the direct
expression).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4ef26bd-acd9-488b-aa5d-d1aa57eb0a14
📒 Files selected for processing (2)
crates/weaverd/src/dispatch/observe/enrich_test_utils.rscrates/weaverd/src/dispatch/observe/enrich_tests.rs
…lity Removed the unused local variable `character` in `run_non_ascii_enrichment` to clean up the code and return the character value directly. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaverd/src/dispatch/observe/enrich_test_utils.rs`:
- Around line 95-130: rust_card duplicates SymbolCard construction from
test_symbol_card_with_pos; change rust_card to call
test_symbol_card_with_pos(...) and override only the differing fields
(symbol_id/symbol_ref.uri/symbol_ref.range/start/end/symbol_ref.language/symbol_ref.name/symbol_ref.kind
and provenance.extracted_at and provenance.sources) so the shared construction
lives in test_symbol_card_with_pos; update rust_card to pass appropriate
SourcePosition values (line/column) and then set the Rust-specific fields on the
returned SymbolCard before returning.
- Around line 52-66: Replace the inline Hover construction in assert_deprecation
with the existing test helper markdown_hover from the test_support module: call
markdown_hover(text) to create the Hover passed into parse_hover_response inside
assert_deprecation; update the assert_deprecation function to use that helper
(preserving the call to parse_hover_response and the assert_eq on
info.deprecated) and then remove the now-unused imports Hover, HoverContents,
MarkupContent, and MarkupKind from the top of the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3144bda2-2e17-46ff-a408-e2fab3b95ef4
📒 Files selected for processing (1)
crates/weaverd/src/dispatch/observe/enrich_test_utils.rs
… imports - Replaced explicit construction of a `SymbolCard` with calls to helper functions. - Removed unused imports related to LSP hover types. - Streamlined `assert_deprecation` by using `markdown_hover` helper. - Reduced code duplication and improved test utility maintainability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaverd/src/dispatch/observe/enrich_test_utils.rs`:
- Around line 102-107: The function test_symbol_card_with_pos currently accepts
four u32 coordinates; change its signature to accept two grouped SourcePosition
values (e.g., start: SourcePosition, end: SourcePosition) and update the body to
use start.line/start.column and end.line/end.column when constructing the
SymbolCard; rename parameters in the signature and update all call-sites to pass
SourcePosition instances instead of individual scalars, ensuring types like
SourcePosition and SymbolCard are imported/visible where used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd696b06-6ba5-4efb-9d30-a2a6d3c73088
📒 Files selected for processing (1)
crates/weaverd/src/dispatch/observe/enrich_test_utils.rs
…rameters Refactored test_symbol_card_with_pos and related calls to use SourcePosition structs instead of separate line and column integers for start and end positions, improving code clarity and consistency in position handling. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Improved the formatting of SourcePosition structs in enrich_test_utils.rs for better readability by expanding inline structs into multi-line format. Also added a new empty .repo.lock file under context_pack/packs as part of repository maintenance. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/weaverd/src/dispatch/observe/enrich_test_utils.rs (1)
142-147:⚠️ Potential issue | 🟡 MinorFix rustfmt drift in the non-ASCII
SourcePositionblock.Run
cargo fmt --alland commit the formatted output for this block; CI reports a rustfmt mismatch here.As per coding guidelines: "Run
make check-fmt,make lint, andmake testbefore committing, which executecargo fmt --workspace -- --check,cargo clippy --workspace --all-targets --all-features -- -D warnings, andcargo test --workspacerespectively."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/observe/enrich_test_utils.rs` around lines 142 - 147, The SourcePosition block passed to test_symbol_card_with_pos is misformatted (rustfmt drift) causing CI to fail; run cargo fmt --all (or make check-fmt) and reformat the SourcePosition initializer so it matches rustfmt style (e.g., align commas/spacing on the SourcePosition { line: 0, column: 12 } and SourcePosition { line: 0, column: 15 } arguments), then commit the formatted changes so the call to test_symbol_card_with_pos is rustfmt-compliant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/weaverd/src/dispatch/observe/enrich_test_utils.rs`:
- Around line 142-147: The SourcePosition block passed to
test_symbol_card_with_pos is misformatted (rustfmt drift) causing CI to fail;
run cargo fmt --all (or make check-fmt) and reformat the SourcePosition
initializer so it matches rustfmt style (e.g., align commas/spacing on the
SourcePosition { line: 0, column: 12 } and SourcePosition { line: 0, column: 15
} arguments), then commit the formatted changes so the call to
test_symbol_card_with_pos is rustfmt-compliant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26e266b3-95fd-448e-98d5-6c68d64040a5
📒 Files selected for processing (1)
crates/weaverd/src/dispatch/observe/enrich_test_utils.rs
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaverd/src/dispatch/observe/enrich_test_utils.rs`:
- Around line 77-85: The base test fixture currently pre-seeds a degraded
provenance which masks regressions; stop pre-seeding and make the base card
neutral (only tree_sitter), then in assert_enrichment_degrades (which calls
run_enrichment_with_server) assert that the returned card's provenance equals
tree_sitter_degraded_semantic and not just that card.lsp is None. Update the
base fixture setup to only include tree_sitter provenance, remove any pre-seeded
degraded flag, and add an assertion in assert_enrichment_degrades that verifies
card.provenance == tree_sitter_degraded_semantic (and keep the existing
assert_eq!(outcome, EnrichmentOutcome::Degraded)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66d2406a-d49e-419f-996a-813e57fb6167
⛔ Files ignored due to path filters (1)
.agents/mcp/context_pack/packs/.repo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
crates/weaverd/src/dispatch/observe/enrich_test_utils.rs
…provenance This adds an assertion to ensure that when enrichment degrades, the provenance sources remain unchanged. It also adjusts the rust_card test utility to start with neutral provenance to better validate provenance preservation or modification during enrichment. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Weaverd enrichment
Weave LSP hover capability updates
Tests
Documentation & ExecPlan
Task
Why
How it works (high level)
Validation plan
Compatibility & notes
Artefacts
New: weaverd/src/dispatch/observe/enrich.rs
Updated: crates/weaverd/src/dispatch/observe/get_card.rs (signature and enrichment call)
Updated: crates/weaverd/src/dispatch/router.rs (pass backends to handler)
Updated: crates/weaver-lsp-host (hover capability additions across capability.rs, server.rs, host.rs, doc_support.rs, errors.rs, tests)
Added: docs/execplans/7-1-3-implement-optional-lsp-enrichment-for-observe-get-card.md
Updated: docs/roadmap.md, docs/users-guide.md
Reference
📎 Task: https://www.devboxer.com/task/abcffead-b952-4fc4-a99d-e49e9cd8fd5b