From 349f13f67435d8724e950b9ac71faeabf56cce24 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 8 Jul 2026 17:33:25 -0400 Subject: [PATCH 1/3] docs: persistence framing (Arc 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Q#PS1-9: hybrid (4 thin Rust primitives + Lua policy). state_dir() + path-confined pmacs.state.{read,write,remove,path} (empty-XDG fix, not a pure refactor); per-buffer file_path; goto_byte/set_view_top; pmacs.session.save_desktop/restore_desktop (Rust serde — layout mirror + active-leaf preorder index). Line-based state (no Lua JSON codec). Deferred restore (armed by desktop_mode, triggered post-file-routing only when no file arg). Encoded session key (name:/cwd:). recentf MRU on after-load + after-switch. Default-on saveplace/recentf with enable-knobs + cfg(test) write-inertness. Incorporates the 2026-07-08 review findings. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/persistence-framing.md | 295 ++++++++++++++++++++++++++++++++++++ 1 file changed, 295 insertions(+) create mode 100644 docs/persistence-framing.md diff --git a/docs/persistence-framing.md b/docs/persistence-framing.md new file mode 100644 index 0000000..3269cf2 --- /dev/null +++ b/docs/persistence-framing.md @@ -0,0 +1,295 @@ +# Persistence — framing (Arc 3) + +pmacs forgets everything on exit except minibuffer history. Reopen a +file and the cursor is at the top; restart and your open buffers, +splits, and recently-visited files are gone; a crash loses unsaved +work with no recovery. This arc adds the four classic session- +persistence features — **saveplace** (cursor memory), **recentf** +(recent files), **desktop-save** (buffers + layout + positions), and +**autosave + crash recovery** — generalizing the one persistence +pattern that already works (`$XDG_STATE_HOME/pmacs/` history). + +Roadmap: `docs/roadmap-2026-07.md` Arc 3, including its open question +"what is a 'session' in a daemon world" (Q#PS6). + +## What already exists (verified) + +- **State-dir precedent** (`src/minibuffer.rs`): `resolve_history_dir` + → `$XDG_STATE_HOME/pmacs/history` (fallback + `~/.local/state/pmacs/history`); `load_history_file` / + `append_history_file` (newline-delimited, `create_dir_all` on + write). Env is passed as args, never read inline — the + `#![forbid(unsafe_code)]` discipline. **But the `/history` segment + is baked in; there is no generic `state_dir()`** returning the bare + `.../pmacs/` base (Q#PS2). +- **Buffer/path model**: `Buffer.file_path: Option` (`None` + for scratch/unsaved) + `file_meta: Option`; registry + `ids()` is stable insertion order. Open flow is Lua + (`pmacs.buffer.find_or_open` → `file_io::load_file` → + `set_buffer_path`/`set_buffer_meta` → `buffer.after-load`). **No + `is_special` flag** — file buffers are just `file_path().is_some()`. + **Gap**: `pmacs.buffer.list()` yields ids with no per-id + `file_path()` method; only `pmacs.editor.file_path()` (active + buffer) reads a path. +- **Layout** (`src/window.rs`): `LayoutNode = Leaf(WindowId) | + Split{orientation, weights: Vec, children}`; ratios are the + integer weights (survive resize). **Not serde; `WindowId` is a + process counter (not restart-stable)** — a saved layout must key + leaves by *path + cursor + view_top* and rebuild structurally. Lua + window API is splits + switch only (can't read/build an arbitrary + tree). +- **Cursor/view**: per-`Window` `cursor: Position` + `view_top`; + `switch_active_buffer` **zeroes both** (restore must run after + open). Reads exist (`pmacs.editor.cursor()` byte); **the only Lua + setter is line-based `move_to_line`** — no byte-offset setter, no + real `set_view_top`. +- **File I/O** (`src/file_io.rs`, complete): `save_atomic` (temp + + rename + fsync, mode-preserving), `load_file`, `current_meta`; + `FileMeta{mtime,size}` is the external-change detector. +- **Cadence**: `process.after-tick` hook (every frame — needs a + `monotonic_ms()` throttle) *or* `pmacs.async` + `pmacs.workers.sleep` + (the `fs.watch` pattern — cleaner for a fixed interval). +- **Session identity**: `pmacs.instance.identity()` → `{ instance_name, + working_directory }`. `--socket work` vs `--socket personal` give + distinct `instance_name`s; `instance_name` is `None` for the default + daemon / in-process (fall back to a `working_directory` hash). +- **Recentf / saveplace / desktop / autosave: confirmed absent.** + +## Decisions + +### Q#PS1 — Hybrid: four thin Rust primitives + Lua policy + +Orchestration is Lua-friendly (enumerate via `pmacs.buffer.list`, +observe via `buffer.after-load` / `buffer.before-save` / +`editor.before-quit` hooks, cadence via `pmacs.async`, session key via +`pmacs.instance.identity`). But Lua can't do the load-bearing parts. +Add exactly four Rust surfaces; keep serialization + policy + cadence +in Lua builtin modules: + +1. **`state_dir()` + `pmacs.state.{read,write,remove,path}`** — a + generic, **path-confined** key→file store under the generalized + state dir, atomic-write via `file_io::save_atomic` (avoids the + audit-flagged raw `io.open`). `remove` is needed by autosave + cleanup (Q#PS8). +2. **per-`BufferId` `file_path()`** getter (desktop-save enumerates + file buffers without switching to each). +3. **`pmacs.editor.goto_byte(pos)` + `set_view_top(n)`** — byte-exact + restore (saveplace/desktop), since switch zeroes both. +4. **`pmacs.session.save_desktop()` / `restore_desktop()`** — the + layout serde mirror + structural rebuild live in Rust (WindowIds + aren't restart-stable; the tree can't be rebuilt from the thin Lua + split API). This one primitive owns buffer-set + layout + cursor + serialization end-to-end. + +### Q#PS2 — `state_dir()` generalization + `pmacs.state.*` (path-confined) + +Factor `state_dir(xdg_state, home) -> Option` returning +`.../pmacs`; rewrite `resolve_history_dir` as +`state_dir().map(|d| d.join("history"))`. **This is NOT a +behavior-preserving refactor at one edge** (an empty `XDG_STATE_HOME`): +the current `resolve_history_dir` treats `Some("")` as present and +returns a *relative* `pmacs/history` (writes into the cwd — a latent +bug). `state_dir()` **fixes this deliberately**: an empty/blank +`XDG_STATE_HOME` is treated as absent and falls through to +`~/.local/state/pmacs`. A test pins the new empty-XDG behavior; history +persistence keeps working (its own tests are the guard). Bet #3 is +scored on this being the *only* observable change to history. + +`pmacs.state.{write(name,str), read(name)->str?, remove(name), +path(name)}` over `state_dir().join(name)`, atomic write + +`create_dir_all`. **Name confinement (High):** a name is a *relative* +key — reject absolute paths, empty names, any `.`/`..` component, +leading/trailing/`//` separators, and control chars; the only allowed +shape is one-or-more components of `[A-Za-z0-9._-]+` joined by `/` +(so `recentf`, `places`, `autosave/` pass; `../x`, `/etc/x`, +`a//b`, `` all reject). The resolved path is additionally asserted to +start with `state_dir()` (canonical-prefix belt). Without this, +`pmacs.state` would be the arbitrary-io primitive it exists to avoid. +Reads/writes are **no-ops when the state dir is unconfigured** — +which is the case under `cfg(test)` (Q#PS9), so default-on builtins +never touch a developer's real `$XDG_STATE_HOME` in `cargo test`. + +### Q#PS3 — Serialization: line-based text for Lua state; Rust serde for desktop + +Decided up front so phase 1 doesn't discover a fifth primitive: **Lua +has no public JSON codec** (`lua_to_json` exists but only internally +for the MCP wire), and it doesn't need one. The Lua-owned state is +line-based text, the history-file shape `pmacs.state` already returns: +`recentf` is newline-delimited paths; `places` is one +` ` line per file (numbers first so the path — +which may contain spaces — is the whitespace-split remainder). The one +breaker, a newline inside a path, is pathological and named as +deferred. **desktop-save serializes in Rust** (`pmacs.session.*`, Q#PS5) +with `serde_json` internally — never crossing the Lua boundary — so no +Lua JSON primitive is added anywhere in the arc. + +### Q#PS3b — saveplace (Lua, phase 1) + +`builtin/runtime/saveplace.lua`: on `buffer.before-save` and on +`editor.before-quit`, record `path → {cursor, view_top}` into the +`places` state file (the line format above); on `buffer.after-load`, +look up the active buffer's path and `goto_byte` + `set_view_top`. +LRU-cap the map (~200 files). **On by default** with a disable knob +(Q#PS9). + +### Q#PS4 — recentf (Lua, phase 1) + +`builtin/runtime/recentf.lua`: a handler on **both `buffer.after-load` +(first open) and `buffer.after-switch` (re-visiting an already-open +file buffer)** moves the active buffer's path to the front of a deduped, +capped (~50) `recentf` state file — MRU, so re-visits refresh the +order, not just first loads. A `recent-files` command + binding opens +`minibuffer.read` over the list (the `editor.switch-buffer` shape) → +`find_or_open`. Recording is automatic; the picker is invoked on demand. +(The Arc 1b `listview` panel is an alternative surface if a browsable +list is wanted later.) + +### Q#PS5 — desktop-save (Rust `pmacs.session.*`, phase 2) + +`save_desktop()` serializes `{ session-key, layout mirror +(orientation/weights tree; leaf = {path, cursor byte, view_top}), +active_leaf: usize }` to `state_dir()/desktop/`. **Only +file buffers** (`file_path().is_some()`) — scratch/`*special*` leaves +are dropped from the tree. **The active window is a leaf *preorder +index*, not a path** (Med/high): the same file can appear in multiple +leaves with different cursor/view state, so a path can't identify which +one had focus — the ordinal into the preorder leaf sequence can. + +**Does NOT save buffer contents.** Like Emacs `desktop.el`, it saves +the *file list + positions*, not unsaved edits (that's autosave's job, +Q#PS8). A leaf whose buffer was modified at save time is recorded +**informationally only** — restore opens the on-disk file (clean) and +surfaces a one-line warning ("N buffers had unsaved changes when the +desktop was saved"); it never reconstructs dirty state. `restore_desktop()` +opens each file (`find_or_open`), rebuilds the split tree structurally, +sets each leaf's cursor/view_top, and focuses `active_leaf`. + +**Opt-in** (auto-restore surprises): `pmacs.session.desktop_mode(true)` +in init.lua wires `editor.before-quit` → save and *arms* startup +restore (Q#PS7 — restore is triggered by the entry point, not inline). + +### Q#PS6 — Session key (the daemon question) + +Key on `pmacs.instance.identity()`. **Never use raw `instance_name` as +a filename** (even though `/` is mostly constrained today, other +separators/dots aren't) — use a stable *encoded* key: `name:` +when `instance_name` is set (`--socket NAME`), else `cwd:` of +`working_directory` (Emacs's per-directory desktop model). The +encoding must itself satisfy the Q#PS2 name confinement (it becomes +the `desktop/` subpath), so `` is a hex/percent form or +a hash, never the raw string. So `--socket work` and `--socket +personal` restore different desktops; two plain `pmacs` sessions in +different project dirs likewise; the default daemon in one cwd shares +one desktop. Tests cover both the named-socket key and the +cwd-fallback key. No new identity is needed — the socket name already +threads to `InstanceIdentity`. + +### Q#PS7 — Restore timing (restore is deferred, never inline in init) + +The startup order is the trap: `EditorState::open(path)` calls +`EditorState::new()` first, and `new()` loads `init.lua` **before** the +file is opened (`src/editor.rs`). So `desktop_mode(true)` running inside +init cannot know a positional file arg is coming — if it restored +inline it would clobber (or race) the file the user asked for. + +Therefore **`desktop_mode(true)` does not restore; it arms restore.** +It registers intent (a flag the core reads). The startup entry point — +after `new()`/`open()` has done its file-open routing — calls +`pmacs.session.restore_desktop()` exactly once, and **only when no +positional file arg was given** (a file arg means "open this," not +"restore my desktop" — Emacs's rule). Concretely: `main` threads a +`restore_desktop: bool` (true iff armed AND no file arg) into the +post-construction step that triggers the restore. Restore never runs +from init; init only sets the mode. + +### Q#PS8 — autosave + crash recovery (Lua, phase 3) + +A `pmacs.async` + `sleep(N s)` loop (the `fs.watch` pattern) writes a +recovery copy of each *modified* file buffer to +`state_dir()/autosave/` plus a sidecar recording the origin +path + the on-disk `FileMeta`. On `find_or_open`, if a recovery file +exists and the on-disk file's `FileMeta` matches the sidecar (the file +wasn't changed elsewhere), prompt to recover. A clean save/kill +**deletes the recovery file via `pmacs.state.remove`** (the Q#PS2 +addition — no raw `pmacs.fs.remove(pmacs.state.path(...))`, which would +route around the confinement). Writes use `pmacs.state.write` under the +`autosave/` subpath; no new primitive beyond Q#PS2. + +### Q#PS9 — Default-on policy + the disable knob + test inertness + +saveplace + recentf: **on by default** (low-risk, quietly useful, +Emacs's `save-place-mode`/`recentf-mode` are commonly enabled). +desktop-save + autosave: **opt-in** via init.lua (auto-restore and +background writes are surprising to enable silently). + +Two hard requirements on default-on: + +- **A clear disable knob**: `pmacs.saveplace.enable(false)` / + `pmacs.recentf.enable(false)` (callable from init.lua) short-circuits + the hooks. The modules read an enabled flag at the top of each + handler. +- **No writes in `cargo test`.** The mechanism is already there: the + state dir is *configured once at startup* (like `history_dir`), and + that wiring is **skipped under `cfg(test)`** (the same guard that + skips init.lua loading). With no state dir configured, + `pmacs.state.write/read/remove` are no-ops (Q#PS2), so the default-on + saveplace/recentf hooks fire but touch no disk — the lib suite never + writes a developer's real `$XDG_STATE_HOME`. Acceptance tests that + *do* exercise persistence inject a tempdir state root explicitly (the + `load_user_config_at` precedent). + +## Phasing + +Three PRs; phase 1 delivers standalone value and is validated before +phase 2. + +1. **State foundation + saveplace + recentf.** Rust: `state_dir()` + (+ empty-XDG fix + test) + `pmacs.state.{read,write,remove,path}` + (+ name-confinement rejection tests) + `goto_byte`/`set_view_top`. + Lua: `saveplace.lua` + `recentf.lua` + `recent-files` + command/binding + `enable(false)` knobs. Acceptance (tempdir state + root injected): state round-trip, confinement rejects `../x` + / absolute / control chars, saveplace restores byte position after + reopen, recentf records + dedups + MRU-refreshes on re-visit + the + picker opens; and a check that with no state dir configured the + hooks write nothing. +2. **desktop-save.** Rust: per-buffer `file_path()` + layout serde + mirror + `pmacs.session.save_desktop/restore_desktop` + encoded + session-key. Lua: `desktop_mode` wiring (before-quit / arm-startup). + Acceptance: save→restore reconstructs a nested/asymmetric split + layout with correct buffers + cursors + `active_leaf` focus; the + same-file-in-two-leaves case; named-socket vs cwd-fallback key; + the no-file-arg restore gate; modified-at-save surfaces a warning + and restores clean (never dirty). +3. **autosave + crash recovery.** Lua: async-timer autosave + + recovery-on-open prompt; `FileMeta` external-change guard. + +## Categorical bets (score at close) + +1. **Four Rust primitives are enough** — the Lua policy fills in + saveplace/recentf/autosave and only desktop-save's layout needs + Rust. No fifth surface surfaces mid-arc. +2. **Structural layout rebuild is faithful** — reopening the serde + mirror reconstructs weighted split trees correctly (the untested + claim; nested/asymmetric splits are the risk). +3. **The `state_dir()` change touches history at exactly one edge** — + the deliberate empty-`XDG_STATE_HOME` fix (Q#PS2) is the *only* + observable difference; normal history persistence is unchanged (its + tests + a new empty-XDG test are the guard). Not a pure refactor — + scored on nothing else shifting. +4. **On-by-default saveplace/recentf is unsurprising** — no + "why did my cursor jump" or "what's writing this file" reports. + +## Deferred (named, not silently dropped) + +- Saving unsaved buffer *content* in the desktop (autosave's job). +- Named/multiple desktops per session key (one desktop per key in v1). +- Restoring window-local overlays / minor state (buffers + positions + only). +- Remote/cross-machine desktop (paths are local). +- recentf as a browsable `listview` panel (minibuffer picker in v1). +- Per-project `init.lua` interaction with desktop scoping (post-v0.1, + per `docs/project.md`). +- saveplace for non-file buffers. +- Paths containing a literal newline (the line-based `places`/`recentf` + format breaks on them; pathological, unhandled in v1). From 4dd4b9ab97f9e2cc2f11abb23276f5f7a77d0cb9 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 8 Jul 2026 17:57:01 -0400 Subject: [PATCH 2/3] feat(persistence): state foundation + saveplace + recentf (Arc 3 phase 1) Framing: docs/persistence-framing.md. The four Rust primitives the Lua-vs-Rust scout said were unavoidable, plus two Lua policy modules. Rust: - src/state.rs: state_dir(xdg,home) returning .../pmacs (generalizes the baked-in history path). Deliberate empty-XDG fix (Q#PS2): a blank XDG_STATE_HOME fell through to a RELATIVE pmacs/... path (a cwd-write bug); now treated as absent so it falls to HOME. Confined key->file store: validate_name rejects absolute / .. / empty / // / control chars, plus a canonical-prefix belt; read/write/remove go through file_io::save_atomic, never raw io.open. A PMACS_STATE_HOME override lets CI / privacy-conscious users / integration harnesses redirect all state to a scratch dir. History routed through the shared resolver so it honors the override too. - pmacs.state.{write,read,remove,path,available}: a no-op when the state dir is unconfigured (cfg(test) / no HOME), so default-on builtins write nothing in the lib suite. Configured once at startup like history_dir, skipped under cfg(test). - pmacs.editor.goto_byte / set_view_top: byte-exact restore (switch zeroes the cursor). Lua (builtin/runtime): - saveplace.lua: record the active file's cursor+view_top on before-save / before-quit; restore on after-load. LRU-capped places state file. On by default; pmacs.saveplace.enable(false). - recentf.lua: MRU record on after-load AND after-switch (re-visits refresh the order); deduped/capped recentf file; a recent-files command bound C-x C-r opens the minibuffer picker. Tests: state.rs units (validate/resolve/round-trip/empty-XDG), tests/persistence_acceptance.rs (state round-trip + confinement rejections, inert-when-unconfigured, recentf MRU/dedup, saveplace restore-on-reload, disable knob) injecting a tempdir state root. One describe-hook test made robust to a builtin now subscribing to buffer.before-save. Co-Authored-By: Claude Opus 4.8 (1M context) --- builtin/runtime/recentf.lua | 86 ++++++++++ builtin/runtime/saveplace.lua | 91 ++++++++++ src/editor.rs | 35 +++- src/editor_core.rs | 21 +++ src/lib.rs | 1 + src/lua_bindings/mod.rs | 100 +++++++++++ src/minibuffer.rs | 23 +-- src/state.rs | 294 ++++++++++++++++++++++++++++++++ tests/persistence_acceptance.rs | 167 ++++++++++++++++++ 9 files changed, 801 insertions(+), 17 deletions(-) create mode 100644 builtin/runtime/recentf.lua create mode 100644 builtin/runtime/saveplace.lua create mode 100644 src/state.rs create mode 100644 tests/persistence_acceptance.rs diff --git a/builtin/runtime/recentf.lua b/builtin/runtime/recentf.lua new file mode 100644 index 0000000..c607cf0 --- /dev/null +++ b/builtin/runtime/recentf.lua @@ -0,0 +1,86 @@ +-- recentf.lua --- a most-recently-visited file list (Arc 3 Q#PS4). +-- +-- Records every file buffer opened (buffer.after-load) or re-visited +-- (buffer.after-switch) into a deduped, capped, MRU-ordered `recentf` +-- state file (newline-delimited paths). `M-x recent-files` (bound +-- C-x C-r) opens the list in the minibuffer picker and visits the +-- choice. +-- +-- On by default; disable from init.lua with +-- `pmacs.recentf.enable(false)`. Inert when no state dir is configured +-- (cfg(test) / no HOME). +-- +-- Framing: docs/persistence-framing.md. + +pmacs.recentf = pmacs.recentf or {} + +local STATE_KEY = "recentf" +local MAX_ENTRIES = 50 + +local enabled = true +function pmacs.recentf.enable(on) + enabled = (on ~= false) +end + +local function load_list() + local out = {} + local text = pmacs.state.read(STATE_KEY) + if not text then return out end + for line in text:gmatch("([^\n]+)") do + out[#out + 1] = line + end + return out +end + +-- Move `path` to the front (MRU), dedup, cap. +local function record(path) + if not (enabled and pmacs.state.available()) or not path then return end + local list = load_list() + local kept = { path } + for _, p in ipairs(list) do + if p ~= path and #kept < MAX_ENTRIES then + kept[#kept + 1] = p + end + end + pmacs.state.write(STATE_KEY, table.concat(kept, "\n") .. (#kept > 0 and "\n" or "")) +end + +local function record_active() + pcall(record, pmacs.editor.file_path()) +end + +-- First open and every re-visit of an already-open file refresh MRU. +pmacs.hook.add("buffer.after-load", record_active) +pmacs.hook.add("buffer.after-switch", record_active) + +-- The public list (MRU-first), for the picker or a user script. +function pmacs.recentf.list() + return load_list() +end + +pmacs.command.define { + name = "recent-files", + description = "Visit a recently opened file (Arc 3).", + fn = function() + local list = load_list() + if #list == 0 then + pmacs.editor.set_status("recentf: no recent files") + return + end + pmacs.minibuffer.read { + prompt = "Recent file: ", + source = function() return list end, + history = "recent-files", + on_accept = function(path) + if path == nil or path == "" then return end + local ok, err = pcall(pmacs.buffer.find_or_open, path) + if not ok then + pmacs.editor.set_status("recentf: " .. tostring(err)) + end + end, + } + end, +} + +pmacs.keymap.bind { scope = "global", sequence = "C-x C-r", command = "recent-files" } + diff --git a/builtin/runtime/saveplace.lua b/builtin/runtime/saveplace.lua new file mode 100644 index 0000000..dba7b95 --- /dev/null +++ b/builtin/runtime/saveplace.lua @@ -0,0 +1,91 @@ +-- saveplace.lua --- remember the cursor position per file (Arc 3 Q#PS3b). +-- +-- Records the active file buffer's (cursor byte, view_top) on save and +-- at quit, and restores it when the file is reopened. Storage is the +-- `places` state file, one ` ` line per file +-- (numbers first so the path, which may contain spaces, is the +-- whitespace-split remainder). LRU-capped. +-- +-- On by default; disable from init.lua with +-- `pmacs.saveplace.enable(false)`. Inert when no state dir is +-- configured (cfg(test) / no HOME), so the lib suite writes nothing. +-- +-- Framing: docs/persistence-framing.md. + +pmacs.saveplace = pmacs.saveplace or {} + +local STATE_KEY = "places" +local MAX_ENTRIES = 200 + +local enabled = true +function pmacs.saveplace.enable(on) + enabled = (on ~= false) +end + +local function active_ready() + return enabled and pmacs.state.available() and pmacs.editor.file_path() ~= nil +end + +-- Load the places file into an ordered list of {path, cursor, view_top} +-- (most-recently-recorded first) plus a path->index lookup. +local function load_places() + local list, index = {}, {} + local text = pmacs.state.read(STATE_KEY) + if not text then return list, index end + for line in text:gmatch("([^\n]+)") do + -- " " + local cur, vt, path = line:match("^(%d+)%s+(%d+)%s+(.+)$") + if path and not index[path] then + list[#list + 1] = { path = path, cursor = tonumber(cur), view_top = tonumber(vt) } + index[path] = #list + end + end + return list, index +end + +local function save_places(list) + local lines = {} + for i = 1, math.min(#list, MAX_ENTRIES) do + local e = list[i] + lines[#lines + 1] = string.format("%d %d %s", e.cursor, e.view_top, e.path) + end + pmacs.state.write(STATE_KEY, table.concat(lines, "\n") .. (#lines > 0 and "\n" or "")) +end + +-- Record the active buffer's place, moving it to the front (LRU). +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 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 }) + save_places(list) +end + +-- Restore the just-loaded file's place, if we have one. +local function restore_active() + if not active_ready() then return end + local path = pmacs.editor.file_path() + local list, index = load_places() + local i = index[path] + 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 +end + +-- Record on save and on quit; restore on open. before-save / +-- before-quit are short-circuit hooks — returning nil never vetoes. +pmacs.hook.add("buffer.before-save", function() + pcall(record_active) +end) + +pmacs.hook.add("editor.before-quit", function() + pcall(record_active) +end) + +pmacs.hook.add("buffer.after-load", function() + pcall(restore_active) +end) diff --git a/src/editor.rs b/src/editor.rs index 7656365..4627152 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -172,6 +172,15 @@ impl EditorState { 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 async runtime: install pmacs._async raw helpers, then // load the friendly Lua surface (`pmacs.async`, Handle class, @@ -301,6 +310,22 @@ impl EditorState { include_str!("../builtin/runtime/completion.lua"), ) .expect("load completion builtin chunk"); + // Arc 3: persistence builtins (saveplace + recentf). Load after + // the LSP/completion runtimes; they subscribe to buffer hooks + // and drive `pmacs.state` (inert until the state dir is + // configured — never in `cfg(test)`). + lua_host + .eval( + Some("@pmacs/builtin/runtime/saveplace.lua"), + include_str!("../builtin/runtime/saveplace.lua"), + ) + .expect("load saveplace builtin chunk"); + lua_host + .eval( + Some("@pmacs/builtin/runtime/recentf.lua"), + include_str!("../builtin/runtime/recentf.lua"), + ) + .expect("load recentf builtin chunk"); // T M7.11 bundled-package bootstrap. Through M7.10 the REPL // was loaded directly via `eval(include_str!(...))`; the // M7.11 deliverable migrates it to the package system so it @@ -4128,9 +4153,13 @@ mod tests { assert!(info.get::("source").unwrap().contains(':')); let callbacks: mlua::Table = info.get("callbacks").unwrap(); let len = callbacks.len().unwrap(); - assert_eq!(len, 2, "expected 2 callbacks; describe says {len}"); - let cb1: mlua::Table = callbacks.get(1).unwrap(); - let cb2: mlua::Table = callbacks.get(2).unwrap(); + // A builtin (saveplace) also subscribes to `buffer.before-save`, + // registered at startup, so it precedes the two the test adds. + // Assert on the *last two* callbacks — the ones this chunk just + // registered — rather than the exact total (robust to builtins). + assert!(len >= 2, "expected >= 2 callbacks; describe says {len}"); + let cb1: mlua::Table = callbacks.get(len - 1).unwrap(); + let cb2: mlua::Table = callbacks.get(len).unwrap(); let s1: String = cb1.get("source").unwrap(); let s2: String = cb2.get("source").unwrap(); // Both registrations come from the test chunk; the second diff --git a/src/editor_core.rs b/src/editor_core.rs index 9eae480..1432b75 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -493,6 +493,27 @@ impl EditorCore { self.active_window().view_top } + /// Set the active window's cursor to a byte offset, clamped to the + /// buffer extent (Arc 3 Q#PS1 — saveplace/desktop restore). Resets + /// the goal column. Since `switch_active_buffer` zeroes the cursor, + /// restore calls this *after* the open/switch. + pub fn set_cursor_byte(&mut self, byte: u64) { + let clamped = byte.min(self.active_buffer_len()); + let aw = self.active_window_mut(); + aw.cursor = clamped; + aw.goal_col = None; + } + + /// Set the active window's `view_top` (first visible source line), + /// clamped to the buffer's line count (Arc 3 Q#PS1 — desktop + /// restore). A file that shrank since the desktop was saved can't + /// scroll past its end. + pub fn set_view_top(&mut self, top: usize) { + let lines = self.active_window().text_view.line_count(); + let clamped = top.min(lines.saturating_sub(1)); + self.active_window_mut().view_top = clamped; + } + /// Active buffer's byte length. #[must_use] pub fn active_buffer_len(&self) -> u64 { diff --git a/src/lib.rs b/src/lib.rs index 8e35f45..3e6003f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -123,6 +123,7 @@ pub mod semantic_render; pub mod semantic_tokens; pub mod signature; pub mod socket_path; +pub mod state; pub mod symbol; pub mod syntax; pub mod text_view; diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index b6c97af..874d8e3 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1961,10 +1961,82 @@ pub fn install( pmacs.set("instance", install_instance_module(lua, registry)?)?; pmacs.set("ansi", install_ansi_module(lua)?)?; pmacs.set("packages", install_packages_module(lua)?)?; + pmacs.set("state", install_state_module(lua)?)?; lua.globals().set("pmacs", pmacs)?; Ok(()) } +/// The configured base state directory (Arc 3, Q#PS2). Present as Lua +/// app-data only when a real dir was resolved at startup; its absence +/// (the `cfg(test)` case, and any host without `HOME`/`XDG_STATE_HOME`) +/// makes every `pmacs.state.*` call a no-op, so default-on persistence +/// builtins never touch disk in `cargo test`. +pub struct StateDir(pub std::path::PathBuf); + +/// `pmacs.state.{write,read,remove,path}` — the confined key→file store +/// (Q#PS2). All keys pass [`crate::state::validate_name`], so a state +/// call can never read or write outside the state directory. When the +/// state dir is unconfigured every call is inert: `write`/`remove` +/// return `false`, `read`/`path` return `nil`. +fn install_state_module(lua: &Lua) -> mlua::Result { + let m = lua.create_table()?; + + m.set( + "write", + lua.create_function(|lua, (name, content): (String, mlua::String)| { + let Some(base) = lua.app_data_ref::() else { + return Ok(false); + }; + crate::state::write(&base.0, &name, &content.as_bytes()) + .map_err(mlua::Error::external)?; + Ok(true) + })?, + )?; + + m.set( + "read", + lua.create_function(|lua, name: String| { + let Some(base) = lua.app_data_ref::() else { + return Ok(None); + }; + crate::state::read(&base.0, &name).map_err(mlua::Error::external) + })?, + )?; + + m.set( + "remove", + lua.create_function(|lua, name: String| { + let Some(base) = lua.app_data_ref::() else { + return Ok(false); + }; + crate::state::remove(&base.0, &name).map_err(mlua::Error::external)?; + Ok(true) + })?, + )?; + + m.set( + "path", + lua.create_function(|lua, name: String| { + let Some(base) = lua.app_data_ref::() else { + return Ok(None); + }; + match crate::state::resolve(&base.0, &name) { + Ok(p) => Ok(Some(p.display().to_string())), + Err(e) => Err(mlua::Error::external(e)), + } + })?, + )?; + + // True when a state directory is configured — lets Lua modules tell + // "unconfigured (test / no HOME)" from "configured but empty". + m.set( + "available", + lua.create_function(|lua, ()| Ok(lua.app_data_ref::().is_some()))?, + )?; + + Ok(m) +} + /// Build the `pmacs.attach` Lua function (T M5.6d). /// /// Init-time-only: refuses to run after [`InitCompleteFlag`] has been @@ -10681,6 +10753,34 @@ fn install_session(editor: &Table, lua: &Lua, core: &SharedCore) -> mlua::Result })?, )?; } + { + // goto_byte(pos): set the active cursor to a byte offset + // (clamped). The byte-exact restore saveplace/desktop need + // (Arc 3) — `move_to_line` is line-based, and switch zeroes the + // cursor, so restore sets it here after opening. + let cc = core.clone(); + editor.set( + "goto_byte", + lua.create_function(move |_, pos: i64| { + let byte = u64::try_from(pos).map_err(mlua::Error::external)?; + cc.borrow_mut().set_cursor_byte(byte); + Ok(()) + })?, + )?; + } + { + // set_view_top(line): set the first visible source line + // (clamped to the buffer's line count) — desktop restore. + let cc = core.clone(); + editor.set( + "set_view_top", + lua.create_function(move |_, top: i64| { + let top = usize::try_from(top).map_err(mlua::Error::external)?; + cc.borrow_mut().set_view_top(top); + Ok(()) + })?, + )?; + } { let cc = core.clone(); editor.set( diff --git a/src/minibuffer.rs b/src/minibuffer.rs index 889cc2d..eb8ce00 100644 --- a/src/minibuffer.rs +++ b/src/minibuffer.rs @@ -726,30 +726,25 @@ impl History { /// var is set. #[must_use] pub fn user_history_dir() -> Option { - resolve_history_dir( - std::env::var_os("XDG_STATE_HOME").as_deref(), - std::env::var_os("HOME").as_deref(), - ) + // Route through the shared state-dir resolver so history honors the + // `PMACS_STATE_HOME` override too (Arc 3 Q#PS2). + crate::state::user_state_dir().map(|d| d.join("history")) } /// Pure helper for [`user_history_dir`], factored out so tests can /// inject paths directly without touching the process environment /// (R55: `unsafe_code = "forbid"` rules out `env::set_var`). +/// +/// History lives under the shared editor state dir +/// ([`crate::state::state_dir`], Arc 3 Q#PS2) in a `history/` +/// subdirectory. A blank `XDG_STATE_HOME` now falls through to `HOME` +/// instead of yielding a relative path (the empty-XDG fix). #[must_use] pub fn resolve_history_dir( xdg_state: Option<&std::ffi::OsStr>, home: Option<&std::ffi::OsStr>, ) -> Option { - if let Some(xdg) = xdg_state { - return Some(PathBuf::from(xdg).join("pmacs").join("history")); - } - home.map(|h| { - PathBuf::from(h) - .join(".local") - .join("state") - .join("pmacs") - .join("history") - }) + crate::state::state_dir(xdg_state, home).map(|d| d.join("history")) } fn history_path(dir: &Path, bucket: &str) -> PathBuf { diff --git a/src/state.rs b/src/state.rs new file mode 100644 index 0000000..88a9bea --- /dev/null +++ b/src/state.rs @@ -0,0 +1,294 @@ +// state.rs --- persistent editor state directory (Arc 3, Q#PS2). + +//! The `$XDG_STATE_HOME/pmacs/` base that all persisted editor state +//! lives under: minibuffer history (the original tenant), plus the +//! Arc 3 persistence features (recent files, saveplace, desktop, and +//! autosave recovery). +//! +//! Env is passed in as arguments, never read inline, so the pure +//! resolver is testable without touching the process environment +//! (`#![forbid(unsafe_code)]` rules out `env::set_var`). + +use std::ffi::OsStr; +use std::path::PathBuf; + +/// The base state directory `.../pmacs`, or `None` when neither +/// `XDG_STATE_HOME` nor `HOME` is usably set. +/// +/// Order: `$XDG_STATE_HOME/pmacs`, then `$HOME/.local/state/pmacs`. +/// +/// A **blank** `XDG_STATE_HOME` is treated as *absent* (Q#PS2 fix): the +/// prior history resolver returned a *relative* `pmacs/…` for +/// `Some("")`, which would write state into the process's current +/// directory — a latent bug. Here an empty (or all-whitespace) value +/// falls through to `HOME`, and a `HOME` that is itself blank yields +/// `None` rather than a relative path. +#[must_use] +pub fn state_dir(xdg_state: Option<&OsStr>, home: Option<&OsStr>) -> Option { + if let Some(xdg) = xdg_state.filter(|s| !is_blank(s)) { + return Some(PathBuf::from(xdg).join("pmacs")); + } + home.filter(|s| !is_blank(s)) + .map(|h| PathBuf::from(h).join(".local").join("state").join("pmacs")) +} + +/// Resolve the base state directory from the process environment. +/// +/// A `PMACS_STATE_HOME` override wins over `XDG_STATE_HOME`/`HOME` when +/// set (and non-blank): `.../pmacs` under it. This is the redirect a +/// test harness, CI, or a privacy-conscious user points at a scratch +/// dir so persistence never touches the real `~/.local/state/pmacs` +/// (integration tests link the lib without `cfg(test)`, so the +/// startup wiring runs — the override is how they stay clean). +#[must_use] +pub fn user_state_dir() -> Option { + if let Some(over) = std::env::var_os("PMACS_STATE_HOME") + .as_deref() + .filter(|s| !is_blank(s)) + { + return Some(PathBuf::from(over).join("pmacs")); + } + state_dir( + std::env::var_os("XDG_STATE_HOME").as_deref(), + std::env::var_os("HOME").as_deref(), + ) +} + +/// True when `s` is empty or all ASCII/Unicode whitespace — an +/// unusable env value we treat as unset. +fn is_blank(s: &OsStr) -> bool { + match s.to_str() { + Some(text) => text.trim().is_empty(), + // Non-UTF-8 path bytes are a real (if exotic) directory name; + // only the empty OsStr counts as blank there. + None => s.is_empty(), + } +} + +// --------------------------------------------------------------------------- +// Confined key→file store (Q#PS2) +// --------------------------------------------------------------------------- + +use std::path::Path; + +/// Validate a state key so `pmacs.state.*` can never escape the state +/// directory (Q#PS2 path confinement). A key is a **relative** path of +/// one or more `/`-separated components, each non-empty and drawn from +/// `[A-Za-z0-9._-]`, and no component may be `.` or `..`. Everything +/// else — an absolute path, an empty key, a `.`/`..` component, `//`, +/// or any other byte (separators, control chars, spaces) — is rejected. +/// +/// Without this, a state binding meant to *avoid* raw `io.open` would +/// become an arbitrary read/write anywhere on disk. +/// +/// # Errors +/// Returns a static message describing the first rule the key violates. +pub fn validate_name(name: &str) -> Result<(), &'static str> { + if name.is_empty() { + return Err("state key is empty"); + } + // Reject a leading `/` up front so the split below can't be fooled. + if name.starts_with('/') { + return Err("state key must be relative"); + } + let mut components = 0usize; + for part in name.split('/') { + if part.is_empty() { + return Err("state key has an empty path component"); + } + if part == "." || part == ".." { + return Err("state key may not contain `.` or `..`"); + } + if !part + .bytes() + .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'.' | b'_' | b'-')) + { + return Err("state key component has a disallowed character"); + } + components += 1; + } + if components == 0 { + return Err("state key is empty"); + } + Ok(()) +} + +/// Resolve a validated key to its absolute path under `base`, with a +/// canonical-prefix belt: the joined path must still start with `base`. +/// +/// # Errors +/// Propagates [`validate_name`], or errors if the join escapes `base` +/// (which [`validate_name`] already prevents — this is defense in depth). +pub fn resolve(base: &Path, name: &str) -> Result { + validate_name(name)?; + let path = base.join(name); + if !path.starts_with(base) { + return Err("state key escapes the state directory"); + } + Ok(path) +} + +/// Read a state file's contents, or `Ok(None)` when it does not exist. +/// +/// # Errors +/// Invalid key, or an IO error other than not-found. +pub fn read(base: &Path, name: &str) -> Result, StateError> { + let path = resolve(base, name).map_err(StateError::Name)?; + match std::fs::read_to_string(&path) { + Ok(s) => Ok(Some(s)), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(e) => Err(StateError::Io(e)), + } +} + +/// Atomically write `content` to a state file (creating parents), +/// via [`crate::file_io::save_atomic`] — same durability the editor's +/// own saves get, and no raw `io.open`. +/// +/// # Errors +/// Invalid key, or a save failure. +pub fn write(base: &Path, name: &str, content: &[u8]) -> Result<(), StateError> { + let path = resolve(base, name).map_err(StateError::Name)?; + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).map_err(StateError::Io)?; + } + crate::file_io::save_atomic(&path, content).map_err(StateError::Save)?; + Ok(()) +} + +/// Remove a state file. Missing file is success (idempotent). +/// +/// # Errors +/// Invalid key, or an IO error other than not-found. +pub fn remove(base: &Path, name: &str) -> Result<(), StateError> { + let path = resolve(base, name).map_err(StateError::Name)?; + match std::fs::remove_file(&path) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(StateError::Io(e)), + } +} + +/// Error from a confined state operation. +#[derive(Debug)] +pub enum StateError { + /// The key failed [`validate_name`]. + Name(&'static str), + /// An underlying IO failure (read / remove). + Io(std::io::Error), + /// An atomic-write failure. + Save(crate::file_io::SaveError), +} + +impl std::fmt::Display for StateError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + StateError::Name(m) => write!(f, "invalid state key: {m}"), + StateError::Io(e) => write!(f, "{e}"), + StateError::Save(e) => write!(f, "{e}"), + } + } +} + +impl std::error::Error for StateError {} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn prefers_xdg_state_home() { + let d = state_dir(Some(OsStr::new("/x/state")), Some(OsStr::new("/home/u"))).unwrap(); + assert_eq!(d, PathBuf::from("/x/state/pmacs")); + } + + #[test] + fn falls_back_to_home_local_state() { + let d = state_dir(None, Some(OsStr::new("/home/u"))).unwrap(); + assert_eq!(d, PathBuf::from("/home/u/.local/state/pmacs")); + } + + #[test] + fn none_when_neither_is_set() { + assert!(state_dir(None, None).is_none()); + } + + #[test] + fn validate_name_accepts_keys_and_subpaths() { + for ok in ["recentf", "places", "autosave/deadbeef", "a.b_c-1/x2"] { + assert!(validate_name(ok).is_ok(), "{ok:?} should be accepted"); + } + } + + #[test] + fn validate_name_rejects_escapes() { + for bad in [ + "", + "/etc/passwd", + "..", + "../x", + "a/../b", + "a//b", + "a/", + "/a", + ".", + "a/.", + "with space", + "tab\t", + "null\0", + "sub/../../x", + "..\\x", + ] { + assert!(validate_name(bad).is_err(), "{bad:?} must be rejected"); + } + } + + #[test] + fn resolve_stays_under_base() { + let base = PathBuf::from("/state/pmacs"); + assert_eq!( + resolve(&base, "autosave/x").unwrap(), + PathBuf::from("/state/pmacs/autosave/x") + ); + assert!(resolve(&base, "../escape").is_err()); + } + + #[test] + fn write_read_remove_round_trip() { + let dir = std::env::temp_dir().join(format!("pmacs-state-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + assert!(read(&dir, "recentf").unwrap().is_none(), "absent → None"); + write(&dir, "recentf", b"a\nb\n").unwrap(); + assert_eq!(read(&dir, "recentf").unwrap().as_deref(), Some("a\nb\n")); + // Subpath creates its parent dir. + write(&dir, "autosave/h1", b"x").unwrap(); + assert_eq!(read(&dir, "autosave/h1").unwrap().as_deref(), Some("x")); + remove(&dir, "recentf").unwrap(); + assert!(read(&dir, "recentf").unwrap().is_none(), "removed → None"); + remove(&dir, "recentf").unwrap(); // idempotent + // An invalid key errors rather than escaping. + assert!(read(&dir, "../x").is_err()); + assert!(write(&dir, "/abs", b"x").is_err()); + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn blank_xdg_falls_through_to_home_not_a_relative_path() { + // The Q#PS2 fix: an empty / whitespace XDG_STATE_HOME must NOT + // produce a relative `pmacs/...` (which would write into the + // cwd). It falls through to HOME instead. + for blank in ["", " ", "\t"] { + let d = state_dir(Some(OsStr::new(blank)), Some(OsStr::new("/home/u"))).unwrap(); + assert_eq!( + d, + PathBuf::from("/home/u/.local/state/pmacs"), + "blank XDG {blank:?} must fall through to HOME" + ); + assert!(d.is_absolute(), "state dir is never relative"); + } + // Blank XDG and no HOME → None, not a relative path. + assert!(state_dir(Some(OsStr::new("")), None).is_none()); + // A blank HOME is likewise unusable. + assert!(state_dir(None, Some(OsStr::new(" "))).is_none()); + } +} diff --git a/tests/persistence_acceptance.rs b/tests/persistence_acceptance.rs new file mode 100644 index 0000000..8b0a5f6 --- /dev/null +++ b/tests/persistence_acceptance.rs @@ -0,0 +1,167 @@ +//! Persistence phase 1 acceptance (Arc 3): the `pmacs.state` confined +//! store, saveplace (cursor restored on reopen), and recentf (MRU +//! record + dedup + picker), driven through the real Lua surface. +//! +//! Integration tests link the lib without `cfg(test)`, so +//! `EditorState::new()` configures the state dir from the environment. +//! Each test **overrides that with a private tempdir** (via the +//! `StateDir` app-data) before touching any file, so the suite never +//! reads or writes a developer's real `~/.local/state/pmacs`. +//! +//! Framing: docs/persistence-framing.md. + +use pmacs::editor::EditorState; +use pmacs::lua_bindings::StateDir; +use std::path::PathBuf; + +/// A fresh editor whose state dir is a private, empty tempdir (unique +/// per call so parallel tests never share or wipe each other's dirs). +fn editor_with_state_dir() -> (EditorState, PathBuf) { + use std::sync::atomic::{AtomicUsize, Ordering}; + static SEQ: AtomicUsize = AtomicUsize::new(0); + let dir = std::env::temp_dir().join(format!( + "pmacs-persist-{}-{}", + std::process::id(), + SEQ.fetch_add(1, Ordering::Relaxed) + )); + std::fs::create_dir_all(&dir).expect("mk state tempdir"); + let s = EditorState::new(); + // Override whatever startup configured with our tempdir. + s.lua_host.lua().remove_app_data::(); + s.lua_host.lua().set_app_data(StateDir(dir.clone())); + (s, dir) +} + +/// Write a real file under `dir` and return its path string. +fn write_file(dir: &std::path::Path, name: &str, body: &str) -> String { + let p = dir.join(name); + std::fs::write(&p, body).expect("write test file"); + p.display().to_string() +} + +#[test] +fn state_round_trips_and_rejects_escapes() { + let (s, dir) = editor_with_state_dir(); + let out: (bool, Option, bool, bool) = s + .lua_host + .lua() + .load( + r#" + local wrote = pmacs.state.write("recentf", "a\nb\n") + local back = pmacs.state.read("recentf") + -- Confinement: an escaping key must error (pcall → false). + local esc_ok = pcall(pmacs.state.write, "../escape", "x") + local abs_ok = pcall(pmacs.state.read, "/etc/passwd") + return wrote, back, esc_ok, abs_ok + "#, + ) + .eval() + .expect("state round-trip"); + assert!(out.0, "write returned true"); + assert_eq!( + out.1.as_deref(), + Some("a\nb\n"), + "read returns what was written" + ); + assert!(!out.2, "`../escape` key is rejected"); + assert!(!out.3, "absolute key is rejected"); + // And it actually landed under our tempdir, nowhere else. + assert!(dir.join("recentf").exists()); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn state_is_inert_when_unconfigured() { + let s = EditorState::new(); + // Simulate no state dir (the cfg(test) lib case, or no HOME). + s.lua_host.lua().remove_app_data::(); + let (avail, wrote, read): (bool, bool, Option) = s + .lua_host + .lua() + .load( + r#" + return pmacs.state.available(), + pmacs.state.write("recentf", "should not persist"), + pmacs.state.read("recentf") + "#, + ) + .eval() + .expect("inert state"); + assert!(!avail, "unconfigured → not available"); + assert!(!wrote, "write is a no-op (returns false)"); + assert_eq!(read, None, "read returns nil"); +} + +#[test] +fn recentf_records_dedups_and_orders_mru() { + let (s, dir) = editor_with_state_dir(); + let a = write_file(&dir, "a.rs", "fn a() {}\n"); + let b = write_file(&dir, "b.rs", "fn b() {}\n"); + // Open a, then b, then a again — MRU should be [a, b]. + for path in [&a, &b, &a] { + s.lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open({path:?})")) + .exec() + .expect("open file"); + } + let list: Vec = s + .lua_host + .lua() + .load("return pmacs.recentf.list()") + .eval() + .expect("recentf list"); + assert_eq!(list, vec![a.clone(), b.clone()], "MRU-first, deduped"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn saveplace_restores_cursor_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 + // (buffer.after-load), the cross-session path saveplace targets. + let cursor: i64 = s + .lua_host + .lua() + .load(format!( + r#" + local b = pmacs.buffer.find_or_open({f:?}) + pmacs.editor.goto_byte(12) + 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() + "# + )) + .eval() + .expect("place + save + kill + reopen"); + assert_eq!(cursor, 12, "saveplace restored the cursor byte on reload"); + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn saveplace_can_be_disabled() { + let (s, dir) = editor_with_state_dir(); + let f = write_file(&dir, "off.rs", "aaaa\nbbbb\ncccc\n"); + let cursor: i64 = s + .lua_host + .lua() + .load(format!( + r#" + pmacs.saveplace.enable(false) + local b = pmacs.buffer.find_or_open({f:?}) + pmacs.editor.goto_byte(10) + pmacs.command.invoke("buffer.save") + pmacs.buffer.kill(b) + pmacs.buffer.find_or_open({f:?}) + return pmacs.editor.cursor() + "# + )) + .eval() + .expect("disabled saveplace flow"); + assert_eq!(cursor, 0, "disabled saveplace leaves the cursor at the top"); + std::fs::remove_dir_all(&dir).ok(); +} From d5d75e63edb49c3819e0666c6a7b230ad72d5124 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 8 Jul 2026 18:21:46 -0400 Subject: [PATCH 3/3] 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) --- builtin/runtime/recentf.lua | 1 - builtin/runtime/saveplace.lua | 4 +- src/daemon.rs | 2 + src/editor.rs | 47 +++++++++++------- src/lua_bindings/mod.rs | 12 +++++ src/state.rs | 87 ++++++++++++++++++++++++++++++--- tests/persistence_acceptance.rs | 37 ++++++++++---- 7 files changed, 150 insertions(+), 40 deletions(-) diff --git a/builtin/runtime/recentf.lua b/builtin/runtime/recentf.lua index c607cf0..ac4dc29 100644 --- a/builtin/runtime/recentf.lua +++ b/builtin/runtime/recentf.lua @@ -83,4 +83,3 @@ pmacs.command.define { } pmacs.keymap.bind { scope = "global", sequence = "C-x C-r", command = "recent-files" } - diff --git a/builtin/runtime/saveplace.lua b/builtin/runtime/saveplace.lua index dba7b95..84174e6 100644 --- a/builtin/runtime/saveplace.lua +++ b/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 / diff --git a/src/daemon.rs b/src/daemon.rs index c025d43..c2148cb 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -466,6 +466,8 @@ pub fn run_daemon(socket_path: PathBuf, instance_name: Option) -> 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 diff --git a/src/editor.rs b/src/editor.rs index 4627152..855e0bf 100644 --- a/src/editor.rs +++ b/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 { @@ -1516,6 +1523,8 @@ pub fn run(file: Option) -> 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 diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 874d8e3..ef73963 100644 --- a/src/lua_bindings/mod.rs +++ b/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. diff --git a/src/state.rs b/src/state.rs index 88a9bea..d0b6865 100644 --- a/src/state.rs +++ b/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 { + // `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 { .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 { 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())); diff --git a/tests/persistence_acceptance.rs b/tests/persistence_acceptance.rs index 8b0a5f6..c3a6c9d 100644 --- a/tests/persistence_acceptance.rs +++ b/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::(); + assert!( + s.lua_host.lua().app_data_ref::().is_none(), + "new() must leave the state dir unconfigured" + ); let (avail, wrote, read): (bool, bool, Option) = 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(); }