fix(process): pmacs.process.spawn REQUIRES a purpose — review blocker
Review round 1 on worker identity Stage 1. The lane shipped `purpose` as
a required field on `ProcessSpec` but made it OPTIONAL at the
`pmacs.process.spawn` Lua surface, defaulting to `label`.
**That preserved compatibility and delivered nothing.** `COHERENCE.md`
§9's complaint about `ProcessSpec` is precisely that `label` is
"caller-supplied, unvalidated convention" — so a purpose defaulting to
the label hands every existing caller back the exact convention this lane
exists to replace. The approved framing said required; this makes it
required where callers actually are.
The two fields answer different questions and neither substitutes for the
other. `label` IDENTIFIES — `lsp:rust-analyzer`, a terminal's buffer
name — so two processes running the same binary can be told apart.
`purpose` DESCRIBES: it answers "what is happening", which is what §3's
promise of visible asynchronous work is about, and which a label chosen
for uniqueness routinely does not answer.
**The refusal covers five shapes, not one.** Absent; empty;
whitespace-only; wrong type; and metatable-provided. The middle two
matter 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, and a required
field that accepts `""` is not required in any sense a reader benefits
from. The read is RAW, matching the posture `stdin` and `group` already
document in the same function: a spec table is plain data, so `__index`
cannot smuggle a purpose in.
Every refusal also asserts **the process list is unchanged**. A
validation that rejects after spawning has already done the thing it was
rejecting.
**This is a BREAKING CHANGE to a public Lua API, taken deliberately and
now rather than later.** Weighed and reported rather than decided
silently: §10 grades extension trust "missing (one class)" and P7 package
lifecycle has not started, so the third-party population calling this
binding is ~zero and the cost of the change only rises from here. Checked
for a reason that would be wrong and found none — `pmacs.process.spawn`
has no API-reference documentation and no stability promise anywhere in
`docs/`; the guide's only mentions are an audit-rule classification and a
pointer to the bundled REPL, and its semver language governs *packages'*
own versioning, not pmacs's Lua surface. `lua_to_spec` has exactly one
caller, so the blast radius is this one binding.
Eleven executable call sites updated, each with a real description rather
than the label copied across — copying it would satisfy the type and
defeat the point as surely as the default did:
builtin/packages/repl/init.lua "interactive <interpreter> session"
builtin/runtime/compile.lua "compiling: <cmdline>"
builtin/runtime/lean.lua "checking the Lean toolchain version…"
tests/fixtures/pmacs-magit/status.lua the full argv, not just the
subcommand the label carries — "git
log" and "git log --oneline -20" are
one label and different work
tests/compile_mode_acceptance.rs (4), tests/m4_acceptance.rs (1),
tests/worker_identity_acceptance.rs (2)
`lean.lua`'s site is the clearest case for the field: its comment said
the label was where "a user wondering why their editor touched `lake`
finds an owner" — one string doing identity AND explanation, which is the
conflation being undone. The label stays a key; the purpose is now the
sentence.
Two references are deliberately NOT updated: `src/audit/mod.rs` and
`tests/m7_9_acceptance.rs` contain `pmacs.process.spawn("ls")` as **audit
fixture source text**. It is lexed by the audit engine, never executed,
and editing it would change what those rule tests scan.
`required_purpose` is extracted rather than inlined because inlining it
pushed `lua_to_spec` past the 100-line clippy bound — the validation has
its own rules and its own rationale, so it gets its own function instead
of an `#[allow]`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai
This commit is contained in:
parent
1ca76e055b
commit
d01cde9432
|
|
@ -259,6 +259,10 @@ function repl.spawn(opts)
|
|||
|
||||
local spec = {
|
||||
label = name,
|
||||
-- Worker identity Stage 1: the label is the REPL's session name,
|
||||
-- which distinguishes two REPLs from each other and says nothing
|
||||
-- about what is running. The purpose names the interpreter.
|
||||
purpose = "interactive " .. h._display_name .. " session",
|
||||
command = argv[1],
|
||||
args = args,
|
||||
pty = { rows = rows, cols = cols, mode = "raw" },
|
||||
|
|
|
|||
|
|
@ -875,6 +875,11 @@ local function start_run(slot, cmdline, opts)
|
|||
-- stdin, own process group, TERM=dumb.
|
||||
local spec = {
|
||||
label = slot.label,
|
||||
-- Worker identity Stage 1: the label distinguishes one compile slot
|
||||
-- from another; the purpose is the command the user actually asked
|
||||
-- for, which is what they want to see when they wonder why the
|
||||
-- editor is busy.
|
||||
purpose = "compiling: " .. cmdline,
|
||||
command = "/bin/sh",
|
||||
args = { "-c", "exec 2>&1; " .. cmdline },
|
||||
env = { TERM = "dumb" },
|
||||
|
|
|
|||
|
|
@ -494,10 +494,14 @@ local function start_probe(root)
|
|||
-- "lake": a user pointing `command` at an absolute path to lake should
|
||||
-- have THAT probed, not whatever `lake` resolves to on PATH.
|
||||
local spec = {
|
||||
-- COHERENCE §9: `ProcessSpec.label` is the only identity a process
|
||||
-- carries, and it is what `pmacs.process.list` renders. A user
|
||||
-- wondering why their editor touched `lake` finds an owner here.
|
||||
-- COHERENCE §9: `ProcessSpec.label` identifies the process, and it
|
||||
-- is what `pmacs.process.list` renders alongside the purpose. A user
|
||||
-- wondering why their editor touched `lake` finds it here.
|
||||
label = "lean:lake-version-probe",
|
||||
-- Worker identity Stage 1: the label was carrying both jobs — the
|
||||
-- identity AND the explanation — which is the conflation the purpose
|
||||
-- field exists to undo. The label stays a key; this is the sentence.
|
||||
purpose = "checking the Lean toolchain version before starting a server",
|
||||
command = cfg.command,
|
||||
args = { "--version" },
|
||||
stdin = "null",
|
||||
|
|
|
|||
|
|
@ -8746,24 +8746,66 @@ fn parse_restart(name: &str) -> mlua::Result<RestartPolicy> {
|
|||
})
|
||||
}
|
||||
|
||||
/// Read the **required** `purpose` out of a `pmacs.process.spawn` spec
|
||||
/// (worker identity Stage 1, `COHERENCE.md` §9).
|
||||
///
|
||||
/// An earlier revision of this lane defaulted the field to `label` so
|
||||
/// that existing callers kept working. That preserved compatibility and
|
||||
/// delivered nothing: §9's complaint about `ProcessSpec` is precisely
|
||||
/// that `label` is "caller-supplied, unvalidated convention", so a
|
||||
/// purpose defaulting to the label hands every caller back the
|
||||
/// convention this lane exists to replace.
|
||||
///
|
||||
/// The two fields answer different questions and neither substitutes for
|
||||
/// the other. `label` **identifies** — `lsp:rust-analyzer`, a terminal's
|
||||
/// buffer name — so that two processes running the same binary can be
|
||||
/// told apart. `purpose` **describes**: it answers "what is happening",
|
||||
/// which is the question §3's promise of visible asynchronous work is
|
||||
/// about, and which a label chosen for uniqueness routinely does not
|
||||
/// answer.
|
||||
///
|
||||
/// # 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.
|
||||
///
|
||||
/// 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::Nil) => {
|
||||
return Err(mlua::Error::external(
|
||||
"pmacs.process.spawn: purpose is required — a short description of what \
|
||||
this process is DOING, e.g. purpose = \"running the project's test suite\". \
|
||||
It is not the label: the label identifies the process, the purpose says \
|
||||
what it is for.",
|
||||
));
|
||||
}
|
||||
Ok(other) => {
|
||||
return Err(mlua::Error::external(format!(
|
||||
"pmacs.process.spawn: purpose must be a string; got {}",
|
||||
other.type_name()
|
||||
)));
|
||||
}
|
||||
Err(error) => return Err(error),
|
||||
};
|
||||
if purpose.trim().is_empty() {
|
||||
return Err(mlua::Error::external(
|
||||
"pmacs.process.spawn: purpose must not be empty or whitespace-only",
|
||||
));
|
||||
}
|
||||
Ok(purpose)
|
||||
}
|
||||
|
||||
fn lua_to_spec(table: &Table) -> mlua::Result<ProcessSpec> {
|
||||
let label: String = table.get("label").unwrap_or_else(|_| "unnamed".to_owned());
|
||||
let command: String = table.get("command")?;
|
||||
// Worker identity Stage 1: required on the Rust struct, optional at
|
||||
// this surface, falling back to the label.
|
||||
//
|
||||
// Requiring it here would break every existing `pmacs.process.spawn`
|
||||
// caller, and the compiler obligation this lane is buying is on the
|
||||
// *Rust* construction sites — the ones a future field would silently
|
||||
// skip. A Lua caller that supplies nothing gets its own label back,
|
||||
// which is what the caller already chose to call this work; it is
|
||||
// less informative than a real description but it is not a
|
||||
// fabrication, which is the bar `owner` failed (framing §3).
|
||||
let purpose: String = table
|
||||
.get::<Option<String>>("purpose")
|
||||
.ok()
|
||||
.flatten()
|
||||
.unwrap_or_else(|| label.clone());
|
||||
let purpose = required_purpose(table)?;
|
||||
let args: Vec<String> = table.get("args").unwrap_or_default();
|
||||
let cwd: Option<String> = table.get("cwd").ok().flatten();
|
||||
let env_table: Option<Table> = table.get("env").ok().flatten();
|
||||
|
|
|
|||
|
|
@ -1836,7 +1836,8 @@ fn r1f6_wrong_spec_types_error_instead_of_defaulting() {
|
|||
&s,
|
||||
r#"
|
||||
local ok, err = pcall(pmacs.process.spawn,
|
||||
{ label = "t", command = "/bin/true", stdin = true })
|
||||
{ label = "t", purpose = "type-check probe", command = "/bin/true",
|
||||
stdin = true })
|
||||
return ok, tostring(err)
|
||||
"#,
|
||||
);
|
||||
|
|
@ -1846,7 +1847,8 @@ fn r1f6_wrong_spec_types_error_instead_of_defaulting() {
|
|||
&s,
|
||||
r#"
|
||||
local ok, err = pcall(pmacs.process.spawn,
|
||||
{ label = "t", command = "/bin/true", group = "true" })
|
||||
{ label = "t", purpose = "type-check probe", command = "/bin/true",
|
||||
group = "true" })
|
||||
return ok, tostring(err)
|
||||
"#,
|
||||
);
|
||||
|
|
@ -2234,7 +2236,8 @@ fn r3f3_spec_fields_are_raw_reads_metatables_not_honored() {
|
|||
&s,
|
||||
r#"
|
||||
local spec = setmetatable(
|
||||
{ label = "mt", command = "/bin/sh", args = { "-c", "sleep 30" } },
|
||||
{ label = "mt", purpose = "raw-read probe", command = "/bin/sh",
|
||||
args = { "-c", "sleep 30" } },
|
||||
{ __index = function(_, k)
|
||||
if k == "group" then return true end
|
||||
return nil
|
||||
|
|
@ -2265,7 +2268,8 @@ fn r3f3_spec_fields_are_raw_reads_metatables_not_honored() {
|
|||
&s,
|
||||
r#"
|
||||
local spec = setmetatable(
|
||||
{ label = "mt2", command = "/bin/sh", args = { "-c", "exit 0" } },
|
||||
{ label = "mt2", purpose = "raw-read probe", command = "/bin/sh",
|
||||
args = { "-c", "exit 0" } },
|
||||
{ __index = function() error("hostile spec metatable") end })
|
||||
local ok = pcall(pmacs.process.spawn, spec)
|
||||
return ok
|
||||
|
|
|
|||
|
|
@ -54,6 +54,10 @@ function M.run_git(args, opts)
|
|||
opts = opts or {}
|
||||
local id = pmacs.process.spawn {
|
||||
label = "git " .. (args[1] or ""),
|
||||
-- Worker identity Stage 1: `purpose` is required. The full argument
|
||||
-- vector, not just the subcommand the label carries -- "git log" and
|
||||
-- "git log --oneline -20" are the same label and different work.
|
||||
purpose = "git " .. table.concat(args, " "),
|
||||
command = "git",
|
||||
args = args,
|
||||
cwd = opts.cwd,
|
||||
|
|
|
|||
|
|
@ -1173,6 +1173,7 @@ fn m4_4_lua_surface_drives_lifecycle() {
|
|||
r#"
|
||||
local id = pmacs.process.spawn {
|
||||
label = "lua-hello",
|
||||
purpose = "greeting the Lua surface end to end",
|
||||
command = "/bin/sh",
|
||||
args = { "-c", "printf hi-from-lua && exit 0" },
|
||||
}
|
||||
|
|
|
|||
|
|
@ -264,33 +264,84 @@ fn every_entry_shape_records_what_its_work_is() {
|
|||
pump(&mut state);
|
||||
}
|
||||
|
||||
/// A `pmacs.process.spawn` caller that supplies no purpose keeps
|
||||
/// working, and gets its own label back rather than an empty field.
|
||||
/// **`pmacs.process.spawn` REFUSES a spec with no purpose, and spawns
|
||||
/// nothing.**
|
||||
///
|
||||
/// The Rust struct's field is required — the compiler enforces that at
|
||||
/// every construction site. This surface is deliberately lenient,
|
||||
/// because requiring it here would break every existing caller for no
|
||||
/// coverage the compiler is not already providing.
|
||||
/// An earlier revision of this lane defaulted the field to `label` so
|
||||
/// that existing callers kept working. That preserved compatibility and
|
||||
/// delivered nothing: §9's complaint about `ProcessSpec` is precisely
|
||||
/// that `label` is "caller-supplied, unvalidated convention", so a
|
||||
/// 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:
|
||||
///
|
||||
/// * 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;
|
||||
/// * **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`.
|
||||
#[test]
|
||||
fn a_process_spawned_without_a_purpose_falls_back_to_its_label() {
|
||||
fn spawning_without_a_real_purpose_is_refused_and_starts_nothing() {
|
||||
let mut state = editor();
|
||||
exec(
|
||||
&state,
|
||||
r#"P = pmacs.process.spawn {
|
||||
label = "legacy-caller",
|
||||
command = "/bin/sh",
|
||||
args = { "-c", "sleep 5" },
|
||||
}"#,
|
||||
);
|
||||
let purpose: String = eval(
|
||||
&state,
|
||||
"for _, row in ipairs(pmacs.process.list()) do
|
||||
if row.label == 'legacy-caller' then return row.purpose end
|
||||
end
|
||||
return '<absent>'",
|
||||
);
|
||||
assert_eq!(purpose, "legacy-caller");
|
||||
exec(&state, "pmacs.process.terminate(P)");
|
||||
let baseline: usize = eval(&state, "return #pmacs.process.list()");
|
||||
|
||||
for (label, spec, expected) in [
|
||||
(
|
||||
"absent",
|
||||
r#"{ label = "x", command = "/bin/sh", args = { "-c", "sleep 5" } }"#,
|
||||
"purpose is required",
|
||||
),
|
||||
(
|
||||
"empty",
|
||||
r#"{ label = "x", purpose = "", command = "/bin/sh", args = { "-c", "sleep 5" } }"#,
|
||||
"must not be empty",
|
||||
),
|
||||
(
|
||||
"whitespace-only",
|
||||
r#"{ label = "x", purpose = " ", command = "/bin/sh", args = { "-c", "sleep 5" } }"#,
|
||||
"must not be empty",
|
||||
),
|
||||
(
|
||||
"wrong type",
|
||||
r#"{ label = "x", purpose = 7, command = "/bin/sh", args = { "-c", "sleep 5" } }"#,
|
||||
"purpose must be a string",
|
||||
),
|
||||
(
|
||||
"metatable-provided",
|
||||
r#"setmetatable(
|
||||
{ label = "x", command = "/bin/sh", args = { "-c", "sleep 5" } },
|
||||
{ __index = function(_, k)
|
||||
if k == "purpose" then return "smuggled" end
|
||||
return nil
|
||||
end })"#,
|
||||
"purpose is required",
|
||||
),
|
||||
] {
|
||||
let (ok, err): (bool, String) = eval(
|
||||
&state,
|
||||
&format!(
|
||||
"local ok, err = pcall(pmacs.process.spawn, {spec})
|
||||
return ok, tostring(err)"
|
||||
),
|
||||
);
|
||||
assert!(!ok, "{label}: spawn must refuse");
|
||||
assert!(
|
||||
err.contains(expected),
|
||||
"{label}: the refusal must name the field and the rule; got {err:?}"
|
||||
);
|
||||
assert_eq!(
|
||||
eval::<usize>(&state, "return #pmacs.process.list()"),
|
||||
baseline,
|
||||
"{label}: a refused spawn must start no process"
|
||||
);
|
||||
}
|
||||
pump(&mut state);
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue