From 9ffd1952d2a85c1129550100f9313f13e83da24e Mon Sep 17 00:00:00 2001 From: claude Date: Sun, 12 Jul 2026 23:42:10 +0200 Subject: [PATCH] feat(core,engine): bound params become data-merged defaults; Composite::reopen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 1-2 of the bound-override cycle: PrimitiveBuilder::bind/try_bind stop capturing the bound value in a wrapped build closure and store it only as BoundParam data; build() merges bound values back into the full-arity vector at call time (ascending original position — behaviour-identical to the retired closure chain). This makes the reversal possible: unbind(slot) returns the param to the declared surface at its original order. aura-engine gains the path-addressed Composite::reopen (mirroring collect_params' prefix rules, lockstep with expansion_map) plus the read-only bound_param_space() enumeration (path-qualified BoundSpec with values) and the ReopenError/BoundSpec exports. An e2e proves a reopened param rebuilds from the freshly supplied value through compile_with_params, not the stale default. refs #246 --- crates/aura-core/src/node.rs | 116 +++++++-- crates/aura-engine/src/blueprint.rs | 229 ++++++++++++++++++ crates/aura-engine/src/lib.rs | 4 +- .../aura-engine/tests/reopen_rebuild_e2e.rs | 103 ++++++++ 4 files changed, 425 insertions(+), 27 deletions(-) create mode 100644 crates/aura-engine/tests/reopen_rebuild_e2e.rs diff --git a/crates/aura-core/src/node.rs b/crates/aura-core/src/node.rs index 842a741..520712e 100644 --- a/crates/aura-core/src/node.rs +++ b/crates/aura-core/src/node.rs @@ -84,10 +84,13 @@ pub fn zip_params(space: &[ParamSpec], point: &[Cell]) -> Vec<(String, Scalar)> space.iter().zip(point).map(|(ps, c)| (ps.name.clone(), Scalar::from_cell(ps.kind, *c))).collect() } -/// One param bound to a structural constant by [`PrimitiveBuilder::bind`] — a -/// render/debug surface only (C23), dropped at lowering. `pos` is the slot's -/// position in the node's ORIGINAL (pre-bind) param list, so the model serializer -/// and viewer can place it back into slot order regardless of bind sequence. +/// One param bound to a structural constant by [`PrimitiveBuilder::bind`] — the +/// render/debug surface (C23) AND, since #246, what [`PrimitiveBuilder::build`] +/// merges back into the full-arity vector the pristine build closure expects +/// (the value it holds is the default supplied at bind time). `pos` is the +/// slot's position in the node's ORIGINAL (pre-bind) param list, so the model +/// serializer, the viewer, and `build`'s merge can all place it back into slot +/// order regardless of bind sequence. #[derive(Clone, Debug, PartialEq, serde::Serialize, serde::Deserialize)] pub struct BoundParam { pub pos: usize, @@ -187,11 +190,26 @@ impl PrimitiveBuilder { pub fn params(&self) -> &[ParamSpec] { &self.schema.params } - /// Build a sized node from its param slice — tag-free [`Cell`]s, already - /// kind-checked against the declared slots by the caller (the param-plane - /// frontend), so the closure reads each by its own declared kind. + /// Build a sized node from its OPEN param slice — tag-free [`Cell`]s, + /// already kind-checked against the declared slots by the caller (the + /// param-plane frontend). The pristine build closure always expects the + /// FULL declared arity, so any bound values are merged back in first, at + /// their original slot positions, before the closure runs. pub fn build(&self, params: &[Cell]) -> Box { - (self.build)(params) + if self.bound.is_empty() { + return (self.build)(params); + } + // Merge the bound values back into the full-arity vector the pristine + // closure expects. Inserting in ascending ORIGINAL position reconstructs + // the pre-bind order exactly as the retired per-bind closure chain did + // (each wrap inserted at its own pre-bind coordinate). + let mut full = params.to_vec(); + let mut by_pos: Vec<&BoundParam> = self.bound.iter().collect(); + by_pos.sort_unstable_by_key(|b| b.pos); + for b in by_pos { + full.insert(b.pos, b.value.cell()); + } + (self.build)(&full) } /// The param-generic render label for the blueprint view (C22): just the node /// type, e.g. `SMA`. @@ -229,25 +247,47 @@ impl PrimitiveBuilder { self.schema.params[pos].kind, "bind: kind mismatch for param `{slot}`", ); - // [render side] record the bound slot for the model/viewer at its ORIGINAL - // pre-bind position. `pos` indexes the already-shrunk array (earlier binds - // removed), so map it back to the full-arity index by adding one for each - // earlier-bound slot at an original position <= it. Render/debug only (C23); - // the closure capture below is what bootstrap actually reads. + // record the bound slot at its ORIGINAL pre-bind position. `pos` indexes + // the already-shrunk array (earlier binds removed), so map it back to the + // full-arity index by adding one for each earlier-bound slot at an + // original position <= it. Load-bearing since #246: `build` merges these + // values back into the full-arity vector (and the model/viewer still + // reads them for render). let name = self.schema.params[pos].name.clone(); let kind = self.schema.params[pos].kind; let orig = self.original_pos(pos); self.bound.push(BoundParam { pos: orig, name, kind, value }); self.schema.params.remove(pos); // [param_space side] shrink the declared surface - let inner = self.build; // [value side] wrap the build closure - self.build = Box::new(move |open: &[Cell]| { - let mut full = open.to_vec(); - full.insert(pos, value.cell()); // bound value (kind-checked above) → Cell, at its original slot - inner(&full) - }); self } + /// Reverse a [`bind`](Self::bind): re-open the bound param named `slot`, + /// returning it to the declared surface (`params()` / `schema().params` / + /// the aggregated `param_space`) at the position its original slot + /// dictates. The formerly bound value is discarded by this call — a + /// re-opened param is an ordinary open param again (#246: a bound value is + /// a default; callers that need it back reload the authored document). + /// Panics if `slot` is not currently bound. Unlike `bind` (which guards + /// against a genuinely ambiguous *externally-supplied* `schema.params`), + /// `self.bound` can never hold two entries sharing a name — `bind`/ + /// `try_bind` only ever record a slot after it uniquely matched an OPEN + /// param name, so no defensive "ambiguous" branch is needed here. + /// `&mut self` rather than fluent: the engine's `reopen` walk operates in + /// place. + pub fn unbind(&mut self, slot: &str) { + let idx = self + .bound + .iter() + .position(|b| b.name == slot) + .unwrap_or_else(|| panic!("unbind: no bound param named `{slot}`")); + let b = self.bound.remove(idx); + // current-coordinates insert position: the original slot minus one for + // each still-bound slot at an earlier original position (the inverse + // of the reconstruction `original_pos` performs). + let cur = b.pos - self.bound.iter().filter(|x| x.pos < b.pos).count(); + self.schema.params.insert(cur, ParamSpec { name: b.name, kind: b.kind }); + } + /// The fallible twin of [`bind`](Self::bind): same exactly-one-match + kind /// rule, returning [`BindOpError`] instead of panicking. Used by the op-script /// construction surface, where a bad param is rejected at the op, not a panic. @@ -277,12 +317,6 @@ impl PrimitiveBuilder { let orig = self.original_pos(pos); self.bound.push(BoundParam { pos: orig, name, kind, value }); self.schema.params.remove(pos); - let inner = self.build; - self.build = Box::new(move |open: &[Cell]| { - let mut full = open.to_vec(); - full.insert(pos, value.cell()); - inner(&full) - }); Ok(self) } } @@ -637,6 +671,38 @@ mod tests { let _ = b.bind("length", Scalar::f64(2.0)); // F64 value for an I64 slot } + /// unbind reverses bind: the slot returns to the declared surface at its + /// original order, the BoundParam entry disappears, and a subsequent build + /// receives the re-opened value from the open cells again. + #[test] + fn unbind_reopens_the_slot_at_its_original_position() { + // bind b then a, then unbind b: a stays bound, b re-opens before c. + let mut b = probe3().bind("b", Scalar::i64(8)).bind("a", Scalar::i64(7)); + b.unbind("b"); + assert_eq!( + b.params().iter().map(|p| p.name.as_str()).collect::>(), + ["b", "c"], // b back at its original slot order; a still bound + ); + assert_eq!( + b.bound_params(), + [BoundParam { pos: 0, name: "a".into(), kind: ScalarKind::I64, value: Scalar::i64(7) }] + .as_slice(), + ); + // build after unbind ≡ the never-bound build over the same full cells + let built = b.build(&[Cell::from_i64(8), Cell::from_f64(9.0)]); // inject b, c + assert_eq!( + built.label(), + format!("{:?}", vec![Cell::from_i64(7), Cell::from_i64(8), Cell::from_f64(9.0)]), + ); + } + + #[test] + #[should_panic(expected = "no bound param named")] + fn unbind_unknown_slot_panics() { + let mut b = probe3().bind("b", Scalar::i64(8)); + b.unbind("a"); // a is open, not bound + } + #[test] fn try_bind_ok_and_typed_errors() { use super::BindOpError; diff --git a/crates/aura-engine/src/blueprint.rs b/crates/aura-engine/src/blueprint.rs index 76540e9..a7cd5a7 100644 --- a/crates/aura-engine/src/blueprint.rs +++ b/crates/aura-engine/src/blueprint.rs @@ -254,6 +254,33 @@ impl Composite { out } + /// Re-open ONE bound param at its `param_space()`-style path (#246): the + /// param returns to the open surface at the slot `collect_params` order + /// dictates; its bound value is forgotten by this value (the authored + /// document still carries it — per-use callers reload). Path coordinates + /// are this composite's own frame (a top-level leaf is `.`; + /// an interior composite prefixes its name), exactly `collect_params`' + /// prefix rules. Errors on zero matches and on more than one; a partial + /// mutation before an `Ambiguous` error is unobservable (self is consumed + /// and dropped on Err). + pub fn reopen(mut self, path: &str) -> Result { + match reopen_in(&mut self.nodes, path) { + 1 => Ok(self), + 0 => Err(ReopenError::NoSuchBoundParam(path.to_string())), + _ => Err(ReopenError::Ambiguous(path.to_string())), + } + } + + /// The aggregated, flat, path-qualified BOUND param surface (#246): one + /// entry per bound param, qualified exactly like `param_space()` (same + /// walk shape, same prefix rules), each carrying its bound value. Gang + /// addresses never appear here (gangs gang open params). + pub fn bound_param_space(&self) -> Vec { + let mut out = Vec::new(); + collect_bound(&self.nodes, "", &mut out); + out + } + /// Compile this composite as the ROOT graph from a VALIDATED cell point — the /// construction base (the cell side of the param-plane split). Structural /// validation runs here (wiring, root roles, arity), but NOT the per-value kind @@ -411,6 +438,27 @@ pub enum BindError { Compile(CompileError), } +/// A bound-param re-open (#246) failed: the path names no bound param, or +/// names more than one. Boundary-facing (the CLI derives override sets and +/// rejects at the family boundary); engine-direct callers match on it. +#[derive(Debug, PartialEq, Eq)] +pub enum ReopenError { + /// No bound param lives at this `param_space()`-style path. + NoSuchBoundParam(String), + /// More than one bound param matches this path (duplicate names). + Ambiguous(String), +} + +/// One entry of the aggregated BOUND param surface (#246): the path-qualified +/// twin of [`ParamSpec`] carrying the bound value — the default a sweep axis +/// may override, and what `--list-axes` renders as `default=`. +#[derive(Debug, Clone, PartialEq)] +pub struct BoundSpec { + pub name: String, + pub kind: ScalarKind, + pub value: Scalar, +} + /// A fluent accumulator of named knob bindings for a single run, terminated by /// [`Binder::bootstrap`]. Bindings are resolved against `param_space()` once, at /// the terminal. @@ -944,6 +992,70 @@ fn collect_params(items: &[BlueprintNode], gangs: &[Gang], prefix: &str, out: &m } } +/// Recursive mutable walk for `Composite::reopen` (#246): mirrors +/// `collect_params`' prefix rules (lockstep with `expansion_map`) — a leaf owns +/// `.`, a composite prefixes its `name()` and recurses. Returns +/// the number of bound params matching `path` across the whole frame; exactly +/// one match has been unbound when 1 is returned (a first match is unbound +/// eagerly; on >1 the caller discards the composite, so the partial mutation +/// never escapes). +fn reopen_in(items: &mut [BlueprintNode], path: &str) -> usize { + let mut hits = 0usize; + for item in items.iter_mut() { + match item { + BlueprintNode::Primitive(b) => { + let node = b.node_name(); + if let Some(rest) = path.strip_prefix(&format!("{node}.")) { + let k = b.bound_params().iter().filter(|p| p.name == rest).count(); + if k == 1 && hits == 0 { + b.unbind(rest); + } + hits += k; + } + } + BlueprintNode::Composite(c) => { + if let Some(rest) = path.strip_prefix(&format!("{}.", c.name())) { + hits += reopen_in(&mut c.nodes, rest); + } + } + } + } + hits +} + +/// Read-only twin of `collect_params` over the BOUND surface (#246): same +/// recursion shape and prefix rules, but enumerating `bound_params()` (with +/// values) instead of the open `params()`. Gangs are irrelevant here — a gang +/// members' params are open by construction. +fn collect_bound(items: &[BlueprintNode], prefix: &str, out: &mut Vec) { + for item in items { + match item { + BlueprintNode::Primitive(b) => { + let node = if prefix.is_empty() { + b.node_name() + } else { + format!("{prefix}.{}", b.node_name()) + }; + for p in b.bound_params() { + out.push(BoundSpec { + name: format!("{node}.{}", p.name), + kind: p.kind, + value: p.value, + }); + } + } + BlueprintNode::Composite(c) => { + let child = if prefix.is_empty() { + c.name().to_string() + } else { + format!("{prefix}.{}", c.name()) + }; + collect_bound(c.nodes(), &child, out); + } + } + } +} + /// For each raw open param slot (the `lower_items` cursor order), the index of /// the public-space entry (`param_space()`/`collect_params` order) that supplies /// its cell (#61 task 2). Un-ganged slots consume the next public index in @@ -2926,6 +3038,123 @@ mod tests { assert_eq!(names, ["exp.scale"]); } + /// #246: reopen returns a bound knob to param_space at collect_params + /// order — the bound value becomes a default an axis may override. + #[test] + fn reopen_returns_a_bound_knob_to_param_space() { + use aura_std::{Bias, Sma}; + let strat = Composite::new( + "sma2_entry", + vec![ + Sma::builder().named("bias").bind("length", Scalar::i64(2)).into(), + Bias::builder().named("exp").into(), + ], + vec![], + vec![], + vec![], + ); + let strat = strat.reopen("bias.length").expect("bound knob re-opens"); + let space = strat.param_space(); + let names: Vec<&str> = space.iter().map(|p| p.name.as_str()).collect(); + assert_eq!(names, ["bias.length", "exp.scale"]); + } + + /// #246: a path naming no bound param (here: an OPEN param) is a named + /// error, not a silent no-op — open axes pass through without reopen. + #[test] + fn reopen_unknown_or_open_path_errors() { + use aura_std::{Bias, Sma}; + let strat = Composite::new( + "sma2_entry", + vec![ + Sma::builder().named("bias").bind("length", Scalar::i64(2)).into(), + Bias::builder().named("exp").into(), + ], + vec![], + vec![], + vec![], + ); + match strat.reopen("exp.scale") { + Err(ReopenError::NoSuchBoundParam(p)) => assert_eq!(p, "exp.scale"), + other => panic!("expected NoSuchBoundParam, got {:?}", other.map(|_| "Ok(composite)")), + } + } + + /// #246: bound_param_space mirrors param_space's path qualification and + /// carries the bound value (the default the CLI renders in --list-axes). + #[test] + fn bound_param_space_is_path_qualified_with_values() { + use aura_std::{Bias, Sma}; + let strat = Composite::new( + "sma2_entry", + vec![ + Sma::builder().named("fast").bind("length", Scalar::i64(2)).into(), + Bias::builder().named("exp").bind("scale", Scalar::f64(0.5)).into(), + ], + vec![], + vec![], + vec![], + ); + let bound = strat.bound_param_space(); + let view: Vec<(&str, ScalarKind, Scalar)> = + bound.iter().map(|b| (b.name.as_str(), b.kind, b.value)).collect(); + assert_eq!( + view, + [ + ("fast.length", ScalarKind::I64, Scalar::i64(2)), + ("exp.scale", ScalarKind::F64, Scalar::f64(0.5)), + ] + ); + } + + /// #246: reopen resolves through composite nesting with the same prefix + /// rules as collect_params (composite name prefixes, leaf owns + /// `.`). Construction mirrors + /// `param_space_mirrors_compiled_flat_node_param_order_under_nesting`. + #[test] + fn reopen_addresses_a_nested_composite_path() { + use aura_std::Sma; + let inner = Composite::new( + "inner", + vec![Sma::builder().named("sma").bind("length", Scalar::i64(3)).into()], + vec![], + vec![], + vec![], + ); + let outer = Composite::new("outer", vec![inner.into()], vec![], vec![], vec![]); + let outer = outer.reopen("inner.sma.length").expect("nested path resolves"); + let space = outer.param_space(); + assert_eq!(space[0].name, "inner.sma.length"); + assert_eq!(space[0].kind, ScalarKind::I64); + } + + /// #246: two same-named nodes each carrying a same-named bound param is a + /// duplicate the open-space injectivity check (which only ever sees the + /// OPEN surface) cannot see — this is `reopen`'s `Ambiguous` branch's + /// whole reason to exist. Also pins the "partial unbind before an + /// `Ambiguous` error is unobservable" invariant: the first `bias.length` + /// is unbound in place before the second match is found, but `self` is + /// consumed and dropped on `Err`, so no caller ever observes the + /// half-mutated composite. + #[test] + fn reopen_duplicate_bound_name_across_sibling_nodes_is_ambiguous() { + use aura_std::Sma; + let strat = Composite::new( + "dup", + vec![ + Sma::builder().named("bias").bind("length", Scalar::i64(2)).into(), + Sma::builder().named("bias").bind("length", Scalar::i64(3)).into(), + ], + vec![], + vec![], + vec![], + ); + match strat.reopen("bias.length") { + Err(ReopenError::Ambiguous(p)) => assert_eq!(p, "bias.length"), + other => panic!("expected Ambiguous, got {:?}", other.map(|_| "Ok(composite)")), + } + } + /// E2E (issue #34): the C23/#31 mirror invariant *under composite nesting*. /// `param_space()` (via `collect_params`) duplicates `lower_items`' depth- /// first traversal rather than sharing it, so the two orders must stay in diff --git a/crates/aura-engine/src/lib.rs b/crates/aura-engine/src/lib.rs index 4741cc9..000f21d 100644 --- a/crates/aura-engine/src/lib.rs +++ b/crates/aura-engine/src/lib.rs @@ -54,8 +54,8 @@ mod sweep; mod walkforward; pub use blueprint::{ - BindError, Binder, BlueprintNode, CompileError, Composite, Gang, GangFault, GangMember, - OutField, RandomBinder, Role, SweepBinder, + BindError, Binder, BlueprintNode, BoundSpec, CompileError, Composite, Gang, GangFault, + GangMember, OutField, RandomBinder, ReopenError, Role, SweepBinder, }; pub use blueprint_serde::{ blueprint_from_json, blueprint_identity_json, blueprint_to_json, BlueprintDoc, CompositeData, diff --git a/crates/aura-engine/tests/reopen_rebuild_e2e.rs b/crates/aura-engine/tests/reopen_rebuild_e2e.rs new file mode 100644 index 0000000..2d25cfa --- /dev/null +++ b/crates/aura-engine/tests/reopen_rebuild_e2e.rs @@ -0,0 +1,103 @@ +//! End-to-end coverage for #246 tasks 1-2: `PrimitiveBuilder::bind`/`unbind` +//! storing the bound value as data merged back at build time (aura-core), and +//! `Composite::reopen` re-opening a bound param at its `param_space()` path +//! (aura-engine). The in-crate unit tests (`node.rs`/`blueprint.rs` `mod +//! tests`) already pin the mechanism directly against `PrimitiveBuilder`; this +//! file instead drives the **full public compile pipeline** +//! (`Composite::reopen` -> `compile_with_params` -> `FlatGraph`) through only +//! `aura_engine`'s exported surface — the seam the future CLI axis-override +//! path (the rest of #246) will call. A regression that broke the merge only +//! when routed through `compile_with_params` (as opposed to calling +//! `PrimitiveBuilder::build` directly) would pass every existing unit test +//! while breaking this. + +use aura_core::{Scalar, ScalarKind}; +use aura_engine::{Composite, Role, Target}; +use aura_std::{Bias, Sma}; + +/// A single bound `Sma`, wired from a root role so it compiles standalone. +fn one_bound_sma() -> Composite { + Composite::new( + "sig", + vec![Sma::builder().named("sma").bind("length", Scalar::i64(2)).into()], + vec![], + vec![Role { + name: "series".into(), + targets: vec![Target { node: 0, slot: 0 }], + source: Some(ScalarKind::F64), + }], + vec![], + ) +} + +/// Property: a bound param, once reopened, is genuinely re-bindable through +/// the full `compile_with_params` pipeline — the compiled node reflects the +/// value supplied AT COMPILE TIME, not the stale bound default. This is the +/// build-time merge (`PrimitiveBuilder::build`, aura-core #246 task 1) proven +/// under the compile boundary it must survive (aura-engine #246 task 2), not +/// just called directly. +#[test] +fn reopened_param_rebuilds_with_the_freshly_supplied_value_not_the_stale_default() { + // Before reopen: fully bound, nothing to supply, and the default (2) is + // what gets built. (A fresh `Composite` — `reopen`/`compile_with_params` + // both consume `self`, so the "before" and "after" instances are built + // separately from the same constructor rather than cloned.) + let flat = one_bound_sma().compile_with_params(&[]).expect("fully-bound composite compiles with no params"); + assert_eq!(flat.nodes[0].label(), "SMA(2)", "the bound default builds unchanged"); + + let reopened = one_bound_sma().reopen("sma.length").expect("bound knob reopens"); + let space = reopened.param_space(); + assert_eq!(space.len(), 1, "reopen returns exactly the one knob to the open surface"); + assert_eq!(space[0].name, "sma.length"); + + let flat2 = reopened + .compile_with_params(&[Scalar::i64(9)]) + .expect("reopened composite compiles once the freed slot is supplied"); + assert_eq!( + flat2.nodes[0].label(), + "SMA(9)", + "the compiled node must be built from the freshly supplied value (9), not the stale bound default (2)" + ); +} + +/// Property: reopening one node's bound param does not disturb a SIBLING +/// node's own still-bound default. `BoundParam`'s per-node bookkeeping +/// (`pos`, restored on `unbind`) must stay node-local through the compile +/// pipeline — a bug that let one node's reopen leak into another's merge +/// would build the sibling from the wrong cell. +#[test] +fn reopening_one_node_leaves_a_sibling_bound_default_intact_through_compile() { + let signal = Composite::new( + "sig", + vec![ + Sma::builder().named("fast").bind("length", Scalar::i64(2)).into(), + Bias::builder().named("b").bind("scale", Scalar::f64(0.5)).into(), + ], + vec![], + vec![ + Role { + name: "series".into(), + targets: vec![Target { node: 0, slot: 0 }], + source: Some(ScalarKind::F64), + }, + Role { + name: "signal".into(), + targets: vec![Target { node: 1, slot: 0 }], + source: Some(ScalarKind::F64), + }, + ], + vec![], + ); + + let reopened = signal.reopen("fast.length").expect("fast.length reopens"); + let flat = reopened + .compile_with_params(&[Scalar::i64(7)]) + .expect("compiles once the one freed slot is supplied"); + + assert_eq!(flat.nodes[0].label(), "SMA(7)", "the reopened node builds from the freshly supplied value"); + assert_eq!( + flat.nodes[1].label(), + "Bias(0.5)", + "the sibling's still-bound default must build unchanged — reopen must not perturb it" + ); +}