diff --git a/builtin/runtime/async.lua b/builtin/runtime/async.lua index 000be49..eb813da 100644 --- a/builtin/runtime/async.lua +++ b/builtin/runtime/async.lua @@ -468,6 +468,26 @@ function pmacs.workers.dispatch(name, args, opts) return finish_dispatch(pcall(handler, args, opts)) end +-- Worker identity Stage 1: the name registered here is DISPLAY TEXT. +-- +-- It used to be type-checked and nothing more, which was defensible +-- while it died inside `dispatch`. It no longer dies there: the ambient +-- carries it into every job the handler allocates, and it is composed +-- into `purpose` as `": "`, which the `*workers*` table +-- and the modeline indicator both render. So it gets the same +-- meaningful-value standard `purpose` already gets in +-- `required_purpose` (`src/lua_bindings/mod.rs`) --- and one rule +-- `purpose` deliberately does NOT get. +-- +-- The asymmetry is the point. A purpose may legitimately contain a +-- newline: a filesystem path can, and `pmacs-magit`'s spawn purpose is a +-- whole argv --- so its one-line constraint is enforced by ESCAPING at +-- the surfaces that have one row (`purpose_for_one_row`), following the +-- `#228` decision on `Command.description`. A registered handler NAME +-- has no such case. It is an identifier a package chooses for itself and +-- passes back to `dispatch`, so a control character in it is a mistake +-- or an attempt at one, and refusing at the source costs nobody +-- anything. function pmacs.workers.register(name, handler) -- Allows future Rust-side modules (or test harnesses) to register -- additional dispatchable names. v0.1 has no plugin loader but the @@ -475,6 +495,20 @@ function pmacs.workers.register(name, handler) if type(name) ~= "string" then error("pmacs.workers.register: name must be a string") end + -- Empty and whitespace-only satisfy the type and say nothing --- the + -- exact pair `required_purpose` rejects, and the exact pair R42 + -- rejects for config descriptions. + if name:match("^%s*$") ~= nil then + error("pmacs.workers.register: name must not be empty or whitespace-only") + end + -- `%c` is the C control class: NUL, the C0 range, DEL. A newline + -- forges a row in `*workers*`, a CR rewrites one on a terminal and an + -- ESC starts a sequence in one. Checked AFTER the whitespace rule so + -- a name that is only "\n" reports the emptier problem, which is the + -- one the caller can act on. + if name:find("%c") ~= nil then + error("pmacs.workers.register: name must not contain control characters") + end if type(handler) ~= "function" then error("pmacs.workers.register: handler must be a function") end diff --git a/docs/active-work.md b/docs/active-work.md index 474e6a9..83f2701 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -339,10 +339,16 @@ the authoritative tip** — the ref, not a SHA. Recover with so making terminal PTYs visible is deferred to Stage 2 with a separate accessor rather than by widening this one. - **Deliberate deviation from the audit, flagged for review:** §9 names - owner/purpose/**parent** together as the prerequisite; Stage 1 takes - only the first two. A parent needs an ambient "currently-running job" - context, and an unpopulated `parent` reads as "no parent" rather than - "not tracked" (Q#W-5). + owner/**purpose**/parent together as the prerequisite; Stage 1 takes + **only `purpose`** — one of the three, not two. `owner` was removed in + revision 2: nothing in the runtime knows which package asked for a + job, so an `owner` field could only have been filled with the same + handler name `purpose` already carries, and an empty one reads as + "unowned" rather than "not tracked". `parent` is out for the matching + reason — it needs an ambient "currently-running job" context, and an + unpopulated `parent` reads as "no parent" rather than "not tracked" + (Q#W-5). The package-ownership slot stays **deliberately empty** until + P3 can fill it with a real signal (framing §3, §7). - **Gates:** `scripts/gate --acceptance worker_identity_acceptance --acceptance journey_acceptance --acceptance statusline_segments_acceptance --acceptance compile_mode_acceptance diff --git a/docs/worker-identity-framing.md b/docs/worker-identity-framing.md index b2280a4..aa165e0 100644 --- a/docs/worker-identity-framing.md +++ b/docs/worker-identity-framing.md @@ -5,8 +5,9 @@ removed that title overclaimed the lane: it answers **what**, and — under `pmacs.workers.dispatch` — **under which registered handler**. Neither is who owns it.)* -**Status: revision 4, APPROVED 2026-08-09. Implementation may -proceed.** +**Status: revision 4, APPROVED 2026-08-09. IMPLEMENTED — see +`docs/active-work.md` for the commits, the gate outcome and the review +rounds.** **Revision 4 scopes rule 1's claim to what it can actually enforce, and takes Q#W-7 into this lane.** Revision 3 said the rule covered "all diff --git a/src/async_runtime.rs b/src/async_runtime.rs index e60d0ae..284d2ee 100644 --- a/src/async_runtime.rs +++ b/src/async_runtime.rs @@ -58,8 +58,10 @@ //! search with cooperative cancellation and frame-boundary coalescing. //! Tree-sitter and LSP land in M4 on the same dispatch shape. +use std::borrow::Cow; use std::cell::{Cell, RefCell}; use std::collections::{HashMap, VecDeque}; +use std::fmt::Write; use std::path::{Path, PathBuf}; use std::rc::Rc; use std::sync::atomic::{AtomicU64, Ordering}; @@ -607,15 +609,87 @@ pub struct ActivitySummary { /// is always present costs modeline width forever to say "nothing /// is happening". pub in_flight: usize, - /// The **oldest** in-flight job's purpose. + /// The **oldest** in-flight job's purpose, already passed through + /// [`purpose_for_one_row`]. /// /// Oldest, not newest and not "busiest": jobs carry no cost /// estimate, so "busiest" is not a defined quantity, while oldest /// is computable from `dispatched_at` and answers the question a /// user actually asks of a stuck editor. + /// + /// Escaped here rather than at the Lua provider because this struct + /// **is** the indicator's read surface — it exists for one consumer, + /// and that consumer has exactly one row. `workers_snapshot` is the + /// free-form path and stays raw. pub oldest_purpose: String, } +/// A `purpose` rendered for a surface that gives it exactly **one row**. +/// +/// # A row must not be able to forge another row +/// +/// That is the property, and it is the only reason this exists. A +/// purpose is free-form text supplied by whoever dispatched the work, +/// and it is legitimately multi-line: a filesystem path may contain a +/// newline, and `pmacs-magit`'s spawn purpose is a whole argv. Rendered +/// raw into a row-per-job table, one such purpose becomes two physical +/// lines — the second of which the reader has no way to tell from a real +/// job row, because a real job row is just text in the same buffer. +/// The same applies to `\r`, which rewrites a rendered line in place on +/// a terminal, and to `\u{1b}`, which starts an escape sequence in one. +/// +/// # Escape, do not reject, and do not clip +/// +/// This follows the `#228` decision recorded on +/// [`crate::command::Command::description`]: the one-line constraint +/// belongs to the **surface that has it**, not to the registry that does +/// not. There, a free-form description is clipped by +/// `Command::description_first_line` at the two single-row consumers +/// while the registry keeps every line. Here the equivalent is escaping +/// rather than clipping, because a purpose's later lines are not +/// decoration — an argv's second word is as load-bearing as its first, +/// and a clip would silently drop the part that says which file. +/// +/// `pmacs.workers.snapshot()` is this lane's `describe-command`: it +/// hands Lua the raw purpose, so nothing is lost, only made safe where +/// a row boundary means something. +/// +/// # What is not escaped +/// +/// A backslash. Escaping it would make a purpose containing no control +/// characters **not** byte-identical after this call, and byte-identity +/// for ordinary text is a property worth more than distinguishing a +/// literal `\n` from an escaped newline — the ambiguity is cosmetic, +/// while forging a row is not, and no amount of literal backslashes +/// produces a second row. +#[must_use] +pub fn purpose_for_one_row(purpose: &str) -> Cow<'_, str> { + // `char::is_control` is the Unicode `Cc` category: C0 (`\0`–`\x1f`), + // `\x7f`, and C1 (`\u{80}`–`\u{9f}`, which includes NEL). Borrowing + // when there is nothing to do keeps the common path allocation-free + // AND makes the byte-identity property structural rather than + // asserted. + if !purpose.contains(char::is_control) { + return Cow::Borrowed(purpose); + } + let mut out = String::with_capacity(purpose.len() + 8); + for ch in purpose.chars() { + match ch { + '\n' => out.push_str("\\n"), + '\r' => out.push_str("\\r"), + '\t' => out.push_str("\\t"), + other if other.is_control() => { + // `\u{1b}`, the same spelling Rust's own `escape_debug` + // uses, so the rendered form is one a reader can paste + // back into either language and get the byte returned. + let _ = write!(out, "\\u{{{:x}}}", other as u32); + } + other => out.push(other), + } + } + Cow::Owned(out) +} + /// One frame's worth of streamed items for a single stream id, /// returned by [`AsyncRuntime::take_stream_batches`]. T M3.5. #[derive(Clone, Debug)] @@ -1514,7 +1588,10 @@ impl AsyncRuntime { let (_, purpose) = oldest?; Some(ActivitySummary { in_flight, - oldest_purpose: purpose.to_owned(), + // The modeline is one row and a segment is one line; + // `purpose_for_one_row` is what keeps a purpose carrying a + // newline (a path, an argv) from breaking it. + oldest_purpose: purpose_for_one_row(purpose).into_owned(), }) } diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index c47b678..406a3d6 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -7580,12 +7580,27 @@ pub fn install_async( // `pmacs.workers.dispatch`, which brackets these itself under // `pcall`. A package pushing by hand and failing to pop would poison // every later dispatch in the session with a stale name. + // + // `mlua::String`, not `String`: the parameter is a Lua BYTE string, + // so an `mlua`-driven `String` conversion would refuse a non-UTF-8 + // name with a generic message naming neither the argument nor the + // rule. `pmacs.workers.register` enforces the rest of the + // display-text standard (non-empty, no control characters) but + // cannot see UTF-8 validity from Lua 5.1, so the byte-level half is + // enforced here — the one point where Rust sees the name — with a + // message that names both. { let rt = runtime.clone(); async_mod.set( "_push_dispatch_name", - lua.create_function(move |_, name: String| { - rt.push_dispatch_name(name); + lua.create_function(move |_, name: mlua::String| { + let Ok(text) = name.to_str() else { + return Err(mlua::Error::external( + "pmacs.workers.dispatch: handler name must be valid UTF-8 — it is \ + displayed to the user as part of every job's purpose.", + )); + }; + rt.push_dispatch_name(&*text); Ok(()) })?, )?; @@ -8766,18 +8781,35 @@ fn parse_restart(name: &str) -> mlua::Result { /// /// # Errors /// -/// Absent, empty, whitespace-only, or non-string. Empty and -/// whitespace-only are rejected because they satisfy the type and defeat -/// the point exactly as copying the label across would — R42 already -/// rejects whitespace-only `description`s in the config registry for the -/// same reason. +/// Absent, empty, whitespace-only, non-string, or **not valid UTF-8**. +/// Empty and whitespace-only are rejected because they satisfy the type +/// and defeat the point exactly as copying the label across would — R42 +/// already rejects whitespace-only `description`s in the config registry +/// for the same reason. +/// +/// The UTF-8 case is a **reachable input class, not an internal +/// invariant**: Lua strings are byte strings, so `purpose = +/// string.char(255)` is a value a caller can write. Converting it with +/// `?` would surface mlua's generic conversion error *before* any of the +/// diagnostics below is constructed, and the caller would be told +/// neither the field nor the rule — so the conversion failure is mapped +/// onto this function's own message instead. /// /// The read is **raw**, matching the posture `stdin` and `group` already /// document in [`lua_to_spec`]: a spec table is plain data, so a /// metatable cannot smuggle a purpose in through `__index`. fn required_purpose(table: &Table) -> mlua::Result { let purpose = match table.raw_get::("purpose") { - Ok(mlua::Value::String(value)) => value.to_str()?.to_owned(), + Ok(mlua::Value::String(value)) => match value.to_str() { + Ok(text) => text.to_owned(), + Err(_) => { + return Err(mlua::Error::external( + "pmacs.process.spawn: purpose must be valid UTF-8 — it is displayed \ + to the user in *workers* and in the modeline, and arbitrary bytes \ + have no display form there.", + )); + } + }, Ok(mlua::Value::Nil) => { return Err(mlua::Error::external( "pmacs.process.spawn: purpose is required — a short description of what \ diff --git a/src/workers_buffer.rs b/src/workers_buffer.rs index 02d6d03..2a3bd59 100644 --- a/src/workers_buffer.rs +++ b/src/workers_buffer.rs @@ -42,6 +42,12 @@ //! `Status` right rather than being truncated: losing the end of a path //! is a worse failure than an uneven column. //! +//! This table is **one row per job**, and the purpose is the only free +//! text in it, so every row goes through +//! [`crate::async_runtime::purpose_for_one_row`]: a row must not be able +//! to forge another row. See that function for why the escaping lives +//! here rather than as a rule on the purpose itself. +//! //! Lua reads the snapshot via `pmacs.workers.snapshot()`; the //! `pmacs.workers.show()` builtin invokes [`render`] on it and //! returns the buffer id. Auto-refresh hooks into @@ -50,7 +56,7 @@ use std::fmt::Write; use crate::async_runtime::{ - ActiveJobInfo, CompletedJobInfo, JobOutcome, JobResult, WorkersSnapshot, + ActiveJobInfo, CompletedJobInfo, JobOutcome, JobResult, WorkersSnapshot, purpose_for_one_row, }; use crate::buffer::{Buffer, BufferId, EditOp}; use crate::buffer_registry::BufferRegistry; @@ -197,7 +203,7 @@ fn write_active_row(text: &mut String, job: &ActiveJobInfo) { if job.is_stream { status.push_str(" [stream]"); } - let purpose = &job.purpose; + let purpose = purpose_for_one_row(&job.purpose); let _ = writeln!( text, "{id:<7} {kind:<11} {age:>9} {key:<11} {purpose:9} {key:<11} {purpose:(&state, "return RAN"), + "and it must refuse BEFORE running the handler" + ); + assert!( + !eval::(&state, "return pmacs._async._in_dispatch_name_scope()"), + "a push that failed must leave no name on the stack" + ); + pump(&mut state); +} + /// **Rule 7 + the defect itself.** A job dispatched through /// `pmacs.workers.dispatch("name", …)` reports `"name"`. /// @@ -898,3 +1053,145 @@ fn the_workers_buffer_renders_the_purpose_column() { exec(&state, "pmacs.workers.hide()"); pump(&mut state); } + +// --------------------------------------------------------------------------- +// 7 — the display-text boundary: a row must not forge another row +// --------------------------------------------------------------------------- +// +// A purpose is free-form caller text and is legitimately multi-line — a +// filesystem path may contain a newline and `pmacs-magit`'s spawn +// purpose is a whole argv — so the one-line constraint belongs to the +// surfaces that have one line, not to the purpose. That is `#228`'s +// decision on `Command.description`, applied here as escaping rather +// than clipping, because a purpose's later words are load-bearing. +// +// Every test below drives the REAL rendering path. Calling +// `purpose_for_one_row` directly would prove the escaper escapes and say +// nothing about whether either surface calls it. +// +// `register_external` is the witness in all three because it is the one +// entry shape whose purpose is verbatim caller text: the pool +// dispatchers all `format!` their own, and `{:?}` in those formats +// already escapes, so a hostile purpose cannot reach a row through them. + +/// **The spoofing property, and the whole reason the escaping exists.** +/// +/// A purpose crafted to look like a row boundary followed by a plausible +/// job row does not produce a second row. Asserted by COUNTING the rows, +/// not by looking for the escape sequence: a renderer that dropped the +/// purpose entirely would satisfy "no forged row" while destroying the +/// feature, so the escaped text is asserted present in the surviving row +/// as well. +#[test] +fn a_purpose_shaped_like_a_row_boundary_does_not_produce_a_second_row() { + let mut state = editor(); + let (job_id, _token) = state.async_runtime.register_external( + JobKind::LspRequest, + None, + "lsp definition\n#99 grep 0ms forged \ + cancelled by nobody", + ); + exec(&state, "BUF = pmacs.workers.show()"); + let text: String = eval(&state, "return BUF:slice(0, BUF:len())"); + + let job_rows: Vec<&str> = text.lines().filter(|line| line.starts_with('#')).collect(); + assert_eq!( + job_rows.len(), + 1, + "one job must render as exactly ONE row:\n{text}" + ); + assert!( + job_rows[0].contains("lsp definition\\n#99"), + "and the break must be rendered, escaped, INSIDE that row:\n{text}" + ); + assert!( + !text.contains("\n#99"), + "no line may begin with the forged id:\n{text}" + ); + + // `#228`'s other half, and what makes this a rendering decision + // rather than data loss: the free-form surface still hands Lua every + // byte, unescaped. + let raw = active_purposes(&state); + assert!( + raw.iter().any(|purpose| purpose.contains('\n')), + "pmacs.workers.snapshot() is the raw path and must stay raw: {raw:?}" + ); + + exec(&state, "pmacs.workers.hide()"); + state.async_runtime.complete_external_cancelled(job_id); + pump(&mut state); +} + +/// **The modeline is one line, and that is enforced where the modeline +/// reads.** +/// +/// Two assertions, because they exclude different failures: the segment +/// carries no break at all (a composed modeline splicing one would +/// misplace every segment after it), and the escaped text survives the +/// real per-frame paint rather than only the evaluator. +#[test] +fn a_purpose_that_spans_lines_reaches_the_modeline_as_one_line() { + let mut state = editor(); + let (job_id, _token) = state.async_runtime.register_external( + JobKind::LspRequest, + None, + "lsp didOpen\nfile:///tmp/x.rs", + ); + + let segment = activity_segment(&state).expect("work is in flight, so a segment exists"); + assert!( + !segment.contains(['\n', '\r']), + "a modeline segment is ONE line: {segment:?}" + ); + assert_eq!( + segment, "⋯1 lsp didOpen\\nfile:///tmp/x.rs", + "and the break is escaped in place, not clipped away" + ); + + let cells = paint(&state, 24, 160); + let modeline = row_text(&cells, 160, 22); + assert!( + modeline.contains("⋯1 lsp didOpen\\nfile:///tmp/x.rs"), + "the painted modeline must carry it too; got {modeline:?}" + ); + + state.async_runtime.complete_external_cancelled(job_id); + pump(&mut state); +} + +/// **A purpose with no control characters is byte-identical after +/// escaping** — on both surfaces. +/// +/// The fixture is chosen to break a careless escaper: a literal +/// backslash (which a JSON-style escaper would double, and which is +/// deliberately NOT escaped here — no number of backslashes produces a +/// second row), a `\v` that is text rather than a vertical tab, quotes, +/// and a non-ASCII character. +#[test] +fn a_purpose_with_no_control_characters_is_unchanged_by_the_boundary() { + const PURPOSE: &str = r#"grep "fn \d+" in /tmp/pro—ject\v2"#; + + let mut state = editor(); + let (job_id, _token) = + state + .async_runtime + .register_external(JobKind::LspRequest, None, PURPOSE); + + exec(&state, "BUF = pmacs.workers.show()"); + let text: String = eval(&state, "return BUF:slice(0, BUF:len())"); + assert!( + text.contains(PURPOSE), + "the *workers* row must reproduce an ordinary purpose byte for byte:\n{text}" + ); + + assert_eq!( + activity_segment(&state).as_deref(), + Some(format!("⋯1 {PURPOSE}").as_str()), + "and so must the modeline segment" + ); + + exec(&state, "pmacs.workers.hide()"); + state.async_runtime.complete_external_cancelled(job_id); + pump(&mut state); +}