From ee55d0ee6348c6a9a09706e20c82d52d9466563e Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Sun, 4 Oct 2026 03:45:59 +0800 Subject: [PATCH 1/9] feat(lsp,language): wire provider-driven rename through product surfaces 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 --- Cargo.lock | 1 + crates/wright-cli/src/mcp.rs | 14 ++ crates/wright-cli/tests/serve.rs | 6 + crates/wright-driver/src/service.rs | 33 +-- crates/wright-driver/src/session.rs | 30 +++ crates/wright-language/Cargo.toml | 1 + crates/wright-language/src/lib.rs | 3 +- crates/wright-language/src/service.rs | 327 +++++++++++++++++++++++++- crates/wright-lsp/src/main.rs | 125 +++++++++- crates/wright-lsp/tests/lsp.rs | 105 ++++++++- docs/agent-contract.md | 8 +- docs/language-services.md | 30 ++- 12 files changed, 627 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7df94048..c6f14ab0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2165,6 +2165,7 @@ dependencies = [ "url", "workshop-rs", "wright-driver", + "wright-lpp", ] [[package]] diff --git a/crates/wright-cli/src/mcp.rs b/crates/wright-cli/src/mcp.rs index a06c6d02..c7b89585 100644 --- a/crates/wright-cli/src/mcp.rs +++ b/crates/wright-cli/src/mcp.rs @@ -101,6 +101,18 @@ const TOOLS: &[ToolSpec] = &[ drop_fields: &["sources"], description: "Validate and preview a source-edit transaction atomically against the session's project; no filesystem writes. Sources default to the on-disk text.", }, + ToolSpec { + op: "providerSemanticRename", + request_def: "ProviderSemanticRenameRequest", + drop_fields: &[], + description: "Provider-owned rename for source-language projects (e.g. OverPy): the configured provider computes the edits, Wright verifies document versions and source preconditions, the provider validates the transaction, and the edited project is rechecked — returning validated edits or a structured refusal. `documents` and `sources` are supplied by the caller.", + }, + ToolSpec { + op: "providerValidateEdit", + request_def: "ProviderValidateEditRequest", + drop_fields: &[], + description: "Validate a caller-proposed source-edit transaction for a provider-owned language through the same provider-backed pipeline as providerSemanticRename; no filesystem writes. `documents` and `sources` are supplied by the caller.", + }, ]; /// A tool definition as listed by `tools/list`. @@ -359,6 +371,8 @@ mod tests { assert_eq!(names[7], "wright_cost_estimate"); assert_eq!(names[8], "wright_semantic_rename"); assert_eq!(names[9], "wright_validate_edit_transaction"); + assert_eq!(names[10], "wright_provider_semantic_rename"); + assert_eq!(names[11], "wright_provider_validate_edit"); assert!(names.iter().all(|name| name.starts_with("wright_"))); } diff --git a/crates/wright-cli/tests/serve.rs b/crates/wright-cli/tests/serve.rs index d606766a..04c60987 100644 --- a/crates/wright-cli/tests/serve.rs +++ b/crates/wright-cli/tests/serve.rs @@ -650,6 +650,8 @@ fn mcp_transport_lists_the_initial_tool_set_within_capabilities() { "wright_cost_estimate", "wright_semantic_rename", "wright_validate_edit_transaction", + "wright_provider_semantic_rename", + "wright_provider_validate_edit", ] ); // tools/list is a subset of the contract's advertised operations. @@ -667,6 +669,8 @@ fn mcp_transport_lists_the_initial_tool_set_within_capabilities() { "cost_estimate" => "costEstimate", "semantic_rename" => "semanticRename", "validate_edit_transaction" => "validateEditTransaction", + "provider_semantic_rename" => "providerSemanticRename", + "provider_validate_edit" => "providerValidateEdit", other => other, }; assert!( @@ -739,6 +743,8 @@ fn mcp_transport_results_match_the_service_contract() { "cost_estimate" => "costEstimate", "semantic_rename" => "semanticRename", "validate_edit_transaction" => "validateEditTransaction", + "provider_semantic_rename" => "providerSemanticRename", + "provider_validate_edit" => "providerValidateEdit", other => other, }; let mut request = arguments.clone(); diff --git a/crates/wright-driver/src/service.rs b/crates/wright-driver/src/service.rs index 40cd6cf6..ee7dcdd8 100644 --- a/crates/wright-driver/src/service.rs +++ b/crates/wright-driver/src/service.rs @@ -600,16 +600,8 @@ impl<'a> ToolService<'a> { self.session.language_provider(language_id) } - /// Run a provider-driven mutation flow (#139) over a fresh provider - /// session: spawn by opaque language id, initialize, run the flow, and - /// terminate gracefully. - /// - /// Any failure before the flow — an unconfigured language id, a spawn - /// failure, a failed handshake — is the same structured - /// [`crate::provider_edit::ProviderMutation`] refusal surface the flow - /// itself uses, so callers handle one refusal contract. The provider - /// process never outlives the request: graceful shutdown when possible, - /// and the session's drop guard terminates it otherwise. + /// Run a provider-driven mutation flow (#139) through the session's + /// shared provider lifecycle path. fn run_provider_flow( &self, language_id: &str, @@ -617,19 +609,14 @@ impl<'a> ToolService<'a> { &mut dyn wright_lpp::LanguageProvider, ) -> crate::provider_edit::ProviderMutation, ) -> crate::provider_edit::ProviderMutation { - let mut provider = match self.session.language_provider(language_id) { - Ok(provider) => provider, - Err(error) => return crate::provider_edit::provider_failure(&error), - }; - if let Err(error) = provider.initialize(Some(&wright_lpp::ClientInfo { - name: SERVICE_NAME.to_string(), - version: SERVICE_VERSION.to_string(), - })) { - return crate::provider_edit::provider_failure(&error); - } - let mutation = flow(provider.as_mut()); - let _ = provider.shutdown(); - mutation + self.session.run_provider_flow( + language_id, + &wright_lpp::ClientInfo { + name: SERVICE_NAME.to_string(), + version: SERVICE_VERSION.to_string(), + }, + flow, + ) } fn ok(&self, result: serde_json::Value) -> ToolResponse { diff --git a/crates/wright-driver/src/session.rs b/crates/wright-driver/src/session.rs index 46df31ea..c4af9a44 100644 --- a/crates/wright-driver/src/session.rs +++ b/crates/wright-driver/src/session.rs @@ -529,6 +529,36 @@ impl CompilerSession { .map(|b| Box::new(b) as Box) } + /// Run a provider-driven mutation flow (#139) over a fresh provider + /// session: spawn by opaque language id, initialize with `client`, run + /// the flow, and terminate gracefully. + /// + /// Any failure before the flow — an unconfigured language id, a spawn + /// failure, a failed handshake — is the same structured + /// [`crate::provider_edit::ProviderMutation`] refusal surface the flow + /// itself uses, so callers handle one refusal contract. The provider + /// process never outlives the request: graceful shutdown when possible, + /// and the provider's drop guard terminates it otherwise. + pub fn run_provider_flow( + &self, + language_id: &str, + client: &wright_lpp::ClientInfo, + flow: impl FnOnce( + &mut dyn wright_lpp::LanguageProvider, + ) -> crate::provider_edit::ProviderMutation, + ) -> crate::provider_edit::ProviderMutation { + let mut provider = match self.language_provider(language_id) { + Ok(provider) => provider, + Err(error) => return crate::provider_edit::provider_failure(&error), + }; + if let Err(error) = provider.initialize(Some(client)) { + return crate::provider_edit::provider_failure(&error); + } + let mutation = flow(provider.as_mut()); + let _ = provider.shutdown(); + mutation + } + /// The locale a Workshop input resolved to, if the last load was /// Workshop-origin. pub fn resolved_locale(&self) -> Option { diff --git a/crates/wright-language/Cargo.toml b/crates/wright-language/Cargo.toml index 6001b082..042d191c 100644 --- a/crates/wright-language/Cargo.toml +++ b/crates/wright-language/Cargo.toml @@ -14,5 +14,6 @@ workspace = true serde = { workspace = true, features = ["derive"] } url = "2" wright-driver.workspace = true +wright-lpp = { path = "../wright-lpp" } # Single pinned reference: `[workspace.dependencies]` in the root Cargo.toml. workshop-rs.workspace = true diff --git a/crates/wright-language/src/lib.rs b/crates/wright-language/src/lib.rs index 06b00e05..92777f5f 100644 --- a/crates/wright-language/src/lib.rs +++ b/crates/wright-language/src/lib.rs @@ -2,4 +2,5 @@ pub mod document; pub mod service; pub use document::{Document, DocumentStore, Position, Range}; -pub use service::{LanguageService, SourceDiagnostic}; +pub use service::{LanguageService, RenameOutcome, SourceDiagnostic, SourceTextEdit}; +pub use wright_driver::{OpyProviderConfig, SessionConfig}; diff --git a/crates/wright-language/src/service.rs b/crates/wright-language/src/service.rs index db307f5c..a8e80ab6 100644 --- a/crates/wright-language/src/service.rs +++ b/crates/wright-language/src/service.rs @@ -1,10 +1,12 @@ //! The editor-neutral language service (#63, #65, #66). +use std::collections::BTreeMap; use std::path::PathBuf; use serde::Serialize; +use wright_driver::{CompilerSession, SessionConfig}; -use crate::document::{DocumentStore, Position, Range}; +use crate::document::{DocumentStore, Position, Range, char_offset_to_utf16}; #[derive(Debug, Clone, Serialize)] pub struct SourceDiagnostic { @@ -17,16 +19,47 @@ pub struct SourceDiagnostic { pub document_version: i32, } +/// One validated text replacement produced by a provider-owned edit, in the +/// document store's UTF-16 coordinates. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SourceTextEdit { + pub uri: String, + pub range: Range, + pub new_text: String, +} + +/// The outcome of a provider-owned source mutation (#156): validated edits +/// the caller applies, or an explicit structured refusal — unsupported +/// documents, an unconfigured provider, a provider refusal, and validation +/// failures all land in the same channel rather than a textual fallback. +#[derive(Debug)] +pub enum RenameOutcome { + Applied(Vec), + Refused { code: String, message: String }, +} + pub struct LanguageService { pub store: DocumentStore, pub root: PathBuf, + config: SessionConfig, + session: Option, } impl LanguageService { pub fn new(root: PathBuf) -> LanguageService { + Self::with_config(root, SessionConfig::default()) + } + + /// A service whose session behavior is explicitly configured — hosts + /// register providers or point the OPY provider resolver at a concrete + /// executable through `config`. The session itself is constructed lazily + /// on the first provider request. + pub fn with_config(root: PathBuf, config: SessionConfig) -> LanguageService { LanguageService { store: DocumentStore::new(root.clone()), root, + config, + session: None, } } @@ -42,7 +75,9 @@ impl LanguageService { range: empty_range(), severity: "error".to_string(), code: "source-provider-unavailable".to_string(), - message: "source-language analysis is provider-owned and no editor capability is currently negotiated".to_string(), + message: + "source-language analysis is provider-owned; diagnostics are not yet negotiated" + .to_string(), source_version: document.version, document_version: document.version, }] @@ -55,12 +90,176 @@ impl LanguageService { Vec::new() } } + + /// Rename the symbol at `position` in an open source-language document + /// (#156). + /// + /// The request runs the same provider-owned mutation path as the CLI + /// and agent surfaces: the provider computes the edits for the open + /// document set, Wright verifies document versions, applies source + /// preconditions, asks the provider to validate the transaction, and + /// checks the edited project before returning anything. A refusal — + /// unopen or unsupported document, unconfigured provider, provider + /// refusal, validation failure — is structured and final; no textual + /// fallback exists here. + pub fn rename(&mut self, uri: &str, position: Position, new_name: &str) -> RenameOutcome { + if self.store.document(uri).is_none() { + return RenameOutcome::Refused { + code: "rename-unknown-document".to_string(), + message: format!("{uri} is not open in this session"), + }; + } + let Some(language_id) = source_language_id(uri) else { + return RenameOutcome::Refused { + code: "rename-unsupported-document".to_string(), + message: format!( + "rename is provider-owned and {uri} is not a source-language document" + ), + }; + }; + + // The document set holds this language's open documents: foreign + // source documents cannot produce rename edits and an unrelated + // diagnostic on one must not block this rename. + let mut documents = wright_lpp::DocumentSet::new(); + let mut sources = BTreeMap::new(); + for doc_uri in self.store.uris() { + if source_language_id(doc_uri).as_deref() != Some(language_id.as_str()) { + continue; + } + let doc = self + .store + .document(doc_uri) + .expect("uri came from the store"); + documents.insert( + doc_uri.to_string(), + wright_lpp::Document { + uri: doc.uri.clone(), + language_id: language_id.clone(), + version: doc.version as i64, + text: doc.text.clone(), + }, + ); + sources.insert(doc_uri.to_string(), doc.text.clone()); + } + let request = wright_driver::provider_edit::ProviderRenameRequest { + documents, + position_document_uri: uri.to_string(), + position: wright_lpp::Position { + line: position.line, + character: position.character, + }, + new_name: new_name.to_string(), + project_root: url::Url::from_directory_path(&self.root) + .ok() + .map(|u| u.to_string()), + sources, + }; + + if self.session.is_none() { + self.session = match CompilerSession::new(self.config.clone()) { + Ok(session) => Some(session), + Err(diagnostic) => { + return RenameOutcome::Refused { + code: diagnostic.code, + message: diagnostic.message, + }; + } + }; + } + let mutation = self + .session + .as_ref() + .expect("session initialized above") + .run_provider_flow( + &language_id, + &wright_lpp::ClientInfo { + name: "wright-language".to_string(), + version: env!("CARGO_PKG_VERSION").to_string(), + }, + |provider| wright_driver::provider_edit::semantic_rename(provider, &request), + ); + + if !mutation.ok { + let diagnostic = mutation.diagnostics.first(); + return RenameOutcome::Refused { + code: mutation + .provider_code + .or_else(|| diagnostic.map(|d| d.code.clone())) + .unwrap_or_else(|| "rename-refused".to_string()), + message: mutation + .provider_message + .or_else(|| diagnostic.map(|d| d.message.clone())) + .unwrap_or_else(|| "the provider refused the rename".to_string()), + }; + } + let edits = mutation + .transaction + .map(|transaction| transaction.edits) + .unwrap_or_default(); + let mut applied = Vec::with_capacity(edits.len()); + for edit in edits { + // `finish_transaction` refuses edits outside the document set, so + // every source is an open document; a partial `Applied` set must + // never escape. + let Some(doc) = self.store.document(&edit.source) else { + return RenameOutcome::Refused { + code: "rename-edit-outside-open-documents".to_string(), + message: format!( + "the provider validated an edit against {}, which is not an open document", + edit.source + ), + }; + }; + applied.push(SourceTextEdit { + uri: edit.source, + range: edit_range(&doc.text, &edit.range), + new_text: edit.new_text, + }); + } + RenameOutcome::Applied(applied) + } +} + +/// Map a document URI to its provider language id when it is a +/// source-language document. +fn source_language_id(uri: &str) -> Option { + let ext = crate::document::uri_to_path(uri)? + .extension()? + .to_string_lossy() + .to_lowercase(); + match ext.as_str() { + "opy" => Some(wright_driver::opy_provider::OPY_LANGUAGE_ID.to_string()), + // DEL/OSTW have no provider implementation yet; the extension is the + // opaque id so the registry refusal stays explicit and names the + // language once a provider ships. + "ostw" | "del" => Some(ext), + _ => None, + } } fn is_source_document(uri: &str) -> bool { - crate::document::uri_to_path(uri) - .and_then(|p| p.extension().map(|e| e.to_string_lossy().to_lowercase())) - .is_some_and(|ext| matches!(ext.as_str(), "opy" | "ostw" | "del")) + source_language_id(uri).is_some() +} + +/// Convert a driver [`wright_driver::edit::EditRange`] (1-based line and +/// character column, half-open) back into UTF-16 document coordinates. +fn edit_range(text: &str, range: &wright_driver::edit::EditRange) -> Range { + let lines: Vec<&str> = text.split('\n').collect(); + let position = |line: u32, col: u32| { + let line_index = line.saturating_sub(1) as usize; + let character = lines.get(line_index).map_or(0, |line| { + char_offset_to_utf16(line, col.saturating_sub(1) as usize) + }); + Position { + line: line_index as u32, + character: character as u32, + } + }; + Range { + start: position(range.start_line, range.start_col), + end: position(range.end_line, range.end_col), + } } fn empty_range() -> Range { @@ -75,3 +274,121 @@ fn empty_range() -> Range { }, } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::document::Document; + + fn service() -> LanguageService { + LanguageService::new(PathBuf::from("/project")) + } + + fn open(service: &mut LanguageService, name: &str, text: &str) -> String { + let uri = format!("file:///project/{name}"); + service.store.open(Document::with_version( + uri.clone(), + text, + PathBuf::from("/project"), + 1, + )); + uri + } + + fn refused(outcome: RenameOutcome) -> (String, String) { + match outcome { + RenameOutcome::Refused { code, message } => (code, message), + RenameOutcome::Applied(_) => panic!("expected a refusal, got applied edits"), + } + } + + #[test] + fn rename_on_an_unopen_document_refuses_without_a_provider() { + let mut service = service(); + let (code, message) = refused(service.rename( + "file:///project/main.opy", + Position { + line: 0, + character: 0, + }, + "renamed", + )); + assert_eq!(code, "rename-unknown-document"); + assert!(message.contains("not open"), "{message}"); + } + + #[test] + fn rename_on_a_non_source_document_refuses_without_a_provider() { + let mut service = service(); + let uri = open(&mut service, "notes.txt", "hello\n"); + let (code, _) = refused(service.rename( + &uri, + Position { + line: 0, + character: 0, + }, + "renamed", + )); + assert_eq!(code, "rename-unsupported-document"); + } + + #[test] + fn rename_on_a_source_document_without_a_provider_refuses() { + // No DEL provider exists, so `.del` deterministically reaches the + // unconfigured-provider refusal — the explicit contract, not a + // textual fallback. + let mut service = service(); + let uri = open(&mut service, "main.del", "x\n"); + let (code, message) = refused(service.rename( + &uri, + Position { + line: 0, + character: 0, + }, + "renamed", + )); + assert_eq!(code, "provider-not-configured"); + assert!(message.contains("del"), "{message}"); + } + + #[test] + fn rename_on_opy_with_an_unresolvable_provider_refuses() { + let config = SessionConfig { + opy_provider: wright_driver::OpyProviderConfig::with_executable( + "/nonexistent/opy-provider", + ), + ..SessionConfig::default() + }; + let mut service = LanguageService::with_config(PathBuf::from("/project"), config); + let uri = open(&mut service, "main.opy", "x\n"); + let (code, _) = refused(service.rename( + &uri, + Position { + line: 0, + character: 0, + }, + "renamed", + )); + // The explicit executable fails validation before any spawn. + assert_ne!(code, "rename-refused", "a structured code must surface"); + } + + #[test] + fn utf16_edit_ranges_convert_from_driver_columns() { + // EditRange cols are 1-based character columns over a line whose + // first char is 🎯 (two UTF-16 units). + let text = "🎯 = 1\ny = 🎯 + 2\n"; + let range = wright_driver::edit::EditRange { + start_line: 2, + start_col: 5, + end_line: 2, + end_col: 6, + }; + let converted = edit_range(text, &range); + assert_eq!(converted.start.line, 1); + // Char col 5 is 🎯 itself, starting at utf16 offset 4 (y, space, =, + // space before it); the end col 6 lands after it at utf16 offset 6. + assert_eq!(converted.start.character, 4); + assert_eq!(converted.end.character, 6); + } +} diff --git a/crates/wright-lsp/src/main.rs b/crates/wright-lsp/src/main.rs index a0d863f7..037d6143 100644 --- a/crates/wright-lsp/src/main.rs +++ b/crates/wright-lsp/src/main.rs @@ -9,34 +9,56 @@ use lsp_types::notification::{Notification, PublishDiagnostics}; use lsp_types::{ Diagnostic as LspDiagnostic, DiagnosticSeverity, DidChangeTextDocumentParams, DidCloseTextDocumentParams, DidOpenTextDocumentParams, InitializeParams, InitializeResult, - Position as LspPosition, PositionEncodingKind, PublishDiagnosticsParams, Range as LspRange, - ServerCapabilities, ServerInfo, TextDocumentSyncCapability, TextDocumentSyncKind, - TextDocumentSyncOptions, Uri, + OneOf, Position as LspPosition, PositionEncodingKind, PublishDiagnosticsParams, + Range as LspRange, RenameParams, ServerCapabilities, ServerInfo, TextDocumentSyncCapability, + TextDocumentSyncKind, TextDocumentSyncOptions, TextEdit, Uri, WorkspaceEdit, }; use serde_json::Value; -use wright_language::LanguageService; -use wright_language::document::{Document, Range}; +use wright_language::document::{Document, Position, Range}; +use wright_language::service::RenameOutcome; +use wright_language::{LanguageService, OpyProviderConfig, SessionConfig}; type PublicationOwnership = BTreeMap>; fn main() { - if std::env::args().any(|arg| arg == "--version" || arg == "-V") { - println!("wright-lsp {}", env!("CARGO_PKG_VERSION")); - return; + let mut args = std::env::args().skip(1); + let mut opy_provider: Option = None; + while let Some(arg) = args.next() { + match arg.as_str() { + "--version" | "-V" => { + println!("wright-lsp {}", env!("CARGO_PKG_VERSION")); + return; + } + "--opy-provider" => { + let Some(path) = args.next() else { + eprintln!("wright-lsp: --opy-provider requires a PATH value"); + std::process::exit(2); + }; + opy_provider = Some(PathBuf::from(path)); + } + _ => {} + } } - if let Err(message) = run() { + if let Err(message) = run(opy_provider) { eprintln!("wright-lsp: {message}"); std::process::exit(1); } } -fn run() -> Result<(), String> { +fn run(opy_provider: Option) -> Result<(), String> { let mut reader = std::io::stdin().lock(); let mut writer = std::io::stdout().lock(); + let config = SessionConfig { + opy_provider: OpyProviderConfig { + executable: opy_provider, + ..Default::default() + }, + ..Default::default() + }; let mut root = std::env::current_dir().map_err(|e| e.to_string())?; - let mut service = LanguageService::new(root.clone()); + let mut service = LanguageService::with_config(root.clone(), config.clone()); let mut ownership: PublicationOwnership = BTreeMap::new(); loop { @@ -56,7 +78,7 @@ fn run() -> Result<(), String> { if let Ok(init) = serde_json::from_value::(params) { if let Some(resolved) = initialize_root(&init) { root = resolved; - service = LanguageService::new(root.clone()); + service = LanguageService::with_config(root.clone(), config.clone()); ownership.clear(); } } @@ -110,6 +132,64 @@ fn run() -> Result<(), String> { publish_affected_diagnostics(&mut writer, &service, &mut ownership, &uri)?; } "textDocument/didSave" => {} + "textDocument/rename" => { + let params: RenameParams = parse_params(params)?; + let outcome = service.rename( + params.text_document_position.text_document.uri.as_str(), + Position { + line: params.text_document_position.position.line, + character: params.text_document_position.position.character, + }, + ¶ms.new_name, + ); + match outcome { + RenameOutcome::Applied(edits) => { + // `WorkspaceEdit.changes` keys on `Uri`, whose cached + // parse trips mutable-key-type; the key is hashed by + // its string form only. A dropped edit would apply a + // partial rename, so an unparseable URI is an error. + #[allow(clippy::mutable_key_type)] + let mut changes: std::collections::HashMap< + Uri, + Vec, + > = std::collections::HashMap::new(); + let mut malformed = None; + for edit in edits { + match Uri::from_str(&edit.uri) { + Ok(uri) => changes.entry(uri).or_default().push(TextEdit { + range: convert_range(edit.range), + new_text: edit.new_text, + }), + Err(_) => malformed = Some(edit.uri), + } + } + if let Some(uri) = malformed { + write_error( + &mut writer, + id, + -32803, + &format!( + "the provider produced an edit against an unparseable URI {uri}" + ), + "rename-edit-malformed-uri", + )?; + } else { + write_response( + &mut writer, + id, + serde_json::to_value(WorkspaceEdit { + changes: Some(changes), + ..Default::default() + }) + .unwrap(), + )?; + } + } + RenameOutcome::Refused { code, message } => { + write_error(&mut writer, id, -32803, &message, &code)?; + } + } + } _ => { if id.is_some() { write_response(&mut writer, id, Value::Null)?; @@ -131,6 +211,7 @@ fn initialize_result() -> InitializeResult { ..Default::default() }, )), + rename_provider: Some(OneOf::Left(true)), ..Default::default() }, server_info: Some(ServerInfo { @@ -175,6 +256,26 @@ fn write_response(writer: &mut impl Write, id: Option, result: Value) -> ) } +/// A JSON-RPC error response that preserves the refusal's structured code in +/// `error.data.code` so clients can distinguish refusals without parsing the +/// message. +fn write_error( + writer: &mut impl Write, + id: Option, + code: i64, + message: &str, + refusal_code: &str, +) -> Result<(), String> { + write_msg( + writer, + serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "error": { "code": code, "message": message, "data": { "code": refusal_code } }, + }), + ) +} + fn parse_params(params: Option) -> Result { serde_json::from_value(params.unwrap()).map_err(|error| error.to_string()) } diff --git a/crates/wright-lsp/tests/lsp.rs b/crates/wright-lsp/tests/lsp.rs index 960fcdd5..6bd1f45b 100644 --- a/crates/wright-lsp/tests/lsp.rs +++ b/crates/wright-lsp/tests/lsp.rs @@ -1,9 +1,13 @@ //! LSP contract tests for the currently backed capability set. //! -//! `wright-lsp` advertises document synchronization only; unbacked editor -//! capabilities are neither advertised nor answered. OPY/DEL/OSTW documents -//! report an explicit `source-provider-unavailable` diagnostic, while raw -//! Workshop documents receive no diagnostic at all. +//! `wright-lsp` advertises document synchronization and rename; unbacked +//! editor capabilities are neither advertised nor answered. OPY/DEL/OSTW +//! documents report an explicit `source-provider-unavailable` diagnostic, +//! while raw Workshop documents receive no diagnostic at all. Rename routes +//! through the provider-owned mutation path: applied renames return a +//! `WorkspaceEdit`, and unsupported documents, unconfigured providers, and +//! provider refusals answer with a `RequestFailed` error carrying the +//! structured refusal code in `error.data.code`. use std::io::{BufRead, BufReader, Read, Write}; use std::path::{Path, PathBuf}; @@ -132,12 +136,15 @@ fn initialize_advertises_only_backed_capabilities() { assert_eq!(init["result"]["serverInfo"]["name"], "wright-lsp"); let capabilities = init["result"]["capabilities"].as_object().unwrap(); assert!(capabilities.contains_key("textDocumentSync")); + assert_eq!( + capabilities["renameProvider"], true, + "rename is provider-backed and advertised" + ); for capability in [ "hoverProvider", "definitionProvider", "referencesProvider", "completionProvider", - "renameProvider", "semanticTokensProvider", ] { assert!( @@ -236,3 +243,91 @@ fn opy_lsp_workflow_reports_provider_boundary_without_static_fallback() { assert!(shutdown["result"].is_null()); client.notify("exit", serde_json::json!(null)); } + +fn rename_request(client: &mut LspClient, id: u64, uri: &str) -> serde_json::Value { + client.request( + id, + "textDocument/rename", + serde_json::json!({ + "textDocument": { "uri": uri }, + "position": { "line": 0, "character": 1 }, + "newName": "renamed", + }), + ) +} + +fn open_document(client: &mut LspClient, uri: &str, language_id: &str, text: &str) { + client.notify( + "textDocument/didOpen", + serde_json::json!({ + "textDocument": { + "uri": uri, + "languageId": language_id, + "version": 1, + "text": text, + } + }), + ); +} + +#[test] +fn rename_on_an_unopen_document_is_a_structured_request_error() { + let root = workspace_root(); + let mut client = LspClient::spawn(&root); + initialize(&mut client); + client.notify("initialized", serde_json::json!({})); + + let response = rename_request(&mut client, 2, "file:///workspace/main.opy"); + assert_eq!(response["error"]["code"], -32803); + assert_eq!( + response["error"]["data"]["code"], "rename-unknown-document", + "the structured refusal code rides in error.data.code: {response}" + ); + + client.notify("exit", serde_json::json!(null)); +} + +#[test] +fn rename_on_a_non_source_document_is_a_structured_request_error() { + let root = workspace_root(); + let mut client = LspClient::spawn(&root); + initialize(&mut client); + client.notify("initialized", serde_json::json!({})); + open_document( + &mut client, + "file:///workspace/notes.txt", + "plaintext", + "hello\n", + ); + client.read_notification("textDocument/publishDiagnostics"); + + let response = rename_request(&mut client, 2, "file:///workspace/notes.txt"); + assert_eq!(response["error"]["code"], -32803); + assert_eq!( + response["error"]["data"]["code"], + "rename-unsupported-document" + ); + + client.notify("exit", serde_json::json!(null)); +} + +#[test] +fn rename_without_a_configured_provider_is_a_structured_request_error() { + // No DEL provider exists, so `.del` deterministically reaches the + // unconfigured-provider refusal rather than any textual fallback. + let root = workspace_root(); + let mut client = LspClient::spawn(&root); + initialize(&mut client); + client.notify("initialized", serde_json::json!({})); + open_document(&mut client, "file:///workspace/main.del", "del", "x\n"); + client.read_notification("textDocument/publishDiagnostics"); + + let response = rename_request(&mut client, 2, "file:///workspace/main.del"); + assert_eq!(response["error"]["code"], -32803); + assert_eq!( + response["error"]["data"]["code"], "provider-not-configured", + "an unconfigured provider refuses explicitly: {response}" + ); + + client.notify("exit", serde_json::json!(null)); +} diff --git a/docs/agent-contract.md b/docs/agent-contract.md index 229c0ccd..959eb909 100644 --- a/docs/agent-contract.md +++ b/docs/agent-contract.md @@ -78,12 +78,16 @@ ADR-0020). The adapter speaks newline-delimited JSON-RPC 2.0 and implements | `costEstimate` | `wright_cost_estimate` | | `semanticRename` | `wright_semantic_rename` | | `validateEditTransaction` | `wright_validate_edit_transaction` | +| `providerSemanticRename` | `wright_provider_semantic_rename` | +| `providerValidateEdit` | `wright_provider_validate_edit` | `tools/list` contains a tool only when its operation is in this set and advertised by `capabilities.operations`. Each tool's `inputSchema` is derived from the operation's request schema with `op` removed (the tool name carries -it); the edit tools also omit `sources`, which then defaults to the on-disk -text (#472). `tools/call` arguments are the request fields. +it); the Workshop edit tools also omit `sources`, which then defaults to the +on-disk text (#472). The provider tools keep `documents` and `sources` +required — the caller owns the document set. `tools/call` arguments are the +request fields. A successful service `result` is returned unchanged as the tool result's JSON text content. A service refusal is a tool result with `isError: true` whose diff --git a/docs/language-services.md b/docs/language-services.md index 35a09f41..2d1510e7 100644 --- a/docs/language-services.md +++ b/docs/language-services.md @@ -1,6 +1,7 @@ # Wright Language Services and LSP -Status: current scope — document synchronization and diagnostics only +Status: current scope — document synchronization, diagnostics, and +provider-backed rename Scope: editor-neutral language services (`wright-language`) and the thin LSP adapter (`wright-lsp`) @@ -29,10 +30,10 @@ analyzer contracts. - `textDocument/publishDiagnostics`: versioned, grouped by source identity, with didClose cleanup. -The `initialize` result advertises `textDocumentSync` and the UTF-16 position -encoding only. It contains no provider entry for hover, definition, -references, completion, rename, or semantic tokens: those capabilities have -no backing implementation and are never advertised. A request for an +The `initialize` result advertises `textDocumentSync`, the UTF-16 position +encoding, and `renameProvider`. It contains no provider entry for hover, +definition, references, completion, or semantic tokens: those capabilities +have no backing implementation and are never advertised. A request for an unadvertised or unknown method receives a `result: null` response and the server keeps running. @@ -41,9 +42,22 @@ publishes an explicit `source-provider-unavailable` error while no provider language-service capability is negotiated. A raw Workshop document (or any document without a source language) publishes no diagnostics. -Provider-backed editor capabilities — hover, definition, references, -completion, rename, and semantic tokens — are future work tracked under #156 -and the owning implementations (for example `opy-rs` language-service +`textDocument/rename` on a source-language document routes through the same +provider-owned mutation path as the CLI and agent surfaces (#156): the +provider computes edits over the open document set, Wright verifies document +versions and source preconditions, asks the provider to validate the +transaction, and rechecks the edited project before returning a +`WorkspaceEdit`. Unsupported documents (`rename-unsupported-document`), +unopen documents (`rename-unknown-document`), unconfigured providers, and +provider or validation refusals answer with a `RequestFailed` error whose +`error.data.code` carries the structured refusal code — there is no textual +search/replace fallback. `--opy-provider ` points the session at an +explicit OPY provider executable; by default the resolver locates or +downloads the released provider like the CLI does. + +Provider-backed editor capabilities beyond rename — hover, definition, +references, completion, and semantic tokens — are future work tracked under +#156 and the owning implementations (for example `opy-rs` language-service capabilities). They arrive through provider capability negotiation, not through Wright-side reimplementation or static fallbacks. From b946fda0c1f7f2cc4dcf32039a39e0f027dc8a2c Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Sun, 4 Oct 2026 12:39:29 +0800 Subject: [PATCH 2/9] fix(language,lsp): share position math and stop panicking on missing 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. --- crates/wright-language/src/document.rs | 31 ++++++++++------------ crates/wright-language/src/service.rs | 36 ++++++++++++++++---------- crates/wright-lsp/src/main.rs | 3 ++- 3 files changed, 38 insertions(+), 32 deletions(-) diff --git a/crates/wright-language/src/document.rs b/crates/wright-language/src/document.rs index 63a5542a..29b6df28 100644 --- a/crates/wright-language/src/document.rs +++ b/crates/wright-language/src/document.rs @@ -156,25 +156,22 @@ pub fn utf16_len(s: &str) -> usize { s.chars().map(|c| c.len_utf16()).sum() } -pub fn span_to_range(span: &workshop_rs::source::Span, source: &str) -> Range { - let sl = span.start.line.saturating_sub(1) as usize; - let el = span.end.line.saturating_sub(1) as usize; - let lines: Vec<&str> = source.lines().collect(); - let sc = lines.get(sl).map_or(0, |line| { - char_offset_to_utf16(line, span.start.col.saturating_sub(1) as usize) - }); - let ec = lines.get(el).map_or(0, |line| { - char_offset_to_utf16(line, span.end.col.saturating_sub(1) as usize) +/// A 1-based line and 1-based character column as a UTF-16 `Position`; a line or column past the end clamps. +pub fn line_col_position(source: &str, line: u32, col: u32) -> Position { + let index = line.saturating_sub(1) as usize; + let character = source.lines().nth(index).map_or(0, |line| { + char_offset_to_utf16(line, col.saturating_sub(1) as usize) }); + Position { + line: index as u32, + character: character as u32, + } +} + +pub fn span_to_range(span: &workshop_rs::source::Span, source: &str) -> Range { Range { - start: Position { - line: sl as u32, - character: sc as u32, - }, - end: Position { - line: el as u32, - character: ec as u32, - }, + start: line_col_position(source, span.start.line, span.start.col), + end: line_col_position(source, span.end.line, span.end.col), } } diff --git a/crates/wright-language/src/service.rs b/crates/wright-language/src/service.rs index a8e80ab6..a5d87176 100644 --- a/crates/wright-language/src/service.rs +++ b/crates/wright-language/src/service.rs @@ -6,7 +6,7 @@ use std::path::PathBuf; use serde::Serialize; use wright_driver::{CompilerSession, SessionConfig}; -use crate::document::{DocumentStore, Position, Range, char_offset_to_utf16}; +use crate::document::{DocumentStore, Position, Range, line_col_position}; #[derive(Debug, Clone, Serialize)] pub struct SourceDiagnostic { @@ -245,20 +245,9 @@ fn is_source_document(uri: &str) -> bool { /// Convert a driver [`wright_driver::edit::EditRange`] (1-based line and /// character column, half-open) back into UTF-16 document coordinates. fn edit_range(text: &str, range: &wright_driver::edit::EditRange) -> Range { - let lines: Vec<&str> = text.split('\n').collect(); - let position = |line: u32, col: u32| { - let line_index = line.saturating_sub(1) as usize; - let character = lines.get(line_index).map_or(0, |line| { - char_offset_to_utf16(line, col.saturating_sub(1) as usize) - }); - Position { - line: line_index as u32, - character: character as u32, - } - }; Range { - start: position(range.start_line, range.start_col), - end: position(range.end_line, range.end_col), + start: line_col_position(text, range.start_line, range.start_col), + end: line_col_position(text, range.end_line, range.end_col), } } @@ -391,4 +380,23 @@ mod tests { assert_eq!(converted.start.character, 4); assert_eq!(converted.end.character, 6); } + + #[test] + fn edit_range_on_crlf_does_not_count_the_carriage_return() { + // str::lines drops the \r: a column past it must land on the last + // real char, matching span_to_range's line split. + let text = "a = 1\r\nb = 2\r\n"; + let range = wright_driver::edit::EditRange { + start_line: 2, + start_col: 1, + end_line: 2, + end_col: 7, + }; + let converted = edit_range(text, &range); + assert_eq!(converted.start.character, 0); + assert_eq!( + converted.end.character, 5, + "'b = 2' is 5 chars; the \\r is not a column" + ); + } } diff --git a/crates/wright-lsp/src/main.rs b/crates/wright-lsp/src/main.rs index 037d6143..55496c07 100644 --- a/crates/wright-lsp/src/main.rs +++ b/crates/wright-lsp/src/main.rs @@ -277,7 +277,8 @@ fn write_error( } fn parse_params(params: Option) -> Result { - serde_json::from_value(params.unwrap()).map_err(|error| error.to_string()) + serde_json::from_value(params.ok_or_else(|| "missing params".to_string())?) + .map_err(|error| error.to_string()) } fn publish_affected_diagnostics( From a95eba42791dcc62a535d52d78d76d42e1dfd539 Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Sun, 4 Oct 2026 13:05:53 +0800 Subject: [PATCH 3/9] fix(lsp): answer invalid params instead of exiting the server 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. --- crates/wright-lsp/src/main.rs | 42 ++++++++++++++++++++++++++++++---- crates/wright-lsp/tests/lsp.rs | 33 ++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/crates/wright-lsp/src/main.rs b/crates/wright-lsp/src/main.rs index 55496c07..75aeed3d 100644 --- a/crates/wright-lsp/src/main.rs +++ b/crates/wright-lsp/src/main.rs @@ -93,7 +93,11 @@ fn run(opy_provider: Option) -> Result<(), String> { "shutdown" => write_response(&mut writer, id, Value::Null)?, "exit" => break, "textDocument/didOpen" => { - let params: DidOpenTextDocumentParams = parse_params(params)?; + let Some(params) = + read_params::(&mut writer, &id, params)? + else { + continue; + }; let uri = params.text_document.uri.to_string(); let document = Document::with_version( uri.clone(), @@ -105,7 +109,11 @@ fn run(opy_provider: Option) -> Result<(), String> { publish_affected_diagnostics(&mut writer, &service, &mut ownership, &uri)?; } "textDocument/didChange" => { - let params: DidChangeTextDocumentParams = parse_params(params)?; + let Some(params) = + read_params::(&mut writer, &id, params)? + else { + continue; + }; let uri = params.text_document.uri.to_string(); if let Some(change) = params.content_changes.last() { service @@ -115,7 +123,11 @@ fn run(opy_provider: Option) -> Result<(), String> { publish_affected_diagnostics(&mut writer, &service, &mut ownership, &uri)?; } "textDocument/didClose" => { - let params: DidCloseTextDocumentParams = parse_params(params)?; + let Some(params) = + read_params::(&mut writer, &id, params)? + else { + continue; + }; let uri = params.text_document.uri.to_string(); let owned = ownership.remove(&uri).unwrap_or_default(); service.store.close(&uri); @@ -133,7 +145,9 @@ fn run(opy_provider: Option) -> Result<(), String> { } "textDocument/didSave" => {} "textDocument/rename" => { - let params: RenameParams = parse_params(params)?; + let Some(params) = read_params::(&mut writer, &id, params)? else { + continue; + }; let outcome = service.rename( params.text_document_position.text_document.uri.as_str(), Position { @@ -281,6 +295,26 @@ fn parse_params(params: Option) -> Result .map_err(|error| error.to_string()) } +/// Params for a message, answering `-32602` when a request carries none or +/// malformed ones. `None` means the message was malformed and already handled — +/// or was a notification, which gets no response — so the caller skips it and +/// the server keeps serving. +fn read_params( + writer: &mut impl Write, + id: &Option, + params: Option, +) -> Result, String> { + match parse_params(params) { + Ok(parsed) => Ok(Some(parsed)), + Err(message) => { + if id.is_some() { + write_error(writer, id.clone(), -32602, &message, "invalid-params")?; + } + Ok(None) + } + } +} + fn publish_affected_diagnostics( writer: &mut impl Write, service: &LanguageService, diff --git a/crates/wright-lsp/tests/lsp.rs b/crates/wright-lsp/tests/lsp.rs index 6bd1f45b..9ac3dd8b 100644 --- a/crates/wright-lsp/tests/lsp.rs +++ b/crates/wright-lsp/tests/lsp.rs @@ -158,6 +158,39 @@ fn initialize_advertises_only_backed_capabilities() { client.notify("exit", serde_json::json!(null)); } +#[test] +fn malformed_params_answer_invalid_params_and_the_server_keeps_serving() { + let root = workspace_root(); + let mut client = LspClient::spawn(&root); + initialize(&mut client); + client.notify("initialized", serde_json::json!({})); + + for (id, message) in [ + ( + 7, + serde_json::json!({"jsonrpc": "2.0", "id": 7, "method": "textDocument/rename"}), + ), + ( + 8, + serde_json::json!({"jsonrpc": "2.0", "id": 8, "method": "textDocument/rename", "params": "bogus"}), + ), + ] { + client.send(message); + let error = client.read_message(); + assert_eq!(error["id"], id); + assert_eq!( + error["error"]["code"], -32602, + "missing or malformed params are Invalid params, not a dead server" + ); + } + + // a malformed notification gets no response but costs no session either + client.send(serde_json::json!({"jsonrpc": "2.0", "method": "textDocument/didOpen"})); + let shutdown = client.request(9, "shutdown", serde_json::json!(null)); + assert!(shutdown["result"].is_null()); + client.notify("exit", serde_json::json!(null)); +} + #[test] fn workshop_lsp_workflow_keeps_protocol_and_lifecycle_contracts() { let root = workspace_root(); From a3f3650b8e4d79160e4859a0073b3f830b6d476a Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Sun, 4 Oct 2026 14:00:59 +0800 Subject: [PATCH 4/9] fix(lsp): answer no rename notification A textDocument/rename sent without an id is a notification: it gets no response and no provider call. --- crates/wright-lsp/src/main.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/wright-lsp/src/main.rs b/crates/wright-lsp/src/main.rs index 75aeed3d..53f6dd58 100644 --- a/crates/wright-lsp/src/main.rs +++ b/crates/wright-lsp/src/main.rs @@ -148,6 +148,9 @@ fn run(opy_provider: Option) -> Result<(), String> { let Some(params) = read_params::(&mut writer, &id, params)? else { continue; }; + if id.is_none() { + continue; // a notification gets no response and no provider call + } let outcome = service.rename( params.text_document_position.text_document.uri.as_str(), Position { From c1628763b34fe0bce3fd2184360d14c3a503e4ca Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Sun, 4 Oct 2026 16:13:42 +0800 Subject: [PATCH 5/9] fix(lsp,language,driver): version rename edits and scope the post-edit 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. --- crates/wright-driver/src/provider_edit.rs | 177 +++++++++++++++++++++- crates/wright-language/src/service.rs | 71 ++++++++- crates/wright-lsp/src/main.rs | 109 +++++++++---- crates/wright-lsp/tests/lsp.rs | 135 ++++++++++++++++- docs/language-services.md | 43 ++++-- 5 files changed, 477 insertions(+), 58 deletions(-) diff --git a/crates/wright-driver/src/provider_edit.rs b/crates/wright-driver/src/provider_edit.rs index ca59a1ac..a2445023 100644 --- a/crates/wright-driver/src/provider_edit.rs +++ b/crates/wright-driver/src/provider_edit.rs @@ -1,4 +1,4 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use serde::{Deserialize, Serialize}; @@ -112,12 +112,29 @@ pub fn semantic_rename( Err(diag) => return refusal(vec![diag], None), }; + // The supplied document set is deliberately wider than the target + // project: only the provider can compute project membership (include + // resolution, entry selection), and `lpp/rename` needs every open + // document to spot references the project must account for. The + // post-edit check verdict is therefore scoped to the mutation the + // provider actually produced — the edit sites plus the position + // document — so an unrelated open document cannot block a valid + // rename. Diagnostics in member files that were never supplied still + // attribute to the entry document's check view and remain blocking. + let mut edit_scope: BTreeSet = transaction + .edits + .iter() + .map(|edit| edit.source.clone()) + .collect(); + edit_scope.insert(request.position_document_uri.clone()); + finish_transaction( provider, &request.documents, transaction, &request.sources, request.project_root.as_deref(), + &edit_scope, ) } @@ -129,12 +146,17 @@ pub fn validate_transaction( Ok(transaction) => transaction, Err(diagnostic) => return refusal(vec![diagnostic], None), }; + // A caller-supplied document set is the declared project: every + // supplied document's errors stay blocking, including unedited + // members — the post-edit check is the only project-level gate here. + let edit_scope: BTreeSet = request.documents.keys().cloned().collect(); finish_transaction( provider, &request.documents, transaction, &request.sources, request.project_root.as_deref(), + &edit_scope, ) } @@ -144,6 +166,7 @@ fn finish_transaction( transaction: EditTransaction, sources: &BTreeMap, project_root: Option<&str>, + edit_scope: &BTreeSet, ) -> ProviderMutation { for edit in &transaction.edits { if let Some(diagnostic) = crate::edit::source_precondition(edit, sources) { @@ -154,9 +177,14 @@ fn finish_transaction( Ok(previews) => previews, Err(diag) => return refusal(vec![diag], None), }; - if let Err((diagnostics, provider)) = - validate_pipeline(provider, documents, &transaction, &previews, project_root) - { + if let Err((diagnostics, provider)) = validate_pipeline( + provider, + documents, + &transaction, + &previews, + project_root, + edit_scope, + ) { return refusal(diagnostics, provider); } ProviderMutation { @@ -175,6 +203,7 @@ fn validate_pipeline( transaction: &EditTransaction, previews: &[SourcePreview], project_root: Option<&str>, + edit_scope: &BTreeSet, ) -> Result<(), (Vec, Option)> { let mut by_source: BTreeMap<&str, Vec<&SourceEdit>> = BTreeMap::new(); for edit in &transaction.edits { @@ -261,6 +290,13 @@ fn validate_pipeline( .check(&edited, project_root) .map_err(provider_failure_tuple)?; for doc in &checked.documents { + // Only documents the mutation is allowed to affect can block it: + // the request set may intentionally carry unrelated open documents + // (see `semantic_rename`), whose diagnostics are not this project's + // problem. + if !edit_scope.contains(&doc.uri) { + continue; + } for diag in &doc.diagnostics { if diag.severity == wright_lpp::DiagnosticSeverity::Error { return Err(( @@ -445,6 +481,7 @@ mod tests { }; const URI: &str = "file:///project/puzzle.xdl"; + const UNRELATED_URI: &str = "file:///project/unrelated.xdl"; const CLEAN: &str = "puzzle clean {\n target = 40\n start = 10\n ops {\n double: x => x * 2\n plus1: x => x + 1\n }\n solution = [ double, double ]\n}"; fn document(text: &str) -> Document { @@ -938,6 +975,138 @@ mod tests { assert!(mutation.preview.is_none()); } + #[test] + fn rename_post_edit_check_drops_unrelated_open_document_errors() { + // The rename document set is language-wide because only the + // provider can compute project membership; a supplied document the + // mutation never touched must not block it. + let mut request = rename_request(); + request.documents.insert( + UNRELATED_URI.to_string(), + wright_lpp::Document { + uri: UNRELATED_URI.to_string(), + language_id: "x-demo-lang".to_string(), + version: 1, + text: "puzzle broken {\n".to_string(), + }, + ); + request + .sources + .insert(UNRELATED_URI.to_string(), "puzzle broken {\n".to_string()); + let mut provider = ScriptedProvider { + rename: Ok(clean_rename_result()), + validate_edits: Ok(ValidateEditsResult { + valid: true, + version: 3, + reason: None, + failing_edit_index: None, + }), + check: Ok(CheckResult { + documents: vec![wright_lpp::DocumentDiagnostics { + uri: UNRELATED_URI.to_string(), + version: 1, + diagnostics: vec![wright_lpp::Diagnostic { + range: wright_lpp::Range { + start: wright_lpp::Position { + line: 0, + character: 0, + }, + end: wright_lpp::Position { + line: 0, + character: 15, + }, + }, + severity: wright_lpp::DiagnosticSeverity::Error, + code: Some("x-demo/unterminated".to_string()), + message: "unterminated puzzle".to_string(), + source: Some("x-demo-lang".to_string()), + }], + }], + }), + }; + let mutation = semantic_rename(&mut provider, &request); + assert!( + mutation.ok, + "an unrelated document's error cannot block the rename: {:?}", + mutation.diagnostics + ); + assert!(mutation.transaction.is_some()); + } + + #[test] + fn validate_transaction_check_scope_covers_the_whole_caller_set() { + // A caller-supplied document set is the declared project — unlike + // the rename-collected set above, an error in an unedited document + // still blocks because the post-edit check is the only project gate. + let transaction = EditTransaction::new(vec![SourceEdit { + edit_kind: "rename".to_string(), + source: URI.to_string(), + source_identity: crate::input_identity(CLEAN), + range: EditRange { + start_line: 5, + start_col: 5, + end_line: 5, + end_col: 11, + }, + new_text: "twice".to_string(), + }]) + .expect("transaction"); + let mut request = ProviderValidateRequest { + documents: document_set(), + transaction, + sources: sources(CLEAN), + project_root: None, + }; + request.documents.insert( + UNRELATED_URI.to_string(), + wright_lpp::Document { + uri: UNRELATED_URI.to_string(), + language_id: "x-demo-lang".to_string(), + version: 1, + text: "puzzle broken {\n".to_string(), + }, + ); + request + .sources + .insert(UNRELATED_URI.to_string(), "puzzle broken {\n".to_string()); + let mut provider = ScriptedProvider { + rename: Ok(RenameResult { edits: vec![] }), + validate_edits: Ok(ValidateEditsResult { + valid: true, + version: 3, + reason: None, + failing_edit_index: None, + }), + check: Ok(CheckResult { + documents: vec![wright_lpp::DocumentDiagnostics { + uri: UNRELATED_URI.to_string(), + version: 1, + diagnostics: vec![wright_lpp::Diagnostic { + range: wright_lpp::Range { + start: wright_lpp::Position { + line: 0, + character: 0, + }, + end: wright_lpp::Position { + line: 0, + character: 15, + }, + }, + severity: wright_lpp::DiagnosticSeverity::Error, + code: Some("x-demo/unterminated".to_string()), + message: "unterminated puzzle".to_string(), + source: Some("x-demo-lang".to_string()), + }], + }], + }), + }; + let mutation = validate_transaction(&mut provider, &request); + assert!(!mutation.ok); + assert_eq!(mutation.diagnostics[0].code, "provider-semantic-error"); + assert!(mutation.transaction.is_none()); + assert!(mutation.preview.is_none()); + } + #[test] fn validate_transaction_runs_the_provider_gates_on_a_caller_transaction() { // The caller proposes a Wright transaction; the flow re-asserts the diff --git a/crates/wright-language/src/service.rs b/crates/wright-language/src/service.rs index a5d87176..fc07a66f 100644 --- a/crates/wright-language/src/service.rs +++ b/crates/wright-language/src/service.rs @@ -20,12 +20,15 @@ pub struct SourceDiagnostic { } /// One validated text replacement produced by a provider-owned edit, in the -/// document store's UTF-16 coordinates. +/// document store's UTF-16 coordinates. `version` is the document version +/// the provider computed and Wright re-validated the edit against; the +/// range is only meaningful to a buffer at that version. #[derive(Debug, Clone, PartialEq, Eq)] pub struct SourceTextEdit { pub uri: String, pub range: Range, pub new_text: String, + pub version: i32, } /// The outcome of a provider-owned source mutation (#156): validated edits @@ -98,10 +101,11 @@ impl LanguageService { /// and agent surfaces: the provider computes the edits for the open /// document set, Wright verifies document versions, applies source /// preconditions, asks the provider to validate the transaction, and - /// checks the edited project before returning anything. A refusal — - /// unopen or unsupported document, unconfigured provider, provider - /// refusal, validation failure — is structured and final; no textual - /// fallback exists here. + /// checks the edited project before returning anything. Every applied + /// edit carries the validated document version so the caller can reject + /// it once its buffer has moved. A refusal — unopen or unsupported + /// document, unconfigured provider, provider refusal, validation + /// failure — is structured and final; no textual fallback exists here. pub fn rename(&mut self, uri: &str, position: Position, new_name: &str) -> RenameOutcome { if self.store.document(uri).is_none() { return RenameOutcome::Refused { @@ -118,9 +122,12 @@ impl LanguageService { }; }; - // The document set holds this language's open documents: foreign - // source documents cannot produce rename edits and an unrelated - // diagnostic on one must not block this rename. + // The document set stays language-wide: only the provider can + // compute project membership, and `lpp/rename` needs every open + // document as a potential include or rename site. `semantic_rename` + // scopes the post-edit check verdict to the documents the provider + // actually edited (plus the position document), so an unrelated + // broken open document cannot block a valid rename. let mut documents = wright_lpp::DocumentSet::new(); let mut sources = BTreeMap::new(); for doc_uri in self.store.uris() { @@ -215,6 +222,10 @@ impl LanguageService { uri: edit.source, range: edit_range(&doc.text, &edit.range), new_text: edit.new_text, + // `semantic_rename` verified the provider's edits against + // this exact version; the caller uses it to reject stale + // edits. + version: doc.version, }); } RenameOutcome::Applied(applied) @@ -362,6 +373,50 @@ mod tests { assert_ne!(code, "rename-refused", "a structured code must surface"); } + /// `WRIGHT_OPY_PROVIDER` points at an `opy-provider` executable; without + /// it the provider-backed rename cannot run and the test self-skips. + #[test] + fn an_unrelated_broken_open_document_does_not_block_a_project_rename() { + let Ok(provider) = std::env::var("WRIGHT_OPY_PROVIDER") else { + eprintln!("SKIPPED: WRIGHT_OPY_PROVIDER is not set"); + return; + }; + let config = SessionConfig { + opy_provider: wright_driver::OpyProviderConfig::with_executable(PathBuf::from( + provider, + )), + ..SessionConfig::default() + }; + let mut service = LanguageService::with_config(PathBuf::from("/project"), config); + let uri = open(&mut service, "main.opy", "globalvar score = 0\n"); + // A broken OPY document in the same language but outside the + // position document's project: its error must not block the rename. + open( + &mut service, + "unrelated/broken.opy", + "#!include \"missing.opy\"\n", + ); + match service.rename( + &uri, + Position { + line: 0, + character: 12, + }, + "vault", + ) { + RenameOutcome::Applied(edits) => { + assert!(!edits.is_empty()); + assert!( + edits.iter().all(|edit| edit.version == 1), + "every applied edit carries the validated document version" + ); + } + RenameOutcome::Refused { code, message } => { + panic!("expected applied edits; refused ({code}): {message}") + } + } + } + #[test] fn utf16_edit_ranges_convert_from_driver_columns() { // EditRange cols are 1-based character columns over a line whose diff --git a/crates/wright-lsp/src/main.rs b/crates/wright-lsp/src/main.rs index 53f6dd58..9fda0ac9 100644 --- a/crates/wright-lsp/src/main.rs +++ b/crates/wright-lsp/src/main.rs @@ -7,11 +7,13 @@ use std::str::FromStr; use lsp_types::notification::{Notification, PublishDiagnostics}; use lsp_types::{ - Diagnostic as LspDiagnostic, DiagnosticSeverity, DidChangeTextDocumentParams, - DidCloseTextDocumentParams, DidOpenTextDocumentParams, InitializeParams, InitializeResult, - OneOf, Position as LspPosition, PositionEncodingKind, PublishDiagnosticsParams, - Range as LspRange, RenameParams, ServerCapabilities, ServerInfo, TextDocumentSyncCapability, - TextDocumentSyncKind, TextDocumentSyncOptions, TextEdit, Uri, WorkspaceEdit, + AnnotatedTextEdit, Diagnostic as LspDiagnostic, DiagnosticSeverity, + DidChangeTextDocumentParams, DidCloseTextDocumentParams, DidOpenTextDocumentParams, + DocumentChanges, InitializeParams, InitializeResult, OneOf, + OptionalVersionedTextDocumentIdentifier, Position as LspPosition, PositionEncodingKind, + PublishDiagnosticsParams, Range as LspRange, RenameParams, ServerCapabilities, ServerInfo, + TextDocumentEdit, TextDocumentSyncCapability, TextDocumentSyncKind, TextDocumentSyncOptions, + TextEdit, Uri, WorkspaceEdit, }; use serde_json::Value; @@ -60,6 +62,10 @@ fn run(opy_provider: Option) -> Result<(), String> { let mut root = std::env::current_dir().map_err(|e| e.to_string())?; let mut service = LanguageService::with_config(root.clone(), config.clone()); let mut ownership: PublicationOwnership = BTreeMap::new(); + // Whether the client negotiated versioned workspace edits + // (`workspace.workspaceEdit.documentChanges`). Renames are only served + // when they can carry the validated document version. + let mut versioned_workspace_edits = false; loop { let message = read_message(&mut reader)?; @@ -74,19 +80,30 @@ fn run(opy_provider: Option) -> Result<(), String> { match method.as_str() { "initialize" => { - if let Some(params) = params { - if let Ok(init) = serde_json::from_value::(params) { - if let Some(resolved) = initialize_root(&init) { - root = resolved; - service = LanguageService::with_config(root.clone(), config.clone()); - ownership.clear(); - } + let init = params + .and_then(|params| serde_json::from_value::(params).ok()); + versioned_workspace_edits = init + .as_ref() + .and_then(|init| { + init.capabilities + .workspace + .as_ref()? + .workspace_edit + .as_ref()? + .document_changes + }) + .unwrap_or(false); + if let Some(init) = init { + if let Some(resolved) = initialize_root(&init) { + root = resolved; + service = LanguageService::with_config(root.clone(), config.clone()); + ownership.clear(); } } write_response( &mut writer, id, - serde_json::to_value(initialize_result()).unwrap(), + serde_json::to_value(initialize_result(versioned_workspace_edits)).unwrap(), )?; } "initialized" => {} @@ -151,6 +168,22 @@ fn run(opy_provider: Option) -> Result<(), String> { if id.is_none() { continue; // a notification gets no response and no provider call } + if !versioned_workspace_edits { + // `WorkspaceEdit.changes` cannot carry the document + // version the provider validated; serving it would let a + // client apply the edit to a buffer that moved while the + // rename was pending without ever detecting it. + write_error( + &mut writer, + id, + -32803, + "rename requires versioned workspace edits \ + (workspace.workspaceEdit.documentChanges is not negotiated); \ + the request is refused rather than risk a stale buffer", + "rename-unversioned-workspace-edit", + )?; + continue; + } let outcome = service.rename( params.text_document_position.text_document.uri.as_str(), Position { @@ -161,23 +194,38 @@ fn run(opy_provider: Option) -> Result<(), String> { ); match outcome { RenameOutcome::Applied(edits) => { - // `WorkspaceEdit.changes` keys on `Uri`, whose cached - // parse trips mutable-key-type; the key is hashed by - // its string form only. A dropped edit would apply a - // partial rename, so an unparseable URI is an error. - #[allow(clippy::mutable_key_type)] - let mut changes: std::collections::HashMap< - Uri, - Vec, - > = std::collections::HashMap::new(); - let mut malformed = None; + // `documentChanges` carries the validated document + // version per file so the client can reject the edit + // if its buffer moved while the rename was pending. + // One `TextDocumentEdit` per document; a dropped + // edit would apply a partial rename, so an + // unparseable URI is an error. + let mut grouped: BTreeMap< + String, + (i32, Vec>), + > = BTreeMap::new(); for edit in edits { - match Uri::from_str(&edit.uri) { - Ok(uri) => changes.entry(uri).or_default().push(TextEdit { + let version = edit.version; + grouped + .entry(edit.uri) + .or_insert_with(|| (version, Vec::new())) + .1 + .push(OneOf::Left(TextEdit { range: convert_range(edit.range), new_text: edit.new_text, + })); + } + let mut document_edits = Vec::with_capacity(grouped.len()); + let mut malformed = None; + for (uri, (version, text_edits)) in grouped { + match Uri::from_str(&uri) { + Ok(uri) => document_edits.push(TextDocumentEdit { + text_document: OptionalVersionedTextDocumentIdentifier::new( + uri, version, + ), + edits: text_edits, }), - Err(_) => malformed = Some(edit.uri), + Err(_) => malformed = Some(uri), } } if let Some(uri) = malformed { @@ -195,7 +243,7 @@ fn run(opy_provider: Option) -> Result<(), String> { &mut writer, id, serde_json::to_value(WorkspaceEdit { - changes: Some(changes), + document_changes: Some(DocumentChanges::Edits(document_edits)), ..Default::default() }) .unwrap(), @@ -217,7 +265,7 @@ fn run(opy_provider: Option) -> Result<(), String> { Ok(()) } -fn initialize_result() -> InitializeResult { +fn initialize_result(versioned_workspace_edits: bool) -> InitializeResult { InitializeResult { capabilities: ServerCapabilities { position_encoding: Some(PositionEncodingKind::UTF16), @@ -228,7 +276,10 @@ fn initialize_result() -> InitializeResult { ..Default::default() }, )), - rename_provider: Some(OneOf::Left(true)), + // Applied renames are returned as versioned `documentChanges`; + // a client that cannot receive the validated document version + // cannot be handed an edit safely, so rename stays unadvertised. + rename_provider: versioned_workspace_edits.then_some(OneOf::Left(true)), ..Default::default() }, server_info: Some(ServerInfo { diff --git a/crates/wright-lsp/tests/lsp.rs b/crates/wright-lsp/tests/lsp.rs index 9ac3dd8b..b235d365 100644 --- a/crates/wright-lsp/tests/lsp.rs +++ b/crates/wright-lsp/tests/lsp.rs @@ -21,8 +21,20 @@ struct LspClient { impl LspClient { fn spawn(cwd: &Path) -> Self { - let mut child = Command::new(env!("CARGO_BIN_EXE_wright-lsp")) - .current_dir(cwd) + Self::launch(Command::new(env!("CARGO_BIN_EXE_wright-lsp")).current_dir(cwd)) + } + + fn spawn_with_provider(cwd: &Path, provider: &Path) -> Self { + Self::launch( + Command::new(env!("CARGO_BIN_EXE_wright-lsp")) + .arg("--opy-provider") + .arg(provider) + .current_dir(cwd), + ) + } + + fn launch(command: &mut Command) -> Self { + let mut child = command .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) @@ -104,6 +116,8 @@ fn workspace_root() -> PathBuf { Path::new(env!("CARGO_MANIFEST_DIR")).join("..").join("..") } +/// A client that can receive versioned workspace edits — the realistic +/// rename-capable configuration. fn initialize(client: &mut LspClient) -> serde_json::Value { client.request( 1, @@ -111,7 +125,9 @@ fn initialize(client: &mut LspClient) -> serde_json::Value { serde_json::json!({ "processId": null, "rootUri": "file:///workspace", - "capabilities": {}, + "capabilities": { + "workspace": { "workspaceEdit": { "documentChanges": true } } + }, }), ) } @@ -364,3 +380,116 @@ fn rename_without_a_configured_provider_is_a_structured_request_error() { client.notify("exit", serde_json::json!(null)); } + +#[test] +fn rename_is_only_negotiated_for_versioned_workspace_edits() { + // `WorkspaceEdit.changes` cannot carry the validated document version, + // so a client without `documentChanges` gets neither the advertisement + // nor the edit. + let root = workspace_root(); + let mut client = LspClient::spawn(&root); + let init = client.request( + 1, + "initialize", + serde_json::json!({ + "processId": null, + "rootUri": "file:///workspace", + "capabilities": {}, + }), + ); + assert!( + init["result"]["capabilities"] + .get("renameProvider") + .is_none(), + "rename is not advertised without versioned workspace edits: {init}" + ); + client.notify("initialized", serde_json::json!({})); + open_document( + &mut client, + "file:///workspace/main.opy", + "opy", + "globalvar score = 0\n", + ); + client.read_notification("textDocument/publishDiagnostics"); + + let response = rename_request(&mut client, 2, "file:///workspace/main.opy"); + assert_eq!(response["error"]["code"], -32803); + assert_eq!( + response["error"]["data"]["code"], "rename-unversioned-workspace-edit", + "an unversioned workspace edit would violate the freshness contract: {response}" + ); + + client.notify("exit", serde_json::json!(null)); +} + +/// `WRIGHT_OPY_PROVIDER` points at an `opy-provider` executable; without it +/// the provider-backed rename cannot run and the test self-skips. +#[test] +fn rename_returns_versioned_document_changes_scoped_to_the_project() { + let Ok(provider) = std::env::var("WRIGHT_OPY_PROVIDER") else { + eprintln!("SKIPPED: WRIGHT_OPY_PROVIDER is not set"); + return; + }; + let root = workspace_root(); + let mut client = LspClient::spawn_with_provider(&root, Path::new(&provider)); + initialize(&mut client); + client.notify("initialized", serde_json::json!({})); + + let main = "file:///workspace/main.opy"; + open_document(&mut client, main, "opy", "globalvar score = 0\n"); + client.read_notification("textDocument/publishDiagnostics"); + // A broken OPY document in the same language but outside main.opy's + // project: its diagnostics must not block the target rename. + open_document( + &mut client, + "file:///workspace/unrelated/broken.opy", + "opy", + "#!include \"missing.opy\"\n", + ); + client.read_notification("textDocument/publishDiagnostics"); + + // Send the rename, then race it: the buffer changes before the client + // reads the response, so the version the provider validated no longer + // matches the client's buffer — the version tag is what lets the + // client detect that. + client.send(serde_json::json!({ + "jsonrpc": "2.0", + "id": 3, + "method": "textDocument/rename", + "params": { + "textDocument": { "uri": main }, + "position": { "line": 0, "character": 12 }, + "newName": "vault", + }, + })); + client.notify( + "textDocument/didChange", + serde_json::json!({ + "textDocument": { "uri": main, "version": 2 }, + "contentChanges": [{ "text": "# extra comment\nglobalvar score = 0\n" }], + }), + ); + + // Requests are served in order: the rename response precedes the + // diagnostics notification the didChange triggers. + let response = client.read_message(); + assert_eq!(response["id"], 3); + let document_changes = response["result"]["documentChanges"] + .as_array() + .unwrap_or_else(|| panic!("versioned documentChanges expected: {response}")); + assert_eq!(document_changes.len(), 1); + let edit = &document_changes[0]; + assert_eq!(edit["textDocument"]["uri"], main); + assert_eq!( + edit["textDocument"]["version"], 1, + "the edit carries the validated version so the client rejects it once its buffer moved: {edit}" + ); + let text_edits = edit["edits"].as_array().expect("text edits"); + assert!(!text_edits.is_empty()); + assert!( + text_edits.iter().all(|e| e["newText"] == "vault"), + "every edit applies the rename: {text_edits:?}" + ); + + client.notify("exit", serde_json::json!(null)); +} diff --git a/docs/language-services.md b/docs/language-services.md index 2d1510e7..7c4ae6d5 100644 --- a/docs/language-services.md +++ b/docs/language-services.md @@ -30,12 +30,15 @@ analyzer contracts. - `textDocument/publishDiagnostics`: versioned, grouped by source identity, with didClose cleanup. -The `initialize` result advertises `textDocumentSync`, the UTF-16 position -encoding, and `renameProvider`. It contains no provider entry for hover, -definition, references, completion, or semantic tokens: those capabilities -have no backing implementation and are never advertised. A request for an -unadvertised or unknown method receives a `result: null` response and the -server keeps running. +The `initialize` result advertises `textDocumentSync` and the UTF-16 +position encoding unconditionally, plus `renameProvider` when the client +negotiates `workspace.workspaceEdit.documentChanges` — applied renames are +returned as versioned `TextDocumentEdit`s, and a client that cannot receive +the validated document version cannot be handed an edit safely. The result +contains no provider entry for hover, definition, references, completion, +or semantic tokens: those capabilities have no backing implementation and +are never advertised. A request for an unadvertised or unknown method +receives a `result: null` response and the server keeps running. Diagnostics: opening a source-language document (`.opy`, `.del`, `.ostw`) publishes an explicit `source-provider-unavailable` error while no provider @@ -44,12 +47,23 @@ document without a source language) publishes no diagnostics. `textDocument/rename` on a source-language document routes through the same provider-owned mutation path as the CLI and agent surfaces (#156): the -provider computes edits over the open document set, Wright verifies document -versions and source preconditions, asks the provider to validate the -transaction, and rechecks the edited project before returning a -`WorkspaceEdit`. Unsupported documents (`rename-unsupported-document`), -unopen documents (`rename-unknown-document`), unconfigured providers, and -provider or validation refusals answer with a `RequestFailed` error whose +provider computes edits over the open document set (only the provider can +compute project membership, so the set stays language-wide), Wright verifies +document versions and source preconditions, asks the provider to validate +the transaction, and rechecks the edited project — with the check verdict +scoped to the documents the provider actually edited plus the position +document, so an unrelated open document cannot block a valid rename — before +returning anything. A successful rename answers with versioned +`WorkspaceEdit.documentChanges`: each `TextDocumentEdit` carries the +document version the provider computed and Wright re-validated, so the +client can reject the edit when its buffer has moved. Clients that did not +negotiate `workspace.workspaceEdit.documentChanges` get no `renameProvider` +advertisement, and their rename requests are refused +(`rename-unversioned-workspace-edit`) rather than served an unversioned +`changes` map a stale buffer could silently absorb. Unsupported documents +(`rename-unsupported-document`), unopen documents +(`rename-unknown-document`), unconfigured providers, and provider or +validation refusals answer with a `RequestFailed` error whose `error.data.code` carries the structured refusal code — there is no textual search/replace fallback. `--opy-provider ` points the session at an explicit OPY provider executable; by default the resolver locates or @@ -76,8 +90,9 @@ through Wright-side reimplementation or static fallbacks. `wright_language::document` (`uri_to_path`, `path_to_uri`) using the standard URL parser, covering percent-encoding, spaces, Unicode filenames, and platform drive paths. -* Every result carries `document_version`; stale results are detectable and - replaceable. +* Every result carries the document version it was validated against + (`document_version`, `SourceTextEdit.version`); stale results are + detectable and replaceable. ## Incremental behavior From 3fecd0e9e89ff6050996b476217ab2fa372883a3 Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Sun, 4 Oct 2026 16:37:37 +0800 Subject: [PATCH 6/9] fix(ci): run provider-backed language tests against the pinned provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/ci.yml | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c8b69ec3..c499b835 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -295,6 +295,12 @@ jobs: needs: [paths, rust-quality, rust-msrv, wright-cli-build] if: needs.paths.outputs.opy == 'true' runs-on: ubuntu-latest + env: + # The pinned provider version and the store it installs into, shared + # by the install step, the provider-backed test step, and the compile + # smoke check. + OPY_PROVIDER_VERSION: "0.1.38" + WRIGHT_PROVIDER_DATA_DIR: ${{ github.workspace }}/.wright-provider-data steps: - name: Checkout uses: actions/checkout@v7 @@ -334,7 +340,15 @@ jobs: && cargo test --locked -p wright-consumer --test consumer - name: Install pinned OPY provider - run: target/debug/wright update provider opy --version 0.1.38 + run: target/debug/wright update provider opy --version "$OPY_PROVIDER_VERSION" + + # The provider-backed language-service/LSP rename regressions + # self-skip without a real provider; point them at the pinned install + # so a stale-edit or project-scope regression fails CI. + - name: Run provider-backed language tests + run: >- + WRIGHT_OPY_PROVIDER="$WRIGHT_PROVIDER_DATA_DIR/providers/opy/$OPY_PROVIDER_VERSION/x86_64-unknown-linux-gnu/opy-provider" + cargo test --locked -p wright-language -p wright-lsp - name: Compile OPY via provider run: >- From b2ee6b3e9632ad5f3b93bcef1e13745bfbb2df85 Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Sun, 4 Oct 2026 17:07:29 +0800 Subject: [PATCH 7/9] fix(ci): build opy-provider from the pinned opy-rs commit for rename regressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/ci.yml | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c499b835..6d742fcf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -295,12 +295,6 @@ jobs: needs: [paths, rust-quality, rust-msrv, wright-cli-build] if: needs.paths.outputs.opy == 'true' runs-on: ubuntu-latest - env: - # The pinned provider version and the store it installs into, shared - # by the install step, the provider-backed test step, and the compile - # smoke check. - OPY_PROVIDER_VERSION: "0.1.38" - WRIGHT_PROVIDER_DATA_DIR: ${{ github.workspace }}/.wright-provider-data steps: - name: Checkout uses: actions/checkout@v7 @@ -340,21 +334,34 @@ jobs: && cargo test --locked -p wright-consumer --test consumer - name: Install pinned OPY provider - run: target/debug/wright update provider opy --version "$OPY_PROVIDER_VERSION" - - # The provider-backed language-service/LSP rename regressions - # self-skip without a real provider; point them at the pinned install - # so a stale-edit or project-scope regression fails CI. - - name: Run provider-backed language tests - run: >- - WRIGHT_OPY_PROVIDER="$WRIGHT_PROVIDER_DATA_DIR/providers/opy/$OPY_PROVIDER_VERSION/x86_64-unknown-linux-gnu/opy-provider" - cargo test --locked -p wright-language -p wright-lsp + run: target/debug/wright update provider opy --version 0.1.38 - name: Compile OPY via provider run: >- target/debug/wright compile tests/fixtures/opy/basic-rule.opy --profile compat + # The provider-backed language-service/LSP rename regressions need + # `lpp/rename`, which the released distribution (0.1.38) predates — + # 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 + uses: actions/checkout@v7 + with: + repository: wrightkit/opy-rs + ref: 4e2be75af207b2d792b4558b83af4ed83918bcf8 + path: opy-rs + + - name: Build OPY provider + run: cargo build --locked -p opy-provider + working-directory: opy-rs + + - name: Run provider-backed language tests + env: + WRIGHT_OPY_PROVIDER: ${{ github.workspace }}/opy-rs/target/debug/opy-provider + run: cargo test --locked -p wright-language -p wright-lsp + # ------------------------------------------------------------------------- # [3] LPP INTEGRATION (Wright's Language Provider Protocol client contract) # ------------------------------------------------------------------------- From 0c1bd0dc6bd901883695c7a3d8ae25ed9db7b9bb Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Mon, 5 Oct 2026 00:31:51 +0800 Subject: [PATCH 8/9] fix(ci): cover the provider emit -> re-parse seam for the pinned opy-rs The source-built provider links its own workshop-rs (1.4.2 today against Wright's 1.3.2): provider-emitted canonical Workshop text is re-parsed by Wright's workshop-rs in session.rs. Re-running the compile smoke with --opy-provider exercises that seam for the pinned revision, so a deps: bump that changes emitted text fails in CI instead of one provider release later. --- .github/workflows/ci.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6d742fcf..0e8e14e0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -357,6 +357,17 @@ jobs: run: cargo build --locked -p opy-provider working-directory: opy-rs + # The source-built provider carries its own workshop-rs: re-running the + # compile smoke against it keeps the emit -> re-parse artifact seam + # (session.rs `parse_with_context`) covered for the pinned revision, so + # a `deps:` bump that changes emitted Workshop text fails here rather + # than one provider release later. + - name: Compile OPY via source-built provider + run: >- + target/debug/wright compile tests/fixtures/opy/basic-rule.opy + --profile compat + --opy-provider ${{ github.workspace }}/opy-rs/target/debug/opy-provider + - name: Run provider-backed language tests env: WRIGHT_OPY_PROVIDER: ${{ github.workspace }}/opy-rs/target/debug/opy-provider From 510ad65632aa1cf21bbef3502d7f26ace3af6685 Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Mon, 5 Oct 2026 02:14:03 +0800 Subject: [PATCH 9/9] test(lsp): cover versioned rename edits hermetically, drop pinned opy-rs CI coupling Extract the RenameOutcome::Applied to WorkspaceEdit conversion into a pure function with unit tests for per-document versions, grouping, malformed URIs and the capability gate. Remove the opy-rs checkout, provider build, source-built compile smoke and the WRIGHT_OPY_PROVIDER-gated tests that froze the rename gate to one owner snapshot. --- .github/workflows/ci.yml | 32 ----- crates/wright-language/src/service.rs | 44 ------- crates/wright-lsp/src/main.rs | 168 +++++++++++++++++--------- crates/wright-lsp/tests/lsp.rs | 81 ------------- 4 files changed, 111 insertions(+), 214 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0e8e14e0..c8b69ec3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -341,38 +341,6 @@ jobs: target/debug/wright compile tests/fixtures/opy/basic-rule.opy --profile compat - # The provider-backed language-service/LSP rename regressions need - # `lpp/rename`, which the released distribution (0.1.38) predates — - # 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 - uses: actions/checkout@v7 - with: - repository: wrightkit/opy-rs - ref: 4e2be75af207b2d792b4558b83af4ed83918bcf8 - path: opy-rs - - - name: Build OPY provider - run: cargo build --locked -p opy-provider - working-directory: opy-rs - - # The source-built provider carries its own workshop-rs: re-running the - # compile smoke against it keeps the emit -> re-parse artifact seam - # (session.rs `parse_with_context`) covered for the pinned revision, so - # a `deps:` bump that changes emitted Workshop text fails here rather - # than one provider release later. - - name: Compile OPY via source-built provider - run: >- - target/debug/wright compile tests/fixtures/opy/basic-rule.opy - --profile compat - --opy-provider ${{ github.workspace }}/opy-rs/target/debug/opy-provider - - - name: Run provider-backed language tests - env: - WRIGHT_OPY_PROVIDER: ${{ github.workspace }}/opy-rs/target/debug/opy-provider - run: cargo test --locked -p wright-language -p wright-lsp - # ------------------------------------------------------------------------- # [3] LPP INTEGRATION (Wright's Language Provider Protocol client contract) # ------------------------------------------------------------------------- diff --git a/crates/wright-language/src/service.rs b/crates/wright-language/src/service.rs index fc07a66f..f856e5d8 100644 --- a/crates/wright-language/src/service.rs +++ b/crates/wright-language/src/service.rs @@ -373,50 +373,6 @@ mod tests { assert_ne!(code, "rename-refused", "a structured code must surface"); } - /// `WRIGHT_OPY_PROVIDER` points at an `opy-provider` executable; without - /// it the provider-backed rename cannot run and the test self-skips. - #[test] - fn an_unrelated_broken_open_document_does_not_block_a_project_rename() { - let Ok(provider) = std::env::var("WRIGHT_OPY_PROVIDER") else { - eprintln!("SKIPPED: WRIGHT_OPY_PROVIDER is not set"); - return; - }; - let config = SessionConfig { - opy_provider: wright_driver::OpyProviderConfig::with_executable(PathBuf::from( - provider, - )), - ..SessionConfig::default() - }; - let mut service = LanguageService::with_config(PathBuf::from("/project"), config); - let uri = open(&mut service, "main.opy", "globalvar score = 0\n"); - // A broken OPY document in the same language but outside the - // position document's project: its error must not block the rename. - open( - &mut service, - "unrelated/broken.opy", - "#!include \"missing.opy\"\n", - ); - match service.rename( - &uri, - Position { - line: 0, - character: 12, - }, - "vault", - ) { - RenameOutcome::Applied(edits) => { - assert!(!edits.is_empty()); - assert!( - edits.iter().all(|edit| edit.version == 1), - "every applied edit carries the validated document version" - ); - } - RenameOutcome::Refused { code, message } => { - panic!("expected applied edits; refused ({code}): {message}") - } - } - } - #[test] fn utf16_edit_ranges_convert_from_driver_columns() { // EditRange cols are 1-based character columns over a line whose diff --git a/crates/wright-lsp/src/main.rs b/crates/wright-lsp/src/main.rs index 9fda0ac9..528aec14 100644 --- a/crates/wright-lsp/src/main.rs +++ b/crates/wright-lsp/src/main.rs @@ -18,7 +18,7 @@ use lsp_types::{ use serde_json::Value; use wright_language::document::{Document, Position, Range}; -use wright_language::service::RenameOutcome; +use wright_language::service::{RenameOutcome, SourceTextEdit}; use wright_language::{LanguageService, OpyProviderConfig, SessionConfig}; type PublicationOwnership = BTreeMap>; @@ -193,63 +193,20 @@ fn run(opy_provider: Option) -> Result<(), String> { ¶ms.new_name, ); match outcome { - RenameOutcome::Applied(edits) => { - // `documentChanges` carries the validated document - // version per file so the client can reject the edit - // if its buffer moved while the rename was pending. - // One `TextDocumentEdit` per document; a dropped - // edit would apply a partial rename, so an - // unparseable URI is an error. - let mut grouped: BTreeMap< - String, - (i32, Vec>), - > = BTreeMap::new(); - for edit in edits { - let version = edit.version; - grouped - .entry(edit.uri) - .or_insert_with(|| (version, Vec::new())) - .1 - .push(OneOf::Left(TextEdit { - range: convert_range(edit.range), - new_text: edit.new_text, - })); + RenameOutcome::Applied(edits) => match versioned_workspace_edit(edits) { + Ok(edit) => { + write_response(&mut writer, id, serde_json::to_value(edit).unwrap())? } - let mut document_edits = Vec::with_capacity(grouped.len()); - let mut malformed = None; - for (uri, (version, text_edits)) in grouped { - match Uri::from_str(&uri) { - Ok(uri) => document_edits.push(TextDocumentEdit { - text_document: OptionalVersionedTextDocumentIdentifier::new( - uri, version, - ), - edits: text_edits, - }), - Err(_) => malformed = Some(uri), - } - } - if let Some(uri) = malformed { - write_error( - &mut writer, - id, - -32803, - &format!( - "the provider produced an edit against an unparseable URI {uri}" - ), - "rename-edit-malformed-uri", - )?; - } else { - write_response( - &mut writer, - id, - serde_json::to_value(WorkspaceEdit { - document_changes: Some(DocumentChanges::Edits(document_edits)), - ..Default::default() - }) - .unwrap(), - )?; - } - } + Err(uri) => write_error( + &mut writer, + id, + -32803, + &format!( + "the provider produced an edit against an unparseable URI {uri}" + ), + "rename-edit-malformed-uri", + )?, + }, RenameOutcome::Refused { code, message } => { write_error(&mut writer, id, -32803, &message, &code)?; } @@ -265,6 +222,40 @@ fn run(opy_provider: Option) -> Result<(), String> { Ok(()) } +/// `documentChanges` carries the validated document version per file so the +/// client can reject the edit if its buffer moved while the rename was +/// pending. One `TextDocumentEdit` per document; a dropped edit would apply a +/// partial rename, so an unparseable URI is returned as the error. +fn versioned_workspace_edit(edits: Vec) -> Result { + let mut grouped: BTreeMap>)> = + BTreeMap::new(); + for edit in edits { + let version = edit.version; + grouped + .entry(edit.uri) + .or_insert_with(|| (version, Vec::new())) + .1 + .push(OneOf::Left(TextEdit { + range: convert_range(edit.range), + new_text: edit.new_text, + })); + } + let document_edits = grouped + .into_iter() + .map(|(uri, (version, edits))| { + let parsed = Uri::from_str(&uri).map_err(|_| uri)?; + Ok(TextDocumentEdit { + text_document: OptionalVersionedTextDocumentIdentifier::new(parsed, version), + edits, + }) + }) + .collect::, String>>()?; + Ok(WorkspaceEdit { + document_changes: Some(DocumentChanges::Edits(document_edits)), + ..Default::default() + }) +} + fn initialize_result(versioned_workspace_edits: bool) -> InitializeResult { InitializeResult { capabilities: ServerCapabilities { @@ -514,3 +505,66 @@ fn convert_range(range: Range) -> LspRange { }, } } + +#[cfg(test)] +mod tests { + use super::*; + + fn edit(uri: &str, version: i32, line: u32, new_text: &str) -> SourceTextEdit { + let at = |character| Position { line, character }; + SourceTextEdit { + uri: uri.to_string(), + range: Range { + start: at(10), + end: at(15), + }, + new_text: new_text.to_string(), + version, + } + } + + #[test] + fn rename_edits_are_grouped_per_document_with_their_validated_version() { + let edits = vec![ + edit("file:///w/a.opy", 3, 0, "vault"), + edit("file:///w/b.opy", 7, 1, "vault"), + edit("file:///w/a.opy", 3, 4, "vault"), + ]; + let value = serde_json::to_value(versioned_workspace_edit(edits).unwrap()).unwrap(); + assert!(value.get("changes").is_none(), "unversioned map: {value}"); + let changes = value["documentChanges"].as_array().unwrap(); + assert_eq!(changes.len(), 2); + assert_eq!(changes[0]["textDocument"]["uri"], "file:///w/a.opy"); + assert_eq!(changes[0]["textDocument"]["version"], 3); + assert_eq!(changes[0]["edits"].as_array().unwrap().len(), 2); + assert_eq!(changes[1]["textDocument"]["uri"], "file:///w/b.opy"); + assert_eq!(changes[1]["textDocument"]["version"], 7); + assert_eq!(changes[1]["edits"][0]["newText"], "vault"); + assert_eq!(changes[1]["edits"][0]["range"]["start"]["character"], 10); + } + + #[test] + fn an_unparseable_edit_uri_refuses_the_whole_edit() { + let edits = vec![ + edit("file:///w/a.opy", 1, 0, "vault"), + edit("not a uri", 1, 0, "vault"), + ]; + assert_eq!(versioned_workspace_edit(edits).unwrap_err(), "not a uri"); + } + + #[test] + fn rename_is_advertised_only_with_versioned_workspace_edits() { + assert!( + initialize_result(true) + .capabilities + .rename_provider + .is_some() + ); + assert!( + initialize_result(false) + .capabilities + .rename_provider + .is_none() + ); + } +} diff --git a/crates/wright-lsp/tests/lsp.rs b/crates/wright-lsp/tests/lsp.rs index b235d365..cf2d27f4 100644 --- a/crates/wright-lsp/tests/lsp.rs +++ b/crates/wright-lsp/tests/lsp.rs @@ -24,15 +24,6 @@ impl LspClient { Self::launch(Command::new(env!("CARGO_BIN_EXE_wright-lsp")).current_dir(cwd)) } - fn spawn_with_provider(cwd: &Path, provider: &Path) -> Self { - Self::launch( - Command::new(env!("CARGO_BIN_EXE_wright-lsp")) - .arg("--opy-provider") - .arg(provider) - .current_dir(cwd), - ) - } - fn launch(command: &mut Command) -> Self { let mut child = command .stdin(Stdio::piped()) @@ -421,75 +412,3 @@ fn rename_is_only_negotiated_for_versioned_workspace_edits() { client.notify("exit", serde_json::json!(null)); } - -/// `WRIGHT_OPY_PROVIDER` points at an `opy-provider` executable; without it -/// the provider-backed rename cannot run and the test self-skips. -#[test] -fn rename_returns_versioned_document_changes_scoped_to_the_project() { - let Ok(provider) = std::env::var("WRIGHT_OPY_PROVIDER") else { - eprintln!("SKIPPED: WRIGHT_OPY_PROVIDER is not set"); - return; - }; - let root = workspace_root(); - let mut client = LspClient::spawn_with_provider(&root, Path::new(&provider)); - initialize(&mut client); - client.notify("initialized", serde_json::json!({})); - - let main = "file:///workspace/main.opy"; - open_document(&mut client, main, "opy", "globalvar score = 0\n"); - client.read_notification("textDocument/publishDiagnostics"); - // A broken OPY document in the same language but outside main.opy's - // project: its diagnostics must not block the target rename. - open_document( - &mut client, - "file:///workspace/unrelated/broken.opy", - "opy", - "#!include \"missing.opy\"\n", - ); - client.read_notification("textDocument/publishDiagnostics"); - - // Send the rename, then race it: the buffer changes before the client - // reads the response, so the version the provider validated no longer - // matches the client's buffer — the version tag is what lets the - // client detect that. - client.send(serde_json::json!({ - "jsonrpc": "2.0", - "id": 3, - "method": "textDocument/rename", - "params": { - "textDocument": { "uri": main }, - "position": { "line": 0, "character": 12 }, - "newName": "vault", - }, - })); - client.notify( - "textDocument/didChange", - serde_json::json!({ - "textDocument": { "uri": main, "version": 2 }, - "contentChanges": [{ "text": "# extra comment\nglobalvar score = 0\n" }], - }), - ); - - // Requests are served in order: the rename response precedes the - // diagnostics notification the didChange triggers. - let response = client.read_message(); - assert_eq!(response["id"], 3); - let document_changes = response["result"]["documentChanges"] - .as_array() - .unwrap_or_else(|| panic!("versioned documentChanges expected: {response}")); - assert_eq!(document_changes.len(), 1); - let edit = &document_changes[0]; - assert_eq!(edit["textDocument"]["uri"], main); - assert_eq!( - edit["textDocument"]["version"], 1, - "the edit carries the validated version so the client rejects it once its buffer moved: {edit}" - ); - let text_edits = edit["edits"].as_array().expect("text edits"); - assert!(!text_edits.is_empty()); - assert!( - text_edits.iter().all(|e| e["newText"] == "vault"), - "every edit applies the rename: {text_edits:?}" - ); - - client.notify("exit", serde_json::json!(null)); -}