fix(lsp): decline a non-UTF-8 canonicalization; round 1 review

Six findings from review, one of them a real defect.

**`canonicalize` could emit a path that exists nowhere.**
`p.display().to_string()` substitutes U+FFFD for non-UTF-8 bytes, so a
resolution landing on such a path returned a plausible-looking string
that does not exist on disk — worse than nil, because this value becomes
a server-affinity key through `file_uri_for` and would silently fail to
round-trip, while the doc promised nil for anything unresolvable. Now
`.and_then(|p| p.to_str().map(str::to_owned))`: unrepresentable is a
decline, matching how the fs layer already treats non-UTF-8 symlink
targets.

Pinned by a new acceptance case that reaches a non-UTF-8 target through
an **ASCII** symlink, so the input is representable and only the
resolved output is not — the case a UTF-8 check on the argument would
miss. Bitten: restoring `display()` fails it.

The other five:

- `on_response`'s doc comment now warns that registering against a
  server with no attached buffer is fire-on-death, not fire-on-reply,
  because the drain visits only attached sids. It looks exactly like a
  hung request while debugging, and 3b is the first caller likely to
  hit it.
- `deliver_response`'s comment still carried the pre-correction
  rationale ("removed BEFORE invocation ... must not be re-entered") —
  the claim the bite disproved. It now says what is true: removal is
  unconditional, before-vs-after is unobservable without a re-entrant
  drain, and the reachable bug is gating removal on a clean return.
- Dropped `server_attempt`'s unused second return.
- Deleted a vacuous assertion in the no-attachment test (counting `_G`
  entries to assert "lua globals are readable") — scaffolding that
  pinned nothing, the exact shape the project's own lesson flags.
- `#[cfg(unix)]` on the three symlink-dependent tests.

Gates re-run in full. The sweep's first pass tripped
`composition_overhead_under_ten_percent` at 18.8% against a 10% budget;
it passes 3/3 in isolation here, passes in isolation on main, and the
same run reported the realistic-frame overhead as **-4.6%** — a negative
figure is measurement noise, not added work. Nothing in this diff is on
the render path. Rerun of the full sweep: 3,189 across 93 suites, zero
failures.

builtin/runtime/lsp.lua

@@ -1584,10 +1584,10 @@ local function server_attempt(sid)
   local skey = tostring(sid)
   for _, info in ipairs(pmacs.lsp.list()) do
     if tostring(info.id) == skey then
-      return info.attempt or 0, info.state and info.state.kind
+      return info.attempt or 0
     end
   end
-  return nil, nil
+  return nil
 end
 
 -- fn(sid, params); persistent, fires for every server.
@@ -1605,6 +1605,15 @@ end
 
 -- fn(result, err); ONE-SHOT, keyed to the exact request.
 -- `request_id` is what `pmacs.lsp.send_request` returned.
+--
+-- **Register only against a server with an attached buffer.** The drain
+-- that delivers replies visits only sids present in `attachments`, so a
+-- one-shot on an unattached server will not fire on its reply — the
+-- reply sits in that server's queue and the handler is invoked only when
+-- the purge below decides the server is gone. That is fire-on-death, not
+-- fire-on-reply, and it looks exactly like a hung request while
+-- debugging. The attach path is the ordinary way to get a sid; a
+-- hand-spawned one from `init.lua` is the case to watch.
 function pmacs.lsp.on_response(sid, request_id, fn)
   if not sid or type(request_id) ~= "number" or type(fn) ~= "function" then
     error("pmacs.lsp.on_response(sid, request_id, fn): want sid, number, function")
@@ -1641,8 +1650,13 @@ local function deliver_response(sid, ev)
   if not pend then return end
   local entry = pend[ev.request_id]
   if not entry then return end
-  -- Removed BEFORE invocation: a handler that raises must not be
-  -- re-entered by a later event carrying the same id.
+  -- Removed UNCONDITIONALLY, so a handler that raises is still retired
+  -- and cannot be invoked a second time by the purge. Removing first is
+  -- the defensive order and costs nothing, but it is not what defends
+  -- against re-invocation: `pcall` catches the raise either way, so
+  -- before-vs-after is unobservable without a re-entrant drain. The
+  -- reachable bug is gating removal on a clean return, which acceptance
+  -- 32 bites (2 != 1).
   pend[ev.request_id] = nil
   if next(pend) == nil then pending_responses[skey] = nil end
   local ok, err = pcall(entry.fn, ev.result, ev.error)

