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) <noreply@anthropic.com>

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)

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.
 
 ---
 

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:

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<Vec<String>> {
     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::<Value>(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<String> = 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]

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::<bool>(&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::<bool>(&s, "return pmacs.config.is_set('editing.auto-pair', DEAD)"),
+        "the buffer's locals must be purged when it is removed"
+    );
+    assert!(
+        eval::<bool>(&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