Implement daemon capability resolution for rename-symbol#83
Merged
Conversation
…on for rename-symbol This new execplan documents the design, constraints, tolerances, risks, implementation plan, and validation criteria for roadmap item 5.2.4, which aims to implement daemon-driven capability resolution for the `rename-symbol` refactoring operation. It covers the transition from operator-supplied providers to deterministic, language-aware provider selection, including the machine-readable routing rationale and CLI rendering requirements. The execplan serves as the approved blueprint to guide subsequent coding, testing, and documentation for the feature. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Reviewer's GuideImplements daemon-side capability resolution and machine-readable routing rationale for the Sequence diagram for daemon capability resolution in act_refactor_renamesequenceDiagram
actor User
participant CLI as weaver_cli
participant Daemon as weaverd
participant Runtime as RefactorPluginRuntime
participant Registry as PluginRegistry
participant Plugin as RefactorPlugin
participant ApplyPatch as act_apply_patch
User->>CLI: invoke act refactor [--provider] --refactoring rename --file PATH
CLI->>Daemon: CommandRequest(act, refactor, arguments)
Daemon->>Daemon: parse_refactor_args
Daemon->>Daemon: prepare_plugin_request
Daemon->>Runtime: resolve(ResolutionRequest)
Runtime->>Registry: resolve_provider(PluginRegistry, ResolutionRequest)
Registry-->>Runtime: CapabilityResolutionEnvelope
Runtime-->>Daemon: CapabilityResolutionEnvelope
Daemon->>Daemon: write_capability_resolution(stderr JSON)
Daemon-->>CLI: stderr line with CapabilityResolution JSON
alt selected_provider present
Daemon->>Runtime: execute(selected_provider, PluginRequest)
Runtime->>Plugin: run rename_symbol
Plugin-->>Runtime: PluginResponse(Diff or Error)
Runtime-->>Daemon: PluginResponse
Daemon->>ApplyPatch: act apply-patch with diff
ApplyPatch-->>Daemon: DoubleLock result
Daemon-->>CLI: final JSON status payload
CLI->>CLI: render_human_output
CLI-->>User: human or JSON output
else no selected_provider (refused)
Daemon-->>CLI: exit status 1
CLI->>CLI: render_capability_resolution or render_verification_failures
CLI-->>User: refusal message
end
Class diagram for capability_resolution_and_refactor_runtime_changesclassDiagram
class RefactorPluginRuntime {
<<trait>>
+resolve(request ResolutionRequest) Result~CapabilityResolutionEnvelope, PluginError~
+execute(provider &str, request &PluginRequest) Result~PluginResponse, PluginError~
}
class SandboxRefactorRuntime {
-registry PluginRegistry
-runner PluginRunner_SandboxExecutor_
+new(rope_executable PathBuf, rust_analyzer_executable PathBuf) Result~Self, String~
+resolve(request ResolutionRequest) Result~CapabilityResolutionEnvelope, PluginError~
+execute(provider &str, request &PluginRequest) Result~PluginResponse, PluginError~
}
class NoopRefactorRuntime {
-message String
+new(message String) Self
+resolve(request ResolutionRequest) Result~CapabilityResolutionEnvelope, PluginError~
+execute(provider &str, request &PluginRequest) Result~PluginResponse, PluginError~
}
RefactorPluginRuntime <|.. SandboxRefactorRuntime
RefactorPluginRuntime <|.. NoopRefactorRuntime
class RefactorContext {
+backends FusionBackends_SemanticBackendProvider_
+workspace_root &Path
+runtime &dyn RefactorPluginRuntime
}
class RefactorArgs {
+provider Option~String~
+refactoring String
+file String
+extra Vec~String~
}
class ResolutionRequest {
+capability CapabilityId
+target_file &Path
+explicit_provider Option~&str~
+new(capability CapabilityId, target_file &Path, explicit_provider Option~&str~) Self
}
class CapabilityResolutionEnvelope {
-status &str
-kind &str
-details CapabilityResolutionDetails
+from_details(details CapabilityResolutionDetails) Self
+details() &CapabilityResolutionDetails
}
class CapabilityResolutionDetails {
+capability CapabilityId
+language Option~String~
+requested_provider Option~String~
+selected_provider Option~String~
+selection_mode SelectionMode
+outcome ResolutionOutcome
+refusal_reason Option~RefusalReason~
+candidates Vec~CandidateEvaluation~
+selected_provider() Option~&str~
}
class CandidateEvaluation {
+provider String
+accepted bool
+reason CandidateReason
}
class SelectionMode {
<<enum>>
ExplicitProvider
Automatic
}
class ResolutionOutcome {
<<enum>>
Selected
Refused
}
class RefusalReason {
<<enum>>
UnsupportedLanguage
ProviderNotFound
ExplicitProviderMismatch
NoMatchingProvider
}
class CandidateReason {
<<enum>>
MatchedLanguageAndCapability
NotRequested
UnsupportedLanguage
NotSelectedByPolicy
ExplicitProviderMismatch
}
class CapabilityResolution_models_CLI_ {
}
class CapabilityResolution_CLI_ {
+details CapabilityResolutionDetails_CLI_
}
class CapabilityResolutionDetails_CLI_ {
+capability String
+language Option~String~
+requested_provider Option~String~
+selected_provider Option~String~
+selection_mode String
+outcome String
+refusal_reason Option~String~
+candidates Vec~CapabilityCandidate~
}
class CapabilityCandidate {
+provider String
+accepted bool
+reason String
}
CapabilityResolutionEnvelope --> CapabilityResolutionDetails
CapabilityResolutionDetails --> CandidateEvaluation
ResolutionRequest --> CapabilityResolutionEnvelope : resolve_provider
SandboxRefactorRuntime --> PluginRegistry
CapabilityResolution_models_CLI_ <|-- CapabilityResolution_CLI_
CapabilityResolution_CLI_ --> CapabilityResolutionDetails_CLI_
CapabilityResolutionDetails_CLI_ --> CapabilityCandidate
class OutputContext {
+verb String
+subcommand String
+render_human_output(data &str) Option~String~
}
class Output_rendering {
+render_human_output(context &OutputContext, data &str) Option~String~
+render_capability_resolution(payload &str) Option~String~
+parse_capability_resolution(payload &str) Option~CapabilityResolution_CLI_~
}
Output_rendering --> CapabilityResolution_CLI_
Output_rendering --> OutputContext
RefactorContext --> RefactorPluginRuntime
RefactorContext --> RefactorArgs
RefactorContext --> ResolutionRequest
RefactorContext --> CapabilityResolutionEnvelope
class Helper_functions_weaverd_ {
+prepare_plugin_request(workspace_root &Path, args &RefactorArgs) Result~(PluginRequest, CapabilityId, PathBuf), DispatchError~
+capability_from_operation(operation &str) Result~CapabilityId, DispatchError~
+write_capability_resolution(writer &mut ResponseWriter_W_, resolution &CapabilityResolutionEnvelope) Result~(), DispatchError~
}
Helper_functions_weaverd_ --> CapabilityResolutionEnvelope
Helper_functions_weaverd_ --> ResolutionRequest
Helper_functions_weaverd_ --> RefactorArgs
Helper_functions_weaverd_ --> RefactorContext
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughImplement daemon-side capability resolution for rename-symbol: infer target file language, select or deterministically refuse providers (Python → rope, Rust → rust‑analyzer) with optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "weaver-cli"
participant Daemon as "weaverd (refactor handler)"
participant Registry as "PluginRegistry"
participant Runtime as "PluginRuntime"
User->>CLI: weaver act refactor [--provider] --refactoring OP --file path.py
CLI->>Daemon: send CommandRequest
Daemon->>Daemon: parse_refactor_args()
Daemon->>Daemon: prepare_plugin_request()
Daemon->>Registry: discover candidate manifests
Registry-->>Daemon: return manifests
Daemon->>Daemon: resolve_provider() (explicit or automatic)
Note right of Daemon: emit CapabilityResolutionEnvelope (JSON) to stderr
alt provider selected
Daemon->>Runtime: execute(selected_provider, PluginRequest)
Runtime-->>Daemon: PluginResponse or Error
Daemon->>CLI: output PluginResponse (human/JSON)
else refused
Daemon->>CLI: return status=1 and refusal envelope on stderr
end
CLI->>User: render human output (includes routing summary)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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.
Add capability resolution for `act refactor` to automatically select appropriate plugin provider based on requested capability, target file language, and optional explicit provider override. - Introduce a new resolution module with detailed routing rationale including candidates evaluation and refusal reasons. - Update refactor handler to resolve provider before executing plugin. - Add human-readable rendering of capability resolution to CLI output. - Implement argument parsing and routing tests covering multiple scenarios. - Enable deterministic refusal of unsupported languages and provider mismatches. This improves user experience by removing mandatory provider flag and provides detailed routing diagnostics in daemon output. 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.
Introduced deterministic plugin selection in weaverd for the rename-symbol capability by capability, inferred file language, and optional explicit provider override. This includes structured routing rationale emitted as CapabilityResolution payloads, supporting automatic selection of rope for Python and rust-analyzer for Rust, with clear refusal paths for unsupported languages or provider mismatches. Changed CLI behavior and documentation to reflect daemon-side routing and rationale rendering in both JSON and human-readable modes. Splitted the refactor routing code into dedicated modules for clarity and line-budget compliance. Added comprehensive unit tests, request contract tests, and behavioural scenarios covering success and refusal cases. Updated user guides, roadmap, and design documents to describe and exemplify the new routing behavior and rationale formats. 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: Large Assertion Blockscrates/weaverd/src/dispatch/act/refactor/resolution_tests.rs: What lead to degradation?The test suite contains 5 assertion blocks with at least 4 assertions, threshold = 4 Why does this problem occur?Measures the number of lines with consecutive assert statements in a unit test. The more consecutive assert statements, the lower the code health. The threshold for the Rust language is 4 consecutive lines of assert statements. How to fix it?Consider to encapsulate the assertions (i.e. test criteria) in a custom assert statement. This simplifies the test by letting you communicate the test criteria in the language of your domain. We also recommend to consider the granularity of the tests; sometimes a single test tests too many things; extracting smaller tests often help to simplify the test criteria. Finally, large assertion blocks can also signal missing functionality in the code under test: instead of fetching and comparing individual properties of an object, maybe that class should encapsulate what equality means in an Equals() method? Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# large_assertion_block_example.js
test('T-1000 automatically repairs when damaged', () => {
robT1000.heal();
- assert.equal(100, robT1000.cpuCapacity);
- assert.isOk(robT1000.ramCheckPasses;
- assert.isOK(robT1000.diskAccessible);
- assert.equal(100, robT1000.vision);
- assert.equal(CONSTANTS.FUNCTIONAL, robT1000.equipment);
+ // Replace the low-level assertions with a custom assert that lets
+ // us communicate in the language of our domain. Also encapsulates
+ // the criteria so that we only have one place to change if/when
+ // more properties are added.
+ // Most test frameworks and assertion libraries have support for
+ // custom asserts.
+ assert.fullyOperational(robT1000);
} |
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: Code Duplicationcrates/weaverd/src/dispatch/act/refactor/resolution_tests.rs: What lead to degradation?The module contains 2 functions with similar structure: explicit_provider_mismatch_is_refused,unknown_explicit_provider_is_refused 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 |
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: Large Methodcrates/weaverd/src/dispatch/act/refactor/mod.rs: handle What lead to degradation?handle has 81 lines, threshold = 70 Why does this problem occur?Overly long functions make the code harder to read. The recommended maximum function length for the Rust language is 70 lines of code. Severity: Brain Method - Complex Method - Long Method. How to fix it?We recommend to be careful here -- just splitting long functions don't necessarily make the code easier to read. Instead, look for natural chunks inside the functions that expresses a specific task or concern. Often, such concerns are indicated by a Code Comment followed by an if-statement. Use the EXTRACT FUNCTION refactoring to encapsulate that concern. |
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: Code Duplicationcrates/weaverd/src/dispatch/act/refactor/behaviour.rs: What lead to degradation?The module contains 8 functions with similar structure: diff_for,given_explicit_provider_mismatch_request,given_unsupported_language_request,given_valid_rope_request and 4 more functions 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.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…dd test assertion helpers - Extracted request preparation logic into `prepare_plugin_request` for clarity and reuse. - Introduced `standard_rename_args` and `configure_request` helpers in tests to reduce repetition. - Consolidated and simplified diff formatting functions. - Added assertion helper functions in resolution tests to improve readability and reduce duplication. - Minor improvements in logging and error handling organization in refactor handling. 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.
![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 8 issues, and left some high level feedback:
- The CLI’s
parse_capability_resolutionandrender_capability_resolutionhard‑code the string"CapabilityResolution"while the daemon exposesCAPABILITY_RESOLUTION_TYPE; wiring the CLI to use that shared constant (or at least a single source within the CLI crate) would reduce the risk of drift if the envelope type name ever changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The CLI’s `parse_capability_resolution` and `render_capability_resolution` hard‑code the string `"CapabilityResolution"` while the daemon exposes `CAPABILITY_RESOLUTION_TYPE`; wiring the CLI to use that shared constant (or at least a single source within the CLI crate) would reduce the risk of drift if the envelope type name ever changes.
## Individual Comments
### Comment 1
<location path="crates/weaver-cli/src/output/models.rs" line_range="62-71" />
<code_context>
+/// Parsed capability-resolution payload emitted by daemon routing.
+#[derive(Debug, Deserialize)]
+pub(crate) struct CapabilityResolution {
+ /// Structured routing details.
+ pub(crate) details: CapabilityResolutionDetails,
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid double-parsing the same JSON payload when extracting capability resolution envelopes
`parse_capability_resolution` currently deserializes once into `CapabilityResolution` and then again into `TypedEnvelope` just to read `type`. Consider either using a single envelope struct that includes `status`, `type`, and `details`, or deserializing into `TypedEnvelope` first and only decoding `details` when the type matches. This removes redundant parsing/allocations and aligns the model with the wire format.
Suggested implementation:
```rust
/// Parsed capability-resolution payload emitted by daemon routing.
///
/// This models the full envelope as emitted on the wire so that callers
/// can access `status`, `type`, and `details` from a single deserialization.
#[derive(Debug, Deserialize)]
pub(crate) struct CapabilityResolution {
/// Resolution status (e.g. "ok", "error").
pub(crate) status: String,
/// Payload type discriminator.
#[serde(rename = "type")]
pub(crate) r#type: String,
/// Structured routing details.
pub(crate) details: CapabilityResolutionDetails,
}
```
To fully eliminate the double-parsing described in your review comment, you should also:
1. Update `parse_capability_resolution` (or any equivalent function) to:
- Deserialize the JSON payload directly into `CapabilityResolution` (once).
- Use `capability_resolution.r#type` instead of deserializing into `TypedEnvelope` just to read `type`.
- Use `capability_resolution.details` instead of re-parsing `details`.
2. Remove the secondary `serde_json::from_str`/`from_value` call that currently parses into `TypedEnvelope` for capability resolution messages.
3. If there is any existing `TypedEnvelope`-based logic relying on a different shape, either:
- Migrate it to consume `CapabilityResolution`, or
- Make sure `CapabilityResolution`’s fields/serde attributes match the wire format (`status`, `type`, and `details` keys) used by the daemon routing payloads.
</issue_to_address>
### Comment 2
<location path="crates/weaverd/src/dispatch/act/refactor/tests.rs" line_range="76-85" />
<code_context>
- result,
- Err(DispatchError::InvalidArguments { .. })
- ));
+fn automatic_selection(provider: &str, language: &str) -> CapabilityResolutionEnvelope {
+ CapabilityResolutionEnvelope::from_details(CapabilityResolutionDetails {
+ capability: weaver_plugins::CapabilityId::RenameSymbol,
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `handle` behaviour when capability resolution fails or `resolve` returns an error.
Currently the tests cover plugin execution failures and assert that capability-resolution payloads appear in the streams. Please also cover the `handle` behaviour when:
1. `RefactorPluginRuntime::resolve` returns an error (e.g. `PluginError::Manifest { .. }`), asserting stderr includes `"act refactor failed"` and exit status is 1.
2. `resolve` succeeds but returns a refused outcome (`UnsupportedLanguage`, `ExplicitProviderMismatch`, or `NoMatchingProvider`) with `selected_provider: None`, asserting the command exits with status 1 and `execute` is not called.
These will exercise the routing failure paths at the handler level.
Suggested implementation:
```rust
fn automatic_selection(provider: &str, language: &str) -> CapabilityResolutionEnvelope {
CapabilityResolutionEnvelope::from_details(CapabilityResolutionDetails {
capability: weaver_plugins::CapabilityId::RenameSymbol,
language: Some(String::from(language)),
requested_provider: None,
selected_provider: Some(String::from(provider)),
selection_mode: SelectionMode::Automatic,
outcome: ResolutionOutcome::Selected,
refusal_reason: None,
candidates: vec![CandidateEvaluation {
provider: String::from(provider),
accepted: true,
reason: super::resolution::CandidateReason::MatchedLanguageAndCapability,
}],
})
}
#[test]
fn handle_exits_with_error_when_resolution_fails() {
use super::*;
use weaver_plugins::PluginError;
// Arrange: a runtime where `resolve` fails with a manifest error.
//
// The concrete mock/test runtime used in this module should expose:
// - a way to configure `resolve` to return an `Err(PluginError)`
// - a way to observe `execute` was not called
//
// Here we assume a `TestRuntime` helper already used by the other tests in
// this file, plus a `run_handle_with_runtime` helper that:
// * wires a runtime into `handle`
// * returns `(ExitStatus, stdout_contents, stderr_contents)`.
let mut runtime = TestRuntime::new();
runtime
.expect_resolve()
.returning(|_capability, _language| {
Err(PluginError::Manifest {
message: "bad manifest".into(),
})
});
let (status, _stdout, stderr) = run_handle_with_runtime(runtime.clone());
// Assert: handler reports a failure and includes the high‑level message.
assert_eq!(status.code(), Some(1));
assert!(
stderr.contains("act refactor failed"),
"stderr should contain the generic failure message, got: {stderr}"
);
// `handle` must not attempt to execute any plugin when resolution fails.
assert_eq!(
runtime.execute_call_count(),
0,
"`execute` should not be called when capability resolution fails"
);
}
#[test]
fn handle_exits_with_error_when_resolution_refused_without_provider() {
use super::*;
use weaver_plugins::resolution::{
CandidateEvaluation, CapabilityResolutionDetails, CapabilityResolutionEnvelope,
ResolutionOutcome, SelectionMode,
};
// Arrange: a runtime where `resolve` succeeds but refuses to select a provider
// (e.g. unsupported language, explicit provider mismatch, or no matching provider).
let mut runtime = TestRuntime::new();
let refused_envelope = CapabilityResolutionEnvelope::from_details(
CapabilityResolutionDetails {
capability: weaver_plugins::CapabilityId::RenameSymbol,
language: Some("unknown-lang".to_string()),
requested_provider: None,
selected_provider: None,
selection_mode: SelectionMode::Automatic,
outcome: ResolutionOutcome::Refused,
refusal_reason: Some(
super::resolution::RefusalReason::UnsupportedLanguage,
),
candidates: Vec::<CandidateEvaluation>::new(),
},
);
runtime
.expect_resolve()
.returning(move |_capability, _language| Ok(refused_envelope.clone()));
let (status, _stdout, stderr) = run_handle_with_runtime(runtime.clone());
// Assert: handler reports a failure at the command level.
assert_eq!(status.code(), Some(1));
assert!(
stderr.contains("act refactor failed"),
"stderr should contain the generic failure message, got: {stderr}"
);
// When there is no selected provider, the handler must not call `execute`.
assert_eq!(
runtime.execute_call_count(),
0,
"`execute` should not be called when resolution is refused with no provider"
);
}
```
The above edit assumes several existing test helpers and types that are likely already present in `crates/weaverd/src/dispatch/act/refactor/tests.rs` alongside `automatic_selection`:
1. A `TestRuntime` test double with:
- `fn new() -> Self`
- `fn expect_resolve(&mut self) -> &mut Self` (or similar mockall-style API) to configure `resolve` behaviour.
- `fn execute_call_count(&self) -> usize` (or equivalent way to assert `execute` was not invoked).
- `Clone` implemented (or adjust the tests to borrow it mutably instead of cloning).
2. A `run_handle_with_runtime` helper that:
- Accepts a `TestRuntime` (or `impl RefactorPluginRuntime`) and wires it into the `handle` entrypoint for the `act refactor` subcommand.
- Returns `(std::process::ExitStatus, String, String)` representing the exit status, captured stdout, and captured stderr of `handle`.
3. The `PluginError::Manifest { message: String }` variant in `weaver_plugins::PluginError`, and `RefusalReason::UnsupportedLanguage` under `super::resolution`. If the enum names or variants differ, update the two tests accordingly.
4. If your existing tests import items differently (e.g. via `use super::fixtures::*;` or a different path for `CapabilityResolutionEnvelope` and friends), align the `use` statements in the new tests with the existing patterns.
If your mocking/story for `RefactorPluginRuntime` differs (e.g. you use `mockall` with a generated `MockRefactorPluginRuntime` rather than a `TestRuntime` helper), adapt the `expect_resolve` / `execute_call_count` usage to match your mock type while keeping the same behavioural assertions:
- For the first test, `resolve` must return `Err(PluginError::Manifest { .. })`.
- For the second test, `resolve` must return an `Ok(CapabilityResolutionEnvelope)` with `outcome` == `Refused` and `selected_provider == None`.
Finally, ensure `automatic_selection`’s return type and the `use` paths for `CapabilityResolutionEnvelope`, `CapabilityResolutionDetails`, `CandidateEvaluation`, `ResolutionOutcome`, and `SelectionMode` match how they are already imported in this test module (you may be re-exporting them via `super::resolution::*;` etc.); adjust the `use` statements and type paths accordingly if compilation fails.
</issue_to_address>
### Comment 3
<location path="crates/weaverd/src/dispatch/act/refactor/resolution.rs" line_range="141" />
<code_context>
+ explicit_provider: Option<&'a str>,
+}
+
+#[derive(Debug)]
+struct RefusalContext {
+ capability: CapabilityId,
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining refusal construction into a single `refused` helper and removing `RefusalContext`/`refused_details` to simplify the error path.
You can drop `RefusalContext` + `refused_details` and build the refusal envelope directly. This removes one struct and one function without changing behavior, and makes the refusal path easier to follow.
For example:
```rust
fn refused(
capability: CapabilityId,
language: Option<SupportedLanguage>,
requested_provider: Option<String>,
selection_mode: SelectionMode,
refusal_reason: RefusalReason,
candidates: Vec<CandidateEvaluation>,
) -> CapabilityResolutionEnvelope {
CapabilityResolutionEnvelope::from_details(CapabilityResolutionDetails {
capability,
language: language.map(|l| l.as_str().to_owned()),
requested_provider,
selected_provider: None,
selection_mode,
outcome: ResolutionOutcome::Refused,
refusal_reason: Some(refusal_reason),
candidates,
})
}
```
Call sites become more direct, e.g.:
```rust
let Some(language) = language else {
return refused(
request.capability,
None,
requested_provider,
selection_mode,
RefusalReason::UnsupportedLanguage,
candidates
.iter()
.map(|manifest| CandidateEvaluation {
provider: manifest.name().to_owned(),
accepted: false,
reason: CandidateReason::UnsupportedLanguage,
})
.collect(),
);
};
```
And in `resolve_automatic_provider`:
```rust
if matching.is_empty() {
return refused(
capability,
Some(language),
None,
SelectionMode::Automatic,
RefusalReason::NoMatchingProvider,
candidates
.iter()
.map(|manifest| CandidateEvaluation {
provider: manifest.name().to_owned(),
accepted: false,
reason: CandidateReason::UnsupportedLanguage,
})
.collect(),
);
}
```
Similarly in `resolve_explicit_provider` you can replace the two `refused_details(RefusalContext { ... })` branches with direct `refused(...)` calls using the already-built `evaluations`. This keeps all the refusal data in one place per call and removes the extra hop through `RefusalContext`/`refused_details`.
</issue_to_address>
### Comment 4
<location path="crates/weaverd/src/dispatch/act/refactor/behaviour.rs" line_range="270" />
<code_context>
+ format_diff(relative_path, &format!("diff --git a/{0}", relative_path))
+}
+
+fn original_content_for(relative_path: &str) -> &'static str {
+ if relative_path.ends_with(".py") {
+ "old_name = 1\nprint(old_name)\n"
</code_context>
<issue_to_address>
**suggestion (review_instructions):** `original_content_for` (and `updated_content_for`) use a 3-branch `if/else if/else` chain instead of extracting the multi-branch conditional into a predicate function as required.
The functions `original_content_for` and `updated_content_for` each contain an `if/else if/else` chain with three branches (`.py`, `.rs`, and default). The review guideline asks to move conditionals with more than two branches into a predicate function.
Consider extracting a small helper that classifies the file kind (for example, by extension) and then matching on that enum, or otherwise encapsulating the multi-branch logic into a dedicated predicate so these helpers only handle the mapping from that predicate result to content strings.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Move conditionals with >2 branches into a predicate function.
</details>
</issue_to_address>
### Comment 5
<location path="docs/execplans/5-2-4-daemon-capability-resolution-for-rename-symbol.md" line_range="57" />
<code_context>
+ `crates/weaver-plugins/src/capability/` is already complete. This roadmap
+ item resolves providers in the daemon; it must not redefine the contract
+ schema introduced by 5.2.1.
+2. Keep the CLI command shape stable. `--refactoring rename`, `offset`, and
+ `new_name` remain the operator-facing inputs. `--provider` may become
+ optional, but it must not be removed in 5.2.4 because 5.2.6 is the roadmap
</code_context>
<issue_to_address>
**suggestion (review_instructions):** CLI is introduced as an acronym without being defined on first use.
To follow the documentation guidance, expand CLI the first time it appears, for example: "Keep the command-line interface (CLI) command shape stable." Subsequent uses can use CLI alone.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 6
<location path="docs/execplans/5-2-4-daemon-capability-resolution-for-rename-symbol.md" line_range="42" />
<code_context>
+ Double-Lock path.
+3. Running the command against an unsupported language or an incompatible
+ explicit provider exits non-zero with a deterministic structured refusal.
+4. JSON mode exposes a machine-readable capability-resolution payload that
+ includes at least the requested capability, inferred language, selected or
+ refused provider, policy source, and candidate evaluation reasons.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** JSON is used here without being defined on first use in this document.
Consider expanding JSON the first time it appears, for example: "JavaScript Object Notation (JSON) mode exposes a machine-readable capability-resolution payload…" After that, JSON alone is fine.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 7
<location path="docs/execplans/5-2-4-daemon-capability-resolution-for-rename-symbol.md" line_range="114" />
<code_context>
+ Likelihood: high Mitigation: introduce a dedicated resolution type and split
+ "resolve" from "execute" inside `crates/weaverd/src/dispatch/act/refactor/`.
+
+- Risk: machine-readable rationale may tempt an implementation that dumps raw
+ JSON onto human stderr output. Severity: high Likelihood: medium Mitigation:
+ treat rationale as a first-class payload shape, then update the CLI renderer
</code_context>
<issue_to_address>
**suggestion (review_instructions):** JSON is referenced later in this bullet without being defined earlier in the document.
Once JSON is defined on its first occurrence, this later reference will implicitly be covered. At present, the document never expands JSON, so adding an earlier definition (for example in the "Progress" or "Observable success" sections) would resolve this.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 8
<location path="docs/execplans/5-2-4-daemon-capability-resolution-for-rename-symbol.md" line_range="81" />
<code_context>
+ is suitable.
+10. Any design decision taken during implementation must be recorded in
+ `docs/weaver-design.md`, not only in this ExecPlan.
+11. The final implementation must be validated with unit tests and behavioural
+ tests, and the full workspace gates `make check-fmt`, `make lint`, and
+ `make test` must pass. Because this item also updates Markdown documents,
</code_context>
<issue_to_address>
**suggestion (review_instructions):** LSP is introduced later in the document without being defined on first use.
Later, the document refers to "the LSP capability matrix" without expanding LSP. To comply with the style guidance, introduce it as "Language Server Protocol (LSP)" at its first mention.
<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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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/arguments.rs`:
- Around line 67-84: The parser currently accepts the next token as a value even
when it's another flag (e.g., "--file --provider"), so update the parsing logic
in arguments.rs (the block that sets builder.provider, builder.refactoring, and
builder.file using iter.next()) to detect and reject flag-like tokens: after
calling iter.next() check whether the token starts with "-" or matches any
recognised flag names and, if so, return
DispatchError::invalid_arguments("--<flag> requires a value"); otherwise accept
it as the value. Use the same DispatchError::invalid_arguments path already
referenced and apply this check for the provider, refactoring, and file branches
that populate builder.provider, builder.refactoring, and builder.file.
In `@crates/weaverd/src/dispatch/act/refactor/behaviour.rs`:
- Around line 305-309: given_workspace_file writes the default target
(notes.txt) before
configure_request()/given_explicit_provider_mismatch_request() can change the
request to a different file (notes.py), causing then_target_file_unchanged to
read a file that was never created; fix by moving the workspace seeding to after
the request is configured: update given_workspace_file (and the duplicate at the
other location) to call world.target_file() only after
configure_request()/given_explicit_provider_mismatch_request() has run, or
change the test setup so configure_request() is invoked before world.write_file
in the setup sequence, ensuring the written fixture matches the final request
(refer to functions given_workspace_file, configure_request,
given_explicit_provider_mismatch_request, then_target_file_unchanged).
- Around line 48-77: StubRuntime::resolve currently ignores the incoming
ResolutionRequest and uses hard-coded/placeholder values; change it to read the
request fields (e.g., ResolutionRequest.language,
ResolutionRequest.requested_provider/override, and selection mode) and build the
CapabilityResolutionEnvelope accordingly instead of discarding _request; for
automatic routing branches (RoutingMode::AutomaticPython / AutomaticRust) derive
the selected provider and language from the request (use request.language to
pick language-specific targets and request.requested_provider or selection mode
to decide explicit-vs-automatic behavior), and for refused_resolution branches
populate RefusedResolution.language, requested_provider, SelectionMode and
candidates using values from the request; update uses of selected_resolution,
refused_resolution, and rejected_candidate to pass real language/provider names
from the request rather than the current placeholders.
In `@crates/weaverd/src/dispatch/act/refactor/contract_tests.rs`:
- Around line 129-138: The test currently assigns the dispatch call to a
discarded binding (`let _result = handle(...).expect("dispatch result")`) which
violates the workspace lint and may mask failures; update the call to consume
the Result instead by removing the underscore discard — either call
handle(...).expect("dispatch result") directly or bind to a meaningful variable
(e.g., `result`) so the Result is used, and if appropriate add assertions on
`result`; target the `handle` invocation and the `RefactorContext`/`backends`
usage in this test to make the change.
In `@crates/weaverd/src/dispatch/act/refactor/mod.rs`:
- Around line 323-330: capability_from_operation currently only maps
"rename-symbol" to CapabilityId::RenameSymbol and returns a generic
invalid_arguments error for anything else; update this by either (a) extending
the match to include additional refactor operation strings mapped to their
corresponding CapabilityId variants, or (b) make the error message from
DispatchError::invalid_arguments more descriptive and include a TODO comment
listing planned/allowed operations—refer to the function
capability_from_operation, the enum variant CapabilityId::RenameSymbol, and the
error constructor DispatchError::invalid_arguments when making the change.
- Around line 332-338: The unit test in tests.rs is asserting stdout but the
implementation intentionally writes the CapabilityResolutionEnvelope to stderr
via write_capability_resolution (which calls writer.write_stderr); update the
failing test to read the test harness' captured stderr (not stdout) and assert
it contains the serialized CapabilityResolutionEnvelope JSON (or the expected
substring) emitted by write_capability_resolution so the test matches the
intended design of emitting the capability-resolution on stderr.
In `@crates/weaverd/src/dispatch/act/refactor/resolution.rs`:
- Around line 365-371: preferred_provider currently returns the placeholder
string "typescript-unimplemented" for SupportedLanguage::TypeScript; add a
visible TODO comment or link to a tracking issue inside the preferred_provider
function (or adjacent doc comment) noting that TypeScript provider is
unimplemented and referencing the issue/PR ID so it won't be overlooked—update
the function block containing preferred_provider and the
SupportedLanguage::TypeScript arm to include the TODO/issue link.
- Around line 293-297: Document the invariant that `matching` is non-empty
before selecting a provider: add a short comment above the `let selected_name =
matching.iter().min_by_key(|manifest| provider_rank(manifest.name(),
language)).map(|manifest| manifest.name()).unwrap_or("unreachable");` line
stating that an earlier guard guarantees `matching` cannot be empty (so the
fallback is unreachable), and keep the current code (or switch to
`unwrap_or_else` with the same comment) instead of using `.expect()` since
`.expect()` is forbidden outside tests.
In `@crates/weaverd/src/dispatch/act/refactor/tests.rs`:
- Around line 244-274: The test uses a hardcoded Path::new("/tmp/workspace") for
RefactorContext.workspace_root which is platform-dependent; change it to use the
TempDir provided by the test (socket_dir) so the workspace is temporary and
consistent—replace workspace_root: Path::new("/tmp/workspace") with
workspace_root: socket_dir.path() (or socket_dir.path().as_ref() if needed)
inside the handle_returns_error_for_unsupported_refactoring test when
constructing RefactorContext.
In `@docs/execplans/5-2-4-daemon-capability-resolution-for-rename-symbol.md`:
- Around line 34-50: The top-level ordered list items in the document (the six
numbered success criteria beginning with "Running the Python flow..." through
"reflect the shipped behaviour...") must be converted to first-level dash
bullets to match repo Markdown style; update every first-level numeric list in
this file (including analogous lists in success criteria, constraints, and
milestone task sections) to use "-" instead of "1.", "2.", etc., preserving the
exact text and indentation of any nested lists and keeping references like
`rope`, `rust-analyzer`, "Double-Lock", and the JSON/human-readable mode lines
unchanged.
In `@docs/users-guide.md`:
- Around line 703-709: Add short Markdown captions immediately above the two act
refactor tables: place a one-line caption above the table that lists
`--provider`, `--refactoring`, `--file`, and `KEY=VALUE` (the act refactor flags
table) and another one-line caption above the second table at 728-734; keep
captions concise (e.g., "Table: act refactor flags" and "Table: <brief
descriptor>") so they satisfy the repo rule that every table/diagram must have a
caption and do not change table contents or formatting.
🪄 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: 6200f51f-38cc-4190-a89d-c48379bb5414
📒 Files selected for processing (14)
crates/weaver-cli/src/output/mod.rscrates/weaver-cli/src/output/models.rscrates/weaverd/src/dispatch/act/refactor/arguments.rscrates/weaverd/src/dispatch/act/refactor/behaviour.rscrates/weaverd/src/dispatch/act/refactor/contract_tests.rscrates/weaverd/src/dispatch/act/refactor/mod.rscrates/weaverd/src/dispatch/act/refactor/resolution.rscrates/weaverd/src/dispatch/act/refactor/resolution_tests.rscrates/weaverd/src/dispatch/act/refactor/tests.rscrates/weaverd/tests/features/refactor.featuredocs/execplans/5-2-4-daemon-capability-resolution-for-rename-symbol.mddocs/roadmap.mddocs/users-guide.mddocs/weaver-design.md
- Add stable envelope type for capability-resolution payloads in weaver-cli - Replace legacy envelope parsing with direct CapabilityResolution deserialization - Improve argument validation for --provider, --refactoring, and --file flags - Introduce file classification for refactor behaviour tests - Refactor resolution logic to use new RefusalDetails struct - Add deterministic refusal handling for unsupported languages and providers - Expand tests to cover resolution success, refusal, and error cases - Update documentation to describe capability resolution, CLI flags, and routing policy This change enables the daemon to select or refuse refactoring providers based on capability and language, providing structured feedback to clients and preserving existing command-line interface stability and sync execution. 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: 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/act/refactor/behaviour.rs (1)
1-411: 🛠️ Refactor suggestion | 🟠 MajorSplit this module to satisfy the repository file-size limit.
Extract helper-heavy sections (
diff_for,malformed_diff_for, file-content/classification helpers, request builders) into a collocated test helper module and keep scenario steps in this file.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/weaverd/src/dispatch/act/refactor/behaviour.rs` around lines 1 - 411, Split out helper-heavy code from this test module into a collocated helper module (e.g. create a sibling module refactor_helpers and include it with `mod refactor_helpers; use refactor_helpers::*;`), moving the pure helpers and data helpers into it: format_diff, diff_for, malformed_diff_for, original_content_for, updated_content_for, classify_file, FileKind, command_request, standard_rename_args, configure_request, selected_resolution, refused_resolution, rejected_candidate, build_backends and any small pure-data structs/impls that are not scenario steps; keep the scenario step functions, RefactorWorld struct and StubRuntime/behaviour wiring in the original file and update all references to those moved symbols to import from the new module so the file shrinks below the 400-line limit.
♻️ Duplicate comments (3)
docs/execplans/5-2-4-daemon-capability-resolution-for-rename-symbol.md (1)
228-233:⚠️ Potential issue | 🟡 MinorReplace first-level ordered lists with dash bullets throughout this document.
Rewrite the first-level numbered items as
-bullets in every listed section to match repository Markdown rules and keep list style consistent with the rest of the file.Patch pattern
-1. There is no language inference step for `act refactor`. -2. There is no policy object that turns `(capability, language, explicit - provider?)` into a deterministic provider choice or refusal. -3. There is no structured rationale payload for that decision. -4. The CLI does not yet know how to render such a payload cleanly. +- There is no language inference step for `act refactor`. +- There is no policy object that turns `(capability, language, explicit + provider?)` into a deterministic provider choice or refusal. +- There is no structured rationale payload for that decision. +- The CLI does not yet know how to render such a payload cleanly.As per coding guidelines: "
**/*.md: Use dashes (-) for list bullets in Markdown."Also applies to: 261-267, 277-287, 337-341, 347-353, 368-374, 378-383, 392-400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/5-2-4-daemon-capability-resolution-for-rename-symbol.md` around lines 228 - 233, Replace all first-level ordered (numbered) lists with dash bullets in this Markdown file: change the numbered items such as "1. There is no language inference step for `act refactor`.", "2. There is no policy object...", "3. There is no structured rationale payload...", and "4. The CLI does not yet know how to render such a payload cleanly." into "- " bullet lines, and apply the same replacement for the other specified list blocks (the sections corresponding to the comment ranges referencing lines 261-267, 277-287, 337-341, 347-353, 368-374, 378-383, 392-400); ensure only top-level list markers are converted (do not change nested lists or indentation), preserving the existing text, spacing, and code spans like `act refactor`.crates/weaverd/src/dispatch/act/refactor/behaviour.rs (2)
323-339:⚠️ Potential issue | 🟠 MajorUse language-specific
--filefixtures in automatic-routing scenarios.Set the rope scenario file to
.pyand the rust-analyzer scenario file to.rs. Keep.txtfor unsupported-language paths only. Enforce language inference coverage through realistic request inputs.Suggested patch
fn given_valid_rope_request(world: &mut RefactorWorld) { configure_request( world, - standard_rename_args("notes.txt"), + standard_rename_args("notes.py"), RoutingMode::AutomaticPython, ); } @@ fn given_valid_rust_request(world: &mut RefactorWorld) { configure_request( world, - standard_rename_args("notes.txt"), + standard_rename_args("notes.rs"), RoutingMode::AutomaticRust, ); }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/act/refactor/behaviour.rs` around lines 323 - 339, Replace the generic ".txt" fixture with language-specific filenames in the automatic-routing test setups: in given_valid_rope_request call standard_rename_args with "notes.py" (while still using RoutingMode::AutomaticPython) and in given_valid_rust_request call standard_rename_args with "notes.rs" (while still using RoutingMode::AutomaticRust); update the configure_request invocations accordingly (functions: given_valid_rope_request, given_valid_rust_request, configure_request, standard_rename_args, RoutingMode::AutomaticPython, RoutingMode::AutomaticRust) and leave ".txt" fixtures only for unsupported-language tests.
48-77:⚠️ Potential issue | 🟠 MajorDrive
StubRuntime::resolvefrom the incomingResolutionRequest.Stop discarding the request. Populate language, provider override, selection mode, and refusal candidate details from the incoming payload so these behavioural tests fail when daemon request wiring regresses.
Suggested patch
- fn resolve( - &self, - _request: ResolutionRequest<'_>, - ) -> Result<CapabilityResolutionEnvelope, PluginError> { + fn resolve( + &self, + request: ResolutionRequest<'_>, + ) -> Result<CapabilityResolutionEnvelope, PluginError> { Ok(match self.routing { - RoutingMode::AutomaticPython => selected_resolution("python", "rope"), - RoutingMode::AutomaticRust => selected_resolution("rust", "rust-analyzer"), + RoutingMode::AutomaticPython => { + selected_resolution(request.language.unwrap_or("python"), "rope") + } + RoutingMode::AutomaticRust => { + selected_resolution(request.language.unwrap_or("rust"), "rust-analyzer") + } RoutingMode::UnsupportedLanguage => refused_resolution(RefusedResolution { - language: None, - requested_provider: None, + language: request.language, + requested_provider: request.requested_provider, selection_mode: SelectionMode::Automatic, refusal_reason: RefusalReason::UnsupportedLanguage, candidates: vec![ @@ RoutingMode::ExplicitProviderMismatch => refused_resolution(RefusedResolution { - language: Some("python"), - requested_provider: Some("rust-analyzer"), + language: request.language, + requested_provider: request.requested_provider, selection_mode: SelectionMode::ExplicitProvider, refusal_reason: RefusalReason::ExplicitProviderMismatch, candidates: vec![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/act/refactor/behaviour.rs` around lines 48 - 77, The resolve implementation currently ignores the incoming ResolutionRequest; update StubRuntime::resolve to extract values from the request and populate the returned CapabilityResolutionEnvelope accordingly: use request.language (Option<&str>) for RefusedResolution.language, request.provider_override/requested_provider for RefusedResolution.requested_provider, and map request.selection_mode into SelectionMode in the refusal; when building candidates (via rejected_candidate) derive CandidateReason from the request (e.g., NotRequested vs ExplicitProviderMismatch vs UnsupportedLanguage) and include the request-derived details in RefusedResolution.refusal_reason (RefusalReason::ExplicitProviderMismatch or UnsupportedLanguage) and selection_mode; keep the existing RoutingMode logic to choose between selected_resolution and refused_resolution but fill all RefusedResolution fields from the ResolutionRequest instead of hardcoding them.
🤖 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-cli/src/output/models.rs`:
- Around line 248-271: Add a unit test that exercises the branch in
parse_capability_resolution where the JSON "type" discriminator does not match
CAPABILITY_RESOLUTION_TYPE: create a payload with "type": "SomethingElse" and
minimal valid "details", then assert that
parse_capability_resolution(payload).is_none(); reference the existing test
function name style (e.g., parse_capability_resolution_rejects_mismatched_type)
so it sits alongside the current parses_capability_resolution test and validates
the rejection behavior of parse_capability_resolution and the
CAPABILITY_RESOLUTION_TYPE check.
- Around line 73-74: Replace the forbidden #[allow(dead_code)] applied to the
status field with a narrowly scoped expect attribute: change it to
#[expect(dead_code, reason = "explain why status exists but is unused (e.g.,
reserved for future serialization/deserialization or externally constructed)")]
on the status field so the lint is acknowledged with a clear reason; locate the
field named status in the models (status: String) and update its attribute
accordingly.
In `@crates/weaverd/src/dispatch/act/refactor/arguments.rs`:
- Around line 103-135: Add a new unit test in the existing tests mod to cover
the case where a flag is supplied where a value is expected: create a test named
flag_as_value_is_rejected that calls parse_refactor_args with args including
"--file" followed by "--provider" and assert that it returns an Err matching
crate::dispatch::errors::DispatchError::InvalidArguments; place it alongside the
existing provider_is_optional and missing_file_is_rejected tests so it verifies
the flag-as-value rejection behavior of parse_refactor_args.
- Around line 67-101: The three branches parsing "--provider", "--refactoring",
and "--file" duplicate the same logic checking next value and ensuring it
doesn't start with "--"; extract a small helper (e.g. parse_flag_value or
expect_next_value) that takes the flag name and the iterator, returns
Result<String, DispatchError>, and encapsulates the ok_or_else and
starts_with("--") check and error construction; then replace the duplicated
blocks so builder.provider, builder.refactoring, and builder.file are set from
the helper's returned String (use clone or move as appropriate).
---
Outside diff comments:
In `@crates/weaverd/src/dispatch/act/refactor/behaviour.rs`:
- Around line 1-411: Split out helper-heavy code from this test module into a
collocated helper module (e.g. create a sibling module refactor_helpers and
include it with `mod refactor_helpers; use refactor_helpers::*;`), moving the
pure helpers and data helpers into it: format_diff, diff_for,
malformed_diff_for, original_content_for, updated_content_for, classify_file,
FileKind, command_request, standard_rename_args, configure_request,
selected_resolution, refused_resolution, rejected_candidate, build_backends and
any small pure-data structs/impls that are not scenario steps; keep the scenario
step functions, RefactorWorld struct and StubRuntime/behaviour wiring in the
original file and update all references to those moved symbols to import from
the new module so the file shrinks below the 400-line limit.
---
Duplicate comments:
In `@crates/weaverd/src/dispatch/act/refactor/behaviour.rs`:
- Around line 323-339: Replace the generic ".txt" fixture with language-specific
filenames in the automatic-routing test setups: in given_valid_rope_request call
standard_rename_args with "notes.py" (while still using
RoutingMode::AutomaticPython) and in given_valid_rust_request call
standard_rename_args with "notes.rs" (while still using
RoutingMode::AutomaticRust); update the configure_request invocations
accordingly (functions: given_valid_rope_request, given_valid_rust_request,
configure_request, standard_rename_args, RoutingMode::AutomaticPython,
RoutingMode::AutomaticRust) and leave ".txt" fixtures only for
unsupported-language tests.
- Around line 48-77: The resolve implementation currently ignores the incoming
ResolutionRequest; update StubRuntime::resolve to extract values from the
request and populate the returned CapabilityResolutionEnvelope accordingly: use
request.language (Option<&str>) for RefusedResolution.language,
request.provider_override/requested_provider for
RefusedResolution.requested_provider, and map request.selection_mode into
SelectionMode in the refusal; when building candidates (via rejected_candidate)
derive CandidateReason from the request (e.g., NotRequested vs
ExplicitProviderMismatch vs UnsupportedLanguage) and include the request-derived
details in RefusedResolution.refusal_reason
(RefusalReason::ExplicitProviderMismatch or UnsupportedLanguage) and
selection_mode; keep the existing RoutingMode logic to choose between
selected_resolution and refused_resolution but fill all RefusedResolution fields
from the ResolutionRequest instead of hardcoding them.
In `@docs/execplans/5-2-4-daemon-capability-resolution-for-rename-symbol.md`:
- Around line 228-233: Replace all first-level ordered (numbered) lists with
dash bullets in this Markdown file: change the numbered items such as "1. There
is no language inference step for `act refactor`.", "2. There is no policy
object...", "3. There is no structured rationale payload...", and "4. The CLI
does not yet know how to render such a payload cleanly." into "- " bullet lines,
and apply the same replacement for the other specified list blocks (the sections
corresponding to the comment ranges referencing lines 261-267, 277-287, 337-341,
347-353, 368-374, 378-383, 392-400); ensure only top-level list markers are
converted (do not change nested lists or indentation), preserving the existing
text, spacing, and code spans like `act refactor`.
🪄 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: b4c1c3ef-c04a-486f-aa6b-6b3c5e667394
📒 Files selected for processing (9)
crates/weaver-cli/src/output/models.rscrates/weaverd/src/dispatch/act/refactor/arguments.rscrates/weaverd/src/dispatch/act/refactor/behaviour.rscrates/weaverd/src/dispatch/act/refactor/contract_tests.rscrates/weaverd/src/dispatch/act/refactor/mod.rscrates/weaverd/src/dispatch/act/refactor/resolution.rscrates/weaverd/src/dispatch/act/refactor/tests.rsdocs/execplans/5-2-4-daemon-capability-resolution-for-rename-symbol.mddocs/users-guide.md
Owner
Author
|
@coderabbitai have the following now been resolved? |
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/weaverd/src/dispatch/act/refactor/tests.rs Comment on file let workspace = TempDir::new().expect("workspace");
let file_path = workspace.path().join("notes.txt");
std::fs::write(&file_path, "hello\n").expect("write");
std::fs::write(workspace.path().join("notes.py"), "hello\n").expect("write");❌ New issue: Code Duplication |
leynos
commented
Mar 19, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
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/act/refactor/mod.rs (1)
344-401:⚠️ Potential issue | 🟡 MinorReduce file to 400 lines maximum by extracting plugin response handling functions into a sibling module.
This file is 401 lines and exceeds the 400-line guideline. Extract
handle_plugin_responseandforward_diff_to_apply_patchintocrates/weaverd/src/dispatch/act/refactor/response_handling.rsand import them inmod.rs.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/weaverd/src/dispatch/act/refactor/mod.rs` around lines 344 - 401, Move the two functions handle_plugin_response and forward_diff_to_apply_patch into a new sibling module named response_handling (create response_handling.rs), keep their signatures and return types exactly the same, and ensure they import the same types (Write, ResponseWriter, FusionBackends<SemanticBackendProvider>, Path, CommandRequest, CommandDescriptor and apply_patch::handle) so compilation doesn't break; then in the original mod.rs replace the function bodies with a mod declaration (mod response_handling;) and add use response_handling::{handle_plugin_response, forward_diff_to_apply_patch}; making the functions visible to the rest of the module without changing call sites or behavior.
🤖 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/resolution.rs`:
- Around line 265-269: The current branch sets refusal_reason to
RefusalReason::ExplicitProviderMismatch for both an existing provider that lacks
the required capability and one that exists but doesn't support the inferred
language; add a new enum variant RefusalReason::ProviderLacksCapability (or
similarly named) to distinguish these, then change the logic that computes
refusal_reason to inspect the resolved provider (via registry.get(provider_name)
-> provider) and choose ProviderLacksCapability when provider exists but does
not advertise the required capability (e.g.,
provider.supports_capability("rename-symbol") or provider.capabilities()),
choose ExplicitProviderMismatch only when the provider exists and has the
capability but not the inferred language (e.g.,
!provider.supports_language(inferred_language)), and keep ProviderNotFound when
registry.get returns None; if you purposely want the previous conflation,
instead add a brief comment at the top of this block explaining that
ExplicitProviderMismatch intentionally covers both cases.
---
Outside diff comments:
In `@crates/weaverd/src/dispatch/act/refactor/mod.rs`:
- Around line 344-401: Move the two functions handle_plugin_response and
forward_diff_to_apply_patch into a new sibling module named response_handling
(create response_handling.rs), keep their signatures and return types exactly
the same, and ensure they import the same types (Write, ResponseWriter,
FusionBackends<SemanticBackendProvider>, Path, CommandRequest, CommandDescriptor
and apply_patch::handle) so compilation doesn't break; then in the original
mod.rs replace the function bodies with a mod declaration (mod
response_handling;) and add use response_handling::{handle_plugin_response,
forward_diff_to_apply_patch}; making the functions visible to the rest of the
module without changing call sites or behavior.
🪄 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: e85281a7-39da-4d46-9034-b8765d19b426
📒 Files selected for processing (4)
crates/weaverd/src/dispatch/act/refactor/contract_tests.rscrates/weaverd/src/dispatch/act/refactor/mod.rscrates/weaverd/src/dispatch/act/refactor/resolution.rscrates/weaverd/src/dispatch/act/refactor/tests.rs
…o separate module - Moved response handling and diff forwarding logic from mod.rs to a new response_handling.rs file - Updated mod.rs to use the new response_handling module - Added new refusal reason ProviderLacksCapability in resolution.rs for better provider capability checks This change improves code organization and separation of concerns in the act refactor dispatch logic. 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: 1
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/act/refactor/mod.rs (1)
157-196:⚠️ Potential issue | 🟠 MajorUse the resolved workspace path when constructing the rename URI.
prepare_plugin_requestalready resolvesfile_path, but the rename-symbol mapping still receives the workspace-relative CLI string. For a normal input such assrc/lib.rs, that yieldsfile:///src/lib.rsinstead of the real workspace file, so rename requests target the wrong document in every non-root workspace. Passfile_pathinto the mapper and build the URI from that path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/act/refactor/mod.rs` around lines 157 - 196, The rename mapping currently uses the CLI string in args.file, producing incorrect URIs; update prepare_plugin_request to pass the resolved file_path (the PathBuf returned by resolve_file) into the rename mapper instead of args.file, and ensure the mapper (apply_rename_symbol_mapping) constructs the file:// URI from that resolved Path (or accepts a Path/PathBuf and converts to a file URI internally) so rename-symbol uses the workspace-resolved document URI; change the call in prepare_plugin_request from apply_rename_symbol_mapping(&mut plugin_args, &args.file) to use the resolved path and adjust apply_rename_symbol_mapping signature/implementation if needed to accept and convert the Path/PathBuf into the proper file URI.
🤖 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 223-226: The current call to
context.backends.ensure_started(BackendKind::Semantic) is happening too early
(in mod.rs) before capability resolution and plugin diff creation, causing
backend startup errors to surface instead of routing/plugin errors; move the
ensure_started(BackendKind::Semantic) invocation out of the early path and
instead call it immediately before the "act apply-patch" hand-off where the
response_handling.rs code actually needs backends. Concretely, remove or delay
the ensure_started call in the refactor flow in mod.rs and add a new
ensure_started(BackendKind::Semantic) call right before the code that invokes
the apply-patch handoff (the point that transitions to act apply-patch /
response_handling.rs), so refused requests and plugin failures resolve first and
the backend is only started when the diff branch is certain.
---
Outside diff comments:
In `@crates/weaverd/src/dispatch/act/refactor/mod.rs`:
- Around line 157-196: The rename mapping currently uses the CLI string in
args.file, producing incorrect URIs; update prepare_plugin_request to pass the
resolved file_path (the PathBuf returned by resolve_file) into the rename mapper
instead of args.file, and ensure the mapper (apply_rename_symbol_mapping)
constructs the file:// URI from that resolved Path (or accepts a Path/PathBuf
and converts to a file URI internally) so rename-symbol uses the
workspace-resolved document URI; change the call in prepare_plugin_request from
apply_rename_symbol_mapping(&mut plugin_args, &args.file) to use the resolved
path and adjust apply_rename_symbol_mapping signature/implementation if needed
to accept and convert the Path/PathBuf into the proper file URI.
🪄 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: cbe755fe-d8c1-4b43-b709-790285835cfe
📒 Files selected for processing (3)
crates/weaverd/src/dispatch/act/refactor/mod.rscrates/weaverd/src/dispatch/act/refactor/resolution.rscrates/weaverd/src/dispatch/act/refactor/response_handling.rs
Added #![allow(dead_code)] and #![allow(clippy::duplicate_mod)] attributes to suppress warnings in refactor_helpers.rs, improving code cleanliness during development. 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/act/refactor/refactor_helpers.rs`:
- Around line 3-4: Remove the blanket file-wide attributes
`#![allow(dead_code)]` and `#![allow(clippy::duplicate_mod)]` from
refactor_helpers.rs; instead, eliminate or refactor unused/duplicated items so
the lints are satisfied, and where suppression is absolutely necessary apply
narrowly-scoped annotations (e.g. add `#[expect(dead_code, reason = "...")]` to
the specific function/module or use `#[cfg(test)]` to restrict test-only code)
on the precise items that require it (refer to the top-of-file attributes and
any offending functions/modules inside refactor_helpers.rs such as test helpers
or duplicated submodules) so the file no longer uses blanket allows.
🪄 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: 0b6cfa42-116a-444f-86a6-acf4b529c014
📒 Files selected for processing (1)
crates/weaverd/src/dispatch/act/refactor/refactor_helpers.rs
…ugin execution Previously, the semantic backend was started early in the refactor handle function. This change moves the startup to occur only after plugin execution succeeds, ensuring the backend is running before handling plugin responses. Additionally, added clippy allow annotations for shared test helper modules used across multiple test files and annotated test helpers with reasons for dead code suppression. 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: 4
♻️ Duplicate comments (1)
crates/weaverd/src/dispatch/act/refactor/behaviour.rs (1)
252-256: 🧹 Nitpick | 🔵 TrivialWorkspace fixture seeding still runs before request configuration.
The past review noted that
given_workspace_fileruns beforegiven_explicit_provider_mismatch_requestin the Gherkin scenario, so it writes based on the defaultnotes.txttarget. Althoughprepare_routed_fixturelater overwrites the patch target, this ordering is fragile and confusing.Move the file seeding into
configure_requestor reorder the step definitions to ensure the target file is known before writing fixtures.Proposed fix
fn configure_request(world: &mut RefactorWorld, args: Vec<String>, routing_mode: RoutingMode) { - world.request = command_request(args); - world.routing_mode = routing_mode; + configure_request(&mut world.request, args); + world.routing_mode = routing_mode; + let target_file = world.target_file(); + let target_path = Path::new(&target_file); + if !world.path(&target_file).exists() { + world.write_file(&target_file, original_content_for(target_path)); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/act/refactor/behaviour.rs` around lines 252 - 256, The fixture seeding in given_workspace_file races with request configuration because it runs before given_explicit_provider_mismatch_request, causing the wrong target (default notes.txt) to be written; fix by moving the file-write into configure_request (or otherwise after prepare_routed_fixture) so the target is known: locate the given_workspace_file function and remove or change its write-to-disk behavior, then add the write using world.write_file(&target_file, original_content_for(Path::new(&target_file))) inside configure_request (or call a helper from configure_request) after prepare_routed_fixture has set world.target_file(); alternatively reorder the step definitions so prepare_routed_fixture / given_explicit_provider_mismatch_request run before given_workspace_file to ensure the correct target is used.
🤖 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/behaviour.rs`:
- Around line 17-24: Replace the forbidden #[allow(...)] attribute on the
refactor_helpers module with #[expect(...)] so the same lints
(clippy::duplicate_mod) and the existing reason string "Shared test helpers
loaded by multiple test modules" are preserved; specifically change the
attribute that currently precedes the mod refactor_helpers declaration to
#[expect(...)] while keeping the #[path = "refactor_helpers.rs"] mod
refactor_helpers; and the use refactor_helpers::* import unchanged.
In `@crates/weaverd/src/dispatch/act/refactor/contract_tests.rs`:
- Around line 9-14: Replace the forbidden #[allow(...)] attribute on the test
helper module with #[expect(...)]: change the attribute that currently reads
#[allow(clippy::duplicate_mod, reason = "Shared test helpers loaded by multiple
test modules")] so it becomes #[expect(clippy::duplicate_mod, reason = "Shared
test helpers loaded by multiple test modules")] applied to the same mod
refactor_helpers declaration in contract_tests.rs; keep the same lint name and
reason string so the intent and documentation remain identical.
In `@crates/weaverd/src/dispatch/act/refactor/refactor_helpers.rs`:
- Around line 19-27: Replace the forbidden #[allow(dead_code, reason = "...")]
annotations with narrowly scoped #[expect(dead_code, reason = "...")] on all
test/helper items in this file — e.g., change the attribute on RefusedResolution
(and the other helper structs/enums/functions referenced in the review) so each
uses #[expect(dead_code, reason = "Test helper struct reserved for future use")]
(or the same reason text used previously for that item); ensure you update every
occurrence mentioned in the comment so no #[allow(...)] remain.
In `@crates/weaverd/src/dispatch/act/refactor/tests.rs`:
- Around line 7-12: Replace the forbidden #[allow(...)] on the module
declaration for mod refactor_helpers with #[expect(...)] preserving the same
lint name and reason; specifically change the attribute that currently reads
#[allow(clippy::duplicate_mod, reason = "Shared test helpers loaded by multiple
test modules")] to #[expect(clippy::duplicate_mod, reason = "Shared test helpers
loaded by multiple test modules")] on the mod refactor_helpers declaration so
the lint intent is kept but uses the approved attribute.
---
Duplicate comments:
In `@crates/weaverd/src/dispatch/act/refactor/behaviour.rs`:
- Around line 252-256: The fixture seeding in given_workspace_file races with
request configuration because it runs before
given_explicit_provider_mismatch_request, causing the wrong target (default
notes.txt) to be written; fix by moving the file-write into configure_request
(or otherwise after prepare_routed_fixture) so the target is known: locate the
given_workspace_file function and remove or change its write-to-disk behavior,
then add the write using world.write_file(&target_file,
original_content_for(Path::new(&target_file))) inside configure_request (or call
a helper from configure_request) after prepare_routed_fixture has set
world.target_file(); alternatively reorder the step definitions so
prepare_routed_fixture / given_explicit_provider_mismatch_request run before
given_workspace_file to ensure the correct target is 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: ce5a8457-71a3-48d2-b1d6-5d4a624a871e
📒 Files selected for processing (5)
crates/weaverd/src/dispatch/act/refactor/behaviour.rscrates/weaverd/src/dispatch/act/refactor/contract_tests.rscrates/weaverd/src/dispatch/act/refactor/mod.rscrates/weaverd/src/dispatch/act/refactor/refactor_helpers.rscrates/weaverd/src/dispatch/act/refactor/tests.rs
…D step for clarity The `given_workspace_file` function no longer creates a file as the setup is now handled by `prepare_routed_fixture()` in subsequent steps. This change improves BDD readability by clarifying that this step performs no action. 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: 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/act/refactor/behaviour.rs`:
- Around line 252-256: Replace the forbidden #[allow(unused_variables)] on the
given_workspace_file function with a narrowly scoped #[expect(...)] that
documents why the variable is intentionally unused (e.g.,
#[expect(unused_variables, reason = "BDD step exists for readability; file
creation happens in prepare_routed_fixture()")]); update the attribute on the
given_workspace_file function signature so the expectation name and concise
reason explain the intentional unused parameter and satisfy linting rules.
🪄 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: ccf1223b-f03a-45c6-a56f-0013e117c327
📒 Files selected for processing (1)
crates/weaverd/src/dispatch/act/refactor/behaviour.rs
…xpect(dead_code)] for unused test helpers The #[allow(dead_code)] attributes on test helper structs and functions have been changed to #[expect(dead_code)] for better clarity on their intended reserved use. No functional changes were made. 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: 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/act/refactor/refactor_helpers.rs`:
- Around line 67-140: The annotated test helpers are actually used by tests
(behaviour.rs, resolution.rs) so remove the #[expect(dead_code, ...)] attributes
from configure_request, selected_resolution, refused_resolution,
rejected_candidate, routed_diff_for, and routed_malformed_diff_for; leave
#[expect(...)] only on truly unused helpers such as format_diff,
routed_format_diff, classify_file, content_table and other genuinely unused
items beyond line 140. Ensure you edit the attribute lines above each listed
function (e.g., configure_request, selected_resolution, refused_resolution,
rejected_candidate, routed_diff_for, routed_malformed_diff_for) to delete the
expect(...) annotation so the linter no longer flags unfulfilled expectations.
🪄 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: d0cc9190-3844-4fdd-85d2-3acc4bc21652
📒 Files selected for processing (1)
crates/weaverd/src/dispatch/act/refactor/refactor_helpers.rs
…elper functions - Removed `#[expect(dead_code, reason = "Test helper function reserved for future use")]` annotations from multiple helper functions in `refactor_helpers.rs` as they were unnecessary. - Added a more descriptive `#[expect(unused_variables, reason = "BDD step exists for readability; file creation happens in prepare_routed_fixture()")]` to `given_workspace_file` in `behaviour.rs` to clarify its usage for BDD readability purposes. These changes clarify intent by cleaning up redundant annotations and improving annotation messages for better code 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: 2
♻️ Duplicate comments (1)
crates/weaverd/src/dispatch/act/refactor/behaviour.rs (1)
17-24:⚠️ Potential issue | 🟠 MajorReplace
#[allow(...)]with#[expect(...)].The coding guidelines explicitly state "
#[allow]is forbidden." Use#[expect]instead.Proposed fix
-#[allow( +#[expect( clippy::duplicate_mod, reason = "Shared test helpers loaded by multiple test modules" )] #[path = "refactor_helpers.rs"] mod refactor_helpers;🤖 Prompt for AI Agents
In crates/weaverd/src/dispatch/act/refactor/behaviour.rs at lines 17-20, change the #[allow(clippy::duplicate_mod, reason = "...")] attribute to #[expect(clippy::duplicate_mod, reason = "Shared test helpers loaded by multiple test modules")]. The reason string may remain unchanged. Run `make lint` to verify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/act/refactor/behaviour.rs` around lines 17 - 24, Replace the forbidden attribute on the module declaration: change the #[allow(clippy::duplicate_mod, reason = "Shared test helpers loaded by multiple test modules")] applied to mod refactor_helpers to #[expect(clippy::duplicate_mod, reason = "Shared test helpers loaded by multiple test modules")] so the attribute uses expect instead of allow for the refactor_helpers module import; run the linter (make lint) to confirm no warnings remain.
🤖 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/refactor_helpers.rs`:
- Around line 19-107: The build is failing due to dual-compilation (Clippy sees
these helpers as unused) caused by a standalone module inclusion; remove the
extra module declaration so these helpers (RefusedResolution,
SelectedResolution, standard_rename_args, configure_request,
selected_resolution, refused_resolution, rejected_candidate) are only included
via the #[path] usage in behaviour.rs—locate and delete the standalone `mod
refactor_helpers;` declaration that causes the duplicate compile unit.
- Around line 109-136: Remove the unnecessary #[expect(dead_code)] attributes
from the helper functions that are actually used: delete the attribute
annotations on format_diff, routed_patch_path, routed_format_diff,
routed_diff_for, routed_malformed_diff_for (and any other helper functions in
this file flagged in the review) so the compiler no longer reports stale
expectations; locate the attributes by the function names (format_diff,
routed_patch_path, routed_format_diff, routed_diff_for,
routed_malformed_diff_for) and simply remove those #[expect(dead_code, ...)]
lines, then run the test/lint suite to verify no remaining expect(dead_code)
expectations fail.
---
Duplicate comments:
In `@crates/weaverd/src/dispatch/act/refactor/behaviour.rs`:
- Around line 17-24: Replace the forbidden attribute on the module declaration:
change the #[allow(clippy::duplicate_mod, reason = "Shared test helpers loaded
by multiple test modules")] applied to mod refactor_helpers to
#[expect(clippy::duplicate_mod, reason = "Shared test helpers loaded by multiple
test modules")] so the attribute uses expect instead of allow for the
refactor_helpers module import; run the linter (make lint) to confirm no
warnings remain.
🪄 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: 574d0fb4-7d48-4141-a321-55142da7b1f9
📒 Files selected for processing (2)
crates/weaverd/src/dispatch/act/refactor/behaviour.rscrates/weaverd/src/dispatch/act/refactor/refactor_helpers.rs
… clean up dead code Replaced `#[allow(clippy::duplicate_mod)]` directives with `#[expect(clippy::duplicate_mod)]` to indicate expected duplication in test helpers. Added `#![allow(dead_code)]` for unused functions in refactor_helpers.rs and removed multiple `#[expect(dead_code, reason = ...)]` annotations by cleaning up dead code markers. Overall cleanup improves lint management and test helper annotations. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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/act/refactor/refactor_helpers.rs`:
- Line 6: Remove the file-wide #![allow(dead_code)] attribute at the top of the
file, then audit each item (functions, structs, impls) in refactor_helpers.rs to
determine which are actually unused; for any intentionally retained but unused
items, annotate them individually with #[expect(dead_code, reason = "brief
reason why retained")] directly on the item (e.g., on the specific fn or struct
declarations) and remove the blanket attribute, then run make lint to confirm
there are no remaining forbidden #[allow] uses or unfulfilled expectations.
🪄 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: adca4699-d904-4adc-b295-6b1020c7049b
📒 Files selected for processing (4)
crates/weaverd/src/dispatch/act/refactor/behaviour.rscrates/weaverd/src/dispatch/act/refactor/contract_tests.rscrates/weaverd/src/dispatch/act/refactor/refactor_helpers.rscrates/weaverd/src/dispatch/act/refactor/tests.rs
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
Validation
Documentation impact
Notes
📎 Task: https://www.devboxer.com/task/cb90af9d-b33a-4592-9962-b3ae321d07b9