fix(persistence): symlink confinement, real test-inertness, view_top restore

Addresses the PR #98 review:

- HIGH symlink escape: resolve() did only a lexical starts_with, so a
  base/autosave symlink -> /tmp/out let state.write("autosave/x") write
  outside the state dir. Now every existing component the key adds under
  base is lstat'd and a symlink (live OR broken) is rejected; base itself
  may still be a symlink (dotfile-managed ~/.local/state). Unix symlink
  escape test added (live + broken + plain-subdir-ok).

- MEDIUM integration-test state leak: the state/history dir wiring moved
  out of EditorState::new() into EditorState::install_state_dirs(),
  called only by the real entry points (editor::run, run_daemon). Unit
  AND integration tests construct EditorState directly, so they never
  configure a real dir -> default-on recentf/saveplace write nothing to
  ~/.local/state/pmacs during cargo test. The inertness test now asserts
  a bare new() leaves StateDir unconfigured (direct proof).

- MEDIUM saveplace never recorded view_top: exposed the missing
  pmacs.editor.view_top() getter (set_view_top existed but no getter, so
  the Lua stored 0). saveplace now records+restores the viewport;
  acceptance asserts view_top restores, not just the cursor byte.

- MEDIUM/LOW relative XDG_STATE_HOME / PMACS_STATE_HOME: a relative
  value rooted state at a cwd-relative pmacs/... (same footgun class as
  the empty case). Both are now required absolute; relative values are
  ignored (XDG falls through to HOME). Test added.

- LOW trailing blank line at recentf.lua EOF (git diff --check).

Gates: fmt + workspace clippy clean; lib 1483; crdt 1654; persistence 5;
m4 90; m8_1/m8_2 daemon 10/15; query-replace/completion/listview/overlay/
cua green; GPU 58; git diff --check clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

builtin/runtime/recentf.lua

@@ -83,4 +83,3 @@ pmacs.command.define {
 }
 
 pmacs.keymap.bind { scope = "global", sequence = "C-x C-r", command = "recent-files" }
-

builtin/runtime/saveplace.lua

@@ -57,7 +57,7 @@ local function record_active()
   if not active_ready() then return end
   local path = pmacs.editor.file_path()
   local cursor = pmacs.editor.cursor()
-  local view_top = pmacs.editor.view_top and pmacs.editor.view_top() or 0
+  local view_top = pmacs.editor.view_top()
   local list, index = load_places()
   if index[path] then table.remove(list, index[path]) end
   table.insert(list, 1, { path = path, cursor = cursor, view_top = view_top })
@@ -73,7 +73,7 @@ local function restore_active()
   if not i then return end
   local e = list[i]
   pmacs.editor.goto_byte(e.cursor)
-  if pmacs.editor.set_view_top then pmacs.editor.set_view_top(e.view_top) end
+  pmacs.editor.set_view_top(e.view_top)
 end
 
 -- Record on save and on quit; restore on open. before-save /

src/daemon.rs

@@ -466,6 +466,8 @@ pub fn run_daemon(socket_path: PathBuf, instance_name: Option<String>) -> Result
     // The editor outlives any single attachment; constructed once on
     // the dispatcher thread and used until daemon shutdown.
     let mut editor = EditorState::new();
+    // Real session: wire up on-disk persistence (history + pmacs.state).
+    editor.install_state_dirs();
     // Mirror the daemon's `--socket NAME` and start time into the
     // editor's `LocalInstanceInfo` so `pmacs.instance.identity()`
     // (T M5.6f) reports the same identity the daemon hands back over

src/editor.rs