src/lua_bindings/mod.rs

@@ -6550,9 +6550,18 @@ pub fn install_async(
                 // symlink is ordinary, and raising would surface through
                 // `resolve_root_fn`'s pcall as a config bug, which it is
                 // not.
+                //
+                // `to_str`, NOT `display()`. A resolution that lands on
+                // non-UTF-8 bytes has no faithful string form, and
+                // `display()` would substitute U+FFFD and hand back a
+                // path that does not exist on disk — strictly worse than
+                // nil here, because this value becomes a server-affinity
+                // key via `file_uri_for` and would silently fail to
+                // round-trip. Unrepresentable is a decline, matching how
+                // the fs layer already treats non-UTF-8 symlink targets.
                 Ok(std::fs::canonicalize(&path)
                     .ok()
-                    .map(|p| p.display().to_string()))
+                    .and_then(|p| p.to_str().map(str::to_owned)))
             })?,
         )?;
         pmacs.set("_fs", fs_priv)?;

tests/lsp_dispatch_seams_acceptance.rs

@@ -493,12 +493,6 @@ fn acc34_purge_reaches_a_server_that_is_in_no_attachment() {
     );
     settle(&mut state);
 
-    let attached: i64 = eval(
-        &state,
-        "local n = 0 for _ in pairs(_G) do n = n + 1 end return n",
-    );
-    assert!(attached > 0, "lua globals are readable");
-
     exec(
         &state,
         r#"
@@ -528,6 +522,7 @@ fn acc34_purge_reaches_a_server_that_is_in_no_attachment() {
 // ---------------------------------------------------------------------------
 
 #[test]
+#[cfg(unix)]
 fn acc34a_canonicalize_resolves_symlinks_and_dot_segments() {
     let fx = Fixture::new();
     fx.write("pkg/sub/a.txt", "x\n");
@@ -576,6 +571,7 @@ fn server_count(state: &EditorState) -> i64 {
 }
 
 #[test]
+#[cfg(unix)]
 fn acc34b_canonicalizing_resolver_reuses_one_server_across_a_symlink() {
     let fx = Fixture::new();
     fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n");
@@ -627,6 +623,7 @@ fn acc34b_canonicalizing_resolver_reuses_one_server_across_a_symlink() {
 }
 
 #[test]
+#[cfg(unix)]
 fn acc34b_falsified_by_a_resolver_that_skips_canonicalization() {
     let fx = Fixture::new();
     fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n");
@@ -671,3 +668,47 @@ fn acc34b_falsified_by_a_resolver_that_skips_canonicalization() {
          spawn two servers — this is what 34b's positive case rules out"
     );
 }
+
+// ---------------------------------------------------------------------------
+// Acceptance 34a, non-UTF-8 arm — an unrepresentable resolution declines
+// rather than returning a lossy string.
+//
+// Review finding on PR #167: `display().to_string()` substitutes U+FFFD,
+// which would hand back a path that does not exist on disk. That is
+// strictly worse than nil here, because the value becomes a
+// server-affinity key via `file_uri_for` and would silently fail to
+// round-trip. Bites against the `display()` form, which returns a
+// non-nil string for this fixture.
+// ---------------------------------------------------------------------------
+
+#[test]
+#[cfg(unix)]
+fn acc34a_canonicalize_declines_a_non_utf8_resolution() {
+    use std::ffi::OsStr;
+    use std::os::unix::ffi::OsStrExt as _;
+
+    let fx = Fixture::new();
+    // 0xFF is not valid UTF-8 in any position.
+    let raw = OsStr::from_bytes(b"bad-\xffname");
+    let target = fx.root.join(raw);
+    std::fs::write(&target, "x\n").unwrap();
+    // Reached through an ASCII symlink, so the *input* is representable
+    // and only the resolved output is not — which is the case
+    // `to_str()` has to catch and a UTF-8-only input check would miss.
+    let link = fx.dir("ascii-link");
+    std::os::unix::fs::symlink(&target, &link).unwrap();
+
+    let state = editor();
+    let got: String = eval(
+        &state,
+        &format!(
+            "return tostring(pmacs.fs.canonicalize(\"{}\"))",
+            lua_str(&link)
+        ),
+    );
+    assert_eq!(
+        got, "nil",
+        "a resolution that lands on non-UTF-8 bytes must decline, not \
+         return a U+FFFD-substituted path that exists nowhere"
+    );
+}