From f86c966090d0f5847be1681202b6a86568bf0922 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 21 Jul 2026 16:50:48 -0400 Subject: [PATCH] fix(config): reject wrongly-typed spec fields; make trim-on-save buffer-aware Review round 1, findings 2-4 plus doc notes. Finding 1 landed in fd80bcb. Finding 3 --- spec fields meaningless for the declared type are now rejected. DEFINE_SPEC_FIELDS whitelists all nine keys for every type and the kind parser only reads its own arm's fields, so `{ type = "string", choices = {...} }` silently defined a string that accepts anything (the author meant enum) and `min` on a boolean was dropped. These are typo-shaped bugs the R50 whitelist structurally cannot see: the key is spelled correctly, it is on the wrong type. `check_fields_relevant_to_kind` closes it with a pointed error naming the misplaced field, and a companion test pins that each field is still accepted where it belongs, including `min`/`max` on number as well as integer. Finding 4 --- the after_buffer_removed purge had no end-to-end test. Every existing test called ConfigRegistry::remove_buffer directly, so deleting the three lines wired into mod.rs would have left the whole suite green. The new acceptance test kills a buffer through pmacs.buffer.remove (the real remove_buffer_and_fire route) and asserts the locals are gone; bite-verified by removing the hunk and watching it fail. Finding 2 (the half with a natural buffer) --- editing.trim-on-save is now resolved against the buffer being saved rather than the global chain. Reading globally meant set_local was accepted, stored, and reported by describe, then never consulted: a pin the user believes in that does nothing, which is the shape F1 exists to prevent. Two tests, one for the override and one for the global fallback the change could have broken; the override test fails against the old global read. Both new save tests initially passed VACUOUSLY and were rewritten: pmacs.editor.save() is the raw save, while buffer.before-save fires inside the buffer.save COMMAND (default.lua:224), and save() no-ops on an unmodified buffer --- so the original form asserted on a file that was never rewritten. They now insert content to dirty the buffer and go through pmacs.command.invoke("buffer.save"). The other half of finding 2 --- a per-buffer autosave.interval-ms is semantically meaningless yet still accepted --- is recorded as a named deferral proposing a define-time `scope = "global"` flag, alongside deferrals for bound-parse field naming and StartupOnly reset symmetry. Also recorded: interval_ms(1e30) now raises instead of storing a nonsense float, an improvement but a real divergence from "the wrapper's shape stays exactly as it was". Doc: the module header cited framing revision 2; the shipped doc is revision 3, whose corrections are what the code implements. Gates: fmt, clippy -D warnings, --lib (1691), --lib --features crdt (1865), lua54 backend, config_registry_acceptance (16), editops (72), autosave (29), PMACS_REQUIRE_GPU=1 pmacs-gpu (109), and the full workspace sweep (2806 tests, exit 0). git diff --check clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- builtin/runtime/editops.lua | 10 +- docs/config-registry-framing.md | 29 ++++++ src/config_registry.rs | 2 +- src/lua_bindings/config.rs | 149 ++++++++++++++++++++++++++++ tests/config_registry_acceptance.rs | 94 ++++++++++++++++++ 5 files changed, 282 insertions(+), 2 deletions(-) diff --git a/builtin/runtime/editops.lua b/builtin/runtime/editops.lua index f78b677..6a071f5 100644 --- a/builtin/runtime/editops.lua +++ b/builtin/runtime/editops.lua @@ -874,7 +874,15 @@ pmacs.hook.add("buffer.before-save", function() -- either way). Both reports are pcall'd — a broken reporting -- channel must not resurrect the veto. local ok, err = pcall(function() - if pmacs.config.get("editing.trim-on-save") then + -- Resolved against the buffer being saved, not the global chain + -- (review round 1, finding 2). `buffer.before-save` fires for the + -- ACTIVE buffer -- which is also the one `trim_active` rewrites -- + -- so passing it here is what makes a buffer-local override mean + -- something. Reading globally would accept `set_local`, store it, + -- report it from `describe`, and then never consult it: a pin the + -- user believes in that does nothing, which is the same failure + -- shape F1 exists to prevent. + if pmacs.config.get("editing.trim-on-save", pmacs.window.buffer()) then trim_active("delete-trailing-whitespace (on save)") end end) diff --git a/docs/config-registry-framing.md b/docs/config-registry-framing.md index a6173b7..209b194 100644 --- a/docs/config-registry-framing.md +++ b/docs/config-registry-framing.md @@ -550,6 +550,14 @@ already-conforming value. The registry stays strict; the legacy API stays lenient; acceptance 27-28 pin both directions on inputs no current test covers. +One honest divergence from "the wrapper's shape stays exactly as it +was", found in review round 1: `pmacs.autosave.interval_ms(1e30)` +previously stored the float, and now raises `NonIntegral` because the +floored value exceeds `i64` range. The old behavior stored a nonsense +interval; the new one refuses it. An improvement, but a behavior change +at the extreme rather than a pure no-op migration, so it is recorded +here rather than claimed away. + Richer feature-specific APIs are explicitly not deleted: `pmacs.gpu.set_font` remains the font preference API until a separately framed migration decides how a daemon-global preference and @@ -809,6 +817,27 @@ points. - **`M-x list-settings`** as a listview panel (Q#CR11). - **A `customize`-style editing UI**, and a `:set`-style minibuffer command. +- **A `scope = "global"` define flag** (review round 1, finding 2). + `set_local` currently succeeds for any `Live` setting, including ones + whose consumer only ever reads the global chain. `editing.trim-on-save` + was fixed by making its consumer buffer-aware — the save hook now + resolves against the buffer being saved — but a per-buffer + `autosave.interval-ms` is *semantically* meaningless (there is one + sweep timer, not one per buffer) and the API still accepts it, stores + it, and reports it from `describe`. That is a stored value nothing + reads, the same shape F1 exists to prevent. The fix is a define-time + scope declaration letting the registry refuse `set_local` outright, + exactly as `StartupOnlyLocal` already refuses another meaningless + combination. Deferred rather than taken in review round 1 because it + adds public API surface after review. +- **Field-naming in bound-parse errors** — a bad `min` reports the + config name and the expected type but not *which* of `min`/`max` + offended. Cheap, but wants its own error variant rather than a + synthesized pseudo-name. +- **`reset(name, buf)` symmetry for `StartupOnly`** — `set_local` is + refused with `StartupOnlyLocal` but the buffer-local `reset` is + allowed. Unreachable-harmless today (no such local can exist), so a + rejection would be symmetry rather than a fix. --- diff --git a/src/config_registry.rs b/src/config_registry.rs index f089c6a..04db52a 100644 --- a/src/config_registry.rs +++ b/src/config_registry.rs @@ -2,7 +2,7 @@ //! Configuration settings. //! -//! Per `docs/config-registry-framing.md` (revision 2), every editor +//! Per `docs/config-registry-framing.md` (revision 3), every editor //! setting is a named, typed [`ConfigDefinition`] registered in a //! [`ConfigRegistry`], mirroring [`crate::command::CommandRegistry`] //! and [`crate::hook::HookRegistry`] in shape and error vocabulary: diff --git a/src/lua_bindings/config.rs b/src/lua_bindings/config.rs index 960576e..4739ce7 100644 --- a/src/lua_bindings/config.rs +++ b/src/lua_bindings/config.rs @@ -185,6 +185,22 @@ enum ConfigBindingError { name: String, }, + /// `define` carried a spec field that means nothing for the + /// declared `type` --- `choices` on a string, `min` on a boolean. + /// Correctly spelled, wrongly placed: the R50 whitelist cannot see + /// it, so it is checked against the kind instead. + #[error( + "config field `{field}` is meaningless for type = \"{got_type}\"; \ + `min`/`max` require integer or number, `choices` requires enum, \ + `allow_empty` requires string" + )] + FieldIrrelevantForType { + /// The offending field name. + field: &'static str, + /// The declared type it was paired with. + got_type: String, + }, + /// `define`'s `mutability` field wasn't `"live"` or `"startup"`. #[error("unknown config mutability `{got}`; expected one of: live, startup")] UnknownMutability { @@ -347,10 +363,42 @@ fn read_choices(spec: &Table, name: &str) -> mlua::Result> { Ok(choices) } +/// Reject spec fields that are meaningless for the declared `type` +/// (review round 1, finding 3). +/// +/// `DEFINE_SPEC_FIELDS` whitelists every key for every type, and the +/// kind parser below only reads the ones its own arm cares about --- +/// so without this check `{ type = "string", choices = {"a","b"} }` +/// silently defines a string that accepts anything (the author meant +/// `enum`), and `{ type = "boolean", min = 1 }` silently drops the +/// bound. Both are typo-shaped bugs of exactly the class R50 exists to +/// catch; the whitelist alone only catches misspelled keys, not +/// correctly-spelled keys on the wrong type. +fn check_fields_relevant_to_kind(spec: &Table, type_str: &str) -> mlua::Result<()> { + let numeric = matches!(type_str, "integer" | "number"); + for (field, allowed_for) in [ + ("min", numeric), + ("max", numeric), + ("choices", type_str == "enum"), + ("allow_empty", type_str == "string"), + ] { + if !allowed_for && !matches!(spec.raw_get::(field)?, Value::Nil) { + return Err(mlua::Error::external( + ConfigBindingError::FieldIrrelevantForType { + field, + got_type: type_str.to_owned(), + }, + )); + } + } + Ok(()) +} + fn parse_kind_and_default(spec: &Table, name: &str) -> mlua::Result<(ConfigKind, ConfigValue)> { let type_str: Option = spec.raw_get("type")?; let type_str = type_str.ok_or_else(|| mlua::Error::external(ConfigBindingError::MissingType))?; + check_fields_relevant_to_kind(spec, &type_str)?; let kind = match type_str.as_str() { "boolean" => ConfigKind::Boolean, @@ -931,6 +979,107 @@ mod tests { ); } + // ---- review round 1, finding 3: correctly spelled, wrongly typed -------- + + #[test] + fn define_rejects_spec_fields_meaningless_for_the_declared_type() { + // The R50 whitelist accepts all nine keys for every type, so + // these are invisible to it: each field below is spelled + // correctly but means nothing for the type it is paired with. + // Before the kind cross-check, `type = "string"` with `choices` + // silently defined a string that accepts ANYTHING -- the author + // plainly meant `enum` -- and `min` on a boolean was dropped. + for (spec, offender) in [ + ( + r#"{ name="a.b", description="d", type="string", default="x", choices={"a"} }"#, + "choices", + ), + ( + r#"{ name="a.b", description="d", type="boolean", default=true, min=1 }"#, + "min", + ), + ( + r#"{ name="a.b", description="d", type="boolean", default=true, max=1 }"#, + "max", + ), + ( + r#"{ name="a.b", description="d", type="integer", default=1, allow_empty=true }"#, + "allow_empty", + ), + ( + r#"{ name="a.b", description="d", type="enum", choices={"x"}, default="x", min=1 }"#, + "min", + ), + ] { + let (lua, _reg) = fresh(); + let err = run(&lua, &format!("pmacs.config.define {spec}")).unwrap_err(); + assert!( + err.to_string().contains(offender), + "the error must name the misplaced field {offender}: {err}" + ); + let n: i64 = eval(&lua, "return #pmacs.config.list()").unwrap(); + assert_eq!(n, 0, "a rejected define registers nothing"); + } + } + + #[test] + fn define_still_accepts_each_field_on_its_own_type() { + // The guard must not over-reject: every field remains legal + // where it belongs, including on `number` as well as `integer`. + let (lua, _reg) = fresh(); + for spec in [ + r#"{ name="a.int", description="d", type="integer", default=5, min=1, max=9 }"#, + r#"{ name="a.num", description="d", type="number", default=0.5, min=0.0, max=1.0 }"#, + r#"{ name="a.str", description="d", type="string", default="", allow_empty=true }"#, + r#"{ name="a.enum", description="d", type="enum", default="x", choices={"x","y"} }"#, + r#"{ name="a.bool", description="d", type="boolean", default=true }"#, + ] { + run(&lua, &format!("pmacs.config.define {spec}")) + .unwrap_or_else(|e| panic!("{spec} must be accepted: {e}")); + } + let n: i64 = eval(&lua, "return #pmacs.config.list()").unwrap(); + assert_eq!(n, 5); + } + + // ---- review round 1, finding 1: the saturating i64 boundary, via Lua ---- + + #[test] + fn set_rejects_the_saturating_i64_boundary_from_lua() { + // 2^63 is a float on BOTH backends (LuaJIT has no integer + // subtype; Lua 5.4's `2^63` is a float too). Before the `>=` + // fix this stored i64::MAX -- a silent off-by-one against the + // module's own "never silently change its value" contract. + let (lua, _reg) = fresh(); + run( + &lua, + r#"pmacs.config.define{ name="a.n", description="d", type="integer", default=0 }"#, + ) + .unwrap(); + let err = run(&lua, "pmacs.config.set('a.n', 2^63)").unwrap_err(); + assert!( + err.to_string().contains("integer") || err.to_string().contains("integral"), + "2^63 must be refused, not saturated: {err}" + ); + let still: i64 = eval(&lua, "return pmacs.config.get('a.n')").unwrap(); + assert_eq!(still, 0, "the refused set must not have stored anything"); + } + + #[test] + fn define_rejects_the_saturating_i64_boundary_in_a_bound() { + // `min`/`max` go through read_bound_i64 -> lua_exact_i64 -> + // int_from_f64, so the same boundary must be refused in a spec. + let (lua, _reg) = fresh(); + let err = run( + &lua, + r#"pmacs.config.define{ name="a.n", description="d", type="integer", default=0, max=2^63 }"#, + ) + .unwrap_err(); + assert!( + err.to_string().contains("integer") || err.to_string().contains("integral"), + "a 2^63 bound must be refused: {err}" + ); + } + // ---- acceptance 9: SourceLocation from the DEFINING module -------------- #[test] diff --git a/tests/config_registry_acceptance.rs b/tests/config_registry_acceptance.rs index b5e7a74..41749fa 100644 --- a/tests/config_registry_acceptance.rs +++ b/tests/config_registry_acceptance.rs @@ -224,6 +224,50 @@ fn auto_pair_defaults_on_so_the_migration_changed_no_default() { assert_eq!(active_text(&s), "()", "pairing is on by default"); } +// --------------------------------------------------------------------------- +// Item 13 — the purge runs on the REAL buffer-death path +// --------------------------------------------------------------------------- + +#[test] +fn killing_a_buffer_through_the_real_path_purges_its_locals() { + // Review round 1, finding 4. Every other test of the purge calls + // `ConfigRegistry::remove_buffer` directly, so deleting the three + // lines wired into `after_buffer_removed` would leave them all + // green. This drives `pmacs.buffer.remove`, which is the production + // route (`remove_buffer_and_fire` -> `after_buffer_removed`), and + // fails if that wiring is absent. + // + // The assertion reads through the DEAD handle on purpose: BufferIds + // are never reused (buffer_registry.rs), so a stale id cannot alias + // a later buffer, and `is_set` against it reports exactly whether + // the registry still holds that buffer's map. + let dir = fresh_state_dir(); + let s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("DEAD = pmacs.buffer.find_or_open({a:?})")); + exec( + &s, + "pmacs.config.set_local(DEAD, 'editing.auto-pair', false)", + ); + assert!( + eval::(&s, "return pmacs.config.is_set('editing.auto-pair', DEAD)"), + "precondition: the buffer-local override is stored" + ); + + // Switch away first so killing the buffer cannot leave the window + // pointing at a dead buffer, then remove it through the real path. + exec(&s, "pmacs.buffer.remove(DEAD)"); + + assert!( + !eval::(&s, "return pmacs.config.is_set('editing.auto-pair', DEAD)"), + "the buffer's locals must be purged when it is removed" + ); + assert!( + eval::(&s, "return pmacs.config.get('editing.auto-pair', DEAD)"), + "and resolution falls back to the global default" + ); +} + // --------------------------------------------------------------------------- // Items 27 / 28 — the migration wrappers keep their legacy coercion (F4) // --------------------------------------------------------------------------- @@ -246,6 +290,56 @@ fn trim_on_save_wrapper_and_registry_are_interchangeable_both_ways() { ); } +#[test] +fn trim_on_save_honors_a_buffer_local_override() { + // Review round 1, finding 2. The save hook resolves against the + // buffer being saved, so `set_local` is a real per-buffer switch + // rather than a stored value nothing ever reads. + let dir = fresh_state_dir(); + let s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("BUF = pmacs.buffer.find_or_open({a:?})")); + // The content must be INSERTED, not merely present on disk: + // `save()` no-ops on an unmodified buffer, so a freshly-opened + // buffer would leave the file byte-identical and this test would + // pass without the save hook ever running. + exec(&s, r#"BUF:insert(0, "keep me \n")"#); + + // Globally on, but off for this buffer: trailing space survives. + exec(&s, "pmacs.config.set('editing.trim-on-save', true)"); + exec( + &s, + "pmacs.config.set_local(BUF, 'editing.trim-on-save', false)", + ); + exec(&s, "pmacs.command.invoke('buffer.save')"); + assert_eq!( + std::fs::read_to_string(&a).unwrap(), + "keep me \n", + "a buffer-local false must suppress trimming for this buffer" + ); +} + +#[test] +fn trim_on_save_still_falls_back_to_the_global_value() { + // The other half of finding 2's fix, and its regression guard: + // now that the hook passes a buffer, a broken fallback would make + // the global setting silently stop working. A separate editor and + // file because `save()` no-ops on an unmodified buffer, so the two + // cases cannot share one save cycle. + let dir = fresh_state_dir(); + let s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("BUF = pmacs.buffer.find_or_open({a:?})")); + exec(&s, r#"BUF:insert(0, "trim me \n")"#); + exec(&s, "pmacs.config.set('editing.trim-on-save', true)"); + exec(&s, "pmacs.command.invoke('buffer.save')"); + assert_eq!( + std::fs::read_to_string(&a).unwrap(), + "trim me\n", + "with no buffer-local override the global setting must still apply" + ); +} + #[test] fn trim_on_save_keeps_its_lenient_truthiness() { // F4: the registry is strict (a real boolean or nothing), but this