From 37fac4324a7b53bffac23d7fda42d91d904248fa Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Mon, 13 Jul 2026 16:30:41 +0100 Subject: [PATCH] =?UTF-8?q?fix(compile):=20PR=20#113=20round=202=20?= =?UTF-8?q?=E2=80=94=20rule=20snapshots,=20finite=20indexes,=20shell=20iso?= =?UTF-8?q?lation,=20parser=20reset?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding-by-finding (framing revision 8; bites via scripts/bite): 1. Rule validation is a stable, total snapshot: validated scalar fields are copied into per-run plain tables via raw reads (rawget; metatable-provided fields deliberately not honored), so post-run mutation of the user's rule objects cannot alter an in-flight run and a hostile __index is a counted skip, not an error thrown through the pump mid-batch. The container traversal is itself pcall-protected; traversal-raise semantics are Lua-flavor-dependent (5.2+ ipairs consults __index, LuaJIT reads raw) and the test pins both flavors. 2. Capture indexes must be FINITE (floor(math.huge) == math.huge, so integrality alone passed it); math.huge is now a counted malformed entry. 3. Shell-command never touches the rule table: no spurious compile-rule warnings on M-!, and no rule-container state can block a run that performs no parsing. 4. AnsiParser::finish() (and parser:finish()) now fully resets the parser — in-flight CSI/OSC/escape state and alt-screen suppression included — so a post-finish feed parses a fresh stream. Three direct unit tests in ansi.rs plus a Lua-driven twin in the acceptance suite (the twin exists because a scripts/bite file swap replaces the in-file units along with the fix). 5. Comment corrections: fractional capture indexes read a distinct absent key (not a neighboring capture); the group-coercion comment describes truthiness, not false; the AnsiParserLua rustdoc lists finish(). Bites: r2f1 (both shapes), r2f2, r2f3 fail against pre-fix compile.lua; r2f4 fails against pre-fix ansi.rs. Gates: fmt, clippy workspace all-targets, lib 1525, crdt lib 1699, compile acceptance 50, crdt acceptance 1, m4 101, GPU 59, workspace sweep 2501/0 (one flaky-suite rerun per the standing m8 rule), git diff --check. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VoiEyuPjoBhvwACf8HAnLB --- builtin/runtime/compile.lua | 90 +++++++++----- docs/compile-mode-framing.md | 46 +++++-- src/ansi.rs | 65 +++++++++- src/lua_bindings/mod.rs | 20 +-- tests/compile_mode_acceptance.rs | 202 +++++++++++++++++++++++++++++++ 5 files changed, 374 insertions(+), 49 deletions(-) diff --git a/builtin/runtime/compile.lua b/builtin/runtime/compile.lua index 07f4565..19bb779 100644 --- a/builtin/runtime/compile.lua +++ b/builtin/runtime/compile.lua @@ -91,46 +91,59 @@ pmacs.compile.rules = { { pattern = "([^%s:][^:]*):(%d+):", file = 1, line = 2 }, } --- A capture index must be a positive INTEGER (a fractional index --- silently reads a neighbouring capture via Lua table coercion — --- round-1 finding 4). +-- A capture index must be a positive, FINITE integer. Fractional +-- indexes read a distinct (absent) table key, not a capture; +-- math.floor(math.huge) == math.huge, so integrality alone does not +-- imply finiteness (round-1 finding 4; round-2 finding 2). local function is_capture_index(v) - return type(v) == "number" and v >= 1 and v == math.floor(v) + return type(v) == "number" and v >= 1 and v < math.huge and v == math.floor(v) end -local function rule_is_valid(rule) - if type(rule) ~= "table" then return false end - if type(rule.pattern) ~= "string" then return false end +-- Validate one rule via RAW reads (rawget): a metatable-backed entry +-- whose __index raises must be a skipped malformed entry, not an +-- error thrown through the per-frame pump mid-batch (round-2 +-- finding 1). Fail-closed posture: metatable-provided fields are +-- deliberately not honored. Returns a plain-table copy of the +-- validated scalar fields, or nil — the copy is the run's snapshot, +-- immune to post-validation mutation of the user's rule object. +local function validated_rule_copy(rule) + if type(rule) ~= "table" then return nil end + local pattern = rawget(rule, "pattern") + if type(pattern) ~= "string" then return nil end -- Probe the pattern against the empty string so a malformed Lua -- pattern is caught (and counted in the status note) here at -- validation time, not silently at match time. - if not pcall(string.match, "", rule.pattern) then return false end - if not is_capture_index(rule.file) then return false end - if not is_capture_index(rule.line) then return false end - if rule.col ~= nil and not is_capture_index(rule.col) then return false end - if rule.severity ~= nil and rule.severity ~= "error" and rule.severity ~= "warning" then - return false + if not pcall(string.match, "", pattern) then return nil end + local file = rawget(rule, "file") + local line = rawget(rule, "line") + local col = rawget(rule, "col") + local severity = rawget(rule, "severity") + if not is_capture_index(file) then return nil end + if not is_capture_index(line) then return nil end + if col ~= nil and not is_capture_index(col) then return nil end + if severity ~= nil and severity ~= "error" and severity ~= "warning" then + return nil end - return true + return { pattern = pattern, file = file, line = line, col = col, severity = severity } end --- Validate the (user-mutable) rule table once per run, fail-closed --- per entry (Q#CM4): a non-table container degrades to the built-in --- defaults; malformed entries are skipped; one status note per run --- counts the skips. Never raises — this feeds the per-frame pump. --- -- The defaults are a private deep copy taken at load time: an alias -- of the public table would keep in-place user mutations live after -- the "using built-in defaults" degradation (round-1 finding 10). local BUILTIN_RULES = {} for i, rule in ipairs(pmacs.compile.rules) do - local copy = {} - for k, v in pairs(rule) do - copy[k] = v - end - BUILTIN_RULES[i] = copy + BUILTIN_RULES[i] = validated_rule_copy(rule) end +-- Validate the (user-mutable) rule table once per run, fail-closed +-- per entry (Q#CM4): a non-table container degrades to the built-in +-- defaults; malformed entries are skipped; one status note per run +-- counts the skips. Never raises — this feeds the per-frame pump — +-- so the container traversal itself is protected too (a hostile +-- __index on the OUTER table can raise from inside ipairs; round-2 +-- finding 1). The returned list holds per-run plain-table copies: +-- validation is a stable, total snapshot, and mutating the user's +-- rule objects after compile.run() cannot alter an in-flight run. local function validated_rules() local rules = pmacs.compile.rules if type(rules) ~= "table" then @@ -138,12 +151,20 @@ local function validated_rules() return BUILTIN_RULES, 0 end local valid, skipped = {}, 0 - for _, rule in ipairs(rules) do - if rule_is_valid(rule) then - valid[#valid + 1] = rule - else - skipped = skipped + 1 + local ok = pcall(function() + for _, rule in ipairs(rules) do + local copy = validated_rule_copy(rule) + if copy then + valid[#valid + 1] = copy + else + skipped = skipped + 1 + end end + end) + if not ok then + pmacs.editor.set_status( + "compile: pmacs.compile.rules raised during traversal; using built-in defaults") + return BUILTIN_RULES, 0 end return valid, skipped end @@ -638,8 +659,15 @@ local function start_run(slot, cmdline, opts) pmacs.editor.set_status(slot.label .. ": superseded previous run") end - -- Fresh run state. - slot.rules, slot.skipped_rules = validated_rules() + -- Fresh run state. Only error-parsing slots touch the rule table + -- at all: shell-command performs no parsing, so it must neither + -- surface compile-rule warnings nor fail on a hostile rule + -- container (round-2 finding 3). + if slot.parse then + slot.rules, slot.skipped_rules = validated_rules() + else + slot.rules, slot.skipped_rules = {}, 0 + end slot.parse_errors = slot.parse slot.errors = {} slot.err_index = 0 diff --git a/docs/compile-mode-framing.md b/docs/compile-mode-framing.md index 8048656..776bb0f 100644 --- a/docs/compile-mode-framing.md +++ b/docs/compile-mode-framing.md @@ -1,7 +1,25 @@ # Compile-mode — framing (Arc 5 stage 1, terminal) -**Revision 7 — 2026-07-13. Status: implemented on branch -`compile-mode` (PR #113); revision 7 folds in PR round 1.** +**Revision 8 — 2026-07-13. Status: implemented on branch +`compile-mode` (PR #113); revisions 7–8 fold in PR rounds 1–2.** + +Revision 8 (PR #113 round 2, findings 1–5): rule validation is a +stable, total snapshot — validated scalar fields are copied into +per-run plain tables via raw reads (rawget; metatable-provided +fields are deliberately not honored), and the container traversal is +itself protected, so neither post-run mutation of the user's rule +objects nor a hostile `__index` can alter an in-flight run or raise +through the pump mid-batch (traversal-raise semantics are +Lua-flavor-dependent: 5.2+ `ipairs` consults `__index`, LuaJIT reads +raw — both pinned); capture indexes must also be FINITE +(`math.floor(math.huge) == math.huge`); shell-command never touches +the rule table (no spurious warnings, no rule-container failure can +block a run that parses nothing); `parser:finish()` now RESETS the +parser — in-flight CSI/OSC/escape state and alt-screen suppression +included — so a post-finish feed parses a fresh stream (direct unit +tests plus a Lua-driven twin); three stale comments corrected. +Bites: four acceptance tests against pre-fix compile.lua, one +against pre-fix ansi.rs. Revision 7 (PR #113 round 1, findings 1–10): stored coordinates must be finite integers and both cursor walks are movement-bounded @@ -397,13 +415,17 @@ No protocol change, no frontend change. same-length replaces, so undoing one changes content without changing length. Revision increments on every edit, undo, and redo. -5. **`AnsiParser::finish()` + `parser:finish()` (Revision 7)** — +5. **`AnsiParser::finish()` + `parser:finish()` (Revisions 7–8)** — stream-end finalization: the feed-boundary contract deliberately buffers an incomplete UTF-8 sequence for the next feed, but at process EOF there is no next feed — `finish()` emits U+FFFD for the pending prefix (the same posture an interrupting control byte - gets) and flushes the text run. Compile-mode calls it once at the - terminal event, before finalizing the pending line. + gets) and flushes the text run, **then fully resets the parser + (in-flight CSI/OSC/escape state and alt-screen suppression + included): a feed after finish parses a NEW stream** rather than + continuing a pre-EOF escape or staying suppressed. Compile-mode + calls it once at the terminal event, before finalizing the + pending line. ## Decisions @@ -608,9 +630,17 @@ invoke time, so commands/runtime load order stays irrelevant for it. mutations live after the degradation); malformed entries are skipped, invalid Lua patterns are caught (and counted) at validation time via a probe match, and match-time pattern calls - stay pcall'd as belt-and-braces. One status note per run counts - the skipped entries; a later valid rule still matches; the - per-frame pump never raises. + stay pcall'd as belt-and-braces. **Validation is a stable, total + snapshot (Revision 8):** validated scalar fields are copied into + per-run plain tables via raw reads — mutating the user's rule + objects after `compile.run()` cannot alter the in-flight run, a + hostile `__index` is a counted skip rather than an error through + the pump, and the container traversal itself is protected + (5.2+ `ipairs` consults `__index`; LuaJIT reads raw — both + degrade cleanly). Capture indexes must be positive, FINITE + integers. One status note per run counts the skipped entries; a + later valid rule still matches; the per-frame pump never raises. + Shell-command slots never touch the rule table at all. - Each match appends `{ file, line, col, severity, line_start_byte }` to the run's ordered error list (reset per run). Relative paths resolve against the run's cwd. diff --git a/src/ansi.rs b/src/ansi.rs index 5a1d2f9..f94e556 100644 --- a/src/ansi.rs +++ b/src/ansi.rs @@ -399,8 +399,14 @@ impl AnsiParser { /// next feed, so the pending prefix can never complete. Emit /// U+FFFD for it (the same posture `flush_text_run` takes when a /// control byte interrupts a sequence) and flush the resulting - /// text run. Idempotent once drained. First consumer: - /// compile-mode's terminal-event path (Q#CM4). + /// text run. + /// + /// The parser is then fully reset — in-flight CSI/OSC/escape + /// state AND alt-screen suppression included — so a `feed` after + /// `finish` parses a NEW stream from a clean slate rather than + /// continuing a pre-EOF escape sequence or staying suppressed + /// (PR #113 round-2 finding 4). Idempotent once drained. First + /// consumer: compile-mode's terminal-event path (Q#CM4). pub fn finish(&mut self) -> Vec { let mut events = Vec::new(); self.flush_pending_utf8_as_replacement(); @@ -410,6 +416,11 @@ impl AnsiParser { } else { self.text_run.clear(); } + self.reset(); + // `reset` deliberately preserves alt-screen suppression (a + // mid-stream reset must not unhide alt-screen contents); a + // stream END does end the suppression. + self.alt_screen_active = false; events } @@ -1780,4 +1791,54 @@ mod tests { let evs_text = p.feed(b"hi"); assert_eq!(collect_text(&evs_text), "hi"); } + + // ----------------------------------------------------------------- + // Stream-end finish() (compile-mode Q#CM4; PR #113 rounds 1–2) + // ----------------------------------------------------------------- + + #[test] + fn finish_flushes_truncated_utf8_as_replacement() { + let mut p = AnsiParser::new(); + // 0xC3 opens a two-byte sequence that never completes. + let evs = p.feed(b"abc\xC3"); + assert_eq!(collect_text(&evs), "abc", "prefix buffered across feeds"); + let evs = p.finish(); + assert_eq!( + collect_text(&evs), + "\u{FFFD}", + "stream end must surface the pending prefix as U+FFFD" + ); + // Idempotent once drained. + assert!(p.finish().is_empty(), "second finish drains nothing"); + } + + #[test] + fn feed_after_finish_starts_a_fresh_stream() { + // Mid-CSI at stream end: without the finish-time reset, a + // subsequent feed would keep consuming bytes as CSI + // parameters instead of parsing a new stream (PR #113 + // round-2 finding 4). + let mut p = AnsiParser::new(); + let _ = p.feed(b"\x1b[3"); // incomplete CSI + let _ = p.finish(); + let evs = p.feed(b"plain"); + assert_eq!( + collect_text(&evs), + "plain", + "post-finish feeds must not continue a pre-EOF escape" + ); + } + + #[test] + fn finish_ends_alt_screen_suppression() { + let mut p = AnsiParser::new(); + let _ = p.feed(b"\x1b[?1049hhidden"); // enter alt screen + let _ = p.finish(); + let evs = p.feed(b"visible"); + assert_eq!( + collect_text(&evs), + "visible", + "a stream END ends suppression; a new stream starts unsuppressed" + ); + } } diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index ae990af..084ff5e 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -2984,9 +2984,11 @@ fn attach_style_overlay_to_visible_windows( /// Lua-facing wrapper around [`crate::ansi::AnsiParser`]. /// -/// Constructed via `pmacs.ansi.parser()`; methods `feed(bytes)` and -/// `reset()` mirror the Rust API. `feed` returns an array of event -/// tables --- see [`event_to_lua_table`] for the schema. The wrapper +/// Constructed via `pmacs.ansi.parser()`; methods `feed(bytes)`, +/// `reset()`, and `finish()` mirror the Rust API. `feed` and +/// `finish` return an array of event tables --- see +/// [`event_to_lua_table`] for the schema; `finish` drains stream-end +/// state and resets the parser for a fresh stream. The wrapper /// is `RefCell`-internal so multiple Lua-side methods can borrow /// safely; the Lua VM is single-threaded so the borrow can never /// race. @@ -7001,11 +7003,13 @@ fn lua_to_spec(table: &Table) -> mlua::Result { // Compile-mode process shape (Q#CM3). Both options are // pipe-mode-only; the supervisor rejects them at spawn under PTY // so misconfiguration surfaces as a spawn error, not silence. - // Type errors are HARD errors, not silent defaults: `stdin = - // true` quietly becoming a piped stdin (hang) or `group = - // "true"` quietly becoming false (descendant leak) would undo - // exactly the guarantees these fields exist to carry (PR #113 - // round-1 finding 6). + // Type errors are HARD errors, not silent coercions: `stdin = + // true` would quietly keep a piped stdin (hang), and a mistyped + // `group` would coerce through Lua truthiness to whichever + // boolean its truthiness happens to be — either way the caller's + // intent is unverifiable, and these fields carry process-hygiene + // guarantees (PR #113 round-1 finding 6; wording corrected in + // round 2 finding 5). let stdin_raw: Option = table.get("stdin").map_err(|_| { mlua::Error::external("stdin must be the string \"piped\" or \"null\"".to_owned()) })?; diff --git a/tests/compile_mode_acceptance.rs b/tests/compile_mode_acceptance.rs index 7db88e8..44b2fa4 100644 --- a/tests/compile_mode_acceptance.rs +++ b/tests/compile_mode_acceptance.rs @@ -1702,6 +1702,208 @@ fn r1f9_truncated_utf8_at_eof_becomes_the_replacement_character() { ); } +// --------------------------------------------------------------------------- +// PR #113 round 2 — bite tests +// --------------------------------------------------------------------------- + +#[test] +fn r2f1_rule_validation_is_a_stable_snapshot() { + // Mutating the user's rule object AFTER compile.run() must not + // alter the in-flight run: validation copies scalar fields into + // per-run plain tables. + let dir = tempfile::tempdir().unwrap(); + let script = write_script(dir.path(), "slow.sh", "sleep 0.5\nprintf 'm.c:3: e\\n'\n"); + let mut s = editor(); + exec( + &s, + r#"pmacs.compile.rules = { { pattern = "(m%.c):(%d+):", file = 1, line = 2 } }"#, + ); + compile_run(&s, &format!("sh {script}"), dir.path()); + // The output hasn't arrived yet; sabotage the live rule object. + exec(&s, "pmacs.compile.rules[1].pattern = 'nevermatch'"); + assert!(pump_until(&mut s, 10_000, |s| compilation_text(s) + .contains("[compile exited"))); + assert_eq!( + compile_errors(&s), + vec![("m.c".to_owned(), 2, 0, None)], + "the run must parse with its validated snapshot, not the \ + mutated object" + ); +} + +#[test] +fn r2f1_metatable_backed_rules_cannot_raise_through_the_pump() { + // A rule whose field reads raise (hostile __index) and a + // container whose traversal raises: both must degrade cleanly — + // no error thrown through the per-frame pump, terminal cleanup + // intact. + let dir = tempfile::tempdir().unwrap(); + let mut s = editor(); + exec( + &s, + r#" + local hostile = setmetatable({}, { __index = function() error("boom") end }) + pmacs.compile.rules = { hostile, + { pattern = "(k%.c):(%d+):", file = 1, line = 2 } } + "#, + ); + compile_run(&s, "printf 'k.c:4: e\\n'", dir.path()); + assert!( + status(&s).contains("skipped 1 malformed"), + "the hostile entry is a counted skip; got: {}", + status(&s) + ); + assert!(pump_until(&mut s, 10_000, |s| compilation_text(s) + .contains("[compile exited with code 0]"))); + assert_eq!( + compile_errors(&s), + vec![("k.c".to_owned(), 3, 0, None)], + "the valid entry still parses" + ); + assert!( + errors_buffer(&s).is_empty(), + "nothing raised through the pump: {}", + errors_buffer(&s) + ); + assert!( + pump_until(&mut s, 3_000, |s| process_count(s) == 0), + "terminal cleanup ran" + ); + + // Hostile CONTAINER whose traversal raises. Flavor-dependent by + // Lua semantics: 5.2+ `ipairs` consults __index (the raise fires + // and the pcall degrades to defaults with a note); LuaJIT/5.1 + // reads raw (the container is simply empty — no rules, no note). + // Both flavors must complete cleanly with nothing thrown + // through the pump. + let mut s = editor(); + exec( + &s, + r#" + pmacs.compile.rules = setmetatable({}, { + __index = function() error("container boom") end, + }) + "#, + ); + compile_run(&s, "printf 'a.c:1:1: error: e\\n'", dir.path()); + let is_lua54: bool = eval(&s, "return _VERSION ~= 'Lua 5.1'"); + if is_lua54 { + assert!( + status(&s).contains("raised during traversal"), + "degradation note under 5.2+ ipairs semantics; got: {}", + status(&s) + ); + } + assert!(pump_until(&mut s, 10_000, |s| compilation_text(s) + .contains("[compile exited"))); + if is_lua54 { + assert_eq!( + compile_errors(&s).len(), + 1, + "built-in defaults still parse after container degradation" + ); + } else { + assert!( + compile_errors(&s).is_empty(), + "under raw-ipairs flavors the hostile container reads as \ + an (empty) rule table" + ); + } + assert!( + errors_buffer(&s).is_empty(), + "no spam: {}", + errors_buffer(&s) + ); +} + +#[test] +fn r2f2_infinite_capture_index_is_counted_malformed() { + let dir = tempfile::tempdir().unwrap(); + let mut s = editor(); + exec( + &s, + r#" + pmacs.compile.rules = { + { pattern = "(z%.c):(%d+):", file = 1, line = 2, col = math.huge }, + } + "#, + ); + compile_run(&s, "printf 'z.c:2: e\\n'", dir.path()); + assert!( + status(&s).contains("skipped 1 malformed"), + "math.huge is not a capture index (floor(huge) == huge, so \ + integrality alone passes it); got: {}", + status(&s) + ); + assert!(pump_until(&mut s, 10_000, |s| compilation_text(s) + .contains("[compile exited"))); +} + +#[test] +fn r2f3_shell_command_ignores_the_compile_rule_table() { + let dir = tempfile::tempdir().unwrap(); + let mut s = editor(); + // Both degradation shapes at once: a non-table container would + // warn, a raising container would abort — shell-command performs + // no parsing and must see neither. + exec(&s, "pmacs.compile.rules = 42"); + exec( + &s, + &format!( + "pmacs.shell.command('echo shellok', {{ cwd = {:?} }})", + dir.path().display().to_string() + ), + ); + assert!( + !status(&s).contains("not a table"), + "no compile-rule warning on a shell run; got: {}", + status(&s) + ); + assert!( + pump_until(&mut s, 10_000, |s| named_text(s, "*shell-command*") + .contains("[shell exited with code 0]")), + "shell-command runs regardless of rule-table state" + ); +} + +#[test] +fn r2f4_parser_finish_resets_for_a_fresh_stream() { + // Lua-driven twin of the ansi.rs units (which live inside the + // file a scripts/bite swap replaces): after finish(), a feed + // must parse a NEW stream — not continue a pre-EOF escape, not + // stay alt-screen-suppressed. + let s = editor(); + let (after_csi, after_alt): (String, String) = eval( + &s, + r#" + local function text_of(evs) + local out = {} + for _, ev in ipairs(evs) do + if ev.kind == "text" then out[#out + 1] = ev.text end + end + return table.concat(out) + end + local p = pmacs.ansi.parser() + p:feed("\27[3") -- incomplete CSI at stream end + p:finish() + local a = text_of(p:feed("plain")) + local q = pmacs.ansi.parser() + q:feed("\27[?1049hhidden") -- alt screen active at stream end + q:finish() + local b = text_of(q:feed("visible")) + return a, b + "#, + ); + assert_eq!( + after_csi, "plain", + "post-finish feed must not continue the pre-EOF CSI" + ); + assert_eq!( + after_alt, "visible", + "stream end must end alt-screen suppression" + ); +} + #[test] fn r1f10_builtin_default_rules_survive_in_place_mutation() { let dir = tempfile::tempdir().unwrap();