Refactor plugin failures and file URI normalization; update tests#80
Merged
Refactor plugin failures and file URI normalization; update tests#80
Conversation
…odes - Introduce `PluginFailure` struct encapsulating error message and optional `ReasonCode`. - Provide constructors for plain and reason-coded failures. - Add conversion helper `failure_response` to create protocol-compliant failure responses. - Refactor rust-analyzer adapter to use `PluginFailure` for error handling and enrich diagnostic info. - Normalize plugin request URIs to uniform 'file://' scheme and reject relative URIs. - Update tests and features to handle new failure reason codes and URI format. This change enables richer, stable failure reporting in the Rust analyzer plugin. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Reviewer's GuideRefactors the rust-analyzer plugin to route all failures through a new structured failure module, introduces URI normalization/validation for file:// requests, updates weaverd to always send file:// URIs, and aligns tests/docs with the new URI and failure semantics. Sequence diagram for rename-symbol flow with file URI normalization and structured failuressequenceDiagram
actor User
participant WeaverdRefactor as WeaverdRefactorDispatcher
participant RustPlugin as RustAnalyzerPlugin
participant Adapter as RustAnalyzerLspAdapter
User->>WeaverdRefactor: act refactor rename-symbol(file)
WeaverdRefactor->>WeaverdRefactor: to_file_uri(path)
WeaverdRefactor->>RustPlugin: PluginRequest{ uri = file://path, file_payload }
RustPlugin->>RustPlugin: execute_rename(request)
RustPlugin->>RustPlugin: normalize_request_uri(arguments.uri)
alt URI not file:// or invalid path
RustPlugin->>RustPlugin: PluginFailure::with_reason(message, IncompletePayload)
RustPlugin->>RustPlugin: failure_response(failure)
RustPlugin-->>WeaverdRefactor: PluginResponse::failure(reason_code = IncompletePayload)
WeaverdRefactor-->>User: Error response
else URI valid and matches payload path
RustPlugin->>Adapter: rename_symbol(path, ByteOffset, new_name)
alt Adapter returns error (e.g. symbol not found)
RustPlugin->>RustPlugin: PluginFailure::plain(error.to_string())
RustPlugin->>RustPlugin: failure_response(failure)
RustPlugin-->>WeaverdRefactor: PluginResponse::failure(no reason_code)
WeaverdRefactor-->>User: Error response
else Adapter returns modified content
RustPlugin->>RustPlugin: build_search_replace_patch
RustPlugin-->>WeaverdRefactor: PluginResponse::success(Diff)
WeaverdRefactor-->>User: Refactor succeeded
end
end
Class diagram for PluginFailure, failure_response, and URI normalizationclassDiagram
class PluginFailure {
-String message
-Option_ReasonCode reason_code
+plain(message: String) PluginFailure
+with_reason(message: String, reason: ReasonCode) PluginFailure
+message() String
+reason_code() Option_ReasonCode
}
class ReasonCode {
}
class PluginResponse {
+failure(diagnostics: Vec_PluginDiagnostic_) PluginResponse
}
class PluginDiagnostic {
+new(severity: DiagnosticSeverity, message: String) PluginDiagnostic
+with_reason_code(reason_code: ReasonCode) PluginDiagnostic
}
class DiagnosticSeverity {
}
class FailureModule {
+failure_response(failure: PluginFailure) PluginResponse
}
class RustAnalyzerAdapterError {
<<enum>>
InvalidPath(message: String)
OtherVariants
}
class UriNormalization {
+normalize_request_uri(uri: String) Result_String_RustAnalyzerAdapterError_
}
class PathUtils {
+path_to_slash(path: Path) String
+validate_relative_path(path: Path) Result_Void_RustAnalyzerAdapterError_
}
class WeaverdRefactorDispatch {
+to_file_uri(path: String) String
+apply_rename_symbol_mapping(plugin_args: Map_String_Value_, file: String)
}
PluginFailure --> ReasonCode : uses
FailureModule --> PluginFailure : parameter
FailureModule --> PluginResponse : builds
FailureModule --> PluginDiagnostic : builds
PluginDiagnostic --> DiagnosticSeverity : uses
PluginDiagnostic --> ReasonCode : optional
UriNormalization --> RustAnalyzerAdapterError : error
UriNormalization --> PathUtils : uses
WeaverdRefactorDispatch --> UriNormalization : via plugin request
WeaverdRefactorDispatch --> WeaverdRefactorDispatch : to_file_uri used when building uri
UriNormalization ..> PluginFailure : mapped to IncompletePayload
PathUtils ..> RustAnalyzerAdapterError : may return
note for FailureModule "Represents contents of failure.rs: PluginFailure and failure_response"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughRefactor failure handling into a new crate-private Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Weaverd
participant Plugin
participant Adapter
rect rgba(200,220,255,0.5)
Client->>Weaverd: Request rename (path)
Weaverd->>Weaverd: to_file_uri(path) -> "file://..."
Weaverd->>Plugin: PluginRequest { uri: "file://..." }
Plugin->>Plugin: normalize_request_uri(uri)
alt invalid URI
Plugin->>Plugin: PluginFailure::plain(...)
Plugin->>Weaverd: failure_response(PluginFailure)
else valid URI
Plugin->>Adapter: rename(FilePayload{path}, offset, new_name)
Adapter-->>Plugin: Result (changed / unchanged / error)
Plugin->>Plugin: map result -> PluginResponse (attach ReasonCode if needed)
Plugin-->>Weaverd: PluginResponse
end
Weaverd-->>Client: Final result / diagnostic
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
![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: a0d33b9f76
ℹ️ 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: 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/weaver-plugin-rust-analyzer/src/failure.rs`:
- Around line 9-13: Implement std::error::Error for PluginFailure by deriving or
implementing Error (prefer using thiserror::Error) on the PluginFailure struct
so it becomes a proper error type callers can use with `?` and error utilities;
update the struct definition for PluginFailure to derive Error (and
Debug/Display via thiserror) and remove the manual Display implementation if
present, and ensure the ReasonCode enum (if used for error details) implements
or derives Display/Debug as required for the derived Error implementation to
compile.
In `@crates/weaver-plugin-rust-analyzer/src/lib.rs`:
- Around line 344-353: normalize_request_uri currently strips "file://" naively;
instead parse the URI with the url crate: call Url::parse(uri), ensure scheme ==
"file" and convert to a filesystem path via Url::to_file_path() (or extract and
percent-decode the path component if needed), map parse/validation failures into
RustAnalyzerAdapterError::InvalidPath, then pass the resulting Path (or PathBuf)
to validate_relative_path and path_to_slash as before; reference
normalize_request_uri, RustAnalyzerAdapterError, validate_relative_path, and
path_to_slash when making the change.
🪄 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: da221aec-950a-433b-913e-7a77f9330389
📒 Files selected for processing (10)
crates/weaver-plugin-rust-analyzer/src/failure.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/mod.rscrates/weaverd/src/dispatch/act/refactor/tests.rsdocs/execplans/2-2-3-top-level-version-output.mddocs/execplans/5-2-3-update-rust-plugin-to-declare-rename-symbol.mddocs/users-guide.md
…sing - Use `url::Url` to parse and validate file:// URIs instead of manual string stripping - Reject URIs with authorities or invalid schemes explicitly - Normalize and strip root directory from file paths safely - Update related tests to use valid file:/// URIs - Replace manual Display impl on PluginFailure with thiserror::Error derive - Use Url for file URI creation in weaverd refactor module These changes improve correctness and robustness around URI handling in the rust-analyzer plugin, ensuring only valid local file URIs without authority components are accepted and normalized consistently. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
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/weaver-plugin-rust-analyzer/src/tests/mod.rs (1)
1-420: 🧹 Nitpick | 🔵 TrivialFile length at guideline boundary.
At 420 lines, this file slightly exceeds the 400-line guideline. The current structure is acceptable, but decompose into separate test modules (e.g.,
argument_validation.rs,dispatch_layer.rs) if further tests are added.🤖 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 1 - 420, This file exceeds the 400-line guideline; split tests into focused modules to keep each file small. Move the argument-validation related tests and helpers (functions and fixtures like rename_arguments, request_with_args, all mutate helpers, and the rename_argument_validation test) into a new module/file (e.g., argument_validation) and move the stdin/stdout dispatch layer tests and helpers (valid_request_json, dispatch_stdin, run_with_adapter_dispatch_layer, failure_responses_include_reason_codes, request_with_path, and related cases) into a separate dispatch_layer module/file; retain shared mocks (mock! Adapter, adapter_returning, adapter_unused, adapter_returning_with_path) either by placing them in a common test utils module or re-exporting them so both new files can use them, and update mod declarations (mod behaviour) and test imports accordingly so tests like rename_non_mutating_or_error_returns_failure and rename_rejects_empty_or_curdir_path remain in the most relevant module.
🤖 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/act/refactor/mod.rs`:
- Around line 328-334: Change to_file_uri to return Result<String,
url::ParseError> instead of unwrapping: parse the base with
Url::parse("?file:///")? (or Url::parse("file:///")?) and use
url.path_segments_mut().expect("file URL cannot be a
base").extend(path.split('/')) so each component is percent-encoded safely, then
Ok(url.to_string()); remove all .expect()/.unwrap() uses. Update the call site
in apply_rename_symbol_mapping to propagate or map the url::ParseError into the
existing DispatchError (e.g., using ? or .map_err(|e| DispatchError::...)) so
errors are handled rather than panicking.
In `@docs/execplans/2-2-3-top-level-version-output.md`:
- Around line 184-186: The example text currently uses an absolute,
workstation-specific path (`/home/user/project/Cargo.toml`); change that to a
repo-relative reference such as "the workspace Cargo.toml (repository root)" or
"Cargo.toml in the workspace root" so the sentence reads like: The workspace
version is `0.1.0` (set in the workspace Cargo.toml) — keep the rest of the
sentence and ensure no machine-specific paths remain.
- Around line 503-514: Add a short caption line immediately before the markdown
table that begins with the header row "| File | Change | Lines before | Lines
after |" (e.g., "Table 1. File change summary.") so the table is explicitly
captioned per documentation guidelines; update the doc content near that table
(the markdown block in docs/execplans/2-2-3-top-level-version-output.md that
contains the listed file-change rows) to insert the caption as a plain paragraph
placed directly above the table header.
- Around line 409-415: The documentation stages are missing Markdown validation
and formatting steps; update the plan (Stage H / the
docs/execplans/2-2-3-top-level-version-output.md and docs/roadmap.md entries for
2.2.3) to run make markdownlint and make fmt as part of the verification/quality
gate steps so Markdown regressions are caught; add explicit steps that invoke
"make markdownlint" and "make fmt" (and ensure they run alongside the existing
Rust quality gates) and mark those checklist items as done for the 2.2.3 roadmap
entries.
---
Outside diff comments:
In `@crates/weaver-plugin-rust-analyzer/src/tests/mod.rs`:
- Around line 1-420: This file exceeds the 400-line guideline; split tests into
focused modules to keep each file small. Move the argument-validation related
tests and helpers (functions and fixtures like rename_arguments,
request_with_args, all mutate helpers, and the rename_argument_validation test)
into a new module/file (e.g., argument_validation) and move the stdin/stdout
dispatch layer tests and helpers (valid_request_json, dispatch_stdin,
run_with_adapter_dispatch_layer, failure_responses_include_reason_codes,
request_with_path, and related cases) into a separate dispatch_layer
module/file; retain shared mocks (mock! Adapter, adapter_returning,
adapter_unused, adapter_returning_with_path) either by placing them in a common
test utils module or re-exporting them so both new files can use them, and
update mod declarations (mod behaviour) and test imports accordingly so tests
like rename_non_mutating_or_error_returns_failure and
rename_rejects_empty_or_curdir_path remain in the most relevant module.
🪄 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: 76a0dd0d-6bc4-4d25-88ff-074f4733ef57
📒 Files selected for processing (7)
crates/weaver-plugin-rust-analyzer/src/failure.rscrates/weaver-plugin-rust-analyzer/src/lib.rscrates/weaver-plugin-rust-analyzer/src/tests/behaviour.rscrates/weaver-plugin-rust-analyzer/src/tests/mod.rscrates/weaverd/src/dispatch/act/refactor/mod.rscrates/weaverd/src/dispatch/act/refactor/tests.rsdocs/execplans/2-2-3-top-level-version-output.md
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1937 🏁 Script executed: Length of output: 1457
Overall comments
Individual comments
|
… dispatch layer tests - Introduce extensive unit tests for argument validation in the rust-analyzer plugin - Add stdin/stdout dispatch layer tests to ensure robustness of plugin request handling - Refactor test modules to separate support, argument validation, and dispatch tests - Enhance URI handling in rename-symbol mapping with error propagation - Update documentation with markdownlint and formatting improvements This change improves reliability by validating plugin request arguments and verifying dispatch behavior under various input scenarios. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/tests/argument_validation.rs`:
- Around line 65-97: Add a test case covering the omitted new_name field by
defining a remover function (e.g., remove_new_name(arguments: &mut
HashMap<String, serde_json::Value>) that calls arguments.remove("new_name")) and
adding the rstest entry #[case::missing_new_name(remove_new_name as fn(&mut _),
Some("new_name"))] into the rename_argument_validation cases table (alongside
other #[case::...] entries); this will exercise the branch that should return
ReasonCode::IncompletePayload when new_name is absent—add the remover function
near the other helper mutators and run the unit tests.
In `@crates/weaver-plugin-rust-analyzer/src/tests/support.rs`:
- Around line 84-93: The helper request_with_path currently only updates the
FilePayload path but leaves arguments["uri"] hard-coded to file:///src/main.rs;
update request_with_path (and the PluginRequest::with_arguments invocation) to
derive the "uri" argument from the given path (e.g. build a file:// URI from the
path string) so the payload PathBuf and arguments["uri"] are consistent for the
"rename-symbol" request and FilePayload; alternatively change the signature to
accept both path and uri and pass both through to FilePayload and arguments to
avoid mismatch.
In `@docs/execplans/2-2-3-top-level-version-output.md`:
- Around line 427-439: Update the Markdown fenced code blocks that are marked
with ```sh to ```bash for any blocks that use the Bash-specific `set -o
pipefail` construct; specifically change the three occurrences of the ```sh
fence surrounding the blocks that include `set -o pipefail` (the two "After
making edits"/"After making all edits" blocks and the third block at lines
476-477) so the fence identifier is `bash` instead of `sh`.
In `@docs/roadmap.md`:
- Around line 176-177: Remove the redundant sub-task "- [x] Validate the
documentation updates with `make markdownlint` and `make fmt` alongside the Rust
quality gates" from the roadmap so acceptance criteria remain the single source
of truth; update the checklist to omit that procedural validation item and keep
only the acceptance criteria that list the required make targets (`make
check-fmt`, `make markdownlint`, `make fmt`, `make lint`, `make test`) to
express the delivered capability.
🪄 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: 4154fef9-7f55-424a-94e0-8c72d8cc9216
📒 Files selected for processing (7)
crates/weaver-plugin-rust-analyzer/src/tests/argument_validation.rscrates/weaver-plugin-rust-analyzer/src/tests/dispatch_layer.rscrates/weaver-plugin-rust-analyzer/src/tests/mod.rscrates/weaver-plugin-rust-analyzer/src/tests/support.rscrates/weaverd/src/dispatch/act/refactor/mod.rsdocs/execplans/2-2-3-top-level-version-output.mddocs/roadmap.md
…st helpers - Added remove_new_name helper and corresponding test case to argument_validation.rs - Enhanced request_with_path to include URI argument formatted as file URI - Added file_uri_for_path helper creating proper file URIs with url crate - Minor fixes in test support code for clearer argument handling Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/execplans/2-2-3-top-level-version-output.md (1)
423-439:⚠️ Potential issue | 🟡 MinorReplace the workstation-specific workspace path.
Remove
/home/user/project. State that the commands run from the workspace
root, so the execplan remains portable across checkouts.🛠️ Proposed fix
-All commands run from workspace root `/home/user/project`. +All commands run from the workspace root.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/2-2-3-top-level-version-output.md` around lines 423 - 439, Remove the hard-coded workstation path `/home/user/project` and replace it with a generic reference to the workspace root in the two command blocks; update the surrounding text to read something like "All commands run from the workspace root." and ensure both Stage A and Stage B-H examples no longer mention the absolute path so the execplan is portable across checkouts (look for the two occurrences of the path in the command header lines above the Stage A and Stage B-H blocks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/execplans/2-2-3-top-level-version-output.md`:
- Around line 423-439: Remove the hard-coded workstation path
`/home/user/project` and replace it with a generic reference to the workspace
root in the two command blocks; update the surrounding text to read something
like "All commands run from the workspace root." and ensure both Stage A and
Stage B-H examples no longer mention the absolute path so the execplan is
portable across checkouts (look for the two occurrences of the path in the
command header lines above the Stage A and Stage B-H blocks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e23a2eec-51cc-4369-b625-a849be497163
📒 Files selected for processing (4)
crates/weaver-plugin-rust-analyzer/src/tests/argument_validation.rscrates/weaver-plugin-rust-analyzer/src/tests/support.rsdocs/execplans/2-2-3-top-level-version-output.mddocs/roadmap.md
This was referenced Apr 18, 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
📎 Task: https://www.devboxer.com/task/99d3abee-f949-400f-bd69-cc2a3ee3cbcc