Act refactor: require provider/refactoring/file and render contract#102
Open
Act refactor: require provider/refactoring/file and render contract#102
Conversation
WalkthroughEnforce explicit Changes
Possibly related PRs
Suggested labels
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 6 warnings, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideThis PR centralizes the operator-facing contract for Sequence diagram for act_refactor validation and execution flowsequenceDiagram
actor Operator
participant WeaverCLI
participant RefactorArgsParser
participant RefactorRequirements
participant RefactorHandler
participant Runtime
participant Plugin
Operator->>WeaverCLI: run weaver act refactor [args]
WeaverCLI->>RefactorArgsParser: parse_refactor_args(args)
alt missing_required_flags
RefactorArgsParser->>RefactorRequirements: missing_requirements_error()
RefactorRequirements-->>RefactorArgsParser: DispatchError InvalidArguments
RefactorArgsParser-->>WeaverCLI: Err InvalidArguments
WeaverCLI-->>Operator: print full contract error
else unsupported_provider_or_refactoring
RefactorArgsParser->>RefactorRequirements: validate_provider(provider)
RefactorRequirements-->>RefactorArgsParser: Err InvalidArguments
RefactorArgsParser-->>WeaverCLI: Err InvalidArguments
WeaverCLI-->>Operator: print supported providers/refactorings
else all_arguments_valid
RefactorArgsParser-->>WeaverCLI: Ok RefactorArgs
WeaverCLI->>RefactorHandler: handle(args)
RefactorHandler->>Runtime: resolve(capability, file_path, Some(provider))
Runtime-->>RefactorHandler: CapabilityResolution
RefactorHandler->>Plugin: execute rename-symbol
Plugin-->>RefactorHandler: diff
RefactorHandler-->>WeaverCLI: DispatchResult status 0
WeaverCLI-->>Operator: success output
end
Updated class diagram for act_refactor argument handling and requirementsclassDiagram
direction LR
class RefactorArgsBuilder {
-provider: Option~String~
-refactoring: Option~String~
-file: Option~String~
-extra: Vec~String~
+build() Result~RefactorArgs, DispatchError~
}
class RefactorArgs {
+provider: String
+refactoring: String
+file: String
+extra: Vec~String~
}
class RefactorRequirements {
+supported_provider_names() &'static [&'static str]
+supported_refactoring_names() &'static [&'static str]
+validate_provider(provider: &str) Result~(), DispatchError~
+validate_refactoring(refactoring: &str) Result~(), DispatchError~
+missing_requirements_error() DispatchError
-validate_value(kind: &str, supported: &[&str], value: &str) Result~(), DispatchError~
-guidance_lines() String
-format_required_flags() String
}
class RefactorHandler {
+handle(writer: W, context: Context, request: CommandRequest) DispatchResult
}
class Runtime {
+resolve(request: ResolutionRequest) Result~CapabilityResolution, RuntimeError~
}
class DispatchError {
+InvalidArguments message: String
+invalid_arguments(message: String) DispatchError
}
class RefactorHelpers {
+standard_rename_args(file: &str) Vec~String~
-provider_for_file(file: &str) &'static str
}
RefactorArgsBuilder --> RefactorArgs : builds
RefactorArgsBuilder --> RefactorRequirements : uses
RefactorHandler --> RefactorArgs : consumes
RefactorHandler --> Runtime : resolve
RefactorRequirements --> DispatchError : returns
RefactorArgsBuilder --> DispatchError : returns
RefactorHelpers --> RefactorArgsBuilder : provides_args
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaverd/src/dispatch/act/refactor/requirements.rs Comment on file pub(crate) fn validate_provider(provider: &str) -> Result<(), DispatchError> {
if supported_provider_names().contains(&provider) {
Ok(())
} else {
Err(DispatchError::invalid_arguments(format!(
"act refactor does not support provider '{provider}'\n\n{}",
guidance_lines()
)))
}
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69256c779e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
devboxerhub
Bot
force-pushed
the
feat-act-refactor-arg-requirements-p35mzy
branch
from
c6109bd to
5d6cb0b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/weaverd/src/dispatch/act/refactor/tests.rs (1)
76-83:⚠️ Potential issue | 🟡 MinorKeep the mocked resolution metadata consistent with these explicit requests.
Stop pairing
--provider roperequests withautomatic_selection(...)fixtures. Those envelopes still encoderequested_provider: NoneandSelectionMode::Automatic, so this test file can miss regressions in explicit-provider propagation even though the CLI contract is now provider-explicit.Also applies to: 146-153, 200-207, 257-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/act/refactor/tests.rs` around lines 76 - 83, The test builds a CLI request with command_request including "--provider", "rope" but still uses automatic_selection(...) fixtures that encode requested_provider: None and SelectionMode::Automatic; update the mocked resolution metadata to match the explicit provider: replace uses of automatic_selection(...) for the request variable (and the other occurrences at the ranges noted) with a fixture or envelope that sets requested_provider: Some("rope") and SelectionMode::Explicit (or an explicit_selection(...) helper) so the test verifies explicit-provider propagation correctly.
♻️ Duplicate comments (4)
crates/weaverd/src/dispatch/act/refactor/arguments.rs (1)
40-44:⚠️ Potential issue | 🟠 MajorReject malformed trailing arguments here.
Reject any trailing token that is not valid
KEY=VALUE. The current check only blocks--*, so inputs such asweaver act refactor --provider rope --refactoring rename --file src/main.py offsetstill parse and slip past the deterministic pre-runtime contract, only to fail indirectly later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/act/refactor/arguments.rs` around lines 40 - 44, The current check only rejects tokens starting with "--" but allows malformed trailing args (like plain words) to slip through; update the validation over self.extra to find any token that is not a valid KEY=VALUE pair (i.e., does not contain an '=' with non-empty key) and return DispatchError::invalid_arguments with a clear message listing the offending token(s). Locate the block using self.extra.iter().find(|extra| extra.starts_with("--")) and replace/augment it so it also checks for tokens where extra.splitn(2, '=').next() is empty or extra.splitn(2, '=').nth(1) is None, returning an error referencing DispatchError::invalid_arguments and the offending token(s).crates/weaverd/src/dispatch/act/refactor/requirements.rs (2)
82-91: 🧹 Nitpick | 🔵 TrivialGenerate the supported capability-operation list from the catalogue.
Line 89 hard-codes
rename-symbolinto the fallback text. The moment another entry lands inSUPPORTED_REFACTORINGS, this error drifts from the table even though the rest of the module is catalogue-driven. Build the alternatives string fromSUPPORTED_REFACTORINGSinstead.Patch sketch
SUPPORTED_REFACTORINGS .iter() .find(|supported| supported.capability_operation == operation) .map(|supported| supported.capability) .ok_or_else(|| { + let supported = SUPPORTED_REFACTORINGS + .iter() + .map(|supported| supported.capability_operation) + .collect::<Vec<_>>() + .join(", "); DispatchError::invalid_arguments(format!( - "act refactor does not support capability resolution for '{operation}' (only 'rename-symbol' is currently implemented)" + "act refactor does not support capability resolution for '{operation}' (supported operations: {supported})" )) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/act/refactor/requirements.rs` around lines 82 - 91, The error message in capability_for_operation hard-codes "rename-symbol"; instead generate the alternatives list from SUPPORTED_REFACTORINGS and include it in the DispatchError::invalid_arguments text. Modify capability_for_operation to build a string (e.g., by iterating SUPPORTED_REFACTORINGS and collecting each supported.capability_operation, joining them with ", " or "', '" as you prefer) and use that string in the format! call so the fallback message accurately reflects the current SUPPORTED_REFACTORINGS entries when capability resolution fails.
58-108: 🛠️ Refactor suggestion | 🟠 MajorDocument the crate-visible contract helpers.
Document the
pub(crate)functions in this module with Rustdoc socargo doccaptures the supported lists, mapping rules, and error semantics that now define theact refactorcontract.As per coding guidelines "Document public APIs using Rustdoc comments (
///) so documentation can be generated withcargo doc."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/act/refactor/requirements.rs` around lines 58 - 108, Add Rustdoc comments (///) to each crate-visible helper to document the act refactor contract: for supported_provider_names and supported_refactoring_names describe what lists they return; for validate_provider and validate_refactoring explain they validate input and return DispatchError on invalid values; for effective_operation and capability_for_operation document the mapping rules they perform and the exact error semantics (what DispatchError is returned when not found); for missing_requirements_error describe the composed error message and when it is used; also briefly document validate_value (or note it as an internal helper) and include examples or expected values where helpful so cargo doc will capture supported lists, mappings, and error behavior.docs/users-guide.md (1)
1028-1030:⚠️ Potential issue | 🟡 MinorState that
--refactoringand--filecan be missing as well.The failure-condition table still omits missing flags for these two rows, even though the handler now rejects any invocation missing
--provider,--refactoring, or--filein one pass. Align the table with the shipped contract.Patch sketch
-| `--refactoring` | Refactoring operation requested from the plugin. The handler maps `rename` to the `rename-symbol` capability contract before forwarding to the plugin. | Currently only `rename` is implemented by built-in `rope` and `rust-analyzer` plugins. | Missing value, or unsupported operation name (for example `extract_method`) causes failure. | -| `--file` | Target file to load and refactor. | Workspace-relative path to an existing readable file (for example `src/main.py`). | Absolute paths, parent traversal (`..`), or unreadable/missing files cause failure. | +| `--refactoring` | Refactoring operation requested from the plugin. The handler maps `rename` to the `rename-symbol` capability contract before forwarding to the plugin. | Currently only `rename` is implemented by built-in `rope` and `rust-analyzer` plugins. | Missing flag, missing value, or unsupported operation name (for example `extract_method`) causes failure. | +| `--file` | Target file to load and refactor. | Workspace-relative path to an existing readable file (for example `src/main.py`). | Missing flag, missing value, absolute paths, parent traversal (`..`), or unreadable/missing files cause failure. |As per coding guidelines "Ensure that any API or behavioural changes are reflected in the documentation in
docs/".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/users-guide.md` around lines 1028 - 1030, The failure-condition table for the CLI flags is out of sync with the handler: update the three rows for `--provider`, `--refactoring`, and `--file` in docs/users-guide.md so their "Failure condition" column explicitly includes "missing flag" (or "missing value" where applicable) to reflect that the handler rejects invocations missing any of `--provider`, `--refactoring`, or `--file` in one pass; ensure the `--refactoring` row still lists unsupported operations (e.g. `extract_method`) and the `--file` row still lists invalid paths (absolute, `..`, unreadable) while adding "missing flag" for all three to match the shipped handler behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaverd/src/dispatch/act/refactor/mod.rs`:
- Around line 185-187: The rename mapping is being built from the
workspace-relative args.file which yields incorrect URIs; change the call in the
dispatch flow so apply_rename_symbol_mapping receives the resolved absolute
PathBuf (file_path) instead of &args.file, refactor apply_rename_symbol_mapping
to accept &Path and construct the file URI from that absolute path, and replace
the brittle to_file_uri string logic with url::Url::from_file_path(...) to
produce a correct platform-safe file URI. Ensure all call sites and the helper
signature are updated accordingly so providers keyed by uri get the
absolute-file URI.
In `@crates/weaverd/tests/features/refactor.feature`:
- Around line 62-70: Remove the entire scenario titled "Scenario: Refactor
reports the complete required argument contract" from the feature file and
delete the step "And a valid auto-routed act refactor request resolved to rope"
elsewhere in the same feature; then update the scenario "Scenario: Refactor
rejects successful plugin responses without diffs" to use the explicit-provider
step (replace any auto-routed or implicit provider steps with the
explicit-provider step so the scenario asserts a required --provider flag
consistently). Ensure you only remove the auto-routed scenario and replace
references to auto-routing with the explicit-provider step in the named
scenario.
---
Outside diff comments:
In `@crates/weaverd/src/dispatch/act/refactor/tests.rs`:
- Around line 76-83: The test builds a CLI request with command_request
including "--provider", "rope" but still uses automatic_selection(...) fixtures
that encode requested_provider: None and SelectionMode::Automatic; update the
mocked resolution metadata to match the explicit provider: replace uses of
automatic_selection(...) for the request variable (and the other occurrences at
the ranges noted) with a fixture or envelope that sets requested_provider:
Some("rope") and SelectionMode::Explicit (or an explicit_selection(...) helper)
so the test verifies explicit-provider propagation correctly.
---
Duplicate comments:
In `@crates/weaverd/src/dispatch/act/refactor/arguments.rs`:
- Around line 40-44: The current check only rejects tokens starting with "--"
but allows malformed trailing args (like plain words) to slip through; update
the validation over self.extra to find any token that is not a valid KEY=VALUE
pair (i.e., does not contain an '=' with non-empty key) and return
DispatchError::invalid_arguments with a clear message listing the offending
token(s). Locate the block using self.extra.iter().find(|extra|
extra.starts_with("--")) and replace/augment it so it also checks for tokens
where extra.splitn(2, '=').next() is empty or extra.splitn(2, '=').nth(1) is
None, returning an error referencing DispatchError::invalid_arguments and the
offending token(s).
In `@crates/weaverd/src/dispatch/act/refactor/requirements.rs`:
- Around line 82-91: The error message in capability_for_operation hard-codes
"rename-symbol"; instead generate the alternatives list from
SUPPORTED_REFACTORINGS and include it in the DispatchError::invalid_arguments
text. Modify capability_for_operation to build a string (e.g., by iterating
SUPPORTED_REFACTORINGS and collecting each supported.capability_operation,
joining them with ", " or "', '" as you prefer) and use that string in the
format! call so the fallback message accurately reflects the current
SUPPORTED_REFACTORINGS entries when capability resolution fails.
- Around line 58-108: Add Rustdoc comments (///) to each crate-visible helper to
document the act refactor contract: for supported_provider_names and
supported_refactoring_names describe what lists they return; for
validate_provider and validate_refactoring explain they validate input and
return DispatchError on invalid values; for effective_operation and
capability_for_operation document the mapping rules they perform and the exact
error semantics (what DispatchError is returned when not found); for
missing_requirements_error describe the composed error message and when it is
used; also briefly document validate_value (or note it as an internal helper)
and include examples or expected values where helpful so cargo doc will capture
supported lists, mappings, and error behavior.
In `@docs/users-guide.md`:
- Around line 1028-1030: The failure-condition table for the CLI flags is out of
sync with the handler: update the three rows for `--provider`, `--refactoring`,
and `--file` in docs/users-guide.md so their "Failure condition" column
explicitly includes "missing flag" (or "missing value" where applicable) to
reflect that the handler rejects invocations missing any of `--provider`,
`--refactoring`, or `--file` in one pass; ensure the `--refactoring` row still
lists unsupported operations (e.g. `extract_method`) and the `--file` row still
lists invalid paths (absolute, `..`, unreadable) while adding "missing flag" for
all three to match the shipped handler behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30386ec3-4715-47d6-9d7e-adfcd074743d
📒 Files selected for processing (15)
crates/weaverd/src/dispatch/act/refactor/arguments.rscrates/weaverd/src/dispatch/act/refactor/behaviour.rscrates/weaverd/src/dispatch/act/refactor/contract_tests.rscrates/weaverd/src/dispatch/act/refactor/manifests.rscrates/weaverd/src/dispatch/act/refactor/mod.rscrates/weaverd/src/dispatch/act/refactor/refactor_helpers.rscrates/weaverd/src/dispatch/act/refactor/requirements.rscrates/weaverd/src/dispatch/act/refactor/rollback_tests.rscrates/weaverd/src/dispatch/act/refactor/tests.rscrates/weaverd/src/dispatch/router/tests.rscrates/weaverd/tests/features/refactor.featuredocs/execplans/2-3-4-return-argument-requirements-for-act-refactor.mddocs/roadmap.mddocs/users-guide.mddocs/weaver-design.md
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaverd/src/dispatch/act/refactor/mod.rs`:
- Around line 185-187: The code assumes file_path (derived from workspace_root)
is absolute when calling apply_rename_symbol_mapping after computing
effective_operation (effective_operation and apply_rename_symbol_mapping), but
DomainRouter::new (and its workspace_root) isn't validating absoluteness; either
document the absolute-path precondition on DomainRouter::new/workspace_root or
add a runtime check at the router boundary that verifies
workspace_root.is_absolute() and returns an error (or panics with a clear
message) if not; update docs/comments on DomainRouter::new to state the
requirement if you choose documentation instead of a runtime guard so callers
(tests and production) are explicit about passing absolute paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74063d09-5f41-4043-8ce3-325391cd7b8b
📒 Files selected for processing (11)
crates/weaverd/src/dispatch/act/refactor/arguments.rscrates/weaverd/src/dispatch/act/refactor/behaviour.rscrates/weaverd/src/dispatch/act/refactor/contract_tests.rscrates/weaverd/src/dispatch/act/refactor/mod.rscrates/weaverd/src/dispatch/act/refactor/requirements.rscrates/weaverd/src/dispatch/act/refactor/tests.rscrates/weaverd/tests/features/refactor.featuredocs/developers-guide.mddocs/execplans/3-1-1b-plugin-for-rust-analyzer.mddocs/execplans/phase-1-end-to-end-domain-command-execution.mddocs/users-guide.md
Comment on lines
+185
to
+187
| let effective_operation = effective_operation(&args.refactoring)?; | ||
| if effective_operation == "rename-symbol" { | ||
| apply_rename_symbol_mapping(&mut plugin_args, file_path.as_path())?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== RefactorContext constructors ==\n'
rg -n -C3 'RefactorContext\s*\{' --type rust
printf '\n== workspace_root assignments and derivations ==\n'
rg -n -C4 'workspace_root\s*:' --type rust
printf '\n== workspace_root field references and derivations ==\n'
rg -n -C3 'workspace_root' crates/weaverd/src --type rustRepository: leynos/weaver
Length of output: 49669
Consider adding a defensive validation or clear documentation of the absolute-path precondition.
All current callers pass absolute workspace roots (env::current_dir() in production, workspace.path() in tests), so the code functions correctly. However, DomainRouter::new (line 141 in router.rs) does not validate that workspace_root is absolute. Document this precondition explicitly, or add a runtime check at the boundary to catch future violations early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/weaverd/src/dispatch/act/refactor/mod.rs` around lines 185 - 187, The
code assumes file_path (derived from workspace_root) is absolute when calling
apply_rename_symbol_mapping after computing effective_operation
(effective_operation and apply_rename_symbol_mapping), but DomainRouter::new
(and its workspace_root) isn't validating absoluteness; either document the
absolute-path precondition on DomainRouter::new/workspace_root or add a runtime
check at the router boundary that verifies workspace_root.is_absolute() and
returns an error (or panics with a clear message) if not; update docs/comments
on DomainRouter::new to state the requirement if you choose documentation
instead of a runtime guard so callers (tests and production) are explicit about
passing absolute paths.
Owner
Author
|
@coderabbitai Have the following now been resolved? docs/users-guide.md (1)
section still says the daemon selects
documents two incompatible operator flows. 🤖 Prompt for AI Agents |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5534 🏁 Script executed: Length of output: 3178
The earlier text describing automatic |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. crates/weaverd/src/dispatch/act/refactor/arguments.rs Comment on file #[test]
fn missing_file_is_rejected() {
fn missing_flag_value_is_rejected() {❌ New issue: Code Duplication |
Owner
Author
|
@coderabbitai Has this now been resolved in the latest commit? Use codegraph analysis to determine your answer. If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 45648
Individual Comments (acronym definitions in the execplan doc) — All Resolved ✅All four undefined-acronym issues are addressed in the current document:
Overall Comments — Status MixedComment 1 — Comment 2 — In summary: all four individual acronym comments are resolved, and both overall comments are either addressed or no longer applicable. I'll mark this comment as resolved. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1372
No further work is required for either warning. |
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaverd/src/dispatch/act/refactor/arguments.rs`:
- Around line 40-54: The parser currently flattens non-flag tokens into
self.extra in apply_flag(), losing ordering and allowing interleaved KEY=VALUE
before later flags; to fix, enforce the "trailing" requirement during parsing by
rejecting any flag seen after self.extra has non-empty entries (or alternatively
stop flattening extras once a flag appears). Update apply_flag() to check if
self.extra.is_empty() before accepting a flag (or if a flag is processed and
self.extra is non-empty, return DispatchError::invalid_arguments with a message
about interleaved KEY=VALUE), and keep references to is_valid_extra_argument and
build() behavior unchanged so the later validation still checks content but
ordering is enforced at parse time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bcc894b1-ba7a-4887-b141-ce1b6d494e51
📒 Files selected for processing (2)
crates/weaverd/src/dispatch/act/refactor/arguments.rsdocs/developers-guide.md
Comment on lines
+40
to
+54
| let invalid_extra_arguments: Vec<&str> = self | ||
| .extra | ||
| .iter() | ||
| .map(String::as_str) | ||
| .filter(|extra| !is_valid_extra_argument(extra)) | ||
| .collect(); | ||
| if !invalid_extra_arguments.is_empty() { | ||
| let offending_tokens = invalid_extra_arguments | ||
| .iter() | ||
| .map(|token| format!("'{token}'")) | ||
| .collect::<Vec<_>>() | ||
| .join(", "); | ||
| return Err(DispatchError::invalid_arguments(format!( | ||
| "act refactor only accepts trailing KEY=VALUE arguments; invalid trailing arguments: {offending_tokens}. Use only --provider <plugin>, --refactoring <operation>, --file <path>, and trailing KEY=VALUE arguments" | ||
| ))); |
There was a problem hiding this comment.
Enforce the “trailing” requirement during parsing.
Reject interleaved KEY=VALUE tokens before build(). By the time this block runs,
apply_flag() has already flattened every non-flag token into self.extra, so
the original ordering is gone. That means
new_name=woven --provider rope --refactoring rename --file src/main.py still
parses successfully even though the contract and this error text both require
trailing KEY=VALUE arguments.
🩹 Proposed fix
struct RefactorArgsBuilder {
provider: Option<String>,
refactoring: Option<String>,
file: Option<String>,
extra: Vec<String>,
+ saw_extra: bool,
}
@@
fn apply_flag<'a>(
arg: &str,
iter: &mut impl Iterator<Item = &'a String>,
builder: &mut RefactorArgsBuilder,
) -> Result<(), DispatchError> {
+ if builder.saw_extra && arg.starts_with("--") {
+ return Err(DispatchError::invalid_arguments(format!(
+ "act refactor only accepts trailing KEY=VALUE arguments; unexpected flag '{arg}' after trailing arguments"
+ )));
+ }
match arg {
"--provider" => builder.provider = Some(parse_flag_value(arg, iter)?),
"--refactoring" => builder.refactoring = Some(parse_flag_value(arg, iter)?),
"--file" => builder.file = Some(parse_flag_value(arg, iter)?),
- other => builder.extra.push(other.to_owned()),
+ other => {
+ builder.saw_extra = true;
+ builder.extra.push(other.to_owned());
+ }
}
Ok(())
}Add a regression case for an interleaved extra followed by a recognised flag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/weaverd/src/dispatch/act/refactor/arguments.rs` around lines 40 - 54,
The parser currently flattens non-flag tokens into self.extra in apply_flag(),
losing ordering and allowing interleaved KEY=VALUE before later flags; to fix,
enforce the "trailing" requirement during parsing by rejecting any flag seen
after self.extra has non-empty entries (or alternatively stop flattening extras
once a flag appears). Update apply_flag() to check if self.extra.is_empty()
before accepting a flag (or if a flag is processed and self.extra is non-empty,
return DispatchError::invalid_arguments with a message about interleaved
KEY=VALUE), and keep references to is_valid_extra_argument and build() behavior
unchanged so the later validation still checks content but ordering is enforced
at parse time.
Owner
Author
|
@coderabbitai Have the following now been resolved? docs/developers-guide.md (1)
matches
Suggested doc patch-let request = command_request(standard_rename_args("notes.py"));
+let request = command_request(standard_rename_args_for_provider("notes.py", "rope"));Triage: 🤖 Prompt for AI Agents |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1108
let request = command_request(standard_rename_args_for_provider("notes.py", "rope"));This is fully aligned with |
…efactor Introduce a detailed execution plan outlining the roadmap item 2.3.4 to enhance `weaver act refactor` command by reporting all missing required arguments (`--provider`, `--refactoring`, and `--file`) in a single response, along with valid provider and refactoring values. This living document sets constraints, risks, testing strategies, and documentation updates needed before implementation, ensuring consistent operator experience and compliance with the new error-template policy. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…-file for act refactor This change enforces a complete operator-facing contract for the `act refactor` command, making `--provider` a required flag alongside the existing `--refactoring` and `--file` flags. It adds a canonical requirements helper module that declares supported providers and refactorings, centralizing validation and error message generation. Missing any required flag now results in one deterministic actionable error listing all required flags, supported providers, supported refactorings, and a concrete next command example. Existing code paths that formerly allowed omitted `--provider` now require explicit specification or fail early. Behavioral scenarios, tests, and documentation are updated accordingly to reflect these stricter and clearer requirements, improving user experience and reducing partial failure modes. This is a non-breaking change for internal APIs but slightly changes CLI usage contract, enforcing explicit provider specification for disambiguation and validation. Refs: docs/execplans/2-3-4-return-argument-requirements-for-act-refactor.md, #Gap 5b Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…efactoring Extracted common logic for validating provider and refactoring names into a single helper function `validate_value` to avoid code duplication and improve maintainability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…rovider/refactoring specs - Reject unexpected top-level flags in act refactor arguments with clear error messages. - Introduce a unified built-in provider specification structure for plugin manifests. - Centralize supported refactorings with capability and operation mappings. - Replace ad hoc mappings with supported refactoring catalogue for validation. - Improve guidance output listing all required flags, providers, and refactorings. - Update tests to cover validation and operation-capability mapping. - Refactor argument builders and test requests to include explicit provider flags. - Clean up capability resolution to use centralized refactoring capability mappings. - Update documentation to reflect required explicit provider usage and improved UX guidance. This change enhances user experience by enforcing correct argument usage upfront, simplifying provider manifest definitions, and consolidating refactoring capability logic for maintainability and extensibility. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Restore the provider-aware refactor test helper after the rebase and\nupdate rollback tests to use the explicit provider contract.\n\nThis preserves the branch's required --provider behaviour while\nkeeping the rollback coverage aligned with the current act refactor\nrequest shape on main.
Validate that all trailing extra arguments for `act refactor` are in key=value form and reject flags or malformed arguments. Previously, unrecognized top-level flags were rejected but malformed trailing arguments (e.g., missing `=`) were not properly detected. This change produces clearer errors listing the offending tokens and enforces the requirement that only trailing key=value arguments are allowed after the standard flags. Added tests to verify rejection of malformed trailing arguments. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Added an assertion to ensure that the DomainRouter's workspace_root is always an absolute path. This prevents issues in dispatch handlers that rely on workspace-root-relative file paths to construct canonical file URIs. Included tests to verify that providing a relative path causes a panic with a clear error message. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…ation Add comprehensive documentation to the developers guide describing the `weaverd/src/dispatch/act/refactor/requirements.rs` module. This module is the single source of truth for the operator-facing contract of `act refactor` and is used by both argument-parsing logic and tests to maintain consistency in validation, guidance text, and lists of supported values. The docs detail the seven public functions exposed, their roles in validating providers and refactorings, mapping operations to capabilities, and generating standardized error messages for missing requirements. Also include minor related code comments and a helper function to support explicit-provider test paths. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Previously, DomainRouter::new and DispatchConnectionHandler::new assumed the workspace_root path was absolute and would panic otherwise. This change replaces panics with proper error handling by returning DispatchError::InvalidArguments if the workspace_root is not absolute. All call sites have been updated to handle this error appropriately. Added tests to verify that relative workspace roots are rejected with errors rather than panics. This improves robustness of workspace root validation and prevents unexpected panics at runtime. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…r_provider Update the sample code in the developers guide to use `standard_rename_args_for_provider` instead of `standard_rename_args`, reflecting the required provider argument for the rename command. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Restore test module wiring and BDD step registration after the rebase onto origin/main, and adjust helper panic patterns so whitaker accepts the merged test support code without suppressing lint rules.
lodyai
Bot
force-pushed
the
feat-act-refactor-arg-requirements-p35mzy
branch
from
daf734d to
367baa2
Compare
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| arguments.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. crates/weaverd/src/dispatch/act/refactor/arguments.rs Comment on file #[test]
fn missing_file_is_rejected() {
fn missing_flag_value_is_rejected() {❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/weaverd/src/dispatch/act/refactor/refactor_helpers.rs (1)
41-63:⚠️ Potential issue | 🟠 MajorStop inferring the provider in shared rename args.
standard_rename_args()still fabricates--providerfrom the file extension, and the_ => "rope"fallback lets new tests pass without naming the provider under test. Remove the implicit helper, or fail fast on unknown extensions, so every caller states the provider explicitly.🩹 Proposed refactor
- pub(crate) fn standard_rename_args(file: &str) -> Vec<String> { - standard_rename_args_for_provider(file, provider_for_file(file)) - } - pub(crate) fn standard_rename_args_for_provider(file: &str, provider: &str) -> Vec<String> { vec![ String::from("--provider"), String::from(provider), String::from("--refactoring"), String::from("rename"), String::from("--file"), String::from(file), String::from("offset=1"), String::from("new_name=woven"), ] } - - fn provider_for_file(file: &str) -> &'static str { - match Path::new(file).extension().and_then(|ext| ext.to_str()) { - Some("rs") => "rust-analyzer", - _ => "rope", - } - } @@ - const _: fn(&str) -> Vec<String> = standard_rename_args; const _: fn(&str, &str) -> Vec<String> = standard_rename_args_for_provider; - const _: fn(&str) -> &'static str = provider_for_file;Based on learnings, act refactor must require
--provider,--refactoring, and--fileflags; missing or incomplete invocations must be rejected before plugin resolution or file I/O.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/act/refactor/refactor_helpers.rs` around lines 41 - 63, standard_rename_args currently infers a provider via provider_for_file which hides the required --provider flag; remove that implicit behavior by deleting provider_for_file and standard_rename_args, and update all call sites to call standard_rename_args_for_provider(file, provider) with an explicit provider string (ensuring callers supply the correct provider); this enforces that every caller names the provider and prevents falling back to "rope".
♻️ Duplicate comments (3)
crates/weaverd/src/dispatch/act/refactor/arguments.rs (2)
135-151: 🛠️ Refactor suggestion | 🟠 MajorDelete the duplicated negative-path tests.
Keep
invalid_arguments_are_rejectedas the single table for these cases and drop the repeated one-off tests below it. The current mix reintroduces the duplication this file was already flagged for and makes future message changes twice as expensive to maintain.As per coding guidelines: "Replace duplicated tests with
#[rstest(...)]parameterised cases in Rust."Also applies to: 305-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/act/refactor/arguments.rs` around lines 135 - 151, Remove the duplicated negative-path one-off tests (e.g., flag_as_value_is_rejected and any other individual invalid cases) and consolidate them into the existing parameterized test table used by invalid_arguments_are_rejected; for each removed test add its args vector as an additional case in the #[rstest(...)] list so parse_refactor_args is exercised with those inputs, and delete the redundant test functions (including references to flag_as_value_is_rejected and other one-off invalid tests) so only the single rstest-based invalid_arguments_are_rejected remains.
89-100:⚠️ Potential issue | 🟠 MajorReject recognised flags after the first trailing argument.
apply_flag()still acceptsnew_name=woven --provider rope --refactoring rename --file src/main.pybecause it recordsnew_name=woveninextraand then happily parses later flags. That breaks the documented “trailingKEY=VALUEonly” contract and lets malformed invocations survive parsing.🩹 Proposed fix
struct RefactorArgsBuilder { provider: Option<String>, refactoring: Option<String>, file: Option<String>, extra: Vec<String>, + saw_extra: bool, } @@ fn apply_flag<'a>( arg: &str, iter: &mut impl Iterator<Item = &'a String>, builder: &mut RefactorArgsBuilder, ) -> Result<(), DispatchError> { + if builder.saw_extra && arg.starts_with("--") { + return Err(DispatchError::invalid_arguments(format!( + "act refactor only accepts trailing KEY=VALUE arguments; unexpected flag '{arg}' after trailing arguments" + ))); + } + match arg { "--provider" => builder.provider = Some(parse_flag_value(arg, iter)?), "--refactoring" => builder.refactoring = Some(parse_flag_value(arg, iter)?), "--file" => builder.file = Some(parse_flag_value(arg, iter)?), - other => builder.extra.push(other.to_owned()), + other => { + builder.saw_extra = true; + builder.extra.push(other.to_owned()); + } } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/act/refactor/arguments.rs` around lines 89 - 100, apply_flag currently allows recognized flags even after a trailing positional (stored in RefactorArgsBuilder.extra), violating the "trailing KEY=VALUE only" rule; change apply_flag so that if builder.extra is non-empty and arg is one of the recognized flags ("--provider", "--refactoring", "--file") it returns a DispatchError (e.g. InvalidArgument or a suitable variant) instead of parsing the flag with parse_flag_value, otherwise behave as before; update the match in apply_flag to check builder.extra.is_empty() (or explicitly check before matching) and return an error when a flag is seen after any trailing argument.crates/weaverd/src/dispatch/act/refactor/requirements.rs (1)
27-42: 🛠️ Refactor suggestion | 🟠 MajorCollapse the refactoring catalogue into one definition.
SUPPORTED_REFACTORING_NAMESduplicatesSUPPORTED_REFACTORINGS. The next refactoring added here can updateeffective_operation()yet still be rejected byvalidate_refactoring()and omitted from the guidance text. Generate the names view from the canonical table, or define both through one macro so they cannot drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaverd/src/dispatch/act/refactor/requirements.rs` around lines 27 - 42, SUPPORTED_REFACTORING_NAMES is a duplicate of SUPPORTED_REFACTORINGS which can drift; remove the separate constant and make supported_refactoring_names() derive its slice from the canonical SUPPORTED_REFACTORINGS (or replace both with a single macro that emits the canonical &[SupportedRefactoring] and the corresponding &[&str] simultaneously) so additions update effective_operation() and validate_refactoring() and the guidance text atomically; update supported_refactoring_names(), and keep references to SUPPORTED_REFACTORINGS, supported_refactoring_names(), effective_operation(), and validate_refactoring() in mind when changing the definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaver-e2e/tests/test_support/mod.rs`:
- Around line 80-85: Replace the inline match used to construct the
DispatchConnectionHandler with the shared required_result helper: call
required_result(DispatchConnectionHandler::new(backend_manager.clone(),
workspace_root)) to unwrap or fail uniformly, then wrap that result in Arc::new
and assign to handler; this removes the explicit match/Err panic and reuses the
test-support error-unwrapping logic (keep the variable name handler and the
Arc::new wrapper).
In `@crates/weaverd/src/dispatch/act/refactor/behaviour.rs`:
- Around line 372-385: The helper then_dispatch_error_contains currently panics
when world.dispatch_result is an Ok variant; change it to return a Result::Err
string instead so the function keeps the Result<(), String> failure shape. In
then_dispatch_error_contains, after obtaining result from
world.dispatch_result.as_ref().ok_or("result missing")?, match on result to
extract the Err(error) case and if it's an Ok(status) return
Err(format!("expected dispatch error, got status: {status:?}")); then continue
using error.to_string() and assert/return Err(...) on mismatch as before so all
failures propagate via Result rather than panic.
In `@crates/weaverd/src/dispatch/act/refactor/request_building.rs`:
- Around line 39-45: The code currently injects a hard-coded "refactoring"
sentinel ("rename-symbol") instead of using the resolved capability; update the
logic so the rename-specific argument rewrite is driven by the capability
returned from capability_for_operation (use the existing effective_operation and
capability variables) rather than a second hard-coded operation string, remove
the duplicated "rename-symbol" sentinel insertion, and apply the same change to
the duplicated block around the other occurrence (the block referenced at the
later 88-97 region) so requirements.rs remains the single source of truth.
---
Outside diff comments:
In `@crates/weaverd/src/dispatch/act/refactor/refactor_helpers.rs`:
- Around line 41-63: standard_rename_args currently infers a provider via
provider_for_file which hides the required --provider flag; remove that implicit
behavior by deleting provider_for_file and standard_rename_args, and update all
call sites to call standard_rename_args_for_provider(file, provider) with an
explicit provider string (ensuring callers supply the correct provider); this
enforces that every caller names the provider and prevents falling back to
"rope".
---
Duplicate comments:
In `@crates/weaverd/src/dispatch/act/refactor/arguments.rs`:
- Around line 135-151: Remove the duplicated negative-path one-off tests (e.g.,
flag_as_value_is_rejected and any other individual invalid cases) and
consolidate them into the existing parameterized test table used by
invalid_arguments_are_rejected; for each removed test add its args vector as an
additional case in the #[rstest(...)] list so parse_refactor_args is exercised
with those inputs, and delete the redundant test functions (including references
to flag_as_value_is_rejected and other one-off invalid tests) so only the single
rstest-based invalid_arguments_are_rejected remains.
- Around line 89-100: apply_flag currently allows recognized flags even after a
trailing positional (stored in RefactorArgsBuilder.extra), violating the
"trailing KEY=VALUE only" rule; change apply_flag so that if builder.extra is
non-empty and arg is one of the recognized flags ("--provider", "--refactoring",
"--file") it returns a DispatchError (e.g. InvalidArgument or a suitable
variant) instead of parsing the flag with parse_flag_value, otherwise behave as
before; update the match in apply_flag to check builder.extra.is_empty() (or
explicitly check before matching) and return an error when a flag is seen after
any trailing argument.
In `@crates/weaverd/src/dispatch/act/refactor/requirements.rs`:
- Around line 27-42: SUPPORTED_REFACTORING_NAMES is a duplicate of
SUPPORTED_REFACTORINGS which can drift; remove the separate constant and make
supported_refactoring_names() derive its slice from the canonical
SUPPORTED_REFACTORINGS (or replace both with a single macro that emits the
canonical &[SupportedRefactoring] and the corresponding &[&str] simultaneously)
so additions update effective_operation() and validate_refactoring() and the
guidance text atomically; update supported_refactoring_names(), and keep
references to SUPPORTED_REFACTORINGS, supported_refactoring_names(),
effective_operation(), and validate_refactoring() in mind when changing the
definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9859e301-0bd1-425d-8820-56c3f5b74572
📒 Files selected for processing (12)
crates/weaver-e2e/tests/test_support/mod.rscrates/weaverd/src/dispatch/act/refactor/arguments.rscrates/weaverd/src/dispatch/act/refactor/behaviour.rscrates/weaverd/src/dispatch/act/refactor/contract_tests.rscrates/weaverd/src/dispatch/act/refactor/manifests.rscrates/weaverd/src/dispatch/act/refactor/mod.rscrates/weaverd/src/dispatch/act/refactor/refactor_helpers.rscrates/weaverd/src/dispatch/act/refactor/request_building.rscrates/weaverd/src/dispatch/act/refactor/requirements.rscrates/weaverd/src/dispatch/act/refactor/rollback_tests.rscrates/weaverd/src/dispatch/act/refactor/tests.rscrates/weaverd/src/dispatch/handler.rs
Comment on lines
+80
to
+85
| let handler = Arc::new( | ||
| match DispatchConnectionHandler::new(backend_manager.clone(), workspace_root) { | ||
| Ok(handler) => handler, | ||
| Err(error) => panic!("absolute workspace root: {error}"), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use the existing required_result helper for handler construction.
Replace the inline match at Line 81 with the shared helper to remove duplicated error-unwrapping logic and keep test-support code uniform.
Refactor diff
- let handler = Arc::new(
- match DispatchConnectionHandler::new(backend_manager.clone(), workspace_root) {
- Ok(handler) => handler,
- Err(error) => panic!("absolute workspace root: {error}"),
- },
- );
+ let handler = Arc::new(required_result(
+ DispatchConnectionHandler::new(backend_manager.clone(), workspace_root),
+ "absolute workspace root",
+ ));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let handler = Arc::new( | |
| match DispatchConnectionHandler::new(backend_manager.clone(), workspace_root) { | |
| Ok(handler) => handler, | |
| Err(error) => panic!("absolute workspace root: {error}"), | |
| }, | |
| ); | |
| let handler = Arc::new(required_result( | |
| DispatchConnectionHandler::new(backend_manager.clone(), workspace_root), | |
| "absolute workspace root", | |
| )); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/weaver-e2e/tests/test_support/mod.rs` around lines 80 - 85, Replace
the inline match used to construct the DispatchConnectionHandler with the shared
required_result helper: call
required_result(DispatchConnectionHandler::new(backend_manager.clone(),
workspace_root)) to unwrap or fail uniformly, then wrap that result in Arc::new
and assign to handler; this removes the explicit match/Err panic and reuses the
test-support error-unwrapping logic (keep the variable name handler and the
Arc::new wrapper).
Comment on lines
+372
to
+385
| #[then("the dispatch error contains {text}")] | ||
| fn then_dispatch_error_contains(world: &mut RefactorWorld, text: String) -> Result<(), String> { | ||
| let needle = text.trim_matches('"'); | ||
| let result = world.dispatch_result.as_ref().ok_or("result missing")?; | ||
| let Err(error) = result else { | ||
| panic!("expected dispatch error, got status: {result:?}"); | ||
| }; | ||
| let rendered = error.to_string(); | ||
| assert!( | ||
| rendered.contains(needle), | ||
| "expected dispatch error to contain '{needle}', got: {rendered}" | ||
| ); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Return the step failure instead of panicking.
Keep this helper on the same Result<(), String> path as the rest of the module. The direct panic! makes the failure shape inconsistent and drops the useful ?-based chaining used elsewhere in these tests.
🩹 Proposed refactor
fn then_dispatch_error_contains(world: &mut RefactorWorld, text: String) -> Result<(), String> {
let needle = text.trim_matches('"');
let result = world.dispatch_result.as_ref().ok_or("result missing")?;
- let Err(error) = result else {
- panic!("expected dispatch error, got status: {result:?}");
- };
+ let error = result
+ .as_ref()
+ .err()
+ .ok_or_else(|| format!("expected dispatch error, got status: {result:?}"))?;
let rendered = error.to_string();
assert!(
rendered.contains(needle),
"expected dispatch error to contain '{needle}', got: {rendered}"
);Based on learnings, in this repo’s Rust code (especially within test modules/helpers), prefer propagating errors using Result<T, String> and the ? operator.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/weaverd/src/dispatch/act/refactor/behaviour.rs` around lines 372 -
385, The helper then_dispatch_error_contains currently panics when
world.dispatch_result is an Ok variant; change it to return a Result::Err string
instead so the function keeps the Result<(), String> failure shape. In
then_dispatch_error_contains, after obtaining result from
world.dispatch_result.as_ref().ok_or("result missing")?, match on result to
extract the Err(error) case and if it's an Ok(status) return
Err(format!("expected dispatch error, got status: {status:?}")); then continue
using error.to_string() and assert/return Err(...) on mismatch as before so all
failures propagate via Result rather than panic.
Comment on lines
+39
to
45
| let effective_operation = map_effective_operation(&mut plugin_args, args, &resolved_file.path)?; | ||
| plugin_args.insert( | ||
| String::from("refactoring"), | ||
| serde_json::Value::String(effective_operation.clone()), | ||
| ); | ||
| let capability = capability_from_operation(&effective_operation)?; | ||
| let capability = capability_for_operation(&effective_operation)?; | ||
| let file_content = load_file_contents(&resolved_file.path)?; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the duplicated rename-symbol sentinel.
Route the rename-specific argument rewrite from the resolved capability, not from a second hard-coded operation string. That keeps requirements.rs as the sole contract source and avoids silent drift if the mapping changes.
♻️ Proposed refactor
- let effective_operation = map_effective_operation(&mut plugin_args, args, &resolved_file.path)?;
+ let (effective_operation, capability) =
+ map_effective_operation(&mut plugin_args, args, &resolved_file.path)?;
plugin_args.insert(
String::from("refactoring"),
serde_json::Value::String(effective_operation.clone()),
);
- let capability = capability_for_operation(&effective_operation)?;
let file_content = load_file_contents(&resolved_file.path)?;
@@
fn map_effective_operation(
plugin_args: &mut HashMap<String, serde_json::Value>,
args: &arguments::RefactorArgs,
file_path: &Path,
-) -> Result<String, DispatchError> {
+) -> Result<(String, CapabilityId), DispatchError> {
let operation = supported_effective_operation(&args.refactoring)?;
- if operation == "rename-symbol" {
- apply_rename_symbol_mapping(plugin_args, file_path)?;
+ let capability = capability_for_operation(operation)?;
+ match capability {
+ CapabilityId::RenameSymbol => apply_rename_symbol_mapping(plugin_args, file_path)?,
+ _ => {}
}
- Ok(String::from(operation))
+ Ok((String::from(operation), capability))
}Also applies to: 88-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/weaverd/src/dispatch/act/refactor/request_building.rs` around lines 39
- 45, The code currently injects a hard-coded "refactoring" sentinel
("rename-symbol") instead of using the resolved capability; update the logic so
the rename-specific argument rewrite is driven by the capability returned from
capability_for_operation (use the existing effective_operation and capability
variables) rather than a second hard-coded operation string, remove the
duplicated "rename-symbol" sentinel insertion, and apply the same change to the
duplicated block around the other occurrence (the block referenced at the later
88-97 region) so requirements.rs remains the single source of truth.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
weaver act refactor(2.3.4): require provider, refactoring, and file flags, plus a deterministic, full-contract error when arguments are missing. Adds an ExecPlan living document to guide implementation, testing, and docs.Changes
crates/weaverd/src/dispatch/act/refactor/mod.rsto reflect the new required-arguments contract in the command handler and loggingRationale
Review guidance
Implementation plan (summary)
Validation plan
Docs impact
Progress tracking
◳ Generated by DevBoxer
ℹ️ Tag @devboxerhub to address PR feedback
📎 Task: https://www.devboxer.com/task/c84a1800-74c2-4659-a655-422bcd862e05