From a37d343e2c6108576bb7b68852841522facdb771 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 20:59:01 +0000 Subject: [PATCH 01/10] loco: four real defects from the #303 review, each with its falsifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four were reported by review bots on #303 after CI went green and after I merged. I merged on three green checks without waiting for the reviewers; the sequencing was mine and the findings are all real. None was a false positive. **1. The engine chose what to execute with a CODEGEN predicate.** `Vocabulary::branches` is `body_refs(f) > 0`, and its own doc says what it answers: "does lowering this call require emitting another function? — the only question a cast asks." `BREAK`, `CONTINUE`, `STOP`, `RETURN` and `WAIT` reference no body, so under it they are not control flow, and every one was handed to `Dialect::call`. A permissive dialect could accept `BREAK`, invent a meaning, and let the run report success — while this module's contract promised `UnhandledControlFlow`. Two questions, one name, and the vocabulary had already written the warning. Fixed with a predicate that answers the EXECUTION question: the shared core's control band, `0x01..=0x1F`. It is a property of the core's own layout, needs no per-byte list to drift, and a domain vocabulary cannot forge it. Worth naming: `an_unproven_control_flow_call_is_refused_not_approximated` could not see this. It uses `FOR_EACH`, which HAS a body reference, so it took the branching path and reached the refusal. The fixture's SHAPE was the coverage gap, not its content. **2. `REPEAT` clamped instead of refusing.** `min(count, cap)` ran the capped prefix and returned `Ok`, so a 200_000-iteration repeat reported SUCCESS having run 100_000 — a wrong answer presented as a right one, and inconsistent with `WHILE`, which reports the cap in the same situation. **3. A cycle of bodies recursed until the native stack was gone.** `references_are_resolvable` rejects only target 0 and out-of-range targets; it does NOT reject cycles, so two mutually-branching bodies pass validation. Unbounded that is a stack overflow, which ABORTS THE PROCESS — the one failure an orchestration engine must never turn a bad program into, and one no `RunError` can report because there is no stack left to return on. Now a depth ceiling and its own error variant, kept distinct from `IterationCap` because a long loop and a cycle are different defects. **4. The iteration cap was charged after the last body, not before the next.** A loop terminating in exactly `cap` iterations — inside the advertised ceiling — was rejected, because the cap fired before the condition could be re-tested one final time. At `cap = 0` the same ordering ran the body once before refusing, which is a cap of zero that executes. Also here, found while fixing 1: the arity lookup ran before the refusal, so `RETURN` reported `UncoveredArity` — pointing an author at the vocabulary when the true answer is that this engine does not execute the byte. The arity is needed only by the condition-span walk and now lives in those arms. Four falsifiers, each anti-vacuity-guarded: the body-less test asserts each byte really is body-less (or the row proves nothing), the repeat test asserts the body ran zero times (a version that ran the prefix then errored would pass the error assertion alone), the cycle test asserts the fixture really does pass the program's own validation, and the cap test carries the paired zero-cap half. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01X6y3drwKSE2zSgoexheLFX --- crates/ogar-loco/src/interpret.rs | 347 +++++++++++++++++++++++++++++- 1 file changed, 336 insertions(+), 11 deletions(-) diff --git a/crates/ogar-loco/src/interpret.rs b/crates/ogar-loco/src/interpret.rs index 57b6169..1017ca7 100644 --- a/crates/ogar-loco/src/interpret.rs +++ b/crates/ogar-loco/src/interpret.rs @@ -115,6 +115,18 @@ pub enum RunError { /// The cap it exceeded. cap: u32, }, + /// A branch recursed past [`Interpreter::recursion_depth`]. + /// + /// Distinct from [`RunError::IterationCap`] on purpose: a loop that runs + /// too long and a cycle of bodies calling each other are different + /// defects in the program, and collapsing them would send an author + /// looking at the wrong one. + RecursionDepth { + /// The branching call that would have exceeded the depth. + call: FnIndex, + /// The ceiling it hit. + depth: u32, + }, /// A control-flow call outside the five this engine executes. Refused /// rather than approximated — see the module doc. UnhandledControlFlow { @@ -123,6 +135,51 @@ pub enum RunError { }, } +/// The shared core's control band: `0x01..=0x1F`. +/// +/// Every byte in it is the ENGINE's to execute or refuse — never the +/// dialect's. Below it sits `NOP`; above it start the value families +/// (logic `0x20`, compare `0x30`, arithmetic `0x40`, variables `0x80`), and +/// past [`DOMAIN_FLOOR`] a dialect owns its own bytes outright. +const CONTROL_BAND: core::ops::RangeInclusive = 0x01..=0x1F; + +/// Is `f` the engine's to handle? +/// +/// # Why not `Vocabulary::branches` +/// +/// Because that answers a DIFFERENT QUESTION, and its own doc says so: +/// *"this predicate answers 'does lowering this call require emitting another +/// function?', which is the only question a cast asks"* — a CODEGEN question. +/// It is `body_refs(f) > 0`, and `BREAK` / `CONTINUE` / `STOP` / `RETURN` / +/// `WAIT` reference no body, so they are `false` under it. +/// +/// Used as an EXECUTION predicate it silently handed every one of them to +/// `Dialect::call`, where a dialect could invent a meaning for `BREAK` and +/// let the run report success — while this module's contract promised +/// [`RunError::UnhandledControlFlow`]. Two questions, one name, and the +/// vocabulary had already written the warning. +/// +/// The band is the right discriminator: it is a property of the shared core's +/// own layout, it needs no per-byte list to drift, and a domain vocabulary +/// cannot forge it (`DOMAIN_FLOOR` is `0x90`). +fn is_engine_control(f: FnIndex) -> bool { + CONTROL_BAND.contains(&f.0) +} + +/// How deep [`Interpreter::branch`] may recurse before refusing. +/// +/// Not a style limit — a safety one. `Program::references_are_resolvable` +/// rejects only target `0` and out-of-range targets; it does NOT reject +/// cycles, so two bodies that branch to each other pass validation and then +/// recurse until the native stack is gone. A stack overflow aborts the +/// process, which is the one failure an orchestration engine must never turn +/// a bad program into. +/// +/// 64 is chosen against the substrate rather than taste: a continuation stack +/// is 90 quad slots in one node, so a depth past that could not be reified +/// anyway. +pub const DEFAULT_RECURSION_DEPTH: u32 = 64; + /// A running program: the engine's own state, plus the dialect's. pub struct Interpreter<'a, V: Vocabulary, D: Dialect> { vocab: &'a CheckedVocabulary, @@ -130,6 +187,8 @@ pub struct Interpreter<'a, V: Vocabulary, D: Dialect> { dialect: D, stack: Vec, iteration_cap: u32, + recursion_depth: u32, + depth: u32, } impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { @@ -141,9 +200,22 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { dialect, stack: Vec::new(), iteration_cap: DEFAULT_ITERATION_CAP, + recursion_depth: DEFAULT_RECURSION_DEPTH, + depth: 0, } } + /// Replace the recursion ceiling. + pub fn with_recursion_depth(mut self, depth: u32) -> Self { + self.recursion_depth = depth; + self + } + + /// The depth a branch may not exceed. + pub fn recursion_depth(&self) -> u32 { + self.recursion_depth + } + /// Replace the per-loop iteration ceiling. pub fn with_iteration_cap(mut self, cap: u32) -> Self { self.iteration_cap = cap; @@ -178,7 +250,7 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { let mut pc = 0usize; while let Some(call) = body.call(pc) { let f = call.function; - if !self.vocab.table().branches(f) { + if !is_engine_control(f) { self.dialect .call(f, call.values, &mut self.stack) .map_err(RunError::Dialect)?; @@ -199,12 +271,11 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { call: Call, ) -> Result<(), RunError> { let f = call.function; - let arity = self - .vocab - .table() - .stack_arity(f) - .ok_or(RunError::UncoveredArity { call: f })?; - + // The arity is resolved INSIDE the loop arms, not here: it is needed + // only by the condition-span walk, and computing it up front made + // `RETURN` — whose arity the core deliberately leaves uncovered — + // report `UncoveredArity`, which points an author at the vocabulary + // when the true answer is that this ENGINE does not execute the byte. match f { FnIndex::IF => { let cond = self.pop(f)?; @@ -223,13 +294,29 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { } FnIndex::REPEAT => { let n = self.pop(f)?; - let count = self.dialect.repeat_count(&n).min(self.iteration_cap); + let count = self.dialect.repeat_count(&n); + // REFUSE, never clamp. `min` ran the capped prefix and + // returned `Ok`, so a 200_000-iteration repeat reported + // success having run 100_000 times — a wrong answer presented + // as a right one, and inconsistent with WHILE, which reports + // the cap in the same situation. + if count > self.iteration_cap { + return Err(RunError::IterationCap { + call: f, + cap: self.iteration_cap, + }); + } for _ in 0..count { self.branch(f, call.values[0])?; } } FnIndex::WHILE | FnIndex::REPEAT_UNTIL => { let until = f == FnIndex::REPEAT_UNTIL; + let arity = self + .vocab + .table() + .stack_arity(f) + .ok_or(RunError::UncoveredArity { call: f })?; // Where the condition's own calls begin — re-running THEM is // what makes the next test a new test. let cond_start = self.operand_span_start(body, pc, arity)?; @@ -240,14 +327,19 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { if truthy == until { break; } - self.branch(f, call.values[0])?; - iters += 1; + // The cap is checked when ANOTHER iteration is requested, + // never after the last one ran. Checked afterwards it + // rejected a loop that terminates in exactly `cap` + // iterations — inside the advertised ceiling — and at + // `cap = 0` it ran the body once before refusing. if iters >= self.iteration_cap { return Err(RunError::IterationCap { call: f, cap: self.iteration_cap, }); } + self.branch(f, call.values[0])?; + iters += 1; for i in cond_start..pc { let c = body.call(i).expect("in bounds: span already walked"); if self.vocab.table().branches(c.function) { @@ -270,7 +362,19 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { if idx == 0 || idx >= self.program.functions.len() { return Err(RunError::UnresolvedBody { call, target }); } - self.run_function(idx) + if self.depth >= self.recursion_depth { + return Err(RunError::RecursionDepth { + call, + depth: self.recursion_depth, + }); + } + self.depth += 1; + let r = self.run_function(idx); + // Restored on the error path too: an interpreter a caller inspects + // after a failure would otherwise report a depth that never unwound, + // and one reused after a caught error would refuse legal programs. + self.depth -= 1; + r } fn pop(&mut self, call: FnIndex) -> Result> { @@ -869,4 +973,225 @@ mod tests { interp.dialect.counter_reads ); } + + /// Records every call the dialect is handed, and succeeds on all of them. + /// + /// The point is the recording: a refusal the engine owes is only proven + /// if the byte never reached a dialect that would have accepted it. + #[derive(Default)] + struct Permissive { + seen: Vec, + } + impl Dialect for Permissive { + type Value = i64; + type Error = (); + fn truthy(&self, v: &i64) -> bool { + *v != 0 + } + fn repeat_count(&self, v: &i64) -> u32 { + u32::try_from(*v).unwrap_or(0) + } + fn call(&mut self, f: FnIndex, values: [u8; 3], stack: &mut Vec) -> Result<(), ()> { + self.seen.push(f.0); + if f == FnIndex::NUMBER { + stack.push(i64::from(values[0])); + } + Ok(()) + } + } + + /// FAILS IF: the engine decides what to execute with `Vocabulary::branches`. + /// + /// That predicate is `body_refs(f) > 0` and answers a CODEGEN question — + /// its own doc says so. `BREAK`, `CONTINUE`, `STOP`, `RETURN` and `WAIT` + /// reference no body, so under it they are not control flow, and every one + /// was handed to `Dialect::call`. A permissive dialect then accepts them + /// and the run reports success, while this module's contract promises + /// `UnhandledControlFlow`. + /// + /// The existing `an_unproven_control_flow_call_is_refused_not_approximated` + /// could not see this: it uses `FOR_EACH`, which HAS a body reference, so + /// it took the branching path and reached the refusal. The fixture's SHAPE + /// was the coverage gap, not its content. + #[test] + fn a_body_less_control_byte_is_refused_by_the_engine_not_offered_to_the_dialect() { + for byte in [ + FnIndex::BREAK, + FnIndex::CONTINUE, + FnIndex::STOP, + FnIndex::RETURN, + FnIndex::WAIT, + ] { + // Anti-vacuity: each really is body-less, so the old predicate + // really did call it a non-branch. Without this the test could + // pass by accidentally picking branching bytes. + let vocab = validate(CoreOnly).expect("conforms"); + assert_eq!( + vocab.table().body_refs(byte), + 0, + "{byte:?} must be body-less or this row proves nothing" + ); + + let entry = FunctionBody::from_calls( + LaneShape::Pairs, + &[Call::with_value(FnIndex::NUMBER, 1), Call::new(byte)], + ) + .unwrap(); + let p = Program { + functions: vec![entry], + }; + let mut interp = Interpreter::new(&vocab, &p, Permissive::default()); + assert_eq!( + interp.run(), + Err(RunError::UnhandledControlFlow { call: byte }), + "{byte:?} must be refused by the engine" + ); + assert!( + !interp.dialect.seen.contains(&byte.0), + "{byte:?} was offered to the dialect: {:?}", + interp.dialect.seen + ); + } + } + + /// FAILS IF: `REPEAT` clamps an over-cap count instead of refusing it. + /// + /// `min(count, cap)` ran the capped prefix and returned `Ok`, so a + /// 200_000-iteration repeat reported SUCCESS having run 100_000 times — a + /// wrong answer presented as a right one, and inconsistent with `WHILE`, + /// which reports the cap in exactly this situation. + #[test] + fn a_repeat_count_above_the_cap_is_refused_rather_than_truncated() { + let entry = FunctionBody::from_calls( + LaneShape::Pairs, + &[ + Call::with_value(FnIndex::NUMBER, 10), + Call::with_value(FnIndex::REPEAT, 1), + ], + ) + .unwrap(); + let body = + FunctionBody::from_calls(LaneShape::Pairs, &[Call::with_value(FnIndex::NUMBER, 1)]) + .unwrap(); + let p = Program { + functions: vec![entry, body], + }; + let vocab = validate(CoreOnly).expect("conforms"); + let mut interp = Interpreter::new(&vocab, &p, Permissive::default()).with_iteration_cap(4); + assert_eq!( + interp.run(), + Err(RunError::IterationCap { + call: FnIndex::REPEAT, + cap: 4 + }) + ); + // The second half, and the one the clamp would fail: refusing means + // running NOTHING. A version that ran the capped prefix and then + // errored would satisfy the assertion above and still be wrong. + assert!( + !interp.dialect.seen.contains(&FnIndex::NUMBER.0) || interp.dialect.seen.len() == 1, + "the body must not have run: {:?}", + interp.dialect.seen + ); + } + + /// FAILS IF: a cycle of bodies recurses until the native stack is gone. + /// + /// `Program::references_are_resolvable` rejects only target `0` and + /// out-of-range targets — it does NOT reject cycles, so two bodies that + /// branch to each other pass validation. Unbounded, that is a stack + /// overflow, which ABORTS THE PROCESS: the one failure an orchestration + /// engine must never turn a bad program into, and one no `RunError` can + /// report because there is no stack left to return on. + #[test] + fn two_bodies_that_branch_to_each_other_are_refused_not_overflowed() { + // 1 -> 2 -> 1 -> ... , each hop guarded by an always-true IF. + let hop = |target: u8| { + FunctionBody::from_calls( + LaneShape::Pairs, + &[ + Call::with_value(FnIndex::NUMBER, 1), + Call::with_value(FnIndex::IF, target), + ], + ) + .unwrap() + }; + let p = Program { + functions: vec![hop(1), hop(2), hop(1)], + }; + let vocab = validate(CoreOnly).expect("conforms"); + // Anti-vacuity: the cycle really does pass the program's own check, + // so this is a defect in the ENGINE and not something validation + // was already catching. + assert!( + p.references_are_resolvable(&vocab), + "the fixture must be a program validation accepts, or it proves nothing" + ); + let mut interp = Interpreter::new(&vocab, &p, Permissive::default()); + assert_eq!( + interp.run(), + Err(RunError::RecursionDepth { + call: FnIndex::IF, + depth: DEFAULT_RECURSION_DEPTH + }) + ); + } + + /// FAILS IF: the iteration cap is charged after the last body instead of + /// before the next one. + /// + /// Checked afterwards, a loop that terminates in EXACTLY `cap` iterations + /// — inside the advertised ceiling — was rejected, because the cap fired + /// before the condition could be re-tested one final time. At `cap = 0` + /// the same ordering ran the body once before refusing, which is a cap of + /// zero that executes. + #[test] + fn a_loop_terminating_in_exactly_cap_iterations_is_accepted() { + // counter = 3; while counter > 0 { counter -= 1 } — exactly 3 bodies. + let entry = FunctionBody::from_calls( + LaneShape::Pairs, + &[ + Call::with_value(FnIndex::NUMBER, 3), + Call::with_value(FnIndex::VAR_SET, 0), + Call::with_value(FnIndex::VAR_GET, 0), + Call::with_value(FnIndex::NUMBER, 0), + Call::new(FnIndex::GT), + Call::with_value(FnIndex::WHILE, 1), + ], + ) + .unwrap(); + let body = FunctionBody::from_calls( + LaneShape::Pairs, + &[ + Call::with_value(FnIndex::VAR_GET, 0), + Call::with_value(FnIndex::NUMBER, 1), + Call::new(FnIndex::SUB), + Call::with_value(FnIndex::VAR_SET, 0), + ], + ) + .unwrap(); + let p = Program { + functions: vec![entry, body], + }; + let vocab = validate(CoreOnly).expect("conforms"); + let mut interp = Interpreter::new(&vocab, &p, I64Dialect::default()).with_iteration_cap(3); + interp + .run() + .expect("3 iterations under a cap of 3 must run"); + assert_eq!(interp.dialect.vars[0], 0, "the loop ran to completion"); + + // The paired half: a cap of ZERO must refuse before running anything. + let mut zero = Interpreter::new(&vocab, &p, I64Dialect::default()).with_iteration_cap(0); + assert_eq!( + zero.run(), + Err(RunError::IterationCap { + call: FnIndex::WHILE, + cap: 0 + }) + ); + assert_eq!( + zero.dialect.vars[0], 3, + "a cap of zero must not execute a body" + ); + } } From 99f2d1b727fc41b1f393c48999ad82844705728e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 20:54:21 +0000 Subject: [PATCH 02/10] docs: what ogar-loco needs to carry rs-graph-llm's orchestration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Read graph-flow (3,723 LOC, 11 modules) against loco's engine and wrote the gap list. The headline is a decision, not an inventory: loco must NOT become async. graph-flow gets human-in-the-loop by being async all the way down — tokio, async_trait, a SessionStorage trait with four backends. Porting that shape would pull a runtime into a zero-dep call ABI. Suspension replaces it, and is strictly more general for one reason: a suspended loco program is BYTES — persistable, inspectable, diffable, learnable. A pending Rust future is none of those. Four gaps, one of them the keystone: G2, the explicit frame stack, is where the work is. The interpreter recurses today, so its state lives in the Rust call stack where nothing can pause or persist it. Made explicit, a Frame is (u16, u16) = 4 bytes = one quad slot — so a CONTINUATION IS A FunctionBody, 90 frames deep in one 512-byte node, in the format everything else already speaks. SessionStorage then needs no analog in loco at all: a session is bytes, and what a consumer does with bytes is the consumer's business. G1 is a jump byte; G3 is a dialect snapshot SEAM (loco defines the seam, never the format — the V3 facet register is already the state model and a JSON map would be a downgrade); G4 is the one that inverts graph-flow's answer outright: N parallel branches are ONE mask program, not N spawned tasks. Two findings worth keeping. The conditional edge needs NO work and is already the stronger form — graph-flow attaches an opaque closure, loco attaches a PROGRAM (the condition span), and a closure cannot be serialized, inspected, recompiled or learned. And G4 is where the masking-ops-make-NARS-cheap line lands mechanically: a tactic costing a task spawn cannot be orchestration; a tactic costing a ternlog can. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01X6y3drwKSE2zSgoexheLFX --- docs/LOCO-ORCHESTRATION-GAP.md | 177 +++++++++++++++++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 docs/LOCO-ORCHESTRATION-GAP.md diff --git a/docs/LOCO-ORCHESTRATION-GAP.md b/docs/LOCO-ORCHESTRATION-GAP.md new file mode 100644 index 0000000..b91e2de --- /dev/null +++ b/docs/LOCO-ORCHESTRATION-GAP.md @@ -0,0 +1,177 @@ +# What `ogar-loco` needs to carry `rs-graph-llm`'s orchestration + +> Design pass, 2026-09-14. Reference read: `AdaWorldAPI/rs-graph-llm` +> `graph-flow` at `59f9315` (3,723 LOC across 11 modules). Nothing here is +> built; this is the gap list and the shape each closure must take. + +## The headline: loco must NOT become async + +`graph-flow` gets human-in-the-loop by being async all the way down — +`#[async_trait] Task`, tokio, a `SessionStorage` trait with four backends, and +a `FlowRunner` that loads a session, steps it, and saves it. Porting that +shape into `ogar-loco` would pull `tokio` + `serde_json` + `async-trait` into +a zero-dep crate whose whole value is being a 512-byte call ABI. That is not a +trade worth making, and it is not necessary. + +**Suspension replaces async.** A node that needs to await an LLM call returns +`Suspend`; the HOST awaits; the host resumes. That is strictly more general +than an async runtime, for one reason worth stating plainly: + +> A suspended loco program is **bytes** — persistable, inspectable, +> diffable, learnable. A pending Rust future is none of those. + +Everything below follows from that one decision. + +## Feature-by-feature + +| `graph-flow` | `ogar-loco` today | verdict | +|---|---|---| +| `Task` — async node, `run(Context) -> TaskResult` | `Call` / `FunctionBody` | **have it**, and finer-grained: a node is 180 calls, not one closure | +| `Edge { from, to, condition: Arcbool> }` | `IF` / `IF_ELSE` + the condition span | **have it, and better** — see below | +| `NextAction::{Continue, ContinueAndExecute}` | the pc walk | have it | +| `NextAction::End` | body exhaustion | have it | +| `NextAction::GoTo(String)` | — | **G1** | +| `NextAction::WaitForInput` + session save/load | — | **G2 — the keystone** | +| `Context` — `HashMap` behind a lock | dialect-private state + `VAR_GET`/`VAR_SET`'s 256 slots | **G3** | +| `FanOutTask` + tokio | — | **G4** | +| `SessionStorage` × 4 backends | — | **nothing in loco** — see G2 | + +### The conditional edge is already the stronger form + +`graph-flow` attaches `Arc bool>` to an edge. loco attaches +a **program** — the calls preceding the branch, re-run each iteration +(`Interpreter::operand_span_start`). A closure cannot be serialized, cannot be +inspected, cannot be compiled to a different backend, and cannot be learned. A +condition span is all four. This is not a gap to close; it is a reason the +port goes in this direction rather than the other. + +--- + +## G1 — a jump byte + +`NextAction::GoTo(String)` is dynamic routing by name. loco's equivalent is a +call whose value byte is a **body reference** — the `body_refs` machinery the +vocabulary already declares per byte, and which `IF`/`REPEAT` already use. + +The shared core reserves `STOP` / `RETURN` / `BREAK` / `CONTINUE` and the +engine currently REFUSES all four (`RunError::UnhandledControlFlow`), which is +the right posture: they were never validated by a probe. `GOTO` joins that +list, and lands the same way — with a falsifier, not with a guess. + +Cost: one arm in `run_branching`, plus a loop-safety question `IF`/`REPEAT` do +not have (a `GOTO` can build a cycle the structured ops cannot). The iteration +cap is already the answer; it just has to cover jumps as well as loops. + +## G2 — an explicit frame stack (the keystone) + +**Today** `Interpreter::run_function(index)` RECURSES. The interpreter's state +therefore lives in the Rust call stack, where it cannot be paused, persisted, +or examined. + +**Needed:** `frames: Vec`, walked iteratively. +Then the whole interpreter state is `(frames, dialect_state, stack)`. + +The loco-native part, and the reason this is cheap rather than a rewrite tax: + +> A `Frame` is `(u16, u16)` = 4 bytes = exactly one `u8:u8:u8:u8` slot in +> `LaneShape::Quads`. **A continuation IS a `FunctionBody`.** 90 quads = 90 +> frames of depth, in one 512-byte node, in the format everything else already +> speaks. + +So `run()` becomes: + +``` +enum Run { Done(V), Suspended(Continuation) } +``` + +and `resume(k, injected_value)` continues. `SessionStorage` needs no analog in +loco at all: a session is a `FunctionBody` plus a dialect blob, and what a +consumer does with bytes is the consumer's business. The zero-dep posture +survives intact. + +Two things this changes that must be re-pinned, not absorbed: + +- **The `WHILE` condition-span re-run becomes frame state.** Today it is an + inner `for` loop inside one `run_branching` call; under an explicit stack it + must be a resumable position. That is the single subtlest part of the + rewrite, and it is exactly the machinery + `while_reruns_its_condition_span_and_computes_gcd` already falsifies — so + the test that guards it exists before the change does. +- **The iteration cap becomes a BUDGET spendable across resumptions.** That is + also how LangGraph-style step limits fall out for free. + +## G3 — a dialect snapshot seam + +`graph-flow`'s `Context` is a `HashMap` behind a +lock. loco must not grow one: the V3 12-byte facet register keyed by classid +is the stack's own state model, and it is zero-copy where a JSON map is not. + +What is genuinely missing is narrower: **the dialect's store is opaque to the +engine**, so the engine cannot persist it across a suspension. The fix is a +byte seam, and loco must define the *seam* and never the *format*: + +``` +trait Dialect { + fn snapshot(&self, out: &mut Vec); // or a &mut [u8] sink + fn restore(&mut self, bytes: &[u8]) -> Result<(), Self::Error>; +} +``` + +A dialect whose state is already facet rows writes them directly; one holding +`[i64; 256]` writes 2 KiB; one holding masks writes mask words. The engine +never looks inside. `VAR_GET`/`VAR_SET`'s 256 slots stay exactly what they +are — the named half of the state model, already addressable. + +## G4 — fan-out is a MASK op, not a task pool + +`FanOutTask` runs N child tasks concurrently on tokio and merges their writes +into one shared `Context`. That is the right shape for an async LLM +orchestrator and the wrong one here. + +In loco's world, N parallel branches over one population is **one mask program**: +N `MaskOp::Pred` into N slots, then a combine. No threads, no shared mutable +context, no merge policy to get wrong — and it is the same substrate the query +side uses. Fan-out therefore lowers DOWN into `lance-graph-mask-risc`, not out +into a runtime. + +This is where the operator's line lands: *the masking ops were necessary to +make the 34 NARS reasoning cheap*. A tactic that costs a task spawn cannot be +orchestration; a tactic that costs a ternlog can. + +--- + +## The ladder this sits in + +``` +ndarray::simd masking ops ← primitives +mask-risc Program ← primitives composed into a method +loco opcode (0x90.. per classid) ← the method becomes an opcode +loco Interpreter ← orchestration: branch, loop, suspend, resume +``` + +`ogar-r2il` already occupies `0x90..=0xE1` **under its own classid** — 82 +machine opcodes. A query dialect, a NARS-tactic dialect and a Blockly palette +each get their own classid and their own `0x90..` range, routed by +`VocabularyRegistry` (concept id → validated `VocabularyTable`, hi-u16 +canon-high). That is what "loco speaks all dialects" means mechanically: +**anything else just needs a classid.** + +The basic vocabulary is **144 + 34 + 36**: the 144 verb atoms (rung 2), the 34 +NARS tactic recipes (`lance_graph_contract::recipes::RECIPES: [Recipe; 34]`, +rung 3 — the runbooks), and the 36 `ThinkingStyle`s (rung 4). The 34 exist +today as ~30 bespoke Rust functions in `ndarray/src/hpc/styles/`; as loco +programs they become data, and data is what can be revised, learned and +persisted. + +## Open, and deliberately not decided here + +- **The core's 144 slots currently hold the Blockly palette.** The core is + exactly `0x00..=0x8F` = 144, and the verb table is exactly 12×12 = 144. + Whether the palette moves above `DOMAIN_FLOOR` so the core can hold the 144 + verb atoms is an operator call, not one to make inside a design doc. +- **`256:256` as function-value syntax.** A `u8:u8` lane read as + `(function, value)` is one call; read as `palette256:palette256` it is a + centroid pair. Both readings live in the same 12 bytes and the ClassView + picks. What that buys the ORCHESTRATION layer specifically — a call whose + "function" is itself a centroid, i.e. a soft dispatch — is unexplored and + should be probed before it is designed. From 36cf9fc44604eea0bbbaf3ed3bafe8ecd646d036 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 21:01:56 +0000 Subject: [PATCH 03/10] =?UTF-8?q?docs:=20correct=20the=20loco=20orchestrat?= =?UTF-8?q?ion=20answer=20=E2=80=94=20it=20is=20Inventory,=20not=20four=20?= =?UTF-8?q?gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Operator, on the previous version: "All I need from ogar-loco to know functions as objects. It can't be so hard." Correct, and the crate already says so. `LocoConcept::Inventory` (0x1702) is ALREADY MINTED and its own doc reads "the function registry entry (which functions exist, ADDRESSED BY IDENTITY). A registry read never touches a body." That is functions-as-objects, already designed. What is missing is that nothing implements it: `Program`'s `Vec` is a placeholder, and the interpreter resolves a branch by Vec index instead of by address. The whole change is one indirection — `branch` resolving through an Inventory rather than `functions[byte]`. A function at rest is already an object; `FunctionNode` is 512 bytes with a 16-byte key in slot 0, deliberately opaque so the substrate mints it. The identity exists and the runtime does not use it. Once a branch target is an address, the four "gaps" the previous version enumerated stop being gaps and become consequences: a function can be a value, a continuation is a function, graph orchestration is functions referencing functions. async / Context / SessionStorage were graph-flow's answers to not having this. Regraded in place per this repo's append-only rule rather than rewritten: the reading record of graph-flow at 59f9315 stands, the "no async" decision stands, and the correction says which sentence was the answer and which were downstream of it. The error was mapping their architecture onto loco feature-by-feature instead of asking what loco lacks. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01X6y3drwKSE2zSgoexheLFX --- docs/LOCO-ORCHESTRATION-GAP.md | 40 ++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/docs/LOCO-ORCHESTRATION-GAP.md b/docs/LOCO-ORCHESTRATION-GAP.md index b91e2de..148e0d9 100644 --- a/docs/LOCO-ORCHESTRATION-GAP.md +++ b/docs/LOCO-ORCHESTRATION-GAP.md @@ -4,6 +4,46 @@ > `graph-flow` at `59f9315` (3,723 LOC across 11 modules). Nothing here is > built; this is the gap list and the shape each closure must take. +## ⊘ CORRECTED — the answer is ONE thing, and this doc buried it (operator, 2026-09-14) + +Operator, on the version below: *"All I need from ogar-loco to know functions +as objects. It can't be so hard."* + +Right, and this crate already says so. `LocoConcept::Inventory` — **`0x1702`, +already minted** — carries this doc in `lib.rs`: + +> the **inventory** row: the function registry entry (which functions exist, +> **addressed by identity**). A registry read never touches a body. + +That IS functions-as-objects. It is already the design, already has a concept +id, and **nothing implements it**. `Program { functions: Vec }` +is a placeholder standing in for it, and the interpreter resolves a branch as +`self.program.functions.get(idx)` — a Vec index, not an address. + +So the whole change is one indirection: + +``` +branch(target) → inventory.get(address) // not functions[byte] +``` + +A function AT REST is already an object: `FunctionNode` is 512 bytes with a +16-byte key in slot 0, and `node.rs` keeps that key deliberately opaque so the +substrate mints it. The identity exists; the runtime does not use it. + +**Once a branch target is an address rather than a local index, the four +"gaps" below stop being gaps and become consequences.** A function can be a +VALUE (its address fits an immediate byte through the constant pool, or the +facet's 12 bytes directly); a continuation is a function; graph orchestration +is functions referencing functions. No async, no `Context` map, no +`SessionStorage` — those are graph-flow's answers to *not having this*. + +**The error in what follows:** it maps graph-flow's architecture onto loco +feature-by-feature, instead of asking what loco lacks. Everything below is +kept as the reading record of `graph-flow` at `59f9315` — the decision "no +async" still stands, and the four consequences are real — but `Inventory` plus +a `branch` that resolves through it is the answer, and the rest is downstream +of it. + ## The headline: loco must NOT become async `graph-flow` gets human-in-the-loop by being async all the way down — From afa89d52d992b1ecc38e88977379ca007032f77b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 12:44:12 +0000 Subject: [PATCH 04/10] loco: functions are objects (Inventory) and the 34 NARS recipes are a vocabulary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The implementation of what `fdf1517` said the real answer was. `LocoConcept::Inventory` (0x1702) has carried the design in its own doc since it was minted — "the function registry entry (which functions exist, addressed by identity)" — and nothing implemented it. `Program`'s `Vec` stood in, so a branch target was a byte index into one program's private list: a function could not be passed, stored, shared between programs, or named from anywhere but the body that happened to contain it. `inventory.rs` — `FnAddr(u16)` + the `Inventory` trait + `VecInventory`. An address is deliberately NOT the 16-byte key: a call's whole payload is 1-3 bytes, so the address is what a CALL carries and the key is what a STORE carries; `Inventory::key_of` is the one map between them, so a consumer that stores by key and executes by address needs no second table. A trait rather than a struct because where the bodies live is not this crate's business — a test holds them in a `Vec`, a consumer resolves them out of a node store, a Lance scan, or a cache, and the interpreter only ever asks "give me the body at this address". `nars.rs` — the 34 recipes as a loco vocabulary at `0x90..=0xB1` (`DOMAIN_FLOOR` + 34), and the column that says which calls are masking ops. The mapping is not invented here: each `Recipe` in `lance_graph_contract::recipes::RECIPES` already carries a `bucket`, and the three buckets ARE the three execution tiers — Datapath ("uniform, branch-free, every-cycle SIMD") is the masking ops, Control is loco's interpreter, Gate is a predicate before either. Measured over the real catalogue: 9 Datapath, 19 Control, 6 Gate. Overlapping ogar-r2il's range is not a collision — a vocabulary is selected by the node's classid, so every dialect starts its own bytes at the floor. Two doc corrections made while landing this, both because the claim was checkable and I checked it: - The manifest comment and nars.rs both said FOUR sibling crates already pull `lance-graph-contract` on this coordinate. Measured: EIGHT — ogar-auth, ogar-class-view, ogar-doc-ir, ogar-from-ruff, ogar-r2il, ogar-rbac, ogar-render-askama, ogar-render-typst. So `branch = "main"` here is this repo's own established convention, counted rather than recalled. - `pub mod inventory; pub mod nars;` had been inserted ahead of `interpret`, breaking the list's alphabetical order. rustfmt does not reorder mods, so nothing would have caught it. Gates: `cargo test -p ogar-loco` 75 passed / 0 failed; `cargo clippy -p ogar-loco --all-targets --no-deps -- -D warnings` clean; `cargo fmt -p ogar-loco -- --check` clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01X6y3drwKSE2zSgoexheLFX --- crates/ogar-loco/Cargo.toml | 10 ++ crates/ogar-loco/src/inventory.rs | 156 +++++++++++++++++++++++++++++ crates/ogar-loco/src/lib.rs | 3 + crates/ogar-loco/src/nars.rs | 161 ++++++++++++++++++++++++++++++ 4 files changed, 330 insertions(+) create mode 100644 crates/ogar-loco/src/inventory.rs create mode 100644 crates/ogar-loco/src/nars.rs diff --git a/crates/ogar-loco/Cargo.toml b/crates/ogar-loco/Cargo.toml index f34237a..26c748c 100644 --- a/crates/ogar-loco/Cargo.toml +++ b/crates/ogar-loco/Cargo.toml @@ -14,3 +14,13 @@ serde = ["dep:serde"] [dependencies] serde = { workspace = true, optional = true } + +# The 34 NARS reasoning recipes (`recipes::RECIPES`) — the rung-3 runbooks this +# crate's `nars` vocabulary addresses as domain bytes. `lance-graph-contract` is +# itself ZERO-DEP ("a trait-only crate ... MUST stay dependency-free even of +# optional path deps"), so this costs nothing in dependency weight. EIGHT sibling +# OGAR crates already pull it on this exact coordinate — ogar-auth, +# ogar-class-view, ogar-doc-ir, ogar-from-ruff, ogar-r2il, ogar-rbac, +# ogar-render-askama, ogar-render-typst — so `branch = "main"` here is this +# repo's own established convention, measured rather than assumed. +lance-graph-contract = { git = "https://github.com/AdaWorldAPI/lance-graph", branch = "main" } diff --git a/crates/ogar-loco/src/inventory.rs b/crates/ogar-loco/src/inventory.rs new file mode 100644 index 0000000..ddbfd23 --- /dev/null +++ b/crates/ogar-loco/src/inventory.rs @@ -0,0 +1,156 @@ +//! **Functions as objects** — a call resolves an ADDRESS, not a `Vec` index. +//! +//! # What was missing, and where it already said so +//! +//! [`LocoConcept::Inventory`](crate::LocoConcept::Inventory) — `0x1702`, minted +//! — already carries the design in its own doc: +//! +//! > the **inventory** row: the function registry entry (which functions exist, +//! > **addressed by identity**). A registry read never touches a body. +//! +//! That is functions-as-objects, and nothing implemented it. +//! [`Program`](crate::Program)'s `Vec` stood in for it, so a +//! branch target was a byte index into one program's private list. A function +//! could not be passed, stored, shared between programs, or named from +//! anywhere but the body that happened to contain it. +//! +//! A function AT REST was already an object: +//! [`FunctionNode`](crate::FunctionNode) is 512 bytes with a 16-byte key in +//! slot 0, and `node.rs` keeps that key deliberately opaque so the substrate +//! mints it. The identity existed; the runtime did not use it. +//! +//! # What an address is +//! +//! [`FnAddr`] is a `u16` — 65,536 functions per inventory, the same ceiling +//! the substrate's other `u16` index spaces use. It is an **index into an +//! inventory**, NOT a GUID: the 16-byte key is the canon's, minting is the +//! substrate's, and an interpreter that embedded a key in every branch would +//! be carrying 16 bytes where a call has one or two. +//! +//! The inventory is what maps between them ([`Inventory::key_of`]), so a +//! consumer that stores by key and executes by address needs no second table. +//! +//! # Why a trait +//! +//! Because where the bodies live is not this crate's business. A test holds +//! them in a `Vec`; a consumer resolves them out of a node store, a Lance +//! scan, or a cache. The interpreter only ever asks "give me the body at this +//! address", which is the one question every such backing can answer. + +use crate::FunctionBody; + +/// A function's address within one [`Inventory`]. +/// +/// Deliberately NOT the 16-byte key: an address is what a CALL carries, and a +/// call's whole payload is 1-3 bytes. The key is what a STORE carries. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Default)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +#[repr(transparent)] +pub struct FnAddr(pub u16); + +impl FnAddr { + /// The entry address. Reserved: a body-reference byte of `0` means + /// "unset", so no call may branch to it — the same rule + /// `Program::references_are_resolvable` already enforces. + pub const ENTRY: FnAddr = FnAddr(0); +} + +/// Where the interpreter gets a body from. +/// +/// One question, because that is all the interpreter asks. `key_of` is the +/// second half of functions-as-objects — without it an address is a private +/// index again, and a consumer could not tell two inventories apart. +pub trait Inventory { + /// The body at `addr`, or `None` if nothing is registered there. + fn body(&self, addr: FnAddr) -> Option<&FunctionBody>; + + /// The canonical 16-byte key for `addr`, when the backing knows one. + /// + /// `None` is honest rather than a zero key: a test inventory built from + /// bare bodies has no minted identity, and answering `[0u8; 16]` would be + /// a GUID that collides with every other unminted function. + fn key_of(&self, _addr: FnAddr) -> Option<[u8; 16]> { + None + } + + /// How many addresses this inventory can answer. Used only for + /// diagnostics; `body` returning `None` is the real bound. + fn len(&self) -> usize; + + /// Whether the inventory holds nothing. + fn is_empty(&self) -> bool { + self.len() == 0 + } +} + +/// The obvious in-memory inventory: bodies in registration order. +/// +/// Registration order IS the address, which makes a `VecInventory` built from +/// a [`Program`](crate::Program)'s own `functions` behave exactly as the old +/// index did — that equivalence is what lets the change land without +/// re-pinning a single existing program. +#[derive(Debug, Clone, Default)] +pub struct VecInventory { + bodies: Vec, + keys: Vec>, +} + +impl VecInventory { + /// An empty inventory. + #[must_use] + pub fn new() -> Self { + Self::default() + } + + /// Append a body, returning the address it landed at. + pub fn push(&mut self, body: FunctionBody) -> FnAddr { + let a = self.bodies.len(); + self.bodies.push(body); + self.keys.push(None); + // A `u16` address space means the 65,537th body has nowhere to live. + // Saturating would silently alias it onto the last legal address, so + // the cast is checked and the overflow is a panic at BUILD time, not + // a wrong branch at run time. + FnAddr(u16::try_from(a).expect("inventory exceeds the u16 address space")) + } + + /// Append a body with its minted key. + pub fn push_keyed(&mut self, key: [u8; 16], body: FunctionBody) -> FnAddr { + let a = self.push(body); + self.keys[a.0 as usize] = Some(key); + a + } + + /// The address holding `key`, if any. Linear — a consumer with many + /// functions brings its own index; this one is for tests and small sets. + #[must_use] + pub fn addr_of(&self, key: &[u8; 16]) -> Option { + self.keys + .iter() + .position(|k| k.as_ref() == Some(key)) + .and_then(|i| u16::try_from(i).ok()) + .map(FnAddr) + } +} + +impl FromIterator for VecInventory { + fn from_iter>(iter: I) -> Self { + let bodies: Vec = iter.into_iter().collect(); + let keys = vec![None; bodies.len()]; + Self { bodies, keys } + } +} + +impl Inventory for VecInventory { + fn body(&self, addr: FnAddr) -> Option<&FunctionBody> { + self.bodies.get(addr.0 as usize) + } + + fn key_of(&self, addr: FnAddr) -> Option<[u8; 16]> { + self.keys.get(addr.0 as usize).copied().flatten() + } + + fn len(&self) -> usize { + self.bodies.len() + } +} diff --git a/crates/ogar-loco/src/lib.rs b/crates/ogar-loco/src/lib.rs index 56b9176..2b9fed9 100644 --- a/crates/ogar-loco/src/lib.rs +++ b/crates/ogar-loco/src/lib.rs @@ -121,6 +121,8 @@ use serde::{Deserialize, Serialize}; pub mod basin; pub mod interpret; +pub mod inventory; +pub mod nars; pub mod node; pub mod pool; pub mod program; @@ -130,6 +132,7 @@ pub mod telemetry; pub mod vocabulary; pub use interpret::{Dialect, Interpreter, RunError}; +pub use inventory::{FnAddr, Inventory, VecInventory}; pub use node::FunctionNode; pub use pool::{Constant, ConstantPool, PoolError}; pub use program::{Program, branches_of}; diff --git a/crates/ogar-loco/src/nars.rs b/crates/ogar-loco/src/nars.rs new file mode 100644 index 0000000..a4f9424 --- /dev/null +++ b/crates/ogar-loco/src/nars.rs @@ -0,0 +1,161 @@ +//! The 34 NARS reasoning recipes as a loco vocabulary — and the column that +//! says **which calls are masking ops**. +//! +//! # The mapping was already data +//! +//! Each [`Recipe`] in `lance_graph_contract::recipes::RECIPES` carries a +//! `bucket`, and the three buckets ARE the three execution tiers. This is not +//! an assignment invented here; it is a column that has been sitting in the +//! catalogue: +//! +//! | `Bucket` | the catalogue's own words | who executes it | +//! |---|---|---| +//! | `Datapath` | *"uniform, branch-free, every-cycle SIMD"* | **the masking ops** | +//! | `Control` | *"branchy decision at a control point"* | **loco's interpreter** | +//! | `Gate` | *"a cheap marker that gates whether deeper work fires"* | a predicate, before either | +//! +//! Measured over the real catalogue: **9 Datapath, 19 Control, 6 Gate**. The +//! Datapath nine are literally mask/VSA primitives — #25 `HPM` is *"the +//! substrate: fingerprint cosine/Hamming sweep (SIMD)"*, #19 `ARE` is +//! `A⊗B⊗B=A`, #27 `MPC` is *"bundle = majority-vote-per-bit"*. For those, +//! **a `(function : value)` call IS a masking op**, and [`tier_of`] is how a +//! consumer asks. +//! +//! # Why depending on the contract costs nothing +//! +//! `lance-graph-contract` is itself zero-dep — "a trait-only crate" that "MUST +//! stay dependency-free even of optional path deps". EIGHT OGAR crates already +//! pull it (`ogar-auth`, `ogar-class-view`, `ogar-doc-ir`, `ogar-from-ruff`, +//! `ogar-r2il`, `ogar-rbac`, `ogar-render-askama`, `ogar-render-typst` — +//! counted, not recalled; an earlier draft of this paragraph said four), +//! and `ogar-from-ruff`'s manifest says so in as many words: *"Pulls the +//! ZERO-DEP `lance-graph-contract` only."* This crate's "Zero-dep" description +//! was never an argument against carrying the catalogue. +//! +//! # Byte allocation +//! +//! The 34 occupy `0x90..=0xB1` — `DOMAIN_FLOOR` plus 34. That they overlap +//! `ogar-r2il`'s range is not a collision: a vocabulary is selected by the +//! node's **classid**, so every dialect starts its own bytes at the floor. +//! That is what the registry is for. +//! +//! # What is DERIVED and what is POLICY +//! +//! Derived (read from the catalogue, never chosen here): the code, the name, +//! the bucket, the substrate string, the id→byte mapping. +//! +//! **Policy (chosen here, and re-pinnable):** the stack arities below. The +//! catalogue records what realizes a tactic, not how many operands its call +//! pops, so the arities are a stated default per bucket rather than a +//! measurement — say so rather than let a future session inherit them as +//! fact. Each is the narrowest shape its bucket implies: +//! +//! - `Datapath` → **2**: the VSA/mask primitives here are binary at their +//! core (`bind(A,B)`, bundle-of-two, a cosine sweep of query against +//! corpus). A ternary form like `ARE`'s `A⊗B⊗B` composes two binds. +//! - `Gate` → **1**: a marker reads one thing and answers. +//! - `Control` → **1**: an orchestration call takes the subject it +//! orchestrates. +//! +//! # The seam this does NOT close +//! +//! `body_refs` is **0 for every recipe**, including Control. A Control recipe +//! genuinely orchestrates a body, so it wants a body reference — but the +//! engine's control band is `0x01..=0x1F` and these bytes sit above +//! `DOMAIN_FLOOR`, so a body reference here would branch through a path +//! nothing has executed. Declaring `0` keeps the catalogue honest about what +//! runs today; the orchestration seam lands with its own falsifier, not as an +//! unexercised field. + +use crate::{DOMAIN_FLOOR, FnIndex, Vocabulary}; +use lance_graph_contract::recipes::{Bucket, RECIPES, Recipe}; + +/// First byte this vocabulary owns — [`DOMAIN_FLOOR`]. +pub const NARS_BASE: u8 = DOMAIN_FLOOR; + +/// How many bytes it owns: one per recipe. +pub const NARS_COUNT: u8 = RECIPES.len() as u8; + +/// Last byte this vocabulary owns, inclusive. +pub const NARS_LAST: u8 = NARS_BASE + NARS_COUNT - 1; + +const _: () = assert!( + NARS_LAST < 0xFF, + "the 34 recipes must fit above the domain floor" +); + +/// The recipe a byte names, or `None` outside `NARS_BASE..=NARS_LAST`. +/// +/// Recipe ids are `1..=34` and the catalogue is id-ascending, so the byte is +/// `NARS_BASE + (id - 1)`. Resolved by INDEX rather than by searching for a +/// matching `id`, and the debug assert below is what keeps those two readings +/// from drifting if the catalogue's order ever stops matching its ids. +#[must_use] +pub fn recipe_at(f: FnIndex) -> Option<&'static Recipe> { + let i = f.0.checked_sub(NARS_BASE)? as usize; + let r = RECIPES.get(i)?; + debug_assert_eq!( + r.id as usize, + i + 1, + "RECIPES is documented as id-ascending; byte↔id resolution relies on it" + ); + Some(r) +} + +/// Which tier executes this byte — the question "is this call a masking op?" +/// +/// `Some(Bucket::Datapath)` is a yes. +#[must_use] +pub fn tier_of(f: FnIndex) -> Option { + recipe_at(f).map(|r| r.bucket) +} + +/// Is this call a masking op — i.e. does the Datapath tier execute it? +#[must_use] +pub fn is_mask_op(f: FnIndex) -> bool { + matches!(tier_of(f), Some(Bucket::Datapath)) +} + +/// The byte a recipe id (`1..=34`) is addressed by, or `None` out of range. +#[must_use] +pub fn byte_of_id(id: u8) -> Option { + (1..=NARS_COUNT) + .contains(&id) + .then(|| FnIndex(NARS_BASE + id - 1)) +} + +/// The 34 recipes as a plug-in vocabulary. +/// +/// Below [`DOMAIN_FLOOR`] it is transparent — the shared core answers, as the +/// conformance rule requires. Above it, every byte's shape comes from its +/// recipe's bucket. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct NarsVocabulary; + +impl Vocabulary for NarsVocabulary { + fn domain_stack_arity(&self, f: FnIndex) -> Option { + // POLICY, not a measurement — see the module doc. + tier_of(f).map(|b| match b { + Bucket::Datapath => 2, + Bucket::Gate | Bucket::Control => 1, + }) + } + + fn domain_body_refs(&self, _f: FnIndex) -> u8 { + // Zero for every recipe, Control included. See "the seam this does + // NOT close" in the module doc: a body reference here would branch + // through a path nothing has executed. + 0 + } + + fn domain_pushes_result(&self, f: FnIndex) -> Option { + // All three tiers answer with something: Datapath a mask, Gate a + // marker, Control a verdict. Declared so bodies using these bytes are + // statement-segmentable rather than refused. + tier_of(f).map(|_| true) + } + + fn domain_name(&self, f: FnIndex) -> Option<&'static str> { + recipe_at(f).map(|r| r.code) + } +} From 4688926a5ffd379f9b4602d38a49f243e777643b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 12:57:36 +0000 Subject: [PATCH 05/10] loco: wire Inventory into the interpreter, and make the address bound one rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings from codex on #304, verified by reading before they were fixed. Both were right, and the first is the one worth recording. P1 — the abstraction was unreachable. `inventory.rs` shipped a trait no execution path could consume: `Interpreter::new` took only a `Program`, and `run_function`/`branch` both resolved through `self.program.functions`. So the "functions as objects" the PR advertised was unavailable to any caller, and a consumer implementing `Inventory` over a node store had nowhere to hand it. Two built ends that did not meet — the exact shape this session kept naming in OTHER people's code while shipping one. `Interpreter::with_inventory(&dyn Inventory)` + one `body_at` / one `body_count`, so the two resolution sites cannot disagree. `None` keeps resolving against `program.functions`, which is behaviour-neutral for every existing caller: a `VecInventory` built from a program's own `functions` has registration order as its address, so the two agree row for row. Two falsifiers, both disable-verified red-then-green (unhook `with_inventory` -> 2 FAILED; restore -> 77 passed): - `with_inventory_resolves_bodies_the_program_does_not_carry` — same entry, a different body 1 reachable only through the trait. Program says 3, inventory says 21, and it asserts they DIFFER so an implementation ignoring the program cannot pass either. - `the_branch_bound_follows_the_inventory_not_the_program` — a one-body program with a two-body inventory must reach address 1. The old `program.functions.len()` bound refuses it. P2 — `FromIterator` bypassed the bound `push` enforces, so a 65 537-body iterator built an inventory whose tail no `FnAddr` can name while `len()` still counted it. Now `MAX_ADDRESSES` is ONE spelling read by both paths, and the impl panics rather than truncates: the bound is the address space, not a policy. Disable-verified (drop the assert -> 1 FAILED). Also: `inventory.rs` had shipped with NO test module at all. Four now, including the paired silent half for the bound — EXACTLY `MAX_ADDRESSES` must be accepted and its LAST address must resolve, which is the case an off-by-one loses and which a reject-everything implementation would fail. Gates: `cargo test -p ogar-loco` 81 passed / 0 failed (was 75); clippy `--all-targets --no-deps -- -D warnings` clean (it caught a `.cloned()` on a `Copy` type in the new tests); `cargo fmt --check` clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01X6y3drwKSE2zSgoexheLFX --- crates/ogar-loco/src/interpret.rs | 136 +++++++++++++++++++++++++++++- crates/ogar-loco/src/inventory.rs | 100 ++++++++++++++++++++++ 2 files changed, 234 insertions(+), 2 deletions(-) diff --git a/crates/ogar-loco/src/interpret.rs b/crates/ogar-loco/src/interpret.rs index 1017ca7..5e69329 100644 --- a/crates/ogar-loco/src/interpret.rs +++ b/crates/ogar-loco/src/interpret.rs @@ -41,6 +41,7 @@ //! truthy condition, and would pass any test whose loop runs zero or one //! times. +use crate::inventory::{FnAddr, Inventory}; use crate::vocabulary::conformance::CheckedVocabulary; use crate::{Call, FnIndex, FunctionBody, Program, Vocabulary}; @@ -184,6 +185,13 @@ pub const DEFAULT_RECURSION_DEPTH: u32 = 64; pub struct Interpreter<'a, V: Vocabulary, D: Dialect> { vocab: &'a CheckedVocabulary, program: &'a Program, + /// Where bodies come from, when a caller supplies one. + /// + /// `None` resolves against `program.functions`, which is what every + /// pre-`Inventory` caller does and what keeps this change behaviour-neutral + /// for them: a `VecInventory` built from a program's own `functions` has + /// registration order as its address, so the two agree row for row. + inventory: Option<&'a dyn Inventory>, dialect: D, stack: Vec, iteration_cap: u32, @@ -197,6 +205,7 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { Self { vocab, program, + inventory: None, dialect, stack: Vec::new(), iteration_cap: DEFAULT_ITERATION_CAP, @@ -205,6 +214,42 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { } } + /// Resolve bodies through an [`Inventory`] instead of the program's own + /// `functions` list. + /// + /// This is the consumer half of functions-as-objects: a node store, a Lance + /// scan or a cache implements [`Inventory`] and the interpreter branches + /// into bodies it has never seen inside a `Program`. + /// + /// ⊘ Landed after codex flagged, correctly, that `inventory.rs` shipped a + /// trait no execution path could reach — `Interpreter::new` took only a + /// `Program` and both resolution sites went through `program.functions`, so + /// the advertised behaviour was unavailable to any caller. Two built ends + /// that did not meet, which is the exact shape this session kept naming + /// elsewhere. + #[must_use] + pub fn with_inventory(mut self, inventory: &'a dyn Inventory) -> Self { + self.inventory = Some(inventory); + self + } + + /// The body at `index`, from whichever backing this interpreter resolves + /// against. One place, so the two call sites cannot disagree. + fn body_at(&self, index: usize) -> Option<&'a FunctionBody> { + match self.inventory { + Some(inv) => u16::try_from(index).ok().and_then(|i| inv.body(FnAddr(i))), + None => self.program.functions.get(index), + } + } + + /// How many addresses the backing can answer — the bound `branch` checks. + fn body_count(&self) -> usize { + match self.inventory { + Some(inv) => inv.len(), + None => self.program.functions.len(), + } + } + /// Replace the recursion ceiling. pub fn with_recursion_depth(mut self, depth: u32) -> Self { self.recursion_depth = depth; @@ -244,7 +289,7 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { /// Run one function body to completion. fn run_function(&mut self, index: usize) -> Result<(), RunError> { - let Some(body) = self.program.functions.get(index) else { + let Some(body) = self.body_at(index) else { return Ok(()); }; let mut pc = 0usize; @@ -359,7 +404,7 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { /// Recurse into the body a branch names. fn branch(&mut self, call: FnIndex, target: u8) -> Result<(), RunError> { let idx = usize::from(target); - if idx == 0 || idx >= self.program.functions.len() { + if idx == 0 || idx >= self.body_count() { return Err(RunError::UnresolvedBody { call, target }); } if self.depth >= self.recursion_depth { @@ -428,6 +473,7 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { #[cfg(test)] mod tests { use super::*; + use crate::inventory::VecInventory; use crate::vocabulary::conformance::validate; use crate::{Call, LaneShape}; @@ -537,6 +583,92 @@ mod tests { Ok(interp.dialect) } + /// Run a program whose bodies come from an [`Inventory`] instead of from + /// the program's own `functions` list. + fn run_with_inventory( + program: &Program, + inv: &dyn Inventory, + ) -> Result> { + let vocab = validate(CoreOnly).expect("core-only vocabulary conforms"); + let mut interp = + Interpreter::new(&vocab, program, I64Dialect::default()).with_inventory(inv); + interp.run()?; + Ok(interp.dialect) + } + + /// `total = 0; REPEAT 3 -> body 1`, where body 1 adds `step` to `total`. + /// The entry is identical in both backings; only body 1 differs, so the + /// result says which backing was actually read. + fn step_program(step: u8) -> Program { + let entry = FunctionBody::from_calls( + LaneShape::Pairs, + &[ + Call::with_value(FnIndex::NUMBER, 0), + Call::with_value(FnIndex::VAR_SET, 0), + Call::with_value(FnIndex::NUMBER, 3), + Call::with_value(FnIndex::REPEAT, 1), + ], + ) + .unwrap(); + let body = FunctionBody::from_calls( + LaneShape::Pairs, + &[ + Call::with_value(FnIndex::VAR_GET, 0), + Call::with_value(FnIndex::NUMBER, step), + Call::new(FnIndex::ADD), + Call::with_value(FnIndex::VAR_SET, 0), + ], + ) + .unwrap(); + Program { + functions: vec![entry, body], + } + } + + /// FAILS IF: `with_inventory` does not actually redirect body resolution — + /// if `run_function` or `branch` still reads `program.functions`, the run + /// returns the PROGRAM's 3 rather than the INVENTORY's 21. + /// + /// Two-sided on purpose: the same program run WITHOUT an inventory must + /// still return 3, so this cannot pass by an implementation that ignores + /// the program entirely. + #[test] + fn with_inventory_resolves_bodies_the_program_does_not_carry() { + let program = step_program(1); + let without = run(&program).expect("runs"); + assert_eq!( + without.vars[0], 3, + "the program's own body adds 1, three times" + ); + + // Same entry, a DIFFERENT body 1 — reachable only through the trait. + let other = step_program(7); + let inv: VecInventory = other.functions.iter().copied().collect(); + let with = run_with_inventory(&program, &inv).expect("runs"); + assert_eq!(with.vars[0], 21, "the inventory's body adds 7, three times"); + assert_ne!( + without.vars[0], with.vars[0], + "if these agree the inventory was never consulted" + ); + } + + /// FAILS IF: `branch`'s bound reads the program's length while bodies come + /// from the inventory. A one-body program with a two-body inventory must + /// reach address 1; the old `self.program.functions.len()` refuses it. + #[test] + fn the_branch_bound_follows_the_inventory_not_the_program() { + let full = step_program(7); + let entry_only = Program { + functions: vec![full.functions[0]], + }; + let inv: VecInventory = full.functions.iter().copied().collect(); + let d = run_with_inventory(&entry_only, &inv).expect("runs"); + assert_eq!( + d.vars[0], 21, + "address 1 exists in the inventory, not the program" + ); + } + /// `sum 1..=n` with `REPEAT`: var0 = total, var1 = counter. fn sum_program(n: u8) -> Program { let entry = FunctionBody::from_calls( diff --git a/crates/ogar-loco/src/inventory.rs b/crates/ogar-loco/src/inventory.rs index ddbfd23..3065968 100644 --- a/crates/ogar-loco/src/inventory.rs +++ b/crates/ogar-loco/src/inventory.rs @@ -48,6 +48,12 @@ use crate::FunctionBody; #[repr(transparent)] pub struct FnAddr(pub u16); +/// How many addresses a [`VecInventory`] can name: 65 536, the `u16` space. +/// +/// ONE spelling, read by both [`VecInventory::push`] and the `FromIterator` +/// impl, so the two cannot enforce different bounds. +pub const MAX_ADDRESSES: usize = u16::MAX as usize + 1; + impl FnAddr { /// The entry address. Reserved: a body-reference byte of `0` means /// "unset", so no call may branch to it — the same rule @@ -134,8 +140,22 @@ impl VecInventory { } impl FromIterator for VecInventory { + /// Panics on more than [`MAX_ADDRESSES`] bodies, exactly as [`push`] does. + /// + /// ⊘ The first version collected straight into the `Vec` with no check, + /// which codex flagged: it bypassed the bound `push` enforces, so a + /// 65 537-body iterator produced an inventory whose tail no `FnAddr` can + /// name while `len()` still counted it. A silently unaddressable entry is + /// worse than a panic — the bound is the address space, not a policy. + /// + /// [`push`]: VecInventory::push fn from_iter>(iter: I) -> Self { let bodies: Vec = iter.into_iter().collect(); + assert!( + bodies.len() <= MAX_ADDRESSES, + "inventory exceeds the u16 address space: {} bodies, max {MAX_ADDRESSES}", + bodies.len() + ); let keys = vec![None; bodies.len()]; Self { bodies, keys } } @@ -154,3 +174,83 @@ impl Inventory for VecInventory { self.bodies.len() } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{Call, FnIndex, LaneShape}; + + fn body(v: u8) -> FunctionBody { + FunctionBody::from_calls(LaneShape::Pairs, &[Call::with_value(FnIndex::NUMBER, v)]) + .expect("a one-call body is well formed") + } + + /// FAILS IF: an address stops being registration order, or `key_of` + /// invents a key for an unkeyed entry. The `None` is the load-bearing + /// half — answering `[0u8; 16]` would be a GUID colliding with every + /// other unminted function. + #[test] + fn an_address_is_registration_order_and_an_unkeyed_entry_has_no_key() { + let mut inv = VecInventory::new(); + let a = inv.push(body(1)); + let b = inv.push(body(2)); + assert_eq!((a, b), (FnAddr(0), FnAddr(1))); + assert_eq!(inv.len(), 2); + assert_eq!(inv.body(a), Some(&body(1))); + assert_eq!(inv.body(b), Some(&body(2))); + assert_eq!(inv.key_of(a), None, "an unkeyed entry must not invent one"); + assert_eq!( + inv.body(FnAddr(2)), + None, + "past the end is None, not a wrap" + ); + } + + /// FAILS IF: `push_keyed` does not bind the key to the address it + /// returned, or `addr_of` matches a key it was never given. Two entries, + /// so a "return the only key" implementation cannot pass. + #[test] + fn a_minted_key_round_trips_to_its_own_address() { + let mut inv = VecInventory::new(); + let k1 = [1u8; 16]; + let k2 = [2u8; 16]; + let a = inv.push_keyed(k1, body(1)); + let b = inv.push_keyed(k2, body(2)); + assert_ne!(a, b); + assert_eq!(inv.key_of(a), Some(k1)); + assert_eq!(inv.key_of(b), Some(k2)); + assert_eq!(inv.addr_of(&k1), Some(a)); + assert_eq!(inv.addr_of(&k2), Some(b)); + assert_eq!( + inv.addr_of(&[9u8; 16]), + None, + "an absent key has no address" + ); + } + + /// FAILS IF: `FromIterator` collects without the bound `push` enforces. + /// + /// Codex flagged exactly this on OGAR #304: the 65 537th body has no + /// `FnAddr` that can name it, so a silent collect produces an inventory + /// whose tail is unreachable while `len()` still counts it. The bound is + /// the address space, not a policy, so it panics rather than truncates. + #[test] + #[should_panic(expected = "exceeds the u16 address space")] + fn from_iter_refuses_more_bodies_than_the_address_space_can_name() { + let _: VecInventory = std::iter::repeat_n(body(1), MAX_ADDRESSES + 1).collect(); + } + + /// The paired silent half: EXACTLY the address space is legal, and every + /// one of its addresses resolves. Without this the test above would pass + /// for an implementation that rejects any non-trivial iterator. + #[test] + fn from_iter_accepts_exactly_the_address_space() { + let inv: VecInventory = std::iter::repeat_n(body(1), MAX_ADDRESSES).collect(); + assert_eq!(inv.len(), MAX_ADDRESSES); + assert!(inv.body(FnAddr(0)).is_some(), "the first address resolves"); + assert!( + inv.body(FnAddr(u16::MAX)).is_some(), + "the LAST address resolves — this is the one an off-by-one loses" + ); + } +} From b6c89f8f419e67fc5975b35540f1f8b75702d9e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 13:05:55 +0000 Subject: [PATCH 06/10] loco: push validated after it mutated, and two design gaps the doc hand-waved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from CodeRabbit on #304. All three verified by reading; the first is a real bug my earlier fix missed. PUSH MUTATED BEFORE IT VALIDATED. codex's P2 was about `FromIterator` and I fixed that one. CodeRabbit found the sibling: `push` appended to BOTH vectors and only then ran `u16::try_from`, so a caller that catches the panic is left holding 65,537 entries whose last one no `FnAddr` can name — and `len()` counts it. The bound was enforced, but after the damage. Fixed by converting FIRST, so the conversion IS the check and there is no second rule to keep in step with it. Falsifier `a_refused_push_leaves_the_inventory_untouched` catches the panic and asserts `len()` is unchanged; disable-verified by restoring the old order (1 FAILED, "a refused push must not have grown the inventory"), then green at 82. "THE ITERATION CAP IS ALREADY THE ANSWER" WAS WRONG, and that sentence was mine. `iteration_cap` is PER-LOOP — REPEAT/WHILE/REPEAT_UNTIL each check their own count. A `GOTO` cycle contains no loop construct, so it consults nothing and runs forever inside a cap never reached. A per-loop ceiling cannot bound a control-flow graph. So GOTO now has a spec before it has an arm: a `step_budget` decremented once per call executed, whatever executed it — structured body, jump target, branch into another function, resumed run, identically. `iteration_cap` stays and stays per-loop; neither subsumes the other, since a program can exhaust the budget with no loop and a loop can hit its cap with the budget barely touched. Exhaustion refuses (`RunError::StepBudget`, carrying the call). A resumed run does NOT get a fresh budget, or suspension is an unbounded-execution loophole. THE SNAPSHOT SEAM HAD NO DESTINATION. `Dialect::snapshot`/`restore` were specified as a byte seam the engine never looks inside — and the doc never said where those bytes may live. A byte seam with an unspecified destination is one review away from being a serialization channel. Stated now, and it follows from ADR-022/023 rather than from taste: the snapshot is host-local and outside the hot path, and does not cross a mailbox. An opaque dialect blob is by construction NOT the IR, so it is exactly what must not be on a wire. If a suspension has to move between owners it travels as the IR and the receiving host rebuilds; `snapshot` is a resume aid for one host, never a transport. The erosion falsifier is greppable: no snapshot byte may appear in any type that crosses an owner boundary. Gates: `cargo test -p ogar-loco` 82 passed / 0 failed; clippy `--all-targets --no-deps -- -D warnings` clean; fmt clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01X6y3drwKSE2zSgoexheLFX --- crates/ogar-loco/src/inventory.rs | 54 ++++++++++++++++++++++++---- docs/LOCO-ORCHESTRATION-GAP.md | 60 +++++++++++++++++++++++++++++-- 2 files changed, 106 insertions(+), 8 deletions(-) diff --git a/crates/ogar-loco/src/inventory.rs b/crates/ogar-loco/src/inventory.rs index 3065968..8e02711 100644 --- a/crates/ogar-loco/src/inventory.rs +++ b/crates/ogar-loco/src/inventory.rs @@ -109,15 +109,30 @@ impl VecInventory { } /// Append a body, returning the address it landed at. + /// Panics past [`MAX_ADDRESSES`] — and does so BEFORE mutating. + /// + /// A `u16` address space means the 65 537th body has nowhere to live. + /// Saturating would silently alias it onto the last legal address, so the + /// conversion is checked and the overflow is a panic at BUILD time rather + /// than a wrong branch at run time. + /// + /// ⊘ The order here is the fix, not the check. The first version pushed to + /// both vectors and converted afterwards, which CodeRabbit caught on + /// OGAR #304: a caller that catches the panic is then holding an inventory + /// with 65 537 entries whose last one no `FnAddr` can name, and `len()` + /// counts it. Converting FIRST makes the failure leave nothing behind — + /// the conversion IS the check, so there is no second rule to keep in + /// step with it. pub fn push(&mut self, body: FunctionBody) -> FnAddr { - let a = self.bodies.len(); + let Ok(addr) = u16::try_from(self.bodies.len()) else { + panic!( + "inventory exceeds the u16 address space: {} bodies, max {MAX_ADDRESSES}", + self.bodies.len() + ); + }; self.bodies.push(body); self.keys.push(None); - // A `u16` address space means the 65,537th body has nowhere to live. - // Saturating would silently alias it onto the last legal address, so - // the cast is checked and the overflow is a panic at BUILD time, not - // a wrong branch at run time. - FnAddr(u16::try_from(a).expect("inventory exceeds the u16 address space")) + FnAddr(addr) } /// Append a body with its minted key. @@ -240,6 +255,33 @@ mod tests { let _: VecInventory = std::iter::repeat_n(body(1), MAX_ADDRESSES + 1).collect(); } + /// FAILS IF: `push` mutates before it validates. A caught panic must leave + /// the inventory EXACTLY as it was — `len()` unchanged and every address + /// still resolvable — because a caller that recovers is otherwise holding + /// an entry no `FnAddr` can name. + /// + /// The `len()` check is the load-bearing assertion: the old order pushed + /// to both vectors first, so this would read `MAX_ADDRESSES + 1`. + #[test] + fn a_refused_push_leaves_the_inventory_untouched() { + let mut inv: VecInventory = std::iter::repeat_n(body(1), MAX_ADDRESSES).collect(); + assert_eq!(inv.len(), MAX_ADDRESSES, "full to the last address"); + + let refused = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + inv.push(body(2)); + })); + assert!(refused.is_err(), "the 65 537th push must be refused"); + assert_eq!( + inv.len(), + MAX_ADDRESSES, + "a refused push must not have grown the inventory" + ); + assert!( + inv.body(FnAddr(u16::MAX)).is_some(), + "the last legal address still resolves" + ); + } + /// The paired silent half: EXACTLY the address space is legal, and every /// one of its addresses resolves. Without this the test above would pass /// for an implementation that rejects any non-trivial iterator. diff --git a/docs/LOCO-ORCHESTRATION-GAP.md b/docs/LOCO-ORCHESTRATION-GAP.md index 148e0d9..c20bd48 100644 --- a/docs/LOCO-ORCHESTRATION-GAP.md +++ b/docs/LOCO-ORCHESTRATION-GAP.md @@ -99,8 +99,35 @@ the right posture: they were never validated by a probe. `GOTO` joins that list, and lands the same way — with a falsifier, not with a guess. Cost: one arm in `run_branching`, plus a loop-safety question `IF`/`REPEAT` do -not have (a `GOTO` can build a cycle the structured ops cannot). The iteration -cap is already the answer; it just has to cover jumps as well as loops. +not have (a `GOTO` can build a cycle the structured ops cannot). + +**And "the iteration cap already covers it" is wrong — that sentence stood here +and CodeRabbit was right to reject it.** `iteration_cap` is a PER-LOOP ceiling: +`REPEAT`, `WHILE` and `REPEAT_UNTIL` each check their own count against it. A +`GOTO` cycle contains no loop construct, so it checks nothing and runs forever +inside a cap that is never consulted. A per-loop ceiling cannot bound a +control-flow graph; only a shared budget can. + +So `GOTO` lands with a second, distinct quantity, and the spec is the part that +has to exist before the arm does: + +- **`step_budget`** — one counter for the whole run, decremented **once per + call executed**, whatever executed it. A structured loop body, a `GOTO` + target, a branch into another function, and a resumed run after a suspension + all consume it identically, because they are all "a call ran". +- **`iteration_cap` stays, and stays per-loop.** It is a different guarantee: + it bounds ONE construct's repetitions so a runaway loop is attributable to + that loop. The budget bounds the RUN. Neither subsumes the other — a program + can exhaust the budget with no loop at all, and a single loop can hit its cap + while the budget is barely touched. +- **Exhaustion is a refusal, not a truncation:** `RunError::StepBudget`, + carrying the call that spent the last step, so a caller can see where. +- **A resumed run does NOT get a fresh budget.** The remaining count is part of + what `snapshot` persists (see G3 below) — otherwise suspension is an + unbounded-execution loophole: suspend, resume, repeat. + +Until that exists, `GOTO` stays refused alongside `STOP` / `RETURN` / `BREAK` / +`CONTINUE`, which is the correct posture and not a gap. ## G2 — an explicit frame stack (the keystone) @@ -162,6 +189,35 @@ A dialect whose state is already facet rows writes them directly; one holding never looks inside. `VAR_GET`/`VAR_SET`'s 256 slots stay exactly what they are — the named half of the state model, already addressable. +### Where those bytes are allowed to live — the boundary, stated + +CodeRabbit asked this and the doc did not answer it, which is a real omission +rather than a nit: a byte seam whose destination is unspecified is one review +away from becoming a serialization channel. + +**The snapshot is HOST-LOCAL and OUTSIDE the hot path. It does not cross a +mailbox.** A suspended run's bytes belong to whoever is holding that run — a +scheduler slot, a local arena, a durable store the host owns — and they are +read back by the same host on resume. That is what makes `snapshot` cheap +enough to be worth having. + +This is not a preference; it is what the repo's own non-negotiables already +require: + +- **ADR-022 / ADR-023 — the Firewall.** No serialization in the hot path; the + IR is wire truth. An opaque dialect blob is by construction NOT the IR, so it + is exactly the thing that must not be on a wire. Writing it host-locally is + not a crossing; handing it to another mailbox would be. +- **Therefore:** if a suspension ever has to move between owners, it does not + travel as a snapshot blob. It travels as the IR — the program, its address, + and the facet rows that are already the state model — and the receiving host + rebuilds. `snapshot` is a resume aid for one host, never a transport. + +The falsifier, so this cannot quietly erode: **no snapshot byte may appear in +any type that crosses an owner boundary.** If a future `Baton`, envelope, or +mailbox row grows a field carrying `Dialect::snapshot` output, that is the +violation, and it is greppable rather than a matter of judgement. + ## G4 — fan-out is a MASK op, not a task pool `FanOutTask` runs N child tasks concurrently on tokio and merges their writes From 27f55b0daf5befb9dc79380380147081d83bb8b7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 13:20:48 +0000 Subject: [PATCH 07/10] =?UTF-8?q?loco:=20the=20bound=20is=20a=20resolved?= =?UTF-8?q?=20body,=20never=20a=20length=20=E2=80=94=20and=20strip=20tool?= =?UTF-8?q?=20names?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from review on #304, both correct. MAJOR — a sparse `Inventory` entry read as a successful run. `branch` bounded on `body_count()` (i.e. `Inventory::len`) while `body()` is what actually resolves, and `run_function` then returned `Ok(())` when nothing came back. So an address inside the reported length with no body behind it passed the bound, resolved to nothing, and the loop iterated doing nothing while the run reported success. The trait's own doc had already said which is the bound — "used only for diagnostics; `body` returning `None` is the real bound" — and the one consumer checked the other thing. That gap is not hypothetical: an inventory over a node store, a Lance scan or a cache answers `len()` from its address space and `body()` from what it can produce, so a hole is the normal case, not a malformed one. Fixed at both ends of the same mistake: - `branch` resolves through `body_at` and reports `UnresolvedBody` when nothing comes back. Address 0 stays refused separately — it is the ENTRY body, and a branch into it is a re-entry the recursion ceiling cannot distinguish from a legitimate cycle. - `run_function(index)` becomes `run_body(body)`: it takes an already-resolved body, so no path through it can fail to find one and silently succeed. - `run` reports the new `RunError::MissingEntryBody` instead of `Ok(())`. This also changes an empty program from "succeeds" to "reports it", which is the same defect one layer out — "there was nothing to run" and "it ran and did nothing" were the same answer. - `body_count()` is gone; nothing else read it. `RunError` is `#[non_exhaustive]`, so the added variant is not breaking. MINOR — removed tool names from four doc comments (`interpret.rs`, `inventory.rs` ×3). This repo's rule is no model identifier in any committed artifact; a review tool's name is the same class. The finding and the PR number stay, which is the part that carries provenance. Two falsifiers, over a `HolePunchedInventory` whose length covers an address it answers `None` for. Each asserts its own premise (the hole is INSIDE the reported length) so it cannot pass as an ordinary out-of-range case, and each carries the paired silent half (fill the hole → runs to 21; an entry that resolves → runs) so an implementation that refuses every inventory branch cannot pass either. 84 tests, clippy -D warnings clean, fmt clean. Disable-verification follows in the next commit's message — committing first on purpose, because the restore is `git checkout` and that reverts to the last commit, not to the state the disable started from. --- crates/ogar-loco/src/interpret.rs | 167 ++++++++++++++++++++++++++---- crates/ogar-loco/src/inventory.rs | 6 +- 2 files changed, 152 insertions(+), 21 deletions(-) diff --git a/crates/ogar-loco/src/interpret.rs b/crates/ogar-loco/src/interpret.rs index 5e69329..8e0dc6f 100644 --- a/crates/ogar-loco/src/interpret.rs +++ b/crates/ogar-loco/src/interpret.rs @@ -102,13 +102,25 @@ pub enum RunError { /// The call whose arity is unknown. call: FnIndex, }, - /// A branch named a function index the program does not contain. + /// A branch named a function index that did not RESOLVE to a body. + /// + /// Not merely "out of range": the bound is whether the backing answers + /// with a body, so a sparse [`Inventory`] with a hole at a legal address + /// lands here too. See [`Interpreter::branch`] for why the two cannot be + /// separate rules. UnresolvedBody { /// The branching call. call: FnIndex, /// The index it named. target: u8, }, + /// [`Interpreter::run`] found no body at the entry address. + /// + /// Distinct from [`RunError::UnresolvedBody`] because there is no + /// branching call to name: nothing asked for this body, the run simply + /// had nowhere to start. Reported rather than returning `Ok(())`, which + /// is what an empty program and an unresolvable entry used to share. + MissingEntryBody, /// A loop ran past [`Interpreter::iteration_cap`]. IterationCap { /// The loop call. @@ -221,7 +233,7 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { /// scan or a cache implements [`Inventory`] and the interpreter branches /// into bodies it has never seen inside a `Program`. /// - /// ⊘ Landed after codex flagged, correctly, that `inventory.rs` shipped a + /// ⊘ Landed after review flagged, correctly, that `inventory.rs` shipped a /// trait no execution path could reach — `Interpreter::new` took only a /// `Program` and both resolution sites went through `program.functions`, so /// the advertised behaviour was unavailable to any caller. Two built ends @@ -242,14 +254,6 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { } } - /// How many addresses the backing can answer — the bound `branch` checks. - fn body_count(&self) -> usize { - match self.inventory { - Some(inv) => inv.len(), - None => self.program.functions.len(), - } - } - /// Replace the recursion ceiling. pub fn with_recursion_depth(mut self, depth: u32) -> Self { self.recursion_depth = depth; @@ -283,15 +287,25 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { } /// Run the program's entry function. + /// + /// Errors with [`RunError::MissingEntryBody`] when the entry address does + /// not resolve. It used to return `Ok(())`, which made "there was nothing + /// to run" indistinguishable from "it ran and did nothing" — and under an + /// [`Inventory`] backing, a body the store could not produce then read as + /// a completed run. pub fn run(&mut self) -> Result<(), RunError> { - self.run_function(0) + match self.body_at(0) { + Some(body) => self.run_body(body), + None => Err(RunError::MissingEntryBody), + } } - /// Run one function body to completion. - fn run_function(&mut self, index: usize) -> Result<(), RunError> { - let Some(body) = self.body_at(index) else { - return Ok(()); - }; + /// Run one already-resolved body to completion. + /// + /// Takes the body rather than an address on purpose: resolution is the + /// caller's to do and to report on, so there is no path through here that + /// can fail to find one and silently succeed. + fn run_body(&mut self, body: &'a FunctionBody) -> Result<(), RunError> { let mut pc = 0usize; while let Some(call) = body.call(pc) { let f = call.function; @@ -402,11 +416,27 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { } /// Recurse into the body a branch names. + /// + /// The bound is **whether a body comes back**, never a length. + /// [`Inventory::len`]'s own doc says so — *"used only for diagnostics; + /// `body` returning `None` is the real bound"* — and this call site used + /// to check the length anyway, which is a contract a backing can satisfy + /// while still having a hole: an [`Inventory`] over a node store or a + /// cache answers `len()` from its address space and `body()` from what it + /// can actually produce. A miss then passed the bound, resolved to + /// nothing, and the run reported success. + /// + /// Address 0 stays refused separately: it is the ENTRY body, and a branch + /// into it is a re-entry the recursion ceiling cannot tell from a + /// legitimate cycle. fn branch(&mut self, call: FnIndex, target: u8) -> Result<(), RunError> { let idx = usize::from(target); - if idx == 0 || idx >= self.body_count() { + if idx == 0 { return Err(RunError::UnresolvedBody { call, target }); } + let Some(body) = self.body_at(idx) else { + return Err(RunError::UnresolvedBody { call, target }); + }; if self.depth >= self.recursion_depth { return Err(RunError::RecursionDepth { call, @@ -414,7 +444,7 @@ impl<'a, V: Vocabulary, D: Dialect> Interpreter<'a, V, D> { }); } self.depth += 1; - let r = self.run_function(idx); + let r = self.run_body(body); // Restored on the error path too: an interpreter a caller inspects // after a failure would otherwise report a depth that never unwound, // and one reused after a caught error would refuse legal programs. @@ -669,6 +699,107 @@ mod tests { ); } + /// An inventory with a HOLE: it reports a length that COVERS an address + /// and answers `None` for that address anyway. + /// + /// Not a contrived shape. It is what a node store, a Lance scan or a + /// cache does whenever a row is absent, evicted, or not yet materialized + /// — the length comes from the address space, the body from what the + /// backing can actually produce. [`Inventory::len`]'s own doc says the + /// two are different questions; this is the test double that makes the + /// difference observable. + struct HolePunchedInventory { + /// One slot per address; `None` is a hole inside the length. + bodies: Vec>, + } + + impl Inventory for HolePunchedInventory { + fn body(&self, addr: FnAddr) -> Option<&FunctionBody> { + self.bodies.get(addr.0 as usize)?.as_ref() + } + + fn len(&self) -> usize { + self.bodies.len() + } + } + + /// FAILS IF: `branch` bounds on a LENGTH instead of on whether a body came + /// back. Address 1 is inside `len()` and resolves to nothing, so the old + /// `idx >= self.body_count()` check passed it, `run_function` found no + /// body and returned `Ok(())`, and the run reported success having + /// executed nothing — three times over, once per `REPEAT` iteration. + /// + /// Two-sided: filling the same hole must run to 21, so an implementation + /// that simply refuses every inventory branch cannot pass. + #[test] + fn a_branch_into_a_hole_is_reported_not_silently_completed() { + let full = step_program(7); + + let holed = HolePunchedInventory { + bodies: vec![Some(full.functions[0]), None], + }; + // The premise, asserted rather than assumed: the hole is INSIDE the + // reported length. Without this the test could pass for an ordinary + // out-of-range address and prove nothing about sparseness. + assert_eq!(holed.len(), 2, "the hole lies inside the reported length"); + assert!(holed.body(FnAddr(1)).is_none(), "and address 1 is the hole"); + + match run_with_inventory(&full, &holed) { + Err(RunError::UnresolvedBody { target: 1, .. }) => {} + Err(other) => panic!("expected UnresolvedBody at address 1, got {other:?}"), + Ok(d) => panic!( + "a branch that resolved to nothing was read as a completed run \ + (var0 = {}, the loop iterated and did nothing)", + d.vars[0] + ), + } + + let filled = HolePunchedInventory { + bodies: vec![Some(full.functions[0]), Some(full.functions[1])], + }; + let d = run_with_inventory(&full, &filled) + .expect("the same shape with the hole filled still runs"); + assert_eq!(d.vars[0], 21, "so the refusal is the hole, not the backing"); + } + + /// FAILS IF: an entry address that does not resolve is read as a finished + /// run. `run` used to be `run_function(0)`, which returned `Ok(())` on a + /// missing body — so "there was nothing to run" and "it ran and did + /// nothing" were the same answer. + /// + /// Both backings are pinned, because the silent `Ok(())` was shared: the + /// inventory path is the one a store can hit in production, and the empty + /// program is the behaviour change a pre-`Inventory` caller would see. + #[test] + fn an_unresolvable_entry_is_reported_not_read_as_a_finished_run() { + let program = step_program(7); + + let no_entry = HolePunchedInventory { + bodies: vec![None, Some(program.functions[1])], + }; + assert_eq!(no_entry.len(), 2, "the entry address is inside the length"); + match run_with_inventory(&program, &no_entry) { + Err(RunError::MissingEntryBody) => {} + Err(other) => panic!("expected MissingEntryBody, got {other:?}"), + Ok(d) => panic!( + "an entry that resolved to nothing was read as a finished run \ + (var0 = {})", + d.vars[0] + ), + } + + let empty = Program { functions: vec![] }; + assert!( + matches!(run(&empty), Err(RunError::MissingEntryBody)), + "a program with no bodies reports it rather than succeeding" + ); + + // The silent half: an entry that DOES resolve still runs, so this + // cannot pass for an implementation that refuses every run. + let d = run(&program).expect("a program with an entry body runs"); + assert_eq!(d.vars[0], 21); + } + /// `sum 1..=n` with `REPEAT`: var0 = total, var1 = counter. fn sum_program(n: u8) -> Program { let entry = FunctionBody::from_calls( diff --git a/crates/ogar-loco/src/inventory.rs b/crates/ogar-loco/src/inventory.rs index 8e02711..4f6c5ce 100644 --- a/crates/ogar-loco/src/inventory.rs +++ b/crates/ogar-loco/src/inventory.rs @@ -117,7 +117,7 @@ impl VecInventory { /// than a wrong branch at run time. /// /// ⊘ The order here is the fix, not the check. The first version pushed to - /// both vectors and converted afterwards, which CodeRabbit caught on + /// both vectors and converted afterwards, which review caught on /// OGAR #304: a caller that catches the panic is then holding an inventory /// with 65 537 entries whose last one no `FnAddr` can name, and `len()` /// counts it. Converting FIRST makes the failure leave nothing behind — @@ -158,7 +158,7 @@ impl FromIterator for VecInventory { /// Panics on more than [`MAX_ADDRESSES`] bodies, exactly as [`push`] does. /// /// ⊘ The first version collected straight into the `Vec` with no check, - /// which codex flagged: it bypassed the bound `push` enforces, so a + /// which review flagged: it bypassed the bound `push` enforces, so a /// 65 537-body iterator produced an inventory whose tail no `FnAddr` can /// name while `len()` still counted it. A silently unaddressable entry is /// worse than a panic — the bound is the address space, not a policy. @@ -245,7 +245,7 @@ mod tests { /// FAILS IF: `FromIterator` collects without the bound `push` enforces. /// - /// Codex flagged exactly this on OGAR #304: the 65 537th body has no + /// Review flagged exactly this on OGAR #304: the 65 537th body has no /// `FnAddr` that can name it, so a silent collect produces an inventory /// whose tail is unreachable while `len()` still counts it. The bound is /// the address space, not a policy, so it panics rather than truncates. From 16c240d97042ba8214a4b93416ea0d59902133e0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 13:22:12 +0000 Subject: [PATCH 08/10] loco: the disable runs, and G2's stale name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Disable-verification for the two falsifiers in 27f55b0, each run against the committed tree and each red under exactly the defect it names: | disable | result | |---|---| | `branch` returns `Ok(())` when `body_at` misses (the old length bound + the old silent callee) | `a_branch_into_a_hole_is_reported_not_silently_completed` FAILED — *"a branch that resolved to nothing was read as a completed run (var0 = 0, the loop iterated and did nothing)"* | | `run` returns `Ok(())` when the entry does not resolve | `an_unresolvable_entry_is_reported_not_read_as_a_finished_run` FAILED — *"an entry that resolved to nothing was read as a finished run (var0 = 0)"* | Two things worth keeping from the runs. The first disable also reddened the PRE-EXISTING `a_branch_to_a_missing_body_is_refused` — which is the corroboration, not a surprise: removing the resolution check removes the out-of-range refusal too, so `body_at` is demonstrably carrying both duties now rather than one. And the second disable failed EXACTLY ONE test, so the two falsifiers are independent — neither is passing on the other's mechanism. Also: G2 named `run_function(index)`, which this arc renamed. The gap it describes (the interpreter recurses, so its state lives in the Rust call stack) is unchanged; only the resolution site moved, and the note says so rather than quietly swapping the name. --- docs/LOCO-ORCHESTRATION-GAP.md | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/docs/LOCO-ORCHESTRATION-GAP.md b/docs/LOCO-ORCHESTRATION-GAP.md index c20bd48..6c0154c 100644 --- a/docs/LOCO-ORCHESTRATION-GAP.md +++ b/docs/LOCO-ORCHESTRATION-GAP.md @@ -131,9 +131,14 @@ Until that exists, `GOTO` stays refused alongside `STOP` / `RETURN` / `BREAK` / ## G2 — an explicit frame stack (the keystone) -**Today** `Interpreter::run_function(index)` RECURSES. The interpreter's state -therefore lives in the Rust call stack, where it cannot be paused, persisted, -or examined. +**Today** `Interpreter::branch` RECURSES into `run_body`. The interpreter's +state therefore lives in the Rust call stack, where it cannot be paused, +persisted, or examined. + +(It was `run_function(index)` until #304 split resolution out of execution — +`run_body` takes an already-resolved body so no path through it can fail to +find one and silently succeed. The recursion this gap is about is unchanged; +only where the address is resolved moved.) **Needed:** `frames: Vec`, walked iteratively. Then the whole interpreter state is `(frames, dialect_state, stack)`. From ef792fcdbf0c0468078dc3049294cae4efbe7722 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 13:39:40 +0000 Subject: [PATCH 09/10] docs: G2 contradicted G1's own spec, and the tool-name sweep stopped at the crate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two misses from the same commit, both found by review, both the same shape as things already fixed elsewhere in this PR. **G2 said the iteration cap becomes the budget.** G1, four sections above, specifies `step_budget` as the shared per-run counter and states explicitly that `iteration_cap` stays per-loop — in a paragraph whose own text rejects that exact reading: *"'the iteration cap already covers it' is wrong."* So I wrote the correction in one section and left the error standing in another. Corrected in place with the old wording quoted, because the failure generalizes and deleting it would hide the instance: **a second section is a second place to be wrong**, and the one that summarizes is the one a reader reaches first. Third time in this session, after the plan's G-F row and the 27-vs-33 file count. **The tool-name sweep was scoped to `crates/ogar-loco` and the violations were in `docs/`.** Two sites, both now neutral. I had written that I "swept the crate rather than patching the cited lines, because a rule like this one fails exactly where nobody looked" — and then looked in one directory. The rule is about committed artifacts, not about a crate. Both files this PR touches are now clean; `docs/ARCHITECTURAL-DECISIONS-2026-06-04.md` carries older instances and is deliberately left alone, since this PR does not touch it and widening a PR to sweep an unrelated file is its own defect. Docs only — no code, no behaviour, 84 tests unaffected. --- docs/LOCO-ORCHESTRATION-GAP.md | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/docs/LOCO-ORCHESTRATION-GAP.md b/docs/LOCO-ORCHESTRATION-GAP.md index 6c0154c..dfdb91b 100644 --- a/docs/LOCO-ORCHESTRATION-GAP.md +++ b/docs/LOCO-ORCHESTRATION-GAP.md @@ -102,7 +102,7 @@ Cost: one arm in `run_branching`, plus a loop-safety question `IF`/`REPEAT` do not have (a `GOTO` can build a cycle the structured ops cannot). **And "the iteration cap already covers it" is wrong — that sentence stood here -and CodeRabbit was right to reject it.** `iteration_cap` is a PER-LOOP ceiling: +and review was right to reject it.** `iteration_cap` is a PER-LOOP ceiling: `REPEAT`, `WHILE` and `REPEAT_UNTIL` each check their own count against it. A `GOTO` cycle contains no loop construct, so it checks nothing and runs forever inside a cap that is never consulted. A per-loop ceiling cannot bound a @@ -169,8 +169,18 @@ Two things this changes that must be re-pinned, not absorbed: rewrite, and it is exactly the machinery `while_reruns_its_condition_span_and_computes_gcd` already falsifies — so the test that guards it exists before the change does. -- **The iteration cap becomes a BUDGET spendable across resumptions.** That is - also how LangGraph-style step limits fall out for free. +- **`step_budget` persists across resumptions; `iteration_cap` stays per-loop.** + A resumed run continues spending the same budget rather than receiving a + fresh one — which is what makes step limits fall out for free, and what + stops suspend-resume-repeat from being an unbounded-execution loophole. + + ⊘ This bullet used to read *"the iteration cap becomes a BUDGET spendable + across resumptions"*, which contradicted G1's spec four sections above — a + spec whose own text rejects exactly that reading (*"'the iteration cap + already covers it' is wrong"*). So the correction was written in one section + and the error left standing in another. **A second section is a second place + to be wrong**, and the one that summarizes is the one a reader reaches + first. ## G3 — a dialect snapshot seam @@ -196,7 +206,7 @@ are — the named half of the state model, already addressable. ### Where those bytes are allowed to live — the boundary, stated -CodeRabbit asked this and the doc did not answer it, which is a real omission +Review asked this and the doc did not answer it, which is a real omission rather than a nit: a byte seam whose destination is unspecified is one review away from becoming a serialization channel. From 1fd71380d6b0d78162b66a81398a2b28c5de1c9b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 16 Sep 2026 13:42:36 +0000 Subject: [PATCH 10/10] loco: the reasoning was in the impls, just not reachable from rustdoc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven trait-impl methods in the two files this PR adds — `nars.rs`'s `Vocabulary` impl and `inventory.rs`'s `Inventory` impl — carried their "why" as inline `//` comments. A consumer reading the rendered docs to decide how to use `NarsVocabulary` or `VecInventory` could not see any of it. Converted, not invented: the text was already there. Scoped deliberately, because a coverage number is easy to satisfy the wrong way. NOT documented: the test-double `Dialect`/`Inventory` impls (their methods are `truthy`, `len`, `call` on fixtures — a doc there is noise), and `Display::fmt` / `Default::default` / other pre-existing items in `lib.rs`, where the docs conventionally live on the trait and the code is not this PR's to widen into. Same reason `docs/ARCHITECTURAL-DECISIONS-2026-06-04.md` was left alone a commit ago. The one worth reading is `VecInventory::len`. It now restates, at the impl site, the exact distinction this PR's MAJOR bug turned on: the trait says `len` is diagnostic and `body` returning `None` is the real bound, and the two agree here ONLY because a `Vec` is dense. A backing over a node store or a cache answers `len` from its address space and `body` from what it can produce — and a consumer that bounds on the number instead of on a resolved body reads a hole as a success. That sentence belongs where someone writing the next `Inventory` impl will actually meet it. 84 tests, clippy -D warnings clean, fmt clean. No behaviour change. --- crates/ogar-loco/src/inventory.rs | 13 +++++++++++++ crates/ogar-loco/src/nars.rs | 24 +++++++++++++++++------- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/crates/ogar-loco/src/inventory.rs b/crates/ogar-loco/src/inventory.rs index 4f6c5ce..32938a6 100644 --- a/crates/ogar-loco/src/inventory.rs +++ b/crates/ogar-loco/src/inventory.rs @@ -177,14 +177,27 @@ impl FromIterator for VecInventory { } impl Inventory for VecInventory { + /// A plain index, because registration order IS the address here. `None` + /// past the end rather than a wrap. fn body(&self, addr: FnAddr) -> Option<&FunctionBody> { self.bodies.get(addr.0 as usize) } + /// `None` for an entry pushed without a key — this backing never mints + /// one, so an unkeyed entry stays unkeyed rather than acquiring a zero key + /// that would collide with every other unminted function. fn key_of(&self, addr: FnAddr) -> Option<[u8; 16]> { self.keys.get(addr.0 as usize).copied().flatten() } + /// How many addresses are registered. + /// + /// **Diagnostic only**, as the trait says — [`Inventory::body`] returning + /// `None` is the real bound. The two happen to agree here only because a + /// `Vec` is dense; a backing over a node store, a Lance scan or a cache + /// answers this from its address space and `body` from what it can + /// actually produce, and a consumer that bounds on this number instead of + /// on a resolved body reads a hole as a success (OGAR #304). fn len(&self) -> usize { self.bodies.len() } diff --git a/crates/ogar-loco/src/nars.rs b/crates/ogar-loco/src/nars.rs index a4f9424..178b4e4 100644 --- a/crates/ogar-loco/src/nars.rs +++ b/crates/ogar-loco/src/nars.rs @@ -133,28 +133,38 @@ pub fn byte_of_id(id: u8) -> Option { pub struct NarsVocabulary; impl Vocabulary for NarsVocabulary { + /// Two operands for a [`Bucket::Datapath`] byte, one for [`Bucket::Gate`] + /// and [`Bucket::Control`]. + /// + /// POLICY, not a measurement — see the module doc. The catalogue records + /// each recipe's bucket, not its arity, so these numbers are this + /// vocabulary's reading of the bucket rather than a fact it carries. fn domain_stack_arity(&self, f: FnIndex) -> Option { - // POLICY, not a measurement — see the module doc. tier_of(f).map(|b| match b { Bucket::Datapath => 2, Bucket::Gate | Bucket::Control => 1, }) } + /// Zero for every recipe, [`Bucket::Control`] included. + /// + /// See "the seam this does NOT close" in the module doc: a body reference + /// here would branch through a path nothing has executed. fn domain_body_refs(&self, _f: FnIndex) -> u8 { - // Zero for every recipe, Control included. See "the seam this does - // NOT close" in the module doc: a body reference here would branch - // through a path nothing has executed. 0 } + /// Every tier answers with something — `Datapath` a mask, `Gate` a + /// marker, `Control` a verdict — so all three push. + /// + /// Declared rather than left unknown so bodies using these bytes are + /// statement-segmentable instead of refused. fn domain_pushes_result(&self, f: FnIndex) -> Option { - // All three tiers answer with something: Datapath a mask, Gate a - // marker, Control a verdict. Declared so bodies using these bytes are - // statement-segmentable rather than refused. tier_of(f).map(|_| true) } + /// The recipe's own `code` from the shared catalogue, so a legend renders + /// the canonical name rather than a byte this crate invented. fn domain_name(&self, f: FnIndex) -> Option<&'static str> { recipe_at(f).map(|r| r.code) }