Define rename-symbol contract and expand capability framework#67
Define rename-symbol contract and expand capability framework#67
Conversation
Reviewer's GuideIntroduces a capability contract subsystem centred on the rename-symbol capability, adds stable reason codes, wires capabilities into manifests and the registry, and exposes these types through the weaver-plugins public API along with unit/BDD tests and documentation updates. Sequence diagram for rename-symbol request validation and plugin routingsequenceDiagram
actor User
participant Daemon
participant PluginRegistry
participant RenameSymbolContract
participant Plugin
User->>Daemon: Request rename-symbol(uri, position, new_name)
Daemon->>Daemon: Build PluginRequest(operation="rename-symbol")
Daemon->>RenameSymbolContract: validate_request(PluginRequest)
RenameSymbolContract-->>Daemon: Result Ok or PluginError
alt request invalid
Daemon-->>User: Error (validation failed)
else request valid
Daemon->>PluginRegistry: find_for_language_and_capability(language, CapabilityId::RenameSymbol)
PluginRegistry-->>Daemon: Vec<PluginManifest>
Daemon->>Plugin: Invoke with PluginRequest
Plugin-->>Daemon: PluginResponse
Daemon->>RenameSymbolContract: validate_response(PluginResponse)
RenameSymbolContract-->>Daemon: Result Ok or PluginError
alt response invalid
Daemon-->>User: Error (contract violation)
else response valid
alt PluginResponse.success == true
Daemon-->>User: Apply Diff output
else PluginResponse.success == false
Daemon->>Daemon: Inspect diagnostics and reason_code
Daemon-->>User: Report failure with reason_code
end
end
end
Class diagram for capability contracts, manifests, registry, and diagnosticsclassDiagram
direction LR
class CapabilityId {
<<enum>>
+RenameSymbol
+ExtricateSymbol
+ExtractMethod
+ReplaceBody
+ExtractPredicate
+as_str() String
}
class ContractVersion {
+u16 major
+u16 minor
+new(major u16, minor u16) ContractVersion
+major() u16
+minor() u16
+is_compatible_with(other ContractVersion) bool
}
class CapabilityContract {
<<interface>>
+capability_id() CapabilityId
+version() ContractVersion
+validate_request(request PluginRequest) Result~(), PluginError~
+validate_response(response PluginResponse) Result~(), PluginError~
}
class RenameSymbolRequest {
-String uri
-String position
-String new_name
+new(uri String, position String, new_name String) RenameSymbolRequest
+uri() String
+position() String
+new_name() String
+extract(request PluginRequest) Result~RenameSymbolRequest, PluginError~
}
class RenameSymbolContract {
+capability_id() CapabilityId
+version() ContractVersion
+validate_request(request PluginRequest) Result~(), PluginError~
+validate_response(response PluginResponse) Result~(), PluginError~
}
class ReasonCode {
<<enum>>
+SymbolNotFound
+MacroGenerated
+AmbiguousReferences
+UnsupportedLanguage
+IncompletePayload
+NameConflict
+OperationNotSupported
+as_str() String
}
class PluginManifest {
-String name
-String version
-PluginKind kind
-Vec~String~ languages
-String executable
-Vec~String~ args
-u64 timeout_secs
-Vec~CapabilityId~ capabilities
+new(name String, version String, kind PluginKind, languages Vec~String~, executable String) PluginManifest
+with_capabilities(capabilities Vec~CapabilityId~) PluginManifest
+validate() Result~(), PluginError~
+capabilities() Vec~CapabilityId~
}
class PluginDiagnostic {
-Severity severity
-String message
-Option~PathBuf~ file
-Option~u32~ line
-Option~ReasonCode~ reason_code
+new(severity Severity, message String) PluginDiagnostic
+message() String
+with_reason_code(code ReasonCode) PluginDiagnostic
+reason_code() Option~ReasonCode~
}
class PluginRegistry {
-manifests Map~String, PluginManifest~
+register(manifest PluginManifest) Result~(), PluginError~
+find_for_capability(id CapabilityId) Vec~PluginManifest~
+find_for_language_and_capability(language String, id CapabilityId) Vec~PluginManifest~
}
class PluginRequest {
-String operation
-Map~String, Value~ arguments
+arguments() Map~String, Value~
}
class PluginResponse {
-bool success
-PluginOutput output
-Vec~PluginDiagnostic~ diagnostics
+is_success() bool
+output() PluginOutput
+diagnostics() Vec~PluginDiagnostic~
}
class PluginOutput {
<<enum>>
+Diff
+Analysis
+Empty
}
CapabilityContract <|.. RenameSymbolContract
RenameSymbolContract ..> RenameSymbolRequest : uses for validation
PluginManifest "*" --> "*" CapabilityId : declares
PluginDiagnostic --> ReasonCode : optional
PluginRegistry "1" --> "*" PluginManifest : stores
CapabilityContract ..> PluginRequest : validates
CapabilityContract ..> PluginResponse : validates
PluginResponse --> PluginOutput
PluginResponse --> PluginDiagnostic
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughSummarise the addition of a capability contract system: capability identifiers and versions, a CapabilityContract trait, stable refusal ReasonCode enum, rename-symbol contract with request/response validation, manifest capability declarations and validation, registry capability queries, plus tests and documentation. Changes
sequenceDiagram
participant Client
participant Broker as Daemon/Broker
participant Registry
participant Plugin as Actuator/Plugin
Client->>Broker: send capability request (operation, args)
Broker->>Registry: find_for_language_and_capability(language, capability)
Registry-->>Broker: matching manifests
Broker->>Plugin: forward PluginRequest
Plugin-->>Broker: PluginResponse (success with Diff | failure with reason_code)
Broker->>Broker: validate response against CapabilityContract
Broker-->>Client: return validated response or error with ReasonCode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaver-plugins/src/manifest/tests.rs Comment on file fn manifest_without_capabilities_deserialises_to_empty() {
let json = r#"{
"name": "test",
"version": "0.1",
"kind": "actuator",
"languages": ["rust"],
"executable": "/bin/test"
}"#;
let m: PluginManifest = serde_json::from_str(json).expect("deserialise");
assert!(m.capabilities().is_empty());
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: crates/weaver-plugins/src/manifest/tests.rs Comment on file assert_manifest_from_json(json, |m| {
assert_eq!(m.timeout_secs(), 30);
assert!(m.args().is_empty());
});
}❌ New issue: Code Duplication |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6271
Why the remaining similarity is acceptableThese are independent test cases with distinct purposes:
Each has:
The pattern
RecommendationAccept this duplication. Test independence and clarity trump DRY when tests are validating different behaviour. If CodeScene continues to flag this, configure it to:
No further refactoring is needed—the tests are well-structured as they stand. |
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: 5397
✅ All Resolved (7/7)Overall Comments
Individual Comments
All review comments have been successfully addressed! 🎉 (\_/) |
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/weaver-plugins/src/capability/tests.rs`:
- Around line 185-255: Multiple tests repeat the same RenameSymbolContract
setup; convert them to rstest fixtures and parameterised cases by creating a
fixture that returns the RenameSymbolContract instance (and another fixture that
returns the common args via make_rename_args) and then replace redundant tests
(contract_validate_valid_request, contract_validate_invalid_request,
contract_validate_wrong_operation_rejects,
contract_validate_successful_diff_response,
contract_validate_successful_non_diff_response_fails,
contract_validate_failed_response_passes) with #[rstest] parameterised cases
that take the fixture(s) and inputs (e.g., operation name, PluginOutput
variants, PluginResponse success/failure) and assert
validate_request/validate_response outcomes; keep separate small tests for
capability_id() and version() (contract_capability_id and contract_version)
unchanged but call the fixture if desired.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
crates/weaver-plugins/src/capability/mod.rscrates/weaver-plugins/src/capability/tests.rsdocs/execplans/5-1-1-entry-help.md
…h validation - Define CapabilityId enum for plugin refactoring capabilities as per ADR 001. - Implement ContractVersion for versioning with compatibility checks. - Create CapabilityContract trait for validating plugin requests and responses. - Add rename-symbol contract with typed request, validation logic for requests and responses. - Introduce ReasonCode enum for standardized refusal diagnostics. - Extend PluginManifest to declare supported capabilities with validation. - Add optional reason_code field to PluginDiagnostic for structured refusal reasons. - Implement PluginRegistry methods for capability-based plugin lookup. - Provide comprehensive unit and BDD tests validating contract and manifest extensions. - Update documentation with detailed capability contract and usage info. This completes roadmap item 5.2.1 by enforcing a stable, versioned contract for the rename-symbol capability, enabling reliable broker-plugin interactions and structured error handling. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…tests Introduced `assert_manifest_from_json` helper to reduce repeated deserialization code in manifest tests, improving readability and maintainability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…act requests - Introduce non-empty string checks for 'uri', 'position', and 'new_name' fields in rename-symbol requests. - Add operation check to ensure requests match the expected 'rename-symbol' operation. - Improve error messaging for invalid or empty fields in request extraction. - Update BDD scenarios and unit tests to cover new validation cases. - Refactor capability contract validation error descriptions for clarity. - Adjust manifest validation error to clarify sensor plugins must not declare any capabilities. - Minor docs updates for terminology consistency and user guide corrections. These changes increase robustness of the rename-symbol capability contract by ensuring requests are correctly formed and explicitly validated before processing. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…lookup Normalize all language entries in plugin manifests to ASCII lowercase during registration. This enables allocation-free lookups when querying plugins by language, improving performance and consistency in language matching. Additionally, update the find_for_language and find_for_language_and_capability methods to leverage the normalized language list, avoiding repeated allocations and case conversions. Update docs to fix a minor wording issue regarding ADR 001. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
- Updated spelling of `serialize` and `deserialize` to British English `serialise` and `deserialise` in capability plugin tests. - Fixed documentation to spell `centralised` correctly. - Minor wording clarification in capability module docs (expanded ADR to full form). Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Refactor RenameSymbolContract tests to utilize rstest fixtures for cleaner and more maintainable test code. Introduced fixtures for contract and valid rename requests, replacing manual setup in each test case. The changes improve test readability and leverage rstest features effectively. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Added assertions using matches! to verify that errors returned from validate_request and validate_response are specifically of type PluginError::InvalidOutput. This improves test precision and clarity by checking error variants, not only error messages. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…tests Refined the error handling in rename-symbol related tests by matching PluginError::InvalidOutput explicitly. This improves test clarity and robustness by verifying specific error variants and their messages instead of relying on string containment checks alone. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
77c663e to
43c6d3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-plugins/tests/features/capability_contract.feature`:
- Around line 72-75: Add a trailing newline at the end of the feature file so
the file ends with a single newline character; locate the "Scenario: Contract
version incompatibility with different major" block (the Given/And/Then lines)
and ensure there is a newline after the final line "Then the versions are
incompatible" so the file conforms to POSIX/linter expectations.
In `@docs/execplans/5-2-1-define-the-rename-symbol-capability-contract.md`:
- Around line 410-421: Insert an explicit caption line immediately above the
Markdown table that starts with the header row "| File
| Current lines | Planned additions | Projected total |" in
docs/execplans/5-2-1-define-the-rename-symbol-capability-contract.md; add a
brief descriptive caption (e.g., "Table: Line-budget projection for files
modified by the rename-symbol capability") on its own line so the table is
captioned per the documentation guideline.
In `@docs/users-guide.md`:
- Around line 730-737: The tables listing capability identifiers (e.g., rows
containing `rename-symbol`, `extricate-symbol`, `extract-method`,
`replace-body`, `extract-predicate`) need a one-line caption immediately above
each table; update the documentation by inserting a short descriptive caption
line (e.g., "Table: Code transformation capabilities") directly before each such
markdown table in the capabilities section, and apply the same change to the
other tables referenced that list identifiers (the tables near the other
identifier groups noted in the comment) so every table and diagram has a
caption.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
crates/weaver-plugins/src/capability/mod.rscrates/weaver-plugins/src/capability/reason_code.rscrates/weaver-plugins/src/capability/rename_symbol.rscrates/weaver-plugins/src/capability/tests.rscrates/weaver-plugins/src/lib.rscrates/weaver-plugins/src/manifest/mod.rscrates/weaver-plugins/src/manifest/tests.rscrates/weaver-plugins/src/protocol/mod.rscrates/weaver-plugins/src/protocol/tests.rscrates/weaver-plugins/src/registry/mod.rscrates/weaver-plugins/src/registry/tests.rscrates/weaver-plugins/src/tests/capability_behaviour.rscrates/weaver-plugins/src/tests/mod.rscrates/weaver-plugins/tests/features/capability_contract.featuredocs/execplans/5-2-1-define-the-rename-symbol-capability-contract.mddocs/roadmap.mddocs/users-guide.md
Insert descriptive caption lines immediately above each Markdown table in the rename-symbol execplan and users-guide so every table is captioned per the documentation guidelines. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
The shared rust-build-release GitHub Action resolves binary paths from Cargo manifest metadata. Without an explicit [[bin]] entry the action returned `undefined`, causing "Unable to locate executable file: undefined" during the "Get human-readable stats" step of the release dry run. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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)
docs/users-guide.md (1)
848-848:⚠️ Potential issue | 🟡 MinorExpand the LSP acronym on first use.
The acronym "LSP" appears here without prior expansion. Spell out "Language Server Protocol (LSP)" on first use to comply with documentation standards.
📝 Proposed fix
-The daemon sends the LSP `initialize` handshake followed by +The daemon sends the Language Server Protocol (LSP) `initialize` handshake followed byAs per coding guidelines: "Expand any uncommon acronym on first use in documentation (e.g., Continuous Integration (CI))."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/users-guide.md` at line 848, Update the documentation to expand the acronym on first use: replace the bare "LSP `initialize` handshake" with "Language Server Protocol (LSP) `initialize` handshake" (locate the occurrence of "LSP" near the phrase "`initialize` handshake" in the users-guide text) so the full form appears once before continuing to use the "LSP" abbreviation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/5-2-1-define-the-rename-symbol-capability-contract.md`:
- Around line 312-313: Replace US spellings with en-GB throughout the document:
change every occurrence of "serialized" to "serialised" and "serialization" to
"serialisation" in narrative prose (for example the phrase "serialized as
snake_case strings" and the later occurrence around the "serialization"
wording). Ensure only prose is changed (leave code, API names or examples that
must remain US-spelled untouched) and run a quick search for both terms to
update all instances consistently.
- Around line 26-27: Update the documented validation steps that currently state
"Running `make check-fmt && make lint && make test` passes..." to include the
additional Markdown quality gates by adding `make markdownlint`, `make fmt`, and
`make nixie` to the sequence and examples; specifically edit the occurrence
around the existing `make check-fmt && make lint && make test` sentence and the
repeated block referenced (the other occurrence in the 356-380 region) so the
docs show running `make check-fmt`, then `make markdownlint`, `make fmt`, `make
nixie`, `make lint`, and `make test` (and update any example shell snippet to
pipe output to logs as in the proposed patch).
---
Outside diff comments:
In `@docs/users-guide.md`:
- Line 848: Update the documentation to expand the acronym on first use: replace
the bare "LSP `initialize` handshake" with "Language Server Protocol (LSP)
`initialize` handshake" (locate the occurrence of "LSP" near the phrase
"`initialize` handshake" in the users-guide text) so the full form appears once
before continuing to use the "LSP" abbreviation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3ae9d22-400f-4e06-befb-6a7df9d5fe71
📒 Files selected for processing (3)
crates/weaverd/Cargo.tomldocs/execplans/5-2-1-define-the-rename-symbol-capability-contract.mddocs/users-guide.md
Spell out "Language Server Protocol (LSP)" at the first occurrence so readers encounter the full form before subsequent abbreviations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Stage D modifies documentation, so the quality gates and validation commands must include `make markdownlint` alongside the existing fmt, lint, and test checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/execplans/5-2-1-define-the-rename-symbol-capability-contract.md (1)
26-27:⚠️ Potential issue | 🟠 MajorAdd the full documentation quality gates to validation commands.
Update every documented validation sequence to include
make markdownlint,make fmt, andmake nixie, and keep the shell example aligned.Proposed patch
-- Running `make check-fmt && make lint && make test && make markdownlint` +- Running `make check-fmt && make markdownlint && make fmt && make nixie && make lint && make test` passes with no regressions. @@ -- Validation: `make check-fmt && make lint && make test && make markdownlint` +- Validation: `make check-fmt && make markdownlint && make fmt && make nixie && make lint && make test` all pass. @@ ```sh set -o pipefail make check-fmt 2>&1 | tee /tmp/check-fmt.log +make markdownlint 2>&1 | tee /tmp/markdownlint.log +make fmt 2>&1 | tee /tmp/fmt.log +make nixie 2>&1 | tee /tmp/nixie.log make lint 2>&1 | tee /tmp/lint.log make test 2>&1 | tee /tmp/test.log -make markdownlint 2>&1 | tee /tmp/markdownlint.log</details> As per coding guidelines: "`**/*.md`: Validate Markdown files using `make markdownlint`.", "`**/*.md`: Run `make fmt` after any documentation changes to format all Markdown files and fix table markup.", and "`**/*.md`: Validate Mermaid diagrams in Markdown files by running `make nixie`." Also applies to: 356-357, 377-383 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/execplans/5-2-1-define-the-rename-symbol-capability-contract.mdaround
lines 26 - 27, Update the documented validation sequences (the shell example
that currently runs "make check-fmt && make lint && make test && make
markdownlint") to include the full documentation quality gates by adding "make
markdownlint", "make fmt", and "make nixie" to the sequence and reorder the
tee'd commands to match the proposed patch (e.g., run "make check-fmt", then
"make markdownlint", "make fmt", "make nixie", then "make lint" and "make test")
so the example and each listed validation step (references: the shell example
lines invoking make check-fmt, make lint, make test, make markdownlint) are
consistent with the coding guidelines for validating and formatting Markdown.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In@docs/execplans/5-2-1-define-the-rename-symbol-capability-contract.md:
- Around line 26-27: Update the documented validation sequences (the shell
example that currently runs "make check-fmt && make lint && make test && make
markdownlint") to include the full documentation quality gates by adding "make
markdownlint", "make fmt", and "make nixie" to the sequence and reorder the
tee'd commands to match the proposed patch (e.g., run "make check-fmt", then
"make markdownlint", "make fmt", "make nixie", then "make lint" and "make test")
so the example and each listed validation step (references: the shell example
lines invoking make check-fmt, make lint, make test, make markdownlint) are
consistent with the coding guidelines for validating and formatting Markdown.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: ASSERTIVE **Plan**: Pro **Run ID**: `bf21fa96-7902-4bad-b5d0-f358fe1fad07` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 6f616aa61f380a48a51e7bfb384c434450a15b86 and 21a9f435c92281a29964a2911377c834668be3bf. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `docs/execplans/5-2-1-define-the-rename-symbol-capability-contract.md` * `docs/users-guide.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Summary
Introduce a formal capability contract system for plugin capabilities (starting with rename-symbol) and expand the plugin ecosystem to support capability declarations, contract validation, and capability-based discovery. This includes a new capability module, typed request validation, stable reason codes, and registry lookups, along with tests and documentation updates.
Changes
Rationale
Compatibility and Migration
Testing plan
Documentation and users' guide updates
Review tips
If you want me to tweak the PR message (tone, depth, or section order), tell me your preferences and I’ll adjust accordingly.
📎 Task: https://www.devboxer.com/task/6720b407-5feb-4589-8ee8-2a3572c2ae4a
Summary by Sourcery
Introduce a versioned capability contract system (starting with rename-symbol) for actuator plugins, including manifest capability declarations, capability-based registry lookup, structured refusal reason codes on diagnostics, and corresponding tests and documentation updates.
New Features:
Enhancements:
Tests: