Declare rename-symbol capability for rust-analyzer plugin#75
Merged
Conversation
Reviewer's GuideImplements the shared rename-symbol capability contract for the rust-analyzer plugin, including dedicated argument parsing and structured failure handling with reason codes, wires rust-analyzer into the daemon’s manifest-based capability system alongside rope, and adds CLI transport retry logic plus tests and docs updates to reflect the new behaviour. Sequence diagram for the rename-symbol request flow through daemon and rust-analyzer pluginsequenceDiagram
actor User
participant CLI as weaver_cli
participant Daemon as weaverd
participant Runtime as SandboxRefactorRuntime
participant RAPlugin as weaver_plugin_rust_analyzer
User->>CLI: invoke act_refactor(provider=rust-analyzer, refactoring=rename)
CLI->>Daemon: Socket request (operation rename)
Daemon->>Daemon: map refactoring rename -> operation rename-symbol
Daemon->>Runtime: dispatch capability RenameSymbol
Runtime->>Runtime: lookup rust_analyzer_manifest(capability RenameSymbol)
Runtime->>RAPlugin: launch plugin process with PluginRequest
Note over RAPlugin: operation = rename-symbol
Note over RAPlugin: arguments = { uri, position, new_name }
RAPlugin->>RAPlugin: read_request(stdin)
RAPlugin->>RAPlugin: execute_request(adapter, request)
RAPlugin->>RAPlugin: match operation
RAPlugin->>RAPlugin: execute_rename(adapter, request)
RAPlugin->>RAPlugin: parse_rename_symbol_arguments(arguments)
alt invalid arguments or uri mismatch
RAPlugin->>RAPlugin: Err(PluginFailure with ReasonCode IncompletePayload)
RAPlugin->>RAPlugin: failure_response(PluginFailure)
RAPlugin-->>Runtime: PluginResponse::failure(diagnostic with reason_code)
else valid arguments
RAPlugin->>RAPlugin: adapter.rename(file, ByteOffset, new_name)
alt adapter error or unchanged content
RAPlugin->>RAPlugin: Err(PluginFailure with ReasonCode SymbolNotFound)
RAPlugin->>RAPlugin: failure_response(PluginFailure)
RAPlugin-->>Runtime: PluginResponse::failure(diagnostic with reason_code)
else successful diff
RAPlugin->>RAPlugin: build_search_replace_patch
RAPlugin-->>Runtime: PluginResponse::success(PluginOutput::Diff)
end
end
Runtime-->>Daemon: PluginResponse
Daemon-->>CLI: formatted result
CLI-->>User: print success or structured error with reason_code
Sequence diagram for CLI daemon connection with retrysequenceDiagram
actor User
participant CLI as weaver_cli
participant Transport as transport_rs
participant Daemon as weaverd
User->>CLI: run command that may auto-start daemon
CLI->>Daemon: try auto_start_daemon()
Daemon-->>CLI: started successfully
CLI->>Transport: connect_with_retry(endpoint, retry_window)
loop until success or retry_window elapsed
Transport->>Transport: call connect(endpoint)
alt daemon socket ready
Transport-->>CLI: Ok(Connection)
else daemon not running yet
Transport->>Transport: is_daemon_not_running(error) == true
Transport->>Transport: compute sleep_duration
Transport->>Transport: thread::sleep(sleep_duration)
else other error
Transport-->>CLI: Err(AppError)
end
end
alt connection established
CLI-->>User: proceed with request over Connection
else connection failed
CLI-->>User: print error and exit with failure
end
Updated class diagram for rust-analyzer plugin rename-symbol handlingclassDiagram
class RustAnalyzerAdapter {
<<trait>>
+rename(file: FilePayload, offset: ByteOffset, new_name: &str) Result<String, RustAnalyzerAdapterError>
}
class RustAnalyzerLspAdapter {
+rename(file: FilePayload, offset: ByteOffset, new_name: &str) Result<String, RustAnalyzerAdapterError>
}
RustAnalyzerLspAdapter ..|> RustAnalyzerAdapter
class PluginFailure {
-message: String
-reason_code: Option~ReasonCode~
+plain(message: String) PluginFailure
+with_reason(message: String, reason: ReasonCode) PluginFailure
+message() &str
+reason_code() Option~ReasonCode~
+fmt(self, f: Formatter) fmt::Result
}
class RenameSymbolArgs {
-uri: String
-offset: usize
-new_name: String
+uri() &str
+offset() usize
+new_name() &str
}
class arguments_rs {
+parse_rename_symbol_arguments(arguments: HashMap~String, Value~) Result~RenameSymbolArgs, String~
-parse_uri(arguments: HashMap~String, Value~) Result~String, String~
-parse_position(arguments: HashMap~String, Value~) Result~usize, String~
-parse_new_name(arguments: HashMap~String, Value~) Result~String, String~
-json_value_to_string(value: Value) Option~String~
}
arguments_rs --> RenameSymbolArgs
class lib_rs {
+run(stdin: BufRead, stdout: Write) Result~(), PluginDispatchError~
+run_with_adapter(stdin: BufRead, stdout: Write, adapter: RustAnalyzerAdapter) Result~(), PluginDispatchError~
+read_request(stdin: BufRead) Result~PluginRequest, PluginFailure~
+execute_request(adapter: RustAnalyzerAdapter, request: PluginRequest) Result~PluginResponse, PluginFailure~
+execute_rename(adapter: RustAnalyzerAdapter, request: PluginRequest) Result~PluginResponse, PluginFailure~
+failure_response(failure: PluginFailure) PluginResponse
}
lib_rs --> RustAnalyzerAdapter
lib_rs --> PluginFailure
lib_rs --> RenameSymbolArgs
lib_rs --> arguments_rs
class CapabilityTypes {
<<external>>
ReasonCode
CapabilityId
}
class PluginProtocolTypes {
<<external>>
PluginRequest
PluginResponse
PluginOutput
PluginDiagnostic
FilePayload
DiagnosticSeverity
}
PluginFailure --> CapabilityTypes
lib_rs --> PluginProtocolTypes
class manifests_rs {
+rope_manifest(executable: PathBuf) PluginManifest
+rust_analyzer_manifest(executable: PathBuf) PluginManifest
}
class PluginManifest {
<<external>>
+with_capabilities(caps: Vec~CapabilityId~) PluginManifest
+with_timeout_secs(timeout_secs: u64) PluginManifest
}
manifests_rs --> PluginManifest
manifests_rs --> CapabilityTypes
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughThis pull request finalises the rename-symbol capability contract for the Rust Analyzer plugin. It adds connection retry logic with timeout support to the CLI transport layer, introduces structured error handling via PluginFailure with stable reason codes, refactors plugin argument parsing into reusable RenameSymbolArgs, updates daemon-side manifest factories, and aligns all tests and documentation to the new contract semantics using uri, position, and new_name arguments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
leynos
added a commit
that referenced
this pull request
Mar 7, 2026
…#73) * feat(observe-get-card): define stable JSON schemas and add handler with refusal - Introduce new crate `weaver-cards` with stable serde-annotated types for `observe get-card` request and response schemas. - Define SymbolCard data structures, symbol identity, progressive detail levels, requests, responses, refusal reasons, and errors. - Add insta snapshot tests and BDD feature tests to lock JSON shapes and validate request parsing. - Update `weaverd` to recognize `observe get-card` operation and dispatch to a new handler. - The handler parses arguments and currently returns a structured refusal indicating extraction is not yet implemented. - Document `observe get-card` command in `docs/users-guide.md`. - Update workspace and crate Cargo.toml files and mark roadmap item 7.1.1 complete. This lays the foundation for future Tree-sitter extraction and LSP enrichment in follow-up tasks. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(tests): simplify and modularize snapshot test helpers Refactored snapshot tests in crates/weaver-cards by consolidating duplicate fixture builders into reusable sample functions with parameters, improving clarity and reducing code duplication. Updated tests to use these refactored helpers without changing test logic. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * feat(weaver-cards): add DetailLevel parsing with error type and improved tests - Introduce DetailLevelParseError and FromStr impl for DetailLevel to parse strings into detail levels with error reporting. - Refactor request parsing to use DetailLevel::from_str and map errors appropriately. - Export DetailLevelParseError from crate. - Add round-trip serde tests using rstest parameterization. - Add snapshot tests for refusal responses for unsupported language and backend unavailable. - Add router test for get-card structured refusal. - Fix multiple documentation typos related to serialization and American English conventions. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(weaver-cards): improve argument parsing and test code clarity - Enhance require_arg_value to reject flag-like values as arguments, improving input validation. - Update parse_detail and related functions for clearer error mapping. - Simplify and strengthen BDD and snapshot tests by parsing detail levels directly. - Refactor refusal response constructors into helper functions to reduce duplication. - Improve JSON assertion robustness in tests. - Fix formatting and wording in documentation for consistency and clarity. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(tests): Refactor BDD helpers and snapshot tests for card refusals - Improved parsing and building of refusal reasons in BDD test helpers - Added detailed refusal response builder for varied refusal reasons - Converted multiple snapshot refusal tests into parameterized rstest cases - Renamed snapshot files for consistency - Minor docs formatting adjustment related to testing explanations Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * docs(execplans): clarify repository root and tooling instructions Updated documentation to refer to the repository root as `./` instead of a specific absolute path, and simplified validation instructions removing conditional phrasing for `markdownlint`. Changed all command paths to indicate execution from the repository root for clarity. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * docs(execplans): fix typo 'sub-modules' to 'submodules' in documentation Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * docs(execplans): improve formatting and add footnote reference in docs Updated the 7-1-1-stable-jsonl-schemas-for-observe-get-card.md documentation to format roadmap task reference and issue closing statement more clearly. Added a footnote for the roadmap.md lines to enhance readability and referencing. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * docs(execplans): fix formatting in roadmap completion statement Correct spacing around the text closing issue #75 in the roadmap completion statement for improved clarity and consistency. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * feat(cards): add attachments and interstitial data to SymbolCard schema - Introduce AttachmentsInfo, NormalizedAttachments, ImportInterstitialInfo, and InterstitialInfo structs - Add attachments and interstitial fields to SymbolCard at structure and file/module detail levels - Update DetailLevel enum docs to include attachments in Structure level - Support skipping unknown --flags in GetCardRequest parsing for forward compatibility - Update tests, snapshots, and docs to reflect new card fields and parsing behavior This enhances SymbolCard with bundled doc comments, decorators, and import blocks, enabling richer code metadata in observe get-card responses. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(tests): refactor behaviour tests to return SymbolCard directly with detail levels - Changed `build_card` to return `SymbolCard` directly instead of Option - Implemented helper functions for sample data (signature, doc, structure, lsp, metrics, deps) - Simplified `build_card_at_level` to construct SymbolCard with progressively detailed fields - Renamed serialization step functions and feature file steps to use American English spelling "serialized" - Fixed minor typos in function names and docs - Updated round-trip test for consistent American English spelling - Minor doc tweak to users-guide.md for clarity and punctuation Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(tests): simplify JSON serialization and improve JSON field access Simplified the JSON serialization logic in `when_card_serialized` by removing fallback serialization of the response and enforcing that a card must be set. Improved the JSON field access in `then_json_field_has_value` by using JSON pointer syntax to support nested keys. Also included minor documentation markdown formatting fixes unrelated to tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(tests): use JSON pointer for nested field checks in behaviour tests Refactored tests in crates/weaver-cards to use JSON pointer syntax when checking for the presence and absence of nested JSON fields. This improves correctness and robustness of JSON validation by supporting nested keys represented with dot notation. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(tests): extract shared fixture builders to dedicated module - Moved fixture builder functions from behaviour and snapshot_tests into a new shared module `tests::fixtures` for reuse and better organization. - Updated test modules to delegate fixture construction to this shared module, reducing duplication. - Modified `build_card` in behaviour tests to call new shared fixture's `build_card_at_level`. - Adjusted snapshot_tests to wrap shared fixtures with snapshot-specific data. - Added `mod fixtures;` in `tests/mod.rs` to include the new module. - Minor docs fix in users-guide.md for punctuation and formatting. This reorganization enhances maintainability and consistency of test data across weaver-cards crate tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(weaver-cards): refactor refusal response builder and clean test snapshots - Simplified build_refusal_response by extracting refusal_message helper - Returned early for NotYetImplemented case - Cleaned up snapshot_full_card test to use structure_card base Additionally, updated users-guide.md to clarify zero-based ranges in card.symbol.ref.range. This improves code clarity and consistency. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(tests): refactor JSON test helpers for clarity and reuse Refactor the JSON-related test step implementations in behaviour.rs by extracting common code into a helper function that parses JSON and constructs JSON pointers. This reduces duplication and improves readability. Additionally, add handling for the `NotYetImplemented` refusal reason. Also update the users-guide.md formatting for better readability of card detail levels. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * feat(weaver-cards): support unknown flags in get-card requests Add handling of unknown flags in get-card CLI requests so they are silently skipped. Update related behaviour tests and feature scenarios to cover this new behavior. Refactor refusal message construction to handle non-exhaustive reason variants. Enhance user guide documentation to clarify response envelope structure based on status. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> --------- 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: crates/weaver-cli/src/tests/unit/auto_start.rs Comment on file // robust under typical test loads.
let socket_path_for_thread = socket_path.clone();
let listener_handle = thread::spawn(move || {
let listener_handle = thread::spawn(move || -> Result<(), String> {❌ New issue: Large Method |
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
leynos
changed the title
…y update Add detailed execution plan documenting the migration of the Rust actuator plugin from the legacy rename request shape to the shared `rename-symbol` capability contract. This ExecPlan covers purpose, constraints, risks, progress stages, decisions, and implementation for roadmap item 5.2.3, ensuring the Rust plugin and daemon support the standardized rename-symbol capability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…uctured errors - Rust analyzer plugin now accepts only "rename-symbol" operation with arguments `uri`, `position`, and `new_name`. - Added structured PluginFailure errors carrying ReasonCode for precise diagnostics. - Updated daemon manifest registration to declare `rename-symbol` capability. - Refactored refactor handler to use new manifest builders for rope and rust-analyzer plugins. - Fixed daemon auto-start race with bounded retry connect in CLI transport layer. - Enhanced tests, including BDD scenarios, to cover new rename-symbol contract and error reason codes. - Documentation updated to reflect capability contract and option naming. This change aligns Rust plugin with the shared rename-symbol contract used by rope, and enables robust error handling and capability-based routing on the daemon. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…et binding Refactored the auto_start_succeeds_and_proceeds test to spawn a Unix socket listener after a short delay, ensuring the first connection attempt fails but the retry succeeds. This removes complex thread coordination by introducing a precise delayed bind via the `spawn_delayed_unix_listener` helper. Extracted common assertions into `assert_auto_start_success` to improve test readability and maintainability. Updated test comments to clearly outline the test flow and timing rationale for socket binding and retries. This enhances test robustness and makes the success path easier to understand and maintain on Unix platforms. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Restore the connect_with_retry call site in weaver-cli after the manual rebase replay dropped it. Without this change the rebased branch left the retry helper unused, regressed daemon auto-start behaviour, and caused both make test and make lint to fail. Validation: - make check-fmt - make test - make lint
devboxerhub
Bot
force-pushed
the
update-rust-plugin-rename-symbol-evd0lq
branch
from
8ebe3a0 to
f815ff5
Compare
leynos
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/weaver-plugin-rust-analyzer/src/tests/mod.rs" line_range="221-230" />
<code_context>
match scenario {
FailureScenario::NoChange => assert!(
- err.contains("no content changes"),
+ err.message().contains("no content changes"),
"expected no-change diagnostic, got: {err}"
),
FailureScenario::AdapterError => assert!(
- err.contains("rust-analyzer adapter failed"),
+ err.message().contains("rust-analyzer adapter failed"),
"expected adapter error message, got: {err}"
),
+ FailureScenario::UriMismatch => assert!(
+ err.message().contains("does not match file payload"),
+ "expected uri mismatch diagnostic, got: {err}"
</code_context>
<issue_to_address>
**suggestion (testing):** Assert `ReasonCode::SymbolNotFound` for non-mutating and adapter-error rename failures.
In `rename_non_mutating_or_error_returns_failure`, we now assert on error messages for each failure case, but not on the corresponding `ReasonCode`. Since `execute_rename` maps both `NoChange` and `AdapterError` to `ReasonCode::SymbolNotFound`, please extend the test to assert `err.reason_code() == Some(ReasonCode::SymbolNotFound)` in those branches, and consider asserting that `UriMismatch` continues to map to `IncompletePayload`. This will better lock in the intended error-to-reason-code mapping.
Suggested implementation:
```rust
match scenario {
FailureScenario::NoChange => {
assert!(
err.message().contains("no content changes"),
"expected no-change diagnostic, got: {err}"
);
assert_eq!(
err.reason_code(),
Some(ReasonCode::SymbolNotFound),
"expected ReasonCode::SymbolNotFound for no-change failure, got: {err:?}"
);
}
FailureScenario::AdapterError => {
assert!(
err.message().contains("rust-analyzer adapter failed"),
"expected adapter error message, got: {err}"
);
assert_eq!(
err.reason_code(),
Some(ReasonCode::SymbolNotFound),
"expected ReasonCode::SymbolNotFound for adapter-error failure, got: {err:?}"
);
}
FailureScenario::UriMismatch => {
assert!(
err.message().contains("does not match file payload"),
"expected uri mismatch diagnostic, got: {err}"
);
assert_eq!(
err.reason_code(),
Some(ReasonCode::IncompletePayload),
"expected ReasonCode::IncompletePayload for URI mismatch, got: {err:?}"
);
}
}
}
```
To compile, ensure `ReasonCode` is in scope in this test module. For example, if it lives in `weaver_diagnostics` or similar, add an appropriate import near the top of `mod.rs`, such as:
- `use weaver_diagnostics::ReasonCode;`
Adjust the path to match the actual location of `ReasonCode` in your codebase. If `reason_code()` is provided by a trait, also import that trait so the method is available in this scope.
</issue_to_address>
### Comment 2
<location path="crates/weaver-plugin-rust-analyzer/src/tests/behaviour.rs" line_range="197-206" />
<code_context>
);
}
+#[then("the failure reason code is {text}")]
+fn then_failure_reason_code(world: &mut World, text: String) {
+ let expected = match text.trim_matches('"') {
+ "incomplete_payload" => ReasonCode::IncompletePayload,
+ "operation_not_supported" => ReasonCode::OperationNotSupported,
+ other => panic!("unsupported reason code in feature: {other}"),
+ };
+ let response = resolved_response(world);
+ assert!(
+ response
+ .diagnostics()
+ .iter()
+ .any(|diagnostic| diagnostic.reason_code() == Some(expected)),
+ "expected reason code {expected:?}, got: {:?}",
+ response.diagnostics(),
+ );
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Add a behaviour scenario that exercises the `symbol_not_found` reason code.
The new `then_failure_reason_code` step validates reason-code wiring, but the matcher only allows `incomplete_payload` and `operation_not_supported`. Since `execute_rename` now uses `ReasonCode::SymbolNotFound` for adapter failures and non-mutating renames, please extend the matcher to accept `"symbol_not_found"` and add a behaviour scenario where the adapter fails or yields no changes, asserting that the reason code is `symbol_not_found` to mirror the unit-level expectations.
Suggested implementation:
```rust
#[then("the failure reason code is {text}")]
fn then_failure_reason_code(world: &mut World, text: String) {
let expected = match text.trim_matches('"') {
"incomplete_payload" => ReasonCode::IncompletePayload,
"operation_not_supported" => ReasonCode::OperationNotSupported,
"symbol_not_found" => ReasonCode::SymbolNotFound,
other => panic!("unsupported reason code in feature: {other}"),
};
let response = resolved_response(world);
assert!(
response
.diagnostics()
.iter()
.any(|diagnostic| diagnostic.reason_code() == Some(expected)),
"expected reason code {expected:?}, got: {:?}",
response.diagnostics(),
);
}
```
To fully implement your comment, you also need to:
1. Add a new behaviour scenario in the relevant `.feature` file under `crates/weaver-plugin-rust-analyzer/src/tests/` (or wherever the rename behaviour features live). The scenario should:
- Drive `execute_rename` into a situation where the adapter fails or produces no changes (i.e., a non-mutating rename).
- Use the existing steps to send the rename request and capture the failure.
- End with a step like:
`Then the failure reason code is "symbol_not_found"`
which will now be handled by the updated `then_failure_reason_code` step.
2. If you have separate feature files for success and failure cases (e.g. `rename_failure.feature`), place the scenario alongside the other failure scenarios to keep the suite organised and consistent with existing conventions.
</issue_to_address>
### Comment 3
<location path="docs/execplans/5-2-3-update-rust-plugin-to-declare-rename-symbol.md" line_range="25" />
<code_context>
+`position`, and `new_name`, and successful responses still return diff output
+that flows through the existing Double-Lock safety harness.
+
+The user-visible command stays the same:
+
+```sh
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym "CLI" is used later in the document without being expanded on first use.
This file is new, so acronyms need defining within it. The text later refers to "CLI" without having expanded it previously. Consider expanding it as "command-line interface (CLI)" at its first appearance in the document.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 4
<location path="docs/execplans/5-2-3-update-rust-plugin-to-declare-rename-symbol.md" line_range="111" />
<code_context>
+- Risk: The rust-analyzer plugin currently accepts `"rename"` with `offset`
+ rather than `"rename-symbol"` with `position`. This migration can easily
+ leave unit and BDD fixtures half-updated. Severity: high Likelihood: high
+ Mitigation: update plugin request builders, unit tests, and feature steps in
+ the same commit slice before changing dispatch assertions.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym "BDD" is introduced later in this paragraph without being defined.
Within this new document, "BDD" appears ("unit and BDD fixtures") without a preceding definition. Please expand it on first use, for example "behaviour-driven development (BDD)", to comply with the acronym rule.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
221
to
+230
| match scenario { | ||
| FailureScenario::NoChange => assert!( | ||
| err.contains("no content changes"), | ||
| err.message().contains("no content changes"), | ||
| "expected no-change diagnostic, got: {err}" | ||
| ), | ||
| FailureScenario::AdapterError => assert!( | ||
| err.contains("rust-analyzer adapter failed"), | ||
| err.message().contains("rust-analyzer adapter failed"), | ||
| "expected adapter error message, got: {err}" | ||
| ), | ||
| FailureScenario::UriMismatch => assert!( |
There was a problem hiding this comment.
suggestion (testing): Assert ReasonCode::SymbolNotFound for non-mutating and adapter-error rename failures.
In rename_non_mutating_or_error_returns_failure, we now assert on error messages for each failure case, but not on the corresponding ReasonCode. Since execute_rename maps both NoChange and AdapterError to ReasonCode::SymbolNotFound, please extend the test to assert err.reason_code() == Some(ReasonCode::SymbolNotFound) in those branches, and consider asserting that UriMismatch continues to map to IncompletePayload. This will better lock in the intended error-to-reason-code mapping.
Suggested implementation:
match scenario {
FailureScenario::NoChange => {
assert!(
err.message().contains("no content changes"),
"expected no-change diagnostic, got: {err}"
);
assert_eq!(
err.reason_code(),
Some(ReasonCode::SymbolNotFound),
"expected ReasonCode::SymbolNotFound for no-change failure, got: {err:?}"
);
}
FailureScenario::AdapterError => {
assert!(
err.message().contains("rust-analyzer adapter failed"),
"expected adapter error message, got: {err}"
);
assert_eq!(
err.reason_code(),
Some(ReasonCode::SymbolNotFound),
"expected ReasonCode::SymbolNotFound for adapter-error failure, got: {err:?}"
);
}
FailureScenario::UriMismatch => {
assert!(
err.message().contains("does not match file payload"),
"expected uri mismatch diagnostic, got: {err}"
);
assert_eq!(
err.reason_code(),
Some(ReasonCode::IncompletePayload),
"expected ReasonCode::IncompletePayload for URI mismatch, got: {err:?}"
);
}
}
}To compile, ensure ReasonCode is in scope in this test module. For example, if it lives in weaver_diagnostics or similar, add an appropriate import near the top of mod.rs, such as:
use weaver_diagnostics::ReasonCode;
Adjust the path to match the actual location ofReasonCodein your codebase. Ifreason_code()is provided by a trait, also import that trait so the method is available in this scope.
Comment on lines
+197
to
+206
| #[then("the failure reason code is {text}")] | ||
| fn then_failure_reason_code(world: &mut World, text: String) { | ||
| let expected = match text.trim_matches('"') { | ||
| "incomplete_payload" => ReasonCode::IncompletePayload, | ||
| "operation_not_supported" => ReasonCode::OperationNotSupported, | ||
| other => panic!("unsupported reason code in feature: {other}"), | ||
| }; | ||
| let response = resolved_response(world); | ||
| assert!( | ||
| response |
There was a problem hiding this comment.
suggestion (testing): Add a behaviour scenario that exercises the symbol_not_found reason code.
The new then_failure_reason_code step validates reason-code wiring, but the matcher only allows incomplete_payload and operation_not_supported. Since execute_rename now uses ReasonCode::SymbolNotFound for adapter failures and non-mutating renames, please extend the matcher to accept "symbol_not_found" and add a behaviour scenario where the adapter fails or yields no changes, asserting that the reason code is symbol_not_found to mirror the unit-level expectations.
Suggested implementation:
#[then("the failure reason code is {text}")]
fn then_failure_reason_code(world: &mut World, text: String) {
let expected = match text.trim_matches('"') {
"incomplete_payload" => ReasonCode::IncompletePayload,
"operation_not_supported" => ReasonCode::OperationNotSupported,
"symbol_not_found" => ReasonCode::SymbolNotFound,
other => panic!("unsupported reason code in feature: {other}"),
};
let response = resolved_response(world);
assert!(
response
.diagnostics()
.iter()
.any(|diagnostic| diagnostic.reason_code() == Some(expected)),
"expected reason code {expected:?}, got: {:?}",
response.diagnostics(),
);
}To fully implement your comment, you also need to:
-
Add a new behaviour scenario in the relevant
.featurefile undercrates/weaver-plugin-rust-analyzer/src/tests/(or wherever the rename behaviour features live). The scenario should:- Drive
execute_renameinto a situation where the adapter fails or produces no changes (i.e., a non-mutating rename). - Use the existing steps to send the rename request and capture the failure.
- End with a step like:
Then the failure reason code is "symbol_not_found"
which will now be handled by the updatedthen_failure_reason_codestep.
- Drive
-
If you have separate feature files for success and failure cases (e.g.
rename_failure.feature), place the scenario alongside the other failure scenarios to keep the suite organised and consistent with existing conventions.
| `position`, and `new_name`, and successful responses still return diff output | ||
| that flows through the existing Double-Lock safety harness. | ||
|
|
||
| The user-visible command stays the same: |
There was a problem hiding this comment.
suggestion (review_instructions): The acronym "CLI" is used later in the document without being expanded on first use.
This file is new, so acronyms need defining within it. The text later refers to "CLI" without having expanded it previously. Consider expanding it as "command-line interface (CLI)" at its first appearance in the document.
Review instructions:
Path patterns: **/*.md
Instructions:
Define uncommon acronyms on first use.
| - Risk: The rust-analyzer plugin currently accepts `"rename"` with `offset` | ||
| rather than `"rename-symbol"` with `position`. This migration can easily | ||
| leave unit and BDD fixtures half-updated. Severity: high Likelihood: high | ||
| Mitigation: update plugin request builders, unit tests, and feature steps in |
There was a problem hiding this comment.
suggestion (review_instructions): The acronym "BDD" is introduced later in this paragraph without being defined.
Within this new document, "BDD" appears ("unit and BDD fixtures") without a preceding definition. Please expand it on first use, for example "behaviour-driven development (BDD)", to comply with the acronym rule.
Review instructions:
Path patterns: **/*.md
Instructions:
Define uncommon acronyms on first use.
![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: f815ff59df
ℹ️ 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".
| })?; | ||
|
|
||
| let request_path = path_to_slash(file.path()); | ||
| if arguments.uri() != request_path { |
There was a problem hiding this comment.
The URI consistency gate now compares the raw uri argument with path_to_slash(file.path()), but only the payload path is normalized. For valid inputs like --file ./src/main.rs, the request can carry uri="./src/main.rs" while request_path becomes src/main.rs, causing a false incomplete_payload failure even though both refer to the same file. This introduces a user-facing regression for rust-analyzer rename requests whenever callers pass equivalent non-canonical relative paths.
Useful? React with 👍 / 👎.
Comment on lines
+284
to
+286
| .map_err(|error| { | ||
| PluginFailure::with_reason(error.to_string(), ReasonCode::SymbolNotFound) | ||
| })?; |
There was a problem hiding this comment.
The new error mapping wraps every adapter error in ReasonCode::SymbolNotFound, including spawn/workspace/protocol/timeout failures that are operational faults rather than missing symbols. This misclassifies failures for any consumer that branches on reason_code, making infrastructure breakages look like user-level rename misses. Only actual lookup failures should emit symbol_not_found; other adapter failures should remain uncoded or use a different reason.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/users-guide.md (1)
788-810:⚠️ Potential issue | 🟡 MinorNormalize the capability examples and inventory.
Use one representation for this field throughout the guide. These new bullets show
capabilities = rename-symbol, but the manifest example later uses
capabilities = ["rename-symbol"], and the later plugin inventory still omits
the rust-analyzer capability entry. Align all three sections so the guide does
not teach two wire shapes for the same manifest field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/users-guide.md` around lines 788 - 810, The docs show inconsistent manifest shapes for the plugin "capabilities" field (e.g., `capabilities = rename-symbol` vs `capabilities = ["rename-symbol"]`) and omits the rust-analyzer capability in the inventory; normalize all examples to the array form `capabilities = ["rename-symbol"]` (update the rope and rust-analyzer bullet examples, the manifest sample that currently uses `capabilities = ["rename-symbol"]` if needed, and any env/path notes that reference capabilities) and add the rust-analyzer entry to the plugin inventory with the same array-form capability so every occurrence uses the identical wire format.crates/weaverd/src/dispatch/act/refactor/tests.rs (1)
325-373:⚠️ Potential issue | 🟠 MajorConstruct file URIs in the
uriargument, not raw paths.The refactor handler's
apply_rename_symbol_mappingfunction (mod.rs, lines 318–320) inserts the rawfileparameter directly into theuriargument. This violates therename-symbolcontract which definesurias a file URI. Convert the file path tofile://...format before insertion. Update the test assertions at lines 335 and 371 to expect the proper URI format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/act/refactor/tests.rs` around lines 325 - 373, The test and handler use a raw file path for the "uri" argument but the rename-symbol contract requires a file URI; update the refactor handler function apply_rename_symbol_mapping to convert the provided file path into a proper file:// URI (e.g., canonicalize/absolute path and prepend "file://") before inserting into the plugin request arguments, and update the tests (handler_overwrites_pre_existing_uri_with_file_path and the earlier dispatch_inspecting_rename assertions that check args.get("uri")) to expect the file://... formatted URI instead of "notes.txt"; ensure you use the same URI construction method in both the handler and tests so comparisons match.crates/weaver-plugin-rust-analyzer/src/tests/mod.rs (1)
47-72: 🧹 Nitpick | 🔵 TrivialAssert the forwarded contract values in the happy-path tests.
The new fixture and request builder exercise
uriandposition, but the mock still ignores the forwardedByteOffsetandnew_name. Lock the migration down by asserting that at least one success-path expectation receivesByteOffset::new(3)and"new_name"; otherwise a parser or forwarding regression would still pass.Based on learnings or 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/weaver-plugin-rust-analyzer/src/tests/mod.rs` around lines 47 - 72, The tests build a PluginRequest with rename_arguments but the mock expectations don't assert that the forwarded ByteOffset and new_name are passed; update the happy-path test(s) that consume request_with_args / rename_arguments to assert the handler receives ByteOffset::new(3) and the string "new_name" (e.g., in the success-path mock expectation for the rename handler), so the request forwarding/parsing is validated; locate the expectations around PluginRequest::with_arguments / request_with_args and enhance them to check these exact values.crates/weaver-plugin-rust-analyzer/src/lib.rs (1)
1-404: 🛠️ Refactor suggestion | 🟠 MajorSplit this module back under the hard 400-line cap.
This file is now 404 lines long. Move one cohesive chunk—such as
PluginFailure/response mapping or patch construction—into a sibling module so this crate stays inside the repository limit.As per coding guidelines "Files must not exceed 400 lines in length".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-plugin-rust-analyzer/src/lib.rs` around lines 1 - 404, The file exceeds the 400-line limit; extract a cohesive chunk into a sibling module. Move the PluginFailure struct, its associated impls (plain, with_reason, message, reason_code), the Display impl for PluginFailure, and the failure_response function into a new module (e.g., response.rs), declare mod response; in lib.rs replace the original definitions with a use or pub use (e.g., use crate::response::PluginFailure; use crate::response::failure_response;) and update any references (failure_response, PluginFailure) to the new module path so compilation and public visibility remain unchanged. Ensure tests still import/refer to PluginFailure via the new module if needed.
🤖 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-plugin-rust-analyzer/src/lib.rs`:
- Around line 266-276: The comparison currently checks arguments.uri() directly
against request_path (computed via path_to_slash(file.path())), which rejects
equivalent relative URIs containing "./" or "src/./file" segments; normalize
both sides before comparing by resolving and collapsing "." components (and
redundant separators) so relative-equivalent paths match; update the check
around request_path and arguments.uri() (the variables and call sites using
path_to_slash, arguments.uri(), PluginFailure::with_reason,
ReasonCode::IncompletePayload) to compare their normalized forms, and add a
regression test asserting a './'-prefixed URI (e.g., "./src/main.rs") is
accepted.
- Around line 278-286: The current map_err on adapter.rename always wraps any
adapter error as PluginFailure::with_reason(..., ReasonCode::SymbolNotFound);
change this to inspect the adapter error (in the map_err closure) and only
return ReasonCode::SymbolNotFound when the adapter explicitly indicates a true
symbol-not-found variant (e.g., match on the adapter error enum variant that
represents missing symbol); for all other errors (workspace creation, spawn,
protocol, timeouts, etc.) return a generic/unclassified PluginFailure (e.g.,
PluginFailure::with_reason(error.to_string(), ReasonCode::Unclassified) or omit
the specific reason) so opaque adapter failures are not mislabelled. Ensure you
update the map_err closure around adapter.rename(...) and keep references to
PluginFailure::with_reason and ReasonCode::SymbolNotFound intact.
In `@docs/execplans/5-2-3-update-rust-plugin-to-declare-rename-symbol.md`:
- Around line 8-13: The status block in
docs/execplans/5-2-3-update-rust-plugin-to-declare-rename-symbol.md is
inconsistent: it currently says "Status: COMPLETE" but the body says "is now in
progress"; pick and replace the status line to match the actual state (either
"Status: IN-PROGRESS" or "Status: COMPLETE") so the header and text agree—update
the single line beginning with "Status:" to reflect the chosen final state and
ensure the surrounding text (e.g., the sentence "Implementation was approved on
2026-03-07 and is now in progress.") is adjusted if you choose COMPLETE.
---
Outside diff comments:
In `@crates/weaver-plugin-rust-analyzer/src/lib.rs`:
- Around line 1-404: The file exceeds the 400-line limit; extract a cohesive
chunk into a sibling module. Move the PluginFailure struct, its associated impls
(plain, with_reason, message, reason_code), the Display impl for PluginFailure,
and the failure_response function into a new module (e.g., response.rs), declare
mod response; in lib.rs replace the original definitions with a use or pub use
(e.g., use crate::response::PluginFailure; use
crate::response::failure_response;) and update any references (failure_response,
PluginFailure) to the new module path so compilation and public visibility
remain unchanged. Ensure tests still import/refer to PluginFailure via the new
module if needed.
In `@crates/weaver-plugin-rust-analyzer/src/tests/mod.rs`:
- Around line 47-72: The tests build a PluginRequest with rename_arguments but
the mock expectations don't assert that the forwarded ByteOffset and new_name
are passed; update the happy-path test(s) that consume request_with_args /
rename_arguments to assert the handler receives ByteOffset::new(3) and the
string "new_name" (e.g., in the success-path mock expectation for the rename
handler), so the request forwarding/parsing is validated; locate the
expectations around PluginRequest::with_arguments / request_with_args and
enhance them to check these exact values.
In `@crates/weaverd/src/dispatch/act/refactor/tests.rs`:
- Around line 325-373: The test and handler use a raw file path for the "uri"
argument but the rename-symbol contract requires a file URI; update the refactor
handler function apply_rename_symbol_mapping to convert the provided file path
into a proper file:// URI (e.g., canonicalize/absolute path and prepend
"file://") before inserting into the plugin request arguments, and update the
tests (handler_overwrites_pre_existing_uri_with_file_path and the earlier
dispatch_inspecting_rename assertions that check args.get("uri")) to expect the
file://... formatted URI instead of "notes.txt"; ensure you use the same URI
construction method in both the handler and tests so comparisons match.
In `@docs/users-guide.md`:
- Around line 788-810: The docs show inconsistent manifest shapes for the plugin
"capabilities" field (e.g., `capabilities = rename-symbol` vs `capabilities =
["rename-symbol"]`) and omits the rust-analyzer capability in the inventory;
normalize all examples to the array form `capabilities = ["rename-symbol"]`
(update the rope and rust-analyzer bullet examples, the manifest sample that
currently uses `capabilities = ["rename-symbol"]` if needed, and any env/path
notes that reference capabilities) and add the rust-analyzer entry to the plugin
inventory with the same array-form capability so every occurrence uses the
identical wire format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 336a7def-e80b-4f0a-b1e1-9a350e2e8ca5
📒 Files selected for processing (15)
crates/weaver-cli/src/lib.rscrates/weaver-cli/src/tests/unit/auto_start.rscrates/weaver-cli/src/transport.rscrates/weaver-plugin-rust-analyzer/src/arguments.rscrates/weaver-plugin-rust-analyzer/src/lib.rscrates/weaver-plugin-rust-analyzer/src/tests/behaviour.rscrates/weaver-plugin-rust-analyzer/src/tests/mod.rscrates/weaver-plugin-rust-analyzer/tests/features/rust_analyzer_plugin.featurecrates/weaverd/src/dispatch/act/refactor/manifests.rscrates/weaverd/src/dispatch/act/refactor/mod.rscrates/weaverd/src/dispatch/act/refactor/tests.rsdocs/execplans/5-2-3-update-rust-plugin-to-declare-rename-symbol.mddocs/roadmap.mddocs/users-guide.mddocs/weaver-design.md
Comment on lines
+266
to
+276
| let request_path = path_to_slash(file.path()); | ||
| if arguments.uri() != request_path { | ||
| return Err(PluginFailure::with_reason( | ||
| format!( | ||
| "uri argument '{}' does not match file payload '{}'", | ||
| arguments.uri(), | ||
| request_path, | ||
| ), | ||
| ReasonCode::IncompletePayload, | ||
| )); | ||
| } |
There was a problem hiding this comment.
Normalise equivalent relative URIs before comparing them.
Line 267 compares the raw uri string to path_to_slash(file.path()), which strips . components. That rejects valid requests such as ./src/main.rs and src/./main.rs, even though the payload path itself is accepted. Compare normalised relative paths on both sides, and add a regression test for a ./-prefixed path.
🛠️ Proposed fix
let request_path = path_to_slash(file.path());
- if arguments.uri() != request_path {
+ let uri_path = Path::new(arguments.uri());
+ validate_relative_path(uri_path).map_err(|error| {
+ PluginFailure::with_reason(error.to_string(), ReasonCode::IncompletePayload)
+ })?;
+ let request_uri = path_to_slash(uri_path);
+ if request_uri != request_path {
return Err(PluginFailure::with_reason(
format!(
"uri argument '{}' does not match file payload '{}'",
arguments.uri(),
request_path,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/weaver-plugin-rust-analyzer/src/lib.rs` around lines 266 - 276, The
comparison currently checks arguments.uri() directly against request_path
(computed via path_to_slash(file.path())), which rejects equivalent relative
URIs containing "./" or "src/./file" segments; normalize both sides before
comparing by resolving and collapsing "." components (and redundant separators)
so relative-equivalent paths match; update the check around request_path and
arguments.uri() (the variables and call sites using path_to_slash,
arguments.uri(), PluginFailure::with_reason, ReasonCode::IncompletePayload) to
compare their normalized forms, and add a regression test asserting a
'./'-prefixed URI (e.g., "./src/main.rs") is accepted.
Comment on lines
278
to
+286
| let modified = adapter | ||
| .rename(file, offset, &new_name) | ||
| .map_err(|error| error.to_string())?; | ||
| .rename( | ||
| file, | ||
| ByteOffset::new(arguments.offset()), | ||
| arguments.new_name(), | ||
| ) | ||
| .map_err(|error| { | ||
| PluginFailure::with_reason(error.to_string(), ReasonCode::SymbolNotFound) | ||
| })?; |
There was a problem hiding this comment.
Stop classifying every adapter failure as SymbolNotFound.
This path currently folds workspace creation failures, spawn failures, protocol failures, and timeouts into the same reason code as a genuine missing symbol. Preserve opaque adapter failures as unclassified unless the adapter can prove a real symbol-not-found condition.
🛠️ Proposed fix
let modified = adapter
.rename(
file,
ByteOffset::new(arguments.offset()),
arguments.new_name(),
)
- .map_err(|error| {
- PluginFailure::with_reason(error.to_string(), ReasonCode::SymbolNotFound)
- })?;
+ .map_err(|error| PluginFailure::plain(error.to_string()))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let modified = adapter | |
| .rename(file, offset, &new_name) | |
| .map_err(|error| error.to_string())?; | |
| .rename( | |
| file, | |
| ByteOffset::new(arguments.offset()), | |
| arguments.new_name(), | |
| ) | |
| .map_err(|error| { | |
| PluginFailure::with_reason(error.to_string(), ReasonCode::SymbolNotFound) | |
| })?; | |
| let modified = adapter | |
| .rename( | |
| file, | |
| ByteOffset::new(arguments.offset()), | |
| arguments.new_name(), | |
| ) | |
| .map_err(|error| PluginFailure::plain(error.to_string()))?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/weaver-plugin-rust-analyzer/src/lib.rs` around lines 278 - 286, The
current map_err on adapter.rename always wraps any adapter error as
PluginFailure::with_reason(..., ReasonCode::SymbolNotFound); change this to
inspect the adapter error (in the map_err closure) and only return
ReasonCode::SymbolNotFound when the adapter explicitly indicates a true
symbol-not-found variant (e.g., match on the adapter error enum variant that
represents missing symbol); for all other errors (workspace creation, spawn,
protocol, timeouts, etc.) return a generic/unclassified PluginFailure (e.g.,
PluginFailure::with_reason(error.to_string(), ReasonCode::Unclassified) or omit
the specific reason) so opaque adapter failures are not mislabelled. Ensure you
update the map_err closure around adapter.rename(...) and keep references to
PluginFailure::with_reason and ReasonCode::SymbolNotFound intact.
Comment on lines
+8
to
+13
| Status: COMPLETE | ||
|
|
||
| This document must be maintained in accordance with `AGENTS.md` at the | ||
| repository root. | ||
|
|
||
| Implementation was approved on 2026-03-07 and is now in progress. |
There was a problem hiding this comment.
Make the status block internally consistent.
Status: COMPLETE conflicts with “is now in progress”. Leave one final state here; otherwise the execution log is self-contradictory for readers and tooling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/execplans/5-2-3-update-rust-plugin-to-declare-rename-symbol.md` around
lines 8 - 13, The status block in
docs/execplans/5-2-3-update-rust-plugin-to-declare-rename-symbol.md is
inconsistent: it currently says "Status: COMPLETE" but the body says "is now in
progress"; pick and replace the status line to match the actual state (either
"Status: IN-PROGRESS" or "Status: COMPLETE") so the header and text agree—update
the single line beginning with "Status:" to reflect the chosen final state and
ensure the surrounding text (e.g., the sentence "Implementation was approved on
2026-03-07 and is now in progress.") is adjusted if you choose COMPLETE.
This was referenced Mar 11, 2026
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
Rationale
Validation plan
Next steps
Notes for reviewers
◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/ec189dd0-0c0f-433b-a913-cccf8a3ce8c7
Summary by Sourcery
Align the rust-analyzer plugin and daemon with the shared
rename-symbolcapability contract and improve CLI-daemon connection robustness.New Features:
rename-symbolrequests in the rust-analyzer plugin, validatinguri,position, andnew_name.RenameSymbolcapability, matching the rope plugin.Bug Fixes:
Enhancements:
PluginFailuretype that surfaces stableReasonCodevalues for unsupported operations, malformed payloads, and symbol-not-found conditions.rename-symbolcontract semantics and capability wiring for rust-analyzer.Documentation:
rename-symbolcapability contract for rust-analyzer in the design and user guides, and mark roadmap item 5.2.3 as complete.rename-symbol.