From 95df6b3ebd1081449be63f39577a1c5500cdbd36 Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Wed, 30 Sep 2026 19:29:46 +0800 Subject: [PATCH 1/4] feat(driver): validate and rename raw Workshop edits through workshop-rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raw Workshop input now carries the advertised source-edit contract: validateEditTransaction applies the caller's transaction to the supplied current sources and reparses/revalidates the edited project through the session's own workshop-rs path, and semanticRename resolves a symbol (numeric id or declared name, as references/usage address them) or a source/line/col position and rewrites exactly the identifier spans the parsed program records — declarations, the Subroutine event binding, Call Subroutine callees, variable arguments, and Global.name/Event Player.name references. There is no textual-search fallback; unmapped occurrences refuse with rename-unmapped-span, same-namespace collisions with rename-name-collision, and edits that break the reparse with the real Workshop diagnostics. wright rename [INPUT] exposes the same validated rename: a source diff by default, an atomic write with --write that rechecks source identities and refuses edit-stale-source. Non-Workshop input stays at the provider boundary: the raw operations refuse with edit-requires-provider naming providerValidateEdit/providerSemanticRename, keeping provider-owned paths distinct. Closes #434 --- crates/wright-cli/src/cli.rs | 28 +- crates/wright-cli/src/main.rs | 5 + crates/wright-cli/src/present.rs | 55 + crates/wright-cli/tests/agent_contract.rs | 24 + crates/wright-cli/tests/cli.rs | 161 +++ crates/wright-cli/tests/serve.rs | 57 +- crates/wright-consumer/src/workflow.rs | 108 +- crates/wright-driver/src/edit.rs | 1216 ++++++++++++++++++--- crates/wright-driver/src/service.rs | 28 +- crates/wright-driver/src/session.rs | 1 + crates/wright-driver/src/session/edit.rs | 144 +++ crates/wright-driver/tests/edit.rs | 95 +- docs/agent-contract.md | 38 + docs/architecture/tooling.md | 2 + docs/cli/commands.md | 30 + schemas/wright-agent-v1.schema.json | 14 +- 16 files changed, 1767 insertions(+), 239 deletions(-) create mode 100644 crates/wright-driver/src/session/edit.rs diff --git a/crates/wright-cli/src/cli.rs b/crates/wright-cli/src/cli.rs index d979fcad..5c45465e 100644 --- a/crates/wright-cli/src/cli.rs +++ b/crates/wright-cli/src/cli.rs @@ -24,8 +24,9 @@ pub(crate) struct Cli { pub(crate) const LONG_ABOUT: &str = "Wright compiler and Workshop tooling CLI. -Commands check correctness, summarize semantic hotspots, lint, compile, or -reconstruct source through the typed wright-driver result envelope. `inspect` +Commands check correctness, summarize semantic hotspots, lint, compile, +rename Workshop symbols, or reconstruct source through the typed +wright-driver result envelope. `inspect` prints the semantic summary, and its query subcommands (symbols, refs, cfg, callgraph, cost) expose each detail area. `compile` and `convert` keep their source artifact stdout contracts; JSON mode prints only one @@ -89,6 +90,9 @@ pub(crate) enum Command { subcommand_precedence_over_arg = true )] Inspect(InspectArgs), + /// Rename a Workshop variable or subroutine semantically (#434): previews + /// the validated source diff by default; `--write` applies it atomically. + Rename(RenameArgs), /// Generate static shell completion from the command model. Completion(CompletionArgs), /// Update Wright-managed components: a standalone installation and @@ -191,6 +195,26 @@ pub(crate) struct CommonArgs { pub(crate) color: ColorArg, } +/// Arguments of `rename` (#434): the declared symbol name and the new +/// identifier as positionals, then `[INPUT]` through the shared workflow +/// options. `wright rename` covers raw Workshop input only; source languages +/// are rename surfaces of their providers. +#[derive(Debug, Args)] +pub(crate) struct RenameArgs { + /// The declared name of the variable or subroutine to rename. + #[arg(value_name = "NAME")] + pub(crate) name: String, + /// The new identifier. + #[arg(value_name = "NEW_NAME")] + pub(crate) to: String, + /// Apply the validated rename to the input file atomically instead of + /// previewing the diff. + #[arg(long)] + pub(crate) write: bool, + #[command(flatten)] + pub(crate) common: CommonArgs, +} + /// Arguments of `inspect`: an optional query subcommand naming one detail /// area, plus the shared workflow options used by the bare summary. #[derive(Debug, Args)] diff --git a/crates/wright-cli/src/main.rs b/crates/wright-cli/src/main.rs index 98e883cc..340f8b94 100644 --- a/crates/wright-cli/src/main.rs +++ b/crates/wright-cli/src/main.rs @@ -137,6 +137,11 @@ fn run_workflow(command: Command) -> ExitCode { wright_driver::CompilerSession::analyze, ) } + Command::Rename(args) => run_configured( + config_from_common(&args.common, false), + present::Presentation::from_common(&args.common), + move |session| session.rename(&args.name, &args.to, args.write), + ), Command::Lint(args) => { let mut config = config_from_common(&args.common, true); config.selection = selection_from_args(&args.select); diff --git a/crates/wright-cli/src/present.rs b/crates/wright-cli/src/present.rs index e299cec6..8ddd009f 100644 --- a/crates/wright-cli/src/present.rs +++ b/crates/wright-cli/src/present.rs @@ -12,6 +12,7 @@ use std::time::Duration; use wright_driver::Severity; use wright_driver::config::OutputFormat; +use wright_driver::edit::RenameResult; use wright_driver::progress::{ProgressEvent, ProgressObserver, ProgressPhase, ProgressUnit}; use wright_driver::result::{ AnalyzeResult, CallGraphResult, CfgResult, CheckResult, CompileResult, ConvertResult, @@ -595,6 +596,24 @@ impl ResultPresentation for CheckResult { fn render_body(&self, _ctx: &RenderContext<'_>) {} } +impl ResultPresentation for RenameResult { + fn metadata(&self) -> Option { + let edits = self + .transaction + .as_ref() + .map_or(0, |transaction| transaction.edits.len()); + let sources = self.preview.as_ref().map_or(0, Vec::len); + Some(if self.written.is_empty() { + format!("{edits} edit(s) across {sources} source(s); preview — pass --write to apply") + } else { + format!("{edits} edit(s) applied to {}", self.written.join(", ")) + }) + } + fn render_body(&self) { + render_rename(self); + } +} + impl ResultPresentation for AnalyzeResult { fn metadata(&self) -> Option { let rules = count(&self.program, "rules"); @@ -1175,6 +1194,42 @@ fn render_convert(result: &ConvertResult) { print!("{}", result.text); } +/// The `rename` human report (#434): per-source, the lines the validated +/// transaction changes, shown as `-`/`+` pairs with their line numbers — +/// rename edits only ever rewrite identifier occurrences in place. Written +/// files are listed after the diff when `--write` applied them. +fn render_rename(result: &RenameResult) { + let Some(previews) = &result.preview else { + return; + }; + for preview in previews { + println!("\n{}", preview.source); + let original = result + .originals + .get(&preview.source) + .map_or("", String::as_str); + let original_lines: Vec<&str> = original.split('\n').collect(); + let edited_lines: Vec<&str> = preview.new_text.split('\n').collect(); + let rows = original_lines.len().max(edited_lines.len()); + for index in 0..rows { + match (original_lines.get(index), edited_lines.get(index)) { + (Some(old), Some(new)) if old == new => {} + (old, new) => { + if let Some(old) = old { + println!(" {:>4} - {}", index + 1, old); + } + if let Some(new) = new { + println!(" {:>4} + {}", index + 1, new); + } + } + } + } + } + for written in &result.written { + println!("wrote {written}"); + } +} + fn array_len(value: &serde_json::Value) -> usize { value.as_array().map_or(0, Vec::len) } diff --git a/crates/wright-cli/tests/agent_contract.rs b/crates/wright-cli/tests/agent_contract.rs index 02c367aa..68e2fd3c 100644 --- a/crates/wright-cli/tests/agent_contract.rs +++ b/crates/wright-cli/tests/agent_contract.rs @@ -261,6 +261,30 @@ fn agent_v1_schema_covers_every_advertised_request_and_response() { ); } + // #434: `semanticRename` targets address a symbol by id or name, or by a + // position inside one identifier occurrence; both forms validate and + // deserialize. + for request in [ + json!({"op":"semanticRename","sources":{},"target":{"symbol":0,"to":"renamed"}}), + json!({"op":"semanticRename","sources":{},"target":{"symbol":"score","to":"renamed"}}), + json!({"op":"semanticRename","sources":{},"target":{"source":"a.ws","line":1,"col":3,"to":"renamed"}}), + ] { + assert!( + request_schema.is_valid(&request), + "invalid request: {request}" + ); + serde_json::from_value::(request).expect("request deserializes"); + } + for request in [ + json!({"op":"semanticRename","sources":{},"target":{"symbol":true,"to":"x"}}), + json!({"op":"semanticRename","sources":{},"target":{"to":"x","extra":1}}), + ] { + assert!( + !request_schema.is_valid(&request), + "schema accepted an invalid rename target: {request}" + ); + } + let capabilities_response = json!({"result":current}); assert!(response_schema.is_valid(&capabilities_response)); assert!(response_schema.is_valid(&json!({ diff --git a/crates/wright-cli/tests/cli.rs b/crates/wright-cli/tests/cli.rs index 742331f2..3a54a215 100644 --- a/crates/wright-cli/tests/cli.rs +++ b/crates/wright-cli/tests/cli.rs @@ -2168,6 +2168,42 @@ fn inspect_names_unnamed_rules_by_index() { let _ = std::fs::remove_dir_all(path.parent().unwrap()); } +#[test] +fn rename_previews_a_validated_diff_without_writing() { + // #434: `wright rename` defaults to a source diff; the file stays + // untouched and no temporary file leaks next to it. + let original = corpus_workshop("synthetic/declarations-numbers"); + let path = temp_file("rename.ws", &original); + let output = run(&[ + "rename", + "score", + "total", + path.to_str().unwrap(), + "--kind", + "workshop", + ]); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("PASS rename"), "{stdout}"); + assert!(stdout.contains("- 0: score"), "{stdout}"); + assert!(stdout.contains("+ 0: total"), "{stdout}"); + assert!( + stdout.contains("--write"), + "the preview names the apply flag" + ); + assert_eq!(std::fs::read_to_string(&path).unwrap(), original); + assert_eq!( + path.parent().unwrap().read_dir().unwrap().count(), + 1, + "no temporary sibling remains" + ); + let _ = std::fs::remove_dir_all(path.parent().unwrap()); +} + #[test] fn inspect_overview_and_cfg_bound_large_detail() { // A program larger than one screen: 13 rules, one of them a wide graph. @@ -2206,3 +2242,128 @@ fn inspect_overview_and_cfg_bound_large_detail() { ); let _ = std::fs::remove_dir_all(Path::new(path).parent().unwrap()); } + +#[test] +fn rename_write_applies_the_validated_change_atomically() { + // #434: --write replaces the input; the result reparses cleanly. + let path = temp_file( + "rename.ws", + &corpus_workshop("synthetic/declarations-numbers"), + ); + let path_str = path.to_str().unwrap(); + let output = run(&[ + "rename", "score", "total", path_str, "--kind", "workshop", "--write", + ]); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains(&format!("wrote {path_str}")), "{stdout}"); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.contains("0: total"), "{text}"); + assert!(text.contains("Set Global Variable(total, 5)"), "{text}"); + assert!(!text.contains("score"), "{text}"); + let check = run(&["check", path_str, "--kind", "workshop"]); + assert!(check.status.success(), "{}", command_result(&check)); + let _ = std::fs::remove_dir_all(path.parent().unwrap()); +} + +#[test] +fn rename_reports_the_envelope_in_json_mode() { + let path = temp_file( + "rename.ws", + &corpus_workshop("synthetic/declarations-numbers"), + ); + let output = run(&[ + "rename", + "score", + "total", + path.to_str().unwrap(), + "--kind", + "workshop", + "-f", + "json", + ]); + assert!(output.status.success(), "{}", command_result(&output)); + let envelope = parse_json(&output.stdout); + assert_eq!(envelope["command"], "rename"); + assert_eq!(envelope["ok"], true); + assert_eq!(envelope["wright"]["contract"], "wright-result/v1"); + assert!( + envelope["result"]["transaction"]["edits"] + .as_array() + .unwrap() + .len() + >= 2, + "{envelope}" + ); + assert!( + envelope["result"]["preview"][0]["new_text"] + .as_str() + .unwrap() + .contains("0: total"), + "{envelope}" + ); + let _ = std::fs::remove_dir_all(path.parent().unwrap()); +} + +#[test] +fn rename_refusals_carry_structured_diagnostics_and_write_nothing() { + // #434: unknown names, collisions, and non-Workshop input refuse with the + // structured codes; --write still writes nothing. + let original = corpus_workshop("synthetic/declarations-numbers"); + let path = temp_file("rename.ws", &original); + for (name, code) in [ + ("missing", "unknown-symbol"), + ("numbers", "rename-unsupported-kind"), + ] { + let output = run(&[ + "rename", + name, + "renamed", + path.to_str().unwrap(), + "--kind", + "workshop", + "-f", + "json", + "--write", + ]); + assert_eq!(output.status.code(), Some(1), "{name}"); + let envelope = parse_json(&output.stdout); + assert_eq!(envelope["diagnostics"][0]["code"], code, "{envelope}"); + assert_eq!(std::fs::read_to_string(&path).unwrap(), original); + } + + // OPY input routes to the provider operation rather than renaming + // through the Workshop path. + let opy = temp_file("program.opy", "rule \"r\":\n pass\n"); + let output = run(&[ + "rename", + "r", + "renamed", + opy.to_str().unwrap(), + "--kind", + "opy", + "-f", + "json", + ]); + assert_eq!(output.status.code(), Some(1)); + let envelope = parse_json(&output.stdout); + assert_eq!(envelope["diagnostics"][0]["code"], "edit-requires-provider"); + assert!( + envelope["diagnostics"][0]["message"] + .as_str() + .unwrap() + .contains("providerSemanticRename"), + "{}", + envelope["diagnostics"][0]["message"] + ); + assert_eq!( + std::fs::read_to_string(&opy).unwrap(), + "rule \"r\":\n pass\n" + ); + let _ = std::fs::remove_dir_all(path.parent().unwrap()); + let _ = std::fs::remove_dir_all(opy.parent().unwrap()); +} diff --git a/crates/wright-cli/tests/serve.rs b/crates/wright-cli/tests/serve.rs index 506734b8..803dd6bb 100644 --- a/crates/wright-cli/tests/serve.rs +++ b/crates/wright-cli/tests/serve.rs @@ -368,31 +368,27 @@ fn capability_negotiation_is_preserved() { #[test] fn stdio_transport_serves_mutation_operations() { - // #130: the stdio adapter exposes the shared mutation operations as + // #130/#434: the stdio adapter exposes the shared mutation operations as // thin mappings — validated edit preview and semantic rename — with the - // same structured all-or-nothing results as in-process consumers. + // same structured all-or-nothing results as in-process consumers. On raw + // Workshop input both validate through `workshop-rs` reparse. let input = corpus_workshop("synthetic/control-flow"); let source = std::fs::read_to_string(&input).unwrap(); let identity = wright_driver::input_identity(&source); - let line_count = source.lines().count().max(1) as u32; - let end_col = source - .lines() - .last() - .map(|line| line.chars().count() as u32 + 1) - .unwrap_or(1); + // A rule-name rewrite stays valid Workshop: `"bounded while"` at 6:8-21. let request = serde_json::json!({ "op": "validateEditTransaction", "sources": { input.to_string_lossy().into_owned(): source.clone() }, "transaction": { "edits": [{ - "kind": "rename", + "kind": "edit", "source": input.to_string_lossy().into_owned(), "source_identity": identity, "range": { - "start_line": 1, "start_col": 1, - "end_line": line_count, "end_col": end_col, + "start_line": 6, "start_col": 8, + "end_line": 6, "end_col": 21, }, - "new_text": source.replace("j", "total") + "new_text": "bounded loop" }] } }); @@ -401,33 +397,41 @@ fn stdio_transport_serves_mutation_operations() { &input, &[&serde_json::to_string(&request).unwrap()], ); - assert_eq!(responses[0]["result"]["ok"], false, "{responses:?}"); + assert_eq!(responses[0]["result"]["ok"], true, "{responses:?}"); + assert!( + responses[0]["result"]["preview"][0]["new_text"] + .as_str() + .unwrap() + .contains("\"bounded loop\""), + "the preview carries the edited source: {responses:?}" + ); - // Semantic rename through the same transport. + // Semantic rename through the same transport (#434): the declared name + // addresses the symbol; every occurrence rewrites through provenance. let rename = serde_json::json!({ "op": "semanticRename", - "sources": { - input.to_string_lossy().into_owned(): - std::fs::read_to_string(&input).unwrap() - }, - "target": { "source": input.to_string_lossy().into_owned(), "line": 1, "col": 11, "to": "total" } + "sources": { input.to_string_lossy().into_owned(): source.clone() }, + "target": { "symbol": "index", "to": "counter" } }); let responses = run_lines("stdio", &input, &[&serde_json::to_string(&rename).unwrap()]); - assert_eq!(responses[0]["result"]["ok"], false, "{responses:?}"); + assert_eq!(responses[0]["result"]["ok"], true, "{responses:?}"); + let preview = responses[0]["result"]["preview"][0]["new_text"] + .as_str() + .unwrap(); + assert!(preview.contains("0: counter"), "{preview}"); + assert!(preview.contains("Global.counter"), "{preview}"); } #[test] fn transports_are_equivalent_for_mutation_operations() { - // #130: stdio and JSON-RPC map the same mutation request to the same + // #130/#434: stdio and JSON-RPC map the same mutation request to the same // in-process behavior. let input = corpus_workshop("synthetic/control-flow"); + let source = std::fs::read_to_string(&input).unwrap(); let rename = serde_json::json!({ "op": "semanticRename", - "sources": { - input.to_string_lossy().into_owned(): - std::fs::read_to_string(&input).unwrap() - }, - "target": { "source": input.to_string_lossy().into_owned(), "line": 1, "col": 11, "to": "total" } + "sources": { input.to_string_lossy().into_owned(): source }, + "target": { "symbol": "index", "to": "counter" } }); let stdio = run_lines("stdio", &input, &[&serde_json::to_string(&rename).unwrap()]); let jsonrpc_request = serde_json::json!({ @@ -439,4 +443,5 @@ fn transports_are_equivalent_for_mutation_operations() { &[&serde_json::to_string(&jsonrpc_request).unwrap()], ); assert_eq!(stdio[0]["result"], jsonrpc[0]["result"]); + assert_eq!(stdio[0]["result"]["ok"], true, "{:?}", stdio[0]); } diff --git a/crates/wright-consumer/src/workflow.rs b/crates/wright-consumer/src/workflow.rs index 76edc845..7d03c2ed 100644 --- a/crates/wright-consumer/src/workflow.rs +++ b/crates/wright-consumer/src/workflow.rs @@ -86,63 +86,63 @@ pub fn run_consumer(input: &str) -> Result<(), String> { } } - if input.ends_with(".opy") { - if let Some(name) = first_global(&source) { - let identity = wright_driver::input_identity(&source); - let rename = wright_driver::edit::rename_symbol( - &source, - &wright_driver::edit::RenameRequest { - symbol_kind: "globalVariable".to_string(), - from: name.to_string(), - to: "renamed_by_consumer".to_string(), - source: input.to_string(), - source_identity: identity, - }, - ) - .map_err(|e| e.message)?; - let sources = std::collections::BTreeMap::from([(input.to_string(), source.clone())]); - let validation = wright_driver::edit::validate_transaction( - &SessionConfig { - input: InputSpec::Path(input.into()), - ..SessionConfig::default() - }, - &sources, - &wright_driver::edit::EditTransaction::new(vec![rename]).map_err(|e| e.message)?, - ); - assert!( - validation.ok, - "rename validates: {:?}", - validation.diagnostics - ); - assert!( - validation - .preview - .as_ref() - .unwrap() - .iter() - .any(|p| p.new_text.contains("renamed_by_consumer")) - ); - println!("edit: safe rename validated and previewed"); - } else { - println!("edit: no global variable to rename (skipped)"); + // Raw Workshop semantic rename (#434): address one declared symbol by + // name through the service surface, and verify the validated preview + // rewrites its occurrences. + if input.ends_with(".ws") { + let renamed = match service.handle(&ToolRequest::Symbols { + kind: Some("globalVariable".to_string()), + }) { + wright_driver::service::ToolResponse::Ok { result } => result + .as_array() + .and_then(|symbols| symbols.first()) + .and_then(|symbol| symbol.get("name")) + .and_then(serde_json::Value::as_str) + .map(str::to_string), + wright_driver::service::ToolResponse::Error { error } => { + panic!("symbols failed: {error:?}") + } + }; + match renamed { + Some(name) => { + let rename = service.handle(&ToolRequest::SemanticRename { + sources: std::collections::BTreeMap::from([( + input.to_string(), + source.clone(), + )]), + target: wright_driver::edit::RenameTarget { + symbol: Some(wright_driver::service::Address::Name(name)), + source: None, + line: None, + col: None, + to: "renamed_by_consumer".to_string(), + }, + }); + match rename { + wright_driver::service::ToolResponse::Ok { result } => { + assert!(result["ok"].as_bool().unwrap_or(false), "rename validates"); + assert!( + result["preview"] + .as_array() + .unwrap() + .iter() + .any(|p| p["new_text"] + .as_str() + .unwrap_or_default() + .contains("renamed_by_consumer")), + "the preview rewrites the identifier" + ); + println!("edit: semantic rename validated and previewed"); + } + wright_driver::service::ToolResponse::Error { error } => { + panic!("rename failed: {error:?}") + } + } + } + None => println!("edit: no global variable to rename (skipped)"), } } println!("consumer: all public-API workflows succeeded"); Ok(()) } - -fn first_global(source: &str) -> Option<&str> { - source - .lines() - .find_map(|line| { - let trimmed = line.trim_start(); - trimmed.strip_prefix("globalvar").map(|rest| { - rest.trim_start() - .split(|c: char| c.is_whitespace() || c == '=') - .next() - .unwrap_or("") - }) - }) - .filter(|name| !name.is_empty()) -} diff --git a/crates/wright-driver/src/edit.rs b/crates/wright-driver/src/edit.rs index 31b41378..7269a9fc 100644 --- a/crates/wright-driver/src/edit.rs +++ b/crates/wright-driver/src/edit.rs @@ -1,14 +1,28 @@ //! Tools and agents propose edits as validated, source-oriented //! [`SourceEdit`]s — never as mutations of Wright's internal IR. +//! +//! Raw Workshop input is edited through `workshop-rs` itself (#434): a +//! transaction applies to the caller-supplied current sources, the edited +//! result is reparsed and validated through the session's own Workshop +//! parse path, and a semantic rename rewrites exactly the identifier spans +//! the parsed program reports — declarations, references, and positions +//! carry the same provenance. Source languages stay at the provider +//! boundary (`providerValidateEdit` / `providerSemanticRename`); there is +//! no textual-search rename and no static fallback for them. use std::collections::BTreeMap; -use std::path::Path; +use std::path::{Path, PathBuf}; use serde::{Deserialize, Serialize}; +use workshop_rs::catalog::{Catalog, Locale}; +use wright_analyzer::canonical::{ReferenceKind, SemanticIndex, Symbol, SymbolId, SymbolKind}; use crate::config::{SessionConfig, SourceKind}; use crate::diag::{Diagnostic, Position, SourceSpan, Stage, source_provider_unavailable}; +use crate::input::{self, ResolvedInput}; use crate::result::exit_code_from; +use crate::service::Address; +use crate::session::{Loaded, workshop_diag}; /// One proposed source edit. #[derive(Debug, Clone, Serialize, Deserialize)] @@ -93,10 +107,19 @@ impl EditTransaction { Ok(EditTransaction { edits }) } + /// Apply the transaction to the supplied current sources. Every edit's + /// `source` key must be present in `sources` and carry the identity of + /// the supplied text — the precondition is checked per edit before any + /// application. pub fn apply( &self, sources: &BTreeMap, ) -> Result, Diagnostic> { + for edit in &self.edits { + if let Some(diagnostic) = source_precondition(edit, sources) { + return Err(diagnostic); + } + } let mut grouped: BTreeMap<&str, Vec<&SourceEdit>> = BTreeMap::new(); for edit in &self.edits { grouped.entry(&edit.source).or_default().push(edit); @@ -140,87 +163,142 @@ pub struct EditValidation { pub preview: Option>, } -fn validate_project_input( - config: &SessionConfig, - sources: &BTreeMap, - previews: Option<&[SourcePreview]>, -) -> Result<(), Diagnostic> { - let Some(main_path) = config.input.path() else { - return Err(Diagnostic::error( - "edit-input-stdin", - Stage::Discovery, - "edit validation requires a path-based input so the edited project's main source identity is established; stdin has no project identity", - )); - }; - validate_source_kind(config, main_path)?; - - let main_source = main_path.to_string_lossy().into_owned(); - if previews - .and_then(|previews| preview_of(previews, main_path)) - .is_some() - || sources.contains_key(&main_source) - { - return Ok(()); - } - std::fs::read_to_string(main_path).map(|_| ()).map_err(|e| { - Diagnostic::error( - "input-io", - Stage::Discovery, - format!("cannot read input '{}': {e}", main_path.display()), - ) - }) -} - +/// `validateEditTransaction` (#434): every edit must name a supplied, +/// current source; the transaction applies atomically to the caller's +/// sources; and on raw Workshop input the edited project is reparsed and +/// validated through the session's own `workshop-rs` path, so an invalid +/// transaction refuses with the real Workshop diagnostics and no partial +/// preview. Other source languages route to `providerValidateEdit`. pub fn validate_transaction( config: &SessionConfig, + catalog: &Catalog, sources: &BTreeMap, transaction: &EditTransaction, ) -> EditValidation { - let mut diagnostics = Vec::new(); - for edit in &transaction.edits { - if let Some(diagnostic) = source_precondition(edit, sources) { - diagnostics.push(diagnostic); - } + let refuse = |diagnostics: Vec| EditValidation { + ok: false, + exit: exit_code_from(&diagnostics), + diagnostics, + preview: None, + }; + let resolved = match resolve_edit_input(config) { + Ok(resolved) => resolved, + Err(diagnostic) => return refuse(vec![diagnostic]), + }; + if let Some(diagnostic) = edit_kind_gate(resolved.kind, "providerValidateEdit") { + return refuse(vec![diagnostic]); } + let mut diagnostics: Vec = transaction + .edits + .iter() + .filter_map(|edit| source_precondition(edit, sources)) + .collect(); if diagnostics .iter() .any(|d| d.severity == crate::diag::Severity::Error) { - return refusal(diagnostics); + return refuse(diagnostics); } - let previews = match transaction.apply(sources) { Ok(previews) => previews, Err(diagnostic) => { diagnostics.push(diagnostic); - return refusal(diagnostics); + return refuse(diagnostics); } }; + // A Workshop project is one source file: the edited text is the preview + // when the transaction touched the input, else the caller's current + // text for it, else the on-disk input the edits were checked against. + let edited = previews + .iter() + .find(|preview| same_source(&preview.source, &resolved)) + .map(|preview| preview.new_text.as_str()) + .or_else(|| { + sources + .iter() + .find(|(source, _)| same_source(source, &resolved)) + .map(|(_, text)| text.as_str()) + }) + .unwrap_or(&resolved.text); + match parse_workshop(edited, catalog, config.locale.as_deref(), &resolved) { + Ok(_) => EditValidation { + ok: true, + exit: 0, + diagnostics, + preview: Some(previews), + }, + Err(parse_diagnostics) => refuse(parse_diagnostics), + } +} - if let Err(diagnostic) = validate_project_input(config, sources, Some(&previews)) { - diagnostics.push(diagnostic); - return refusal(diagnostics); +/// Resolve the session input for an edit operation without consuming stdin: +/// stdin carries no source identity, so edit operations refuse it outright. +fn resolve_edit_input(config: &SessionConfig) -> Result { + if config.input.path().is_none() { + return Err(edit_stdin_refusal()); } - refusal(vec![source_provider_unavailable()]) + input::resolve(config) } -fn refusal(diagnostics: Vec) -> EditValidation { - EditValidation { - ok: false, - exit: exit_code_from(&diagnostics), - diagnostics, - preview: None, +fn edit_stdin_refusal() -> Diagnostic { + Diagnostic::error( + "edit-input-stdin", + Stage::Discovery, + "edit operations require a path-based input; stdin has no source identity", + ) +} + +/// The source-kind boundary shared by the raw edit operations (#434): raw +/// Workshop input validates through `workshop-rs`; OPY and other source +/// languages belong to the provider operations (`providerValidateEdit` / +/// `providerSemanticRename`), which keep their own contract. The refusal +/// names the operation a caller should route to. +pub(crate) fn edit_kind_gate(kind: SourceKind, provider_operation: &str) -> Option { + match kind { + SourceKind::Workshop => None, + SourceKind::Opy => Some(Diagnostic::error( + "edit-requires-provider", + Stage::Discovery, + format!( + "raw edit operations cover Workshop input; for OPY sources route through '{provider_operation}' instead" + ), + )), + SourceKind::Ostw => Some(source_provider_unavailable()), + other => Some(Diagnostic::error( + "edit-unsupported-kind", + Stage::Discovery, + format!( + "edit operations are not defined for '{}' input", + other.as_str() + ), + )), } } +/// The `semanticRename` target (#434): either a symbol address — the numeric +/// id or the declared name, exactly as `references`/`usage` address symbols +/// (#429) — or a `source`/`line`/`col` position inside one identifier +/// occurrence. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct RenameTarget { - pub source: String, - pub line: u32, - pub col: u32, + /// The symbol to rename, by numeric id or declared name. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub symbol: Option
, + /// Position addressing: the source file the position is interpreted in. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub source: Option, + /// Position addressing: the 1-based line. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub line: Option, + /// Position addressing: the 1-based column. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub col: Option, + /// The new identifier. pub to: String, } +/// The `semanticRename` result: the validated transaction and its previews, +/// or refusal diagnostics with no transaction and no partial preview. #[derive(Debug, Clone, Serialize)] pub struct SemanticRename { pub ok: bool, @@ -230,8 +308,35 @@ pub struct SemanticRename { pub preview: Option>, } +/// The `wright rename` result (#434): the validated transaction and the +/// per-source previews; `written` lists the files `--write` updated. +#[derive(Debug, Clone, Default, Serialize)] +pub struct RenameResult { + /// The validated rename transaction (absent on refusal). + #[serde(skip_serializing_if = "Option::is_none")] + pub transaction: Option, + /// Per-source previews of the validated transaction. + #[serde(skip_serializing_if = "Option::is_none")] + pub preview: Option>, + /// The files updated by `--write` (empty in preview mode). + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub written: Vec, + /// The source text the transaction was validated against — presentation + /// input for the human diff, not part of the JSON contract. + #[serde(skip)] + pub originals: BTreeMap, +} + +/// `semanticRename` on raw Workshop input (#434): the target resolves +/// against the loaded program's semantic index — the same symbol and +/// position addressing `references`/`usage` use — and every occurrence is +/// rewritten through the program's own identifier provenance, never a +/// textual search. The proposed transaction is validated by reparsing the +/// edited source; every refusal carries structured diagnostics and no +/// partial edit set. pub fn semantic_rename( - config: &SessionConfig, + loaded: &Loaded, + catalog: &Catalog, sources: &BTreeMap, target: &RenameTarget, ) -> SemanticRename { @@ -241,7 +346,6 @@ pub fn semantic_rename( diagnostics, preview: None, }; - if target.to.is_empty() { return refuse(vec![Diagnostic::error( "rename-invalid-name", @@ -249,52 +353,462 @@ pub fn semantic_rename( "rename requires a non-empty new name", )]); } - - if let Err(diagnostic) = validate_project_input(config, sources, None) { + if let Some(diagnostic) = edit_kind_gate(loaded.input.kind, "providerSemanticRename") { return refuse(vec![diagnostic]); } - refuse(vec![source_provider_unavailable()]) -} -fn validate_source_kind(config: &SessionConfig, main_path: &Path) -> Result<(), Diagnostic> { - match config.kind { - SourceKind::Opy => Ok(()), - SourceKind::Ostw => Err(source_provider_unavailable()), - SourceKind::Auto => match main_path - .extension() - .and_then(|e| e.to_str()) - .map(|e| e.to_ascii_lowercase()) - .as_deref() - { - Some("opy") => Ok(()), - Some("ostw" | "del") => Err(source_provider_unavailable()), - _ => Err(Diagnostic::error( - "edit-unsupported-kind", + let Some(input_path) = loaded.input.path.clone() else { + return refuse(vec![edit_stdin_refusal()]); + }; + // The caller supplies the current text of the file being renamed; it + // must be the text the loaded program was parsed from, so every + // provenance span indexes into it exactly. + let Some((source, current)) = sources + .iter() + .find(|(source, _)| same_file(source, &input_path)) + .map(|(source, text)| (source.clone(), text.clone())) + else { + return refuse(vec![Diagnostic::error( + "edit-unknown-source", + Stage::Discovery, + format!( + "semantic rename needs the current text of '{}' in `sources`", + loaded.input.display + ), + )]); + }; + if current != loaded.input.text { + return refuse(vec![Diagnostic::error( + "edit-stale-source", + Stage::Discovery, + format!( + "the supplied text of '{source}' differs from the loaded program; reload the project and retry" + ), + )]); + } + + let index = SemanticIndex::build(&loaded.program); + let symbol = match resolve_rename_target(&index, loaded, &input_path, target) { + Ok(symbol) => symbol, + Err(diagnostic) => return refuse(vec![diagnostic]), + }; + match symbol.kind { + SymbolKind::GlobalVariable | SymbolKind::PlayerVariable | SymbolKind::Subroutine => {} + other => { + return refuse(vec![Diagnostic::error( + "rename-unsupported-kind", Stage::Discovery, format!( - "cannot detect the source kind of '{}' for edit validation; pass an explicit `opy` source kind", - main_path.display() + "semantic rename covers variables and subroutines, not '{}' symbols", + other.as_str() ), - )), + )]); + } + } + // Same-namespace collision: `to` must not already name another symbol of + // the same kind — global and player variables are separate namespaces, + // so the check is kind-scoped. + if let Some(other) = index + .symbols() + .find(|other| other.id != symbol.id && other.kind == symbol.kind && other.name == target.to) + { + return refuse(vec![Diagnostic::error( + "rename-name-collision", + Stage::Discovery, + format!( + "'{}' already names {} #{}; renaming '{}' would collide", + target.to, + other.kind.as_str(), + other.id.index(), + symbol.name + ), + )]); + } + + // Rewrite every occurrence through the program's exact identifier + // provenance. A reference without an authored span — or one whose span + // does not cover exactly the identifier in the supplied text — refuses + // rather than guessing. + let mut spans: Vec = Vec::new(); + for reference in index.references(symbol.id) { + let Some(occurrence) = reference.occurrence else { + return refuse(vec![rename_unmapped(loaded, symbol)]); + }; + let Ok(edit) = loaded.program.edit_source(occurrence, &target.to) else { + return refuse(vec![rename_unmapped(loaded, symbol)]); + }; + if current.get(edit.range()) != Some(symbol.name.as_str()) { + return refuse(vec![rename_unmapped(loaded, symbol)]); + } + if !spans.contains(&occurrence) { + spans.push(occurrence); + } + } + if spans.is_empty() { + return refuse(vec![Diagnostic::error( + "rename-invalid-target", + Stage::Discovery, + format!("symbol '{}' carries no identifier occurrences", symbol.name), + )]); + } + spans.sort_by_key(|span| { + ( + span.file.index(), + span.start.line, + span.start.col, + span.end.line, + span.end.col, + ) + }); + let identity = crate::input_identity(¤t); + let transaction = match EditTransaction::new( + spans + .iter() + .map(|span| SourceEdit { + edit_kind: "rename".to_string(), + source: source.clone(), + source_identity: identity.clone(), + range: EditRange { + start_line: span.start.line, + start_col: span.start.col, + end_line: span.end.line, + end_col: span.end.col, + }, + new_text: target.to.clone(), + }) + .collect(), + ) { + Ok(transaction) => transaction, + Err(diagnostic) => return refuse(vec![diagnostic]), + }; + let previews = match transaction.apply(sources) { + Ok(previews) => previews, + Err(diagnostic) => return refuse(vec![diagnostic]), + }; + // The rename edits only ever touch the loaded input's source. + let edited = previews + .iter() + .find(|preview| same_file(&preview.source, &input_path)) + .expect("rename edits only ever touch the loaded input") + .new_text + .clone(); + match verify_rename(&index, symbol, &target.to, &edited, loaded, catalog) { + Ok(()) => SemanticRename { + ok: true, + transaction: Some(transaction), + diagnostics: Vec::new(), + preview: Some(previews), }, - other => Err(Diagnostic::error( - "edit-unsupported-kind", + Err(diagnostics) => refuse(diagnostics), + } +} + +/// Resolve the rename target: a numeric or name `symbol` address (#429), or +/// a `source`/`line`/`col` position inside one identifier occurrence. +fn resolve_rename_target<'a>( + index: &'a SemanticIndex, + loaded: &Loaded, + input_path: &Path, + target: &RenameTarget, +) -> Result<&'a Symbol, Diagnostic> { + match &target.symbol { + Some(Address::Id(id)) => { + index + .symbol(SymbolId::from_index(*id as usize)) + .ok_or_else(|| { + Diagnostic::error( + "invalid-id", + Stage::Discovery, + format!("unknown symbol {id}"), + ) + }) + } + Some(Address::Name(name)) => resolve_named_symbol(index, name), + None => symbol_at_position(index, loaded, input_path, target), + } +} + +/// Resolve a declared name to one symbol — the same resolution `references` +/// and `usage` apply: unmatched names are `unknown-symbol`, names shared by +/// more than one symbol are `ambiguous-symbol` listing candidate ids. +fn resolve_named_symbol<'a>( + index: &'a SemanticIndex, + name: &str, +) -> Result<&'a Symbol, Diagnostic> { + let matches: Vec<&Symbol> = index + .symbols() + .filter(|symbol| symbol.name == name) + .collect(); + match matches.as_slice() { + [symbol] => Ok(symbol), + [] => Err(Diagnostic::error( + "unknown-symbol", + Stage::Discovery, + format!("unknown symbol '{name}'"), + )), + _ => Err(Diagnostic::error( + "ambiguous-symbol", Stage::Discovery, format!( - "edit validation is declared over the OPY source provider; '{}' input is not an editable source kind", - other.as_str() + "ambiguous symbol '{name}': {}", + matches + .iter() + .map(|symbol| format!("{} {}", symbol.kind.as_str(), symbol.id.index())) + .collect::>() + .join(", ") + ), + )), + } +} + +/// Resolve a `source`/`line`/`col` position to the symbol whose identifier +/// occurrence contains it. +fn symbol_at_position<'a>( + index: &'a SemanticIndex, + loaded: &Loaded, + input_path: &Path, + target: &RenameTarget, +) -> Result<&'a Symbol, Diagnostic> { + let (Some(source), Some(line), Some(col)) = (target.source.as_deref(), target.line, target.col) + else { + return Err(Diagnostic::error( + "rename-invalid-target", + Stage::Discovery, + "semanticRename needs a `symbol` (numeric id or declared name) or a `source`/`line`/`col` position", + )); + }; + if !same_file(source, input_path) { + return Err(Diagnostic::error( + "rename-invalid-target", + Stage::Discovery, + format!( + "the position source '{source}' does not name the loaded input '{}'", + loaded.input.display + ), + )); + } + let mut hits = Vec::new(); + for symbol in index.symbols() { + let hit = index.references(symbol.id).iter().any(|reference| { + reference + .occurrence + .is_some_and(|span| position_in_span(span, line, col)) + }); + if hit { + hits.push(symbol); + } + } + match hits.as_slice() { + [symbol] => Ok(symbol), + [] => Err(Diagnostic::error( + "rename-invalid-target", + Stage::Discovery, + format!("no symbol identifier covers {source}:{line}:{col}"), + )), + _ => Err(Diagnostic::error( + "ambiguous-symbol", + Stage::Discovery, + format!( + "more than one symbol covers {source}:{line}:{col}: {}", + hits.iter() + .map(|symbol| format!("{} {}", symbol.kind.as_str(), symbol.id.index())) + .collect::>() + .join(", ") ), )), } } -fn preview_of<'a>(previews: &'a [SourcePreview], main_path: &Path) -> Option<&'a SourcePreview> { - previews.iter().find(|p| same_file(&p.source, main_path)) +/// Whether the 1-based `line`/`col` falls inside the half-open span. +fn position_in_span(span: workshop_rs::source::Span, line: u32, col: u32) -> bool { + (line > span.start.line || (line == span.start.line && col >= span.start.col)) + && (line < span.end.line || (line == span.end.line && col < span.end.col)) +} + +/// A provenance gap on one reference: either no authored identifier span +/// exists, or the span does not cover exactly the symbol's name in the +/// caller's text. Both are refusals — rename never degrades to a textual +/// search. +fn rename_unmapped(loaded: &Loaded, symbol: &Symbol) -> Diagnostic { + Diagnostic::error( + "rename-unmapped-span", + Stage::Discovery, + format!( + "a '{}' occurrence of {} '{}' does not map to an exact identifier span in '{}'", + symbol.name, + symbol.kind.as_str(), + symbol.id.index(), + loaded.input.display + ), + ) +} + +/// Reparse `edited` through the loaded program's parse context and verify +/// the rename invariant: the text stays a valid Workshop program and the +/// `(kind, to)` symbol binds the same reference-kind multiset that `(kind, +/// from)` bound before. A name that does not survive reparsing — or a +/// reference set that changed — refuses with no transaction. +fn verify_rename( + index: &SemanticIndex, + symbol: &Symbol, + to: &str, + edited: &str, + loaded: &Loaded, + catalog: &Catalog, +) -> Result<(), Vec> { + let locale = Locale::new( + loaded + .origin + .locale + .as_deref() + .expect("Workshop loads record their resolved locale"), + ); + let program = workshop_rs::parser::parse_with_context(edited, catalog, &locale, catalog) + .map_err(|error| vec![workshop_diag(error, &loaded.input)])?; + program + .validate() + .map_err(|error| vec![workshop_diag(error, &loaded.input)])?; + let reparsed = SemanticIndex::build(&program); + let expected = reference_signature(index, symbol.id); + let actual = reparsed + .symbols() + .find(|candidate| candidate.kind == symbol.kind && candidate.name == to) + .map(|candidate| reference_signature(&reparsed, candidate.id)); + match actual { + Some(actual) if actual == expected => Ok(()), + _ => Err(vec![Diagnostic::error( + "rename-mismatch", + Stage::Validation, + format!( + "the edited source no longer binds the same references to '{to}' as '{}' did; refusing rather than reporting a partial rename", + symbol.name + ), + )]), + } +} + +/// The sorted multiset of reference kinds for one symbol. +fn reference_signature(index: &SemanticIndex, id: SymbolId) -> Vec { + let mut kinds: Vec = index + .references(id) + .iter() + .map(|reference| reference.kind) + .collect(); + kinds.sort_by_key(|kind| *kind as usize); + kinds +} + +/// Whether a `sources` key names the resolved input file: plain path +/// spellings compare by canonical identity, and a `file://` URI spelling of +/// the same path binds too. +fn same_source(source: &str, resolved: &ResolvedInput) -> bool { + resolved + .path + .as_deref() + .is_some_and(|path| same_file(source, path)) } -fn same_file(a: &str, b: &Path) -> bool { - let b_str = b.to_string_lossy(); - a == b_str - || matches!((Path::new(a).canonicalize(), b.canonicalize()), (Ok(ca), Ok(cb)) if ca == cb) +fn same_file(source: &str, path: &Path) -> bool { + let source = source.strip_prefix("file://").unwrap_or(source); + source == path.to_string_lossy() + || matches!( + (Path::new(source).canonicalize(), path.canonicalize()), + (Ok(a), Ok(b)) if a == b + ) +} + +/// Parse and validate Workshop text through the session's own path: locale +/// detection (with the session's override), then `workshop-rs` parse and +/// validation — a refused transaction carries the real Workshop +/// diagnostics, not a Wright-side paraphrase. +fn parse_workshop( + text: &str, + catalog: &Catalog, + locale_override: Option<&str>, + resolved: &ResolvedInput, +) -> Result> { + let locale = workshop_rs::detect::resolve_locale( + text, + catalog, + locale_override.map(Locale::new).as_ref(), + ) + .map_err(|error| vec![workshop_diag(error, resolved)])?; + let program = workshop_rs::parser::parse_with_context(text, catalog, &locale, catalog) + .map_err(|error| vec![workshop_diag(error, resolved)])?; + program + .validate() + .map_err(|error| vec![workshop_diag(error, resolved)])?; + Ok(program) +} + +/// Apply validated previews to the filesystem (#434): every file's current +/// bytes must still carry the identity the transaction was validated +/// against — a stale source refuses with no writes — and each file is +/// replaced atomically through a sibling temporary file. +pub fn write_previews( + previews: &[SourcePreview], + transaction: &EditTransaction, +) -> Result, Diagnostic> { + let mut expected: BTreeMap<&str, &str> = BTreeMap::new(); + for edit in &transaction.edits { + expected.insert(edit.source.as_str(), edit.source_identity.as_str()); + } + for preview in previews { + let path = Path::new(&preview.source); + let bytes = std::fs::read(path).map_err(|error| { + Diagnostic::error( + "input-io", + Stage::Discovery, + format!("cannot read '{}': {error}", preview.source), + ) + })?; + let current = String::from_utf8_lossy(&bytes); + let expected_identity = expected + .get(preview.source.as_str()) + .expect("previews only cover edited sources"); + if crate::input_identity(¤t) != *expected_identity { + return Err(Diagnostic::error( + "edit-stale-source", + Stage::Discovery, + format!( + "'{}' changed since the transaction was validated; re-validate and retry", + preview.source + ), + )); + } + } + let mut written = Vec::new(); + for preview in previews { + let path = Path::new(&preview.source); + let temporary = temporary_path(path); + std::fs::write(&temporary, &preview.new_text).map_err(|error| { + Diagnostic::error( + "output-io", + Stage::Emission, + format!("cannot write '{}': {error}", temporary.display()), + ) + })?; + std::fs::rename(&temporary, path).map_err(|error| { + let _ = std::fs::remove_file(&temporary); + Diagnostic::error( + "output-io", + Stage::Emission, + format!("cannot replace '{}': {error}", preview.source), + ) + })?; + written.push(preview.source.clone()); + } + Ok(written) +} + +/// A sibling temporary file next to `path` — same directory, so the final +/// rename is atomic on the same filesystem. +fn temporary_path(path: &Path) -> PathBuf { + let mut name = path + .file_name() + .map(|name| name.to_os_string()) + .unwrap_or_default(); + name.push(format!(".wright-{}.tmp", std::process::id())); + path.with_file_name(name) } pub(crate) fn source_precondition( @@ -409,6 +923,10 @@ pub struct RenameRequest { pub source_identity: String, } +/// Build a whole-file `rename` edit that rewrites every word-boundary +/// occurrence of a name in `source` (#129). This is a proposal helper for +/// callers that only have source text — the raw Workshop `semanticRename` +/// path never uses it; it rewrites exact identifier spans instead (#434). pub fn rename_symbol(source: &str, request: &RenameRequest) -> Result { if request.from.is_empty() || request.to.is_empty() { return Err(Diagnostic::error( @@ -543,16 +1061,11 @@ pub fn range_as_span(range: &EditRange) -> SourceSpan { #[cfg(test)] mod tests { use super::*; + use std::sync::atomic::{AtomicUsize, Ordering}; - const SOURCE: &str = "globalvar score = 0\n\nrule \"r\":\n @Event global\n score += 1\n"; + static COUNTER: AtomicUsize = AtomicUsize::new(0); - fn rename(edit: SourceEdit, sources: &BTreeMap) -> EditValidation { - let config = SessionConfig { - input: crate::InputSpec::Path("program.opy".into()), - ..SessionConfig::default() - }; - validate_transaction(&config, sources, &EditTransaction::new(vec![edit]).unwrap()) - } + const SOURCE: &str = "globalvar score = 0\n\nrule \"r\":\n @Event global\n score += 1\n"; fn rename_edit(source: &str, from: &str, to: &str) -> SourceEdit { rename_symbol( @@ -568,6 +1081,35 @@ mod tests { .unwrap() } + fn catalog() -> Catalog { + Catalog::builtin().expect("builtin catalog") + } + + fn temp_workshop(text: &str) -> (PathBuf, PathBuf) { + let dir = std::env::temp_dir().join(format!( + "wright-edit-{}-{}", + std::process::id(), + COUNTER.fetch_add(1, Ordering::SeqCst) + )); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("input.ws"); + std::fs::write(&path, text).unwrap(); + (dir, path) + } + + const WORKSHOP: &str = "variables {\n global:\n 0: score\n}\n\nrule (\"r\") {\n event {\n Ongoing - Global;\n }\n actions {\n Set Global Variable(score, Add(Global.score, 1));\n }\n}\n"; + + fn workshop_session(text: &str) -> (PathBuf, crate::CompilerSession) { + let (dir, path) = temp_workshop(text); + let session = crate::CompilerSession::new(SessionConfig { + input: crate::InputSpec::Path(path), + kind: SourceKind::Workshop, + ..SessionConfig::default() + }) + .expect("session"); + (dir, session) + } + #[test] fn rename_rewrites_declaration_and_references() { let edit = rename_edit(SOURCE, "score", "total"); @@ -611,9 +1153,10 @@ mod tests { #[test] fn stale_source_identity_is_rejected() { + let (dir, path) = temp_workshop(WORKSHOP); let edit = SourceEdit { edit_kind: "rename".to_string(), - source: "program.opy".to_string(), + source: path.to_string_lossy().into_owned(), source_identity: "wrong-identity".to_string(), range: EditRange { start_line: 1, @@ -623,21 +1166,28 @@ mod tests { }, new_text: String::new(), }; - let sources = BTreeMap::from([("program.opy".to_string(), SOURCE.to_string())]); + let sources = BTreeMap::from([(path.to_string_lossy().into_owned(), WORKSHOP.to_string())]); let validation = validate_transaction( - &SessionConfig::default(), + &SessionConfig { + input: crate::InputSpec::Path(path), + kind: SourceKind::Workshop, + ..SessionConfig::default() + }, + &catalog(), &sources, &EditTransaction::new(vec![edit]).unwrap(), ); assert!(!validation.ok); assert_eq!(validation.diagnostics[0].code, "edit-stale-source"); + let _ = std::fs::remove_dir_all(dir); } #[test] fn stale_source_identity_is_rejected_before_any_validation() { + let (dir, path) = temp_workshop(WORKSHOP); let edit = SourceEdit { edit_kind: "rename".to_string(), - source: "other.opy".to_string(), + source: path.to_string_lossy().into_owned(), source_identity: crate::input_identity(SOURCE), range: EditRange { start_line: 1, @@ -650,13 +1200,19 @@ mod tests { // The current text is missing entirely: the precondition refuses // before any range or compile work. let validation = validate_transaction( - &SessionConfig::default(), + &SessionConfig { + input: crate::InputSpec::Path(path), + kind: SourceKind::Workshop, + ..SessionConfig::default() + }, + &catalog(), &BTreeMap::new(), &EditTransaction::new(vec![edit]).unwrap(), ); assert!(!validation.ok); assert_eq!(validation.diagnostics[0].code, "edit-unknown-source"); assert!(validation.preview.is_none(), "no partial preview"); + let _ = std::fs::remove_dir_all(dir); } #[test] @@ -709,46 +1265,454 @@ mod tests { } #[test] - fn rename_validation_refuses_without_provider() { - let sources = BTreeMap::from([("program.opy".to_string(), SOURCE.to_string())]); - let validation = rename(rename_edit(SOURCE, "score", "total"), &sources); + fn workshop_edits_validate_by_reparsing_the_source() { + let (dir, mut session) = workshop_session(WORKSHOP); + let loaded = session.load().expect("loaded"); + // One identifier edit: `score` → `total` in the declaration. + let edit = SourceEdit { + edit_kind: "edit".to_string(), + source: loaded.input.display.clone(), + source_identity: crate::input_identity(&loaded.input.text), + range: EditRange { + start_line: 3, + start_col: 12, + end_line: 3, + end_col: 17, + }, + new_text: "total".to_string(), + }; + let sources = BTreeMap::from([(loaded.input.display.clone(), loaded.input.text.clone())]); + let validation = validate_transaction( + &session.config, + session.catalog(), + &sources, + &EditTransaction::new(vec![edit]).unwrap(), + ); + assert!(validation.ok, "{:?}", validation.diagnostics); + let preview = validation.preview.expect("preview"); + assert!(preview[0].new_text.contains("0: total")); + let _ = std::fs::remove_dir_all(dir); + } + + #[test] + fn workshop_edits_refuse_with_the_reparse_diagnostics() { + // Renaming the declaration to a name that breaks reparse — `0)` — + // refuses with the Workshop parse diagnostic, not a provider error. + let (dir, mut session) = workshop_session(WORKSHOP); + let loaded = session.load().expect("loaded"); + let edit = SourceEdit { + edit_kind: "edit".to_string(), + source: loaded.input.display.clone(), + source_identity: crate::input_identity(&loaded.input.text), + range: EditRange { + start_line: 3, + start_col: 12, + end_line: 3, + end_col: 17, + }, + new_text: "0)".to_string(), + }; + let sources = BTreeMap::from([(loaded.input.display.clone(), loaded.input.text.clone())]); + let validation = validate_transaction( + &session.config, + session.catalog(), + &sources, + &EditTransaction::new(vec![edit]).unwrap(), + ); assert!(!validation.ok); + assert_eq!(validation.diagnostics[0].code, "parse-error"); + assert!(validation.preview.is_none(), "no partial preview"); + let _ = std::fs::remove_dir_all(dir); + } + + #[test] + fn semantic_rename_rewrites_only_identifier_spans() { + let (dir, mut session) = workshop_session(WORKSHOP); + let loaded = session.load().expect("loaded"); + let sources = BTreeMap::from([(loaded.input.display.clone(), loaded.input.text.clone())]); + let rename = semantic_rename( + &loaded, + session.catalog(), + &sources, + &RenameTarget { + symbol: Some(Address::Name("score".to_string())), + source: None, + line: None, + col: None, + to: "total".to_string(), + }, + ); + assert!(rename.ok, "{:?}", rename.diagnostics); + let transaction = rename.transaction.expect("transaction"); + // Declaration + `Set Global Variable` arg + `Global.score` read. + assert_eq!(transaction.edits.len(), 3); + for edit in &transaction.edits { + assert_eq!(edit.edit_kind, "rename"); + assert_eq!(edit.new_text, "total"); + // The edited range covers exactly the old identifier. + let lines: Vec<&str> = loaded.input.text.split('\n').collect(); + let line = lines[edit.range.start_line as usize - 1]; + assert_eq!(edit.range.start_line, edit.range.end_line); + assert_eq!( + &line[edit.range.start_col as usize - 1..edit.range.end_col as usize - 1], + "score" + ); + } + let preview = rename.preview.expect("preview"); + assert!(preview[0].new_text.contains("0: total")); + assert!(preview[0].new_text.contains("Set Global Variable(total,")); + assert!(preview[0].new_text.contains("Add(Global.total, 1)")); + let _ = std::fs::remove_dir_all(dir); + } + + #[test] + fn semantic_rename_by_position_and_stale_sources_refuse() { + let (dir, mut session) = workshop_session(WORKSHOP); + let loaded = session.load().expect("loaded"); + let sources = BTreeMap::from([(loaded.input.display.clone(), loaded.input.text.clone())]); + // Position inside the `score` occurrence of `Set Global Variable`. + let by_position = semantic_rename( + &loaded, + session.catalog(), + &sources, + &RenameTarget { + symbol: None, + source: Some(loaded.input.display.clone()), + line: Some(11), + col: Some(30), + to: "total".to_string(), + }, + ); + assert!(by_position.ok, "{:?}", by_position.diagnostics); + + // A stale current text refuses: provenance spans must index the + // loaded program's text. + let stale = BTreeMap::from([( + loaded.input.display.clone(), + loaded.input.text.replacen("score", "other", 1), + )]); + let rename = semantic_rename( + &loaded, + session.catalog(), + &stale, + &RenameTarget { + symbol: Some(Address::Name("score".to_string())), + source: None, + line: None, + col: None, + to: "total".to_string(), + }, + ); + assert!(!rename.ok); + assert_eq!(rename.diagnostics[0].code, "edit-stale-source"); + + // Unknown and missing targets refuse explicitly. + for (target, code) in [ + ( + RenameTarget { + symbol: Some(Address::Name("missing".to_string())), + source: None, + line: None, + col: None, + to: "x".to_string(), + }, + "unknown-symbol", + ), + ( + RenameTarget { + symbol: None, + source: Some(loaded.input.display.clone()), + line: Some(1), + col: Some(1), + to: "x".to_string(), + }, + "rename-invalid-target", + ), + ] { + let rename = semantic_rename(&loaded, session.catalog(), &sources, &target); + assert!(!rename.ok); + assert_eq!(rename.diagnostics[0].code, code, "{target:?}"); + } + let _ = std::fs::remove_dir_all(dir); + } + + #[test] + fn semantic_rename_refuses_collisions_and_unsupported_kinds() { + let (dir, mut session) = workshop_session(WORKSHOP); + let loaded = session.load().expect("loaded"); + let sources = BTreeMap::from([(loaded.input.display.clone(), loaded.input.text.clone())]); + // `to` names another global variable: a same-namespace collision. + let collision = semantic_rename( + &loaded, + session.catalog(), + &sources, + &RenameTarget { + symbol: Some(Address::Name("score".to_string())), + source: None, + line: None, + col: None, + to: "score2".to_string(), + }, + ); + assert!(collision.ok); + // Add a second variable and collide with it. + let (dir2, mut session2) = + workshop_session(&WORKSHOP.replacen("0: score", "0: score\n 1: other", 1)); + let loaded2 = session2.load().expect("loaded"); + let sources2 = + BTreeMap::from([(loaded2.input.display.clone(), loaded2.input.text.clone())]); + let collision = semantic_rename( + &loaded2, + session2.catalog(), + &sources2, + &RenameTarget { + symbol: Some(Address::Name("score".to_string())), + source: None, + line: None, + col: None, + to: "other".to_string(), + }, + ); + assert!(!collision.ok); + assert_eq!(collision.diagnostics[0].code, "rename-name-collision"); + // A rule symbol is not a rename target. + let rule = semantic_rename( + &loaded, + session.catalog(), + &sources, + &RenameTarget { + symbol: Some(Address::Name("r".to_string())), + source: None, + line: None, + col: None, + to: "renamed".to_string(), + }, + ); + assert!(!rule.ok); + assert_eq!(rule.diagnostics[0].code, "rename-unsupported-kind"); + let _ = std::fs::remove_dir_all(dir); + let _ = std::fs::remove_dir_all(dir2); + } + + #[test] + fn the_rename_invariant_catches_edits_that_drop_a_reference() { + // Ablation: a transaction missing one occurrence still fails the + // post-reparse reference-shape check rather than "succeeding". + let (dir, mut session) = workshop_session(WORKSHOP); + let loaded = session.load().expect("loaded"); + let index = SemanticIndex::build(&loaded.program); + let symbol = index + .symbols() + .find(|symbol| symbol.name == "score") + .expect("score symbol"); + // Edit only the declaration, leaving the two references stale. + let edited = loaded.input.text.replacen("0: score", "0: total", 1); + let result = verify_rename(&index, symbol, "total", &edited, &loaded, session.catalog()); + let diagnostics = result.expect_err("a partial rename refuses"); + assert_eq!(diagnostics[0].code, "rename-mismatch"); + let _ = std::fs::remove_dir_all(dir); + } + + #[test] + fn semantic_rename_covers_player_variables_and_subroutines() { + // #434 acceptance: player variables rewrite their declaration plus + // `Event Player.name` occurrences (prefix preserved), and + // subroutines rewrite the declaration, the event binding, and calls. + let source = "variables {\n player:\n 1: streak\n}\n\nsubroutines {\n 0: helper\n}\n\nrule (\"r\") {\n event {\n Ongoing - Each Player;\n All;\n All;\n }\n actions {\n Set Player Variable(Event Player, streak, Add(Event Player.streak, 1));\n Call Subroutine(helper);\n }\n}\n\nrule (\"sub\") {\n event {\n Subroutine;\n helper;\n }\n actions {\n Set Player Variable(Event Player, streak, 0);\n }\n}\n"; + let (dir, mut session) = workshop_session(source); + let loaded = session.load().expect("loaded"); + let sources = BTreeMap::from([(loaded.input.display.clone(), loaded.input.text.clone())]); + + let player = semantic_rename( + &loaded, + session.catalog(), + &sources, + &RenameTarget { + symbol: Some(Address::Name("streak".to_string())), + source: None, + line: None, + col: None, + to: "combo".to_string(), + }, + ); + assert!(player.ok, "{:?}", player.diagnostics); + let text = &player.preview.as_ref().unwrap()[0].new_text; + assert!(text.contains("1: combo"), "{text}"); + assert!(text.contains("Event Player, combo,"), "{text}"); + assert!(text.contains("Event Player.combo"), "{text}"); + assert!(!text.contains("streak"), "{text}"); + // Only `combo` identifiers rewrote: `helper` is untouched. + assert!(text.contains("Call Subroutine(helper)"), "{text}"); + + let subroutine = semantic_rename( + &loaded, + session.catalog(), + &sources, + &RenameTarget { + symbol: Some(Address::Name("helper".to_string())), + source: None, + line: None, + col: None, + to: "assist".to_string(), + }, + ); + assert!(subroutine.ok, "{:?}", subroutine.diagnostics); + let text = &subroutine.preview.as_ref().unwrap()[0].new_text; + assert!(text.contains("0: assist"), "{text}"); + assert!(text.contains("Call Subroutine(assist)"), "{text}"); + assert!( + text.contains("Subroutine;\n assist;"), + "the event binding rewrites: {text}" + ); + assert!(!text.contains("helper"), "{text}"); + + // Position addressing inside the `Subroutine; helper;` binding. + let helper_line = source + .lines() + .enumerate() + .find(|(_, line)| line.trim() == "helper;") + .map(|(index, _)| index as u32 + 1) + .expect("event binding"); + let by_position = semantic_rename( + &loaded, + session.catalog(), + &sources, + &RenameTarget { + symbol: None, + source: Some(loaded.input.display.clone()), + line: Some(helper_line), + col: Some(9), + to: "assist".to_string(), + }, + ); + assert!(by_position.ok, "{:?}", by_position.diagnostics); assert_eq!( - validation.diagnostics[0].code, - "source-provider-unavailable" + by_position.transaction.unwrap().edits.len(), + subroutine.transaction.unwrap().edits.len(), + "position addressing resolves the same symbol" ); - assert!(validation.preview.is_none(), "no partial preview"); + let _ = std::fs::remove_dir_all(dir); } #[test] - fn broken_rename_is_refused_with_no_partial_preview() { - let source = "globalvar score = 0\n\nrule \"r\":\n @Event global\n score += 1\n missing(;\n"; - let edit = rename_symbol( - source, - &RenameRequest { - symbol_kind: "globalVariable".to_string(), - from: "score".to_string(), + fn write_previews_refuses_stale_sources_and_updates_atomically() { + // #434: `write_previews` rechecks each file's identity before + // writing; a file that changed since validation refuses with no + // write at all. + let (dir, mut session) = workshop_session(WORKSHOP); + let loaded = session.load().expect("loaded"); + let sources = BTreeMap::from([(loaded.input.display.clone(), loaded.input.text.clone())]); + let rename = semantic_rename( + &loaded, + session.catalog(), + &sources, + &RenameTarget { + symbol: Some(Address::Name("score".to_string())), + source: None, + line: None, + col: None, to: "total".to_string(), - source: "program.opy".to_string(), - source_identity: crate::input_identity(source), }, - ) - .unwrap(); - let sources = BTreeMap::from([("program.opy".to_string(), source.to_string())]); - let validation = validate_transaction( - &SessionConfig { - input: crate::InputSpec::Path("program.opy".into()), - ..SessionConfig::default() + ); + assert!(rename.ok, "{:?}", rename.diagnostics); + let transaction = rename.transaction.as_ref().unwrap(); + let previews = rename.preview.as_ref().unwrap(); + + // Stale: the file changed since the transaction was validated. + std::fs::write(loaded.input.path.as_ref().unwrap(), "// changed\n").unwrap(); + let error = write_previews(previews, transaction).unwrap_err(); + assert_eq!(error.code, "edit-stale-source"); + assert_eq!( + std::fs::read_to_string(loaded.input.path.as_ref().unwrap()).unwrap(), + "// changed\n", + "a stale write leaves the file untouched" + ); + + // Fresh: the file's identity matches and the write lands. + std::fs::write(loaded.input.path.as_ref().unwrap(), &loaded.input.text).unwrap(); + let written = write_previews(previews, transaction).expect("writes"); + assert_eq!(written.len(), 1); + let text = std::fs::read_to_string(loaded.input.path.as_ref().unwrap()).unwrap(); + assert!(text.contains("0: total"), "{text}"); + assert!(text.contains("Global.total"), "{text}"); + // No temporary sibling remains. + let leftovers: Vec<_> = std::fs::read_dir(&dir) + .unwrap() + .filter_map(|entry| entry.ok()) + .filter(|entry| entry.file_name().to_string_lossy().contains(".wright-")) + .collect(); + assert!( + leftovers.is_empty(), + "no temporary file leaks: {leftovers:?}" + ); + let _ = std::fs::remove_dir_all(dir); + } + + #[test] + fn opy_input_routes_to_the_provider_operation() { + let dir = std::env::temp_dir().join(format!( + "wright-edit-opy-{}-{}", + std::process::id(), + COUNTER.fetch_add(1, Ordering::SeqCst) + )); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("input.opy"); + std::fs::write(&path, "rule \"r\":\n pass\n").unwrap(); + let mut session = crate::CompilerSession::new(SessionConfig { + input: crate::InputSpec::Path(path.clone()), + kind: SourceKind::Opy, + ..SessionConfig::default() + }) + .expect("session"); + let sources = BTreeMap::from([( + path.to_string_lossy().into_owned(), + "rule \"r\":\n pass\n".to_string(), + )]); + let edit = SourceEdit { + edit_kind: "edit".to_string(), + source: path.to_string_lossy().into_owned(), + source_identity: crate::input_identity("rule \"r\":\n pass\n"), + range: EditRange { + start_line: 1, + start_col: 1, + end_line: 1, + end_col: 2, }, + new_text: "x".to_string(), + }; + let validation = validate_transaction( + &session.config, + session.catalog(), &sources, &EditTransaction::new(vec![edit]).unwrap(), ); - assert!(!validation.ok, "a rename that breaks the source refuses"); - assert_eq!( - validation.diagnostics[0].code, - "source-provider-unavailable" + assert!(!validation.ok); + assert_eq!(validation.diagnostics[0].code, "edit-requires-provider"); + assert!( + validation.diagnostics[0] + .message + .contains("providerValidateEdit"), + "the refusal names the provider operation" ); - assert!(validation.preview.is_none(), "no partial preview"); + let rename = session.semantic_rename( + &sources, + &RenameTarget { + symbol: Some(Address::Name("x".to_string())), + source: None, + line: None, + col: None, + to: "y".to_string(), + }, + ); + assert!(!rename.ok); + assert_eq!(rename.diagnostics[0].code, "edit-requires-provider"); + assert!( + rename.diagnostics[0] + .message + .contains("providerSemanticRename"), + "the refusal names the provider operation" + ); + let _ = std::fs::remove_dir_all(dir); } #[test] diff --git a/crates/wright-driver/src/service.rs b/crates/wright-driver/src/service.rs index 180c77a0..44683c88 100644 --- a/crates/wright-driver/src/service.rs +++ b/crates/wright-driver/src/service.rs @@ -116,6 +116,9 @@ pub enum ToolRequest { /// Validate and preview a caller-supplied source-edit transaction /// against the session's project (#130): atomic all-or-nothing /// semantics, structured refusal diagnostics, no filesystem writes. + /// Raw Workshop input validates by reparsing the edited sources through + /// `workshop-rs` (#434); source languages route to + /// `providerValidateEdit`. #[serde(rename = "validateEditTransaction")] ValidateEdit { /// The current text of every source the transaction touches, keyed @@ -124,9 +127,14 @@ pub enum ToolRequest { transaction: crate::edit::EditTransaction, }, /// Request a semantic rename through the shared refactoring contract - /// (#129/#130): returns the validated exact-range transaction or - /// structured refusal diagnostics. Wright proposes/validates; applying - /// edits to disk is an explicit consumer responsibility. + /// (#129/#130/#434): returns the validated exact-range transaction or + /// structured refusal diagnostics. On raw Workshop input the target is + /// a `symbol` (numeric id or declared name, as `references`/`usage` + /// address them) or a `source`/`line`/`col` position, and occurrences + /// rewrite through `workshop-rs` identifier provenance. Source + /// languages route to `providerSemanticRename`. Wright + /// proposes/validates; applying edits to disk is an explicit consumer + /// responsibility. SemanticRename { /// The current text of every source the rename may edit, keyed by /// the same source identities the target names. @@ -329,16 +337,14 @@ impl<'a> ToolService<'a> { ToolRequest::ValidateEdit { sources, transaction, - } => self.ok(serde_json::to_value(crate::edit::validate_transaction( - &self.session.config, - sources, - transaction, - )) - .expect("serializes")), - ToolRequest::SemanticRename { sources, target } => self.ok(serde_json::to_value( - crate::edit::semantic_rename(&self.session.config, sources, target), + } => self.ok(serde_json::to_value( + self.session.validate_edit_transaction(sources, transaction), ) .expect("serializes")), + ToolRequest::SemanticRename { sources, target } => { + let rename = self.session.semantic_rename(sources, target); + self.ok(serde_json::to_value(rename).expect("serializes")) + } ToolRequest::ProviderSemanticRename { language_id, documents, diff --git a/crates/wright-driver/src/session.rs b/crates/wright-driver/src/session.rs index b7c0ad2c..1f8a234f 100644 --- a/crates/wright-driver/src/session.rs +++ b/crates/wright-driver/src/session.rs @@ -3,6 +3,7 @@ //! without changing callers. Every workflow returns a typed [`Envelope`] //! whose JSON serialization is the machine-readable CLI contract. +mod edit; mod semantic; pub(crate) use semantic::resolve_span_paths; diff --git a/crates/wright-driver/src/session/edit.rs b/crates/wright-driver/src/session/edit.rs new file mode 100644 index 00000000..1ca71519 --- /dev/null +++ b/crates/wright-driver/src/session/edit.rs @@ -0,0 +1,144 @@ +//! Validated source-edit operations on the session (#434): +//! `validateEditTransaction` and `semanticRename` run on raw Workshop +//! input through `workshop-rs` reparse and identifier provenance; other +//! source languages stay at the provider boundary. `rename` is the +//! `wright rename` CLI workflow — a validated diff by default, an atomic +//! filesystem update with `--write`. + +use std::collections::BTreeMap; + +use super::CompilerSession; +use crate::config::InputSpec; +use crate::diag::{Diagnostic, Stage}; +use crate::edit::{EditTransaction, EditValidation, RenameResult, RenameTarget, SemanticRename}; +use crate::input; +use crate::result::Envelope; +use crate::service::Address; + +impl CompilerSession { + /// `validateEditTransaction` (#434): apply `transaction` to the supplied + /// current sources and, on raw Workshop input, reparse and validate the + /// edited result through `workshop-rs`. OPY and other source languages + /// refuse with `edit-requires-provider` naming `providerValidateEdit`. + pub fn validate_edit_transaction( + &self, + sources: &BTreeMap, + transaction: &EditTransaction, + ) -> EditValidation { + crate::edit::validate_transaction(&self.config, &self.catalog, sources, transaction) + } + + /// `semanticRename` (#434): resolve `target` against the loaded + /// program's semantic index and rewrite every identifier occurrence + /// through `workshop-rs` provenance — never a textual search. OPY and + /// other source languages refuse with `edit-requires-provider` naming + /// `providerSemanticRename`, whether or not a provider is configured. + pub fn semantic_rename( + &mut self, + sources: &BTreeMap, + target: &RenameTarget, + ) -> SemanticRename { + let refuse = |diagnostics: Vec| SemanticRename { + ok: false, + transaction: None, + diagnostics, + preview: None, + }; + match &self.config.input { + InputSpec::Stdin => { + return refuse(vec![Diagnostic::error( + "edit-input-stdin", + Stage::Discovery, + "edit operations require a path-based input; stdin has no source identity", + )]); + } + InputSpec::Path(_) => match input::resolve(&self.config) { + Ok(resolved) => { + if let Some(diagnostic) = + crate::edit::edit_kind_gate(resolved.kind, "providerSemanticRename") + { + return refuse(vec![diagnostic]); + } + } + Err(diagnostic) => return refuse(vec![diagnostic]), + }, + } + match self.load() { + Ok(loaded) => crate::edit::semantic_rename(&loaded, &self.catalog, sources, target), + Err(diagnostic) => refuse(vec![diagnostic]), + } + } + + /// `wright rename [INPUT]` (#434): the validated semantic + /// rename of a Workshop variable or subroutine — the transaction and a + /// source diff by default, an atomic filesystem update with `write`. + /// A stale input or an invalid rename refuses with structured + /// diagnostics and no partial write. + pub fn rename(&mut self, name: &str, to: &str, write: bool) -> Envelope { + let mut result = RenameResult::default(); + match &self.config.input { + InputSpec::Stdin => { + self.diagnostics.push(Diagnostic::error( + "edit-input-stdin", + Stage::Discovery, + "rename requires a path-based input; stdin has no source identity", + )); + return self.finish("rename", result); + } + InputSpec::Path(_) => match input::resolve(&self.config) { + Ok(resolved) => { + if let Some(diagnostic) = + crate::edit::edit_kind_gate(resolved.kind, "providerSemanticRename") + { + self.diagnostics.push(diagnostic); + return self.finish("rename", result); + } + } + Err(diagnostic) => { + self.diagnostics.push(diagnostic); + return self.finish("rename", result); + } + }, + } + let loaded = match self.load() { + Ok(loaded) => loaded, + Err(diagnostic) => { + self.diagnostics.push(diagnostic); + return self.finish("rename", result); + } + }; + let sources = BTreeMap::from([(loaded.input.display.clone(), loaded.input.text.clone())]); + let rename = crate::edit::semantic_rename( + &loaded, + &self.catalog, + &sources, + &RenameTarget { + symbol: Some(Address::Name(name.to_string())), + source: None, + line: None, + col: None, + to: to.to_string(), + }, + ); + self.diagnostics.extend(rename.diagnostics); + result.transaction = rename.transaction; + result.preview = rename.preview; + result.originals = sources; + if !rename.ok { + return self.finish("rename", result); + } + if write { + match crate::edit::write_previews( + result.preview.as_deref().unwrap_or_default(), + result + .transaction + .as_ref() + .expect("a successful rename carries its transaction"), + ) { + Ok(written) => result.written = written, + Err(diagnostic) => self.diagnostics.push(diagnostic), + } + } + self.finish("rename", result) + } +} diff --git a/crates/wright-driver/tests/edit.rs b/crates/wright-driver/tests/edit.rs index 88b6eb16..d4e89516 100644 --- a/crates/wright-driver/tests/edit.rs +++ b/crates/wright-driver/tests/edit.rs @@ -1,10 +1,12 @@ -//! Source edits remain Wright-owned data contracts, while semantic validation -//! is now delegated to provider capabilities instead of a static OPY frontend. +//! Source edits remain Wright-owned data contracts. Raw Workshop +//! transactions validate through `workshop-rs` reparse (#434); source +//! languages route to the provider operations instead of a static frontend. use std::collections::BTreeMap; +use std::sync::atomic::{AtomicUsize, Ordering}; use wright_driver::edit::{EditRange, EditTransaction, SourceEdit, validate_transaction}; -use wright_driver::{InputSpec, SessionConfig}; +use wright_driver::{InputSpec, SessionConfig, SourceKind}; fn edit(source: &str, range: EditRange) -> SourceEdit { SourceEdit { @@ -16,6 +18,19 @@ fn edit(source: &str, range: EditRange) -> SourceEdit { } } +fn temp_source(name: &str, text: &str) -> (std::path::PathBuf, std::path::PathBuf) { + static COUNTER: AtomicUsize = AtomicUsize::new(0); + let dir = std::env::temp_dir().join(format!( + "wright-edit-it-{}-{}", + std::process::id(), + COUNTER.fetch_add(1, Ordering::SeqCst) + )); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join(name); + std::fs::write(&path, text).unwrap(); + (dir, path) +} + #[test] fn transaction_rejects_invalid_ranges_before_provider_validation() { let source = "globalvar score = 0\n"; @@ -35,30 +50,78 @@ fn transaction_rejects_invalid_ranges_before_provider_validation() { } #[test] -fn opy_validation_refuses_without_a_provider_capability() { +fn opy_validation_routes_to_the_provider_operation() { let source = "globalvar score = 0\n"; - let transaction = EditTransaction::new(vec![edit( - source, - EditRange { - start_line: 1, - start_col: 1, - end_line: 1, - end_col: 6, - }, - )]) + let (dir, path) = temp_source("program.opy", source); + let transaction = EditTransaction::new(vec![SourceEdit { + source: path.to_string_lossy().into_owned(), + ..edit( + source, + EditRange { + start_line: 1, + start_col: 1, + end_line: 1, + end_col: 6, + }, + ) + }]) .expect("transaction is structurally valid"); - let sources = BTreeMap::from([("program.opy".to_string(), source.to_string())]); + let sources = BTreeMap::from([(path.to_string_lossy().into_owned(), source.to_string())]); let result = validate_transaction( &SessionConfig { - input: InputSpec::Path("program.opy".into()), + input: InputSpec::Path(path), + kind: SourceKind::Opy, ..SessionConfig::default() }, + &workshop_rs::catalog::Catalog::builtin().expect("catalog"), &sources, &transaction, ); assert!(!result.ok); - assert_eq!(result.diagnostics[0].code, "source-provider-unavailable"); + assert_eq!(result.diagnostics[0].code, "edit-requires-provider"); + assert!( + result.diagnostics[0] + .message + .contains("providerValidateEdit"), + "the refusal names the provider operation: {}", + result.diagnostics[0].message + ); assert!(result.preview.is_none()); + let _ = std::fs::remove_dir_all(dir); +} + +#[test] +fn workshop_validation_applies_and_reparses() { + let source = "variables {\n global:\n 0: score\n}\n\nrule (\"r\") {\n event {\n Ongoing - Global;\n }\n actions {\n Set Global Variable(score, 5);\n }\n}\n"; + let (dir, path) = temp_source("program.ws", source); + let transaction = EditTransaction::new(vec![SourceEdit { + edit_kind: "edit".to_string(), + source: path.to_string_lossy().into_owned(), + source_identity: wright_driver::input_identity(source), + range: EditRange { + start_line: 3, + start_col: 12, + end_line: 3, + end_col: 17, + }, + new_text: "total".to_string(), + }]) + .expect("transaction is structurally valid"); + let sources = BTreeMap::from([(path.to_string_lossy().into_owned(), source.to_string())]); + let result = validate_transaction( + &SessionConfig { + input: InputSpec::Path(path), + kind: SourceKind::Workshop, + ..SessionConfig::default() + }, + &workshop_rs::catalog::Catalog::builtin().expect("catalog"), + &sources, + &transaction, + ); + assert!(result.ok, "{:?}", result.diagnostics); + let preview = result.preview.expect("a valid transaction previews"); + assert!(preview[0].new_text.contains("0: total")); + let _ = std::fs::remove_dir_all(dir); } #[test] diff --git a/docs/agent-contract.md b/docs/agent-contract.md index b0859858..1b23f50b 100644 --- a/docs/agent-contract.md +++ b/docs/agent-contract.md @@ -144,6 +144,44 @@ proposes and validates edits; a caller remains responsible for applying them. Stale, overlapping, unsupported, or semantically invalid edits return an explicit refusal without a partial edit set. +### Validated edits and semantic rename for raw Workshop (#434) + +On raw Workshop input both operations are supported directly — `workshop-rs` +owns the source semantics and Wright orchestrates the transaction: + +* `validateEditTransaction` applies the caller's transaction to the supplied + current `sources` and reparses/revalidates the edited project through the + session's own `workshop-rs` path. A transaction that produces malformed or + invalid Workshop refuses with the real parse/validation diagnostics and no + partial preview; a valid one returns `ok: true` with a `preview` of each + edited source (edited text plus the post-edit identity). +* `semanticRename` resolves `target` either by `symbol` — a numeric id or a + declared name, exactly the addressing `references`/`usage` use — or by a + `source`/`line`/`col` position inside one identifier occurrence. Global + variables, player variables, and subroutines rename through the exact + identifier spans `workshop-rs` records: the declaration, the `Subroutine` + event binding, `Call Subroutine` callees, `Set`/`Modify`/`For` variable + arguments, and `Global.name`/`Event Player.name` value references all + rewrite, while prefixes, comments, strings, and unrelated identifiers stay + untouched. There is no textual-search fallback — an occurrence whose + recorded span does not cover exactly the identifier refuses with + `rename-unmapped-span`. + `sources` must carry the current text of the loaded input file; a text + that differs from the loaded program's refuses with `edit-stale-source` + because provenance spans would no longer index it. A `to` name that + already declares a same-kind symbol refuses with `rename-name-collision`, + and one that does not survive reparsing refuses with the parse diagnostic; + rule names are not rename targets (`rename-unsupported-kind`). +* Non-Workshop input stays at the provider boundary: OPY refuses with + `edit-requires-provider` naming `providerValidateEdit` / + `providerSemanticRename`, and kinds without a shipped provider keep their + `source-provider-unavailable` refusal. + +`wright rename [INPUT]` exposes the same semantic rename +on the CLI: it prints the validated diff by default and applies it +atomically with `--write`, refusing `edit-stale-source` when the file +changed since validation. + ## Results, diagnostics, and errors The service response is either `{ "result": value }` or diff --git a/docs/architecture/tooling.md b/docs/architecture/tooling.md index e1f65d9e..134158f3 100644 --- a/docs/architecture/tooling.md +++ b/docs/architecture/tooling.md @@ -49,6 +49,8 @@ re-check / re-analyze Normal source tooling does not require full-file regeneration. Preserve comments/trivia/formatting/unchanged structure where practical. Unsupported or unsafe edits fail explicitly; do not silently degrade to textual search/replace when semantic correctness is required. +For raw Workshop input this model runs through `workshop-rs` itself (#434): `validateEditTransaction` applies a caller's transaction to the current sources and reparses/revalidates the edited project, and `semanticRename` resolves a symbol- or position-addressed target against the loaded program's semantic index and rewrites exactly the identifier spans the parsed program records — never a textual search. `wright rename [INPUT]` exposes the same validated rename to users (a diff by default, an atomic write with `--write`). OPY and other source languages remain provider-owned edit surfaces: the raw operations refuse them by naming the `providerValidateEdit`/`providerSemanticRename` operations rather than guessing at source semantics Wright does not own. + ## Shared services CLI, LSP, agent/MCP-style adapters, embedding, and CI should reuse common Wright-owned semantic/query/edit services rather than each implementing language-specific logic independently. diff --git a/docs/cli/commands.md b/docs/cli/commands.md index 167ac0c8..4a2e3b24 100644 --- a/docs/cli/commands.md +++ b/docs/cli/commands.md @@ -41,6 +41,7 @@ result. | `wright inspect cfg [INPUT]` | Control-flow graph of one rule, addressed by name | block/edge listing | | `wright inspect callgraph [INPUT]` | Subroutine call graph (caller rules → callee subroutines) | call edges | | `wright inspect cost [INPUT]` | Exact generated-resource counts plus static findings | resource counts and findings | +| `wright rename [INPUT]` | Semantically rename a Workshop variable or subroutine | per-source diff of the validated edits; `--write` applies them | | `wright serve [INPUT]` | Serve `wright-agent/v1` over stdio or JSON-RPC 2.0 | one structured response per request | | `wright completion ` | Generate static completion script for bash, zsh, fish, or powershell | the generated completion script | | `wright completion install [SHELL]` | Install generated completion into standard user-local directory | installation progress and guidance | @@ -122,6 +123,35 @@ block detail for `cfg`, fan-in/fan-out highlights before the edge list for first page of ten entries followed by the withheld count; `--format json` always prints the complete result. +## `wright rename` — semantic rename for raw Workshop (#434) + +`wright rename [INPUT]` renames a global variable, player +variable, or subroutine in raw Workshop input. `NAME` is the declared symbol +name — the same addressing `inspect refs` uses — resolved against the loaded +program's semantic index; an unmatched name is `unknown-symbol` and a name +shared by several symbols is `ambiguous-symbol` (exit 1). + +The rename is semantic, not textual: every edit rewrites exactly the +identifier span `workshop-rs` records for one declaration or reference, so +`Global.score` rewrites `score` and leaves the `Global.` prefix, and comments +or string literals containing the name stay untouched. The proposed +transaction is validated by reparsing the edited source through the session's +own `workshop-rs` path; a name that does not survive reparsing, or one whose +references no longer bind to the renamed symbol, refuses with +`rename-mismatch` and no partial edit set. + +* Default is **preview only**: text mode prints the diff as `-`/`+` line + pairs, and JSON mode returns the validated `transaction` and per-source + `preview` inside the `wright-result/v1` envelope — callers (and agents via + `semanticRename`) carry the same atomic edit set. +* `--write` applies the validated transaction to the input file atomically + (a sibling temporary file and rename). The write rechecks each source's + identity hash first; a file that changed since validation refuses with + `edit-stale-source` and writes nothing. +* Other source kinds are provider surfaces: OPY input refuses with + `edit-requires-provider` naming `providerSemanticRename`, and kinds without + a shipped provider keep their `source-provider-unavailable` refusal. + ## `wright convert` and the reconstruction surface (#126) `wright convert [INPUT] --target opy|ostw` reconstructs **validated Workshop diff --git a/schemas/wright-agent-v1.schema.json b/schemas/wright-agent-v1.schema.json index c8117452..580da63c 100644 --- a/schemas/wright-agent-v1.schema.json +++ b/schemas/wright-agent-v1.schema.json @@ -671,11 +671,17 @@ }, "RenameTarget": { "type": "object", - "required": ["source", "line", "col", "to"], + "required": ["to"], "properties": { - "source": { "type": "string" }, - "line": { "type": "integer", "minimum": 0, "maximum": 4294967295 }, - "col": { "type": "integer", "minimum": 0, "maximum": 4294967295 }, + "symbol": { + "type": ["integer", "string"], + "minimum": 0, + "maximum": 4294967295, + "description": "Symbol addressing (#434): the numeric symbol id or the declared name, exactly as `references`/`usage` address symbols. When present, `source`/`line`/`col` are ignored." + }, + "source": { "type": "string", "description": "Position addressing: the source file the position is interpreted in; requires `line` and `col`." }, + "line": { "type": "integer", "minimum": 0, "maximum": 4294967295, "description": "Position addressing: the 1-based line of a character inside one identifier occurrence." }, + "col": { "type": "integer", "minimum": 0, "maximum": 4294967295, "description": "Position addressing: the 1-based column of a character inside one identifier occurrence." }, "to": { "type": "string" } }, "additionalProperties": false From 2da76207a42998af83b224de05d90cddf2710a1e Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Wed, 30 Sep 2026 20:58:03 +0800 Subject: [PATCH 2/4] fix(driver): enforce wire transaction invariants and index Start Rule callees Review on #454 found two contract violations reachable through the agent surface. Value::Subroutine arguments (Start Rule callees) produced no reference, so a uniquely-named subroutine refused on reparse and a redeclared name could silently retarget a surviving binding; the index now records the call reference and resolves redeclared names to the last declaration, matching workshop-rs name binding. A deserialized EditTransaction also bypassed the constructor's invariants, so empty, overlapping, or unsorted edit lists could produce corrupted ok:true previews; validateTransaction re-normalizes the wire value. Edits that name a source outside the loaded input and renames to the symbol's own name now refuse instead of validating vacuously. Refs #434 --- .../wright-analyzer/src/canonical/symbols.rs | 16 +- crates/wright-cli/src/present.rs | 6 +- crates/wright-driver/src/edit.rs | 246 ++++++++++++++++-- docs/agent-contract.md | 3 +- 4 files changed, 249 insertions(+), 22 deletions(-) diff --git a/crates/wright-analyzer/src/canonical/symbols.rs b/crates/wright-analyzer/src/canonical/symbols.rs index 1d4dd80b..43590094 100644 --- a/crates/wright-analyzer/src/canonical/symbols.rs +++ b/crates/wright-analyzer/src/canonical/symbols.rs @@ -374,10 +374,12 @@ impl SemanticIndex { }) })); } + /// Name resolution mirrors workshop-rs: a name that declares more than + /// once binds its references to the last declaration. fn find_symbol(&self, kind: SymbolKind, name: &str) -> Option { self.symbols .iter() - .find(|symbol| symbol.kind == kind && symbol.name == name) + .rfind(|symbol| symbol.kind == kind && symbol.name == name) .map(|s| s.id) } fn walk_event(&mut self, event: &Event, rule: RuleId, program: &Program) { @@ -514,6 +516,18 @@ impl SemanticIndex { ); } } + Value::Subroutine(name) => { + if let Some(symbol) = self.find_symbol(SymbolKind::Subroutine, name) { + self.push( + symbol, + ReferenceKind::Call, + Some(rule), + action, + Some(value_id), + span, + ); + } + } _ => {} } value_id diff --git a/crates/wright-cli/src/present.rs b/crates/wright-cli/src/present.rs index 8ddd009f..0c0aa1be 100644 --- a/crates/wright-cli/src/present.rs +++ b/crates/wright-cli/src/present.rs @@ -598,10 +598,8 @@ impl ResultPresentation for CheckResult { impl ResultPresentation for RenameResult { fn metadata(&self) -> Option { - let edits = self - .transaction - .as_ref() - .map_or(0, |transaction| transaction.edits.len()); + let transaction = self.transaction.as_ref()?; + let edits = transaction.edits.len(); let sources = self.preview.as_ref().map_or(0, Vec::len); Some(if self.written.is_empty() { format!("{edits} edit(s) across {sources} source(s); preview — pass --write to apply") diff --git a/crates/wright-driver/src/edit.rs b/crates/wright-driver/src/edit.rs index 7269a9fc..edf85478 100644 --- a/crates/wright-driver/src/edit.rs +++ b/crates/wright-driver/src/edit.rs @@ -18,7 +18,7 @@ use workshop_rs::catalog::{Catalog, Locale}; use wright_analyzer::canonical::{ReferenceKind, SemanticIndex, Symbol, SymbolId, SymbolKind}; use crate::config::{SessionConfig, SourceKind}; -use crate::diag::{Diagnostic, Position, SourceSpan, Stage, source_provider_unavailable}; +use crate::diag::{Diagnostic, Stage, source_provider_unavailable}; use crate::input::{self, ResolvedInput}; use crate::result::exit_code_from; use crate::service::Address; @@ -188,6 +188,31 @@ pub fn validate_transaction( if let Some(diagnostic) = edit_kind_gate(resolved.kind, "providerValidateEdit") { return refuse(vec![diagnostic]); } + // A transaction that arrived over the wire carries no structural + // guarantee: re-run the constructor's normalization and invariants so + // empty, overlapping, or unsorted edit lists meet the same contract as + // `EditTransaction::new` callers. + let transaction = match EditTransaction::new(transaction.edits.clone()) { + Ok(transaction) => transaction, + Err(diagnostic) => return refuse(vec![diagnostic]), + }; + // Every edit must target the loaded input itself — applying a caller's + // edit to a source that is not part of this program would validate + // text the transaction never touched. + if let Some(edit) = transaction + .edits + .iter() + .find(|edit| !same_source(&edit.source, &resolved)) + { + return refuse(vec![Diagnostic::error( + "edit-unknown-source", + Stage::Discovery, + format!( + "the edit targets '{}' which is not the loaded input '{}'", + edit.source, resolved.display + ), + )]); + } let mut diagnostics: Vec = transaction .edits .iter() @@ -404,6 +429,13 @@ pub fn semantic_rename( )]); } } + if target.to == symbol.name { + return refuse(vec![Diagnostic::error( + "rename-invalid-name", + Stage::Discovery, + "the new name already names this symbol", + )]); + } // Same-namespace collision: `to` must not already name another symbol of // the same kind — global and player variables are separate namespaces, // so the check is kind-scoped. @@ -1043,21 +1075,6 @@ fn rename_in_line(line: &str, from: &str, to: &str) -> String { out } -pub fn range_as_span(range: &EditRange) -> SourceSpan { - SourceSpan { - file: 0, - path: "".to_string(), - start: Position { - line: range.start_line, - col: range.start_col, - }, - end: Position { - line: range.end_line, - col: range.end_col, - }, - } -} - #[cfg(test)] mod tests { use super::*; @@ -1744,4 +1761,201 @@ mod tests { .collect(); assert_eq!(positions, vec![2, 3], "edits are ordered by position"); } + + #[test] + fn semantic_rename_rewrites_start_rule_callee() { + // A `Start Rule` subroutine argument is a value-position reference: + // its exact identifier span must rewrite with the rename. + let source = "subroutines {\n 0: helper\n}\n\nrule (\"sub\") {\n event {\n Subroutine;\n helper;\n }\n actions {\n Call Subroutine(helper);\n }\n}\n\nrule (\"r\") {\n event {\n Ongoing - Global;\n }\n actions {\n Start Rule(helper, Restart Rule);\n }\n}\n"; + let (dir, mut session) = workshop_session(source); + let loaded = session.load().expect("loaded"); + let sources = BTreeMap::from([(loaded.input.display.clone(), loaded.input.text.clone())]); + let rename = semantic_rename( + &loaded, + session.catalog(), + &sources, + &RenameTarget { + symbol: Some(Address::Name("helper".to_string())), + source: None, + line: None, + col: None, + to: "assist".to_string(), + }, + ); + assert!(rename.ok, "{:?}", rename.diagnostics); + let text = &rename.preview.as_ref().unwrap()[0].new_text; + assert!(text.contains("0: assist"), "{text}"); + assert!(text.contains("Subroutine;\n assist;"), "{text}"); + assert!(text.contains("Call Subroutine(assist)"), "{text}"); + assert!( + text.contains("Start Rule(assist, Restart Rule)"), + "the Start Rule callee rewrites: {text}" + ); + assert!(!text.contains("helper"), "{text}"); + let _ = std::fs::remove_dir_all(dir); + } + + #[test] + fn semantic_rename_binds_redeclared_names_to_the_last_declaration() { + // workshop-rs resolves a redeclared name to its last declaration: + // every use of `helper` binds #1. Renaming #1 rewrites the uses with + // it; renaming #0 touches only its own declaration and leaves the + // surviving `helper` bindings intact. + let source = "subroutines {\n 0: helper\n 1: helper\n}\n\nrule (\"sub\") {\n event {\n Subroutine;\n helper;\n }\n actions {\n Call Subroutine(helper);\n }\n}\n\nrule (\"r\") {\n event {\n Ongoing - Global;\n }\n actions {\n Start Rule(helper, Restart Rule);\n }\n}\n"; + let (dir, mut session) = workshop_session(source); + let loaded = session.load().expect("loaded"); + let sources = BTreeMap::from([(loaded.input.display.clone(), loaded.input.text.clone())]); + let target = |symbol: Address| RenameTarget { + symbol: Some(symbol), + source: None, + line: None, + col: None, + to: "assist".to_string(), + }; + + let last = semantic_rename( + &loaded, + session.catalog(), + &sources, + &target(Address::Id(1)), + ); + assert!(last.ok, "{:?}", last.diagnostics); + let text = &last.preview.as_ref().unwrap()[0].new_text; + assert!(text.contains("0: helper"), "{text}"); + assert!(text.contains("1: assist"), "{text}"); + assert!(text.contains("Subroutine;\n assist;"), "{text}"); + assert!(text.contains("Call Subroutine(assist)"), "{text}"); + assert!(text.contains("Start Rule(assist, Restart Rule)"), "{text}"); + assert_eq!(text.matches("helper").count(), 1, "{text}"); + + let first = semantic_rename( + &loaded, + session.catalog(), + &sources, + &target(Address::Id(0)), + ); + assert!(first.ok, "{:?}", first.diagnostics); + let text = &first.preview.as_ref().unwrap()[0].new_text; + assert!(text.contains("0: assist"), "{text}"); + assert_eq!( + text.matches("helper").count(), + 4, + "the surviving declaration keeps every binding: {text}" + ); + let _ = std::fs::remove_dir_all(dir); + } + + #[test] + fn wire_transactions_recheck_the_transaction_invariants() { + // A transaction deserialized off the wire bypasses + // `EditTransaction::new`; validation re-normalizes it so empty or + // overlapping edits refuse and unsorted edits still apply in source + // order rather than splicing blindly. + let (dir, session) = workshop_session(WORKSHOP); + let path = dir.join("input.ws"); + let key = path.to_string_lossy().into_owned(); + let sources = BTreeMap::from([(key.clone(), WORKSHOP.to_string())]); + let identity = crate::input_identity(WORKSHOP); + let wire = |edits: serde_json::Value| { + serde_json::from_value::(serde_json::json!({ "edits": edits })) + .expect("deserializes") + }; + let edit = |start_col: u32, end_col: u32, new_text: &str| { + serde_json::json!({ + "kind": "edit", + "source": key.clone(), + "source_identity": identity.clone(), + "range": { + "start_line": 11, + "start_col": start_col, + "end_line": 11, + "end_col": end_col, + }, + "new_text": new_text, + }) + }; + + let validation = validate_transaction( + &session.config, + session.catalog(), + &sources, + &wire(serde_json::json!([])), + ); + assert!(!validation.ok); + assert_eq!(validation.diagnostics[0].code, "edit-empty-transaction"); + + // Overlapping ranges on line 11 (`score` and `core,`) refuse. + let validation = validate_transaction( + &session.config, + session.catalog(), + &sources, + &wire(serde_json::json!([ + edit(29, 34, "total"), + edit(30, 35, "x") + ])), + ); + assert!(!validation.ok); + assert_eq!(validation.diagnostics[0].code, "edit-overlap"); + + // Unsorted disjoint edits normalize: `Add` (cols 36-39) listed before + // `score` (cols 29-34) still produces the correctly spliced source. + let validation = validate_transaction( + &session.config, + session.catalog(), + &sources, + &wire(serde_json::json!([ + edit(36, 39, "Subtract"), + edit(29, 34, "total") + ])), + ); + assert!(validation.ok, "{:?}", validation.diagnostics); + let text = &validation.preview.as_ref().unwrap()[0].new_text; + assert!( + text.contains("Set Global Variable(total, Subtract(Global.score, 1))"), + "{text}" + ); + + // An edit targeting a source that is not the loaded input refuses — + // validating it would check text the transaction never touched. + let mut wider = sources.clone(); + wider.insert("other.ws".to_string(), WORKSHOP.to_string()); + let foreign = serde_json::json!({ + "kind": "edit", + "source": "other.ws", + "source_identity": identity, + "range": {"start_line": 3, "start_col": 12, "end_line": 3, "end_col": 17}, + "new_text": "total", + }); + let validation = validate_transaction( + &session.config, + session.catalog(), + &wider, + &wire(serde_json::json!([foreign])), + ); + assert!(!validation.ok); + assert_eq!(validation.diagnostics[0].code, "edit-unknown-source"); + let _ = std::fs::remove_dir_all(dir); + } + + #[test] + fn semantic_rename_refuses_an_unchanged_name() { + let (dir, mut session) = workshop_session(WORKSHOP); + let loaded = session.load().expect("loaded"); + let sources = BTreeMap::from([(loaded.input.display.clone(), loaded.input.text.clone())]); + let rename = semantic_rename( + &loaded, + session.catalog(), + &sources, + &RenameTarget { + symbol: Some(Address::Name("score".to_string())), + source: None, + line: None, + col: None, + to: "score".to_string(), + }, + ); + assert!(!rename.ok); + assert_eq!(rename.diagnostics[0].code, "rename-invalid-name"); + let _ = std::fs::remove_dir_all(dir); + } } diff --git a/docs/agent-contract.md b/docs/agent-contract.md index 1b23f50b..d21281de 100644 --- a/docs/agent-contract.md +++ b/docs/agent-contract.md @@ -160,7 +160,8 @@ owns the source semantics and Wright orchestrates the transaction: `source`/`line`/`col` position inside one identifier occurrence. Global variables, player variables, and subroutines rename through the exact identifier spans `workshop-rs` records: the declaration, the `Subroutine` - event binding, `Call Subroutine` callees, `Set`/`Modify`/`For` variable + event binding, `Call Subroutine` callees, `Start Rule` subroutine + arguments, `Set`/`Modify`/`For` variable arguments, and `Global.name`/`Event Player.name` value references all rewrite, while prefixes, comments, strings, and unrelated identifiers stay untouched. There is no textual-search fallback — an occurrence whose From 51e37beb484f1bb226733f2d96378df8208aaec8 Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Wed, 30 Sep 2026 21:07:04 +0800 Subject: [PATCH 3/4] fix(analyzer): bind redeclared subroutine names to the last declaration in cfg edges action_subroutine first-matched a CallSubroutine callee name, pointing the CFG edge at a shadowed declaration when two subroutines share a name; workshop-rs resolves redeclared names last-wins, and the semantic index now does too. Refs #434 --- crates/wright-analyzer/src/canonical/cfg.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/wright-analyzer/src/canonical/cfg.rs b/crates/wright-analyzer/src/canonical/cfg.rs index fecf2265..ea011c27 100644 --- a/crates/wright-analyzer/src/canonical/cfg.rs +++ b/crates/wright-analyzer/src/canonical/cfg.rs @@ -252,8 +252,10 @@ fn action_subroutine(program: &Program, action: &Action) -> Option { Action::Call { name, .. } => name, _ => return None, }; + // Name resolution mirrors workshop-rs: a redeclared name binds to the + // last declaration. program .subroutines .iter() - .position(|subroutine| subroutine.name == *name) + .rposition(|subroutine| subroutine.name == *name) } From 5ed643e649e0af725b95d6d4ec904d6ce5777ab9 Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Wed, 30 Sep 2026 21:16:26 +0800 Subject: [PATCH 4/4] fix(cli): adapt rename wiring to the rebased presentation surface main gained a subject parameter on run_configured and a RenderContext parameter on ResultPresentation::render_body in the inspect-presentation rework; pass the rename command no subject and render its body without the context. Refs #434 --- crates/wright-cli/src/main.rs | 1 + crates/wright-cli/src/present.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/wright-cli/src/main.rs b/crates/wright-cli/src/main.rs index 340f8b94..6779dbc5 100644 --- a/crates/wright-cli/src/main.rs +++ b/crates/wright-cli/src/main.rs @@ -140,6 +140,7 @@ fn run_workflow(command: Command) -> ExitCode { Command::Rename(args) => run_configured( config_from_common(&args.common, false), present::Presentation::from_common(&args.common), + None, move |session| session.rename(&args.name, &args.to, args.write), ), Command::Lint(args) => { diff --git a/crates/wright-cli/src/present.rs b/crates/wright-cli/src/present.rs index 0c0aa1be..96aa5faa 100644 --- a/crates/wright-cli/src/present.rs +++ b/crates/wright-cli/src/present.rs @@ -607,7 +607,7 @@ impl ResultPresentation for RenameResult { format!("{edits} edit(s) applied to {}", self.written.join(", ")) }) } - fn render_body(&self) { + fn render_body(&self, _ctx: &RenderContext<'_>) { render_rename(self); } }