From 466ed7d922557ec9334c6503924778a6c5313a2d Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 12 Feb 2026 13:43:53 +0000 Subject: [PATCH 1/8] docs(execplans): add comprehensive execplan for rope actuator plugin 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] --- docs/execplans/3-1-1a-plugin-for-rope.md | 372 +++++++++++++++++++++++ 1 file changed, 372 insertions(+) create mode 100644 docs/execplans/3-1-1a-plugin-for-rope.md 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..d5a064c8 --- /dev/null +++ b/docs/execplans/3-1-1a-plugin-for-rope.md @@ -0,0 +1,372 @@ +# 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. + +Status: DRAFT + +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 and behavioural tests (`rstest-bdd` v0.5.0) 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. +- 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`. +- [ ] Validate assumptions about plugin bootstrap and executable discovery. +- [ ] Implement rope plugin crate and protocol adapter. +- [ ] Wire `act refactor` to execute plugin and pass diff through Double-Lock. +- [ ] Add unit and `rstest-bdd` behavioural tests for happy/unhappy/edge paths. +- [ ] Update design/user docs and mark roadmap item done. +- [ ] Run `make check-fmt`, `make lint`, and `make test` successfully. + +## 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 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 + +## Outcomes & Retrospective + +Pending implementation. + +## 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: finalise runtime contract and boundaries + +Define the concrete scope for Phase 3.1.1a: + +- supported rope operations for first release (`rename` and one additional + rope-native operation, e.g. `extract_method`), +- 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` 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 touch points: + +- `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) + +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. + +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` + +### 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. Update design docs, user docs, and roadmap status. +8. 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 + +## 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. +- `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". From 58de980e8918d6c14bf180d7e5eae4f68104a346 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 12 Feb 2026 16:51:33 +0000 Subject: [PATCH 2/8] test(e2e): add end-to-end tests for rope plugin CLI ergonomics - 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] --- docs/execplans/3-1-1a-plugin-for-rope.md | 30 ++++++++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/docs/execplans/3-1-1a-plugin-for-rope.md b/docs/execplans/3-1-1a-plugin-for-rope.md index d5a064c8..c6aa437d 100644 --- a/docs/execplans/3-1-1a-plugin-for-rope.md +++ b/docs/execplans/3-1-1a-plugin-for-rope.md @@ -26,8 +26,8 @@ Observable success: 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 and behavioural tests (`rstest-bdd` v0.5.0) cover happy paths, - unhappy paths, and edge cases. +- 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. @@ -40,6 +40,8 @@ Observable success: - 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 guidance in `docs/rust-doctest-dry-guide.md`. - Keep files under 400 lines by splitting modules where needed. @@ -232,7 +234,7 @@ Go/no-go check: - syntactic/semantic failures leave files unchanged, - invalid plugin responses fail safely. -### Stage E: tests (unit + behavioural) +### Stage E: tests (unit + behavioural + e2e) Add or update tests in both plugin and daemon layers. @@ -252,12 +254,23 @@ Behavioural tests (`rstest-bdd` v0.5.0, `&mut` worlds): - 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 @@ -282,8 +295,9 @@ Run formatting, lint, tests, and markdown checks with `tee` and 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. Update design docs, user docs, and roadmap status. -8. Run quality gates and inspect logs. +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): @@ -298,6 +312,7 @@ 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 @@ -310,6 +325,11 @@ The feature is accepted when all items below are true: - 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. From 8f3d85ebd3e95d30a84f87bf15c8ca9f015a0560 Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 13 Feb 2026 01:21:42 +0000 Subject: [PATCH 3/8] feat(refactor-actuator): add rope actuator plugin with rename operation 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] --- Cargo.lock | 14 + Cargo.toml | 1 + crates/weaver-e2e/Cargo.toml | 1 + .../tests/refactor_rope_cli_snapshots.rs | 237 ++++++++++++ ...napshots__refactor_actuator_isolation.snap | 28 ++ ...apshots__refactor_pipeline_observe_jq.snap | 38 ++ crates/weaver-plugin-rope/Cargo.toml | 18 + crates/weaver-plugin-rope/src/lib.rs | 358 ++++++++++++++++++ crates/weaver-plugin-rope/src/main.rs | 17 + .../weaver-plugin-rope/src/tests/behaviour.rs | 154 ++++++++ crates/weaver-plugin-rope/src/tests/mod.rs | 139 +++++++ .../tests/features/rope_plugin.feature | 32 ++ .../src/dispatch/act/refactor/behaviour.rs | 237 ++++++++++++ .../weaverd/src/dispatch/act/refactor/mod.rs | 216 +++++++++-- .../src/dispatch/act/refactor/tests.rs | 216 +++++++++++ crates/weaverd/src/dispatch/router.rs | 29 +- .../weaverd/tests/features/refactor.feature | 32 ++ docs/execplans/3-1-1a-plugin-for-rope.md | 47 ++- docs/roadmap.md | 2 +- docs/users-guide.md | 36 +- docs/weaver-design.md | 28 ++ 21 files changed, 1832 insertions(+), 48 deletions(-) create mode 100644 crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs create mode 100644 crates/weaver-e2e/tests/snapshots/refactor_rope_cli_snapshots__refactor_actuator_isolation.snap create mode 100644 crates/weaver-e2e/tests/snapshots/refactor_rope_cli_snapshots__refactor_pipeline_observe_jq.snap create mode 100644 crates/weaver-plugin-rope/Cargo.toml create mode 100644 crates/weaver-plugin-rope/src/lib.rs create mode 100644 crates/weaver-plugin-rope/src/main.rs create mode 100644 crates/weaver-plugin-rope/src/tests/behaviour.rs create mode 100644 crates/weaver-plugin-rope/src/tests/mod.rs create mode 100644 crates/weaver-plugin-rope/tests/features/rope_plugin.feature create mode 100644 crates/weaverd/src/dispatch/act/refactor/behaviour.rs create mode 100644 crates/weaverd/src/dispatch/act/refactor/tests.rs create mode 100644 crates/weaverd/tests/features/refactor.feature diff --git a/Cargo.lock b/Cargo.lock index 953b4267..8e0a6e8c 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,19 @@ dependencies = [ "weaver-config", ] +[[package]] +name = "weaver-plugin-rope" +version = "0.1.0" +dependencies = [ + "rstest", + "rstest-bdd", + "rstest-bdd-macros", + "serde_json", + "tempfile", + "thiserror 2.0.17", + "weaver-plugins", +] + [[package]] name = "weaver-plugins" version = "0.1.0" 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..1114011c --- /dev/null +++ b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs @@ -0,0 +1,237 @@ +//! 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::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) + } + + fn requests(&self) -> Vec { + self.requests + .lock() + .map(|items| items.clone()) + .unwrap_or_default() + } + + fn join(self) { + std::mem::drop(self.join_handle.join()); + } +} + +fn serve_requests( + listener: &TcpListener, + expected_requests: usize, + requests: &Arc>>, +) { + for _ in 0..expected_requests { + let Ok((stream, _)) = listener.accept() else { + return; + }; + if respond_to_request(stream, requests).is_err() { + return; + } + } +} + +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(), + }) + }); + + if let Ok(mut guard) = requests.lock() { + guard.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 = 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(), + }; + + 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 { + std::mem::drop(writeln!( + std::io::stderr().lock(), + "Skipping test: jq not available on PATH" + )); + 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(["-lc", 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..7de8c8b3 --- /dev/null +++ b/crates/weaver-plugin-rope/Cargo.toml @@ -0,0 +1,18 @@ +[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] +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..48f35996 --- /dev/null +++ b/crates/weaver-plugin-rope/src/lib.rs @@ -0,0 +1,358 @@ +//! 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 { + validate_relative_path(file.path())?; + + let workspace = + TempDir::new().map_err(|source| RopeAdapterError::WorkspaceCreate { source })?; + let absolute_path = 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(), + })?; + + let _ = absolute_path; + 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, + }, + /// Serialising 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) { + Ok(request) => execute_request(adapter, &request), + 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) -> PluginResponse { + match request.operation() { + "rename" => execute_rename(adapter, request), + other => failure_response(format!("unsupported refactoring operation '{other}'")), + } +} + +fn execute_rename(adapter: &R, request: &PluginRequest) -> PluginResponse { + let operation = parse_rename_arguments(request.arguments()); + let (offset, new_name) = match operation { + Ok(args) => args, + Err(message) => return failure_response(message), + }; + + let Some(file) = request.files().first() else { + return failure_response(String::from("rename operation requires one file payload")); + }; + + let modified = match adapter.rename(file, offset, &new_name) { + Ok(modified) => modified, + Err(error) => return failure_response(error.to_string()), + }; + + if modified == file.content() { + return failure_response(String::from("rename operation produced no content changes")); + } + + let patch = build_search_replace_patch(file.path(), file.content(), &modified); + 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 = json_value_to_string(new_name_value) + .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, 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 | Component::Prefix(_))); + if has_parent_traversal { + return Err(RopeAdapterError::InvalidPath { + message: String::from("path traversal is not allowed"), + }); + } + + Ok(()) +} + +fn build_search_replace_patch(path: &Path, original: &str, modified: &str) -> String { + let unix_path = path_to_slash(path); + let original_block = ensure_trailing_newline(original); + let modified_block = ensure_trailing_newline(modified); + + format!( + "diff --git a/{unix_path} b/{unix_path}\n<<<<<<< SEARCH\n{original_block}=======\n{modified_block}>>>>>>> REPLACE\n" + ) +} + +fn ensure_trailing_newline(content: &str) -> String { + if content.ends_with('\n') { + return content.to_owned(); + } + format!("{content}\n") +} + +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("/") +} + +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..9a798b70 --- /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) { + std::mem::drop(writeln!(io::stderr().lock(), "{error}")); + 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..39528ef1 --- /dev/null +++ b/crates/weaver-plugin-rope/src/tests/behaviour.rs @@ -0,0 +1,154 @@ +//! Behaviour-driven tests for rope plugin request dispatch. + +use std::collections::HashMap; +use std::path::PathBuf; + +use rstest::fixture; +use rstest_bdd_macros::{given, scenario, then, when}; +use weaver_plugins::protocol::{ + DiagnosticSeverity, FilePayload, PluginDiagnostic, PluginOutput, PluginRequest, PluginResponse, +}; + +use crate::{RopeAdapter, RopeAdapterError, execute_request}; + +#[derive(Default)] +struct World { + request: Option, + response: Option, + adapter_mode: AdapterMode, +} + +#[derive(Default, Clone, Copy)] +enum AdapterMode { + #[default] + Success, + NoChange, + Fails, +} + +#[fixture] +fn world() -> World { + World::default() +} + +struct BehaviourAdapter { + mode: AdapterMode, +} + +impl RopeAdapter for BehaviourAdapter { + fn rename( + &self, + file: &FilePayload, + _offset: usize, + _new_name: &str, + ) -> Result { + match self.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 adapter = BehaviourAdapter { + mode: world.adapter_mode, + }; + world.response = Some(execute_request(&adapter, request)); +} + +#[then("the plugin returns successful diff output")] +fn then_successful_diff(world: &mut World) { + let response = world.response.as_ref().expect("response should be present"); + 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 = world.response.as_ref().expect("response should be present"); + assert!(!response.is_success()); + assert!(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 = world.response.as_ref().expect("response should be present"); + let diagnostics: Vec<&PluginDiagnostic> = response.diagnostics().iter().collect(); + assert!( + diagnostics + .iter() + .any(|diagnostic| diagnostic.message().contains(needle)), + "expected diagnostics to contain '{needle}', got: {:?}", + diagnostics + .iter() + .map(|diagnostic| diagnostic.message()) + .collect::>() + ); +} + +#[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..f8b139fc --- /dev/null +++ b/crates/weaver-plugin-rope/src/tests/mod.rs @@ -0,0 +1,139 @@ +//! Unit and behavioural tests for the rope actuator plugin. + +mod behaviour; + +use std::collections::HashMap; +use std::path::PathBuf; + +use rstest::rstest; +use weaver_plugins::protocol::{FilePayload, PluginOutput, PluginRequest}; + +use crate::{RopeAdapter, RopeAdapterError, execute_request}; + +struct MockAdapter { + result: Result, +} + +impl RopeAdapter for MockAdapter { + fn rename( + &self, + _file: &FilePayload, + _offset: usize, + _new_name: &str, + ) -> Result { + self.result.clone() + } +} + +impl Clone for RopeAdapterError { + fn clone(&self) -> Self { + match self { + Self::WorkspaceCreate { source } => Self::WorkspaceCreate { + source: std::io::Error::new(source.kind(), source.to_string()), + }, + Self::WorkspaceWrite { path, source } => Self::WorkspaceWrite { + path: path.clone(), + source: std::io::Error::new(source.kind(), source.to_string()), + }, + Self::Spawn { source } => Self::Spawn { + source: std::io::Error::new(source.kind(), source.to_string()), + }, + Self::EngineFailed { message } => Self::EngineFailed { + message: message.clone(), + }, + Self::InvalidOutput { message } => Self::InvalidOutput { + message: message.clone(), + }, + Self::InvalidPath { message } => Self::InvalidPath { + message: message.clone(), + }, + } + } +} + +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, + ) +} + +#[test] +fn rename_success_returns_diff_output() { + 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")), + ); + + let adapter = MockAdapter { + result: Ok(String::from("def new_name():\n return 1\n")), + }; + + let response = execute_request(&adapter, &request_with_args(arguments)); + assert!(response.is_success()); + assert!(matches!(response.output(), PluginOutput::Diff { .. })); +} + +#[test] +fn rename_missing_offset_returns_failure() { + let mut arguments = HashMap::new(); + arguments.insert( + String::from("new_name"), + serde_json::Value::String(String::from("new_name")), + ); + + let adapter = MockAdapter { + result: Ok(String::from("unused")), + }; + + let response = execute_request(&adapter, &request_with_args(arguments)); + assert!(!response.is_success()); +} + +#[test] +fn unsupported_operation_returns_failure() { + let adapter = MockAdapter { + result: Ok(String::from("unused")), + }; + let request = PluginRequest::new("extract_method", Vec::new()); + + let response = execute_request(&adapter, &request); + 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( + String::from("offset"), + serde_json::Value::String(String::from("4")), + ); + arguments.insert( + String::from("new_name"), + serde_json::Value::String(String::from("new_name")), + ); + + let adapter = if output.is_empty() { + MockAdapter { + result: Err(RopeAdapterError::EngineFailed { + message: String::from("rope failed"), + }), + } + } else { + MockAdapter { result: Ok(output) } + }; + + let response = execute_request(&adapter, &request_with_args(arguments)); + assert!(!response.is_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/src/dispatch/act/refactor/behaviour.rs b/crates/weaverd/src/dispatch/act/refactor/behaviour.rs new file mode 100644 index 00000000..b67313bc --- /dev/null +++ b/crates/weaverd/src/dispatch/act/refactor/behaviour.rs @@ -0,0 +1,237 @@ +//! Behavioural tests for the `act refactor` handler. + +use std::path::PathBuf; + +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, +} + +struct MockRuntime { + mode: RuntimeMode, +} + +impl RefactorPluginRuntime for MockRuntime { + fn execute( + &self, + _provider: &str, + _request: &PluginRequest, + ) -> Result { + 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), + })), + } + } +} + +struct RefactorWorld { + workspace: TempDir, + request: CommandRequest, + runtime_mode: RuntimeMode, + dispatch_result: Option>, + response_stream: String, +} + +impl RefactorWorld { + fn new() -> Self { + Self { + workspace: TempDir::new().expect("workspace"), + 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 runtime = MockRuntime { + mode: self.runtime_mode, + }; + let mut output = Vec::new(); + let mut writer = ResponseWriter::new(&mut output); + let mut backends = build_backends(); + let result = handle( + &self.request, + &mut writer, + &mut backends, + RefactorDependencies::new(self.workspace.path(), &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() -> FusionBackends { + let config = Config { + daemon_socket: SocketEndpoint::unix("/tmp/weaver-test/socket.sock"), + ..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..b7eee2a0 100644 --- a/crates/weaverd/src/dispatch/act/refactor/mod.rs +++ b/crates/weaverd/src/dispatch/act/refactor/mod.rs @@ -5,40 +5,147 @@ //! 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; +} + +/// Dependencies required by the `act refactor` handler. +pub(crate) struct RefactorDependencies<'a> { + workspace_root: &'a Path, + runtime: &'a (dyn RefactorPluginRuntime + Send + Sync), +} + +impl<'a> RefactorDependencies<'a> { + /// Creates a dependency bundle for handler execution. + pub(crate) const fn new( + workspace_root: &'a Path, + runtime: &'a (dyn RefactorPluginRuntime + Send + Sync), + ) -> Self { + Self { + workspace_root, + runtime, + } + } +} + +/// Sandbox-backed runtime that resolves plugins from a registry. +pub(crate) struct SandboxRefactorRuntime { + runner: Option>, + startup_error: Option, +} + +impl SandboxRefactorRuntime { + /// Builds the default runtime from environment configuration. + #[must_use] + pub fn from_environment() -> Self { + 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); + + match registry.register(manifest) { + Ok(()) => Self { + runner: Some(PluginRunner::new(registry, SandboxExecutor)), + startup_error: None, + }, + Err(error) => Self { + runner: None, + startup_error: Some(format!("failed to initialise refactor runtime: {error}")), + }, + } + } +} + +impl RefactorPluginRuntime for SandboxRefactorRuntime { + fn execute( + &self, + provider: &str, + request: &PluginRequest, + ) -> Result { + let runner = self.runner.as_ref().ok_or_else(|| PluginError::Manifest { + message: self + .startup_error + .clone() + .unwrap_or_else(|| String::from("refactor runtime is unavailable")), + })?; + runner.execute(provider, request) + } +} + +/// Constructs the default refactor plugin runtime for daemon dispatch. +#[must_use] +pub(crate) fn default_runtime() -> Arc { + Arc::new(SandboxRefactorRuntime::from_environment()) +} + +/// 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(_) => PathBuf::from(DEFAULT_ROPE_PLUGIN_PATH), + } +} + /// 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, + dependencies: RefactorDependencies<'_>, ) -> Result { let args = parse_refactor_args(&request.arguments)?; @@ -55,12 +162,11 @@ pub fn handle( .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(dependencies.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 +183,27 @@ 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 dependencies + .runtime + .execute(&args.provider, &plugin_request) + { + Ok(response) => { + handle_plugin_response(response, writer, backends, dependencies.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 +313,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..0a10d6c7 --- /dev/null +++ b/crates/weaverd/src/dispatch/act/refactor/tests.rs @@ -0,0 +1,216 @@ +//! Unit tests for the `act refactor` handler. + +use std::path::Path; + +use rstest::rstest; +use tempfile::TempDir; +use weaver_config::{CapabilityMatrix, Config, SocketEndpoint}; +use weaver_plugins::{PluginError, PluginOutput, PluginRequest, PluginResponse}; + +use super::{ + DispatchError, FusionBackends, RefactorDependencies, 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, + } +} + +fn build_backends() -> FusionBackends { + let config = Config { + daemon_socket: SocketEndpoint::unix("/tmp/weaver-test/socket.sock"), + ..Config::default() + }; + let provider = SemanticBackendProvider::new(CapabilityMatrix::default()); + FusionBackends::new(config, provider) +} + +#[test] +fn handle_returns_error_for_missing_provider() { + let request = command_request(vec![ + String::from("--refactoring"), + String::from("rename"), + String::from("--file"), + String::from("notes.txt"), + ]); + let mut backends = build_backends(); + 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, + &mut backends, + RefactorDependencies::new(Path::new("/tmp/workspace"), &runtime), + ); + + assert!(matches!( + result, + Err(DispatchError::InvalidArguments { .. }) + )); +} + +#[test] +fn handle_runtime_error_returns_status_one() { + 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 mut backends = build_backends(); + let mut output = Vec::new(); + let mut writer = ResponseWriter::new(&mut output); + + let result = handle( + &request, + &mut writer, + &mut backends, + RefactorDependencies::new(workspace.path(), &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) { + 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 mut backends = build_backends(); + let mut output = Vec::new(); + let mut writer = ResponseWriter::new(&mut output); + + let result = handle( + &request, + &mut writer, + &mut backends, + RefactorDependencies::new(workspace.path(), &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")); +} + +#[test] +fn handle_diff_output_applies_patch_through_apply_patch_pipeline() { + 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 mut backends = build_backends(); + let mut output = Vec::new(); + let mut writer = ResponseWriter::new(&mut output); + + let result = handle( + &request, + &mut writer, + &mut backends, + RefactorDependencies::new(workspace.path(), &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..b019a6bf 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,27 @@ 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(), + } + } /// Routes a command request to the appropriate domain handler. /// @@ -181,7 +194,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, + backends, + act::refactor::RefactorDependencies::new( + &self.workspace_root, + 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 index c6aa437d..1e7424d9 100644 --- a/docs/execplans/3-1-1a-plugin-for-rope.md +++ b/docs/execplans/3-1-1a-plugin-for-rope.md @@ -4,7 +4,7 @@ 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. -Status: DRAFT +Status: COMPLETE (2026-02-13) This document must be maintained in accordance with `AGENTS.md` at the repository root. @@ -91,12 +91,19 @@ Observable success: - [x] (2026-02-12 00:00Z) Drafted ExecPlan at `docs/execplans/3-1-1a-plugin-for-rope.md`. -- [ ] Validate assumptions about plugin bootstrap and executable discovery. -- [ ] Implement rope plugin crate and protocol adapter. -- [ ] Wire `act refactor` to execute plugin and pass diff through Double-Lock. -- [ ] Add unit and `rstest-bdd` behavioural tests for happy/unhappy/edge paths. -- [ ] Update design/user docs and mark roadmap item done. -- [ ] Run `make check-fmt`, `make lint`, and `make test` successfully. +- [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 @@ -123,9 +130,30 @@ Observable success: 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 -Pending implementation. +- `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 @@ -157,8 +185,7 @@ Testing and style references: Define the concrete scope for Phase 3.1.1a: -- supported rope operations for first release (`rename` and one additional - rope-native operation, e.g. `extract_method`), +- supported rope operations for first release (`rename` only), - required operation arguments and error envelopes, - plugin executable discovery/bootstrap strategy for `weaverd`. 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..64c3cd25 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 From d251898336e579bb4ce1cd57b6e1e34dda8293bb Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 14 Feb 2026 23:37:16 +0000 Subject: [PATCH 4/8] refactor(weaver-plugin-rope): refactor plugin response to use Result 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] --- .../tests/refactor_rope_cli_snapshots.rs | 7 +- crates/weaver-plugin-rope/src/lib.rs | 75 +++--- crates/weaver-plugin-rope/src/main.rs | 2 +- .../weaver-plugin-rope/src/tests/behaviour.rs | 37 +-- crates/weaver-plugin-rope/src/tests/mod.rs | 222 ++++++++++++++++-- .../src/dispatch/act/refactor/behaviour.rs | 3 +- .../weaverd/src/dispatch/act/refactor/mod.rs | 106 +++++---- .../src/dispatch/act/refactor/tests.rs | 16 +- crates/weaverd/src/dispatch/router.rs | 22 +- docs/execplans/3-1-1a-plugin-for-rope.md | 25 +- docs/users-guide.md | 2 +- 11 files changed, 373 insertions(+), 144 deletions(-) diff --git a/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs index 1114011c..4703c54b 100644 --- a/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs +++ b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs @@ -69,7 +69,7 @@ impl FakeDaemon { } fn join(self) { - std::mem::drop(self.join_handle.join()); + self.join_handle.join().ok(); } } @@ -202,10 +202,11 @@ fn refactor_actuator_isolation_cli_snapshot() { fn refactor_pipeline_with_observe_and_jq_snapshot() { let jq_available = Command::new("jq").arg("--version").output().is_ok(); if !jq_available { - std::mem::drop(writeln!( + writeln!( std::io::stderr().lock(), "Skipping test: jq not available on PATH" - )); + ) + .ok(); return; } diff --git a/crates/weaver-plugin-rope/src/lib.rs b/crates/weaver-plugin-rope/src/lib.rs index 48f35996..1f9a5b03 100644 --- a/crates/weaver-plugin-rope/src/lib.rs +++ b/crates/weaver-plugin-rope/src/lib.rs @@ -66,7 +66,7 @@ impl RopeAdapter for PythonRopeAdapter { let workspace = TempDir::new().map_err(|source| RopeAdapterError::WorkspaceCreate { source })?; - let absolute_path = write_workspace_file(workspace.path(), file.path(), file.content())?; + let _absolute_path = write_workspace_file(workspace.path(), file.path(), file.content())?; let relative_path = path_to_slash(file.path()); let mut command = Command::new(PYTHON_BINARY); @@ -97,7 +97,6 @@ impl RopeAdapter for PythonRopeAdapter { message: source.to_string(), })?; - let _ = absolute_path; Ok(modified) } } @@ -112,7 +111,7 @@ pub enum PluginDispatchError { #[source] source: std::io::Error, }, - /// Serialising the response payload failed. + /// Serializing the response payload failed. #[error("failed to serialize plugin response: {source}")] Serialize { /// Underlying serialization error. @@ -177,8 +176,9 @@ pub fn run_with_adapter( stdout: &mut impl Write, adapter: &R, ) -> Result<(), PluginDispatchError> { - let response = match read_request(stdin) { - Ok(request) => execute_request(adapter, &request), + let response = match read_request(stdin).and_then(|request| execute_request(adapter, &request)) + { + Ok(resp) => resp, Err(message) => failure_response(message), }; @@ -218,35 +218,39 @@ fn read_request(stdin: &mut impl BufRead) -> Result { .map_err(|error| format!("invalid plugin request JSON: {error}")) } -fn execute_request(adapter: &R, request: &PluginRequest) -> PluginResponse { +fn execute_request( + adapter: &R, + request: &PluginRequest, +) -> Result { match request.operation() { "rename" => execute_rename(adapter, request), - other => failure_response(format!("unsupported refactoring operation '{other}'")), + other => Err(format!("unsupported refactoring operation '{other}'")), } } -fn execute_rename(adapter: &R, request: &PluginRequest) -> PluginResponse { - let operation = parse_rename_arguments(request.arguments()); - let (offset, new_name) = match operation { - Ok(args) => args, - Err(message) => return failure_response(message), - }; +fn execute_rename( + adapter: &R, + request: &PluginRequest, +) -> Result { + let (offset, new_name) = parse_rename_arguments(request.arguments())?; - let Some(file) = request.files().first() else { - return failure_response(String::from("rename operation requires one file payload")); - }; + let file = request + .files() + .first() + .ok_or_else(|| String::from("rename operation requires one file payload"))?; - let modified = match adapter.rename(file, offset, &new_name) { - Ok(modified) => modified, - Err(error) => return failure_response(error.to_string()), - }; + let modified = adapter + .rename(file, offset, &new_name) + .map_err(|error| error.to_string())?; if modified == file.content() { - return failure_response(String::from("rename operation produced no content changes")); + return Err(String::from("rename operation produced no content changes")); } let patch = build_search_replace_patch(file.path(), file.content(), &modified); - PluginResponse::success(PluginOutput::Diff { content: patch }) + Ok(PluginResponse::success(PluginOutput::Diff { + content: patch, + })) } fn parse_rename_arguments( @@ -325,21 +329,26 @@ fn validate_relative_path(path: &Path) -> Result<(), RopeAdapterError> { fn build_search_replace_patch(path: &Path, original: &str, modified: &str) -> String { let unix_path = path_to_slash(path); - let original_block = ensure_trailing_newline(original); - let modified_block = ensure_trailing_newline(modified); + let sep_after_original = if original.ends_with('\n') { "" } else { "\n" }; + let sep_after_modified = if modified.ends_with('\n') { "" } else { "\n" }; format!( - "diff --git a/{unix_path} b/{unix_path}\n<<<<<<< SEARCH\n{original_block}=======\n{modified_block}>>>>>>> REPLACE\n" + 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 ensure_trailing_newline(content: &str) -> String { - if content.ends_with('\n') { - return content.to_owned(); - } - format!("{content}\n") -} - fn path_to_slash(path: &Path) -> String { path.components() .filter_map(|component| match component { @@ -350,7 +359,7 @@ fn path_to_slash(path: &Path) -> String { .join("/") } -fn failure_response(message: String) -> PluginResponse { +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 index 9a798b70..9ad469b2 100644 --- a/crates/weaver-plugin-rope/src/main.rs +++ b/crates/weaver-plugin-rope/src/main.rs @@ -11,7 +11,7 @@ fn main() { let mut writer = stdout.lock(); if let Err(error) = run(&mut reader, &mut writer) { - std::mem::drop(writeln!(io::stderr().lock(), "{error}")); + 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 index 39528ef1..8c877907 100644 --- a/crates/weaver-plugin-rope/src/tests/behaviour.rs +++ b/crates/weaver-plugin-rope/src/tests/behaviour.rs @@ -6,15 +6,15 @@ use std::path::PathBuf; use rstest::fixture; use rstest_bdd_macros::{given, scenario, then, when}; use weaver_plugins::protocol::{ - DiagnosticSeverity, FilePayload, PluginDiagnostic, PluginOutput, PluginRequest, PluginResponse, + DiagnosticSeverity, FilePayload, PluginOutput, PluginRequest, PluginResponse, }; -use crate::{RopeAdapter, RopeAdapterError, execute_request}; +use crate::{RopeAdapter, RopeAdapterError, execute_request, failure_response}; #[derive(Default)] struct World { request: Option, - response: Option, + execute_result: Option>, adapter_mode: AdapterMode, } @@ -108,21 +108,34 @@ fn when_execute(world: &mut World) { let adapter = BehaviourAdapter { mode: world.adapter_mode, }; - world.response = Some(execute_request(&adapter, request)); + 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 = world.response.as_ref().expect("response should be present"); + 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 = world.response.as_ref().expect("response should be present"); + let response = resolved_response(world); assert!(!response.is_success()); - assert!(response.output() == &PluginOutput::Empty); + assert_eq!(response.output(), &PluginOutput::Empty); assert!( response .diagnostics() @@ -134,17 +147,13 @@ fn then_failure_diagnostics(world: &mut World) { #[then("the failure message contains {text}")] fn then_failure_contains(world: &mut World, text: String) { let needle = text.trim_matches('"'); - let response = world.response.as_ref().expect("response should be present"); - let diagnostics: Vec<&PluginDiagnostic> = response.diagnostics().iter().collect(); + 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 - .iter() - .map(|diagnostic| diagnostic.message()) - .collect::>() + "expected diagnostics to contain '{needle}', got: {diagnostics:?}", ); } diff --git a/crates/weaver-plugin-rope/src/tests/mod.rs b/crates/weaver-plugin-rope/src/tests/mod.rs index f8b139fc..d1c6b2dc 100644 --- a/crates/weaver-plugin-rope/src/tests/mod.rs +++ b/crates/weaver-plugin-rope/src/tests/mod.rs @@ -8,7 +8,7 @@ use std::path::PathBuf; use rstest::rstest; use weaver_plugins::protocol::{FilePayload, PluginOutput, PluginRequest}; -use crate::{RopeAdapter, RopeAdapterError, execute_request}; +use crate::{RopeAdapter, RopeAdapterError, execute_request, run_with_adapter}; struct MockAdapter { result: Result, @@ -78,13 +78,14 @@ fn rename_success_returns_diff_output() { result: Ok(String::from("def new_name():\n return 1\n")), }; - let response = execute_request(&adapter, &request_with_args(arguments)); + let response = execute_request(&adapter, &request_with_args(arguments)) + .expect("execute_request should succeed"); assert!(response.is_success()); assert!(matches!(response.output(), PluginOutput::Diff { .. })); } #[test] -fn rename_missing_offset_returns_failure() { +fn rename_missing_offset_returns_error() { let mut arguments = HashMap::new(); arguments.insert( String::from("new_name"), @@ -95,25 +96,37 @@ fn rename_missing_offset_returns_failure() { result: Ok(String::from("unused")), }; - let response = execute_request(&adapter, &request_with_args(arguments)); - assert!(!response.is_success()); + let err = execute_request(&adapter, &request_with_args(arguments)) + .expect_err("missing offset should fail"); + assert!( + err.contains("offset"), + "expected error mentioning 'offset', got: {err}" + ); } #[test] -fn unsupported_operation_returns_failure() { +fn unsupported_operation_returns_error() { let adapter = MockAdapter { result: Ok(String::from("unused")), }; let request = PluginRequest::new("extract_method", Vec::new()); - let response = execute_request(&adapter, &request); - assert!(!response.is_success()); + 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(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) { +#[case::no_change(FailureScenario::NoChange)] +#[case::adapter_error(FailureScenario::AdapterError)] +fn rename_non_mutating_or_error_returns_failure(#[case] scenario: FailureScenario) { let mut arguments = HashMap::new(); arguments.insert( String::from("offset"), @@ -124,16 +137,191 @@ fn rename_non_mutating_or_error_returns_failure(#[case] output: String) { serde_json::Value::String(String::from("new_name")), ); - let adapter = if output.is_empty() { - MockAdapter { + let adapter = match &scenario { + FailureScenario::AdapterError => MockAdapter { result: Err(RopeAdapterError::EngineFailed { message: String::from("rope failed"), }), - } - } else { - MockAdapter { result: Ok(output) } + }, + FailureScenario::NoChange => MockAdapter { + result: Ok(String::from("def old_name():\n return 1\n")), + }, + }; + + let err = execute_request(&adapter, &request_with_args(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({ + let mut args = HashMap::new(); + args.insert( + String::from("offset"), + serde_json::Value::String(String::from("4")), + ); + args.insert( + String::from("new_name"), + serde_json::Value::String(String::from("new_name")), + ); + args + }); + serde_json::to_string(&request).expect("serialize request") +} + +#[test] +fn run_with_adapter_writes_success_response_to_stdout() { + let input = format!("{}\n", valid_request_json()); + let mut stdin = std::io::Cursor::new(input.into_bytes()); + let mut stdout = Vec::new(); + + let adapter = MockAdapter { + result: Ok(String::from("def new_name():\n return 1\n")), + }; + run_with_adapter(&mut stdin, &mut stdout, &adapter).expect("dispatch should succeed"); + + let output = String::from_utf8(stdout).expect("utf8 stdout"); + let response: weaver_plugins::protocol::PluginResponse = + serde_json::from_str(output.trim()).expect("parse response"); + assert!(response.is_success()); +} + +#[test] +fn run_with_adapter_returns_failure_for_empty_stdin() { + let mut stdin = std::io::Cursor::new(Vec::new()); + let mut stdout = Vec::new(); + + let adapter = MockAdapter { + result: Ok(String::from("unused")), }; + run_with_adapter(&mut stdin, &mut stdout, &adapter).expect("dispatch should succeed"); - let response = execute_request(&adapter, &request_with_args(arguments)); + let output = String::from_utf8(stdout).expect("utf8 stdout"); + let response: weaver_plugins::protocol::PluginResponse = + serde_json::from_str(output.trim()).expect("parse response"); assert!(!response.is_success()); } + +#[test] +fn run_with_adapter_returns_failure_for_invalid_json() { + let mut stdin = std::io::Cursor::new(b"not valid json\n".to_vec()); + let mut stdout = Vec::new(); + + let adapter = MockAdapter { + result: Ok(String::from("unused")), + }; + run_with_adapter(&mut stdin, &mut stdout, &adapter).expect("dispatch should succeed"); + + let output = String::from_utf8(stdout).expect("utf8 stdout"); + let response: weaver_plugins::protocol::PluginResponse = + serde_json::from_str(output.trim()).expect("parse response"); + assert!(!response.is_success()); +} + +// --------------------------------------------------------------------------- +// Argument parsing edge cases +// --------------------------------------------------------------------------- + +#[test] +fn rename_offset_as_number_value_succeeds() { + let mut arguments = HashMap::new(); + arguments.insert( + String::from("offset"), + serde_json::Value::Number(serde_json::Number::from(4)), + ); + arguments.insert( + String::from("new_name"), + serde_json::Value::String(String::from("new_name")), + ); + + let adapter = MockAdapter { + result: Ok(String::from("def new_name():\n return 1\n")), + }; + + let response = execute_request(&adapter, &request_with_args(arguments)) + .expect("numeric offset should succeed"); + assert!(response.is_success()); +} + +#[test] +fn rename_offset_as_boolean_returns_error() { + let mut arguments = HashMap::new(); + arguments.insert(String::from("offset"), serde_json::Value::Bool(true)); + 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("boolean offset should fail"); + assert!( + err.contains("offset"), + "expected error mentioning 'offset', got: {err}" + ); +} + +#[test] +fn rename_empty_new_name_returns_error() { + 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(" ")), + ); + + let adapter = MockAdapter { + result: Ok(String::from("unused")), + }; + + let err = execute_request(&adapter, &request_with_args(arguments)) + .expect_err("empty new_name should fail"); + assert!( + err.contains("new_name"), + "expected error mentioning 'new_name', got: {err}" + ); +} + +#[test] +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}" + ); +} diff --git a/crates/weaverd/src/dispatch/act/refactor/behaviour.rs b/crates/weaverd/src/dispatch/act/refactor/behaviour.rs index b67313bc..ad746dbe 100644 --- a/crates/weaverd/src/dispatch/act/refactor/behaviour.rs +++ b/crates/weaverd/src/dispatch/act/refactor/behaviour.rs @@ -110,7 +110,8 @@ impl RefactorWorld { &self.request, &mut writer, &mut backends, - RefactorDependencies::new(self.workspace.path(), &runtime), + self.workspace.path(), + &runtime, ) .map(|dispatch| dispatch.status); diff --git a/crates/weaverd/src/dispatch/act/refactor/mod.rs b/crates/weaverd/src/dispatch/act/refactor/mod.rs index b7eee2a0..96b6464b 100644 --- a/crates/weaverd/src/dispatch/act/refactor/mod.rs +++ b/crates/weaverd/src/dispatch/act/refactor/mod.rs @@ -48,51 +48,31 @@ pub(crate) trait RefactorPluginRuntime { ) -> Result; } -/// Dependencies required by the `act refactor` handler. -pub(crate) struct RefactorDependencies<'a> { - workspace_root: &'a Path, - runtime: &'a (dyn RefactorPluginRuntime + Send + Sync), -} - -impl<'a> RefactorDependencies<'a> { - /// Creates a dependency bundle for handler execution. - pub(crate) const fn new( - workspace_root: &'a Path, - runtime: &'a (dyn RefactorPluginRuntime + Send + Sync), - ) -> Self { - Self { - workspace_root, - runtime, - } - } -} - /// Sandbox-backed runtime that resolves plugins from a registry. pub(crate) struct SandboxRefactorRuntime { - runner: Option>, - startup_error: Option, + runner: PluginRunner, } impl SandboxRefactorRuntime { - /// Builds the default runtime from environment configuration. - #[must_use] - pub fn from_environment() -> Self { + /// 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); - match registry.register(manifest) { - Ok(()) => Self { - runner: Some(PluginRunner::new(registry, SandboxExecutor)), - startup_error: None, - }, - Err(error) => Self { - runner: None, - startup_error: Some(format!("failed to initialise refactor runtime: {error}")), - }, - } + registry + .register(manifest) + .map_err(|error| format!("failed to initialize refactor runtime: {error}"))?; + + Ok(Self { + runner: PluginRunner::new(registry, SandboxExecutor), + }) } } @@ -102,20 +82,34 @@ impl RefactorPluginRuntime for SandboxRefactorRuntime { provider: &str, request: &PluginRequest, ) -> Result { - let runner = self.runner.as_ref().ok_or_else(|| PluginError::Manifest { - message: self - .startup_error - .clone() - .unwrap_or_else(|| String::from("refactor runtime is unavailable")), - })?; - runner.execute(provider, request) + 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 { - Arc::new(SandboxRefactorRuntime::from_environment()) + 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. @@ -129,7 +123,15 @@ fn resolve_rope_plugin_path(raw_override: Option) -> PathBuf { match std::env::current_dir() { Ok(cwd) => cwd.join(candidate), - Err(_) => PathBuf::from(DEFAULT_ROPE_PLUGIN_PATH), + Err(error) => { + tracing::warn!( + target: DISPATCH_TARGET, + path = %candidate.display(), + %error, + "cannot resolve relative plugin path against working directory; using path as-is" + ); + candidate + } } } @@ -141,11 +143,16 @@ fn resolve_rope_plugin_path(raw_override: Option) -> PathBuf { /// 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. +#[expect( + clippy::too_many_arguments, + reason = "workspace_root and runtime were previously bundled in RefactorDependencies; unbundled per review" +)] pub fn handle( request: &CommandRequest, writer: &mut ResponseWriter, backends: &mut FusionBackends, - dependencies: RefactorDependencies<'_>, + workspace_root: &Path, + runtime: &dyn RefactorPluginRuntime, ) -> Result { let args = parse_refactor_args(&request.arguments)?; @@ -162,7 +169,7 @@ pub fn handle( .map_err(DispatchError::backend_startup)?; // Resolve the target file within the workspace. - let file_path = resolve_file(dependencies.workspace_root, &args.file)?; + let file_path = resolve_file(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)) })?; @@ -189,13 +196,8 @@ pub fn handle( plugin_args, ); - match dependencies - .runtime - .execute(&args.provider, &plugin_request) - { - Ok(response) => { - handle_plugin_response(response, writer, backends, dependencies.workspace_root) - } + 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", diff --git a/crates/weaverd/src/dispatch/act/refactor/tests.rs b/crates/weaverd/src/dispatch/act/refactor/tests.rs index 0a10d6c7..2871114a 100644 --- a/crates/weaverd/src/dispatch/act/refactor/tests.rs +++ b/crates/weaverd/src/dispatch/act/refactor/tests.rs @@ -8,8 +8,8 @@ use weaver_config::{CapabilityMatrix, Config, SocketEndpoint}; use weaver_plugins::{PluginError, PluginOutput, PluginRequest, PluginResponse}; use super::{ - DispatchError, FusionBackends, RefactorDependencies, RefactorPluginRuntime, ResponseWriter, - default_runtime, handle, resolve_rope_plugin_path, + DispatchError, FusionBackends, RefactorPluginRuntime, ResponseWriter, default_runtime, handle, + resolve_rope_plugin_path, }; use crate::dispatch::request::{CommandDescriptor, CommandRequest}; use crate::semantic_provider::SemanticBackendProvider; @@ -75,7 +75,8 @@ fn handle_returns_error_for_missing_provider() { &request, &mut writer, &mut backends, - RefactorDependencies::new(Path::new("/tmp/workspace"), &runtime), + Path::new("/tmp/workspace"), + &runtime, ); assert!(matches!( @@ -109,7 +110,8 @@ fn handle_runtime_error_returns_status_one() { &request, &mut writer, &mut backends, - RefactorDependencies::new(workspace.path(), &runtime), + workspace.path(), + &runtime, ) .expect("dispatch result"); @@ -145,7 +147,8 @@ fn handle_non_diff_output_returns_status_one(#[case] output_variant: PluginOutpu &request, &mut writer, &mut backends, - RefactorDependencies::new(workspace.path(), &runtime), + workspace.path(), + &runtime, ) .expect("dispatch result"); @@ -190,7 +193,8 @@ fn handle_diff_output_applies_patch_through_apply_patch_pipeline() { &request, &mut writer, &mut backends, - RefactorDependencies::new(workspace.path(), &runtime), + workspace.path(), + &runtime, ) .expect("dispatch result"); diff --git a/crates/weaverd/src/dispatch/router.rs b/crates/weaverd/src/dispatch/router.rs index b019a6bf..f8b46f2b 100644 --- a/crates/weaverd/src/dispatch/router.rs +++ b/crates/weaverd/src/dispatch/router.rs @@ -143,6 +143,22 @@ impl DomainRouter { } } + /// 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. /// /// # Errors @@ -198,10 +214,8 @@ impl DomainRouter { request, writer, backends, - act::refactor::RefactorDependencies::new( - &self.workspace_root, - self.refactor_runtime.as_ref(), - ), + &self.workspace_root, + self.refactor_runtime.as_ref(), ), _ => Self::route_fallback(&DomainRoutingContext::ACT, operation.as_str(), writer), } diff --git a/docs/execplans/3-1-1a-plugin-for-rope.md b/docs/execplans/3-1-1a-plugin-for-rope.md index 1e7424d9..9017bf98 100644 --- a/docs/execplans/3-1-1a-plugin-for-rope.md +++ b/docs/execplans/3-1-1a-plugin-for-rope.md @@ -1,8 +1,9 @@ # 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. +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) @@ -40,8 +41,8 @@ Observable success: - 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. +- 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. @@ -107,10 +108,10 @@ Observable success: ## 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 inspection only. +- 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 @@ -181,7 +182,7 @@ Testing and style references: ## Plan of work -### Stage A: finalise runtime contract and boundaries +### Stage A: finalize runtime contract and boundaries Define the concrete scope for Phase 3.1.1a: @@ -200,7 +201,7 @@ Go/no-go check: Add new crate `crates/weaver-plugin-rope/` (workspace member) that: -- reads one `PluginRequest` JSONL line from stdin, +- 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` @@ -222,7 +223,7 @@ Go/no-go check: Extend `weaverd` runtime construction so `DomainRouter`/`act refactor` can execute plugins, including rope manifest registration. -Likely touch points: +Likely touchpoints: - `crates/weaverd/src/process/launch.rs` - `crates/weaverd/src/dispatch/handler.rs` diff --git a/docs/users-guide.md b/docs/users-guide.md index 64c3cd25..e7462aff 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -545,7 +545,7 @@ writing to disk. 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 +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: From e7841f3f382a3b4713779fbcba8142c2640626e3 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 16 Feb 2026 22:13:08 +0000 Subject: [PATCH 5/8] test(weaver-plugin-rope): add mockall mocks and use rstest for rename 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] --- Cargo.lock | 2 + .../tests/refactor_rope_cli_snapshots.rs | 5 +- crates/weaver-plugin-rope/Cargo.toml | 1 + crates/weaver-plugin-rope/src/lib.rs | 13 +- .../weaver-plugin-rope/src/tests/behaviour.rs | 42 +++-- crates/weaver-plugin-rope/src/tests/mod.rs | 171 +++++++----------- crates/weaverd/Cargo.toml | 1 + .../src/dispatch/act/refactor/behaviour.rs | 71 +++++--- 8 files changed, 154 insertions(+), 152 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8e0a6e8c..6952104f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2675,6 +2675,7 @@ dependencies = [ name = "weaver-plugin-rope" version = "0.1.0" dependencies = [ + "mockall", "rstest", "rstest-bdd", "rstest-bdd-macros", @@ -2739,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/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs index 4703c54b..635f3535 100644 --- a/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs +++ b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs @@ -69,7 +69,10 @@ impl FakeDaemon { } fn join(self) { - self.join_handle.join().ok(); + assert!( + self.join_handle.join().is_ok(), + "fake daemon thread should not panic" + ); } } diff --git a/crates/weaver-plugin-rope/Cargo.toml b/crates/weaver-plugin-rope/Cargo.toml index 7de8c8b3..87186761 100644 --- a/crates/weaver-plugin-rope/Cargo.toml +++ b/crates/weaver-plugin-rope/Cargo.toml @@ -10,6 +10,7 @@ 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 diff --git a/crates/weaver-plugin-rope/src/lib.rs b/crates/weaver-plugin-rope/src/lib.rs index 1f9a5b03..026ea363 100644 --- a/crates/weaver-plugin-rope/src/lib.rs +++ b/crates/weaver-plugin-rope/src/lib.rs @@ -66,7 +66,7 @@ impl RopeAdapter for PythonRopeAdapter { let workspace = TempDir::new().map_err(|source| RopeAdapterError::WorkspaceCreate { source })?; - let _absolute_path = write_workspace_file(workspace.path(), file.path(), file.content())?; + write_workspace_file(workspace.path(), file.path(), file.content())?; let relative_path = path_to_slash(file.path()); let mut command = Command::new(PYTHON_BINARY); @@ -317,13 +317,22 @@ fn validate_relative_path(path: &Path) -> Result<(), RopeAdapterError> { let has_parent_traversal = path .components() - .any(|component| matches!(component, Component::ParentDir | Component::Prefix(_))); + .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(()) } diff --git a/crates/weaver-plugin-rope/src/tests/behaviour.rs b/crates/weaver-plugin-rope/src/tests/behaviour.rs index 8c877907..e6c541be 100644 --- a/crates/weaver-plugin-rope/src/tests/behaviour.rs +++ b/crates/weaver-plugin-rope/src/tests/behaviour.rs @@ -3,6 +3,7 @@ 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::{ @@ -31,25 +32,35 @@ fn world() -> World { World::default() } -struct BehaviourAdapter { - mode: AdapterMode, +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") } -impl RopeAdapter for BehaviourAdapter { - fn rename( - &self, - file: &FilePayload, - _offset: usize, - _new_name: &str, - ) -> Result { - match self.mode { +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 { @@ -105,9 +116,10 @@ fn given_no_change_adapter(world: &mut World) { #[when("the plugin executes the request")] fn when_execute(world: &mut World) { let request = world.request.as_ref().expect("request should be present"); - let adapter = BehaviourAdapter { - mode: world.adapter_mode, - }; + 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)); } diff --git a/crates/weaver-plugin-rope/src/tests/mod.rs b/crates/weaver-plugin-rope/src/tests/mod.rs index d1c6b2dc..085a65ef 100644 --- a/crates/weaver-plugin-rope/src/tests/mod.rs +++ b/crates/weaver-plugin-rope/src/tests/mod.rs @@ -5,7 +5,7 @@ mod behaviour; use std::collections::HashMap; use std::path::PathBuf; -use rstest::rstest; +use rstest::{fixture, rstest}; use weaver_plugins::protocol::{FilePayload, PluginOutput, PluginRequest}; use crate::{RopeAdapter, RopeAdapterError, execute_request, run_with_adapter}; @@ -51,6 +51,20 @@ impl Clone for RopeAdapterError { } } +#[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", @@ -62,45 +76,53 @@ fn request_with_args(arguments: HashMap) -> PluginReq ) } -#[test] -fn rename_success_returns_diff_output() { - 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")), - ); - +#[rstest] +fn rename_success_returns_diff_output(rename_arguments: HashMap) { let adapter = MockAdapter { result: Ok(String::from("def new_name():\n return 1\n")), }; - let response = execute_request(&adapter, &request_with_args(arguments)) + 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 { .. })); } -#[test] -fn rename_missing_offset_returns_error() { - let mut arguments = HashMap::new(); +fn remove_offset(arguments: &mut HashMap) { + drop(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("new_name"), - serde_json::Value::String(String::from("new_name")), + String::from("offset"), + serde_json::Value::String(String::from("-1")), ); +} + +#[rstest] +#[case::missing_offset(remove_offset, "offset")] +#[case::boolean_offset(set_boolean_offset, "offset")] +#[case::negative_offset(set_negative_offset, "non-negative integer")] +fn rename_invalid_offset_arguments_return_error( + #[case] mutate: fn(&mut HashMap), + #[case] expected_message: &str, + mut rename_arguments: HashMap, +) { + mutate(&mut rename_arguments); let adapter = MockAdapter { result: Ok(String::from("unused")), }; - let err = execute_request(&adapter, &request_with_args(arguments)) - .expect_err("missing offset should fail"); + let err = execute_request(&adapter, &request_with_args(rename_arguments)) + .expect_err("invalid offset arguments should fail"); assert!( - err.contains("offset"), - "expected error mentioning 'offset', got: {err}" + err.contains(expected_message), + "expected error mentioning '{expected_message}', got: {err}" ); } @@ -126,17 +148,10 @@ enum FailureScenario { #[rstest] #[case::no_change(FailureScenario::NoChange)] #[case::adapter_error(FailureScenario::AdapterError)] -fn rename_non_mutating_or_error_returns_failure(#[case] scenario: FailureScenario) { - 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")), - ); - +fn rename_non_mutating_or_error_returns_failure( + #[case] scenario: FailureScenario, + rename_arguments: HashMap, +) { let adapter = match &scenario { FailureScenario::AdapterError => MockAdapter { result: Err(RopeAdapterError::EngineFailed { @@ -148,7 +163,7 @@ fn rename_non_mutating_or_error_returns_failure(#[case] scenario: FailureScenari }, }; - let err = execute_request(&adapter, &request_with_args(arguments)) + let err = execute_request(&adapter, &request_with_args(rename_arguments)) .expect_err("failure scenario should return Err"); match scenario { @@ -168,18 +183,7 @@ fn rename_non_mutating_or_error_returns_failure(#[case] scenario: FailureScenari // --------------------------------------------------------------------------- fn valid_request_json() -> String { - let request = request_with_args({ - let mut args = HashMap::new(); - args.insert( - String::from("offset"), - serde_json::Value::String(String::from("4")), - ); - args.insert( - String::from("new_name"), - serde_json::Value::String(String::from("new_name")), - ); - args - }); + let request = request_with_args(rename_arguments()); serde_json::to_string(&request).expect("serialize request") } @@ -236,56 +240,27 @@ fn run_with_adapter_returns_failure_for_invalid_json() { // Argument parsing edge cases // --------------------------------------------------------------------------- -#[test] -fn rename_offset_as_number_value_succeeds() { - let mut arguments = HashMap::new(); - arguments.insert( +#[rstest] +fn rename_offset_as_number_value_succeeds( + mut rename_arguments: HashMap, +) { + rename_arguments.insert( String::from("offset"), serde_json::Value::Number(serde_json::Number::from(4)), ); - arguments.insert( - String::from("new_name"), - serde_json::Value::String(String::from("new_name")), - ); let adapter = MockAdapter { result: Ok(String::from("def new_name():\n return 1\n")), }; - let response = execute_request(&adapter, &request_with_args(arguments)) + let response = execute_request(&adapter, &request_with_args(rename_arguments)) .expect("numeric offset should succeed"); assert!(response.is_success()); } -#[test] -fn rename_offset_as_boolean_returns_error() { - let mut arguments = HashMap::new(); - arguments.insert(String::from("offset"), serde_json::Value::Bool(true)); - 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("boolean offset should fail"); - assert!( - err.contains("offset"), - "expected error mentioning 'offset', got: {err}" - ); -} - -#[test] -fn rename_empty_new_name_returns_error() { - let mut arguments = HashMap::new(); - arguments.insert( - String::from("offset"), - serde_json::Value::String(String::from("4")), - ); - arguments.insert( +#[rstest] +fn rename_empty_new_name_returns_error(mut rename_arguments: HashMap) { + rename_arguments.insert( String::from("new_name"), serde_json::Value::String(String::from(" ")), ); @@ -294,34 +269,10 @@ fn rename_empty_new_name_returns_error() { result: Ok(String::from("unused")), }; - let err = execute_request(&adapter, &request_with_args(arguments)) + let err = execute_request(&adapter, &request_with_args(rename_arguments)) .expect_err("empty new_name should fail"); assert!( err.contains("new_name"), "expected error mentioning 'new_name', got: {err}" ); } - -#[test] -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}" - ); -} 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 index ad746dbe..98a9b60a 100644 --- a/crates/weaverd/src/dispatch/act/refactor/behaviour.rs +++ b/crates/weaverd/src/dispatch/act/refactor/behaviour.rs @@ -2,6 +2,7 @@ use std::path::PathBuf; +use mockall::mock; use rstest::fixture; use rstest_bdd_macros::{given, scenario, then, when}; use tempfile::TempDir; @@ -37,30 +38,51 @@ enum RuntimeMode { MalformedDiff, } -struct MockRuntime { - mode: RuntimeMode, -} - -impl RefactorPluginRuntime for MockRuntime { - fn execute( - &self, - _provider: &str, - _request: &PluginRequest, - ) -> Result { - 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), - })), - } +mock! { + Runtime {} + impl RefactorPluginRuntime for Runtime { + fn execute( + &self, + provider: &str, + request: &PluginRequest, + ) -> Result; } } +fn request_has_required_flags(request: &CommandRequest) -> bool { + request + .arguments + .iter() + .any(|argument| argument == "--provider") + && request + .arguments + .iter() + .any(|argument| argument == "--refactoring") + && request + .arguments + .iter() + .any(|argument| argument == "--file") +} + +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, request: CommandRequest, @@ -100,9 +122,10 @@ impl RefactorWorld { } fn execute(&mut self) { - let runtime = MockRuntime { - mode: self.runtime_mode, - }; + 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 mut backends = build_backends(); From f0ad69035552b8e3f5fe4990ea27f7c07568cef4 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 16 Feb 2026 23:32:54 +0000 Subject: [PATCH 6/8] refactor(refactor): consolidate refactor context and improve tests - 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] --- .../tests/refactor_rope_cli_snapshots.rs | 43 ++-- crates/weaver-plugin-rope/src/lib.rs | 18 +- crates/weaver-plugin-rope/src/tests/mod.rs | 229 +++++++----------- .../src/dispatch/act/refactor/behaviour.rs | 32 ++- .../weaverd/src/dispatch/act/refactor/mod.rs | 29 ++- .../src/dispatch/act/refactor/tests.rs | 76 +++--- crates/weaverd/src/dispatch/router.rs | 8 +- 7 files changed, 209 insertions(+), 226 deletions(-) diff --git a/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs index 635f3535..093561c8 100644 --- a/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs +++ b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs @@ -61,11 +61,15 @@ impl FakeDaemon { 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() - .map(|items| items.clone()) - .unwrap_or_default() + .expect("request mutex should not be poisoned") + .clone() } fn join(self) { @@ -91,6 +95,23 @@ fn serve_requests( } } +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>>, @@ -106,9 +127,10 @@ fn respond_to_request( }) }); - if let Ok(mut guard) = requests.lock() { - guard.push(parsed_request.clone()); - } + requests + .lock() + .expect("request mutex should not be poisoned") + .push(parsed_request.clone()); let operation = parsed_request .get("command") @@ -116,16 +138,7 @@ fn respond_to_request( .and_then(serde_json::Value::as_str) .unwrap_or_default(); - let payload = 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(), - }; + let payload = response_payload_for_operation(operation); let mut writer = stream; write_json_line( diff --git a/crates/weaver-plugin-rope/src/lib.rs b/crates/weaver-plugin-rope/src/lib.rs index 026ea363..f1064f1e 100644 --- a/crates/weaver-plugin-rope/src/lib.rs +++ b/crates/weaver-plugin-rope/src/lib.rs @@ -62,8 +62,6 @@ impl RopeAdapter for PythonRopeAdapter { offset: usize, new_name: &str, ) -> Result { - validate_relative_path(file.path())?; - let workspace = TempDir::new().map_err(|source| RopeAdapterError::WorkspaceCreate { source })?; write_workspace_file(workspace.path(), file.path(), file.content())?; @@ -234,10 +232,18 @@ fn execute_rename( ) -> Result { 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 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) diff --git a/crates/weaver-plugin-rope/src/tests/mod.rs b/crates/weaver-plugin-rope/src/tests/mod.rs index 085a65ef..993cca7c 100644 --- a/crates/weaver-plugin-rope/src/tests/mod.rs +++ b/crates/weaver-plugin-rope/src/tests/mod.rs @@ -5,50 +5,37 @@ 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}; -struct MockAdapter { - result: Result, +mock! { + Adapter {} + impl RopeAdapter for Adapter { + fn rename( + &self, + file: &FilePayload, + offset: usize, + new_name: &str, + ) -> Result; + } } -impl RopeAdapter for MockAdapter { - fn rename( - &self, - _file: &FilePayload, - _offset: usize, - _new_name: &str, - ) -> Result { - self.result.clone() - } +/// 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 } -impl Clone for RopeAdapterError { - fn clone(&self) -> Self { - match self { - Self::WorkspaceCreate { source } => Self::WorkspaceCreate { - source: std::io::Error::new(source.kind(), source.to_string()), - }, - Self::WorkspaceWrite { path, source } => Self::WorkspaceWrite { - path: path.clone(), - source: std::io::Error::new(source.kind(), source.to_string()), - }, - Self::Spawn { source } => Self::Spawn { - source: std::io::Error::new(source.kind(), source.to_string()), - }, - Self::EngineFailed { message } => Self::EngineFailed { - message: message.clone(), - }, - Self::InvalidOutput { message } => Self::InvalidOutput { - message: message.clone(), - }, - Self::InvalidPath { message } => Self::InvalidPath { - message: message.clone(), - }, - } - } +/// Builds a `MockAdapter` where rename is never expected. +fn adapter_unused() -> MockAdapter { + MockAdapter::new() } #[fixture] @@ -78,9 +65,7 @@ fn request_with_args(arguments: HashMap) -> PluginReq #[rstest] fn rename_success_returns_diff_output(rename_arguments: HashMap) { - let adapter = MockAdapter { - result: Ok(String::from("def new_name():\n return 1\n")), - }; + 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"); @@ -103,34 +88,52 @@ fn set_negative_offset(arguments: &mut HashMap) { ); } +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(" ")), + ); +} + #[rstest] -#[case::missing_offset(remove_offset, "offset")] -#[case::boolean_offset(set_boolean_offset, "offset")] -#[case::negative_offset(set_negative_offset, "non-negative integer")] -fn rename_invalid_offset_arguments_return_error( +#[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::empty_new_name(set_empty_new_name as fn(&mut _), Some("new_name"))] +fn rename_argument_validation( #[case] mutate: fn(&mut HashMap), - #[case] expected_message: &str, + #[case] expected_error: Option<&str>, mut rename_arguments: HashMap, ) { mutate(&mut rename_arguments); - let adapter = MockAdapter { - result: Ok(String::from("unused")), - }; - - let err = execute_request(&adapter, &request_with_args(rename_arguments)) - .expect_err("invalid offset arguments should fail"); - assert!( - err.contains(expected_message), - "expected error mentioning '{expected_message}', got: {err}" - ); + 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 = MockAdapter { - result: Ok(String::from("unused")), - }; + let adapter = adapter_unused(); let request = PluginRequest::new("extract_method", Vec::new()); let err = execute_request(&adapter, &request).expect_err("unsupported operation should fail"); @@ -153,14 +156,12 @@ fn rename_non_mutating_or_error_returns_failure( rename_arguments: HashMap, ) { let adapter = match &scenario { - FailureScenario::AdapterError => MockAdapter { - result: Err(RopeAdapterError::EngineFailed { - message: String::from("rope failed"), - }), - }, - FailureScenario::NoChange => MockAdapter { - result: Ok(String::from("def old_name():\n return 1\n")), - }, + 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)) @@ -187,92 +188,28 @@ fn valid_request_json() -> String { serde_json::to_string(&request).expect("serialize request") } -#[test] -fn run_with_adapter_writes_success_response_to_stdout() { - let input = format!("{}\n", valid_request_json()); - let mut stdin = std::io::Cursor::new(input.into_bytes()); - let mut stdout = Vec::new(); - - let adapter = MockAdapter { - result: Ok(String::from("def new_name():\n return 1\n")), - }; - run_with_adapter(&mut stdin, &mut stdout, &adapter).expect("dispatch should succeed"); - - let output = String::from_utf8(stdout).expect("utf8 stdout"); - let response: weaver_plugins::protocol::PluginResponse = - serde_json::from_str(output.trim()).expect("parse response"); - assert!(response.is_success()); -} - -#[test] -fn run_with_adapter_returns_failure_for_empty_stdin() { - let mut stdin = std::io::Cursor::new(Vec::new()); - let mut stdout = Vec::new(); - - let adapter = MockAdapter { - result: Ok(String::from("unused")), - }; - run_with_adapter(&mut stdin, &mut stdout, &adapter).expect("dispatch should succeed"); - - let output = String::from_utf8(stdout).expect("utf8 stdout"); - let response: weaver_plugins::protocol::PluginResponse = - serde_json::from_str(output.trim()).expect("parse response"); - assert!(!response.is_success()); -} - -#[test] -fn run_with_adapter_returns_failure_for_invalid_json() { - let mut stdin = std::io::Cursor::new(b"not valid json\n".to_vec()); +/// 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(); - - let adapter = MockAdapter { - result: Ok(String::from("unused")), - }; - run_with_adapter(&mut stdin, &mut stdout, &adapter).expect("dispatch should succeed"); - + run_with_adapter(&mut stdin, &mut stdout, adapter).expect("dispatch should succeed"); let output = String::from_utf8(stdout).expect("utf8 stdout"); - let response: weaver_plugins::protocol::PluginResponse = - serde_json::from_str(output.trim()).expect("parse response"); - assert!(!response.is_success()); + serde_json::from_str(output.trim()).expect("parse response") } -// --------------------------------------------------------------------------- -// Argument parsing edge cases -// --------------------------------------------------------------------------- - #[rstest] -fn rename_offset_as_number_value_succeeds( - mut rename_arguments: HashMap, +#[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, ) { - rename_arguments.insert( - String::from("offset"), - serde_json::Value::Number(serde_json::Number::from(4)), - ); - - let adapter = MockAdapter { - result: Ok(String::from("def new_name():\n return 1\n")), - }; - - let response = execute_request(&adapter, &request_with_args(rename_arguments)) - .expect("numeric offset should succeed"); - assert!(response.is_success()); -} - -#[rstest] -fn rename_empty_new_name_returns_error(mut rename_arguments: HashMap) { - rename_arguments.insert( - String::from("new_name"), - serde_json::Value::String(String::from(" ")), - ); - - let adapter = MockAdapter { - result: Ok(String::from("unused")), - }; - - let err = execute_request(&adapter, &request_with_args(rename_arguments)) - .expect_err("empty new_name should fail"); - assert!( - err.contains("new_name"), - "expected error mentioning 'new_name', got: {err}" - ); + let response = dispatch_stdin(&input, &adapter); + assert_eq!(response.is_success(), expect_success); } diff --git a/crates/weaverd/src/dispatch/act/refactor/behaviour.rs b/crates/weaverd/src/dispatch/act/refactor/behaviour.rs index 98a9b60a..e8061380 100644 --- a/crates/weaverd/src/dispatch/act/refactor/behaviour.rs +++ b/crates/weaverd/src/dispatch/act/refactor/behaviour.rs @@ -49,19 +49,12 @@ mock! { } } +const REQUIRED_FLAGS: &[&str] = &["--provider", "--refactoring", "--file"]; + fn request_has_required_flags(request: &CommandRequest) -> bool { - request - .arguments + REQUIRED_FLAGS .iter() - .any(|argument| argument == "--provider") - && request - .arguments - .iter() - .any(|argument| argument == "--refactoring") - && request - .arguments - .iter() - .any(|argument| argument == "--file") + .all(|flag| request.arguments.iter().any(|argument| argument == flag)) } fn configure_runtime_for_mode(runtime: &mut MockRuntime, mode: RuntimeMode) { @@ -85,6 +78,7 @@ fn configure_runtime_for_mode(runtime: &mut MockRuntime, mode: RuntimeMode) { struct RefactorWorld { workspace: TempDir, + socket_dir: TempDir, request: CommandRequest, runtime_mode: RuntimeMode, dispatch_result: Option>, @@ -95,6 +89,7 @@ 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"), @@ -128,13 +123,16 @@ impl RefactorWorld { } let mut output = Vec::new(); let mut writer = ResponseWriter::new(&mut output); - let mut backends = build_backends(); + let socket_path = self.socket_dir.path().join("socket.sock"); + let mut backends = build_backends(&socket_path); let result = handle( &self.request, &mut writer, - &mut backends, - self.workspace.path(), - &runtime, + RefactorContext { + backends: &mut backends, + workspace_root: self.workspace.path(), + runtime: &runtime, + }, ) .map(|dispatch| dispatch.status); @@ -154,9 +152,9 @@ fn command_request(arguments: Vec) -> CommandRequest { } } -fn build_backends() -> FusionBackends { +fn build_backends(socket_path: &std::path::Path) -> FusionBackends { let config = Config { - daemon_socket: SocketEndpoint::unix("/tmp/weaver-test/socket.sock"), + daemon_socket: SocketEndpoint::unix(socket_path.to_string_lossy().as_ref()), ..Config::default() }; let provider = SemanticBackendProvider::new(CapabilityMatrix::default()); diff --git a/crates/weaverd/src/dispatch/act/refactor/mod.rs b/crates/weaverd/src/dispatch/act/refactor/mod.rs index 96b6464b..eb99a699 100644 --- a/crates/weaverd/src/dispatch/act/refactor/mod.rs +++ b/crates/weaverd/src/dispatch/act/refactor/mod.rs @@ -135,6 +135,16 @@ fn resolve_rope_plugin_path(raw_override: Option) -> PathBuf { } } +/// 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 @@ -143,16 +153,10 @@ fn resolve_rope_plugin_path(raw_override: Option) -> PathBuf { /// 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. -#[expect( - clippy::too_many_arguments, - reason = "workspace_root and runtime were previously bundled in RefactorDependencies; unbundled per review" -)] pub fn handle( request: &CommandRequest, writer: &mut ResponseWriter, - backends: &mut FusionBackends, - workspace_root: &Path, - runtime: &dyn RefactorPluginRuntime, + context: RefactorContext<'_>, ) -> Result { let args = parse_refactor_args(&request.arguments)?; @@ -164,12 +168,13 @@ 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)) })?; @@ -196,8 +201,10 @@ pub fn handle( plugin_args, ); - match runtime.execute(&args.provider, &plugin_request) { - Ok(response) => handle_plugin_response(response, writer, backends, workspace_root), + 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", diff --git a/crates/weaverd/src/dispatch/act/refactor/tests.rs b/crates/weaverd/src/dispatch/act/refactor/tests.rs index 2871114a..6160e727 100644 --- a/crates/weaverd/src/dispatch/act/refactor/tests.rs +++ b/crates/weaverd/src/dispatch/act/refactor/tests.rs @@ -2,14 +2,14 @@ use std::path::Path; -use rstest::rstest; +use rstest::{fixture, rstest}; use tempfile::TempDir; use weaver_config::{CapabilityMatrix, Config, SocketEndpoint}; use weaver_plugins::{PluginError, PluginOutput, PluginRequest, PluginResponse}; use super::{ - DispatchError, FusionBackends, RefactorPluginRuntime, ResponseWriter, default_runtime, handle, - resolve_rope_plugin_path, + DispatchError, FusionBackends, RefactorContext, RefactorPluginRuntime, ResponseWriter, + default_runtime, handle, resolve_rope_plugin_path, }; use crate::dispatch::request::{CommandDescriptor, CommandRequest}; use crate::semantic_provider::SemanticBackendProvider; @@ -47,24 +47,30 @@ fn command_request(arguments: Vec) -> CommandRequest { } } -fn build_backends() -> FusionBackends { +#[fixture] +fn socket_dir() -> TempDir { + TempDir::new().expect("socket dir") +} + +fn build_backends(socket_path: &Path) -> FusionBackends { let config = Config { - daemon_socket: SocketEndpoint::unix("/tmp/weaver-test/socket.sock"), + daemon_socket: SocketEndpoint::unix(socket_path.to_string_lossy().as_ref()), ..Config::default() }; let provider = SemanticBackendProvider::new(CapabilityMatrix::default()); FusionBackends::new(config, provider) } -#[test] -fn handle_returns_error_for_missing_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 mut backends = build_backends(); + 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 { @@ -74,9 +80,11 @@ fn handle_returns_error_for_missing_provider() { let result = handle( &request, &mut writer, - &mut backends, - Path::new("/tmp/workspace"), - &runtime, + RefactorContext { + backends: &mut backends, + workspace_root: Path::new("/tmp/workspace"), + runtime: &runtime, + }, ); assert!(matches!( @@ -85,8 +93,8 @@ fn handle_returns_error_for_missing_provider() { )); } -#[test] -fn handle_runtime_error_returns_status_one() { +#[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"); @@ -102,16 +110,19 @@ fn handle_runtime_error_returns_status_one() { let runtime = MockRuntime { result: MockRuntimeResult::NotFound(String::from("rope")), }; - let mut backends = build_backends(); + 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, - &mut backends, - workspace.path(), - &runtime, + RefactorContext { + backends: &mut backends, + workspace_root: workspace.path(), + runtime: &runtime, + }, ) .expect("dispatch result"); @@ -123,7 +134,10 @@ fn handle_runtime_error_returns_status_one() { #[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) { +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"); @@ -139,16 +153,19 @@ fn handle_non_diff_output_returns_status_one(#[case] output_variant: PluginOutpu let runtime = MockRuntime { result: MockRuntimeResult::Success(PluginResponse::success(output_variant)), }; - let mut backends = build_backends(); + 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, - &mut backends, - workspace.path(), - &runtime, + RefactorContext { + backends: &mut backends, + workspace_root: workspace.path(), + runtime: &runtime, + }, ) .expect("dispatch result"); @@ -157,8 +174,8 @@ fn handle_non_diff_output_returns_status_one(#[case] output_variant: PluginOutpu assert!(stderr.contains("did not return diff output")); } -#[test] -fn handle_diff_output_applies_patch_through_apply_patch_pipeline() { +#[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); @@ -185,16 +202,19 @@ fn handle_diff_output_applies_patch_through_apply_patch_pipeline() { String::from("--file"), relative_file.clone(), ]); - let mut backends = build_backends(); + 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, - &mut backends, - workspace.path(), - &runtime, + RefactorContext { + backends: &mut backends, + workspace_root: workspace.path(), + runtime: &runtime, + }, ) .expect("dispatch result"); diff --git a/crates/weaverd/src/dispatch/router.rs b/crates/weaverd/src/dispatch/router.rs index f8b46f2b..c7479937 100644 --- a/crates/weaverd/src/dispatch/router.rs +++ b/crates/weaverd/src/dispatch/router.rs @@ -213,9 +213,11 @@ impl DomainRouter { "refactor" => act::refactor::handle( request, writer, - backends, - &self.workspace_root, - self.refactor_runtime.as_ref(), + act::refactor::RefactorContext { + backends, + workspace_root: &self.workspace_root, + runtime: self.refactor_runtime.as_ref(), + }, ), _ => Self::route_fallback(&DomainRoutingContext::ACT, operation.as_str(), writer), } From 58a3af6e4190cf8e67dc8d0a92df45ed16238323 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 16 Feb 2026 23:54:34 +0000 Subject: [PATCH 7/8] refactor(fake-daemon): add non-blocking accept with deadline to fake 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] --- .../tests/refactor_rope_cli_snapshots.rs | 45 ++++++++++++++++++- crates/weaver-plugin-rope/src/lib.rs | 5 ++- crates/weaver-plugin-rope/src/tests/mod.rs | 8 ++++ 3 files changed, 54 insertions(+), 4 deletions(-) diff --git a/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs index 093561c8..3e2bfd59 100644 --- a/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs +++ b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs @@ -8,7 +8,8 @@ use std::io::{BufRead, BufReader, Write}; use std::net::{SocketAddr, TcpListener, TcpStream}; use std::process::Output; use std::sync::{Arc, Mutex}; -use std::thread; +use std::time::{Duration, Instant}; +use std::{io, thread}; use assert_cmd::Command; use insta::assert_debug_snapshot; @@ -80,13 +81,24 @@ impl FakeDaemon { } } +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 Ok((stream, _)) = listener.accept() else { + let Some(stream) = accept_before_deadline(listener) else { return; }; if respond_to_request(stream, requests).is_err() { @@ -95,6 +107,35 @@ fn serve_requests( } } +/// 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(), diff --git a/crates/weaver-plugin-rope/src/lib.rs b/crates/weaver-plugin-rope/src/lib.rs index f1064f1e..355fb7f3 100644 --- a/crates/weaver-plugin-rope/src/lib.rs +++ b/crates/weaver-plugin-rope/src/lib.rs @@ -275,13 +275,14 @@ fn parse_rename_arguments( .parse::() .map_err(|error| format!("offset must be a non-negative integer: {error}"))?; - let new_name = json_value_to_string(new_name_value) + 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, new_name)) + Ok((offset, String::from(new_name))) } fn json_value_to_string(value: &serde_json::Value) -> Option { diff --git a/crates/weaver-plugin-rope/src/tests/mod.rs b/crates/weaver-plugin-rope/src/tests/mod.rs index 993cca7c..929c2d15 100644 --- a/crates/weaver-plugin-rope/src/tests/mod.rs +++ b/crates/weaver-plugin-rope/src/tests/mod.rs @@ -102,11 +102,19 @@ fn set_empty_new_name(arguments: &mut HashMap) { ); } +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), From 2b5d97cf6e05bf56e82f3ec8c80efc2a81176192 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 17 Feb 2026 00:05:19 +0000 Subject: [PATCH 8/8] refactor(tests): clean up test code and fix shell command argument - 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] --- crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs | 2 +- crates/weaver-plugin-rope/src/tests/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs index 3e2bfd59..01b32cec 100644 --- a/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs +++ b/crates/weaver-e2e/tests/refactor_rope_cli_snapshots.rs @@ -280,7 +280,7 @@ fn refactor_pipeline_with_observe_and_jq_snapshot() { ); let output = Command::new("bash") - .args(["-lc", shell_script]) + .args(["-c", shell_script]) .env("WEAVER_BIN", weaver_bin) .env("WEAVER_ENDPOINT", endpoint.as_str()) .output() diff --git a/crates/weaver-plugin-rope/src/tests/mod.rs b/crates/weaver-plugin-rope/src/tests/mod.rs index 929c2d15..0c2e284a 100644 --- a/crates/weaver-plugin-rope/src/tests/mod.rs +++ b/crates/weaver-plugin-rope/src/tests/mod.rs @@ -74,7 +74,7 @@ fn rename_success_returns_diff_output(rename_arguments: HashMap) { - drop(arguments.remove("offset")); + arguments.remove("offset"); } fn set_boolean_offset(arguments: &mut HashMap) {