fix(workers): a safe display-text boundary for purpose and handler names
Review round 2, findings P2a and P2b, plus P3's stale recovery summaries. Three defects, and the fix is deliberately different in each place because the constraint is. ## P2a — invalid UTF-8 bypassed the `purpose` diagnostic `required_purpose` read the field with `value.to_str()?`. Lua strings are BYTE strings, so `purpose = string.char(255)` is a value a caller can write, and `?` surfaced mlua's generic conversion error BEFORE this lane's own diagnostic was ever constructed: the caller was told neither the field nor the rule. **This is the third time this project has hit the class** — an unowned Lua string converted with `?` ahead of the owned message; the destination-capture lane corrected the same shape two rounds ago. It refused before spawning and nothing leaked, so the defect was the message, not the outcome. The conversion failure is now mapped onto this function's own message, and the new acceptance row asserts on message CONTENT so retyping the read as a bare `?` breaks the test rather than silently degrading the error. Auditing the rest of the lane's diff for the same class turned up exactly one more: `_push_dispatch_name` took `name: String`, so a registered handler name that was not valid UTF-8 failed at first dispatch with mlua's generic message. It now takes `mlua::String` and maps that failure onto an owned diagnostic naming the argument and the rule. Those are the only two Lua-string reads this lane added; every other binding it adds takes `()`. ## P2b — no safe display-text boundary. Two halves, two different fixes ### Handler names are refused at the source `pmacs.workers.register` type-checked its name and nothing more, which was defensible while the name died inside `dispatch`. It no longer dies there: the ambient carries it into every job the handler allocates and composes it into `purpose`, which `*workers*` and the modeline both render. So it now gets `purpose`'s meaningful-value standard — non-empty, not whitespace-only — plus control characters, which have no legitimate place in a registered identifier. ### Purposes are ESCAPED at presentation, not rejected at the registry A purpose may legitimately contain a newline: a filesystem path can, and `pmacs-magit`'s spawn purpose is a whole argv. **This is the shape of the `#228` decision, and it is consistent with it** — the one-line constraint belongs to the surface that has it, not to the registry that does not. There, `Command.description` stays free-form and the two single-row consumers clip with `description_first_line`. Here the equivalent is escaping rather than clipping, because a purpose's later words are load-bearing: an argv's second word says which file, and a clip would drop it silently. `purpose_for_one_row` states the property it exists for: **a row must not be able to forge another row.** It escapes `\n`, `\r`, `\t` and the rest of the Unicode `Cc` class (which covers ESC, so a purpose cannot open a terminal escape sequence either), borrows unchanged when there is nothing to escape — making byte-identity structural rather than asserted — and deliberately does NOT escape backslashes: no number of them produces a second row, and doubling them would cost byte-identity for ordinary text. Two surfaces call it: the `*workers*` rows, and `ActivitySummary`, which exists for one consumer that has exactly one row. `pmacs.workers.snapshot()` is this lane's `describe-command` and stays raw, which is what makes this a rendering decision rather than data loss — asserted, not assumed. ## P3 — two stale recovery summaries `docs/worker-identity-framing.md` still said "Implementation may proceed"; it is implemented. `docs/active-work.md` still said Stage 1 takes the "first two" of owner/purpose/parent — `owner` was REMOVED in revision 2, so it takes one of the three, and the claim the whole `owner` argument overturned was still standing in the volatile state of record. Both fixed section-locally. ## Verification `tests/worker_identity_acceptance.rs`, 18 -> 24 tests: * invalid-UTF-8 purpose refused by THIS lane's message, asserted on content, alongside the absent / empty / whitespace / wrong-type / metatable rows; * a whitespace-only handler name and a control-character one are each refused AT `register`, asserted on the error and on the handler not being installed (dispatch reports `unknown handler`); * a non-UTF-8 handler name is refused before the handler runs, with the dispatch-name stack left empty; * a purpose containing a newline renders as ONE row in `*workers*` and as one line in the modeline — through the real rendering path, the latter through a painted frame as well as the evaluator; * **a purpose crafted to look like a row boundary does not produce a second row** — asserted by counting rows, with the escaped text asserted present so a renderer that dropped the purpose entirely could not pass; * a purpose with no control characters is byte-identical on both surfaces, fixtured with a literal backslash, a literal `\v`, quotes and a non-ASCII character. Mutation-checked, seven guards, each failing its own test and no other: the purpose UTF-8 diagnostic; the `_push_dispatch_name` one; the register whitespace guard; the register control-character guard; the `*workers*` call site; the `ActivitySummary` call site; and `purpose_for_one_row` itself neutered to the identity, which fails both surfaces' tests and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai
This commit is contained in:
parent
37a81227c7
commit
70262888b4
|
|
@ -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 `"<name>: <purpose>"`, 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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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(),
|
||||
})
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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<RestartPolicy> {
|
|||
///
|
||||
/// # 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<String> {
|
||||
let purpose = match table.raw_get::<mlua::Value>("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 \
|
||||
|
|
|
|||
|
|
@ -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:<PURPOSE_WIDTH$} {status}"
|
||||
|
|
@ -211,7 +217,7 @@ fn write_completed_row(text: &mut String, job: &CompletedJobInfo) {
|
|||
let key = job.supersede_key.as_deref().unwrap_or("");
|
||||
let outcome = format_outcome(&job.outcome);
|
||||
let age = format_duration_ms(job.settled_age_ms);
|
||||
let purpose = &job.purpose;
|
||||
let purpose = purpose_for_one_row(&job.purpose);
|
||||
let _ = writeln!(
|
||||
text,
|
||||
"{id:<7} {kind:<11} {duration:>9} {key:<11} {purpose:<PURPOSE_WIDTH$} {outcome} ({age} ago)"
|
||||
|
|
|
|||
|
|
@ -274,16 +274,24 @@ fn every_entry_shape_records_what_its_work_is() {
|
|||
/// purpose defaulting to the label hands every caller back the
|
||||
/// convention this lane exists to replace.
|
||||
///
|
||||
/// Four refusals, each asserted the same way — the call raises, the
|
||||
/// message names the field, and **the process list is unchanged**,
|
||||
/// because a validation that rejects after spawning has already done the
|
||||
/// thing it was rejecting:
|
||||
/// Six refusals, each asserted the same way — the call raises, **the
|
||||
/// message names the field and the rule**, and the process list is
|
||||
/// unchanged, because a validation that rejects after spawning has
|
||||
/// already done the thing it was rejecting:
|
||||
///
|
||||
/// * absent;
|
||||
/// * empty, and whitespace-only — these satisfy the type and defeat the
|
||||
/// point exactly as copying the label would (R42 rejects
|
||||
/// whitespace-only config descriptions for the same reason);
|
||||
/// * wrong type;
|
||||
/// * **not valid UTF-8**. Lua strings are BYTE strings, so
|
||||
/// `purpose = string.char(255)` is a value a caller can write, and
|
||||
/// converting it with `?` would surface mlua's generic conversion
|
||||
/// error before this lane's own diagnostic was ever constructed. The
|
||||
/// refusal is not the interesting part — it refuses either way, and
|
||||
/// nothing spawns either way — the MESSAGE is, which is why the
|
||||
/// assertion is on content. Retyping this read as a bare `?` breaks
|
||||
/// the row rather than silently degrading the error;
|
||||
/// * **metatable-provided**, which is the `stdin`/`group` raw-read
|
||||
/// posture: a spec table is plain data, so a purpose cannot be
|
||||
/// smuggled in through `__index`.
|
||||
|
|
@ -313,6 +321,12 @@ fn spawning_without_a_real_purpose_is_refused_and_starts_nothing() {
|
|||
r#"{ label = "x", purpose = 7, command = "/bin/sh", args = { "-c", "sleep 5" } }"#,
|
||||
"purpose must be a string",
|
||||
),
|
||||
(
|
||||
"invalid UTF-8",
|
||||
r#"{ label = "x", purpose = "run " .. string.char(255),
|
||||
command = "/bin/sh", args = { "-c", "sleep 5" } }"#,
|
||||
"purpose must be valid UTF-8",
|
||||
),
|
||||
(
|
||||
"metatable-provided",
|
||||
r#"setmetatable(
|
||||
|
|
@ -379,6 +393,147 @@ fn process_list_still_hides_terminal_ptys() {
|
|||
// 2 — the dispatch-name ambient (Q#W-2)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/// **A registered handler name is DISPLAY TEXT, and gets `purpose`'s
|
||||
/// meaningful-value standard.**
|
||||
///
|
||||
/// Before this lane the name died inside `dispatch` and a type check was
|
||||
/// the whole of what it needed. It no longer dies there: the ambient
|
||||
/// carries it into every job the handler allocates and composes it into
|
||||
/// `purpose`, which `*workers*` and the modeline both render. So empty
|
||||
/// and whitespace-only are refused here for the reason they are refused
|
||||
/// in `required_purpose` — they satisfy the type and say nothing.
|
||||
///
|
||||
/// **Refused at `register`, and asserted twice**: the call raises with a
|
||||
/// message naming the rule, *and* nothing is installed under the name —
|
||||
/// a validation that stored the handler first would have registered the
|
||||
/// thing it was rejecting.
|
||||
#[test]
|
||||
fn a_handler_name_that_says_nothing_is_refused_at_registration() {
|
||||
let mut state = editor();
|
||||
for (label, name_expr) in [("empty", r#""""#), ("whitespace-only", r#"" \t ""#)] {
|
||||
let (ok, err): (bool, String) = eval(
|
||||
&state,
|
||||
&format!(
|
||||
"local ok, err = pcall(pmacs.workers.register, {name_expr}, function() end)
|
||||
return ok, tostring(err)"
|
||||
),
|
||||
);
|
||||
assert!(!ok, "{label}: register must refuse");
|
||||
assert!(
|
||||
err.contains("must not be empty or whitespace-only"),
|
||||
"{label}: the refusal must name the rule; got {err:?}"
|
||||
);
|
||||
let (dispatched, dispatch_err): (bool, String) = eval(
|
||||
&state,
|
||||
&format!(
|
||||
"local ok, err = pcall(pmacs.workers.dispatch, {name_expr})
|
||||
return ok, tostring(err)"
|
||||
),
|
||||
);
|
||||
assert!(!dispatched, "{label}: a refused name must install nothing");
|
||||
assert!(
|
||||
dispatch_err.contains("unknown handler"),
|
||||
"{label}: and the name must be genuinely absent from the table; \
|
||||
got {dispatch_err:?}"
|
||||
);
|
||||
}
|
||||
pump(&mut state);
|
||||
}
|
||||
|
||||
/// **Control characters in a handler name are refused at the source —
|
||||
/// and this is the one rule `purpose` deliberately does NOT get.**
|
||||
///
|
||||
/// The asymmetry is the design (`#228`'s shape). 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.
|
||||
/// A handler name is an identifier a package chooses for itself and
|
||||
/// hands back to `dispatch`; a control character in one is a mistake or
|
||||
/// an attempt at one, and refusing costs nobody anything.
|
||||
///
|
||||
/// The rows are chosen to be **not** whitespace-only, so this test
|
||||
/// cannot pass on the previous guard: a newline mid-word, an ESC (which
|
||||
/// starts a terminal escape sequence), and a NUL.
|
||||
#[test]
|
||||
fn a_handler_name_with_control_characters_is_refused_at_registration() {
|
||||
let mut state = editor();
|
||||
for (label, name_expr) in [
|
||||
("newline", r#""index\ner""#),
|
||||
("escape", r#""index" .. string.char(27) .. "[31mer""#),
|
||||
("nul", r#""index" .. string.char(0) .. "er""#),
|
||||
] {
|
||||
let (ok, err): (bool, String) = eval(
|
||||
&state,
|
||||
&format!(
|
||||
"local ok, err = pcall(pmacs.workers.register, {name_expr}, function() end)
|
||||
return ok, tostring(err)"
|
||||
),
|
||||
);
|
||||
assert!(!ok, "{label}: register must refuse");
|
||||
assert!(
|
||||
err.contains("must not contain control characters"),
|
||||
"{label}: the refusal must name the rule; got {err:?}"
|
||||
);
|
||||
let (dispatched, dispatch_err): (bool, String) = eval(
|
||||
&state,
|
||||
&format!(
|
||||
"local ok, err = pcall(pmacs.workers.dispatch, {name_expr})
|
||||
return ok, tostring(err)"
|
||||
),
|
||||
);
|
||||
assert!(!dispatched, "{label}: a refused name must install nothing");
|
||||
assert!(
|
||||
dispatch_err.contains("unknown handler"),
|
||||
"{label}: and the name must be genuinely absent from the table; \
|
||||
got {dispatch_err:?}"
|
||||
);
|
||||
}
|
||||
pump(&mut state);
|
||||
}
|
||||
|
||||
/// **The third rule a display-text name needs, enforced at the one
|
||||
/// place that can see it.**
|
||||
///
|
||||
/// Lua strings are BYTE strings and Lua 5.1 has no `utf8` library, so
|
||||
/// `register` cannot tell a valid name from arbitrary bytes —
|
||||
/// `string.char(255)` is neither whitespace nor a control character by
|
||||
/// `%c`. The name crosses into Rust exactly once, at
|
||||
/// `_push_dispatch_name`, and that is where the byte-level rule is
|
||||
/// enforced. **This is the P2a class again**: an `mlua`-driven
|
||||
/// conversion there would refuse with a generic message naming neither
|
||||
/// the argument nor the rule, so the conversion failure is mapped onto
|
||||
/// an owned diagnostic instead.
|
||||
///
|
||||
/// The refusal lands at `dispatch` rather than at `register`, which is
|
||||
/// later than ideal and is asserted as such — but it is still **before
|
||||
/// the handler runs**, so a name that cannot be displayed never reaches
|
||||
/// a job's purpose.
|
||||
#[test]
|
||||
fn a_handler_name_that_is_not_valid_utf8_is_refused_before_the_handler_runs() {
|
||||
let mut state = editor();
|
||||
let (ok, err): (bool, String) = eval(
|
||||
&state,
|
||||
"RAN = false
|
||||
pmacs.workers.register('bad' .. string.char(255), function() RAN = true end)
|
||||
local ok, err = pcall(pmacs.workers.dispatch, 'bad' .. string.char(255))
|
||||
return ok, tostring(err)",
|
||||
);
|
||||
assert!(!ok, "dispatch must refuse a name it cannot display");
|
||||
assert!(
|
||||
err.contains("handler name must be valid UTF-8"),
|
||||
"the refusal must name the argument and the rule; got {err:?}"
|
||||
);
|
||||
assert!(
|
||||
!eval::<bool>(&state, "return RAN"),
|
||||
"and it must refuse BEFORE running the handler"
|
||||
);
|
||||
assert!(
|
||||
!eval::<bool>(&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);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue