fix(dired): address PR #165 review round 1

F1 (real, small-window misbehavior). `dired.revert`'s re-seat runs after
the read settles, and `pmacs.editor.move_to_line` is AMBIENT -- it moves
whatever window is active. A user who switched buffers (or hit `q`)
while the re-read was in flight had an unrelated buffer's cursor moved
to a line index that only means something in the dired listing. The
paint was already safe because it names its buffer; the seat now runs
only while dired is still the active buffer, and `seat_cursor`'s doc
says which callers are unconditionally in the right place and why.
Pinned by a test that starts the revert, switches to a six-line file
before the pump, and asserts that buffer's cursor never moved -- and
that the dired buffer still reverts when it IS active.

F2 (a trap set for Stage 3). `fmt_size` used `%10d`, so a size past ten
digits -- 10 GB and up, ordinary for VM images and core dumps -- widened
the field and shifted mtime and name right on that line alone. Cosmetic
today, but `_layout` is exported as a contract and Stage 3's
column-classifying intercept is planned against it. It now takes
`fmt_mtime`'s discipline: exact bytes while they fit, else a
fixed-width magnitude, so precision yields to the invariant rather than
the other way round. This is not the deferred human-readable column --
the exact count still shows right up to where it cannot. Pinned with a
sparse 12 GB fixture that skips if the filesystem refuses it.

