[Audit] Scheme runtime, modules, hooks — 5 verified findings (1 medium, 4 low)

[cuttlefisch]

Batched findings for Scheme runtime, modules, hooks from the pre-v0.15 codebase audit (branch audit/pre-v015-review).

Each was produced by an end-to-end capability trace, then independently re-verified by a reviewer whose
brief was to refute it. Only findings that survived refutation appear here, at the verifier's corrected
severity — across the audit, 102 claims became 89 confirmed and 29 were downgraded.

5 findings — 1 medium, 4 low. Tick them off individually; split any one
out into its own issue if it needs real design work.

---

1. R7RS slice/allocation primitives panic the whole editor on out-of-range or negative indices

bug · medium

Impact. (vector-copy #(1 2 3) 0 99), (string-copy "abc" 0 99), (vector->list v 2 1) or (make-vector -1) panic inside the primitive and unwind out of the editor's main loop, terminating the process and losing unsaved buffers. Every entry point reaches it identically: :eval, SPC e l, a line in init.scm or a module autoloads.scm (so the editor fails to start), a hook body drained by drain_hook_evals, and the eval_scheme MCP tool — meaning an AI peer typo crashes the human's editor. Negative arguments are worse than a clean panic: -1i64 as usize is usize::MAX, so make-vector/make-string request a usize::MAX-element allocation (capacity-overflow abort), and vector-copy!'s at + i at vector.rs:198 overflows — a debug build panics, a release build wraps to index 0 and silently writes to the wrong element.

Evidence

crates/scheme/src/stdlib/vector.rs:89-99 (vector->list):

let start = if args.len() > 1 { args[1].as_int()? as usize } else { 0 };
let end = if args.len() > 2 { args[2].as_int()? as usize } else { vec.len() };
Ok(Value::list(vec[start..end].to_vec()))

Same unchecked slice in vector.rs:139-149 (vector-copy), vector.rs:216-226 (vector->string), vector.rs:237-248 (string->vector), crates/scheme/src/stdlib/string.rs:211-221 (string->list), string.rs:243-253 (string-copy). Unchecked allocation from a signed length in vector.rs:21-27 (make-vector: let k = args[0].as_int()? as usize; ... vec![fill; k]), vector.rs:260-266 (make-bytevector), string.rs:40-46 (make-string: std::iter::repeat_n(c, k).collect()).

That this is a defect and not a policy is proved by the siblings that DO check, in the same files: string.rs:99-101 if start > end || end > chars.len() { return Err(LispError::user("substring: index out of range", vec![])); }, vector.rs:56-58 (vector-ref), vector.rs:72-73 (vector-set!), string.rs:78-81 (string-ref).

A third inconsistent behaviour for the same error class, vector.rs:196-201 (vector-copy!), silently succeeds instead: for (i, j) in (start..end).enumerate() { if at + i < to_vec.len() && j < from.len() { to_vec[at + i] = from[j].clone(); } }.

No catch_unwind exists between the event loop and native primitives (grep across crates/scheme/src and crates/mae/src finds one unrelated comment at daemon_supervisor.rs:467).

Verification

Verified verbatim: unchecked slices at vector.rs:89-99,139-149,216-226,237-248 and string.rs:211-221,243-253; unchecked allocation from signed length at vector.rs:21-27,260-266 and string.rs:40-46. Inconsistent with siblings that DO check (vector-ref :56-58, string-ref :78-81, substring :99-101) and with vector-copy! (:196-201) which silently no-ops - a third behaviour. Reachable from both human :eval and AI eval_scheme; catch_tool_panic (tool_dispatch.rs:621) does NOT cover the deferred drain at ai_event_handler.rs:1030.

Scope correction from verification: Downgraded high->medium: robustness/QoI defect, not security or silent corruption. Requires human or AI to write out-of-range Scheme; no ordinary-use path triggers it.

---

2. Module command/keybinding/option/hook inventories are never populated — every introspection surface reports empty

bug · low

Impact. :describe-module org prints Commands (0): (none) and Options (0) for every module in the tree; the list_modules MCP tool and audit_configuration return the same empty arrays to the AI peer. The advertised way to discover what a module gives you is dead on all surfaces, so both actors must read autoloads.scm by hand. The tests that would have caught it construct the data themselves rather than exercising the load path — crates/ai/src/tool_impls/editor_tools.rs:1356-1372 hand-writes commands: vec!["dashboard".into()] into a ModuleInfo and then asserts !m["commands"].as_array().unwrap().is_empty(), a pure confirmation test (principle #14).

Evidence

crates/mae/src/pkg/loader.rs:21-28 declares the fields with docs promising real content:

/// Commands registered by this module's autoloads.
pub commands: Vec<String>,
/// Keybindings registered by this module's autoloads.
pub keybindings: Vec<(String, String, String)>,
/// Options registered by this module.
pub options: Vec<String>,
/// Hooks registered by this module.
pub hooks: Vec<(String, String)>,

register_resolved (loader.rs:73-76) initialises all four to Vec::new(). Nothing ever pushes into them: grepping \.commands\.push|\.keybindings\.push|\.hooks\.push|state\.options\.push across crates/mae/src/pkg/*.rs and bootstrap.rs returns nothing, and the only get_mut callers are mark_loaded/mark_failed (loader.rs:105-116), which touch only status.

The single production consumer copies the empty vectors straight through: crates/mae/src/bootstrap.rs:1514-1515 commands: m.commands.clone(), options: m.options.clone(), into mae_core::editor::ModuleInfo.

Read-back surfaces all render it: crates/core/src/editor/option_ops.rs:1945-1947 lines.push(format!("Commands ({}):", m.commands.len())); if m.commands.is_empty() { lines.push(" (none)".to_string()); }.

Verified live against the running MAE instance over MCP: list_modules returned all 16 loaded modules with "commands": [] and "options": [] — including file-tree, org, keymap-doom, which unquestionably register commands and options.

Verification

loader.rs:21-28 declares commands/keybindings/options/hooks; register_resolved :73-76 and register_skipped :95-99 init all to Vec::new(); nothing ever pushes. Confirmed live over MCP list_modules: all 16 modules return empty. Test editor_tools.rs:1354-1371 hand-constructs a populated ModuleInfo and asserts non-empty - a confirmation test that never exercises the load path (principle #14).

Scope correction from verification: Much narrower, hence low. Named examples are wrong: file-tree/org/keymap-doom register zero commands and zero options; only 3 of 27 modules define any command, and NO shipped module registers an option - so Options (0) is truthful everywhere and Commands (0) is wrong for 3 modules. Keybindings are NOT reported empty (:describe-module derives them from a live keymap lookup, option_ops.rs:1967-1985). hooks is never rendered anywhere. Accurate claim: four dead fields, under-reporting commands for 3 modules.

---

3. A module rejected by the mae_version check still loads its dependents, breaking the resolver's documented consistency invariant

bug · low

Impact. A third-party module foo that depends on bar, where bar declares mae_version = ">=99.0.0": the resolver's prune never sees the rejection, so bar is skipped and foo loads against an absent dependency — the exact state resolve_load_order was written to make impossible. foo's autoloads then reference commands/keymaps bar never registered, producing "scheme keybinding targets unknown keymap" warnings (state_sync_apply.rs:175) rather than one clear "skipped: dependency unavailable" message. The in-tree safety net at bootstrap.rs:1483-1495 only covers keymap-leader and the active flavor; nothing protects user-installed modules from ~/.local/share/mae/modules.

Evidence

crates/mae/src/pkg/resolver.rs:37-47 states the invariant the whole graceful-degradation design rests on: "Skipping a module also skips everything that (transitively) depends on it, so the surviving set is always internally consistent and safe to load." It is enforced by the fixpoint prune at resolver.rs:73-103.

But the version gate runs after resolution, in the per-module load loop — crates/mae/src/bootstrap.rs:1413-1421:

let current_version = env!("CARGO_PKG_VERSION");
for module in &resolved {
    // F2: Enforce version constraints at load time
    if let Err(e) = module.manifest.check_mae_version(current_version) {
        registry.mark_failed(&module.name, e.clone());
        ...
        continue;
    }

continue drops exactly one module. Because resolved is in dependency-first order, every dependent of the rejected module is still ahead in the loop and loads normally.

Verification

Code confirmed verbatim. crates/mae/src/pkg/resolver.rs:45-47 states "Skipping a module also skips everything that (transitively) depends on it, so the surviving set is always internally consistent and safe to load", enforced by the fixpoint prune at :69-101 — which only ever considers missing/disabled deps, never version constraints. crates/mae/src/bootstrap.rs:1413-1421 then runs check_mae_version per module inside the post-resolution load loop and continues past exactly one module, leaving its dependents (which sort later in dependency-first order) to load anyway. The safety net at :1483-1495 covers only keymap-leader and keymap-{flavor}, as claimed.

Severity is inflated. This is architecturally the same shape as the deliberate error-isolation policy immediately below it (bootstrap.rs:1466 "// Continue loading other modules — error isolation"): a module whose autoloads fail at eval time also leaves its dependents loaded. The failure is loud, not silent — registry.mark_failed, an error! log, a message-log entry and an aggregate "N module(s) did not load (see :messages)" status — and it requires the narrow case of a third-party dep declaring an unsatisfiable mae_version.

Scope correction from verification: Narrower accurate claim: check_mae_version runs after resolve_load_order instead of feeding its prune, so a version-rejected module's dependents still load and produce downstream 'unknown keymap' warnings rather than a clean 'skipped: dependency unavailable'. Real but narrow, loudly reported, and consistent with the surrounding deliberate error-isolation policy — the fix is to move the version gate into the resolver's prune input.

---

4. Hook system violates peer parity in both directions: Scheme cannot fire a hook, MCP cannot register one, neither can list

parity-gap · low

Impact. Principle #3 says the AI and the human call the same functions. Here they call disjoint halves of one subsystem: a Scheme script or init.scm can register after-save handlers but cannot trigger one to test it, while the AI peer can fire any hook but cannot install one — so an AI asked to "add a format-on-save hook" must fall back on writing raw text into init.scm. And because nothing enumerates HookRegistry, neither actor can answer "what is currently registered on before-save?" from any surface.

Evidence

Scheme's entire hook surface is registration-only — crates/scheme/src/runtime/keybindings.rs:150-174:

vm.register_fn("add-hook!", "Register a hook callback", Arity::Fixed(2), ...)
vm.register_fn("remove-hook!", "Remove a hook callback", Arity::Fixed(2), ...)

The AI's is firing-only — crates/ai/src/tools/core_tools.rs:308-320:

ToolDefBuilder::new("trigger_hook", "Manually fire a lifecycle hook by name. This triggers all Scheme functions registered for that hook point.")

implemented at crates/ai/src/tool_impls/editor_tools.rs:472 (editor.fire_hook(hook_name)). There is no add_hook/remove_hook/list_hooks MCP tool, and no (run-hooks …)/(fire-hook …)/(list-hooks) Scheme primitive.

The command surface has neither: grep -n "hook" crates/core/src/commands.rs yields one hit, and it is the doc string of an unrelated command (commands.rs:1355).

The enumeration API exists but is dead: crates/core/src/hooks.rs:114-120 pub fn list(&self) -> Vec<(&str, &[String])> has no caller outside hooks.rs's own tests.

The advice half of the same subsystem is the mirror image: HookRegistry::{add_advice, remove_advice, get_advice} (hooks.rs:155-184) is reachable from Scheme but has no MCP tool at all.

Verification

Half of the headline is wrong. "MCP cannot register one" ignores eval_scheme, which is a registered MCP tool (crates/ai/src/tools/core_tools.rs:497, dispatched at crates/ai/src/executor/core_exec.rs:142) and whose output is drained through drain_pending_scheme_evals in BOTH the embedded and MCP handlers (ai_event_handler.rs:203, :1029). (add-hook! ...) pushes to pending_hook_adds (keybindings.rs:157) which state_sync_apply.rs:249-250 applies via editor.hooks.add(...) — so an AI asked to "add a format-on-save hook" calls eval_scheme "(add-hook! \"after-save\" \"fmt\")" and it works. That IS principle #3's mechanism ("the AI agent calls the same Scheme functions"); demanding a dedicated MCP tool for each of the 204 registered Scheme primitives is not what #3 says. Likewise "Scheme cannot fire a hook" is narrower than stated: Scheme has (run-command NAME) (crates/scheme/src/runtime/editor_ops.rs:66) and (execute-ex ...), which fire real hooks — tests/editor/test_keymap_hooks_e2e.scm proves it end-to-end.

What survives: (a) editor.fire_hook(name) by arbitrary name is genuinely AI-only — trigger_hook (core_tools.rs:308-320 -> editor_tools.rs:472) has no command, no Scheme primitive and no keybinding counterpart; (b) HookRegistry::list() (hooks.rs:113-120) really has no caller outside its own test at hooks.rs:249, so no surface can answer "what is registered on before-save?" (only the inverse, hooks_containing, via help_ops.rs:698); (c) add_advice/remove_advice/get_advice have no MCP tool, though again eval_scheme reaches them.

Scope correction from verification: Narrower accurate claim: the only genuine asymmetry runs the other way — trigger_hook lets the AI fire any hook by name with no human/Scheme equivalent — and HookRegistry::list() is dead, so neither actor can enumerate what is registered on a given hook. The 'MCP cannot register a hook' half is refuted by the eval_scheme tool, which is the sanctioned parity mechanism.

---

5. Hook end-to-end tests are vacuous, and the stale comment that justifies them is factually wrong

test-gap · low

Impact. The only hook whose firing is genuinely covered end-to-end is the leader/flavor family. Every core lifecycle hook — before-save, after-save, buffer-open, command-pre/command-post, mode-change, window-split, after-kb-change — has zero test proving a registered Scheme function actually executes. That is precisely the blind spot that let finding #4 (hooks listed but never fired) ship: a real firing test on option-change would have failed the moment the base name stopped being fired. CLAUDE.md's testing section names this anti-pattern directly ("Real event loops for event-loop behavior").

Evidence

tests/editor/test_hooks_firing.scm:3-6 justifies testing nothing:

;;; Note: In the headless test runner, pending_hook_evals are queued by
;;; fire_hook but never flushed (flush_pending_hooks runs in the event loop,
;;; not the test runner). So we test registration/removal mechanics and
;;; verify that the hook-triggering commands exist and execute without error.

That claim is false for the current runner — crates/mae/src/test_runner.rs:521 calls crate::key_handling::drain_hook_evals(editor, scheme); on every tick (and again in the AwaitHook loop at :546). It is disproved in-tree by tests/editor/test_keymap_hooks_e2e.scm, which registers real handlers, drives (feed-keys "SPC t l") and asserts (should %leader-open-fired).

The consequence: test_hooks_firing.scm:15-24 asserts (should #t) after add-hook! and after remove-hook! — assertions that pass regardless of behaviour. tests/editor/test_hooks.scm contains no should form at all, and its first case registers "after-mode-change" (line 9), a hook name that does not exist anywhere in WELL_KNOWN_HOOKS or in any fire_hook call site — yet it passes.

The Rust-side tests stop at the registry: crates/scheme/src/runtime_tests.rs:919-955 asserts only editor.hooks.get("before-save") == ["my-save-fn"], never that the function runs.

Verification

The test-quality observation is right, the accusation against the comment is not. tests/editor/test_hooks_firing.scm:15-24 really does (add-hook! ...) then (should #t), and tests/editor/test_hooks.scm contains no should form at all and registers "after-mode-change", a name absent from WELL_KNOWN_HOOKS and from every fire_hook call site (verified). crates/scheme/src/runtime_tests.rs hook tests stop at the registry.

But the claim "crates/mae/src/test_runner.rs:521 calls drain_hook_evals on every tick" is wrong: :521 sits inside the YieldRequest::Tick arm, i.e. it runs only when the Scheme code explicitly yields. The complete set of drain sites in test_runner.rs is :521 (yield-tick), :546 (await-hook), :680 (module reload) and :712 (feed-keys) — none of which fires after a plain (run-command ...) step. So the file's justifying comment is substantially accurate for an ordinary test step; what it misses is that feed-keys/execute-ex/yield-tick provide a path, which tests/editor/test_keymap_hooks_e2e.scm exploits to assert real firing.

Scope correction from verification: Narrower accurate claim: two hook test files are assertion-free or assert (should #t), and no test proves a registered Scheme function actually runs for any core lifecycle hook (before-save/after-save/buffer-open/command-pre/post/mode-change) — even though test_keymap_hooks_e2e.scm shows the feed-keys/execute-ex path makes such a test feasible. The claim that the justifying comment is factually wrong is itself wrong: drain_hook_evals runs only on yield-tick/await-hook/reload/feed-keys, not every runner tick.

[cuttlefisch]

Part of the pre-v0.15 codebase audit — tracking epic #592.