Add rope actuator plugin with runtime wiring, e2e tests, and docs#59
Merged
Add rope actuator plugin with runtime wiring, e2e tests, and docs#59
Conversation
Add a detailed living ExecPlan document for implementing the first actuator plugin using rope for Python refactoring. This plan outlines purpose, constraints, tolerances, risks, progress, decisions, and detailed stages for development, testing, and documentation updates related to the rope plugin integration in the refactor action. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Reviewer's GuideImplements the first rope-based refactor actuator as a standalone plugin crate and wires Sequence diagram for act refactor rope flow through Double-LocksequenceDiagram
actor User
participant WeaverCLI as weaver_cli
participant Weaverd as weaverd_DomainRouter
participant Refactor as act_refactor_handler
participant Runtime as RefactorPluginRuntime
participant Runner as PluginRunner
participant RopePlugin as weaver_plugin_rope
participant RopeAdapter as PythonRopeAdapter
participant ApplyPatch as act_apply_patch_handler
participant DoubleLock as DoubleLock_harness
User->>WeaverCLI: run act refactor --provider rope --refactoring rename --file notes.py offset new_name
WeaverCLI->>Weaverd: JSONL CommandRequest (domain act, operation refactor)
Weaverd->>Weaverd: DomainRouter::route
Weaverd->>Refactor: handle(CommandRequest, RefactorDependencies)
Refactor->>Refactor: parse_refactor_args, resolve_file, read file content
Refactor->>Refactor: build PluginRequest (FilePayload, arguments)
Refactor->>Runtime: execute(provider, PluginRequest)
activate Runtime
Runtime->>Runner: execute(provider, PluginRequest)
activate Runner
Runner->>RopePlugin: spawn process with SandboxExecutor
activate RopePlugin
RopePlugin->>RopePlugin: run(read PluginRequest from stdin)
RopePlugin->>RopeAdapter: rename(FilePayload, offset, new_name)
activate RopeAdapter
RopeAdapter->>RopeAdapter: create TempDir, write workspace file
RopeAdapter->>RopeAdapter: invoke python3 -c PYTHON_RENAME_SCRIPT
RopeAdapter-->>RopePlugin: modified file content or RopeAdapterError
deactivate RopeAdapter
RopePlugin->>RopePlugin: build PluginResponse
RopePlugin-->>Runner: PluginResponse (stdout JSONL)
deactivate RopePlugin
Runner-->>Runtime: PluginResponse
deactivate Runner
Runtime-->>Refactor: PluginResponse
deactivate Runtime
Refactor->>Refactor: handle_plugin_response
alt PluginOutput Diff
Refactor->>ApplyPatch: forward_diff_to_apply_patch(patch)
ApplyPatch->>DoubleLock: validate syntax and semantics
DoubleLock-->>ApplyPatch: ok
ApplyPatch-->>Refactor: DispatchResult status 0
else PluginOutput not Diff or failure
Refactor-->>WeaverCLI: status 1 with diagnostics
end
Refactor-->>Weaverd: DispatchResult
Weaverd-->>WeaverCLI: CLI exit status and output
WeaverCLI-->>User: show result
Class diagram for rope plugin crate and refactor runtime wiringclassDiagram
class RopeAdapter {
<<trait>>
+rename(file FilePayload, offset usize, new_name &str) Result String,RopeAdapterError
}
class PythonRopeAdapter {
+PythonRopeAdapter()
+rename(file FilePayload, offset usize, new_name &str) Result String,RopeAdapterError
}
RopeAdapter <|.. PythonRopeAdapter
class RopeAdapterError {
<<enum>>
WorkspaceCreate
WorkspaceWrite
Spawn
EngineFailed
InvalidOutput
InvalidPath
}
class PluginDispatchError {
<<enum>>
Write
Serialize
}
class PluginRequest {
}
class PluginResponse {
+success(output PluginOutput) PluginResponse
+failure(diagnostics Vec~PluginDiagnostic~) PluginResponse
}
class PluginOutput {
<<enum>>
Diff
Analysis
Empty
}
class weaver_plugin_rope_lib {
+run(stdin BufRead, stdout Write) Result void,PluginDispatchError
+run_with_adapter(stdin BufRead, stdout Write, adapter RopeAdapter) Result void,PluginDispatchError
-read_request(stdin BufRead) Result PluginRequest,String
-execute_request(adapter RopeAdapter, request PluginRequest) PluginResponse
-execute_rename(adapter RopeAdapter, request PluginRequest) PluginResponse
-parse_rename_arguments(arguments HashMap~String,Value~) Result (usize,String),String
-build_search_replace_patch(path Path, original &str, modified &str) String
-validate_relative_path(path Path) Result void,RopeAdapterError
}
weaver_plugin_rope_lib ..> RopeAdapter : uses
weaver_plugin_rope_lib ..> PluginRequest : uses
weaver_plugin_rope_lib ..> PluginResponse : uses
class RefactorPluginRuntime {
<<trait>>
+execute(provider &str, request &PluginRequest) Result PluginResponse,PluginError
}
class SandboxRefactorRuntime {
-runner Option~PluginRunner~
-startup_error Option~String~
+from_environment() SandboxRefactorRuntime
+execute(provider &str, request &PluginRequest) Result PluginResponse,PluginError
}
RefactorPluginRuntime <|.. SandboxRefactorRuntime
class RefactorDependencies {
+workspace_root &Path
+runtime &RefactorPluginRuntime
+new(workspace_root &Path, runtime &RefactorPluginRuntime) RefactorDependencies
}
class DomainRouter {
-workspace_root PathBuf
-refactor_runtime Arc~RefactorPluginRuntime~
+new(workspace_root PathBuf) DomainRouter
+route_act(request CommandRequest, writer ResponseWriter, backends FusionBackends) DispatchResult
}
class RefactorHandler {
+handle(request CommandRequest, writer ResponseWriter, backends FusionBackends, dependencies RefactorDependencies) DispatchResult,DispatchError
+handle_plugin_response(response PluginResponse, writer ResponseWriter, backends FusionBackends, workspace_root &Path) DispatchResult,DispatchError
+forward_diff_to_apply_patch(patch &str, writer ResponseWriter, backends FusionBackends, workspace_root &Path) DispatchResult,DispatchError
}
DomainRouter ..> RefactorPluginRuntime : holds_arc
DomainRouter ..> RefactorDependencies : constructs
RefactorHandler ..> RefactorDependencies : uses
RefactorHandler ..> RefactorPluginRuntime : execute
SandboxRefactorRuntime ..> PluginRunner : wraps
SandboxRefactorRuntime ..> PluginRegistry : registers_manifest
SandboxRefactorRuntime ..> SandboxExecutor : uses
SandboxRefactorRuntime ..> PluginManifest : builds
SandboxRefactorRuntime ..> PluginMetadata : builds
RefactorHandler ..> PluginRequest : builds
RefactorHandler ..> PluginResponse : consumes
RefactorHandler ..> PluginOutput : matches
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdd a rope-backed actuator plugin crate and CLI, wire a refactor plugin runtime into the daemon via dependency injection, and add unit, behavioural and e2e tests plus feature specs. Update workspace manifests and documentation to register and describe the new plugin. Changes
Sequence DiagramssequenceDiagram
participant Client as CLI Client
participant Daemon as Weaver Daemon
participant Runtime as Refactor Runtime
participant Plugin as Rope Plugin Process
participant FS as File System
Client->>Daemon: act refactor (provider=rope, file, args)
activate Daemon
Daemon->>Runtime: execute(provider, PluginRequest)
activate Runtime
Runtime->>Plugin: spawn executable with JSONL on stdin
activate Plugin
Plugin->>FS: create temp workspace and write file
Plugin->>Plugin: run Python rope engine (rename)
Plugin->>Plugin: compute diff / diagnostics
Plugin-->>Runtime: stream JSONL PluginResponse
deactivate Plugin
Runtime-->>Daemon: return PluginResponse (Diff or Diagnostics)
deactivate Runtime
Daemon->>Daemon: handle_plugin_response
alt Diff
Daemon->>Daemon: forward_diff_to_apply_patch
Daemon->>FS: apply patch to workspace files
else Diagnostics/Failure
Daemon->>Daemon: emit failure diagnostics, do not mutate FS
end
Daemon-->>Client: stream output and exit status
deactivate Daemon
sequenceDiagram
participant Test as E2E Test
participant FakeDaemon as Fake Daemon Server
participant CLI as Weaver CLI
participant Observe as observe (jq pipe)
Test->>FakeDaemon: start listening on TCP port
activate FakeDaemon
Test->>CLI: run refactor pipeline (observe | jq)
activate CLI
CLI->>FakeDaemon: send request
FakeDaemon-->>CLI: stream JSONL responses
CLI->>Observe: pipe output to jq
Observe-->>CLI: transformed output
CLI-->>Test: capture stdout, stderr, exit status
deactivate CLI
Test->>Test: assemble Transcript (command, outputs, requests)
Test->>Test: snapshot with insta
deactivate FakeDaemon
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ 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}
- Add e2e command ergonomics tests in crates/weaver-e2e using assert_cmd and insta snapshots. - Document actuator command usage and pipeline flow from observe through jq to actuator. - Update documentation to reflect addition of e2e tests. - Add assert_cmd as dev-dependency in crates/weaver-e2e. These tests help verify CLI usage flows and improve discoverability of commands. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…on and testing - Implement `weaver-plugin-rope` crate as a one-shot plugin using Python rope for renaming - Wire `act refactor` handler to execute actuator plugins and route diff output through apply-patch pipeline - Provide default rope plugin registration in daemon with override via WEAVER_ROPE_PLUGIN_PATH - Add unit, behavioral, and end-to-end tests covering success, failures, and pipeline usage - Update docs with usage, design decisions, and roadmap completion for rope actuator - Enable CLI ergonomics snapshots for testing act refactor workflows This introduces first actuator plugin support for rope-powered Python refactoring with robust safety harness integration. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 13 issues, and left some high level feedback:
- In
resolve_rope_plugin_path, ifcurrent_dir()fails you silently discard a user-specified relative override and fall back to the default binary path; consider surfacing an explicit error or at least keeping the override rather than silently changing behavior. - In
PythonRopeAdapter::rename,absolute_pathis computed but only assigned to_and never used; this can be removed or used for a sanity check to avoid dead code and clarify intent. - In the e2e
FakeDaemon::join, the thread join result is dropped; consider joining and asserting success so panics in the fake daemon thread are surfaced instead of being silently ignored.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `resolve_rope_plugin_path`, if `current_dir()` fails you silently discard a user-specified relative override and fall back to the default binary path; consider surfacing an explicit error or at least keeping the override rather than silently changing behavior.
- In `PythonRopeAdapter::rename`, `absolute_path` is computed but only assigned to `_` and never used; this can be removed or used for a sanity check to avoid dead code and clarify intent.
- In the e2e `FakeDaemon::join`, the thread join result is dropped; consider joining and asserting success so panics in the fake daemon thread are surfaced instead of being silently ignored.
## Individual Comments
### Comment 1
<location> `crates/weaver-plugin-rope/src/tests/mod.rs:11` </location>
<code_context>
+ DiagnosticSeverity, FilePayload, PluginDiagnostic, PluginOutput, PluginRequest, PluginResponse,
+};
+
+use crate::{RopeAdapter, RopeAdapterError, execute_request};
+
+#[derive(Default)]
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for the stdin/stdout dispatch layer (run/run_with_adapter) to cover empty and malformed requests
Current tests only cover `execute_request`, not the stdin/stdout dispatch layer (`read_request`, `run_with_adapter`, `run`). Please add tests that:
- Send empty stdin to `run_with_adapter` and assert a failure `PluginResponse` with a clear diagnostic.
- Send malformed JSONL input and assert the diagnostic contents.
- Confirm a valid request is written as exactly one JSON line with a trailing newline.
You can use in‑memory `BufRead`/`Write` (e.g. `Cursor<Vec<u8>>`) in tests instead of the real `PythonRopeAdapter`.
</issue_to_address>
### Comment 2
<location> `crates/weaver-plugin-rope/src/tests/mod.rs:54-62` </location>
<code_context>
+ }
+}
+
+fn request_with_args(arguments: HashMap<String, serde_json::Value>) -> PluginRequest {
+ PluginRequest::with_arguments(
+ "rename",
+ vec![FilePayload::new(
+ PathBuf::from("src/main.py"),
+ "def old_name():\n return 1\n",
+ )],
+ arguments,
+ )
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for argument parsing edge cases (offset/new_name types and invalid values)
`parse_rename_arguments` still has several untested branches:
- `offset` as a JSON number vs. string, and a non‑numeric string (e.g. `"abc"`) producing the expected error.
- Missing `new_name`, or `new_name` present but empty/whitespace.
- Negative numeric `offset` values that should fail when parsed into `usize`.
Please extend the existing table‑driven tests to cover these cases by constructing `PluginRequest`s with those argument shapes and asserting that `execute_request` returns a failure `PluginResponse` with the expected diagnostics.
Suggested implementation:
```rust
use rstest::rstest;
use weaver_plugins::protocol::{FilePayload, PluginOutput, PluginRequest};
use crate::{RopeAdapter, RopeAdapterError, execute_request};
use serde_json::json;
/// Helper to build a rename request with a fixed file payload and custom arguments.
fn request_with_args(arguments: HashMap<String, serde_json::Value>) -> PluginRequest {
PluginRequest::with_arguments(
"rename",
vec![FilePayload::new(
PathBuf::from("src/main.py"),
"def old_name():\n return 1\n",
)],
arguments,
)
}
#[rstest]
fn parse_rename_arguments_edge_cases() {
// offset as a JSON number
{
let mut arguments = HashMap::new();
arguments.insert("offset".to_string(), json!(5));
arguments.insert("new_name".to_string(), json!("new_name"));
let request = request_with_args(arguments);
let output = execute_request(request);
match output {
PluginOutput::Failure { diagnostics } => {
assert!(
!diagnostics.is_empty(),
"numeric offset should still be validated and produce diagnostics on error"
);
}
other => panic!("expected Failure for numeric offset case, got: {:?}", other),
}
}
// offset as a numeric-looking string
{
let mut arguments = HashMap::new();
arguments.insert("offset".to_string(), json!("5"));
arguments.insert("new_name".to_string(), json!("new_name"));
let request = request_with_args(arguments);
let output = execute_request(request);
match output {
PluginOutput::Failure { diagnostics } => {
assert!(
!diagnostics.is_empty(),
"string offset should be parsed and produce diagnostics on error"
);
}
other => panic!("expected Failure for string offset case, got: {:?}", other),
}
}
// offset as a non-numeric string
{
let mut arguments = HashMap::new();
arguments.insert("offset".to_string(), json!("abc"));
arguments.insert("new_name".to_string(), json!("new_name"));
let request = request_with_args(arguments);
let output = execute_request(request);
match output {
PluginOutput::Failure { diagnostics } => {
assert!(
!diagnostics.is_empty(),
"non-numeric string offset should fail with diagnostics"
);
}
other => panic!(
"expected Failure for non-numeric string offset case, got: {:?}",
other
),
}
}
// negative numeric offset
{
let mut arguments = HashMap::new();
arguments.insert("offset".to_string(), json!(-1));
arguments.insert("new_name".to_string(), json!("new_name"));
let request = request_with_args(arguments);
let output = execute_request(request);
match output {
PluginOutput::Failure { diagnostics } => {
assert!(
!diagnostics.is_empty(),
"negative offset should fail when parsed into usize and produce diagnostics"
);
}
other => panic!("expected Failure for negative offset case, got: {:?}", other),
}
}
// missing new_name
{
let mut arguments = HashMap::new();
arguments.insert("offset".to_string(), json!(5));
let request = request_with_args(arguments);
let output = execute_request(request);
match output {
PluginOutput::Failure { diagnostics } => {
assert!(
!diagnostics.is_empty(),
"missing new_name should fail with diagnostics"
);
}
other => panic!("expected Failure for missing new_name case, got: {:?}", other),
}
}
// empty new_name
{
let mut arguments = HashMap::new();
arguments.insert("offset".to_string(), json!(5));
arguments.insert("new_name".to_string(), json!(""));
let request = request_with_args(arguments);
let output = execute_request(request);
match output {
PluginOutput::Failure { diagnostics } => {
assert!(
!diagnostics.is_empty(),
"empty new_name should fail with diagnostics"
);
}
other => panic!("expected Failure for empty new_name case, got: {:?}", other),
}
}
// whitespace-only new_name
{
let mut arguments = HashMap::new();
arguments.insert("offset".to_string(), json!(5));
arguments.insert("new_name".to_string(), json!(" "));
let request = request_with_args(arguments);
let output = execute_request(request);
match output {
PluginOutput::Failure { diagnostics } => {
assert!(
!diagnostics.is_empty(),
"whitespace-only new_name should fail with diagnostics"
);
}
other => panic!(
"expected Failure for whitespace-only new_name case, got: {:?}",
other
),
}
}
}
```
1. If `execute_request` returns a `Result<PluginOutput, RopeAdapterError>` rather than a bare `PluginOutput`, wrap the call accordingly (e.g. `let output = execute_request(request).expect("request execution failed");`).
2. If `PluginOutput::Failure` has a different shape (e.g. includes additional fields or wraps diagnostics differently), adjust the pattern match to the actual enum definition and update how `diagnostics` are accessed.
3. If the project already defines a shared helper for constructing `PluginRequest`s for the rename operation, consider reusing that instead of the local `request_with_args` to stay consistent with other tests.
4. To assert on specific diagnostic messages (as opposed to just presence of diagnostics), inspect the concrete diagnostic type in your codebase (usually `weaver_plugins::protocol::Diagnostic`) and replace the `!diagnostics.is_empty()` assertions with checks that the expected substring (e.g. mentioning `offset` or `new_name`) is present in one of the diagnostic messages.
</issue_to_address>
### Comment 3
<location> `crates/weaver-plugin-rope/src/tests/mod.rs:113-116` </location>
<code_context>
+ assert!(!response.is_success());
+}
+
+#[rstest]
+#[case::no_change(String::from("def old_name():\n return 1\n"))]
+#[case::adapter_error(String::new())]
+fn rename_non_mutating_or_error_returns_failure(#[case] output: String) {
+ let mut arguments = HashMap::new();
+ arguments.insert(
</code_context>
<issue_to_address>
**suggestion (testing):** Differentiate between adapter error diagnostics and no‑change diagnostics in tests
This test only checks that both the no‑change and error cases fail, but not that they produce distinct, meaningful diagnostics.
Please extend the assertions to:
- Verify the specific diagnostic text for the no‑change case (e.g., “produced no content changes”).
- Verify that the adapter‑error case surfaces the underlying `RopeAdapterError` message.
That way the test will also guard against regressions in diagnostic quality or incorrect error mapping.
Suggested implementation:
```rust
#[rstest]
#[case::no_change(RenameFailureScenario::NoChange)]
#[case::adapter_error(RenameFailureScenario::AdapterError)]
fn rename_non_mutating_or_error_returns_failure(#[case] scenario: RenameFailureScenario) {
```
To fully implement the requested behavior, you’ll need to:
1. **Introduce a small helper enum** (near the tests, e.g. above them) to clearly distinguish the two cases in the test:
```rust
enum RenameFailureScenario {
NoChange,
AdapterError,
}
```
2. **Replace the body of `rename_non_mutating_or_error_returns_failure`** so it:
- Builds `arguments` the same way the existing test does.
- Constructs a `MockAdapter` whose `result` depends on `scenario`:
- `RenameFailureScenario::NoChange` → `result: Ok("def old_name():\n return 1\n".to_string())`
(i.e. same content as the original, so the rename produces no change).
- `RenameFailureScenario::AdapterError` → `result: Err(RopeAdapterError::Other("simulated adapter error".into()))`
(or whatever constructor/variant is appropriate for your `RopeAdapterError`).
- Calls `execute_request` as before:
```rust
let request = PluginRequest::new("rename", arguments);
let response = execute_request(&adapter, &request);
assert!(!response.is_success());
```
- **Asserts diagnostics for each scenario**:
```rust
let messages: Vec<&str> = response
.diagnostics
.iter()
.map(|d| d.message.as_str())
.collect();
match scenario {
RenameFailureScenario::NoChange => {
assert!(
messages.iter().any(|m| m.contains("produced no content changes")),
"expected a diagnostic mentioning that the rename produced no content changes, got: {:?}",
messages,
);
}
RenameFailureScenario::AdapterError => {
assert!(
messages.iter().any(|m| m.contains("simulated adapter error")),
"expected the RopeAdapterError message to surface in diagnostics, got: {:?}",
messages,
);
}
}
```
3. **Import `RopeAdapterError`** in this test module if it isn’t already:
```rust
use crate::rope_adapter::RopeAdapterError;
```
(Adjust the path to match your actual module layout.)
These changes will ensure the test still checks that both scenarios fail, *and* that the diagnostics are distinct and meaningful, guarding against regressions in error mapping and diagnostic quality.
</issue_to_address>
### Comment 4
<location> `crates/weaverd/src/dispatch/act/refactor/mod.rs:51` </location>
<code_context>
+ ) -> Result<PluginResponse, PluginError>;
+}
+
+/// Dependencies required by the `act refactor` handler.
+pub(crate) struct RefactorDependencies<'a> {
+ workspace_root: &'a Path,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new refactor plumbing by removing the RefactorDependencies wrapper and making SandboxRefactorRuntime construction enforce its runner invariant so the hot path no longer deals with Options and extra indirection.
You can flatten a couple of the new abstractions without losing behavior or testability.
### 1) Drop `RefactorDependencies` and pass parameters directly
`RefactorDependencies<'a>` adds an extra layer and lifetime complexity for two values the router already owns. You can pass them directly without losing flexibility, and tests can still wrap them with small helpers in the test module.
```rust
pub fn handle<W: Write>(
request: &CommandRequest,
writer: &mut ResponseWriter<W>,
backends: &mut FusionBackends<SemanticBackendProvider>,
workspace_root: &Path,
runtime: &dyn RefactorPluginRuntime,
) -> Result<DispatchResult, DispatchError> {
// ...
let file_path = resolve_file(workspace_root, &args.file)?;
// ...
let plugin_request = PluginRequest::with_arguments(
&args.refactoring,
vec![FilePayload::new(PathBuf::from(&args.file), file_content)],
plugin_args,
);
match runtime.execute(&args.provider, &plugin_request) {
Ok(response) => handle_plugin_response(response, writer, backends, workspace_root),
Err(error) => {
writer.write_stderr(format!(
"act refactor failed: {error} (provider={}, refactoring={}, file={})\n",
args.provider, args.refactoring, args.file
))?;
Ok(DispatchResult::with_status(1))
}
}
}
```
Call site from the router becomes simpler:
```rust
// in DomainRouter
act::refactor::handle(
request,
writer,
backends,
&self.workspace_root,
self.refactor_runtime.as_ref(),
)
```
This removes `RefactorDependencies` entirely while preserving behavior.
### 2) Encode `SandboxRefactorRuntime` invariant and remove internal `Option`s
Once `SandboxRefactorRuntime` is constructed successfully, it should always have a runner. Right now `execute` has to handle a missing runner and re-synthesize an error, which complicates the hot path.
You can make construction fallible and introduce a “disabled” runtime for the failure case:
```rust
pub struct SandboxRefactorRuntime {
runner: PluginRunner<SandboxExecutor>,
}
impl SandboxRefactorRuntime {
pub fn from_environment() -> Result<Self, String> {
let mut registry = PluginRegistry::new();
let executable = resolve_rope_plugin_path(std::env::var_os(ROPE_PLUGIN_PATH_ENV));
let metadata =
PluginMetadata::new(ROPE_PLUGIN_NAME, ROPE_PLUGIN_VERSION, PluginKind::Actuator);
let manifest = PluginManifest::new(metadata, vec![String::from("python")], executable);
registry
.register(manifest)
.map_err(|e| format!("failed to initialise refactor runtime: {e}"))?;
Ok(Self {
runner: PluginRunner::new(registry, SandboxExecutor),
})
}
}
impl RefactorPluginRuntime for SandboxRefactorRuntime {
fn execute(
&self,
provider: &str,
request: &PluginRequest,
) -> Result<PluginResponse, PluginError> {
self.runner.execute(provider, request)
}
}
```
Then keep the public `default_runtime` behavior by deciding how to handle initialization failure there, e.g. falling back to a noop runtime:
```rust
struct NoopRefactorRuntime {
message: String,
}
impl RefactorPluginRuntime for NoopRefactorRuntime {
fn execute(
&self,
_provider: &str,
_request: &PluginRequest,
) -> Result<PluginResponse, PluginError> {
Err(PluginError::Manifest {
message: self.message.clone(),
})
}
}
pub(crate) fn default_runtime() -> Arc<dyn RefactorPluginRuntime + Send + Sync> {
match SandboxRefactorRuntime::from_environment() {
Ok(runtime) => Arc::new(runtime),
Err(msg) => Arc::new(NoopRefactorRuntime { message: msg }),
}
}
```
This keeps all current behavior (including error reporting when the runtime is unavailable) but simplifies the core path by removing the `Option` juggling and `startup_error` string plumbing from `SandboxRefactorRuntime::execute`.
</issue_to_address>
### Comment 5
<location> `crates/weaver-plugin-rope/src/lib.rs:221` </location>
<code_context>
+ .map_err(|error| format!("invalid plugin request JSON: {error}"))
+}
+
+fn execute_request<R: RopeAdapter>(adapter: &R, request: &PluginRequest) -> PluginResponse {
+ match request.operation() {
+ "rename" => execute_rename(adapter, request),
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the request/rename execution path to return a Result that is converted to a PluginResponse in one place to simplify control flow and error handling.
You can simplify the control flow and error handling by making the request/rename pipeline return a `Result<PluginResponse, String>` and mapping to a `PluginResponse` only once at the top. This removes several early-return `failure_response` calls and makes the flow easier to follow without changing behavior.
For example:
```rust
type ExecResult = Result<PluginResponse, String>;
```
Change the helpers to return `ExecResult`:
```rust
fn execute_request<R: RopeAdapter>(adapter: &R, request: &PluginRequest) -> ExecResult {
match request.operation() {
"rename" => execute_rename(adapter, request),
other => Err(format!("unsupported refactoring operation '{other}'")),
}
}
fn execute_rename<R: RopeAdapter>(adapter: &R, request: &PluginRequest) -> ExecResult {
let (offset, new_name) = parse_rename_arguments(request.arguments())?;
let file = request
.files()
.first()
.ok_or_else(|| String::from("rename operation requires one file payload"))?;
let modified = adapter
.rename(file, offset, &new_name)
.map_err(|error| error.to_string())?;
if modified == file.content() {
return Err(String::from(
"rename operation produced no content changes",
));
}
let patch = build_search_replace_patch(file.path(), file.content(), &modified);
Ok(PluginResponse::success(PluginOutput::Diff { content: patch }))
}
```
Then `run_with_adapter` can centralize the conversion from error string to `PluginResponse`:
```rust
pub fn run_with_adapter<R: RopeAdapter>(
stdin: &mut impl BufRead,
stdout: &mut impl Write,
adapter: &R,
) -> Result<(), PluginDispatchError> {
let response = match read_request(stdin)
.and_then(|request| execute_request(adapter, &request))
{
Ok(resp) => resp,
Err(msg) => failure_response(msg),
};
let payload =
serde_json::to_string(&response).map_err(|source| PluginDispatchError::Serialize { source })?;
stdout
.write_all(payload.as_bytes())
.and_then(|_| stdout.write_all(b"\n"))
.and_then(|_| stdout.flush())
.map_err(|source| PluginDispatchError::Write { source })
}
```
This keeps all existing behavior (same error messages, same response shape) but reduces duplicated `failure_response(...)` construction and makes the flow from “request → result → response” more linear.
</issue_to_address>
### Comment 6
<location> `crates/weaverd/src/dispatch/act/refactor/behaviour.rs:64` </location>
<code_context>
+ }
+}
+
+struct RefactorWorld {
+ workspace: TempDir,
+ request: CommandRequest,
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the BDD-style `RefactorWorld` and step macros with simple helper functions and rstest-based parameterized tests to make the refactor handler tests more direct and readable while preserving behavior.
You can keep all existing behavior while significantly simplifying the tests by:
1. Removing the BDD world object and macros.
2. Using a small helper function to execute the handler.
3. Using parameterized tests (`rstest`) to cover the different modes and expectations.
Concretely, you can replace `RefactorWorld` + `given/when/then` with something like this:
```rust
#[derive(Clone, Copy)]
enum RuntimeMode {
DiffSuccess,
RuntimeError,
MalformedDiff,
}
struct MockRuntime {
mode: RuntimeMode,
}
impl RefactorPluginRuntime for MockRuntime {
fn execute(
&self,
_provider: &str,
_request: &PluginRequest,
) -> Result<PluginResponse, PluginError> {
match self.mode {
RuntimeMode::DiffSuccess => Ok(PluginResponse::success(PluginOutput::Diff {
content: String::from(VALID_DIFF),
})),
RuntimeMode::RuntimeError => Err(PluginError::NotFound {
name: String::from("rope"),
}),
RuntimeMode::MalformedDiff => Ok(PluginResponse::success(PluginOutput::Diff {
content: String::from(MALFORMED_DIFF),
})),
}
}
}
fn command_request(args: &[&str]) -> CommandRequest {
CommandRequest {
command: CommandDescriptor {
domain: "act".into(),
operation: "refactor".into(),
},
arguments: args.iter().map(|s| s.to_string()).collect(),
patch: None,
}
}
fn execute_refactor(
workspace: &Path,
request: CommandRequest,
runtime_mode: RuntimeMode,
) -> (Result<i32, DispatchError>, String) {
let runtime = MockRuntime { mode: runtime_mode };
let mut output = Vec::new();
let mut writer = ResponseWriter::new(&mut output);
let mut backends = build_backends();
let result = handle(
&request,
&mut writer,
&mut backends,
RefactorDependencies::new(workspace, &runtime),
)
.map(|dispatch| dispatch.status);
let response_stream = String::from_utf8(output).expect("response utf8");
(result, response_stream)
}
```
Then express the scenarios as small parameterized tests instead of BDD steps/stateful `RefactorWorld`:
```rust
use rstest::rstest;
#[rstest]
fn refactor_succeeds_and_updates_file() {
let workspace = TempDir::new().expect("workspace");
let notes = workspace.path().join("notes.txt");
std::fs::write(¬es, ORIGINAL_CONTENT).unwrap();
let request = command_request(&[
"--provider", "rope",
"--refactoring", "rename",
"--file", "notes.txt",
"offset=1",
"new_name=woven",
]);
let (result, stderr) =
execute_refactor(workspace.path(), request, RuntimeMode::DiffSuccess);
assert_eq!(result.expect("status"), 0);
assert_eq!(std::fs::read_to_string(¬es).unwrap(), UPDATED_CONTENT);
assert!(
stderr.contains("some expected log text"),
"stderr: {stderr}"
);
}
#[rstest]
fn missing_provider_rejected_as_invalid_arguments() {
let workspace = TempDir::new().expect("workspace");
let notes = workspace.path().join("notes.txt");
std::fs::write(¬es, ORIGINAL_CONTENT).unwrap();
let request = command_request(&[
"--refactoring", "rename",
"--file", "notes.txt",
]);
let (result, _stderr) =
execute_refactor(workspace.path(), request, RuntimeMode::DiffSuccess);
assert!(matches!(result, Err(DispatchError::InvalidArguments { .. })));
}
```
If you want to keep table-driven coverage for runtime variants without extra macros:
```rust
#[rstest]
#[case(RuntimeMode::RuntimeError, 1, true, ORIGINAL_CONTENT)]
#[case(RuntimeMode::MalformedDiff, 1, true, ORIGINAL_CONTENT)]
fn refactor_failure_modes(
#[case] mode: RuntimeMode,
#[case] expected_status: i32,
#[case] expect_error_in_stderr: bool,
#[case] expected_file_content: &str,
) {
let workspace = TempDir::new().expect("workspace");
let notes = workspace.path().join("notes.txt");
std::fs::write(¬es, ORIGINAL_CONTENT).unwrap();
let request = command_request(&[
"--provider", "rope",
"--refactoring", "rename",
"--file", "notes.txt",
"offset=1",
"new_name=woven",
]);
let (result, stderr) = execute_refactor(workspace.path(), request, mode);
assert_eq!(result.expect("status"), expected_status);
assert_eq!(std::fs::read_to_string(¬es).unwrap(), expected_file_content);
if expect_error_in_stderr {
assert!(
stderr.contains("expected error text"),
"stderr: {stderr}"
);
}
}
```
This keeps:
- Runtime modes and behaviors identical.
- Validation of status codes, file contents, and stderr contents.
But it removes:
- Global mutable `RefactorWorld` with many responsibilities.
- BDD macros and step wiring (`given`/`when`/`then`).
- Indirection between feature file and actual assertions.
The net result is fewer moving parts and easier-to-follow tests while preserving all behavior.
</issue_to_address>
### Comment 7
<location> `crates/weaver-plugin-rope/src/lib.rs:115` </location>
<code_context>
+ #[source]
+ source: std::io::Error,
+ },
+ /// Serialising the response payload failed.
+ #[error("failed to serialize plugin response: {source}")]
+ Serialize {
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Doc comment uses “Serialising” with -ising; per en-GB-oxendict guidance this should use the -ize form (“Serializing”).
The style guide calls for -ize spellings in comments. Here, changing “Serialising the response payload failed.” to “Serializing the response payload failed.” would align with the en-GB-oxendict (-ize / -yse / -our) requirement.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Comments must use en-GB-oxendict (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 8
<location> `crates/weaverd/src/dispatch/act/refactor/mod.rs:338` </location>
<code_context>
+ return Ok(DispatchResult::with_status(1));
+ }
+
+ match response.output() {
+ PluginOutput::Diff { content } => {
+ forward_diff_to_apply_patch(content, writer, backends, workspace_root)
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This match has three branches; the instructions ask for conditionals with more than two branches to be factored into a predicate/helper function.
`match response.output()` distinguishes three cases (`Diff`, `Analysis`, `Empty`). The review guidelines request that conditionals with more than two branches be moved into a predicate function. One way to align would be to extract a helper, e.g. something like `fn as_diff_content(response: &PluginResponse) -> Option<&str>` and then use an `if let`/`else` around that, keeping the main handler logic at two branches.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Move conditionals with >2 branches into a predicate function.
</details>
</issue_to_address>
### Comment 9
<location> `crates/weaverd/src/dispatch/act/refactor/behaviour.rs:50` </location>
<code_context>
+ _provider: &str,
+ _request: &PluginRequest,
+ ) -> Result<PluginResponse, PluginError> {
+ match self.mode {
+ RuntimeMode::DiffSuccess => Ok(PluginResponse::success(PluginOutput::Diff {
+ content: String::from(VALID_DIFF),
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The match on `self.mode` has three branches, which the style instructions say should be pulled out into a predicate/helper when there are more than two branches.
`MockRuntime::execute` has a three-way `match` on `RuntimeMode`. To follow the “>2 branches into a predicate function” rule, you could extract the mode-to-response mapping into a small helper (or predicate returning an enum/Result) and keep `execute` itself to two branches (e.g. `if is_diff_success_mode(...) { ... } else { ... }`).
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Move conditionals with >2 branches into a predicate function.
</details>
</issue_to_address>
### Comment 10
<location> `docs/execplans/3-1-1a-plugin-for-rope.md:3` </location>
<code_context>
+# Implement the first actuator plugin for `rope`
+
+This ExecPlan is a living document. The sections `Constraints`, `Tolerances`,
+`Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, and
+`Outcomes & Retrospective` must be kept up to date as work proceeds.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** ExecPlan is introduced as an acronym without first being expanded.
To follow the acronym rule, expand ExecPlan on first use, for example: "This Execution Plan (ExecPlan) is a living document." Subsequent uses can then use the short form.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 11
<location> `docs/execplans/3-1-1a-plugin-for-rope.md:110` </location>
<code_context>
+
+## Surprises & Discoveries
+
+- Observation: project memory MCP resources were not available in this session
+ (`list_mcp_resources` returned no servers/resources). Evidence: tool output
+ returned empty resource lists. Impact: planning relied on repository docs and
</code_context>
<issue_to_address>
**suggestion (review_instructions):** MCP is used without being expanded on first mention.
Consider expanding MCP on first use, for example: "project memory Model Context Protocol (MCP) resources". Afterwards the short form MCP is fine.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 12
<location> `docs/execplans/3-1-1a-plugin-for-rope.md:43` </location>
<code_context>
+- Do not bypass the Double-Lock harness for plugin-produced edits.
+- Continue using `rstest-bdd` v0.5.0 and write new behavioural tests with
+ mutable world fixtures (`&mut World`) for new scenarios.
+- Add e2e command ergonomics tests in `crates/weaver-e2e/` using `assert_cmd`
+ and `insta` snapshots for CLI usage flows.
+- Keep module-level `//!` comments and rustdoc for public items; follow
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym e2e is used before being expanded to "end-to-end".
Please expand e2e on first use, for example: "Add end-to-end (e2e) command ergonomics tests...". Later references can use just e2e.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 13
<location> `docs/execplans/3-1-1a-plugin-for-rope.md:203` </location>
<code_context>
+
+Add new crate `crates/weaver-plugin-rope/` (workspace member) that:
+
+- reads one `PluginRequest` JSONL line from stdin,
+- validates operation + arguments,
+- performs rope-backed refactoring via an adapter boundary,
</code_context>
<issue_to_address>
**suggestion (review_instructions):** JSONL is used without being expanded on first use in this document.
Consider expanding JSONL the first time it appears, for example: "JSON Lines (JSONL)". After that, JSONL alone is fine.
<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}
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…and improve error handling - Change execute_request to return Result, separating success and failure logic. - Update tests to handle Result and use failure_response for errors. - Simplify read_request and execute_request combination for cleaner flow. - Unbundle RefactorDependencies in weaverd dispatch, passing workspace_root and runtime separately. - Change SandboxRefactorRuntime::from_environment to return Result for error handling. - Provide NoopRefactorRuntime to handle initialization failure gracefully. - Improve domain router to inject custom refactor runtime for testing. - Fix error handling and diagnostic messages in rope plugin and weaverd dispatch. - Minor improvements in stderr handling and path resolution warnings. - Update tests with more granular error scenario coverage and JSON parsing cases. This refactor improves error handling clarity, testability, and runtime initialization robustness. 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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Have the following now been resolved? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll check the current state of the codebase to verify which comments have been resolved. 🏁 Script executed: Length of output: 646 🏁 Script executed: Length of output: 1273 🏁 Script executed: Length of output: 268 🏁 Script executed: Length of output: 203 🏁 Script executed: Length of output: 869 🏁 Script executed: Length of output: 391 🏁 Script executed: Length of output: 289 🏁 Script executed: Length of output: 248 🏁 Script executed: Length of output: 150 🏁 Script executed: Length of output: 90 🏁 Script executed: Length of output: 1200 🏁 Script executed: Length of output: 698 🏁 Script executed: Length of output: 1011
✅ Resolved (10 comments)Individual Comments:
🟡 Partially Resolved (1 comment)Overall Comment 1 (resolve_rope_plugin_path): Improved by logging a warning when ❌ Not Resolved (4 comments)Overall Comments:
Individual Comments:
The majority of the review feedback has been addressed, particularly the testing gaps, complexity issues, and documentation improvements. The remaining items are suggestions for further refinement rather than critical issues. |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaver-plugin-rope/src/tests/mod.rs Comment on lines +306 to +327 fn rename_negative_offset_string_returns_error() {
let mut arguments = HashMap::new();
arguments.insert(
String::from("offset"),
serde_json::Value::String(String::from("-1")),
);
arguments.insert(
String::from("new_name"),
serde_json::Value::String(String::from("new_name")),
);
let adapter = MockAdapter {
result: Ok(String::from("unused")),
};
let err = execute_request(&adapter, &request_with_args(arguments))
.expect_err("negative offset should fail");
assert!(
err.contains("non-negative integer"),
"expected error mentioning 'non-negative integer', got: {err}"
);
}❌ New issue: Code Duplication |
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/mod.rs Comment on file writer: &mut ResponseWriter<W>,
backends: &mut FusionBackends<SemanticBackendProvider>,
workspace_root: &Path,
runtime: &dyn RefactorPluginRuntime,❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
… tests - Replace manual mock adapter with `mockall` mocks in `behaviour.rs` tests. - Parametrize rename argument validation tests with `rstest`. - Simplify test setup by extracting argument fixtures. - Add tests for edge cases: missing, boolean, and negative `offset`, empty `new_name`. - Improve error message checks in rename tests. This enhances test coverage, clarity, and maintenance by using mockall for mocking and rstest for parameterized testing. 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: 8
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs`:
- Around line 109-111: The current code silently ignores a poisoned mutex by
using `if let Ok(mut guard) = requests.lock()`; replace this with a fail-fast
call so tests surface locking failures — e.g., change to `let mut guard =
requests.lock().expect("failed to lock requests mutex");
guard.push(parsed_request.clone());` referencing the `requests.lock()` call and
the `parsed_request.clone()` usage so a poisoned mutex causes the test to panic
with a clear message.
- Around line 64-69: The tests currently hide a poisoned Mutex by calling
unwrap_or_default() in the requests() method; update requests() to call expect()
on the result of self.requests.lock() (e.g.,
self.requests.lock().expect("requests mutex poisoned")) so a poisoned mutex
surfaces as a test failure instead of returning an empty Vec; keep the rest of
requests() logic (cloning the items) unchanged and reference the requests()
function and the self.requests Mutex to locate the change.
- Around line 119-128: Extract the match that builds payload into a new helper
function response_payload_for_operation(operation: &str) -> String, move the
three-branch logic (the "get-definition", "refactor", and default cases using
json!(...).to_string()) into that function, and replace the inline match with a
call to response_payload_for_operation(operation) where payload is currently
assigned; ensure the helper returns a String and uses the same json! constructs
so behavior is unchanged.
In `@crates/weaver-plugin-rope/src/lib.rs`:
- Around line 65-66: Centralize path validation by calling
validate_relative_path(file.path()) at the dispatch boundary inside
execute_rename so all adapters (including custom ones) are protected from
traversal/absolute paths; then remove the duplicate validate_relative_path
invocation from PythonRopeAdapter (and any other adapter-level checks you find
in the same module) to keep the invariant enforced only in execute_rename and
avoid redundant validation.
- Around line 237-241: The current rename handling silently takes the first
payload via request.files().first() and assigns it to file, but we must enforce
exactly one file; change the logic that creates file (replace the .first() use)
to check request.files().len() == 1 and return an Err (e.g.,
String::from("rename operation requires exactly one file payload")) when the
count is not 1, then safely extract that single entry into the variable file for
the rename operation so extra inputs are rejected rather than ignored.
In `@crates/weaver-plugin-rope/src/tests/mod.rs`:
- Around line 8-26: The tests currently define a handwritten MockAdapter
implementing RopeAdapter (struct MockAdapter and its rename method); replace
this with a mockall-generated mock using mockall::mock! for RopeAdapter and add
a small helper like adapter_returning(Ok(...)) / adapter_returning(Err(...))
used across tests; update tests to instantiate the mock via the mockall macro
and configure its rename expectation/return value instead of using MockAdapter,
and remove the manual MockAdapter type and its impl to align with project
mocking conventions.
In `@crates/weaverd/src/dispatch/act/refactor/behaviour.rs`:
- Around line 157-164: The test helper build_backends currently hardcodes a
Unix-only socket path via SocketEndpoint::unix("/tmp/weaver-test/socket.sock"),
which breaks non-Unix CI; change build_backends (and the Config construction) to
create a temporary directory (e.g., via TempDir) and use a socket path inside
it, or alternatively gate the function with #[cfg(unix)] so it only compiles on
Unix. Update references to Config and SocketEndpoint::unix in build_backends to
accept the temp path (or add a #[cfg(unix)] wrapper) so tests are cross-platform
safe.
- Around line 52-65: The function request_has_required_flags currently iterates
request.arguments three times to check for "--provider", "--refactoring", and
"--file"; refactor it to perform a single pass by either converting
request.arguments to a collection that supports contains (e.g., a HashSet) or
iterating once and checking membership against a slice of required flags
(["--provider","--refactoring","--file"]), then return whether all required
flags were found; update the function request_has_required_flags to use that
single-pass approach for clarity and performance.
- Introduce RefactorContext struct to bundle dependencies for refactor operations. - Update act refactor handle function and callers to use RefactorContext. - Pass socket path dynamically in refactor backend tests to avoid hardcoded paths. - Refactor rope plugin to improve argument validation and mocking in tests. - Enhance test suite for act refactor with rstest and fixtures. - Use clippy expect annotations in weaver-e2e test for clearer error handling. - Improve code clarity and error propagation in act refactor command handling. This improves modularity, testability, and code clarity in the act refactor functionality. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs`:
- Around line 83-96: serve_requests currently blocks forever on
listener.accept(); change it to use non-blocking accept with a deadline so the
test fails fast if the CLI never connects: call listener.set_nonblocking(true)
before the loop, compute a deadline (e.g. Instant::now() +
Duration::from_secs(...)) and inside the for loop attempt listener.accept(),
treating Err(e) where e.kind() == io::ErrorKind::WouldBlock by checking the
deadline, sleeping briefly (e.g. thread::sleep(Duration::from_millis(10))) and
retrying until the deadline, and treating other errors as failures; reference
the serve_requests function and the listener.accept() call when making these
changes.
In `@crates/weaver-plugin-rope/src/lib.rs`:
- Around line 262-293: The parse_rename_arguments function currently allows
numeric new_name values because json_value_to_string accepts numbers; change it
to enforce a string-only contract by extracting new_name with
new_name_value.as_str().ok_or_else(...) (or use a new helper that only accepts
strings) and validate non-empty trimmed string, leaving json_value_to_string
unchanged for offset parsing (or use it only for offset). Update references in
parse_rename_arguments to stop converting numeric new_name values to strings so
the error "new_name argument must be a string" is accurate.
…daemon Introduce a helper function accept_before_deadline to poll TCP listener.accept() in non-blocking mode until a connection arrives or a timeout elapses. This replaces the previous blocking accept call and adds a deadline mechanism for more robust test daemon behavior. Additionally, update the new_name argument parsing to accept only string types strictly and add test cases validating rejection of non-string new_name values. 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: 2
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs`:
- Around line 282-287: The test spawns bash with a login flag "-l" via
Command::new("bash").args(["-lc", shell_script]) which pulls in user profile
files and causes non-determinism; change the invocation to drop the "-l" and use
only "-c" so the Command uses .args(["-c", shell_script]) (locate the
Command::new("bash") call and update the args passed to remove "-l").
In `@crates/weaver-plugin-rope/src/tests/mod.rs`:
- Around line 76-78: The function remove_offset unnecessarily wraps
HashMap::remove in drop; remove the explicit drop and simply call
arguments.remove("offset") (or ignore the return value directly) inside
remove_offset to let the Option returned by HashMap::remove be dropped
implicitly; update the remove_offset function body to call
arguments.remove("offset") without drop.
- Changed bash command argument from '-lc' to '-c' in e2e test to correct shell invocation. - Removed unnecessary drop() call around HashMap::remove in plugin-rope tests for clarity. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Documentation
Code
Rationale
Plan alignment
Revision note
📎 Task: https://www.devboxer.com/task/abbb65ed-f29f-42da-b73f-6d1aa2410d11
📎 Task: https://www.devboxer.com/task/33450ba1-c1f7-4278-8ebd-89bbced1a8c0