Conversation
textDocument/rename now runs the same provider-owned mutation path as the CLI and agent surfaces: the provider computes edits over the open document set, Wright verifies versions and source preconditions, the provider validates the transaction, and the edited project is rechecked before a WorkspaceEdit is returned. Unopen or unsupported documents, unconfigured providers, and provider or validation refusals answer with a RequestFailed error whose error.data.code carries the structured refusal code; no textual fallback exists. run_provider_flow moves from ToolService to CompilerSession so the language service shares the one provider lifecycle path. wright-lsp gains --opy-provider for an explicit provider executable, and the MCP surface advertises providerSemanticRename/providerValidateEdit as wright_provider_semantic_rename/wright_provider_validate_edit. Closes #156
…params edit_range re-implemented span_to_range's 1-based-char-col to UTF-16 conversion with a subtly different line split: splitting on a newline char keeps the carriage return while str::lines drops it, so a column at end-of-line on a CRLF file converted differently. Both converters now share document::line_col_position. parse_params unwrapped the params Option, so a request without params panicked the server loop; it now returns the same Err as malformed params.
parse_params returned Err for missing or malformed params, but every call site propagated it out of the dispatch loop, so a single bad request still ended the session without a wire response. read_params now answers requests with -32602 carrying the original id and skips malformed notifications (which get no response by spec), then the loop continues.
A textDocument/rename sent without an id is a notification: it gets no response and no provider call.
Teakowa
left a comment
There was a problem hiding this comment.
One reproducible stale-edit safety defect blocks acceptance. See the inline finding.
Teakowa
left a comment
There was a problem hiding this comment.
Two correctness blockers remain in the LSP rename path.
…t check Review on #498 flagged two defects in the provider-driven rename: - Applied renames returned WorkspaceEdit.changes, which cannot carry the document version the provider validated. A client whose buffer moved while the rename was pending would apply a stale edit without detecting it, violating the documented freshness contract. SourceTextEdit now carries the validated version, the LSP adapter answers with WorkspaceEdit.documentChanges (TextDocumentEdits tagged with OptionalVersionedTextDocumentIdentifier), and rename is only advertised and served when the client negotiates workspace.workspaceEdit.documentChanges; otherwise the request is refused (rename-unversioned-workspace-edit) rather than risk an unversioned edit. - The post-edit check failed the mutation on error diagnostics from any supplied document, so an unrelated broken open document could block a valid target-project rename. The supplied set stays language-wide (only the provider can compute project membership), but the check verdict is now scoped to the documents the provider actually edited plus the position document; validate_transaction keeps the whole caller set because its document set is the declared project. Regressions: driver-level scripted-provider tests pin the scope behavior, and WRIGHT_OPY_PROVIDER-gated tests at the language-service and LSP boundaries cover a real rename alongside an unrelated broken document, versioned documentChanges, and a buffer change while the rename is pending.
Teakowa
left a comment
There was a problem hiding this comment.
The two previous correctness blockers are fixed. One verification blocker remains: the new real-provider regressions do not run in CI.
Review on #498 noted the WRIGHT_OPY_PROVIDER-gated rename regressions self-skip in CI: Product integration installed the pinned OPY provider only after its test steps, and the reusable quality job has no provider at all. The job now pins the provider version and its store in job-level env, installs before the test step that consumes it, and runs the wright-language/wright-lsp suites with WRIGHT_OPY_PROVIDER pointed at the pinned binary — a stale-edit or project-scope regression now fails the job instead of reporting success via early return.
…regressions The previous step pointed the WRIGHT_OPY_PROVIDER-gated tests at the released 0.1.38 distribution, which predates lpp/rename and refused with capability-unavailable in CI. Following the LPP-integration pattern, the job now checks out the pinned opy-rs commit the tests were validated against and builds opy-provider from source; the pin advances through normal deps: bumps as the provider releases. The release-pin install and compile smoke check stay unchanged — they exercise the distribution path itself.
Retracted: requiring Wright CI to run these regressions against a pinned real OPY provider was the wrong verification boundary. Wright should verify its own LPP/edit/LSP contract hermetically; real OPY compatibility should track the owner implementation rather than a historical snapshot.
Teakowa
left a comment
There was a problem hiding this comment.
Correction to my previous review: do not make Wright's rename regressions depend on a pinned OPY implementation. Remove the new cross-repository snapshot coupling and keep Wright-owned behavior hermetically testable.
| # so they build the pinned opy-rs commit the tests were validated | ||
| # against instead of self-skipping. The pin advances through normal | ||
| # `deps:` bumps as the provider releases. | ||
| - name: Checkout opy-rs |
There was a problem hiding this comment.
This new pinned opy-rs checkout is the wrong verification boundary. It freezes Wright's product regression against one historical owner implementation, so later OPY releases (including a future 1.0) can change without this gate exercising them. Remove this checkout/build/test coupling. Keep the Wright-owned guarantees deterministic inside Wright: the driver scoping regression already uses a scripted provider; cover the LSP versioned-documentChanges conversion/capability behavior with a hermetic contract/mock or a pure adapter test. Real OPY end-to-end compatibility should track the current owner implementation on the compatibility/owner surface, not a commit pinned into Wright CI.
Summary
Wires provider-owned mutation (#156) into the two remaining product surfaces, over the shared driver path:
textDocument/renameinwright-lsp— routes throughLanguageService::rename, which runssemantic_renameover the open document set via the session's provider flow: provider-computed edits → Wright version/precondition checks →lpp/validateEdits→ edited-project recheck → versionedWorkspaceEdit.documentChanges. Refusals (unopen/unsupported documents, unconfigured providers, provider and validation refusals) answerRequestFailed(-32803) with the structured code preserved inerror.data.code.--opy-provider <PATH>pins an explicit provider executable.documentChanges: eachTextDocumentEditcarries the document version the provider computed and Wright re-validated, so a client that advanced its buffer while the rename was pending can reject the edit.renameProvideris advertised only when the client negotiatesworkspace.workspaceEdit.documentChanges; without it a rename request is refused (rename-unversioned-workspace-edit) rather than served an unversionedchangesmap a stale buffer could silently absorb.validate_transactionkeeps the whole caller set as the declared project.providerSemanticRename/providerValidateEditare listed aswright_provider_semantic_rename/wright_provider_validate_edit(they were reachable viaserve's genericrequestop but invisible to MCP agents).run_provider_flowmoves fromToolServicetoCompilerSessionso CLI/service and language surfaces use one provider lifecycle (spawn → initialize → flow → graceful shutdown).LanguageService::with_configis the configuration seam for provider executables/registries.Evidence
cargo fmt --all -- --check,cargo clippy --workspace --all-targets --all-features -- -D warnings,cargo test --workspace --all-targets --all-features(withWRIGHT_OPY_PROVIDERandLPP_MOCK_PROVIDERset): clean.opy-provider:textDocument/renameonscoreBankindefs.opyreturned a cross-documentWorkspaceEditcovering declaration + 3 references across two files (UTF-16 correct, tab-indented line handled); a rename on a non-symbol position returned the provider's structuredrename.noSymbolAtPositionrefusal.WRIGHT_OPY_PROVIDER-gated regressions:rename_returns_versioned_document_changes_scoped_to_the_projectsends a rename, changes the buffer to v2 before reading the response, and asserts versioneddocumentChangescarrying v1 alongside an unrelated broken#!includedocument open in the same language;an_unrelated_broken_open_document_does_not_block_a_project_renamepins the same scoping at the language-service boundary.rename_post_edit_check_drops_unrelated_open_document_errorsandvalidate_transaction_check_scope_covers_the_whole_caller_setpin the check-scope asymmetry.wright serve --transport mcp→wright_provider_semantic_renamereturnedok: truewith the same 4-edit validated transaction.projectRootsent as a file URI, DocumentSet filtered to the target language, partial-edit drops made explicit refusals,--opy-providerrequires its value.Test plan
documentChanges+ pending-buffer-change regression