diff --git a/Cargo.lock b/Cargo.lock index 953b4267..6952104f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2625,6 +2625,7 @@ dependencies = [ name = "weaver-e2e" version = "0.1.0" dependencies = [ + "assert_cmd", "camino", "insta", "lsp-types", @@ -2670,6 +2671,20 @@ dependencies = [ "weaver-config", ] +[[package]] +name = "weaver-plugin-rope" +version = "0.1.0" +dependencies = [ + "mockall", + "rstest", + "rstest-bdd", + "rstest-bdd-macros", + "serde_json", + "tempfile", + "thiserror 2.0.17", + "weaver-plugins", +] + [[package]] name = "weaver-plugins" version = "0.1.0" @@ -2725,6 +2740,7 @@ dependencies = [ "derive_more 1.0.0", "dirs 5.0.1", "lsp-types", + "mockall", "nix 0.28.0", "once_cell", "ortho_config", diff --git a/Cargo.toml b/Cargo.toml index 23ef1fde..40a2eeb5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,6 +7,7 @@ members = [ "crates/weaver-graph", "crates/weaverd", "crates/weaver-lsp-host", + "crates/weaver-plugin-rope", "crates/weaver-plugins", "crates/weaver-sandbox", "crates/weaver-syntax", diff --git a/crates/weaver-e2e/Cargo.toml b/crates/weaver-e2e/Cargo.toml index 2ee0713a..0a8834c3 100644 --- a/crates/weaver-e2e/Cargo.toml +++ b/crates/weaver-e2e/Cargo.toml @@ -14,6 +14,7 @@ camino = { workspace = true } thiserror = { workspace = true } [dev-dependencies] +assert_cmd = { workspace = true } rstest = { workspace = true } insta = { workspace = true } tempfile = { workspace = true } diff --git a/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs new file mode 100644 index 00000000..01b32cec --- /dev/null +++ b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs @@ -0,0 +1,295 @@ +//! End-to-end CLI ergonomics snapshots for `act refactor`. +//! +//! These tests run the `weaver` binary with a fake daemon endpoint to capture +//! user-facing command ergonomics, including a shell pipeline that chains an +//! observe query through `jq` into an actuator command. + +use std::io::{BufRead, BufReader, Write}; +use std::net::{SocketAddr, TcpListener, TcpStream}; +use std::process::Output; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; +use std::{io, thread}; + +use assert_cmd::Command; +use insta::assert_debug_snapshot; +use serde::Serialize; +use serde_json::json; + +#[derive(Debug, Serialize)] +struct Transcript { + command: String, + status: i32, + stdout: String, + stderr: String, + requests: Vec, +} + +#[derive(Debug)] +struct FakeDaemon { + address: SocketAddr, + requests: Arc>>, + join_handle: thread::JoinHandle<()>, +} + +#[expect( + deprecated, + reason = "assert_cmd::cargo::cargo_bin resolves workspace binaries for e2e tests" +)] +fn weaver_binary_path() -> std::path::PathBuf { + assert_cmd::cargo::cargo_bin("weaver") +} + +impl FakeDaemon { + fn start(expected_requests: usize) -> Result { + let listener = TcpListener::bind(("127.0.0.1", 0))?; + let address = listener.local_addr()?; + let requests = Arc::new(Mutex::new(Vec::new())); + let shared_requests = Arc::clone(&requests); + + let join_handle = thread::spawn(move || { + serve_requests(&listener, expected_requests, &shared_requests); + }); + + Ok(Self { + address, + requests, + join_handle, + }) + } + + fn endpoint(&self) -> String { + format!("tcp://{}", self.address) + } + + #[expect( + clippy::expect_used, + reason = "poisoned mutex in test fixture must surface as panic for clear diagnostics" + )] + fn requests(&self) -> Vec { + self.requests + .lock() + .expect("request mutex should not be poisoned") + .clone() + } + + fn join(self) { + assert!( + self.join_handle.join().is_ok(), + "fake daemon thread should not panic" + ); + } +} + +const ACCEPT_TIMEOUT: Duration = Duration::from_secs(10); +const ACCEPT_POLL_INTERVAL: Duration = Duration::from_millis(10); + +#[expect( + clippy::expect_used, + reason = "non-blocking configuration is fundamental to the deadline mechanism" +)] +fn serve_requests( + listener: &TcpListener, + expected_requests: usize, + requests: &Arc>>, +) { + listener + .set_nonblocking(true) + .expect("non-blocking mode should be supported"); + + for _ in 0..expected_requests { + let Some(stream) = accept_before_deadline(listener) else { + return; + }; + if respond_to_request(stream, requests).is_err() { + return; + } + } +} + +/// Polls `listener.accept()` until a connection arrives or the deadline elapses. +#[expect( + clippy::expect_used, + reason = "restoring blocking mode on accepted stream must succeed for correct I/O" +)] +fn accept_before_deadline(listener: &TcpListener) -> Option { + let deadline = Instant::now() + ACCEPT_TIMEOUT; + + loop { + match listener.accept() { + Ok((stream, _)) => { + stream + .set_nonblocking(false) + .expect("blocking mode should be supported"); + return Some(stream); + } + Err(error) if error.kind() == io::ErrorKind::WouldBlock => { + assert!( + Instant::now() < deadline, + "fake daemon timed out waiting for CLI connection \ + after {ACCEPT_TIMEOUT:?}" + ); + thread::sleep(ACCEPT_POLL_INTERVAL); + } + Err(_) => return None, + } + } +} + +fn response_payload_for_operation(operation: &str) -> String { + match operation { + "get-definition" => json!([{ "symbol": "renamed_symbol" }]).to_string(), + "refactor" => json!({ + "status": "ok", + "files_written": 1, + "files_deleted": 0 + }) + .to_string(), + _ => json!({ "status": "unexpected", "operation": operation }).to_string(), + } +} + +#[expect( + clippy::expect_used, + reason = "poisoned mutex in test fixture must surface as panic for clear diagnostics" +)] +fn respond_to_request( + stream: TcpStream, + requests: &Arc>>, +) -> Result<(), std::io::Error> { + let mut reader = BufReader::new(stream.try_clone()?); + let mut request_line = String::new(); + reader.read_line(&mut request_line)?; + + let parsed_request: serde_json::Value = serde_json::from_str(request_line.trim()) + .unwrap_or_else(|_| { + json!({ + "invalid_request": request_line.trim(), + }) + }); + + requests + .lock() + .expect("request mutex should not be poisoned") + .push(parsed_request.clone()); + + let operation = parsed_request + .get("command") + .and_then(|command| command.get("operation")) + .and_then(serde_json::Value::as_str) + .unwrap_or_default(); + + let payload = response_payload_for_operation(operation); + + let mut writer = stream; + write_json_line( + &mut writer, + &json!({ + "kind": "stream", + "stream": "stdout", + "data": payload, + }), + )?; + write_json_line(&mut writer, &json!({ "kind": "exit", "status": 0 })) +} + +fn write_json_line( + writer: &mut impl Write, + payload: &serde_json::Value, +) -> Result<(), std::io::Error> { + writer.write_all(payload.to_string().as_bytes())?; + writer.write_all(b"\n")?; + writer.flush() +} + +fn output_to_transcript( + command: String, + output: &Output, + requests: Vec, +) -> Transcript { + let status = output.status.code().unwrap_or(-1); + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + Transcript { + command, + status, + stdout, + stderr, + requests, + } +} + +#[test] +fn refactor_actuator_isolation_cli_snapshot() { + let daemon = FakeDaemon::start(1).expect("fake daemon should start"); + let endpoint = daemon.endpoint(); + + let command_string = String::from( + "weaver --daemon-socket tcp:// --output json act refactor --provider rope --refactoring rename --file src/main.py new_name=renamed_symbol offset=4", + ); + + let mut command = Command::new(weaver_binary_path()); + let output = command + .args([ + "--daemon-socket", + endpoint.as_str(), + "--output", + "json", + "act", + "refactor", + "--provider", + "rope", + "--refactoring", + "rename", + "--file", + "src/main.py", + "new_name=renamed_symbol", + "offset=4", + ]) + .output() + .expect("command should execute"); + + let transcript = output_to_transcript(command_string, &output, daemon.requests()); + daemon.join(); + + assert_debug_snapshot!("refactor_actuator_isolation", transcript); +} + +#[test] +fn refactor_pipeline_with_observe_and_jq_snapshot() { + let jq_available = Command::new("jq").arg("--version").output().is_ok(); + if !jq_available { + writeln!( + std::io::stderr().lock(), + "Skipping test: jq not available on PATH" + ) + .ok(); + return; + } + + let daemon = FakeDaemon::start(2).expect("fake daemon should start"); + let endpoint = daemon.endpoint(); + let weaver_bin = weaver_binary_path(); + + let shell_script = concat!( + "\"$WEAVER_BIN\" --daemon-socket \"$WEAVER_ENDPOINT\" --output json ", + "observe get-definition --symbol old_symbol ", + "| jq -r '.[0].symbol' ", + "| xargs -I{} \"$WEAVER_BIN\" --daemon-socket \"$WEAVER_ENDPOINT\" --output json ", + "act refactor --provider rope --refactoring rename --file src/main.py new_name={} offset=4" + ); + + let output = Command::new("bash") + .args(["-c", shell_script]) + .env("WEAVER_BIN", weaver_bin) + .env("WEAVER_ENDPOINT", endpoint.as_str()) + .output() + .expect("pipeline command should execute"); + + let command_string = + String::from("weaver observe get-definition | jq -r '.[0].symbol' | weaver act refactor"); + let transcript = output_to_transcript(command_string, &output, daemon.requests()); + daemon.join(); + + assert_debug_snapshot!("refactor_pipeline_observe_jq", transcript); +} diff --git a/crates/weaver-e2e/tests/snapshots/refactor_rope_cli_snapshots__refactor_actuator_isolation.snap b/crates/weaver-e2e/tests/snapshots/refactor_rope_cli_snapshots__refactor_actuator_isolation.snap new file mode 100644 index 00000000..3125b6a4 --- /dev/null +++ b/crates/weaver-e2e/tests/snapshots/refactor_rope_cli_snapshots__refactor_actuator_isolation.snap @@ -0,0 +1,28 @@ +--- +source: crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs +expression: transcript +--- +Transcript { + command: "weaver --daemon-socket tcp:// --output json act refactor --provider rope --refactoring rename --file src/main.py new_name=renamed_symbol offset=4", + status: 0, + stdout: "{\"files_deleted\":0,\"files_written\":1,\"status\":\"ok\"}", + stderr: "", + requests: [ + Object { + "arguments": Array [ + String("--provider"), + String("rope"), + String("--refactoring"), + String("rename"), + String("--file"), + String("src/main.py"), + String("new_name=renamed_symbol"), + String("offset=4"), + ], + "command": Object { + "domain": String("act"), + "operation": String("refactor"), + }, + }, + ], +} diff --git a/crates/weaver-e2e/tests/snapshots/refactor_rope_cli_snapshots__refactor_pipeline_observe_jq.snap b/crates/weaver-e2e/tests/snapshots/refactor_rope_cli_snapshots__refactor_pipeline_observe_jq.snap new file mode 100644 index 00000000..4bb36cf1 --- /dev/null +++ b/crates/weaver-e2e/tests/snapshots/refactor_rope_cli_snapshots__refactor_pipeline_observe_jq.snap @@ -0,0 +1,38 @@ +--- +source: crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs +expression: transcript +--- +Transcript { + command: "weaver observe get-definition | jq -r '.[0].symbol' | weaver act refactor", + status: 0, + stdout: "{\"files_deleted\":0,\"files_written\":1,\"status\":\"ok\"}", + stderr: "", + requests: [ + Object { + "arguments": Array [ + String("--symbol"), + String("old_symbol"), + ], + "command": Object { + "domain": String("observe"), + "operation": String("get-definition"), + }, + }, + Object { + "arguments": Array [ + String("--provider"), + String("rope"), + String("--refactoring"), + String("rename"), + String("--file"), + String("src/main.py"), + String("new_name=renamed_symbol"), + String("offset=4"), + ], + "command": Object { + "domain": String("act"), + "operation": String("refactor"), + }, + }, + ], +} diff --git a/crates/weaver-plugin-rope/Cargo.toml b/crates/weaver-plugin-rope/Cargo.toml new file mode 100644 index 00000000..87186761 --- /dev/null +++ b/crates/weaver-plugin-rope/Cargo.toml @@ -0,0 +1,19 @@ +[package] +name = "weaver-plugin-rope" +edition.workspace = true +version.workspace = true + +[dependencies] +serde_json.workspace = true +tempfile.workspace = true +thiserror.workspace = true +weaver-plugins = { path = "../weaver-plugins" } + +[dev-dependencies] +mockall.workspace = true +rstest.workspace = true +rstest-bdd.workspace = true +rstest-bdd-macros.workspace = true + +[lints] +workspace = true diff --git a/crates/weaver-plugin-rope/src/lib.rs b/crates/weaver-plugin-rope/src/lib.rs new file mode 100644 index 00000000..355fb7f3 --- /dev/null +++ b/crates/weaver-plugin-rope/src/lib.rs @@ -0,0 +1,383 @@ +//! Rope-backed actuator plugin entrypoint and request dispatcher. +//! +//! This crate implements a one-shot plugin protocol handler compatible with +//! `weaver-plugins`. The plugin reads exactly one JSONL request from stdin, +//! executes a refactoring operation, and writes one JSONL response to stdout. + +#[cfg(test)] +mod tests; + +use std::collections::HashMap; +use std::io::{BufRead, Write}; +use std::path::{Component, Path, PathBuf}; +use std::process::Command; + +use tempfile::TempDir; +use thiserror::Error; +use weaver_plugins::protocol::{ + DiagnosticSeverity, FilePayload, PluginDiagnostic, PluginOutput, PluginRequest, PluginResponse, +}; + +const PYTHON_BINARY: &str = "python3"; +const PYTHON_RENAME_SCRIPT: &str = concat!( + "import os,sys\n", + "from rope.base.project import Project\n", + "from rope.refactor.rename import Rename\n", + "root, rel_path, offset_s, new_name = sys.argv[1:5]\n", + "offset = int(offset_s)\n", + "project = Project(root)\n", + "try:\n", + " resource = project.get_resource(rel_path)\n", + " renamer = Rename(project, resource, offset)\n", + " changes = renamer.get_changes(new_name)\n", + " project.do(changes)\n", + " with open(os.path.join(root, rel_path), 'r', encoding='utf-8') as handle:\n", + " sys.stdout.write(handle.read())\n", + "finally:\n", + " project.close()\n", +); + +/// Refactoring adapter abstraction used to keep behaviour deterministic in tests. +pub trait RopeAdapter { + /// Executes a rename operation and returns the modified file content. + /// + /// # Errors + /// + /// Returns an error if the adapter cannot complete the operation. + fn rename( + &self, + file: &FilePayload, + offset: usize, + new_name: &str, + ) -> Result; +} + +/// Adapter that delegates to the Python `rope` library. +pub struct PythonRopeAdapter; + +impl RopeAdapter for PythonRopeAdapter { + fn rename( + &self, + file: &FilePayload, + offset: usize, + new_name: &str, + ) -> Result { + let workspace = + TempDir::new().map_err(|source| RopeAdapterError::WorkspaceCreate { source })?; + write_workspace_file(workspace.path(), file.path(), file.content())?; + + let relative_path = path_to_slash(file.path()); + let mut command = Command::new(PYTHON_BINARY); + command.arg("-c"); + command.arg(PYTHON_RENAME_SCRIPT); + command.arg(workspace.path()); + command.arg(relative_path); + command.arg(offset.to_string()); + command.arg(new_name); + + let output = command + .output() + .map_err(|source| RopeAdapterError::Spawn { source })?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_owned(); + return Err(RopeAdapterError::EngineFailed { + message: if stderr.is_empty() { + String::from("python rope adapter failed without stderr output") + } else { + stderr + }, + }); + } + + let modified = + String::from_utf8(output.stdout).map_err(|source| RopeAdapterError::InvalidOutput { + message: source.to_string(), + })?; + + Ok(modified) + } +} + +/// Errors raised while dispatching plugin requests. +#[derive(Debug, Error)] +pub enum PluginDispatchError { + /// Writing the plugin response to stdout failed. + #[error("failed to write plugin response: {source}")] + Write { + /// Underlying I/O error. + #[source] + source: std::io::Error, + }, + /// Serializing the response payload failed. + #[error("failed to serialize plugin response: {source}")] + Serialize { + /// Underlying serialization error. + #[source] + source: serde_json::Error, + }, +} + +/// Errors raised by rope adapter implementations. +#[derive(Debug, Error)] +pub enum RopeAdapterError { + /// Temporary workspace allocation failed. + #[error("failed to create temporary workspace: {source}")] + WorkspaceCreate { + /// Underlying I/O error. + #[source] + source: std::io::Error, + }, + /// Writing request files to the temporary workspace failed. + #[error("failed to materialize workspace file '{}': {source}", path.display())] + WorkspaceWrite { + /// File path being written. + path: PathBuf, + /// Underlying I/O error. + #[source] + source: std::io::Error, + }, + /// Spawning the Python runtime failed. + #[error("failed to spawn python runtime: {source}")] + Spawn { + /// Underlying process spawn error. + #[source] + source: std::io::Error, + }, + /// The Python adapter completed with a non-zero status. + #[error("python rope adapter failed: {message}")] + EngineFailed { + /// Error message captured from stderr. + message: String, + }, + /// The adapter returned malformed output. + #[error("python rope adapter returned invalid output: {message}")] + InvalidOutput { + /// Parsing error details. + message: String, + }, + /// Request path was invalid for sandboxed execution. + #[error("invalid file path for rope operation: {message}")] + InvalidPath { + /// Validation message. + message: String, + }, +} + +/// Executes one plugin request from `stdin` and writes one response to `stdout`. +/// +/// # Errors +/// +/// Returns an error if the response cannot be serialized or written. +pub fn run_with_adapter( + 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(message) => failure_response(message), + }; + + let payload = serde_json::to_string(&response) + .map_err(|source| PluginDispatchError::Serialize { source })?; + stdout + .write_all(payload.as_bytes()) + .map_err(|source| PluginDispatchError::Write { source })?; + stdout + .write_all(b"\n") + .map_err(|source| PluginDispatchError::Write { source })?; + stdout + .flush() + .map_err(|source| PluginDispatchError::Write { source }) +} + +/// Executes one plugin request using the default Python-backed adapter. +/// +/// # Errors +/// +/// Returns an error if the response cannot be written. +pub fn run(stdin: &mut impl BufRead, stdout: &mut impl Write) -> Result<(), PluginDispatchError> { + run_with_adapter(stdin, stdout, &PythonRopeAdapter) +} + +fn read_request(stdin: &mut impl BufRead) -> Result { + let mut line = String::new(); + let bytes_read = stdin + .read_line(&mut line) + .map_err(|error| format!("failed to read request: {error}"))?; + + if bytes_read == 0 { + return Err(String::from("plugin request was empty")); + } + + serde_json::from_str(line.trim()) + .map_err(|error| format!("invalid plugin request JSON: {error}")) +} + +fn execute_request( + adapter: &R, + request: &PluginRequest, +) -> Result { + match request.operation() { + "rename" => execute_rename(adapter, request), + other => Err(format!("unsupported refactoring operation '{other}'")), + } +} + +fn execute_rename( + adapter: &R, + request: &PluginRequest, +) -> Result { + let (offset, new_name) = parse_rename_arguments(request.arguments())?; + + let files = request.files(); + let file = match files { + [single] => single, + other => { + return Err(format!( + "rename operation requires exactly one file payload, got {}", + other.len() + )); + } + }; + + validate_relative_path(file.path()).map_err(|error| error.to_string())?; + + 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, + })) +} + +fn parse_rename_arguments( + arguments: &HashMap, +) -> Result<(usize, String), String> { + let offset_value = arguments + .get("offset") + .ok_or_else(|| String::from("rename operation requires 'offset' argument"))?; + let new_name_value = arguments + .get("new_name") + .ok_or_else(|| String::from("rename operation requires 'new_name' argument"))?; + + let offset_string = json_value_to_string(offset_value) + .ok_or_else(|| String::from("offset argument must be a string or number"))?; + let offset = offset_string + .parse::() + .map_err(|error| format!("offset must be a non-negative integer: {error}"))?; + + let new_name = new_name_value + .as_str() + .ok_or_else(|| String::from("new_name argument must be a string"))?; + if new_name.trim().is_empty() { + return Err(String::from("new_name argument must not be empty")); + } + + Ok((offset, String::from(new_name))) +} + +fn json_value_to_string(value: &serde_json::Value) -> Option { + match value { + serde_json::Value::String(text) => Some(text.to_owned()), + serde_json::Value::Number(number) => Some(number.to_string()), + _ => None, + } +} + +fn write_workspace_file( + workspace_root: &Path, + relative_path: &Path, + content: &str, +) -> Result { + let absolute_path = workspace_root.join(relative_path); + + if let Some(parent) = absolute_path.parent() { + std::fs::create_dir_all(parent).map_err(|source| RopeAdapterError::WorkspaceWrite { + path: parent.to_path_buf(), + source, + })?; + } + + std::fs::write(&absolute_path, content).map_err(|source| RopeAdapterError::WorkspaceWrite { + path: absolute_path.clone(), + source, + })?; + + Ok(absolute_path) +} + +fn validate_relative_path(path: &Path) -> Result<(), RopeAdapterError> { + if path.is_absolute() { + return Err(RopeAdapterError::InvalidPath { + message: String::from("absolute paths are not allowed"), + }); + } + + let has_parent_traversal = path + .components() + .any(|component| matches!(component, Component::ParentDir)); + if has_parent_traversal { + return Err(RopeAdapterError::InvalidPath { + message: String::from("path traversal is not allowed"), + }); + } + + let has_windows_prefix = path + .components() + .any(|component| matches!(component, Component::Prefix(_))); + if has_windows_prefix { + return Err(RopeAdapterError::InvalidPath { + message: String::from("windows path prefixes are not allowed"), + }); + } + + Ok(()) +} + +fn build_search_replace_patch(path: &Path, original: &str, modified: &str) -> String { + let unix_path = path_to_slash(path); + let sep_after_original = if original.ends_with('\n') { "" } else { "\n" }; + let sep_after_modified = if modified.ends_with('\n') { "" } else { "\n" }; + + format!( + concat!( + "diff --git a/{unix_path} b/{unix_path}\n", + "<<<<<<< SEARCH\n", + "{original}{sep_a}", + "=======\n", + "{modified}{sep_b}", + ">>>>>>> REPLACE\n", + ), + unix_path = unix_path, + original = original, + sep_a = sep_after_original, + modified = modified, + sep_b = sep_after_modified, + ) +} + +fn path_to_slash(path: &Path) -> String { + path.components() + .filter_map(|component| match component { + Component::Normal(part) => Some(part.to_string_lossy().into_owned()), + _ => None, + }) + .collect::>() + .join("/") +} + +pub(crate) fn failure_response(message: String) -> PluginResponse { + PluginResponse::failure(vec![PluginDiagnostic::new( + DiagnosticSeverity::Error, + message, + )]) +} diff --git a/crates/weaver-plugin-rope/src/main.rs b/crates/weaver-plugin-rope/src/main.rs new file mode 100644 index 00000000..9ad469b2 --- /dev/null +++ b/crates/weaver-plugin-rope/src/main.rs @@ -0,0 +1,17 @@ +//! Binary entrypoint for the rope actuator plugin. + +use std::io::{self, BufReader, Write}; + +use weaver_plugin_rope::run; + +fn main() { + let stdin = io::stdin(); + let mut reader = BufReader::new(stdin.lock()); + let stdout = io::stdout(); + let mut writer = stdout.lock(); + + if let Err(error) = run(&mut reader, &mut writer) { + writeln!(io::stderr().lock(), "{error}").ok(); + std::process::exit(1); + } +} diff --git a/crates/weaver-plugin-rope/src/tests/behaviour.rs b/crates/weaver-plugin-rope/src/tests/behaviour.rs new file mode 100644 index 00000000..e6c541be --- /dev/null +++ b/crates/weaver-plugin-rope/src/tests/behaviour.rs @@ -0,0 +1,175 @@ +//! Behaviour-driven tests for rope plugin request dispatch. + +use std::collections::HashMap; +use std::path::PathBuf; + +use mockall::mock; +use rstest::fixture; +use rstest_bdd_macros::{given, scenario, then, when}; +use weaver_plugins::protocol::{ + DiagnosticSeverity, FilePayload, PluginOutput, PluginRequest, PluginResponse, +}; + +use crate::{RopeAdapter, RopeAdapterError, execute_request, failure_response}; + +#[derive(Default)] +struct World { + request: Option, + execute_result: Option>, + adapter_mode: AdapterMode, +} + +#[derive(Default, Clone, Copy)] +enum AdapterMode { + #[default] + Success, + NoChange, + Fails, +} + +#[fixture] +fn world() -> World { + World::default() +} + +mock! { + BehaviourAdapter {} + impl RopeAdapter for BehaviourAdapter { + fn rename( + &self, + file: &FilePayload, + offset: usize, + new_name: &str, + ) -> Result; + } +} + +fn should_invoke_rename(request: &PluginRequest) -> bool { + request.operation() == "rename" + && !request.files().is_empty() + && request.arguments().contains_key("offset") + && request.arguments().contains_key("new_name") +} + +fn configure_adapter_for_mode(adapter: &mut MockBehaviourAdapter, mode: AdapterMode) { + adapter.expect_rename().once().returning( + move |file: &FilePayload, _offset: usize, _new_name: &str| match mode { + AdapterMode::Success => Ok(file.content().replace("old_name", "new_name")), + AdapterMode::NoChange => Ok(file.content().to_owned()), + AdapterMode::Fails => Err(RopeAdapterError::EngineFailed { + message: String::from("rope engine failed"), + }), + }, + ); +} + +fn build_request(operation: &str, with_offset: bool, with_new_name: bool) -> PluginRequest { + let mut arguments = HashMap::new(); + if with_offset { + arguments.insert( + String::from("offset"), + serde_json::Value::String(String::from("4")), + ); + } + if with_new_name { + arguments.insert( + String::from("new_name"), + serde_json::Value::String(String::from("new_name")), + ); + } + + PluginRequest::with_arguments( + operation, + vec![FilePayload::new( + PathBuf::from("src/main.py"), + "def old_name():\n return 1\n", + )], + arguments, + ) +} + +#[given("a rename request with required arguments")] +fn given_valid_rename(world: &mut World) { + world.request = Some(build_request("rename", true, true)); +} + +#[given("a rename request missing offset")] +fn given_missing_offset(world: &mut World) { + world.request = Some(build_request("rename", false, true)); +} + +#[given("an unsupported extract method request")] +fn given_unsupported_operation(world: &mut World) { + world.request = Some(build_request("extract_method", true, true)); +} + +#[given("a rope adapter that fails")] +fn given_failing_adapter(world: &mut World) { + world.adapter_mode = AdapterMode::Fails; +} + +#[given("a rope adapter that returns unchanged content")] +fn given_no_change_adapter(world: &mut World) { + world.adapter_mode = AdapterMode::NoChange; +} + +#[when("the plugin executes the request")] +fn when_execute(world: &mut World) { + let request = world.request.as_ref().expect("request should be present"); + let mut adapter = MockBehaviourAdapter::new(); + if should_invoke_rename(request) { + configure_adapter_for_mode(&mut adapter, world.adapter_mode); + } + world.execute_result = Some(execute_request(&adapter, request)); +} + +/// Resolves the world's execute result to a `PluginResponse`, converting +/// `Err` outcomes to failure responses for assertion consistency. +fn resolved_response(world: &World) -> PluginResponse { + match world + .execute_result + .as_ref() + .expect("execute result should be present") + { + Ok(resp) => resp.clone(), + Err(msg) => failure_response(msg.clone()), + } +} + +#[then("the plugin returns successful diff output")] +fn then_successful_diff(world: &mut World) { + let response = resolved_response(world); + assert!(response.is_success()); + assert!(matches!(response.output(), PluginOutput::Diff { .. })); +} + +#[then("the plugin returns failure diagnostics")] +fn then_failure_diagnostics(world: &mut World) { + let response = resolved_response(world); + assert!(!response.is_success()); + assert_eq!(response.output(), &PluginOutput::Empty); + assert!( + response + .diagnostics() + .iter() + .any(|diag| diag.severity() == DiagnosticSeverity::Error) + ); +} + +#[then("the failure message contains {text}")] +fn then_failure_contains(world: &mut World, text: String) { + let needle = text.trim_matches('"'); + let response = resolved_response(world); + let diagnostics = response.diagnostics(); + assert!( + diagnostics + .iter() + .any(|diagnostic| diagnostic.message().contains(needle)), + "expected diagnostics to contain '{needle}', got: {diagnostics:?}", + ); +} + +#[scenario(path = "tests/features/rope_plugin.feature")] +fn rope_plugin_behaviour(world: World) { + let _ = world; +} diff --git a/crates/weaver-plugin-rope/src/tests/mod.rs b/crates/weaver-plugin-rope/src/tests/mod.rs new file mode 100644 index 00000000..0c2e284a --- /dev/null +++ b/crates/weaver-plugin-rope/src/tests/mod.rs @@ -0,0 +1,223 @@ +//! Unit and behavioural tests for the rope actuator plugin. + +mod behaviour; + +use std::collections::HashMap; +use std::path::PathBuf; + +use mockall::mock; +use rstest::{fixture, rstest}; +use weaver_plugins::protocol::{FilePayload, PluginOutput, PluginRequest}; + +use crate::{RopeAdapter, RopeAdapterError, execute_request, run_with_adapter}; + +mock! { + Adapter {} + impl RopeAdapter for Adapter { + fn rename( + &self, + file: &FilePayload, + offset: usize, + new_name: &str, + ) -> Result; + } +} + +/// Builds a `MockAdapter` that expects a single rename call returning `result`. +fn adapter_returning(result: Result) -> MockAdapter { + let mut adapter = MockAdapter::new(); + adapter + .expect_rename() + .once() + .return_once(move |_file, _offset, _new_name| result); + adapter +} + +/// Builds a `MockAdapter` where rename is never expected. +fn adapter_unused() -> MockAdapter { + MockAdapter::new() +} + +#[fixture] +fn rename_arguments() -> HashMap { + let mut arguments = HashMap::new(); + arguments.insert( + String::from("offset"), + serde_json::Value::String(String::from("4")), + ); + arguments.insert( + String::from("new_name"), + serde_json::Value::String(String::from("new_name")), + ); + arguments +} + +fn request_with_args(arguments: HashMap) -> PluginRequest { + PluginRequest::with_arguments( + "rename", + vec![FilePayload::new( + PathBuf::from("src/main.py"), + "def old_name():\n return 1\n", + )], + arguments, + ) +} + +#[rstest] +fn rename_success_returns_diff_output(rename_arguments: HashMap) { + let adapter = adapter_returning(Ok(String::from("def new_name():\n return 1\n"))); + + let response = execute_request(&adapter, &request_with_args(rename_arguments)) + .expect("execute_request should succeed"); + assert!(response.is_success()); + assert!(matches!(response.output(), PluginOutput::Diff { .. })); +} + +fn remove_offset(arguments: &mut HashMap) { + arguments.remove("offset"); +} + +fn set_boolean_offset(arguments: &mut HashMap) { + arguments.insert(String::from("offset"), serde_json::Value::Bool(true)); +} + +fn set_negative_offset(arguments: &mut HashMap) { + arguments.insert( + String::from("offset"), + serde_json::Value::String(String::from("-1")), + ); +} + +fn set_numeric_offset(arguments: &mut HashMap) { + arguments.insert( + String::from("offset"), + serde_json::Value::Number(serde_json::Number::from(4)), + ); +} + +fn set_empty_new_name(arguments: &mut HashMap) { + arguments.insert( + String::from("new_name"), + serde_json::Value::String(String::from(" ")), + ); +} + +fn set_numeric_new_name(arguments: &mut HashMap) { + arguments.insert( + String::from("new_name"), + serde_json::Value::Number(serde_json::Number::from(42)), + ); +} + +#[rstest] +#[case::missing_offset(remove_offset as fn(&mut _), Some("offset"))] +#[case::boolean_offset(set_boolean_offset as fn(&mut _), Some("offset"))] +#[case::negative_offset(set_negative_offset as fn(&mut _), Some("non-negative integer"))] +#[case::numeric_offset_succeeds(set_numeric_offset as fn(&mut _), None)] +#[case::numeric_new_name(set_numeric_new_name as fn(&mut _), Some("new_name argument must be a string"))] +#[case::empty_new_name(set_empty_new_name as fn(&mut _), Some("new_name"))] +fn rename_argument_validation( + #[case] mutate: fn(&mut HashMap), + #[case] expected_error: Option<&str>, + mut rename_arguments: HashMap, +) { + mutate(&mut rename_arguments); + + if let Some(needle) = expected_error { + let adapter = adapter_unused(); + let err = execute_request(&adapter, &request_with_args(rename_arguments)) + .expect_err("invalid arguments should fail"); + assert!( + err.contains(needle), + "expected error mentioning '{needle}', got: {err}" + ); + } else { + let adapter = adapter_returning(Ok(String::from("def new_name():\n return 1\n"))); + let response = execute_request(&adapter, &request_with_args(rename_arguments)) + .expect("valid arguments should succeed"); + assert!(response.is_success()); + } +} + +#[test] +fn unsupported_operation_returns_error() { + let adapter = adapter_unused(); + let request = PluginRequest::new("extract_method", Vec::new()); + + let err = execute_request(&adapter, &request).expect_err("unsupported operation should fail"); + assert!( + err.contains("unsupported"), + "expected error mentioning 'unsupported', got: {err}" + ); +} + +enum FailureScenario { + NoChange, + AdapterError, +} + +#[rstest] +#[case::no_change(FailureScenario::NoChange)] +#[case::adapter_error(FailureScenario::AdapterError)] +fn rename_non_mutating_or_error_returns_failure( + #[case] scenario: FailureScenario, + rename_arguments: HashMap, +) { + let adapter = match &scenario { + FailureScenario::AdapterError => adapter_returning(Err(RopeAdapterError::EngineFailed { + message: String::from("rope failed"), + })), + FailureScenario::NoChange => { + adapter_returning(Ok(String::from("def old_name():\n return 1\n"))) + } + }; + + let err = execute_request(&adapter, &request_with_args(rename_arguments)) + .expect_err("failure scenario should return Err"); + + match scenario { + FailureScenario::NoChange => assert!( + err.contains("no content changes"), + "expected no-change diagnostic, got: {err}" + ), + FailureScenario::AdapterError => assert!( + err.contains("rope failed"), + "expected adapter error message, got: {err}" + ), + } +} + +// --------------------------------------------------------------------------- +// stdin/stdout dispatch layer tests (run_with_adapter) +// --------------------------------------------------------------------------- + +fn valid_request_json() -> String { + let request = request_with_args(rename_arguments()); + serde_json::to_string(&request).expect("serialize request") +} + +/// Dispatches `input` through `run_with_adapter` and parses the response. +fn dispatch_stdin(input: &[u8], adapter: &MockAdapter) -> weaver_plugins::protocol::PluginResponse { + let mut stdin = std::io::Cursor::new(input.to_vec()); + let mut stdout = Vec::new(); + run_with_adapter(&mut stdin, &mut stdout, adapter).expect("dispatch should succeed"); + let output = String::from_utf8(stdout).expect("utf8 stdout"); + serde_json::from_str(output.trim()).expect("parse response") +} + +#[rstest] +#[case::success( + format!("{}\n", valid_request_json()).into_bytes(), + adapter_returning(Ok(String::from("def new_name():\n return 1\n"))), + true +)] +#[case::empty_stdin(Vec::new(), adapter_unused(), false)] +#[case::invalid_json(b"not valid json\n".to_vec(), adapter_unused(), false)] +fn run_with_adapter_dispatch_layer( + #[case] input: Vec, + #[case] adapter: MockAdapter, + #[case] expect_success: bool, +) { + let response = dispatch_stdin(&input, &adapter); + assert_eq!(response.is_success(), expect_success); +} diff --git a/crates/weaver-plugin-rope/tests/features/rope_plugin.feature b/crates/weaver-plugin-rope/tests/features/rope_plugin.feature new file mode 100644 index 00000000..928fe2b8 --- /dev/null +++ b/crates/weaver-plugin-rope/tests/features/rope_plugin.feature @@ -0,0 +1,32 @@ +Feature: Rope actuator plugin + + Scenario: Rename succeeds with diff output + Given a rename request with required arguments + When the plugin executes the request + Then the plugin returns successful diff output + + Scenario: Rename fails when offset is missing + Given a rename request missing offset + When the plugin executes the request + Then the plugin returns failure diagnostics + And the failure message contains "offset" + + Scenario: Unsupported operation fails with diagnostics + Given an unsupported extract method request + When the plugin executes the request + Then the plugin returns failure diagnostics + And the failure message contains "unsupported" + + Scenario: Adapter failures are surfaced + Given a rename request with required arguments + And a rope adapter that fails + When the plugin executes the request + Then the plugin returns failure diagnostics + And the failure message contains "rope engine failed" + + Scenario: Unchanged output is treated as failure + Given a rename request with required arguments + And a rope adapter that returns unchanged content + When the plugin executes the request + Then the plugin returns failure diagnostics + And the failure message contains "no content changes" diff --git a/crates/weaverd/Cargo.toml b/crates/weaverd/Cargo.toml index 0ec8b0d8..2595b0b4 100644 --- a/crates/weaverd/Cargo.toml +++ b/crates/weaverd/Cargo.toml @@ -25,6 +25,7 @@ tempfile.workspace = true [dev-dependencies] derive_more = { version = "1.0", features = ["as_ref", "deref"] } +mockall.workspace = true rstest.workspace = true rstest-bdd.workspace = true rstest-bdd-macros.workspace = true diff --git a/crates/weaverd/src/dispatch/act/refactor/behaviour.rs b/crates/weaverd/src/dispatch/act/refactor/behaviour.rs new file mode 100644 index 00000000..e8061380 --- /dev/null +++ b/crates/weaverd/src/dispatch/act/refactor/behaviour.rs @@ -0,0 +1,259 @@ +//! Behavioural tests for the `act refactor` handler. + +use std::path::PathBuf; + +use mockall::mock; +use rstest::fixture; +use rstest_bdd_macros::{given, scenario, then, when}; +use tempfile::TempDir; +use weaver_config::{CapabilityMatrix, Config, SocketEndpoint}; +use weaver_plugins::{PluginError, PluginOutput, PluginRequest, PluginResponse}; + +use super::*; + +const ORIGINAL_CONTENT: &str = "hello world\n"; +const UPDATED_CONTENT: &str = "hello woven\n"; +const VALID_DIFF: &str = concat!( + "diff --git a/notes.txt b/notes.txt\n", + "<<<<<<< SEARCH\n", + "hello world\n", + "=======\n", + "hello woven\n", + ">>>>>>> REPLACE\n", +); +const MALFORMED_DIFF: &str = concat!( + "diff --git a/notes.txt\n", + "<<<<<<< SEARCH\n", + "hello world\n", + "=======\n", + "hello woven\n", + ">>>>>>> REPLACE\n", +); + +#[derive(Clone, Copy, Default)] +enum RuntimeMode { + #[default] + DiffSuccess, + RuntimeError, + MalformedDiff, +} + +mock! { + Runtime {} + impl RefactorPluginRuntime for Runtime { + fn execute( + &self, + provider: &str, + request: &PluginRequest, + ) -> Result; + } +} + +const REQUIRED_FLAGS: &[&str] = &["--provider", "--refactoring", "--file"]; + +fn request_has_required_flags(request: &CommandRequest) -> bool { + REQUIRED_FLAGS + .iter() + .all(|flag| request.arguments.iter().any(|argument| argument == flag)) +} + +fn configure_runtime_for_mode(runtime: &mut MockRuntime, mode: RuntimeMode) { + runtime + .expect_execute() + .once() + .returning( + move |_provider: &str, _request: &PluginRequest| match 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), + })), + }, + ); +} + +struct RefactorWorld { + workspace: TempDir, + socket_dir: TempDir, + request: CommandRequest, + runtime_mode: RuntimeMode, + dispatch_result: Option>, + response_stream: String, +} + +impl RefactorWorld { + fn new() -> Self { + Self { + workspace: TempDir::new().expect("workspace"), + socket_dir: TempDir::new().expect("socket dir"), + request: command_request(vec![ + String::from("--provider"), + String::from("rope"), + String::from("--refactoring"), + String::from("rename"), + String::from("--file"), + String::from("notes.txt"), + ]), + runtime_mode: RuntimeMode::DiffSuccess, + dispatch_result: None, + response_stream: String::new(), + } + } + + fn path(&self, relative: &str) -> PathBuf { + self.workspace.path().join(relative) + } + + fn write_file(&self, relative: &str, content: &str) { + std::fs::write(self.path(relative), content).expect("write file"); + } + + fn read_file(&self, relative: &str) -> String { + std::fs::read_to_string(self.path(relative)).expect("read file") + } + + fn execute(&mut self) { + let mut runtime = MockRuntime::new(); + if request_has_required_flags(&self.request) { + configure_runtime_for_mode(&mut runtime, self.runtime_mode); + } + let mut output = Vec::new(); + let mut writer = ResponseWriter::new(&mut output); + let socket_path = self.socket_dir.path().join("socket.sock"); + let mut backends = build_backends(&socket_path); + let result = handle( + &self.request, + &mut writer, + RefactorContext { + backends: &mut backends, + workspace_root: self.workspace.path(), + runtime: &runtime, + }, + ) + .map(|dispatch| dispatch.status); + + self.dispatch_result = Some(result); + self.response_stream = String::from_utf8(output).expect("response utf8"); + } +} + +fn command_request(arguments: Vec) -> CommandRequest { + CommandRequest { + command: CommandDescriptor { + domain: String::from("act"), + operation: String::from("refactor"), + }, + arguments, + patch: None, + } +} + +fn build_backends(socket_path: &std::path::Path) -> FusionBackends { + let config = Config { + daemon_socket: SocketEndpoint::unix(socket_path.to_string_lossy().as_ref()), + ..Config::default() + }; + let provider = SemanticBackendProvider::new(CapabilityMatrix::default()); + FusionBackends::new(config, provider) +} + +#[fixture] +fn world() -> RefactorWorld { + RefactorWorld::new() +} + +#[given("a workspace file for refactoring")] +fn given_workspace_file(world: &mut RefactorWorld) { + world.write_file("notes.txt", ORIGINAL_CONTENT); +} + +#[given("a valid act refactor request")] +fn given_valid_request(world: &mut RefactorWorld) { + world.request = command_request(vec![ + String::from("--provider"), + String::from("rope"), + String::from("--refactoring"), + String::from("rename"), + String::from("--file"), + String::from("notes.txt"), + String::from("offset=1"), + String::from("new_name=woven"), + ]); +} + +#[given("a refactor request missing provider")] +fn given_missing_provider_request(world: &mut RefactorWorld) { + world.request = command_request(vec![ + String::from("--refactoring"), + String::from("rename"), + String::from("--file"), + String::from("notes.txt"), + ]); +} + +#[given("a runtime error from the refactor plugin")] +fn given_runtime_error(world: &mut RefactorWorld) { + world.runtime_mode = RuntimeMode::RuntimeError; +} + +#[given("a malformed diff response from the refactor plugin")] +fn given_malformed_diff(world: &mut RefactorWorld) { + world.runtime_mode = RuntimeMode::MalformedDiff; +} + +#[when("the act refactor command executes")] +fn when_refactor_executes(world: &mut RefactorWorld) { + world.execute(); +} + +#[then("the refactor command succeeds")] +fn then_refactor_succeeds(world: &mut RefactorWorld) { + let result = world.dispatch_result.as_ref().expect("result missing"); + let status = result.as_ref().expect("status should be present"); + assert_eq!(*status, 0); +} + +#[then("the refactor command fails with status 1")] +fn then_refactor_fails_status_one(world: &mut RefactorWorld) { + let result = world.dispatch_result.as_ref().expect("result missing"); + let status = result.as_ref().expect("status should be present"); + assert_eq!(*status, 1); +} + +#[then("the refactor command is rejected as invalid arguments")] +fn then_refactor_rejected_invalid_arguments(world: &mut RefactorWorld) { + let result = world.dispatch_result.as_ref().expect("result missing"); + assert!(matches!( + result, + Err(DispatchError::InvalidArguments { .. }) + )); +} + +#[then("the target file is updated")] +fn then_target_file_updated(world: &mut RefactorWorld) { + let updated = world.read_file("notes.txt"); + assert_eq!(updated, UPDATED_CONTENT); +} + +#[then("the target file is unchanged")] +fn then_target_file_unchanged(world: &mut RefactorWorld) { + let updated = world.read_file("notes.txt"); + assert_eq!(updated, ORIGINAL_CONTENT); +} + +#[then("the stderr stream contains {text}")] +fn then_stderr_contains(world: &mut RefactorWorld, text: String) { + let needle = text.trim_matches('"'); + assert!( + world.response_stream.contains(needle), + "expected response stream to contain '{needle}', got: {}", + world.response_stream + ); +} + +#[scenario(path = "tests/features/refactor.feature")] +fn refactor_behaviour(#[from(world)] _world: RefactorWorld) {} diff --git a/crates/weaverd/src/dispatch/act/refactor/mod.rs b/crates/weaverd/src/dispatch/act/refactor/mod.rs index a43fea62..eb99a699 100644 --- a/crates/weaverd/src/dispatch/act/refactor/mod.rs +++ b/crates/weaverd/src/dispatch/act/refactor/mod.rs @@ -5,40 +5,158 @@ //! through the Double-Lock safety harness before any filesystem change is //! committed. //! -//! In this initial Phase 3.1.1 implementation the handler validates the -//! request arguments, resolves the target file, and builds a -//! [`PluginRequest`]. Full plugin execution and safety harness integration -//! will be wired in Phase 3.2 when the daemon runtime holds a -//! [`PluginRunner`] instance. +//! The handler validates the request arguments, resolves the target file, and +//! builds a [`PluginRequest`]. Successful plugin responses with diff output are +//! forwarded to the existing `act apply-patch` pipeline so syntactic and +//! semantic locks are reused without duplicating safety-critical logic. +use std::ffi::OsString; use std::io::Write; use std::path::Path; +use std::path::PathBuf; +use std::sync::Arc; use tracing::debug; -use weaver_plugins::PluginRequest; +use weaver_plugins::manifest::{PluginKind, PluginManifest, PluginMetadata}; +use weaver_plugins::process::SandboxExecutor; use weaver_plugins::protocol::FilePayload; +use weaver_plugins::runner::PluginRunner; +use weaver_plugins::{PluginError, PluginOutput, PluginRegistry, PluginRequest, PluginResponse}; use crate::backends::{BackendKind, FusionBackends}; +use crate::dispatch::act::apply_patch; use crate::dispatch::errors::DispatchError; -use crate::dispatch::request::CommandRequest; +use crate::dispatch::request::{CommandDescriptor, CommandRequest}; use crate::dispatch::response::ResponseWriter; use crate::dispatch::router::{DISPATCH_TARGET, DispatchResult}; use crate::semantic_provider::SemanticBackendProvider; +/// Environment variable overriding the rope plugin executable path. +pub(crate) const ROPE_PLUGIN_PATH_ENV: &str = "WEAVER_ROPE_PLUGIN_PATH"; +const DEFAULT_ROPE_PLUGIN_PATH: &str = "/usr/bin/weaver-plugin-rope"; +const ROPE_PLUGIN_NAME: &str = "rope"; +const ROPE_PLUGIN_VERSION: &str = "0.1.0"; + +/// Runtime abstraction for executing refactor plugins. +pub(crate) trait RefactorPluginRuntime { + /// Executes the named plugin with the provided request. + fn execute( + &self, + provider: &str, + request: &PluginRequest, + ) -> Result; +} + +/// Sandbox-backed runtime that resolves plugins from a registry. +pub(crate) struct SandboxRefactorRuntime { + runner: PluginRunner, +} + +impl SandboxRefactorRuntime { + /// Builds the runtime from environment configuration. + /// + /// # Errors + /// + /// Returns an error description if plugin registration fails. + pub fn from_environment() -> Result { + 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(|error| format!("failed to initialize refactor runtime: {error}"))?; + + Ok(Self { + runner: PluginRunner::new(registry, SandboxExecutor), + }) + } +} + +impl RefactorPluginRuntime for SandboxRefactorRuntime { + fn execute( + &self, + provider: &str, + request: &PluginRequest, + ) -> Result { + self.runner.execute(provider, request) + } +} + +/// Runtime that reports an initialization error on every execution attempt. +struct NoopRefactorRuntime { + message: String, +} + +impl RefactorPluginRuntime for NoopRefactorRuntime { + fn execute( + &self, + _provider: &str, + _request: &PluginRequest, + ) -> Result { + Err(PluginError::Manifest { + message: self.message.clone(), + }) + } +} + +/// Constructs the default refactor plugin runtime for daemon dispatch. +#[must_use] +pub(crate) fn default_runtime() -> Arc { + match SandboxRefactorRuntime::from_environment() { + Ok(runtime) => Arc::new(runtime), + Err(message) => Arc::new(NoopRefactorRuntime { message }), + } +} + +/// Converts an optional executable override to an absolute plugin path. +fn resolve_rope_plugin_path(raw_override: Option) -> PathBuf { + let candidate = raw_override + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from(DEFAULT_ROPE_PLUGIN_PATH)); + if candidate.is_absolute() { + return candidate; + } + + match std::env::current_dir() { + Ok(cwd) => cwd.join(candidate), + Err(error) => { + tracing::warn!( + target: DISPATCH_TARGET, + path = %candidate.display(), + %error, + "cannot resolve relative plugin path against working directory; using path as-is" + ); + candidate + } + } +} + +/// Context for executing refactor operations. +pub(crate) struct RefactorContext<'a> { + /// Mutable reference to the fusion backends for starting semantic services. + pub backends: &'a mut FusionBackends, + /// Root directory of the workspace being refactored. + pub workspace_root: &'a Path, + /// Runtime used to execute the refactor plugin process. + pub runtime: &'a dyn RefactorPluginRuntime, +} + /// Handles `act refactor` requests. /// /// Expects `--provider ` and `--refactoring ` in the /// request arguments, plus `--file ` identifying the target file. /// -/// The handler reads the file content, builds a [`PluginRequest`], and will -/// execute the plugin via a [`PluginRunner`] once the daemon runtime holds -/// a plugin registry (Phase 3.2). +/// The handler reads the file content, executes the plugin, and forwards +/// successful diff output through `act apply-patch` for Double-Lock +/// verification and atomic commit. pub fn handle( request: &CommandRequest, writer: &mut ResponseWriter, - backends: &mut FusionBackends, - workspace_root: &Path, + context: RefactorContext<'_>, ) -> Result { let args = parse_refactor_args(&request.arguments)?; @@ -50,17 +168,17 @@ pub fn handle( "handling act refactor" ); - backends + context + .backends .ensure_started(BackendKind::Semantic) .map_err(DispatchError::backend_startup)?; // Resolve the target file within the workspace. - let file_path = resolve_file(workspace_root, &args.file)?; + let file_path = resolve_file(context.workspace_root, &args.file)?; let file_content = std::fs::read_to_string(&file_path).map_err(|err| { DispatchError::invalid_arguments(format!("cannot read file '{}': {err}", args.file)) })?; - // Build the plugin request with in-band file content. let mut plugin_args = std::collections::HashMap::new(); plugin_args.insert( "refactoring".into(), @@ -77,21 +195,24 @@ pub fn handle( } } - let _plugin_request = PluginRequest::with_arguments( + let plugin_request = PluginRequest::with_arguments( &args.refactoring, - vec![FilePayload::new(file_path, file_content)], + vec![FilePayload::new(PathBuf::from(&args.file), file_content)], plugin_args, ); - // Phase 3.2 will wire in PluginRunner::execute() here. - // For now return "not yet available" so the operation is routed correctly - // but does not attempt execution without a runtime registry. - writer.write_stderr(format!( - "act refactor: plugin execution not yet available \ - (provider={}, refactoring={}, file={})\n", - args.provider, args.refactoring, args.file - ))?; - Ok(DispatchResult::with_status(1)) + match context.runtime.execute(&args.provider, &plugin_request) { + Ok(response) => { + handle_plugin_response(response, writer, context.backends, context.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)) + } + } } // --------------------------------------------------------------------------- @@ -201,3 +322,59 @@ fn resolve_file(workspace_root: &Path, file: &str) -> Result( + response: PluginResponse, + writer: &mut ResponseWriter, + backends: &mut FusionBackends, + workspace_root: &Path, +) -> Result { + if !response.is_success() { + let diagnostics: Vec = response + .diagnostics() + .iter() + .map(|diag| diag.message().to_owned()) + .collect(); + let message = if diagnostics.is_empty() { + String::from("plugin reported failure without diagnostics") + } else { + diagnostics.join("; ") + }; + writer.write_stderr(format!("act refactor failed: {message}\n"))?; + return Ok(DispatchResult::with_status(1)); + } + + match response.output() { + PluginOutput::Diff { content } => { + forward_diff_to_apply_patch(content, writer, backends, workspace_root) + } + PluginOutput::Analysis { .. } | PluginOutput::Empty => { + writer.write_stderr( + "act refactor failed: plugin succeeded but did not return diff output\n", + )?; + Ok(DispatchResult::with_status(1)) + } + } +} + +fn forward_diff_to_apply_patch( + patch: &str, + writer: &mut ResponseWriter, + backends: &mut FusionBackends, + workspace_root: &Path, +) -> Result { + let patch_request = CommandRequest { + command: CommandDescriptor { + domain: String::from("act"), + operation: String::from("apply-patch"), + }, + arguments: Vec::new(), + patch: Some(patch.to_owned()), + }; + apply_patch::handle(&patch_request, writer, backends, workspace_root) +} + +#[cfg(test)] +mod behaviour; +#[cfg(test)] +mod tests; diff --git a/crates/weaverd/src/dispatch/act/refactor/tests.rs b/crates/weaverd/src/dispatch/act/refactor/tests.rs new file mode 100644 index 00000000..6160e727 --- /dev/null +++ b/crates/weaverd/src/dispatch/act/refactor/tests.rs @@ -0,0 +1,240 @@ +//! Unit tests for the `act refactor` handler. + +use std::path::Path; + +use rstest::{fixture, rstest}; +use tempfile::TempDir; +use weaver_config::{CapabilityMatrix, Config, SocketEndpoint}; +use weaver_plugins::{PluginError, PluginOutput, PluginRequest, PluginResponse}; + +use super::{ + DispatchError, FusionBackends, RefactorContext, RefactorPluginRuntime, ResponseWriter, + default_runtime, handle, resolve_rope_plugin_path, +}; +use crate::dispatch::request::{CommandDescriptor, CommandRequest}; +use crate::semantic_provider::SemanticBackendProvider; + +enum MockRuntimeResult { + Success(PluginResponse), + NotFound(String), +} + +struct MockRuntime { + result: MockRuntimeResult, +} + +impl RefactorPluginRuntime for MockRuntime { + fn execute( + &self, + _provider: &str, + _request: &PluginRequest, + ) -> Result { + match &self.result { + MockRuntimeResult::Success(response) => Ok(response.clone()), + MockRuntimeResult::NotFound(name) => Err(PluginError::NotFound { name: name.clone() }), + } + } +} + +fn command_request(arguments: Vec) -> CommandRequest { + CommandRequest { + command: CommandDescriptor { + domain: String::from("act"), + operation: String::from("refactor"), + }, + arguments, + patch: None, + } +} + +#[fixture] +fn socket_dir() -> TempDir { + TempDir::new().expect("socket dir") +} + +fn build_backends(socket_path: &Path) -> FusionBackends { + let config = Config { + daemon_socket: SocketEndpoint::unix(socket_path.to_string_lossy().as_ref()), + ..Config::default() + }; + let provider = SemanticBackendProvider::new(CapabilityMatrix::default()); + FusionBackends::new(config, provider) +} + +#[rstest] +fn handle_returns_error_for_missing_provider(socket_dir: TempDir) { + let request = command_request(vec![ + String::from("--refactoring"), + String::from("rename"), + String::from("--file"), + String::from("notes.txt"), + ]); + let socket_path = socket_dir.path().join("socket.sock"); + let mut backends = build_backends(&socket_path); + let mut output = Vec::new(); + let mut writer = ResponseWriter::new(&mut output); + let runtime = MockRuntime { + result: MockRuntimeResult::NotFound(String::from("rope")), + }; + + let result = handle( + &request, + &mut writer, + RefactorContext { + backends: &mut backends, + workspace_root: Path::new("/tmp/workspace"), + runtime: &runtime, + }, + ); + + assert!(matches!( + result, + Err(DispatchError::InvalidArguments { .. }) + )); +} + +#[rstest] +fn handle_runtime_error_returns_status_one(socket_dir: TempDir) { + let workspace = TempDir::new().expect("workspace"); + let file_path = workspace.path().join("notes.txt"); + std::fs::write(&file_path, "hello\n").expect("write"); + + let request = command_request(vec![ + String::from("--provider"), + String::from("rope"), + String::from("--refactoring"), + String::from("rename"), + String::from("--file"), + String::from("notes.txt"), + ]); + let runtime = MockRuntime { + result: MockRuntimeResult::NotFound(String::from("rope")), + }; + let socket_path = socket_dir.path().join("socket.sock"); + let mut backends = build_backends(&socket_path); + let mut output = Vec::new(); + let mut writer = ResponseWriter::new(&mut output); + + let result = handle( + &request, + &mut writer, + RefactorContext { + backends: &mut backends, + workspace_root: workspace.path(), + runtime: &runtime, + }, + ) + .expect("dispatch result"); + + assert_eq!(result.status, 1); + let stderr = String::from_utf8(output).expect("stderr utf8"); + assert!(stderr.contains("act refactor failed")); +} + +#[rstest] +#[case::analysis(PluginOutput::Analysis { data: serde_json::json!({"k": "v"}) })] +#[case::empty(PluginOutput::Empty)] +fn handle_non_diff_output_returns_status_one( + #[case] output_variant: PluginOutput, + socket_dir: TempDir, +) { + let workspace = TempDir::new().expect("workspace"); + let file_path = workspace.path().join("notes.txt"); + std::fs::write(&file_path, "hello\n").expect("write"); + + let request = command_request(vec![ + String::from("--provider"), + String::from("rope"), + String::from("--refactoring"), + String::from("rename"), + String::from("--file"), + String::from("notes.txt"), + ]); + let runtime = MockRuntime { + result: MockRuntimeResult::Success(PluginResponse::success(output_variant)), + }; + let socket_path = socket_dir.path().join("socket.sock"); + let mut backends = build_backends(&socket_path); + let mut output = Vec::new(); + let mut writer = ResponseWriter::new(&mut output); + + let result = handle( + &request, + &mut writer, + RefactorContext { + backends: &mut backends, + workspace_root: workspace.path(), + runtime: &runtime, + }, + ) + .expect("dispatch result"); + + assert_eq!(result.status, 1); + let stderr = String::from_utf8(output).expect("stderr utf8"); + assert!(stderr.contains("did not return diff output")); +} + +#[rstest] +fn handle_diff_output_applies_patch_through_apply_patch_pipeline(socket_dir: TempDir) { + let workspace = TempDir::new().expect("workspace"); + let relative_file = String::from("notes.txt"); + let file_path = workspace.path().join(&relative_file); + std::fs::write(&file_path, "hello world\n").expect("write"); + + let diff = concat!( + "diff --git a/notes.txt b/notes.txt\n", + "<<<<<<< SEARCH\n", + "hello world\n", + "=======\n", + "hello woven\n", + ">>>>>>> REPLACE\n", + ); + let runtime = MockRuntime { + result: MockRuntimeResult::Success(PluginResponse::success(PluginOutput::Diff { + content: String::from(diff), + })), + }; + let request = command_request(vec![ + String::from("--provider"), + String::from("rope"), + String::from("--refactoring"), + String::from("rename"), + String::from("--file"), + relative_file.clone(), + ]); + let socket_path = socket_dir.path().join("socket.sock"); + let mut backends = build_backends(&socket_path); + let mut output = Vec::new(); + let mut writer = ResponseWriter::new(&mut output); + + let result = handle( + &request, + &mut writer, + RefactorContext { + backends: &mut backends, + workspace_root: workspace.path(), + runtime: &runtime, + }, + ) + .expect("dispatch result"); + + assert_eq!(result.status, 0); + let updated = std::fs::read_to_string(workspace.path().join(relative_file)).expect("read"); + assert_eq!(updated, "hello woven\n"); + let stdout = String::from_utf8(output).expect("stdout utf8"); + assert!(stdout.contains("\"kind\":\"stream\"")); +} + +#[test] +fn resolve_rope_plugin_path_makes_relative_overrides_absolute() { + let path = resolve_rope_plugin_path(Some(std::ffi::OsString::from("bin/rope"))); + assert!(path.is_absolute()); +} + +#[test] +fn default_runtime_returns_shared_trait_object() { + let runtime = default_runtime(); + let request = PluginRequest::new("rename", Vec::new()); + let result = runtime.execute("rope", &request); + assert!(result.is_err()); +} diff --git a/crates/weaverd/src/dispatch/router.rs b/crates/weaverd/src/dispatch/router.rs index 0093d070..c7479937 100644 --- a/crates/weaverd/src/dispatch/router.rs +++ b/crates/weaverd/src/dispatch/router.rs @@ -7,6 +7,7 @@ use std::io::Write; use std::path::PathBuf; +use std::sync::Arc; use tracing::debug; @@ -120,15 +121,43 @@ impl DomainRoutingContext { /// The router parses the domain from the request, validates the operation, and /// delegates to the appropriate handler. MVP handlers return "not implemented" /// responses for all known operations. -#[derive(Debug)] pub struct DomainRouter { workspace_root: PathBuf, + refactor_runtime: Arc, +} + +impl std::fmt::Debug for DomainRouter { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("DomainRouter") + .field("workspace_root", &self.workspace_root) + .finish_non_exhaustive() + } } impl DomainRouter { /// Creates a new domain router with the workspace root. - #[rustfmt::skip] - pub fn new(workspace_root: PathBuf) -> Self { Self { workspace_root } } + pub fn new(workspace_root: PathBuf) -> Self { + Self { + workspace_root, + refactor_runtime: act::refactor::default_runtime(), + } + } + + /// Creates a domain router with a custom refactor runtime. + #[cfg(test)] + #[expect( + dead_code, + reason = "builder for test-injected runtimes; used by future tests" + )] + pub fn with_runtime( + workspace_root: PathBuf, + runtime: Arc, + ) -> Self { + Self { + workspace_root, + refactor_runtime: runtime, + } + } /// Routes a command request to the appropriate domain handler. /// @@ -181,7 +210,15 @@ impl DomainRouter { "apply-patch" => { act::apply_patch::handle(request, writer, backends, &self.workspace_root) } - "refactor" => act::refactor::handle(request, writer, backends, &self.workspace_root), + "refactor" => act::refactor::handle( + request, + writer, + act::refactor::RefactorContext { + backends, + workspace_root: &self.workspace_root, + runtime: self.refactor_runtime.as_ref(), + }, + ), _ => Self::route_fallback(&DomainRoutingContext::ACT, operation.as_str(), writer), } } diff --git a/crates/weaverd/tests/features/refactor.feature b/crates/weaverd/tests/features/refactor.feature new file mode 100644 index 00000000..df43b8ec --- /dev/null +++ b/crates/weaverd/tests/features/refactor.feature @@ -0,0 +1,32 @@ +Feature: Act refactor + + Scenario: Refactor applies a valid plugin diff + Given a workspace file for refactoring + And a valid act refactor request + When the act refactor command executes + Then the refactor command succeeds + And the target file is updated + + Scenario: Refactor reports plugin runtime failures + Given a workspace file for refactoring + And a valid act refactor request + And a runtime error from the refactor plugin + When the act refactor command executes + Then the refactor command fails with status 1 + And the target file is unchanged + And the stderr stream contains "act refactor failed" + + Scenario: Refactor rejects malformed plugin diffs + Given a workspace file for refactoring + And a valid act refactor request + And a malformed diff response from the refactor plugin + When the act refactor command executes + Then the refactor command fails with status 1 + And the target file is unchanged + + Scenario: Refactor validates required arguments + Given a workspace file for refactoring + And a refactor request missing provider + When the act refactor command executes + Then the refactor command is rejected as invalid arguments + And the target file is unchanged diff --git a/docs/execplans/3-1-1a-plugin-for-rope.md b/docs/execplans/3-1-1a-plugin-for-rope.md new file mode 100644 index 00000000..9017bf98 --- /dev/null +++ b/docs/execplans/3-1-1a-plugin-for-rope.md @@ -0,0 +1,420 @@ +# Implement the first actuator plugin for `rope` + +This Execution Plan (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. + +Status: COMPLETE (2026-02-13) + +This document must be maintained in accordance with `AGENTS.md` at the +repository root. + +`PLANS.md` is not present in this repository, so no additional plan governance +applies beyond `AGENTS.md` and this ExecPlan. + +## Purpose / big picture + +Deliver the first real specialist actuator plugin by integrating `rope` for +Python refactoring. After this work, `weaver act refactor --provider rope` +executes a sandboxed rope-backed plugin, receives a unified diff, and applies +it through the existing Double-Lock safety harness so no filesystem changes are +committed on syntactic or semantic failure. + +Observable success: + +- `act refactor` with `--provider rope` succeeds for supported operations and + modifies files only after lock verification. +- Unsupported operations, missing arguments, timeout, plugin protocol errors, + and lock failures return structured failures and leave files unchanged. +- Unit, behavioural, and end-to-end tests cover happy paths, unhappy paths, + and edge cases. +- `docs/weaver-design.md`, `docs/users-guide.md`, and `docs/roadmap.md` + reflect the shipped behaviour. +- `make check-fmt`, `make lint`, and `make test` succeed. + +## Constraints + +- Keep all execution synchronous (no async runtime introduction). +- Keep plugin execution sandboxed via `weaver-sandbox` and + `weaver-plugins::process::SandboxExecutor`. +- 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 end-to-end (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 + guidance in `docs/rust-doctest-dry-guide.md`. +- Keep files under 400 lines by splitting modules where needed. +- Prefer dependency injection and trait boundaries for non-deterministic + concerns (process spawning, rope adapter behaviour), per + `docs/reliable-testing-in-rust-via-dependency-injection.md`. +- Run quality gates with `tee` and `set -o pipefail` before finishing. +- Update user-facing docs and roadmap status in the same change. + +## Tolerances (exception triggers) + +- Scope: if delivery requires touching more than 24 files or ~2200 net lines, + stop and escalate. +- Interface: if public protocol schema in + `crates/weaver-plugins/src/protocol/mod.rs` must break compatibility, stop + and escalate. +- Dependencies: if adding new external crates beyond those already in the + workspace becomes necessary, stop and escalate with justification. +- Iterations: if the same failing test loop repeats 5 times without progress, + stop and escalate. +- Tooling: if rope cannot be executed in CI-compatible tests without unstable + harness workarounds, stop and escalate with options. + +## Risks + +- Risk: `rope` runtime dependency may be missing in some environments. + Severity: high. Likelihood: medium. Mitigation: keep runtime behaviour + explicit when rope is unavailable and use DI-based unit/BDD tests that do not + require a global rope installation. + +- Risk: plugin diff output may not match `apply-patch` parser expectations. + Severity: high. Likelihood: medium. Mitigation: reuse `apply_patch` + parser/executor path for plugin diffs and add contract tests around diff + format compatibility. + +- Risk: sandbox profile may be too restrictive for rope adapter execution. + Severity: medium. Likelihood: medium. Mitigation: explicitly model required + executable/path allowances in manifest bootstrap and add failure tests for + denied execution. + +- Risk: adding runtime plugin state to dispatch may increase coupling. + Severity: medium. Likelihood: medium. Mitigation: introduce a small runtime + abstraction and keep routing logic thin, with unit tests around dependency + boundaries. + +## Progress + +- [x] (2026-02-12 00:00Z) Drafted ExecPlan at + `docs/execplans/3-1-1a-plugin-for-rope.md`. +- [x] (2026-02-13 01:30Z) Validated plugin bootstrap assumptions and added + executable override via `WEAVER_ROPE_PLUGIN_PATH`. +- [x] (2026-02-13 02:00Z) Implemented `crates/weaver-plugin-rope/` with + protocol handling, rope adapter boundary, and error mapping. +- [x] (2026-02-13 02:20Z) Wired `act refactor` to execute plugins and route + diff output through the existing Double-Lock apply-patch flow. +- [x] (2026-02-13 02:50Z) Added unit, behavioural (`rstest-bdd` 0.5.0), and + e2e (`assert_cmd` + `insta`) tests for happy, unhappy, and edge paths. +- [x] (2026-02-13 03:00Z) Updated design and user documentation and marked the + roadmap rope entry as done. +- [x] (2026-02-13 03:20Z) Ran full quality gates successfully: + `make fmt`, `make check-fmt`, `make lint`, `make test`, + `make markdownlint`, and `make nixie`. + +## Surprises & Discoveries + +- Observation: project memory Model Context Protocol (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 inspection only. + +## Decision Log + +- Decision: implement a dedicated rope plugin executable crate in this phase, + rather than embedding rope-specific logic into `weaverd`. Rationale: + preserves plugin architecture boundaries and keeps specialist refactoring + logic replaceable. Date/Author: 2026-02-12 / Codex + +- Decision: route plugin-produced unified diffs through the existing + `act apply-patch` execution path rather than introducing a second + patch-application implementation. Rationale: avoids duplicated + safety-critical logic and guarantees lock behaviour parity. Date/Author: + 2026-02-12 / Codex + +- Decision: use DI boundaries for rope-adapter invocation and plugin runtime + integration so tests do not depend on a system-wide rope installation. + Rationale: deterministic tests with clear unhappy-path coverage. Date/Author: + 2026-02-12 / Codex + +- Decision: add e2e command-ergonomics snapshots with a fake daemon instead of + requiring a real rope runtime in e2e tests. Rationale: validates CLI usage + and JSONL command shapes deterministically while keeping tests hermetic. + Date/Author: 2026-02-13 / Codex + +## Outcomes & Retrospective + +- `act refactor --provider rope` now executes a sandboxed plugin runtime in + `weaverd`, requiring `PluginOutput::Diff` and forwarding diff application to + the existing `act apply-patch` Double-Lock path. +- Added `crates/weaver-plugin-rope/` as the first concrete actuator plugin. + The first shipped operation is `rename`, with required arguments `offset` and + `new_name`. +- Added daemon/runtime unit tests and behavioural tests to cover success, + runtime failures, malformed diff responses, missing arguments, and no-change + edge cases. +- Added end-to-end ergonomics tests in `crates/weaver-e2e/` using + `assert_cmd` and `insta` for: + - isolated `act refactor` invocation; + - pipeline invocation chaining `observe` output through `jq`. +- Updated `docs/weaver-design.md`, `docs/users-guide.md`, and `docs/roadmap.md` + to reflect shipped behaviour and configuration. +- Full repository quality gates passed after implementation and documentation + updates. + +## Context and orientation + +Relevant existing components: + +- `crates/weaver-plugins/` provides manifest, registry, process execution, and + runner abstractions for sandboxed plugins. +- `crates/weaverd/src/dispatch/act/refactor/mod.rs` currently parses request + arguments and builds a `PluginRequest`, but returns a "plugin execution not + yet available" status. +- `crates/weaverd/src/dispatch/act/apply_patch/mod.rs` contains the existing + patch parser + transaction flow that already enforces Double-Lock semantics. +- `crates/weaverd/src/process/launch.rs` constructs the dispatch runtime. +- `docs/roadmap.md` Phase 3 still marks rope plugin as incomplete. +- `docs/users-guide.md` currently documents `act refactor` as not yet wired + end-to-end. + +Testing and style references: + +- `docs/rust-testing-with-rstest-fixtures.md` +- `docs/rstest-bdd-users-guide.md` +- `docs/reliable-testing-in-rust-via-dependency-injection.md` +- `docs/complexity-antipatterns-and-refactoring-strategies.md` +- `docs/rust-doctest-dry-guide.md` + +## Plan of work + +### Stage A: finalize runtime contract and boundaries + +Define the concrete scope for Phase 3.1.1a: + +- supported rope operations for first release (`rename` only), +- required operation arguments and error envelopes, +- plugin executable discovery/bootstrap strategy for `weaverd`. + +Record these decisions in `docs/weaver-design.md` before implementation. + +Go/no-go check: + +- A single written contract exists for request arguments, plugin output, and + failure semantics. + +### Stage B: implement the rope plugin executable crate + +Add new crate `crates/weaver-plugin-rope/` (workspace member) that: + +- reads one `PluginRequest` JSON Lines (JSONL) line from stdin, +- validates operation + arguments, +- performs rope-backed refactoring via an adapter boundary, +- emits one `PluginResponse` JSONL line to stdout, with `PluginOutput::Diff` + on success or diagnostics on failure. + +Implementation notes: + +- Keep main orchestration small; split argument parsing, rope adapter, and diff + rendering into focused modules to avoid high cognitive complexity. +- Add rustdoc usage examples for public APIs where non-trivial. +- Ensure error mapping is explicit and stable. + +Go/no-go check: + +- `cargo test -p weaver-plugin-rope` passes including unhappy-path coverage. + +### Stage C: add daemon plugin runtime bootstrap for rope + +Extend `weaverd` runtime construction so `DomainRouter`/`act refactor` can +execute plugins, including rope manifest registration. + +Likely touchpoints: + +- `crates/weaverd/src/process/launch.rs` +- `crates/weaverd/src/dispatch/handler.rs` +- `crates/weaverd/src/dispatch/router.rs` +- new small runtime wiring module under `crates/weaverd/src/dispatch/` + +Requirements: + +- register rope plugin manifest with absolute executable path, +- return clear configuration/runtime errors when rope plugin is unavailable, +- keep runtime state testable via trait abstraction or injected executor. + +Go/no-go check: + +- unit tests can execute `act refactor` path with a mock executor without + spawning external processes. + +### Stage D: wire `act refactor` through plugin execution + Double-Lock + +Replace current stub behaviour in +`crates/weaverd/src/dispatch/act/refactor/mod.rs` with end-to-end flow: + +- parse and validate refactor arguments, +- read target file payload, +- execute provider plugin via runtime runner, +- require diff output for actuator success, +- feed diff through shared patch-execution path used by `apply-patch`, +- return structured success/failure to caller. + +Refactor/compose `apply_patch` internals as needed so this path reuses existing +patch validation and transaction logic rather than copying it. + +Go/no-go check: + +- successful rope response writes validated changes, +- syntactic/semantic failures leave files unchanged, +- invalid plugin responses fail safely. + +### Stage E: tests (unit + behavioural + e2e) + +Add or update tests in both plugin and daemon layers. + +Unit tests: + +- rope plugin argument validation and operation dispatch, +- rope adapter error mapping, +- diff payload construction and protocol serialization, +- refactor handler flow using mock plugin executor and lock outcomes. + +Behavioural tests (`rstest-bdd` v0.5.0, `&mut` worlds): + +- happy: rope rename operation produces diff and commits after locks pass, +- unhappy: missing required operation argument, +- unhappy: unsupported refactoring operation, +- unhappy: plugin timeout/non-zero execution error, +- edge: plugin returns malformed/empty diff and filesystem remains unchanged, +- edge: lock rejection rolls back changes. + +End-to-end tests (`assert_cmd` + `insta`) in `crates/weaver-e2e`: + +- ergonomics: actuator command in isolation, demonstrating the expected + `act refactor --provider rope ...` usage shape and user-visible output, +- pipeline ergonomics: `observe` query output piped through `jq` and then used + to invoke the actuator command, snapshotting the full command transcript and + resulting output for discoverable usage examples. + +Likely files: + +- `crates/weaver-plugin-rope/tests/features/rope_plugin.feature` +- `crates/weaver-plugin-rope/src/tests/behaviour.rs` +- `crates/weaverd/tests/features/refactor_rope.feature` +- `crates/weaverd/src/tests/refactor_rope_behaviour.rs` +- `crates/weaver-e2e/Cargo.toml` (add `assert_cmd` dev-dependency) +- `crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs` +- `crates/weaver-e2e/tests/snapshots/refactor_rope_cli_snapshots__*.snap` + +### Stage F: documentation and roadmap updates + +Update docs to match shipped behaviour: + +- `docs/weaver-design.md`: add Phase 3.1.1a design decisions for rope adapter, + runtime bootstrap, and diff-to-Double-Lock path. +- `docs/users-guide.md`: remove "not yet available" note for `act refactor` + and document rope-supported operations, arguments, and failure modes. +- `docs/roadmap.md`: mark the rope plugin checklist item as done. + +### Stage G: full quality gates + +Run formatting, lint, tests, and markdown checks with `tee` and +`set -o pipefail`, review logs, and only then finalize. + +## Concrete steps + +1. Create the new crate and module skeletons. +2. Implement protocol handler, rope adapter boundary, and error mapping. +3. Add rope plugin unit tests and BDD scenarios. +4. Implement plugin runtime bootstrap in `weaverd` and inject into dispatch. +5. Replace refactor stub with plugin execution and shared patch application. +6. Add `weaverd` unit tests and BDD scenarios for refactor behaviour. +7. Add `assert_cmd` + `insta` e2e tests for isolated and pipeline ergonomics. +8. Update design docs, user docs, and roadmap status. +9. Run quality gates and inspect logs. + +Commands (run from repository root): + + set -o pipefail && make fmt 2>&1 | tee /tmp/3-1-1a-make-fmt.log + set -o pipefail && make check-fmt 2>&1 | tee /tmp/3-1-1a-check-fmt.log + set -o pipefail && make lint 2>&1 | tee /tmp/3-1-1a-make-lint.log + set -o pipefail && make test 2>&1 | tee /tmp/3-1-1a-make-test.log + set -o pipefail && make markdownlint 2>&1 | tee /tmp/3-1-1a-markdownlint.log + set -o pipefail && make nixie 2>&1 | tee /tmp/3-1-1a-nixie.log + +Targeted test loops while implementing: + + set -o pipefail && cargo test -p weaver-plugin-rope 2>&1 | tee /tmp/3-1-1a-rope-plugin-test.log + set -o pipefail && cargo test -p weaverd refactor 2>&1 | tee /tmp/3-1-1a-weaverd-refactor-test.log + set -o pipefail && cargo test -p weaver-e2e refactor_rope_cli 2>&1 | tee /tmp/3-1-1a-weaver-e2e-refactor.log + +## Validation and acceptance + +The feature is accepted when all items below are true: + +- `weaver act refactor --provider rope ...` executes the rope plugin path, + returns status 0 on success, and writes verified edits. +- `act refactor` failures from plugin/runtime/lock validation are surfaced with + clear errors and no partial filesystem writes. +- Unit tests cover request validation, runtime error mapping, and diff handoff + to the Double-Lock path. +- BDD tests cover happy/unhappy/edge scenarios using `rstest-bdd` v0.5.0. +- E2E tests in `crates/weaver-e2e` use `assert_cmd` and `insta` to document + and verify: + - actuator command ergonomics in isolation, + - a pipeline flow chaining `observe` output through `jq` into actuator + invocation. +- `docs/weaver-design.md` records the decisions made by this implementation. +- `docs/users-guide.md` documents user-visible behaviour and configuration. +- `docs/roadmap.md` marks rope plugin entry as done. +- `make check-fmt`, `make lint`, and `make test` pass. + +## Idempotence and recovery + +- Implementation steps are re-runnable. +- If plugin execution fails, no edits are committed because commit only happens + after Double-Lock success. +- If sandbox/plugin bootstrap fails, fix configuration or executable path and + re-run tests. +- If markdown checks fail, run `make fmt` and re-run markdown validation before + repeating Rust gates. + +## Artifacts and notes + +Expected artifacts: + +- New crate: `crates/weaver-plugin-rope/` +- New/updated refactor runtime wiring in `crates/weaverd/src/dispatch/` +- New behavioural feature files and step definitions for rope plugin and + `act refactor` +- Updated docs: + `docs/weaver-design.md`, `docs/users-guide.md`, `docs/roadmap.md` + +## Interfaces and dependencies + +Planned interfaces (final names may vary but intent must hold): + +- In `crates/weaver-plugin-rope/src/adapter.rs`: + + pub trait RopeAdapter { + fn execute(&self, request: &PluginRequest) -> Result; + } + +- In `crates/weaverd/src/dispatch/act/refactor/...`: + + pub trait RefactorPluginRuntime { + fn execute( + &self, + provider: &str, + request: &PluginRequest, + ) -> Result; + } + +- In `crates/weaverd/src/dispatch/act/apply_patch/...` (shared path): expose a + crate-visible helper that executes a patch string through the existing parser + and `ContentTransaction` flow, so refactor and apply-patch share the same + safety-critical implementation. + +Dependency expectations: + +- Prefer existing workspace dependencies; avoid adding new third-party crates + unless justified by a documented escalation. + +## Revision note + +Initial draft created for roadmap item: "Phase 3 -> first actuator plugin -> +rope". diff --git a/docs/roadmap.md b/docs/roadmap.md index d31f766e..63a560aa 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -171,7 +171,7 @@ language-specific tools.* - [ ] Develop the first set of actuator plugins: - - [ ] A plugin for `rope` to provide advanced Python refactoring. + - [x] A plugin for `rope` to provide advanced Python refactoring. - [ ] A plugin for `srgn` to provide high-performance, precision syntactic editing. diff --git a/docs/users-guide.md b/docs/users-guide.md index ba29a14f..e7462aff 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -530,21 +530,32 @@ weaver act refactor --provider --refactoring --file [KEY=VA Arguments: -| Flag | Description | -| --------------- | ------------------------------------------------------------------- | -| `--provider` | Name of the registered plugin (e.g. `rope`). | -| `--refactoring` | Refactoring operation to request (e.g. `rename`, `extract_method`). | -| `--file` | Path to the target file (relative to workspace root). | -| `KEY=VALUE` | Extra key-value arguments forwarded to the plugin. | +| Flag | Description | +| --------------- | ------------------------------------------------------ | +| `--provider` | Name of the registered plugin (e.g. `rope`). | +| `--refactoring` | Refactoring operation to request (currently `rename`). | +| `--file` | Path to the target file (relative to workspace root). | +| `KEY=VALUE` | Extra key-value arguments forwarded to the plugin. | The plugin receives the file content in-band as part of the JSONL request and does not need filesystem access. The daemon validates the resulting diff through both the syntactic (Tree-sitter) and semantic (LSP) locks before writing to disk. -> **Note:** Full plugin execution requires a configured plugin registry. The -> `act refactor` command validates arguments and builds the plugin request in -> Phase 3.1.1; end-to-end plugin invocation is wired in Phase 3.2. +For the built-in `rope` actuator, `rename` requires `offset=` and +`new_name=` in the trailing `KEY=VALUE` arguments. + +The daemon ships with a default `rope` actuator registration. By default, it +expects the plugin executable at `/usr/bin/weaver-plugin-rope`. Override the +path with: + +```sh +WEAVER_ROPE_PLUGIN_PATH=/absolute/path/to/weaver-plugin-rope +``` + +The override path is resolved to an absolute path at daemon startup. If the +plugin executable cannot be launched, `act refactor` returns a structured +failure and does not modify the filesystem. ## Plugin system @@ -595,6 +606,13 @@ The daemon maintains a `PluginRegistry` that stores validated plugin manifests keyed by name. Plugins can be looked up by name, kind, language, or a combination thereof (e.g. "find all actuator plugins for Python"). +For the first actuator rollout, `weaverd` registers the `rope` plugin using: + +- name: `rope` +- kind: `actuator` +- language: `python` +- executable: `/usr/bin/weaver-plugin-rope` (or `WEAVER_ROPE_PLUGIN_PATH`) + ### Safety harness integration Actuator plugin output (unified diffs) flows through the same Double-Lock diff --git a/docs/weaver-design.md b/docs/weaver-design.md index 7dc173de..3b0088d2 100644 --- a/docs/weaver-design.md +++ b/docs/weaver-design.md @@ -1314,6 +1314,34 @@ during the initial implementation: safety harness code is needed — the plugin system reuses the Double-Lock infrastructure established in Phase 2. +#### 4.1.2. Implementation decisions (Phase 3.1.1a — rope actuator) + +The first concrete actuator plugin for Python refactoring is now implemented as +`weaver-plugin-rope` (`crates/weaver-plugin-rope/`). The following decisions +govern this rollout: + +- **Dedicated executable plugin crate.** Rope-specific refactoring logic lives + in its own executable crate rather than in `weaverd`. This keeps specialist + tool logic replaceable and preserves the broker/plugin boundary. + +- **Default runtime registration in `weaverd`.** The daemon registers a `rope` + actuator manifest at startup. The executable path defaults to + `/usr/bin/weaver-plugin-rope` and can be overridden with + `WEAVER_ROPE_PLUGIN_PATH`. + +- **Refactor diff handoff reuses `act apply-patch`.** Successful + `PluginOutput::Diff` from `act refactor` is forwarded into the existing + apply-patch handler, ensuring one safety-critical path for patch parsing, + syntactic verification, semantic verification, and atomic commit. + +- **One-shot rope operation support.** The first operation is `rename`, taking + `offset` and `new_name` arguments. Unsupported operations and invalid + arguments return plugin diagnostics without filesystem mutation. + +- **Ergonomics-tested CLI workflows.** End-to-end tests now snapshot actuator + usage in isolation and an operator pipeline (`observe` query piped through + `jq` into `act refactor`) using `assert_cmd` and `insta`. + ### 4.2. The "Double-Lock" Safety Harness: Ensuring Syntactic and Semantic Integrity The "Double-Lock" safety harness is the single most critical feature for