@@ -163,25 +163,14 @@ impl EditorState {
         lua_host
             .attach_editor(&core)
             .expect("editor bindings + builtin chunks");
-        // Resolve the on-disk history directory before user config
-        // runs (so the user could in principle override it from
-        // `init.lua`). Skipped in test mode for the same reason as
-        // user config: don't touch the developer's real state dir.
-        #[cfg(not(test))]
-        {
-            if let Some(dir) = crate::minibuffer::user_history_dir() {
-                core.borrow_mut().minibuffer.history_dir = Some(dir);
-            }
-            // Arc 3 (Q#PS2): configure the `pmacs.state.*` base dir. Its
-            // absence under `cfg(test)` (this whole block is skipped) is
-            // what keeps default-on saveplace/recentf from writing to a
-            // developer's real state dir during the lib suite.
-            if let Some(dir) = crate::state::user_state_dir() {
-                lua_host
-                    .lua()
-                    .set_app_data(crate::lua_bindings::StateDir(dir));
-            }
-        }
+        // The on-disk state dirs (minibuffer history + pmacs.state) are
+        // deliberately NOT configured here — see `install_state_dirs`,
+        // called by the real entry points (`run` / `run_daemon`) only.
+        // Constructing an `EditorState` — which unit AND integration
+        // tests do directly — leaves them unconfigured, so default-on
+        // persistence (recentf/saveplace) writes nothing to a
+        // developer's real state dir during `cargo test`. Tests that
+        // exercise persistence inject a `StateDir` app-data explicitly.
         // The async runtime: install pmacs._async raw helpers, then
         // load the friendly Lua surface (`pmacs.async`, Handle class,
         // `pmacs.workers.*`). Both must run before user config so a
@@ -457,6 +446,24 @@ impl EditorState {
             .eval(Some("@pmacs/runtime/async.lua:tick"), "pmacs._async.tick()");
     }
 
+    /// Configure the on-disk state directories (minibuffer history +
+    /// `pmacs.state`) from the environment. The **real** entry points
+    /// (`run`, `run_daemon`) call this after construction; tests do not,
+    /// so neither the unit suite nor integration tests (which link the
+    /// lib without `cfg(test)`) touch a developer's real
+    /// `~/.local/state/pmacs`. Honors the `PMACS_STATE_HOME` override
+    /// (see [`crate::state::user_state_dir`]).
+    pub fn install_state_dirs(&self) {
+        if let Some(dir) = crate::minibuffer::user_history_dir() {
+            self.core.borrow_mut().minibuffer.history_dir = Some(dir);
+        }
+        if let Some(dir) = crate::state::user_state_dir() {
+            self.lua_host
+                .lua()
+                .set_app_data(crate::lua_bindings::StateDir(dir));
+        }
+    }
+
     /// Construct an editor for a path. Empty buffer with `[new file]`
     /// status if the path does not exist; loaded contents otherwise.
     pub fn open(path: PathBuf) -> io::Result<Self> {
@@ -1516,6 +1523,8 @@ pub fn run(file: Option<PathBuf>) -> io::Result<()> {
         Some(path) => EditorState::open(path)?,
         None => EditorState::new(),
     };
+    // Real session: wire up on-disk persistence (history + pmacs.state).
+    state.install_state_dirs();
 
     // Post-init dispatch: read whatever init.lua left in the
     // RequestedAttach slot and decide whether to run local or hand

src/lua_bindings/mod.rs

@@ -10768,6 +10768,18 @@ fn install_session(editor: &Table, lua: &Lua, core: &SharedCore) -> mlua::Result
             })?,
         )?;
     }
