From 5c2a27aaf90939183686a1aa0ae884773225cb8b Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 20:27:24 -0400 Subject: [PATCH 1/3] fix(lsp): negotiate delta before requesting; input-origin signature trigger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the four post-merge findings against PR #102 (merged as 2d157d8). Stacked on the kill-ring branch (PR #103): the trigger redesign rides its command-boundary substrate. - BLOCKING delta without the capability: pull_semantic_tokens_quiet (and the pre-existing manual pmacs.lsp.semantic_tokens(), same bug) used any stored resultId to request /full/delta while only checking that a provider exists. A resultId does not imply delta support --- servers may return one from /full regardless --- and a conforming full-only server rejects the delta request; the pull path swallows the error, so styling stayed silently stale after the first edit. Both sites now require semanticTokensProvider.full.delta == true. The fake's default mode truthfully advertises { "full": { "delta": true } } (it implements delta); a new `fullonly` mode advertises "full": true, REJECTS /full/delta, and bumps its resultId per /full response so the test can observe WHICH pull refreshed the store. Verified the test bites: with the capability check reverted, the post-edit rid stays rid-1 (stale) and the test fails. - HIGH false-positive trigger + cross-frontend misclassification: the cursor-delta heuristic ("same buffer, cursor +1") fired on any one-byte edit --- including a one-byte paste of "(" once PR #103 made paste fire buffer.after-edit --- and its singleton last_typed was shared across frontends. Replaced with the input-origin signal from the #103 substrate: inside after-edit, pmacs.editor.this_command() == "buffer.self-insert" names an edit produced by typing, per frontend, with nothing inferred from cursor deltas. New ed.this_command() binding; handle_remote_crdt_op now classifies a single-codepoint optimistic insert as buffer.self-insert (rotation, not just break --- kill-chain semantics identical since self-insert is not a kill, and GPU typing now carries the same origin signal as TUI typing). Paste/pointer/undo/unbound leave this_command as something else and can never trigger. - MEDIUM first-trigger-ignored: the origin signal needs no prior-edit snapshot, so the very first "(" typed in a buffer triggers. The test that had encoded the warm-up keystroke as "correct" now types a single "(" as the first character. - MEDIUM non-ASCII trigger characters: char_before read one byte and rejected multi-byte strings; LSP trigger characters are strings. Now codepoint-aware (read up to 4 bytes back, take the suffix from the last non-continuation byte). The sighelp fake declares a two-byte trigger ("«") and a test types it. Tests (m4_acceptance 94 -> 97 after +4/-1 rework): arc1c_full_only_server_repulls_via_full_not_delta (bites --- verified), arc1d_signature_help_auto_triggers_on_trigger_char (now first-char), arc1d_signature_help_triggers_on_non_ascii_trigger_char, arc1d_signature_help_ignores_non_typed_edits (movement-stamped programmatic "(" insert + manual after-edit must not trigger --- the case cursor-delta inference cannot distinguish). Daemon unit test updated for the insert classification (break-then-classify: `this` = buffer.self-insert, `last` = None, chain still dead). Note: completion.lua still uses the Q#C9 cursor-delta heuristic and inherits its weaknesses; migrating it to this_command is a named follow-up, out of scope here. Gates: fmt + workspace clippy clean; lib 1500; crdt 1672; m4 97; killring 28; completion 9; GPU 58; git diff --check clean. Co-Authored-By: Claude Fable 5 --- builtin/runtime/lsp.lua | 80 ++++++++++---- src/bin/pmacs_fake_lsp.rs | 61 ++++++++++- src/daemon.rs | 56 +++++++--- src/editor_core.rs | 15 +++ src/lua_bindings/mod.rs | 12 ++ tests/m4_acceptance.rs | 225 ++++++++++++++++++++++++++++++++++++-- 6 files changed, 401 insertions(+), 48 deletions(-) diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 3e9a40d..3f61fac 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -470,6 +470,21 @@ end -- -- Assigns the forward-declared local above (a fresh `local function` -- here would shadow it, leaving `flush_did_change`'s upvalue nil). +-- Whether the server negotiated DELTA semantic-token support: +-- `semanticTokensProvider.full` must be a table with `delta == true`. +-- Holding a `resultId` does NOT imply delta capability — servers may +-- return one from /full regardless — and a conforming full-only server +-- rejects /full/delta. The pull path swallows request errors, so +-- requesting delta without the capability would leave styling silently +-- stale after the first edit. +local function server_supports_semantic_delta(sid) + local ok, caps = pcall(pmacs.lsp.capabilities, sid) + if not ok or not caps then return false end + local p = caps.semanticTokensProvider + if type(p) ~= "table" then return false end + return type(p.full) == "table" and p.full.delta == true +end + function pull_semantic_tokens_quiet(rec) if not rec or not server_is_initialized(rec.server) then return end if not server_supports_semantic_tokens(rec.server) then return end @@ -477,11 +492,13 @@ function pull_semantic_tokens_quiet(rec) -- positions against it. A no-op when called from `flush_did_change` -- itself (the pending entry is removed before the send). flush_did_change_for(rec) - -- Delta when we hold a `resultId` (the server only returns one when it - -- supports delta), full otherwise --- matching `pmacs.lsp - -- .semantic_tokens()`. Never clear the store first: a delta splices - -- against the retained raw stream. - local prev = pmacs.semantic_tokens.result_id(rec.server, rec.uri) + -- Delta only when the server NEGOTIATED it (full.delta == true) and a + -- prior resultId is held; /full otherwise. Never clear the store + -- first: a delta splices against the retained raw stream. + local prev = nil + if server_supports_semantic_delta(rec.server) then + prev = pmacs.semantic_tokens.result_id(rec.server, rec.uri) + end pmacs.async(function() pcall(function() if prev then @@ -640,18 +657,31 @@ end) -- Arc 1d: signature-help auto-trigger ---------------------------------- -- --- `buffer.after-edit` carries no payload, so a *typed character* is --- reconstructed from state exactly the way `completion.lua` does (Q#C9): --- same buffer, cursor advanced by exactly one byte. Paste, undo, kill, --- and remote CRDT edits produce any other delta and never auto-trigger. --- (Trigger characters are ASCII, so a one-byte advance is sound.) -local last_typed = { key = nil, cursor = nil } +-- A *typed character* is recognized by the input-origin signal, not by +-- cursor-delta inference: inside `buffer.after-edit`, +-- `pmacs.editor.this_command() == "buffer.self-insert"` names an edit +-- produced by typing — on either frontend (the daemon classifies +-- single-codepoint optimistic inserts the same way), per-frontend (no +-- cross-frontend misclassification), with no prior-edit snapshot (the +-- first character typed in a buffer triggers). Paste, undo, kill, +-- pointer, and every other input leave `this_command` as something +-- else — a one-byte paste of "(" can never trigger. +-- The last full UTF-8 codepoint ending at `cursor`, as a string. LSP +-- trigger characters are strings, not ASCII bytes, so this must be +-- codepoint-aware: read up to 4 bytes back and take the suffix from +-- the last non-continuation byte. local function char_before(buf, cursor) if cursor <= 0 then return nil end - local ok, s = pcall(function() return buf:slice(cursor - 1, cursor) end) - if not ok or type(s) ~= "string" or #s ~= 1 then return nil end - return s + local from = cursor - 4 + if from < 0 then from = 0 end + local ok, s = pcall(function() return buf:slice(from, cursor) end) + if not ok or type(s) ~= "string" or #s == 0 then return nil end + for i = #s, 1, -1 do + local b = s:byte(i) + if b < 0x80 or b >= 0xC0 then return s:sub(i) end + end + return nil end -- The set of characters that should (re)open signature help, as the @@ -710,13 +740,10 @@ pmacs.hook.add("buffer.after-edit", function() -- O(file) didChange send below is coalesced: render families -- anchored to pre-edit positions are hidden from this edit on. pcall(pmacs.lsp._mark_document_stale, rec.uri) - -- Arc 1d: did the user just type a signature trigger character? - -- Recorded before the early-outs below so the snapshot stays accurate - -- for the *next* edit even when this one doesn't trigger. - local cursor = pmacs.editor.cursor() - local prev_key, prev_cursor = last_typed.key, last_typed.cursor - last_typed.key, last_typed.cursor = key, cursor - local typed_one = key == prev_key and prev_cursor and cursor - prev_cursor == 1 + -- Arc 1d: was this edit a typed character? The input-origin signal + -- (see the trigger block below). + local typed = pmacs.editor.this_command + and pmacs.editor.this_command() == "buffer.self-insert" local now = pmacs.editor.monotonic_ms() local pending = pending_did_change[key] if pending and pending.rec == rec then @@ -726,8 +753,8 @@ pmacs.hook.add("buffer.after-edit", function() end -- Fire *after* queuing the pending didChange: `signature_help_quiet` -- flushes it, so the server sees the character we are asking about. - if not typed_one then return end - local ch = char_before(buf, cursor) + if not typed then return end + local ch = char_before(buf, pmacs.editor.cursor()) if not ch then return end local triggers = signature_trigger_chars(rec.server) if not (triggers and triggers[ch]) then return end @@ -1578,7 +1605,12 @@ function pmacs.lsp.semantic_tokens() return end -- Don't clear: a delta splices against the retained raw stream. - local prev = pmacs.semantic_tokens.result_id(rec.server, rec.uri) + -- Delta only when negotiated (full.delta) — same rule as the + -- auto-pull path; a resultId alone does not imply delta support. + local prev = nil + if server_supports_semantic_delta(rec.server) then + prev = pmacs.semantic_tokens.result_id(rec.server, rec.uri) + end pmacs.async(function() local ok, err = pcall(function() if prev then diff --git a/src/bin/pmacs_fake_lsp.rs b/src/bin/pmacs_fake_lsp.rs index 3b8d837..ec5f98a 100644 --- a/src/bin/pmacs_fake_lsp.rs +++ b/src/bin/pmacs_fake_lsp.rs @@ -32,6 +32,11 @@ //! `rootUri` received in `initialize` to the file named by //! `PMACS_FAKE_LSP_ROOT_SINK`, so a test can assert the //! auto-attach path derives the project root from the opened file. +//! * If launched with `PMACS_FAKE_LSP_MODE=fullonly`: advertises a +//! full-only `semanticTokensProvider` (`"full": true`, no delta +//! member) and rejects `semanticTokens/full/delta` with a JSON-RPC +//! error — a conforming full-only server, for testing that the +//! client never requests delta without the negotiated capability. //! * If launched with `PMACS_FAKE_LSP_MODE=sighelp`: additionally //! advertises `signatureHelpProvider` with `(` / `,` triggers, so a //! test can drive the Arc 1d auto-trigger. Every other mode omits the @@ -54,6 +59,8 @@ fn main() { let mut stdout = io::stdout().lock(); let mut crashed_after_init = false; let mut open_docs: HashMap = HashMap::new(); + // `fullonly` observability: counts /full responses (rid-1, rid-2…). + let mut full_count: u32 = 0; loop { let body = match read_frame(&mut stdin) { Ok(Some(b)) => b, @@ -138,7 +145,12 @@ fn main() { "tokenTypes": ["namespace", "function", "variable"], "tokenModifiers": ["declaration", "readonly"] }, - "full": true + // The default mode implements /full/delta, so + // it truthfully NEGOTIATES delta. Clients may + // only send /full/delta when `full` is + // `{ "delta": true }`; a bare `true` (the + // `fullonly` override below) is full-only. + "full": { "delta": true } } }, "serverInfo": { "name": "pmacs-fake-lsp", "version": "0.1.0" } @@ -158,13 +170,25 @@ fn main() { resp["result"]["capabilities"]["renameProvider"] = serde_json::json!({ "prepareProvider": true }); } + // `fullonly`: a conforming FULL-ONLY semantic-token + // server — advertises `"full": true` (no delta member) + // and REJECTS /full/delta below. Exercises the client + // rule that a stored resultId alone must never cause a + // delta request. + if mode == "fullonly" { + resp["result"]["capabilities"]["semanticTokensProvider"]["full"] = + serde_json::Value::from(true); + } // Arc 1d: advertise signature help only in `sighelp`, so // every other mode keeps the no-auto-trigger path (the // `textDocument/signatureHelp` arm below still answers // the manual `M-x lsp.signature-help` in any mode). if mode == "sighelp" { + // "«" (U+00AB, 2 UTF-8 bytes) exercises the rule + // that LSP trigger characters are strings, not + // ASCII bytes. resp["result"]["capabilities"]["signatureHelpProvider"] = serde_json::json!({ - "triggerCharacters": ["("], + "triggerCharacters": ["(", "\u{ab}"], "retriggerCharacters": [","] }); } @@ -749,6 +773,23 @@ fn main() { write_frame(&mut stdout, &resp); } ("textDocument/semanticTokens/full", Some(idv)) => { + // `fullonly`: bump the resultId per request so a test + // can observe WHICH pull refreshed the store — a + // repull that wrongly went to /full/delta is rejected + // and leaves the previous rid in place. + if mode == "fullonly" { + full_count += 1; + let resp = serde_json::json!({ + "jsonrpc": "2.0", + "id": idv, + "result": { + "resultId": format!("rid-{full_count}"), + "data": [0, 0, 4, 1, 1, 0, 5, 3, 2, 0, 2, 2, 7, 0, 2] + } + }); + write_frame(&mut stdout, &resp); + continue; + } // T M4.5: relative-encoded `data`. Three tokens: // [0,0,4,1,1] line 0 col 0 len 4, function, decl // [0,5,3,2,0] same line col 5 len 3, variable @@ -774,6 +815,22 @@ fn main() { write_frame(&mut stdout, &resp); } ("textDocument/semanticTokens/full/delta", Some(idv)) => { + // `fullonly`: a conforming full-only server rejects a + // delta request outright — the client should never have + // sent it (capabilities advertised `"full": true` with + // no delta member). + if mode == "fullonly" { + let resp = serde_json::json!({ + "jsonrpc": "2.0", + "id": idv, + "error": { + "code": -32601, + "message": "semanticTokens/full/delta not supported" + } + }); + write_frame(&mut stdout, &resp); + continue; + } // T M4.5: a `SemanticTokensDelta` over the /full data // `[0,0,4,1,1, 0,5,3,2,0, 2,2,7,0,2]` — replace the // last 5-int group (idx 10..15) with [3,0,9,1,0], so diff --git a/src/daemon.rs b/src/daemon.rs index 15342de..51d74d4 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -2008,11 +2008,16 @@ fn handle_remote_crdt_op( buffer_id: crate::buffer::BufferId, op: crate::rope::CrdtOp, ) { - // Kill ring Q#KR2: an optimistic edit is non-command input — GPU - // typing, Enter/Tab, Backspace/Delete all arrive here without ever - // touching dispatch_key. It must break the source frontend's - // command chain, or `C-k x C-k` on the GPU would append across the - // typed character. + // Kill ring Q#KR2: an optimistic edit arrives here without ever + // touching dispatch_key, so the source's command boundary must be + // updated — or `C-k x C-k` on the GPU would append across the typed + // character. Break first (covers every early-return path); a + // successful apply refines this below: a single-codepoint insert is + // re-classified as `buffer.self-insert`, giving typed characters the + // same boundary on both frontends. That keeps kill-chain semantics + // identical (self-insert is not a kill) while making `this_command` + // a usable input-origin signal for typed-char consumers (signature + // help; the completion popup can migrate later). editor.core.borrow_mut().break_command_chain(source); // Effect 1: apply to buffer's CRDT + rope. Capture the Edit // (or `None` for an op that imported cleanly but produced no @@ -2045,6 +2050,14 @@ fn handle_remote_crdt_op( // notify but the op still needs broadcasting (F17). if let Some(edit) = edit_opt.as_ref() { let mut core = editor.core.borrow_mut(); + // The input-origin refinement promised above. The optimistic + // layer emits exactly one op per keystroke, so an empty-range + // insert of one codepoint (1–4 UTF-8 bytes) IS a typed + // character — Backspace/Delete/Undo produce deletes or larger + // shapes and stay chain-breaks. + if edit.range.start == edit.range.end && (1..=4).contains(&edit.inserted_len) { + core.rotate_command(source, "buffer.self-insert"); + } // Transient status messages clear on user input. The Key path // gets this from `dispatch_key`'s entry clear; the optimistic // path routes plain typing here instead, and since v15 ships @@ -2511,13 +2524,15 @@ mod tests { ); } - /// Kill ring Q#KR2 — an optimistic edit is non-command input: GPU - /// typing arrives here without touching dispatch_key, so it must - /// break the source frontend's command chain or `C-k x C-k` on the - /// GPU would append across the typed character. + /// Kill ring Q#KR2 — GPU typing arrives here without touching + /// dispatch_key, so it must update the source frontend's command + /// boundary or `C-k x C-k` on the GPU would append across the typed + /// character. A single-codepoint insert classifies as + /// `buffer.self-insert` (the input-origin signal for signature + /// help); anything else breaks the chain outright. #[cfg(feature = "crdt")] #[test] - fn handle_remote_crdt_op_breaks_the_source_command_chain() { + fn handle_remote_crdt_op_classifies_typed_input_and_ends_kill_chains() { use crate::editor::EditorState; use crate::protocol::FrontendId; @@ -2573,11 +2588,26 @@ mod tests { ); let core = editor.core.borrow(); - assert!( + // A single-codepoint optimistic insert classifies as a typed + // character: the boundary rotates to buffer.self-insert (the + // input-origin signal), which — not being a kill command — + // still breaks the kill chain exactly like the TUI typed-char + // path. + assert_eq!( core.command_history .get(&source) - .is_none_or(|b| b.this.is_none()), - "the optimistic edit must break the source's chain" + .and_then(|b| b.this.as_deref()), + Some("buffer.self-insert"), + "a typed optimistic insert classifies as self-insert" + ); + assert_eq!( + core.command_history + .get(&source) + .and_then(|b| b.last.as_deref()), + None, + "the pre-existing kill chain is gone (break-then-classify): a \ + following kill reads last = self-insert after its own rotation \ + and never appends" ); assert_eq!( core.command_history diff --git a/src/editor_core.rs b/src/editor_core.rs index 6eb50d0..4dcccf9 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -2104,6 +2104,21 @@ impl EditorCore { .as_deref() } + /// The active frontend's *current* command — Emacs's `this-command`. + /// Inside a `buffer.after-edit` hook this names the command that + /// produced the edit, which is the **input-origin signal**: + /// `"buffer.self-insert"` means the edit was a typed character + /// (keybound or optimistic), while a paste / pointer / unbound input + /// left it `None`. Per-frontend, so two attached frontends never + /// misclassify each other's input. + #[must_use] + pub fn this_command(&self) -> Option<&str> { + self.command_history + .get(&self.active_frontend)? + .this + .as_deref() + } + /// Copy the active region into the clipboard slot and queue an /// outbound OS-clipboard publish to the originating frontend. /// Returns `false` (a no-op) when there is no region. diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 17ed4fa..dcdab4b 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -10999,6 +10999,18 @@ fn install_session(editor: &Table, lua: &Lua, core: &SharedCore) -> mlua::Result lua.create_function(move |_, ()| Ok(cc.borrow().last_command().map(str::to_owned)))?, )?; } + { + // this_command(): the command currently executing for the + // active frontend — the input-origin signal. Inside + // `buffer.after-edit`, "buffer.self-insert" means the edit was + // a typed character; nil means a non-command input (paste, + // pointer gesture, optimistic delete/undo). Per-frontend. + let cc = core.clone(); + editor.set( + "this_command", + lua.create_function(move |_, ()| Ok(cc.borrow().this_command().map(str::to_owned)))?, + )?; + } { // view_top(): the active window's first visible source line. // The saveplace getter (Arc 3) — pairs with set_view_top so a diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 1f43d54..bf425d3 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -4451,15 +4451,13 @@ fn arc1d_signature_help_auto_triggers_on_trigger_char() { end)()"; assert!(pump_lua_flag(&mut state, initialized, 5), "server init"); - // The first keystroke only seeds the typed-char snapshot; the second - // is the trigger. (A trigger char cannot fire off the very first edit - // in a buffer, which is correct: there is no prior cursor to compare.) - for c in ['f', '('] { - state.dispatch_key( - pmacs::protocol::FrontendId::LOCAL, - KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE), - ); - } + // The very FIRST character typed in the buffer is the trigger: the + // input-origin signal (this_command == buffer.self-insert) needs no + // prior-edit snapshot, so there is no warm-up keystroke. + state.dispatch_key( + pmacs::protocol::FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('('), KeyModifiers::NONE), + ); let deadline = Instant::now() + Duration::from_secs(5); let mut saw = false; while Instant::now() < deadline { @@ -4531,6 +4529,215 @@ fn arc1d_signature_help_does_not_trigger_on_ordinary_typing() { } } +/// Arc 1d — a server-declared NON-ASCII trigger character works. LSP +/// trigger characters are strings; the fake declares "«" (2 UTF-8 +/// bytes), and the codepoint-aware `char_before` must match it. +#[test] +fn arc1d_signature_help_triggers_on_non_ascii_trigger_char() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let a_path = dir.path().join("a.rs"); + std::fs::write(&a_path, b"\n").expect("write a"); + let a_disp = a_path.display().to_string(); + + let mut state = EditorState::new(); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ + command = '{fake}', + env = {{ PMACS_FAKE_LSP_MODE = 'sighelp' }}, + }}" + )) + .exec() + .expect("override rust config"); + state + .lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open('{a_disp}')")) + .exec() + .expect("open a.rs"); + let initialized = "(function() \ + for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then return true end \ + end \ + return false \ + end)()"; + assert!(pump_lua_flag(&mut state, initialized, 5), "server init"); + + state.dispatch_key( + pmacs::protocol::FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('\u{ab}'), KeyModifiers::NONE), + ); + let deadline = Instant::now() + Duration::from_secs(5); + let mut saw = false; + while Instant::now() < deadline { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + if state.core.borrow().status.contains("fn echo(") { + saw = true; + break; + } + } + assert!(saw, "a non-ASCII trigger character must auto-trigger"); +} + +/// Arc 1d — an edit that is NOT a typed character never triggers, even +/// when it inserts exactly one trigger byte. The input-origin signal +/// (`this_command`) distinguishes it; a cursor-delta heuristic could +/// not (a one-byte programmatic insert of `(` looks identical). +#[test] +fn arc1d_signature_help_ignores_non_typed_edits() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let a_path = dir.path().join("a.rs"); + std::fs::write(&a_path, b"\n").expect("write a"); + let a_disp = a_path.display().to_string(); + + let mut state = EditorState::new(); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ + command = '{fake}', + env = {{ PMACS_FAKE_LSP_MODE = 'sighelp' }}, + }}" + )) + .exec() + .expect("override rust config"); + state + .lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open('{a_disp}')")) + .exec() + .expect("open a.rs"); + let initialized = "(function() \ + for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then return true end \ + end \ + return false \ + end)()"; + assert!(pump_lua_flag(&mut state, initialized, 5), "server init"); + + // A movement command stamps this_command = cursor.*; then a + // programmatic one-byte insert of "(" fires after-edit. Under the + // old cursor-delta heuristic this was indistinguishable from + // typing. + state.dispatch_key( + pmacs::protocol::FrontendId::LOCAL, + KeyEvent::new(KeyCode::Down, KeyModifiers::NONE), + ); + state + .lua_host + .lua() + .load( + "pmacs.window.buffer():insert(pmacs.editor.cursor(), '(') \n\ + pmacs.hook.run('buffer.after-edit')", + ) + .exec() + .expect("programmatic insert"); + let deadline = Instant::now() + Duration::from_secs(2); + while Instant::now() < deadline { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + assert!( + !state.core.borrow().status.contains("fn echo("), + "a non-typed one-byte '(' insert must not trigger signature help" + ); + } +} + +/// Arc 1c review fix — a conforming FULL-ONLY server (advertises +/// `"full": true`, rejects /full/delta). Holding a resultId from the +/// first /full pull must NOT cause a delta request: the repull after an +/// edit goes to /full again and the store refreshes. Before the fix, +/// the delta request was rejected, the error swallowed, and semantic +/// styling stayed silently stale after the first edit. +#[test] +fn arc1c_full_only_server_repulls_via_full_not_delta() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let a_path = dir.path().join("a.rs"); + std::fs::write(&a_path, b"fn a() {}\n\n").expect("write a"); + let a_disp = a_path.display().to_string(); + + let mut state = EditorState::new(); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ + command = '{fake}', + env = {{ PMACS_FAKE_LSP_MODE = 'fullonly' }}, + }}" + )) + .exec() + .expect("override rust config"); + state + .lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open('{a_disp}')")) + .exec() + .expect("open a.rs"); + + let has_tokens = format!( + "(function() \ + local sid \ + for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then sid=r.id end \ + end \ + if not sid then return false end \ + local t = pmacs.semantic_tokens.tokens(sid, 'file://{a_disp}') \ + return t ~= nil and #t > 0 \ + end)()" + ); + assert!( + pump_lua_flag(&mut state, &has_tokens, 5), + "attach /full pull" + ); + + // The fullonly fake bumps its resultId per /full response, so the + // store's rid says WHICH pull refreshed it. After the attach pull it + // is rid-1; the post-edit repull must advance it via /full. A repull + // that wrongly went to /full/delta (the pre-fix behavior: a stored + // resultId alone triggered delta) is rejected by the server, the + // error swallowed, and the rid stays rid-1 — silently stale. + let rid_is = |n: u32| { + format!( + "(function() \ + local sid \ + for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then sid=r.id end \ + end \ + if not sid then return false end \ + return pmacs.semantic_tokens.result_id(sid, 'file://{a_disp}') == 'rid-{n}' \ + end)()" + ) + }; + assert!( + pump_lua_flag(&mut state, &rid_is(1), 5), + "attach pull is rid-1" + ); + state.dispatch_key( + pmacs::protocol::FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('x'), KeyModifiers::NONE), + ); + assert!( + pump_lua_flag(&mut state, &rid_is(2), 5), + "a full-only server's repull must refresh via /full, not stale-out on a rejected delta" + ); +} + /// T M4.5 — `textDocument/semanticTokens/range` through the Lua /// surface. Same decode path as `/full`, scoped to a range; the /// fake returns one token. From 625128c13929ac859d4eee775dd1bb952e4550e3 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 20:48:08 -0400 Subject: [PATCH 2/3] fix(lsp): range-only providers, completion-accept boundary, exact codepoint classify MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the round-4 findings against the stack (PR #104 portion). - HIGH range-only semantic-token servers: LSP defines semanticTokensProvider.full and .range as optional, INDEPENDENT capabilities, but the old any-provider gate sent /full regardless — a range-only server rejects it and the swallowed error means no styling, ever. Both the auto-pull and the manual command now gate each request kind on its own capability: /full (delta under full.delta) when negotiated; a range-only provider gets a WHOLE-DOCUMENT /range request. New `rangeonly` fake mode (advertises range without full, rejects /full) + test proving tokens arrive via the range path. - MEDIUM completion acceptance left this_command stale: the popup accept applies its edit and fires after-edit outside command dispatch, so this_command could still read "buffer.self-insert" from the typing that raised the popup — a candidate ending in "(" would spuriously auto-trigger signature help. Accept now stamps its own boundary ("completion.accept"); asserted in the popup acceptance suite. - MEDIUM GPU shape inference tightened: the 1-4-byte predicate accepted a 2-byte "a(" insert (two ASCII codepoints). The classifier now decodes the inserted bytes from the post-edit rope and requires the leading byte's UTF-8 sequence length to equal inserted_len — exactly one codepoint. The daemon unit test now drives an "a(" op and asserts it breaks the chain instead of classifying as typing. Exact wire provenance on the CRDT op remains the named deferred general fix. Gates: fmt + workspace clippy clean; lib 1500; crdt 1672; m4 98; completion 9; killring 30; GPU 58; git diff --check clean. Co-Authored-By: Claude Fable 5 --- builtin/runtime/lsp.lua | 72 +++++++++++++++++++------ src/bin/pmacs_fake_lsp.rs | 29 ++++++++++ src/daemon.rs | 81 ++++++++++++++++++++++++++-- src/editor.rs | 11 ++++ tests/completion_popup_acceptance.rs | 17 ++++++ tests/m4_acceptance.rs | 53 ++++++++++++++++++ 6 files changed, 243 insertions(+), 20 deletions(-) diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 3f61fac..70b520e 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -452,11 +452,28 @@ function pull_inlay_hints_quiet(rec) end) end -local function server_supports_semantic_tokens(sid) +-- LSP defines `semanticTokensProvider.full` and `.range` as optional, +-- INDEPENDENT capabilities: a provider may be range-only, and sending +-- it /full gets a rejection the pull path would swallow. Gate each +-- request kind on its own capability. +local function semantic_provider(sid) local ok, caps = pcall(pmacs.lsp.capabilities, sid) - if not ok or not caps then return false end + if not ok or not caps then return nil end local p = caps.semanticTokensProvider - return p ~= nil and p ~= false + if p == nil or p == false then return nil end + return p +end + +local function server_supports_semantic_full(sid) + local p = semantic_provider(sid) + if type(p) ~= "table" then return false end + return p.full == true or type(p.full) == "table" +end + +local function server_supports_semantic_range(sid) + local p = semantic_provider(sid) + if type(p) ~= "table" then return false end + return p.range == true or type(p.range) == "table" end -- Arc 1c. Semantic tokens are pull-model, exactly like inlay hints: the @@ -478,33 +495,42 @@ end -- requesting delta without the capability would leave styling silently -- stale after the first edit. local function server_supports_semantic_delta(sid) - local ok, caps = pcall(pmacs.lsp.capabilities, sid) - if not ok or not caps then return false end - local p = caps.semanticTokensProvider + local p = semantic_provider(sid) if type(p) ~= "table" then return false end return type(p.full) == "table" and p.full.delta == true end function pull_semantic_tokens_quiet(rec) if not rec or not server_is_initialized(rec.server) then return end - if not server_supports_semantic_tokens(rec.server) then return end + local has_full = server_supports_semantic_full(rec.server) + local has_range = server_supports_semantic_range(rec.server) + if not has_full and not has_range then return end -- The server must see the current text before computing token -- positions against it. A no-op when called from `flush_did_change` -- itself (the pending entry is removed before the send). flush_did_change_for(rec) - -- Delta only when the server NEGOTIATED it (full.delta == true) and a - -- prior resultId is held; /full otherwise. Never clear the store - -- first: a delta splices against the retained raw stream. + -- /full when negotiated (delta only when full.delta == true and a + -- prior resultId is held); a RANGE-ONLY provider gets a + -- whole-document /range request instead — never an unsupported + -- /full. Never clear the store first: a delta splices against the + -- retained raw stream. local prev = nil - if server_supports_semantic_delta(rec.server) then + if has_full and server_supports_semantic_delta(rec.server) then prev = pmacs.semantic_tokens.result_id(rec.server, rec.uri) end + local end_line, end_col + if not has_full then + end_line, end_col = document_end_position(buffer_text(rec.buffer)) + end pmacs.async(function() pcall(function() if prev then pmacs.lsp.request_semantic_tokens_delta(rec.server, rec.uri, prev):await() - else + elseif has_full then pmacs.lsp.request_semantic_tokens(rec.server, rec.uri):await() + else + pmacs.lsp.request_semantic_tokens_range( + rec.server, rec.uri, 0, 0, end_line, end_col):await() end end) end) @@ -1605,19 +1631,33 @@ function pmacs.lsp.semantic_tokens() return end -- Don't clear: a delta splices against the retained raw stream. - -- Delta only when negotiated (full.delta) — same rule as the - -- auto-pull path; a resultId alone does not imply delta support. + -- Same gating as the auto-pull path: /full only when negotiated + -- (delta only under full.delta); a range-only provider gets a + -- whole-document /range request. + local has_full = server_supports_semantic_full(rec.server) + local has_range = server_supports_semantic_range(rec.server) + if not has_full and not has_range then + pmacs.editor.set_status("LSP: server has no semantic-token support") + return + end local prev = nil - if server_supports_semantic_delta(rec.server) then + if has_full and server_supports_semantic_delta(rec.server) then prev = pmacs.semantic_tokens.result_id(rec.server, rec.uri) end + local end_line, end_col + if not has_full then + end_line, end_col = document_end_position(buffer_text(rec.buffer)) + end pmacs.async(function() local ok, err = pcall(function() if prev then pmacs.lsp.request_semantic_tokens_delta( rec.server, rec.uri, prev):await() - else + elseif has_full then pmacs.lsp.request_semantic_tokens(rec.server, rec.uri):await() + else + pmacs.lsp.request_semantic_tokens_range( + rec.server, rec.uri, 0, 0, end_line, end_col):await() end end) if not ok then diff --git a/src/bin/pmacs_fake_lsp.rs b/src/bin/pmacs_fake_lsp.rs index ec5f98a..4f48f87 100644 --- a/src/bin/pmacs_fake_lsp.rs +++ b/src/bin/pmacs_fake_lsp.rs @@ -37,6 +37,10 @@ //! member) and rejects `semanticTokens/full/delta` with a JSON-RPC //! error — a conforming full-only server, for testing that the //! client never requests delta without the negotiated capability. +//! * If launched with `PMACS_FAKE_LSP_MODE=rangeonly`: advertises a +//! range-only `semanticTokensProvider` (`"range": true`, `full` +//! null) and rejects `semanticTokens/full` — per LSP, `full` and +//! `range` are optional, independent capabilities. //! * If launched with `PMACS_FAKE_LSP_MODE=sighelp`: additionally //! advertises `signatureHelpProvider` with `(` / `,` triggers, so a //! test can drive the Arc 1d auto-trigger. Every other mode omits the @@ -179,6 +183,17 @@ fn main() { resp["result"]["capabilities"]["semanticTokensProvider"]["full"] = serde_json::Value::from(true); } + // `rangeonly`: LSP allows a provider to advertise + // `range` WITHOUT `full` — the /full arm below rejects + // in this mode, so a client that ignores the split gets + // a visible failure instead of silent staleness. + if mode == "rangeonly" { + let p = &mut resp["result"]["capabilities"]["semanticTokensProvider"]; + if let Some(obj) = p.as_object_mut() { + obj.remove("full"); + obj.insert("range".into(), serde_json::Value::from(true)); + } + } // Arc 1d: advertise signature help only in `sighelp`, so // every other mode keeps the no-auto-trigger path (the // `textDocument/signatureHelp` arm below still answers @@ -773,6 +788,20 @@ fn main() { write_frame(&mut stdout, &resp); } ("textDocument/semanticTokens/full", Some(idv)) => { + // `rangeonly`: a range-only provider rejects /full — + // the client should have sent a range request. + if mode == "rangeonly" { + let resp = serde_json::json!({ + "jsonrpc": "2.0", + "id": idv, + "error": { + "code": -32601, + "message": "semanticTokens/full not supported" + } + }); + write_frame(&mut stdout, &resp); + continue; + } // `fullonly`: bump the resultId per request so a test // can observe WHICH pull refreshed the store — a // repull that wrongly went to /full/delta is rejected diff --git a/src/daemon.rs b/src/daemon.rs index 51d74d4..0a87188 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1978,6 +1978,28 @@ fn handle_inbound_paste( }); } +/// True when `edit` inserted exactly one UTF-8 codepoint: the leading +/// byte's sequence length equals `inserted_len` (kill ring review +/// round 4 — the typed-character classification for optimistic edits). +#[cfg(feature = "crdt")] +fn is_single_codepoint_insert(edit: &crate::rope::Edit) -> bool { + let len = edit.inserted_len; + if !(1..=4).contains(&len) { + return false; + } + let mut first = [0u8; 1]; + edit.new_rope + .slice(edit.range.start, edit.range.start + 1, &mut first); + let expected = match first[0] { + b if b < 0x80 => 1, + b if b < 0xC0 => return false, // bare continuation byte + b if b < 0xE0 => 2, + b if b < 0xF0 => 3, + _ => 4, + }; + expected == len +} + /// T M10.10 (post-audit) — apply a *pre-validated* /// `FrontendEvent::CrdtOp`. Identity, capability, and scope checks /// happen upstream in `validate_remote_crdt_op`; this function trusts @@ -2052,10 +2074,15 @@ fn handle_remote_crdt_op( let mut core = editor.core.borrow_mut(); // The input-origin refinement promised above. The optimistic // layer emits exactly one op per keystroke, so an empty-range - // insert of one codepoint (1–4 UTF-8 bytes) IS a typed - // character — Backspace/Delete/Undo produce deletes or larger - // shapes and stay chain-breaks. - if edit.range.start == edit.range.end && (1..=4).contains(&edit.inserted_len) { + // insert of EXACTLY ONE codepoint is a typed character — + // Backspace/Delete/Undo produce deletes or larger shapes and + // stay chain-breaks. Decoding the inserted bytes (they are in + // the post-edit rope) rather than trusting `inserted_len` + // alone: a 2-byte insert of "a(" is two ASCII codepoints and + // must NOT classify as typing (review round 4 — it would + // spuriously auto-trigger signature help). Exact provenance on + // the wire op is the named deferred general fix. + if edit.range.start == edit.range.end && is_single_codepoint_insert(edit) { core.rotate_command(source, "buffer.self-insert"); } // Transient status messages clear on user input. The Key path @@ -2609,6 +2636,52 @@ mod tests { following kill reads last = self-insert after its own rotation \ and never appends" ); + drop(core); + + // A TWO-codepoint insert ("a(") must NOT classify as typing + // (review round 4): its 2-byte length satisfies a naive 1-4 + // predicate, but decoding shows two ASCII codepoints — a typed + // key never produces that, and classifying it would let a + // multi-char op spuriously auto-trigger signature help. + editor + .core + .borrow_mut() + .rotate_command(source, "edit.kill-line"); + let snapshot_bytes = { + let core = editor.core.borrow(); + let reg = core.registry.borrow(); + reg.get(buffer_id) + .expect("buffer") + .crdt_state() + .expect("crdt-backed") + .export_snapshot() + .expect("export snapshot") + }; + let peer2 = loro::LoroDoc::new(); + peer2.set_peer_id(7).expect("set peer id"); + peer2.import(&snapshot_bytes).expect("import snapshot"); + let v_before = peer2.oplog_vv(); + peer2.get_text("body").insert(0, "a(").expect("peer insert"); + let op_bytes = peer2 + .export(loro::ExportMode::updates(&v_before)) + .expect("export op"); + handle_remote_crdt_op( + &mut editor, + source, + buffer_id, + crate::rope::CrdtOp { + peer_id: 7, + bytes: op_bytes, + }, + ); + let core = editor.core.borrow(); + assert_eq!( + core.command_history + .get(&source) + .and_then(|b| b.this.as_deref()), + None, + "a multi-codepoint insert breaks the chain instead of classifying as typing" + ); assert_eq!( core.command_history .get(&FrontendId::LOCAL) diff --git a/src/editor.rs b/src/editor.rs index 2b946f7..b9bdf03 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -930,6 +930,17 @@ impl EditorState { CompletionPopupKey::Prev => self.core.borrow_mut().completion_popup_step(-1), CompletionPopupKey::Dismiss => self.core.borrow_mut().completion_popup_close(), CompletionPopupKey::Accept => { + // Accepting a completion is its own command boundary + // (review round 4): without this stamp, `this_command` + // could still read "buffer.self-insert" from the typing + // that raised the popup, and the after-edit fired below + // would let a candidate ending in "(" spuriously + // auto-trigger signature help. + { + let mut core = self.core.borrow_mut(); + let fid = core.active_frontend; + core.rotate_command(fid, "completion.accept"); + } let pre_revision = self.active_buffer_revision(); self.core.borrow_mut().completion_popup_accept(); if pre_revision != self.active_buffer_revision() { diff --git a/tests/completion_popup_acceptance.rs b/tests/completion_popup_acceptance.rs index c2ffeaf..f15a705 100644 --- a/tests/completion_popup_acceptance.rs +++ b/tests/completion_popup_acceptance.rs @@ -72,6 +72,23 @@ fn typing_opens_popup_and_tab_accepts() { assert_eq!(text, "hello_world hello_world", "TAB replaces the prefix"); assert!(!visible, "accept closes the popup"); assert_eq!(cursor, 23, "cursor lands just past the inserted text"); + + // Kill ring review round 4: accepting a completion is its own + // command boundary. Without the stamp, this_command would still + // read "buffer.self-insert" from the typing that raised the popup, + // and a candidate ending in "(" would spuriously auto-trigger + // signature help from the accept's after-edit. + let this: Option = s + .lua_host + .lua() + .load("return pmacs.editor.this_command()") + .eval() + .unwrap(); + assert_eq!( + this.as_deref(), + Some("completion.accept"), + "accept stamps its own boundary, not the typing's self-insert" + ); } /// C-n moves the highlight before RET accepts, so the second diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index bf425d3..8d4dd16 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -4529,6 +4529,59 @@ fn arc1d_signature_help_does_not_trigger_on_ordinary_typing() { } } +/// Arc 1c review fix — a RANGE-ONLY provider (LSP: `full` and `range` +/// are optional, independent capabilities). The client must serve it a +/// whole-document /range request, never /full — the fake rejects /full +/// outright, so a client ignoring the split gets an empty store and +/// this test fails. +#[test] +fn arc1c_range_only_server_is_served_range_requests() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let a_path = dir.path().join("a.rs"); + std::fs::write(&a_path, b"fn a() {}\n\n").expect("write a"); + let a_disp = a_path.display().to_string(); + + let mut state = EditorState::new(); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ + command = '{fake}', + env = {{ PMACS_FAKE_LSP_MODE = 'rangeonly' }}, + }}" + )) + .exec() + .expect("override rust config"); + state + .lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open('{a_disp}')")) + .exec() + .expect("open a.rs"); + + // The rangeonly fake's /range reply carries one token; its + // presence proves the auto-pull went through the range path. + let has_tokens = format!( + "(function() \ + local sid \ + for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then sid=r.id end \ + end \ + if not sid then return false end \ + local t = pmacs.semantic_tokens.tokens(sid, 'file://{a_disp}') \ + return t ~= nil and #t > 0 \ + end)()" + ); + assert!( + pump_lua_flag(&mut state, &has_tokens, 5), + "a range-only provider must be served a whole-document /range request" + ); +} + /// Arc 1d — a server-declared NON-ASCII trigger character works. LSP /// trigger characters are strings; the fake declares "«" (2 UTF-8 /// bytes), and the codepoint-aware `char_before` must match it. From 685547f2a713144b85d4deee7a1dd4f1c42fbeec Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 9 Jul 2026 21:17:50 -0400 Subject: [PATCH 3/3] fix(lsp): convert semantic-range (and code-action) bounds per position encoding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the round-5 finding: the whole-document range that serves a RANGE-ONLY semantic-token provider derived its columns from UTF-8 byte counts and sent them unchanged — unlike the inlay path, it skipped outbound_position. A UTF-16 server receives an invalid end character for non-ASCII text ("é" is two bytes, one UTF-16 unit) and may reject the request; since /range is a range-only provider's ONLY pull path, that means no semantic styling at all. Both bounds of request_semantic_tokens_range now go through outbound_position. request_code_action had the identical bug (byte columns, no conversion) and is fixed in the same stroke — same class, same one-line shape, commented as such. Fixture: `rangeonly16` fake mode = rangeonly + negotiated UTF-16 + STRICT UTF-16 bounds validation on /range (fail-closed: a missing didOpen record or absent uri also rejects, so the fixture can never pass vacuously). An env-gated PMACS_FAKE_RANGE_SINK records the received range for debugging. Test opens a file whose last line ends in non-ASCII and asserts tokens arrive; verified it bites — with the conversion removed the wire carries the byte column (13 vs the valid 11), the fake rejects, and the test fails. Honest note: an earlier bite-check in this session produced a vacuous pass because short, non-unique edit patterns hit the WRONG json! block (temporarily regressing the inlay conversion and accidentally converting code-action). The final diff is anchored uniquely and verified: inlay unchanged (whitespace only), semantic + code-action converted, bite-check red/green confirmed against the exact lines. Gates: fmt + workspace clippy clean; lib 1500; crdt 1672; m4 99; killring 30; completion 9; GPU 58; git diff --check clean. Co-Authored-By: Claude Fable 5 --- src/bin/pmacs_fake_lsp.rs | 83 ++++++++++++++++++++++++++++++++++++--- src/lsp.rs | 21 +++++++--- tests/m4_acceptance.rs | 53 +++++++++++++++++++++++++ 3 files changed, 147 insertions(+), 10 deletions(-) diff --git a/src/bin/pmacs_fake_lsp.rs b/src/bin/pmacs_fake_lsp.rs index 4f48f87..9be1cd0 100644 --- a/src/bin/pmacs_fake_lsp.rs +++ b/src/bin/pmacs_fake_lsp.rs @@ -38,9 +38,13 @@ //! error — a conforming full-only server, for testing that the //! client never requests delta without the negotiated capability. //! * If launched with `PMACS_FAKE_LSP_MODE=rangeonly`: advertises a -//! range-only `semanticTokensProvider` (`"range": true`, `full` -//! null) and rejects `semanticTokens/full` — per LSP, `full` and -//! `range` are optional, independent capabilities. +//! range-only `semanticTokensProvider` (`"range": true`, no `full`) +//! and rejects `semanticTokens/full` — per LSP, `full` and `range` +//! are optional, independent capabilities. +//! * If launched with `PMACS_FAKE_LSP_MODE=rangeonly16`: `rangeonly` +//! plus UTF-16 position encoding, with strict UTF-16 bounds +//! validation on `/range` — rejects a client that sent raw byte +//! columns for non-ASCII text. //! * If launched with `PMACS_FAKE_LSP_MODE=sighelp`: additionally //! advertises `signatureHelpProvider` with `(` / `,` triggers, so a //! test can drive the Arc 1d auto-trigger. Every other mode omits the @@ -187,13 +191,20 @@ fn main() { // `range` WITHOUT `full` — the /full arm below rejects // in this mode, so a client that ignores the split gets // a visible failure instead of silent staleness. - if mode == "rangeonly" { + if mode.starts_with("rangeonly") { let p = &mut resp["result"]["capabilities"]["semanticTokensProvider"]; if let Some(obj) = p.as_object_mut() { obj.remove("full"); obj.insert("range".into(), serde_json::Value::from(true)); } } + // `rangeonly16` additionally negotiates UTF-16, so the + // /range arm can validate that the client converted its + // byte columns to UTF-16 code units. + if mode == "rangeonly16" { + resp["result"]["capabilities"]["positionEncoding"] = + serde_json::Value::from("utf-16"); + } // Arc 1d: advertise signature help only in `sighelp`, so // every other mode keeps the no-auto-trigger path (the // `textDocument/signatureHelp` arm below still answers @@ -790,7 +801,7 @@ fn main() { ("textDocument/semanticTokens/full", Some(idv)) => { // `rangeonly`: a range-only provider rejects /full — // the client should have sent a range request. - if mode == "rangeonly" { + if mode.starts_with("rangeonly") { let resp = serde_json::json!({ "jsonrpc": "2.0", "id": idv, @@ -834,6 +845,27 @@ fn main() { write_frame(&mut stdout, &resp); } ("textDocument/semanticTokens/range", Some(idv)) => { + // `rangeonly16`: strict bounds validation in UTF-16 + // units. A client that sent raw byte columns for + // non-ASCII text overshoots the last line's UTF-16 + // length and is rejected — the fixture for the + // outbound-position conversion. + if mode == "rangeonly16" + && let Ok(sink) = std::env::var("PMACS_FAKE_RANGE_SINK") + { + let _ = std::fs::write(&sink, format!("{params}")); + } + if mode == "rangeonly16" + && let Some(message) = utf16_range_error(¶ms, &open_docs) + { + let resp = serde_json::json!({ + "jsonrpc": "2.0", + "id": idv, + "error": { "code": -32602, "message": message } + }); + write_frame(&mut stdout, &resp); + continue; + } // T M4.5: same shape as /full, scoped to a range. // One token: line 1 col 0 len 3, variable. let resp = serde_json::json!({ @@ -1053,6 +1085,47 @@ fn document_end_position(text: &str) -> (u64, u64) { (line, col) } +/// `rangeonly16` bounds validation: the request's end position must not +/// exceed the document end measured in UTF-16 code units (the +/// negotiated encoding). Byte-column overshoot on non-ASCII text is +/// exactly the client bug this catches. +fn utf16_range_error( + params: &serde_json::Value, + open_docs: &HashMap, +) -> Option { + // Fail-CLOSED: a fixture that silently skips validation on an + // unexpected state (missing uri / unrecorded doc) reads as a pass. + let Some(uri) = params + .get("textDocument") + .and_then(|t| t.get("uri")) + .and_then(serde_json::Value::as_str) + else { + return Some("range request carried no textDocument.uri".into()); + }; + let Some(text) = open_docs.get(uri) else { + return Some(format!("no didOpen text recorded for {uri}")); + }; + let (mut line, mut col) = (0u64, 0u64); + for ch in text.chars() { + if ch == '\n' { + line += 1; + col = 0; + } else { + col += ch.len_utf16() as u64; + } + } + let end = params.get("range")?.get("end")?; + let end_line = end.get("line")?.as_u64()?; + let end_col = end.get("character")?.as_u64()?; + if end_line > line || (end_line == line && end_col > col) { + Some(format!( + "invalid utf-16 range end {end_line}:{end_col}; document ends at {line}:{col}" + )) + } else { + None + } +} + fn inlay_range_error( params: &serde_json::Value, open_docs: &HashMap, diff --git a/src/lsp.rs b/src/lsp.rs index e777c98..9849b3e 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -2003,11 +2003,16 @@ impl LspManager { diagnostics: &[Value], ) -> Result { let uri = uri.into(); + // Same encoding rule as every outbound range: columns are pmacs + // byte offsets and must convert to the negotiated units — a + // UTF-16 server receiving byte columns resolves the wrong range + // for non-ASCII lines (same bug class as the semantic-token + // range fix in this change). let params = json!({ "textDocument": { "uri": uri.clone() }, "range": { - "start": { "line": start_line, "character": start_col }, - "end": { "line": end_line, "character": end_col }, + "start": self.outbound_position(sid, &uri, start_line, start_col), + "end": self.outbound_position(sid, &uri, end_line, end_col), }, "context": { "diagnostics": diagnostics }, }); @@ -2038,7 +2043,7 @@ impl LspManager { "textDocument": { "uri": uri.clone() }, "range": { "start": self.outbound_position(sid, &uri, start_line, start_col), - "end": self.outbound_position(sid, &uri, end_line, end_col), + "end": self.outbound_position(sid, &uri, end_line, end_col), }, }); let req_id = self.send_request(sid, "textDocument/inlayHint", params)?; @@ -2081,11 +2086,17 @@ impl LspManager { end_col: u32, ) -> Result { let uri = uri.into(); + // Columns arrive as pmacs byte offsets; convert per the + // negotiated position encoding, exactly like the inlay-hint + // range. A UTF-16 server receiving raw byte columns gets an + // invalid end character for non-ASCII text ("é" is two bytes + // but one UTF-16 unit) and may reject the request — fatal for + // a range-only provider whose ONLY pull path this is. let params = json!({ "textDocument": { "uri": uri.clone() }, "range": { - "start": { "line": start_line, "character": start_col }, - "end": { "line": end_line, "character": end_col }, + "start": self.outbound_position(sid, &uri, start_line, start_col), + "end": self.outbound_position(sid, &uri, end_line, end_col), }, }); let req_id = self.send_request(sid, "textDocument/semanticTokens/range", params)?; diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 8d4dd16..bb8b77f 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -4582,6 +4582,59 @@ fn arc1c_range_only_server_is_served_range_requests() { ); } +/// Arc 1c review fix — a range-only server that negotiated UTF-16. +/// The whole-document range's columns are derived from pmacs byte +/// offsets; they must go through `outbound_position` like every other +/// outbound position. The last line ends in non-ASCII ("é" = 2 bytes, +/// 1 UTF-16 unit), and the fake validates the end bound strictly in +/// UTF-16 units — raw byte columns overshoot and are rejected, leaving +/// the store empty and this test failing. +#[test] +fn arc1c_range_only_utf16_server_gets_converted_bounds() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let a_path = dir.path().join("a.rs"); + std::fs::write(&a_path, "fn a() {}\nlet x = \u{e9}\u{e9};".as_bytes()).expect("write a"); + let a_disp = a_path.display().to_string(); + + let mut state = EditorState::new(); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ + command = '{fake}', + env = {{ PMACS_FAKE_LSP_MODE = 'rangeonly16' }}, + }}" + )) + .exec() + .expect("override rust config"); + state + .lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open('{a_disp}')")) + .exec() + .expect("open a.rs"); + + let has_tokens = format!( + "(function() \ + local sid \ + for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then sid=r.id end \ + end \ + if not sid then return false end \ + local t = pmacs.semantic_tokens.tokens(sid, 'file://{a_disp}') \ + return t ~= nil and #t > 0 \ + end)()" + ); + assert!( + pump_lua_flag(&mut state, &has_tokens, 5), + "a UTF-16 range-only server must receive converted (not byte) columns" + ); +} + /// Arc 1d — a server-declared NON-ASCII trigger character works. LSP /// trigger characters are strings; the fake declares "«" (2 UTF-8 /// bytes), and the codepoint-aware `char_before` must match it.