F3 (honesty and a doubled read). The symlink arm claimed the probe cost
"one syscall"; it was a full `read_dir` -- opendir plus one lstat per
child -- and on success `open_directory` immediately read the same
directory again. Since `open_directory` reads before touching editor
state and raises having changed nothing (acceptance 15's invariant), its
failure IS the "not a directory" answer: the probe is gone, one read
remains, and the comment says what it actually does. New test pins both
arms -- a symlink to a directory descends under the path the user walked
(canonicalization is lexical, so the link is not resolved), and a
symlink to a file opens with the target's contents.

F4 (deliberate failure mode). A tolerant listing recorded readdir
iterator errors without bound, and `std::fs::ReadDir` need not terminate
after yielding one. Cancellation is NOT an adequate backstop here --
which is the reason for a constant rather than a comment saying it is: a
dired listing carries no supersede key, so nothing cancels it. A
directory whose iterator produces nothing but errors now fails with the
last error the way an unopenable directory does, after
READDIR_MAX_CONSECUTIVE_ENTRY_ERRORS; the counter resets on any entry
that materializes. Documented as untested and why: faking a failing
iterator needs the walk generic over it, a refactor with no other
consumer.

Smaller notes, all taken: READ_ONLY_LIMIT renamed NAME_VARIANT_LIMIT (it
caps `<2>`..`<99>`, nothing read-only); `fmt_perms`' omission of
setuid/setgid/sticky documented as a decision tied to the M8.3 fixture's
nine-bit parser; `format_outcome` binds the slice in the pattern instead
of re-traversing; and `pmacs.path.canonicalize`'s `to_string_lossy` is
noted as inside the existing non-UTF-8-path deferral rather than an
exception to it.

Process note, learned the hard way twice now: the round-1 dired.lua
fixes were briefly wiped because a mutation-bite helper restores with
`git checkout --`, which reverts to HEAD -- so a fix must be committed
BEFORE it is bitten, not after.

builtin/runtime/dired.lua

@@ -224,6 +224,11 @@ end
 -- `rwxr-xr-x`, without the leading kind char (rendered separately so a
 -- symlink shows `l` and a directory `d`). Arithmetic rather than bit
 -- ops: this file has to run on LuaJIT (5.1) as well as Lua 5.4.
+--
+-- The nine basic bits only: setuid / setgid / sticky are deliberately
+-- not surfaced as Emacs's `s` / `t`, matching the M8.3 fixture's
+-- `parse_perm_string`, which edits exactly these nine. Rendering a bit
+-- Stage 3 could not accept back would be worse than omitting it.
 local function fmt_perms(mode)
   local function tri(bits)
     local r = (bits >= 4) and "r" or "-"
@@ -244,8 +249,33 @@ local function kind_char(kind)
   end
 end
 
+-- Exact bytes while they fit the column; a magnitude past that.
+--
+-- `%10d` holds ten digits, so a file of 10 GB or more (VM images, core
+-- dumps --- ordinary things) widens the field and shifts mtime and name
+-- right on that line alone. That is only cosmetic today, but
+-- `_layout.NAME_START` is exported as a contract and Stage 3's
+-- column-classifying intercept is planned against these constants, so a
+-- line that violates them now is a Stage 3 trap. Same discipline as
+-- `fmt_mtime`: the width is the invariant, and precision yields to it.
+--
+-- This is NOT the deferred human-readable size column (§13): the exact
+-- byte count is still what a listing shows, right up to the point where
+-- it cannot be shown at all.
+local SIZE_UNITS = { "K", "M", "G", "T", "P", "E" }
+
 local function fmt_size(n)
-  return string.format("%" .. SIZE_BYTES .. "d", n)
+  local exact = string.format("%" .. SIZE_BYTES .. "d", n)
+  if #exact <= SIZE_BYTES then return exact end
+  local value, unit = n, SIZE_UNITS[#SIZE_UNITS]
+  for _, suffix in ipairs(SIZE_UNITS) do
+    value = value / 1024
+    unit = suffix
+    if value < 1024 then break end
+  end
+  local scaled = string.format("%.1f%s", value, unit)
+  if #scaled > SIZE_BYTES then scaled = scaled:sub(1, SIZE_BYTES) end
+  return string.rep(" ", SIZE_BYTES - #scaled) .. scaled
 end
 
 local function fmt_mtime(secs)
@@ -348,6 +378,13 @@ end
 -- Re-seat by BASENAME (Q#DR9), falling back to the nearest surviving
 -- line. Every repaint is wholesale, so without this a revert, a sort,
 -- or any Stage 2 operation would drop the cursor to the header.
+--
+-- `move_to_line` is AMBIENT --- it moves the active window's cursor, not
+-- `handle.buf`'s --- so every caller that can run after an `:await()`
+-- has to check that dired is still the active buffer first. Painting is
+-- safe either way (it names the buffer); seating is not. Callers that
+-- activate the buffer themselves (an open, which displays first) are
+-- unconditionally in the right place.
 local function seat_cursor(handle, name, fallback_line)
   local count = #handle.entries
   if count == 0 then
@@ -416,7 +453,8 @@ end
 -- Buffer ownership
 -- ---------------------------------------------------------------------------
 
-local READ_ONLY_LIMIT = 99
+-- How far the `<2>`, `<3>`, ... disambiguation walks before giving up.
+local NAME_VARIANT_LIMIT = 99
 
 -- `pmacs.buffer.create` takes any caller-chosen name, so a foreign
 -- buffer may already be called `*dired:/tmp*`. Painting into it through
@@ -435,7 +473,7 @@ local function claim_handle(path)
   local name = buffer_name(path)
   if buffer_named(name) then
     local unique = nil
-    for i = 2, READ_ONLY_LIMIT do
+    for i = 2, NAME_VARIANT_LIMIT do
       local candidate = string.format("%s<%d>", name, i)
       if buffer_named(candidate) == nil then
         unique = candidate
@@ -680,19 +718,20 @@ pmacs.command.define {
       return
     end
     if entry.kind == "symlink" then
-      -- `read_dir`/`stat` are lstat-based, so the only way to learn
+      -- `read_dir` and `stat` are both lstat-based, so nothing in the
-      -- whether a link points at a directory is to try to list it. A
+      -- entry says whether the link points at a directory --- the only
-      -- symlinked directory is an ordinary thing to walk into, and the
+      -- way to find out is to try to list it. A symlinked directory is
-      -- probe costs one syscall on symlink lines only.
+      -- an ordinary thing to walk into, so try the descent and fall back
+      -- to a file visit.
+      --
+      -- `open_directory` is the try: it reads before touching any editor
+      -- state and raises having changed nothing (acceptance 15), so its
+      -- failure IS the "not a directory" answer. An explicit probe
+      -- followed by the real open would list the whole directory TWICE
+      -- --- opendir plus one lstat per child, each time.
       pmacs.async(function()
-        local ok = pcall(function()
+        local descended = pcall(open_directory, target, nil, handle)
-          return pmacs.fs.read_dir(target, { tolerant = true }):await()
+        if descended then return end
-        end)
-        if ok then
-          local descended, err = pcall(open_directory, target, nil, handle)
-          if not descended then report("dired", err) end
-          return
-        end
         local visited, err = pcall(pmacs.window.display_file, target, { select = true })
         if not visited then report("dired", err) end
       end)
@@ -742,7 +781,14 @@ pmacs.command.define {
       handle.entries = entries
       handle.errors = errors
       paint(handle)
-      seat_cursor(handle, name, line)
+      -- The re-read settles a tick or more later, and the user may have
+      -- left (a buffer switch, or `q`) in the meantime. The paint names
+      -- its buffer and is safe; seating is ambient, so a stale seat here
+      -- would move an unrelated buffer's cursor to a line index that
+      -- only means something in this listing.
+      if pmacs.window.buffer() == handle.buf then
+        seat_cursor(handle, name, line)
+      end
     end)
   end,
 }

src/fs.rs

@@ -42,6 +42,28 @@ use crate::worker::CancellationToken;
 /// directories.
 const READDIR_CANCEL_POLL_EVERY: usize = 32;
 
+/// How many *consecutive* `readdir` iterator errors a tolerant listing
+/// records before giving up and failing (dired Q#DR6).
+///
+/// [`std::fs::ReadDir`] is not obliged to terminate after yielding an
+/// `Err`: a directory pulled out from under a stalled network mount can
+/// keep producing them. Tolerant mode records-and-continues, so without
+/// a bound that is an unbounded error vector on a worker thread.
+///
+/// Cancellation is **not** an adequate backstop here, which is the
+/// reason this constant exists rather than a comment saying it is: a
+/// dired listing carries no supersede key and nothing cancels it, so the
+/// only thing that would stop the loop is the directory itself. A
+/// directory whose iterator produces nothing but errors has no partial
+/// answer worth rendering, so the listing fails with the last error the
+/// way an unopenable directory does.
+///
+/// Deliberately untested: forcing a real `readdir` to yield errors
+/// repeatedly is not portable, and faking it would need the walk to be
+/// generic over its iterator — a refactor with no other consumer. The
+/// counter resets on any entry that materializes.
+const READDIR_MAX_CONSECUTIVE_ENTRY_ERRORS: usize = 1024;
+
 /// One directory entry as returned by [`read_dir_blocking`].
 ///
 /// The shape is what `dired` / `magit-class` / `outline-class`
@@ -277,6 +299,7 @@ pub fn read_dir_blocking(
     let mut errors: Option<Vec<FsDirEntryError>> =
         matches!(tolerance, ReadDirTolerance::PerEntry).then(Vec::new);
     let parent_str = path.display().to_string();
+    let mut consecutive_entry_errors = 0usize;
     for (i, entry_result) in iter.enumerate() {
         if i % READDIR_CANCEL_POLL_EVERY == 0 && cancel.is_cancelled() {
             return Err(FsError::Cancelled);
@@ -286,17 +309,19 @@ pub fn read_dir_blocking(
             Err(source) => {
                 // R2-2: the entry never materialized, so there is no
                 // name to report and the error names the parent.
-                record_entry_error(
+                let error = FsError::Io {
-                    &mut errors,
+                    path: parent_str.clone(),
-                    None,
+                    source,
-                    FsError::Io {
+                };
-                        path: parent_str.clone(),
+                consecutive_entry_errors += 1;
-                        source,
+                if consecutive_entry_errors > READDIR_MAX_CONSECUTIVE_ENTRY_ERRORS {
-                    },
+                    return Err(error);
-                )?;
+                }
+                record_entry_error(&mut errors, None, error)?;
                 continue;
             }
         };
+        consecutive_entry_errors = 0;
         let entry_path = entry.path();
         // Resolved first so a later per-entry failure can name it.
         let name = path_to_utf8_string(&entry.file_name(), &parent_str)?;

src/lua_bindings/mod.rs

@@ -3574,6 +3574,13 @@ impl UserData for AnsiParserLua {
 /// an edge (`//tmp`, `~` with `HOME` unset, a `..` that would escape
 /// root) would mint two buffers for one directory with no error
 /// anywhere.
+///
+/// The result crosses the boundary through `to_string_lossy`, so a
+/// non-UTF-8 `$HOME` (or a non-UTF-8 argument) can yield a Lua string
+/// that no longer names the `PathBuf` the registry keys on. That is the
+/// same limit `pmacs.fs` already documents — byte-preserving paths are
+/// post-v0.1 work that widens every path in the API — and it is recorded
+/// here so this binding is not read as an exception to it.
 fn install_path_module(lua: &Lua) -> mlua::Result<Table> {
     let path = lua.create_table()?;
     path.set(

src/workers_buffer.rs

@@ -207,10 +207,10 @@ fn format_outcome(outcome: &JobOutcome) -> String {
             // tolerant listing that dropped half a directory is not the
             // same observable outcome as a clean one.
             match listing.errors.as_deref() {
-                Some([_, ..]) => format!(
+                Some(errors @ [_, ..]) => format!(
                     "ok ({} entries, {} unreadable)",
                     listing.entries.len(),
-                    listing.errors.as_ref().map_or(0, Vec::len)
+                    errors.len()
                 ),
                 _ => format!("ok ({} entries)", listing.entries.len()),
             }

tests/dired_acceptance.rs

@@ -417,6 +417,75 @@ fn dired_renders_a_header_and_one_line_per_entry() {
     );
 }
 
+/// The columns are a CONTRACT, not a formatting preference: `_layout` is
+/// exported and Stage 3's column-classifying intercept is planned
+/// against it. A size that does not fit ten digits (10 GB and up — VM
+/// images, core dumps) must therefore yield precision rather than width,
+/// the way `fmt_mtime` already does. Without that, one line's mtime and
+/// name shift right and nothing notices until Stage 3.
+#[test]
+fn dired_keeps_its_columns_when_a_size_exceeds_the_field() {
+    let td = tempfile::tempdir().expect("tempdir");
+    std::fs::write(td.path().join("small.txt"), b"x").expect("write small");
+    let huge = td.path().join("huge.img");
+    // Sparse: `set_len` allocates nothing on any filesystem pmacs
+    // supports. If one refuses, the premise cannot be established.
+    let file = std::fs::File::create(&huge).expect("create huge");
+    if file.set_len(12_000_000_000).is_err() {
+        eprintln!("filesystem refused a sparse 12 GB file; skipping");
+        return;
+    }
+    drop(file);
+    let reported = std::fs::metadata(&huge).expect("stat huge").len();
+    assert!(
+        reported > 9_999_999_999,
+        "fixture premise: the size must exceed ten digits, got {reported}"
+    );
+
+    let mut s = editor();
+    open_ok(&mut s, td.path(), "nil");
+    let size_start = layout(&s, "SIZE_START");
+    let mtime_start = layout(&s, "MTIME_START");
+    let name_start = layout(&s, "NAME_START");
+
+    let lines = active_lines(&s);
+    for name in ["huge.img", "small.txt"] {
+        let line = &lines[line_of(&s, name)];
+        let size = &line[size_start..mtime_start - 1];
+        assert_eq!(
+            size.len(),
+            10,
+            "the size field must stay ten columns wide: {line:?}"
+        );
+        let stamp = &line[mtime_start..name_start - 1];
+        assert!(
+            stamp.starts_with("20") && stamp.contains(':'),
+            "so the mtime still starts where the layout says: {line:?}"
+        );
+        assert_eq!(
+            line_name(&s, line),
+            name,
+            "and the name still starts at NAME_START"
+        );
+    }
+
+    // The oversized value degrades to a magnitude rather than a
+    // placeholder, so the listing still says how big the file is.
+    let huge_line = &lines[line_of(&s, "huge.img")];
+    let size = huge_line[size_start..mtime_start - 1].trim();
+    assert!(
+        size.ends_with('G') || size.ends_with('T'),
+        "an oversized size keeps its magnitude: {size:?}"
+    );
+    // A size that DOES fit stays exact.
+    let small_line = &lines[line_of(&s, "small.txt")];
+    assert_eq!(
+        small_line[size_start..mtime_start - 1].trim(),
+        "1",
+        "a size that fits is still the exact byte count"
+    );
+}
+
 // ---------------------------------------------------------------------------
 // 2 --- visit dispatches on kind, through the panel-safe primitive
 // ---------------------------------------------------------------------------
@@ -474,6 +543,54 @@ fn dired_visit_dispatches_on_entry_kind() {
     );
 }
 
+/// A symlink's kind is `"symlink"` in both `read_dir` and `stat` (both
+/// are lstat-based), so nothing in the entry says what it points at.
+/// `RET` therefore tries the descent and falls back to a file visit —
+/// one read, since `open_directory` reads before touching any editor
+/// state and its failure *is* the "not a directory" answer.
+#[test]
+fn dired_visit_follows_a_symlink_to_the_kind_of_its_target() {
+    let td = fixture_dir();
+    std::os::unix::fs::symlink("subdir", td.path().join("linkdir")).expect("symlink to dir");
+
+    let mut s = editor();
+    open_ok(&mut s, td.path(), "nil");
+
+    // A symlink to a directory descends. The path is NOT resolved
+    // (canonicalization is lexical), so the buffer names the way the user
+    // navigated — Emacs parity.
+    seat_on(&s, "linkdir");
+    press(&mut s, KeyCode::Enter);
+    pump(&mut s);
+    assert_eq!(
+        active_name(&s),
+        format!("*dired:{}*", canon(&td.path().join("linkdir"))),
+        "a symlinked directory descends under the path we walked"
+    );
+    assert_eq!(
+        line_name(&s, &active_lines(&s)[1]),
+        "inner.txt",
+        "and shows the target directory's contents"
+    );
+
+    // A symlink to a file opens the file.
+    type_char(&mut s, '^');
+    pump(&mut s);
+    seat_on(&s, "link");
+    press(&mut s, KeyCode::Enter);
+    pump(&mut s);
+    let path = active_path(&s).expect("a file must be open");
+    assert!(
+        path.ends_with("/link"),
+        "the visit keeps the link's own path; got {path}"
+    );
+    assert_eq!(
+        eval::<String>(&s, "return pmacs.window.buffer():slice(0, 5)"),
+        "hello",
+        "with the target's contents"
+    );
+}
+
 /// The panel case, which is the real assertion (Q#DR10): with dired
 /// displayed as a panel, `RET` on a file leaves the dired panel alive
 /// and puts the file in the document window. Falsified by swapping
@@ -1018,6 +1135,68 @@ fn dired_revert_reseats_the_cursor_by_basename() {
     );
 }
 
+/// A revert settles a tick or more later, and the user may have left in
+/// the meantime. `pmacs.editor.move_to_line` is **ambient** — it moves
+/// whatever window is active — so an unguarded re-seat moves an
+/// unrelated buffer's cursor to a line index that only means something
+/// in the dired listing. The paint is safe either way because it names
+/// its buffer; this pins the half that does not.
+#[test]
+fn dired_revert_does_not_seat_a_buffer_the_user_switched_to() {
+    let td = tempfile::tempdir().expect("tempdir");
+    for name in ["a.txt", "b.txt", "c.txt", "d.txt", "e.txt"] {
+        std::fs::write(td.path().join(name), b"x").expect("write");
+    }
+    let notes = td.path().join("notes.txt");
+    std::fs::write(&notes, b"one\ntwo\nthree\nfour\nfive\nsix\n").expect("write notes");
+
+    let mut s = editor();
+    open_ok(&mut s, td.path(), "nil");
+    exec(&s, "_G.DIRED_BUF = pmacs.window.buffer()");
+    // A late line, so a stale seat would be visible in the other buffer.
+    seat_on(&s, "e.txt");
+    let dired_line = cursor_line(&s);
+    assert!(
+        dired_line >= 4,
+        "fixture premise: a late line, got {dired_line}"
+    );
+
+    // Start the revert, then leave BEFORE the read settles.
+    type_char(&mut s, 'g');
+    exec(
+        &s,
+        &format!(
+            "pmacs.buffer.find_or_open({:?})",
+            notes.display().to_string()
+        ),
+    );
+    assert_eq!(cursor_line(&s), 0, "a freshly opened file starts at line 0");
+    pump(&mut s);
+
+    assert_eq!(
+        active_path(&s).map(PathBuf::from),
+        Some(PathBuf::from(canon(&notes))),
+        "the switch stands: the revert must not pull the user back"
+    );
+    assert_eq!(
+        cursor_line(&s),
+        0,
+        "and it must not move the cursor of the buffer they moved to"
+    );
+
+    // The revert itself still happened: the dired buffer is repainted,
+    // and returning to it seats normally on the next command.
+    std::fs::write(td.path().join("f.txt"), b"x").expect("write f");
+    exec(&s, "pmacs.window.switch_buffer(_G.DIRED_BUF)");
+    type_char(&mut s, 'g');
+    pump(&mut s);
+    assert!(
+        active_text(&s).contains("f.txt"),
+        "the dired buffer still reverts when it is the active one: {:?}",
+        active_text(&s)
+    );
+}
+
 // ---------------------------------------------------------------------------
 // 8 --- sort modes
 // ---------------------------------------------------------------------------