+    {
+        // view_top(): the active window's first visible source line.
+        // The saveplace getter (Arc 3) — pairs with set_view_top so a
+        // reopen restores the viewport, not just the cursor.
+        let cc = core.clone();
+        editor.set(
+            "view_top",
+            lua.create_function(move |_, ()| {
+                i64::try_from(cc.borrow().view_top()).map_err(mlua::Error::external)
+            })?,
+        )?;
+    }
     {
         // set_view_top(line): set the first visible source line
         // (clamped to the buffer's line count) — desktop restore.

src/state.rs

@@ -25,11 +25,21 @@ use std::path::PathBuf;
 /// `None` rather than a relative path.
 #[must_use]
 pub fn state_dir(xdg_state: Option<&OsStr>, home: Option<&OsStr>) -> Option<PathBuf> {
+    // `XDG_STATE_HOME` must be an absolute path per the XDG spec; a
+    // relative value would root state at a *cwd-relative* `pmacs/...`
+    // (the same latent bug the empty case had). Ignore relative values
+    // and fall through to `HOME`, which likewise must be absolute.
     if let Some(xdg) = xdg_state.filter(|s| !is_blank(s)) {
-        return Some(PathBuf::from(xdg).join("pmacs"));
+        let p = PathBuf::from(xdg);
+        if p.is_absolute() {
+            return Some(p.join("pmacs"));
+        }
     }
-    home.filter(|s| !is_blank(s))
-        .map(|h| PathBuf::from(h).join(".local").join("state").join("pmacs"))
+    home.filter(|s| !is_blank(s)).and_then(|h| {
+        let p = PathBuf::from(h);
+        p.is_absolute()
+            .then(|| p.join(".local").join("state").join("pmacs"))
+    })
 }
 
 /// Resolve the base state directory from the process environment.
@@ -46,7 +56,12 @@ pub fn user_state_dir() -> Option<PathBuf> {
         .as_deref()
         .filter(|s| !is_blank(s))
     {
-        return Some(PathBuf::from(over).join("pmacs"));
+        // The override must also be absolute — a relative redirect would
+        // reintroduce the cwd-relative-state footgun.
+        let p = PathBuf::from(over);
+        if p.is_absolute() {
+            return Some(p.join("pmacs"));
+        }
     }
     state_dir(
         std::env::var_os("XDG_STATE_HOME").as_deref(),
@@ -113,18 +128,37 @@ pub fn validate_name(name: &str) -> Result<(), &'static str> {
     Ok(())
 }
 
-/// Resolve a validated key to its absolute path under `base`, with a
-/// canonical-prefix belt: the joined path must still start with `base`.
+/// Resolve a validated key to its path under `base`, refusing any
+/// route that could escape the state directory.
+///
+/// Two guards beyond [`validate_name`]'s lexical rules:
+/// 1. a `starts_with(base)` belt (redundant with `validate_name`, kept
+///    as defense in depth);
+/// 2. **symlink confinement** — every *existing* component the key adds
+///    under `base` is `lstat`'d, and a symlink (live *or* broken) is
+///    rejected. Without this, a `base/autosave` symlink pointing at
+///    `/tmp/out` would let `state.write("autosave/x", …)` write outside
+///    `base` — the lexical check alone can't catch it. `base` itself may
+///    be a symlink (a dotfile-managed `~/.local/state`); only the
+///    components the *key* contributes are guarded.
 ///
 /// # Errors
-/// Propagates [`validate_name`], or errors if the join escapes `base`
-/// (which [`validate_name`] already prevents — this is defense in depth).
+/// Propagates [`validate_name`], or errors on an escaping / symlinked key.
 pub fn resolve(base: &Path, name: &str) -> Result<PathBuf, &'static str> {
     validate_name(name)?;
     let path = base.join(name);
     if !path.starts_with(base) {
         return Err("state key escapes the state directory");
     }
+    let mut cur = base.to_path_buf();
+    for part in name.split('/') {
+        cur.push(part);
+        if let Ok(meta) = std::fs::symlink_metadata(&cur)
+            && meta.file_type().is_symlink()
+        {
+            return Err("state key traverses a symlink");
+        }
+    }
     Ok(path)
 }
 
@@ -253,6 +287,43 @@ mod tests {
         assert!(resolve(&base, "../escape").is_err());
     }
 
+    #[cfg(unix)]
+    #[test]
+    fn resolve_rejects_symlink_components() {
+        let root = std::env::temp_dir().join(format!("pmacs-symlink-{}", std::process::id()));
+        let base = root.join("pmacs");
+        let outside = root.join("outside");
+        std::fs::create_dir_all(&base).unwrap();
+        std::fs::create_dir_all(&outside).unwrap();
+
+        // A live symlink `base/evil -> outside` must be refused (else a
+        // write through it escapes the state dir).
+        let evil = base.join("evil");
+        std::os::unix::fs::symlink(&outside, &evil).unwrap();
+        assert!(resolve(&base, "evil/x").is_err(), "live symlink escape");
+        assert!(write(&base, "evil/x", b"nope").is_err());
+        assert!(!outside.join("x").exists(), "nothing was written outside");
+
+        // A broken symlink component is also refused (lstat sees it).
+        let broken = base.join("broken");
+        std::os::unix::fs::symlink(root.join("does-not-exist"), &broken).unwrap();
+        assert!(resolve(&base, "broken/y").is_err(), "broken symlink escape");
+
+        // A plain subdir is fine.
+        assert!(resolve(&base, "autosave/ok").is_ok());
+        std::fs::remove_dir_all(&root).ok();
+    }
+
+    #[test]
+    fn relative_xdg_and_home_are_ignored() {
+        // A relative XDG_STATE_HOME (spec violation) must not root state
+        // at a cwd-relative path; it falls through to HOME.
+        let d = state_dir(Some(OsStr::new("relstate")), Some(OsStr::new("/home/u"))).unwrap();
+        assert_eq!(d, PathBuf::from("/home/u/.local/state/pmacs"));
+        // Relative XDG and relative HOME → None, never a relative root.
+        assert!(state_dir(Some(OsStr::new("rel")), Some(OsStr::new("relhome"))).is_none());
+    }
+
     #[test]
     fn write_read_remove_round_trip() {
         let dir = std::env::temp_dir().join(format!("pmacs-state-{}", std::process::id()));

tests/persistence_acceptance.rs

@@ -12,6 +12,7 @@
 
 use pmacs::editor::EditorState;
 use pmacs::lua_bindings::StateDir;
+use std::fmt::Write as _;
 use std::path::PathBuf;
 
 /// A fresh editor whose state dir is a private, empty tempdir (unique
@@ -72,9 +73,16 @@ fn state_round_trips_and_rejects_escapes() {
 
 #[test]
 fn state_is_inert_when_unconfigured() {
+    // A plain EditorState::new() must NOT configure a state dir — that
+    // is what keeps the whole integration-test suite (which links the
+    // lib without cfg(test)) from writing to a developer's real
+    // ~/.local/state/pmacs. Only the real entry points call
+    // install_state_dirs(); tests construct EditorState directly.
     let s = EditorState::new();
-    // Simulate no state dir (the cfg(test) lib case, or no HOME).
-    s.lua_host.lua().remove_app_data::<StateDir>();
+    assert!(
+        s.lua_host.lua().app_data_ref::<StateDir>().is_none(),
+        "new() must leave the state dir unconfigured"
+    );
     let (avail, wrote, read): (bool, bool, Option<String>) = s
         .lua_host
         .lua()
@@ -116,29 +124,38 @@ fn recentf_records_dedups_and_orders_mru() {
 }
 
 #[test]
-fn saveplace_restores_cursor_on_reopen() {
+fn saveplace_restores_cursor_and_view_top_on_reopen() {
     let (s, dir) = editor_with_state_dir();
-    let f = write_file(&dir, "place.rs", "line0\nline1\nline2\nline3\n");
-    // Open, move to byte 12 ("line2"), save (before-save records the
-    // place), then KILL the buffer — so the reopen is a fresh load
+    // A tall file so view_top can be non-zero.
+    let mut body = String::new();
+    for i in 0..40 {
+        let _ = writeln!(body, "line{i}");
+    }
+    let f = write_file(&dir, "place.rs", &body);
+    // Open, scroll to view_top 7, put the cursor at byte 48 (start of
+    // "line8", within that viewport), save (before-save records the
+    // place), then KILL — so the reopen is a fresh load
     // (buffer.after-load), the cross-session path saveplace targets.
-    let cursor: i64 = s
+    // "line0\n".."line9\n" are 6 bytes each → line8 begins at byte 48.
+    let (cursor, view_top): (i64, i64) = s
         .lua_host
         .lua()
         .load(format!(
             r#"
             local b = pmacs.buffer.find_or_open({f:?})
-            pmacs.editor.goto_byte(12)
+            pmacs.editor.set_view_top(7)
+            pmacs.editor.goto_byte(48)
             pmacs.command.invoke("buffer.save")
             pmacs.buffer.kill(b)
             -- Reopen from scratch: after-load fires → saveplace restores.
             pmacs.buffer.find_or_open({f:?})
-            return pmacs.editor.cursor()
+            return pmacs.editor.cursor(), pmacs.editor.view_top()
             "#
         ))
         .eval()
         .expect("place + save + kill + reopen");
-    assert_eq!(cursor, 12, "saveplace restored the cursor byte on reload");
+    assert_eq!(cursor, 48, "saveplace restored the cursor byte on reload");
+    assert_eq!(view_top, 7, "saveplace restored the viewport (view_top)");
     std::fs::remove_dir_all(&dir).ok();
 }