From 48a7bce04dea37938bf7edb4e3c7984bde3e6961 Mon Sep 17 00:00:00 2001 From: tegwick Date: Sun, 9 Aug 2026 00:54:34 +0200 Subject: [PATCH] CB-WP-0048 T02/T03/T04: Configuration replaces Variant in the state The three-armed enum could not express a configuration carrying two modules. It does now: --variant h1 expands to problem_stress.flat_any_open AND attack_relief.self_soothe_ge4, and scoped_plus_attack_soothe plays. Legacy names alias forever via serde(alias="variant") plus a scalar-or-map deserialiser. serde(default) alone would have been a silent migration bug -- every H1/H2 recording would have come back as baseline. All 26 scenarios pass unchanged; none pins a state hash. Three things shipped broken first, all caught by gates rather than by reading: 1. `profile` was in the state hash. with_config(select("ground-darvo-r0")) sets profile=Some("baseline") where setup alone leaves None, so two states at THE SAME POINT IN ASPECT SPACE hashed differently and a replay bundle stopped reproducing its own initial state. Now serde(skip): a hash covers what determines play. This was the open judgement from T01 and it did not survive contact with the replay path. 2. A YAML parse inside the event loop. rules() -> resolve() -> catalog() re-parsed catalog.yaml per rule check; AM-6 fell to 9,345 events/s against a 100,000 target. OnceLock, and aspect validation moved to where a configuration is BUILT. Then I nearly optimised a phantom: 470k still looked like a 3x regression against the "~1.7M on bnt-lap001" reference in the gate's own message. Making rules() free measured 491k -- this machine's ceiling. Before optimising against a reference, measure the ceiling with the suspect code removed. 3. The refusal did not fire on the path a player takes. `--module problem_deal.pressure_deck` played a full baseline game and reported success, because with_config is a builder and fell back to the printed rules -- the silent no-op ADR-0022 exists to refuse. Every unit test of resolve() passed. The helper was tested and the driver was not, which is CB-WP-0033's finding verbatim. The new test asserts refusal BY NAME and that no game was played. Co-Authored-By: Claude Opus 5 --- crates/cb-render-html/src/doc.rs | 18 ++- crates/cb-render-html/src/lib.rs | 88 +++++++++++++- crates/cb-render-html/src/testfix.rs | 2 +- games/ground/src/catalog.rs | 17 ++- games/ground/src/config.rs | 114 +++++++++++++++--- games/ground/src/lib.rs | 81 +++++++++++-- games/ground/src/view.rs | 13 +- tools/cb-play/src/hotseat.rs | 25 +++- tools/cb-play/src/inspect.rs | 40 +++++- tools/cb-play/src/main.rs | 38 ++++-- tools/cb-play/src/table.rs | 92 +++++++++++++- ...B-WP-0048-a-configuration-not-a-variant.md | 77 +++++++++++- 12 files changed, 538 insertions(+), 67 deletions(-) diff --git a/crates/cb-render-html/src/doc.rs b/crates/cb-render-html/src/doc.rs index d5d3456..6fe6f9d 100644 --- a/crates/cb-render-html/src/doc.rs +++ b/crates/cb-render-html/src/doc.rs @@ -1089,7 +1089,10 @@ pub fn document_with_log( // not, so a player who ran the default and was told the layout had // changed reported "no changes" — correctly, because the baseline // is deliberately unchanged and nothing on screen said so. - variant = esc(view.variant.id()), + // CB-WP-0048: the page names every LIVE MODULE, not one package + // id. A configuration is a point in aspect space and can carry + // two modules at once; `h2` was never able to say that. + variant = esc(&config_label(&view.config)), who = match view.viewer { Some(p) => format!("{} (their hand only)", seat_name(p)), None => "a spectator (no hands)".to_string(), @@ -1572,6 +1575,19 @@ fn stress_scope_rule_text() -> Option { games_ground::edition::stress_scope_rule() } +/// Which modules are live, in words (CB-WP-0048, ADR-0022). +/// +/// The baseline says so by name rather than showing an empty list: "no +/// modules" and "we could not read the catalog" must not look the same +/// on a page a player uses to tell one rules set from another. +fn config_label(c: &games_ground::config::Configuration) -> String { + let live = c.active_modules(); + if live.is_empty() { + return c.baseline.clone(); + } + live.join(" + ") +} + /// The Mode card's printed title, not the Rust variant's name. /// /// Falls back to the id, which is at least a thing the edition says — diff --git a/crates/cb-render-html/src/lib.rs b/crates/cb-render-html/src/lib.rs index d5f92b2..1e5dd37 100644 --- a/crates/cb-render-html/src/lib.rs +++ b/crates/cb-render-html/src/lib.rs @@ -192,7 +192,10 @@ mod coverage { // that could only be satisfied by geometry would have to be // OMITTED, and the page would be illegible to `text_of` and to a // screen reader alike. - ("variant", "ground-darvo-r0"), + // The page shows the LIVE modules, or the baseline id when none + // are — a player is asking "which rules am I playing", not "what + // is every aspect set to". The fixture is the baseline. + ("config.baseline", "ground-darvo-r0"), // The fixture plays SCN_03, so the token is that card's TITLE. // Not "SCN_03": a probe that matches an id would go green // against a page showing the id, which is the machine's name for @@ -211,10 +214,37 @@ mod coverage { ]; /// Deliberate omissions, each with a reason. - const OMITTED: &[(&str, &str)] = &[( - "players.*.hand", - "null for a non-viewer seat; the absence is rendered as a count", - )]; + const OMITTED: &[(&str, &str)] = &[ + ( + "players.*.hand", + "null for a non-viewer seat; the absence is rendered as a count", + ), + // An aspect sitting at its default IS the printed game, and + // naming all four on every page is the wall of rules a player + // stops reading — the same reasoning that keeps the GROUND-mode + // explanation to the moment a mode is being chosen. + // + // **A LIVE module is a different matter and is not omitted**: + // `the_table_names_every_live_module` asserts it by name, so + // this omission covers only the case where there is nothing to + // say. The machine-facing `cb-play inspect` prints all four. + ( + "config.modules.problem_stress", + "shown only when it differs from the printed rules; see the_table_names_every_live_module", + ), + ( + "config.modules.attack_relief", + "shown only when it differs from the printed rules; see the_table_names_every_live_module", + ), + ( + "config.modules.end_condition", + "shown only when it differs from the printed rules; see the_table_names_every_live_module", + ), + ( + "config.modules.problem_deal", + "shown only when it differs from the printed rules; see the_table_names_every_live_module", + ), + ]; fn fixture() -> GroundView { crate::testfix::view(Some(PlayerId(0))) @@ -780,6 +810,52 @@ mod gamelog { } } + /// **The table names every live module** (CB-WP-0048 T04). + /// + /// CB-WP-0044's finding, generalised: a rules change the page cannot + /// name is reported as "no changes". A configuration can now carry + /// two modules at once, so naming one package id would be the wrong + /// shape — `h2` could never have said + /// `problem_stress.scoped + attack_relief.self_soothe_ge4`. + #[test] + fn the_table_names_every_live_module() { + let combo = games_ground::config::Configuration::select("scoped_plus_attack_soothe") + .expect("the catalog ships this profile"); + let mut v = crate::testfix::view(Some(PlayerId(0))); + v.config = combo.clone(); + let text = crate::text_of(&crate::doc::document( + &v, + &[], + "/c", + Some(PlayerId(0)), + false, + )); + for m in combo.active_modules() { + assert!( + text.contains(&m), + "the page does not name the live module {m}" + ); + } + + // And the baseline says so by NAME rather than showing nothing: + // "no modules" and "we could not read the catalog" must not look + // the same on the line a player uses to tell rules sets apart. + let mut base = crate::testfix::view(Some(PlayerId(0))); + base.config = games_ground::config::Configuration::default(); + let plain = crate::text_of(&crate::doc::document( + &base, + &[], + "/c", + Some(PlayerId(0)), + false, + )); + assert!(plain.contains("ground-darvo-r0"), "the baseline is unnamed"); + assert!( + !plain.contains("problem_stress."), + "the baseline page names a module that is not live" + ); + } + /// **A scope says what it does** (CB-WP-0046). /// /// CB-WP-0045 shipped the placement and recorded the gap with the @@ -790,7 +866,7 @@ mod gamelog { #[test] fn a_scoped_table_says_what_a_scope_does() { let mut v = crate::testfix::view(Some(PlayerId(0))); - v.variant = games_ground::Variant::H2ScopedProblemStress; + v.config = games_ground::Variant::H2ScopedProblemStress.configuration(); // One card that is not everyone's — the condition for the rule // to be worth stating at all. if let Some(m) = v.problem_markers.values_mut().next() { diff --git a/crates/cb-render-html/src/testfix.rs b/crates/cb-render-html/src/testfix.rs index ca6d8de..1465096 100644 --- a/crates/cb-render-html/src/testfix.rs +++ b/crates/cb-render-html/src/testfix.rs @@ -83,7 +83,7 @@ pub fn view(viewer: Option) -> GroundView { (Pair::new(p2, p3), Relation::Bond), ]), problems, - variant: games_ground::Variant::Baseline, + config: games_ground::config::Configuration::default(), focus: BTreeMap::from([(p1, p3)]), selections: BTreeMap::from([ ( diff --git a/games/ground/src/catalog.rs b/games/ground/src/catalog.rs index dcec8b7..f3d5d48 100644 --- a/games/ground/src/catalog.rs +++ b/games/ground/src/catalog.rs @@ -113,8 +113,21 @@ pub struct Catalog { /// says so by name rather than returning an empty aspect list. pub const SCHEMA: u32 = 2; -/// Parse the vendored catalog. -pub fn catalog() -> Result { +/// Parse the vendored catalog. **Parsed once.** +/// +/// This returned an owned `Catalog` and parsed the YAML on every call — +/// and `GroundState::rules()` calls it per rule check, inside the event +/// loop. AM-6 throughput fell from ~1.7M events/s to **9,345**, a 180x +/// regression, and the spec gate caught it the same run. +/// +/// The catalog is a compile-time constant (`include_str!`), so parsing +/// it more than once was never doing anything but work. +pub fn catalog() -> Result<&'static Catalog, String> { + static PARSED: std::sync::OnceLock> = std::sync::OnceLock::new(); + PARSED.get_or_init(parse).as_ref().map_err(|e| e.clone()) +} + +fn parse() -> Result { let f: CatalogFile = serde_yaml::from_str(CATALOG_YAML).map_err(|e| format!("catalog.yaml: {e}"))?; if f.schema_version != SCHEMA { diff --git a/games/ground/src/config.rs b/games/ground/src/config.rs index 497f594..0cd4d1b 100644 --- a/games/ground/src/config.rs +++ b/games/ground/src/config.rs @@ -36,20 +36,82 @@ use std::collections::BTreeMap; /// *resolved*: every aspect the catalog declares has an entry, defaults /// filled in, so a recording states the whole point in aspect space /// rather than the delta from an assumed origin. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] pub struct Configuration { /// The content package (ADR-0011), e.g. `ground-darvo-r0`. pub baseline: String, /// aspect id → module id, resolved. pub modules: BTreeMap, - /// The profile that was asked for, if one was. **Kept for the - /// account, never consulted for behaviour** — two runs with the same - /// modules play identically whether or not a profile named them, and - /// a reader still wants to know what was requested. - #[serde(default, skip_serializing_if = "Option::is_none")] + /// The profile that was asked for, if one was. + /// + /// **Kept for the account, and deliberately OUT of the state.** Two + /// runs with the same modules play identically whether or not a + /// profile named them, so a label must not change the state hash. + /// + /// It was serialised at first, and the replay walk caught it within + /// the hour: `with_config(select("ground-darvo-r0"))` sets + /// `profile = Some("baseline")` while `setup` alone leaves it + /// `None`, so two states at **the same point in aspect space** hashed + /// differently and a bundle stopped reproducing its own initial + /// state. A hash should cover what determines play; provenance of + /// what was *requested* belongs in the recording's manifest, which is + /// where a reader can still find it. + #[serde(skip)] pub profile: Option, } +/// **Reads a legacy name as well as a configuration** (ADR-0022 D2). +/// +/// A recording written before this type serialised `variant: Baseline` +/// or a catalog id. Deserialising that as "no configuration" would set +/// every such recording to the baseline — silently right for most of +/// them and silently *wrong* for any played under H1 or H2, which is the +/// one failure a migration must not have. +/// +/// So a scalar is accepted and expanded through the catalog, and a map is +/// read as itself. Writing is always the map: the legacy spelling is a +/// thing we can read, not a thing we produce. +impl<'de> Deserialize<'de> for Configuration { + fn deserialize>(d: D) -> Result { + #[derive(Deserialize)] + #[serde(untagged)] + enum Wire { + Named(String), + Full { + baseline: String, + modules: BTreeMap, + }, + } + match Wire::deserialize(d)? { + Wire::Full { baseline, modules } => Ok(Configuration { + baseline, + modules, + // Not on the wire; see the field's own note. + profile: None, + }), + Wire::Named(name) => Configuration::select_legacy(&name) + .map_err(|e| serde::de::Error::custom(format!("configuration {name:?}: {e}"))), + } + } +} + +impl Configuration { + /// A legacy `variant` value, as an enum name or a catalog id. + /// + /// The Rust names are here because they are what `serde` wrote into + /// recordings; they are **this repo's** spelling of ground-game's + /// ids and have no other home to be read from. + pub fn select_legacy(name: &str) -> Result { + let id = match name { + "Baseline" => "ground-darvo-r0", + "H1ProblemStress" => "h1-problem-stress", + "H2ScopedProblemStress" => "h2-scoped-problem-stress", + other => other, + }; + Configuration::select(id) + } +} + impl Default for Configuration { /// The printed game: every aspect at its default module. /// @@ -119,7 +181,7 @@ impl Configuration { let mut claimed: BTreeMap = BTreeMap::new(); for id in modules { let Some(aspect) = cat.aspect_of(id) else { - return Err(unknown_module(&cat, id)); + return Err(unknown_module(cat, id)); }; if let Some(first) = claimed.get(aspect) { return Err(format!( @@ -130,11 +192,13 @@ impl Configuration { claimed.insert(aspect.to_string(), id.clone()); chosen.insert(aspect.to_string(), id.clone()); } - Ok(Configuration { + let c = Configuration { baseline: baseline.to_string(), modules: chosen, profile: None, - }) + }; + c.validate()?; + Ok(c) } /// Build from a named profile. @@ -212,19 +276,35 @@ impl Configuration { out } + /// Every aspect named is one the catalog declares. + /// + /// Called where a configuration is **built**, not where it is used: + /// it cannot change between rule checks, and keeping it out of + /// `resolve` is what AM-6 required. + pub fn validate(&self) -> Result<(), String> { + let cat = crate::catalog::catalog()?; + for aspect in self.modules.keys() { + if cat.aspect(aspect).is_none() { + return Err(format!("{aspect:?} is not an aspect the catalog declares")); + } + } + Ok(()) + } + /// **Identity → behaviour.** The boundary in ADR-0022 D1. /// /// Refuses, by name and with the reason, any module the catalog has /// and this kernel cannot run. pub fn resolve(&self) -> Result { - let cat = crate::catalog::catalog()?; let mut rules = Rules::default(); - for (aspect, id) in &self.modules { - // An aspect the catalog does not declare is not something to - // guess about. - if cat.aspect(aspect).is_none() { - return Err(format!("{aspect:?} is not an aspect the catalog declares")); - } + for id in self.modules.values() { + // **No catalog lookup in this loop.** `resolve` runs per rule + // check inside the event loop, and validating that each + // aspect is declared meant a linear scan of the catalog per + // call. AM-6 measured the cost: throughput fell to 9,345 + // events/s against a 100,000 target. Whether an aspect exists + // is a property of the CONFIGURATION, checked once where one + // is built (`validate`), not once per rule. match id.as_str() { "problem_stress.none" => rules.problem_stress = ProblemStress::None, "problem_stress.flat_any_open" => rules.problem_stress = ProblemStress::FlatAnyOpen, @@ -239,7 +319,7 @@ impl Configuration { // look", and the `other` arm below can then be a real // refusal rather than a shrug. "end_condition.fixed_rounds_5" | "problem_deal.fixed_setup" => {} - other => return Err(no_kernel_path(&cat, other)), + other => return Err(no_kernel_path(crate::catalog::catalog()?, other)), } } Ok(rules) diff --git a/games/ground/src/lib.rs b/games/ground/src/lib.rs index 5b8bdaa..9acd597 100644 --- a/games/ground/src/lib.rs +++ b/games/ground/src/lib.rs @@ -211,8 +211,20 @@ pub struct GroundState { /// diverge silently, and the recording is what every other artifact /// rests on. `#[serde(default)]` so every scenario written before /// variants existed still loads, as baseline — which is what it was. - #[serde(default)] - pub variant: Variant, + /// Which point in aspect space this game is played at + /// ([ADR-0022](../../../decisions/ADR-0022-a-configuration-is-a-point-in-aspect-space.md)). + /// + /// **Was `variant: Variant`, a three-armed enum** — which could not + /// express `problem_stress.scoped` × `attack_relief.self_soothe_ge4`, + /// a profile the catalog ships. Not "unimplemented": unrepresentable. + /// + /// `alias = "variant"` plus `Configuration`'s scalar-or-map + /// deserialiser means a recording written before this loads by its + /// old name and old spelling. **`serde(default)` alone would have + /// been a silent migration bug**: every recording played under H1 or + /// H2 would have come back as the baseline. + #[serde(default, alias = "variant")] + pub config: crate::config::Configuration, /// Which of the edition's four Scenarios is on the table /// (CB-WP-0047). /// @@ -317,6 +329,16 @@ pub enum Variant { } impl Variant { + /// This legacy name as a **configuration** (ADR-0022 D2). + /// + /// The enum survives as a spelling the maintainer types and every + /// recording carries; it is no longer what the kernel branches on. + /// Expansion goes through the catalog's own `legacy_experiment_id`, + /// so ground-game's mapping has one home. + pub fn configuration(self) -> crate::config::Configuration { + crate::config::Configuration::select(self.id()).unwrap_or_default() + } + /// The catalog's `variant_id`, which is how ground-game names these. pub fn id(self) -> &'static str { match self { @@ -350,14 +372,41 @@ impl GroundState { /// owners at setup, and `state.variant = v` would leave them /// unassigned — a silently wrong game rather than a failing one. /// Every driver path goes through here. - pub fn with_variant(mut self, variant: Variant) -> Self { - self.variant = variant; - if variant == Variant::H2ScopedProblemStress { - self.apply_h2_setup(); + pub fn with_variant(self, variant: Variant) -> Self { + self.with_config(variant.configuration()) + } + + /// Select the configuration, and apply whatever its modules need at + /// setup (ADR-0022 D3). + /// + /// **A builder rather than a bare field write.** + /// `problem_stress.scoped` assigns Problem owners at setup, and + /// `state.config = c` would leave them unassigned — a silently wrong + /// game rather than a failing one. That defect had three call sites + /// under the old enum; with N aspects it is N times as likely, so + /// every driver path goes through here. + pub fn with_config(mut self, config: crate::config::Configuration) -> Self { + let rules = config.resolve(); + self.config = config; + if let Ok(r) = rules { + if r.problem_stress == crate::config::ProblemStress::Scoped { + self.apply_h2_setup(); + } } self } + /// What this game's configuration means for the kernel. + /// + /// **Falls back to the printed rules** when the configuration names + /// something this kernel cannot run. `setup` and `with_config` are + /// where that is refused loudly; by the time a rule is being applied + /// the game is already in progress, and a mid-game panic would turn + /// a selection error into a crash at an unrelated moment. + fn rules(&self) -> crate::config::Rules { + self.config.resolve().unwrap_or_default() + } + /// H2-SCOPE and H2-OWN, applied to the dealt board. /// /// Owners go to the **non-global** Problems in **ascending hidden @@ -1659,7 +1708,9 @@ impl GroundState { // The DARVO extra Attack comes through here too, which the delta // requires: "DARVO-stage extra Attack uses the same Attack // resolution (so it can self-soothe too if Stress >= 4)". - if self.variant == Variant::H1ProblemStress && attacker_stress_before >= 4 { + if self.rules().attack_relief == crate::config::AttackRelief::SelfSootheGe4 + && attacker_stress_before >= 4 + { let stress = self.stress_after(attacker, -1); events.push(GroundEvent::StressSet { player: attacker, @@ -1713,7 +1764,7 @@ impl GroundState { // ticks only the seats in ITS scope, and `stacking: true` — two // open bond Problems hit the network twice, so this accumulates // per Problem rather than applying once. - if self.variant == Variant::H2ScopedProblemStress { + if self.rules().problem_stress == crate::config::ProblemStress::Scoped { let mut ticks: BTreeMap = BTreeMap::new(); for problem in self.problems.values() { if problem.claimed_by.is_some() { @@ -1751,7 +1802,7 @@ impl GroundState { } } - if self.variant == Variant::H1ProblemStress + if self.rules().problem_stress == crate::config::ProblemStress::FlatAnyOpen && self.problems.values().any(|p| p.claimed_by.is_none()) { for seat in self.seat_order() { @@ -2302,7 +2353,7 @@ impl ScenarioGame for GroundState { // before the hash is taken, which is the route `mode` uses // (`table.rs`) — so a recorded session replays under the // variant it was played under. - variant: Variant::default(), + config: crate::config::Configuration::default(), outcome: None, seed, }) @@ -2437,7 +2488,11 @@ mod tests { seed, ) .expect("setup"); - assert_eq!(untouched.variant, Variant::Baseline, "the default moved"); + assert_eq!( + untouched.config, + crate::config::Configuration::default(), + "the default moved" + ); assert_eq!( cb_events::state_hash_hex(&base), cb_events::state_hash_hex(&untouched), @@ -2472,7 +2527,7 @@ mod tests { // The wrong way. let mut bare = base.clone(); - bare.variant = Variant::H2ScopedProblemStress; + bare.config = Variant::H2ScopedProblemStress.configuration(); assert!( bare.problems.values().all(|p| p.scope.is_none()), "a bare write should leave scopes unassigned — if it does not, \ @@ -3501,7 +3556,7 @@ mod tests { support_responses: BTreeMap::new(), darvo_targets: BTreeMap::new(), mode: ScoringMode::SharedGround, - variant: Variant::Baseline, + config: crate::config::Configuration::default(), scenario: default_scenario(), outcome: None, seed: 0, diff --git a/games/ground/src/view.rs b/games/ground/src/view.rs index b8ea886..dbd4ef2 100644 --- a/games/ground/src/view.rs +++ b/games/ground/src/view.rs @@ -46,8 +46,15 @@ pub struct GroundView { /// **The page had no way to say this**, so a player could not tell /// baseline from H1 from H2 by looking — and reported a variant /// change as "no changes" after playing the default. - #[serde(default)] - pub variant: crate::Variant, + /// Which point in aspect space is being played (ADR-0022). + /// + /// **The page had no way to say this**, so a player could not tell + /// baseline from H1 from H2 by looking — and reported a variant + /// change as "no changes" after playing the default (CB-WP-0044). + /// It carries the whole configuration now, not a single name, so a + /// two-module combination is nameable on the table. + #[serde(default, alias = "variant")] + pub config: crate::config::Configuration, /// Which Scenario is on the table (CB-WP-0047). /// /// The player was told the premise by nothing: the same four boards @@ -185,7 +192,7 @@ impl Project for GroundState { lead: self.lead, step: self.step, mode: self.mode, - variant: self.variant, + config: self.config.clone(), scenario: self.scenario.clone(), players: self .players diff --git a/tools/cb-play/src/hotseat.rs b/tools/cb-play/src/hotseat.rs index 4f6160e..7d6d7a3 100644 --- a/tools/cb-play/src/hotseat.rs +++ b/tools/cb-play/src/hotseat.rs @@ -70,6 +70,25 @@ pub struct TrialNote { pub text: String, } +/// A configuration, as one token for a log line (CB-WP-0048). +/// +/// **The live modules, or the baseline id when none are.** The trial log +/// stamps this so a note can be read back knowing which rules produced +/// it (CB-WP-0046) — and a configuration is now a point in aspect space, +/// so a single package name would be the wrong shape for a session +/// carrying two modules. +/// +/// Dots and commas are kept out: the value rides an HTML-comment marker +/// attribute and the parser accepts `[A-Za-z0-9._-]`, so modules are +/// joined with `+`. +pub fn configuration_stamp(c: &games_ground::config::Configuration) -> String { + let live = c.active_modules(); + if live.is_empty() { + return c.baseline.clone(); + } + live.join("+") +} + /// Where game `n`'s recording goes (ADR-0019 D2). /// /// **Game 1 keeps the path it was given.** GameDesign §5 documents that @@ -225,7 +244,7 @@ impl Server { state_hash: hash[..12].to_string(), text: note.text.clone(), }); - write_trial_log(path, &log, state.variant.id()) + write_trial_log(path, &log, &configuration_stamp(&state.config)) } /// Begin the next game (ADR-0019 D1). @@ -1290,7 +1309,7 @@ mod tests { mode: games_ground::ScoringMode::SharedGround, scenario: "SCN_01".into(), pace: crate::table::Pace::Speed, - variant: games_ground::Variant::Baseline, + config: games_ground::config::Configuration::default(), }, std::io::Cursor::new(Vec::new()), out, @@ -1482,7 +1501,7 @@ mod tests { mode: games_ground::ScoringMode::SharedGround, scenario: "SCN_01".into(), pace: crate::table::Pace::Speed, - variant: games_ground::Variant::Baseline, + config: games_ground::config::Configuration::default(), }, std::io::Cursor::new(Vec::new()), out, diff --git a/tools/cb-play/src/inspect.rs b/tools/cb-play/src/inspect.rs index a05da88..41cc418 100644 --- a/tools/cb-play/src/inspect.rs +++ b/tools/cb-play/src/inspect.rs @@ -137,6 +137,18 @@ fn render_player(id: PlayerId, p: &PlayerView, is_viewer: bool) -> String { } /// One seat's whole picture, from the projection and nothing else. +/// Every aspect and its module, in catalog order where available. +fn full_configuration(c: &games_ground::config::Configuration) -> String { + let mut parts = vec![format!("baseline={}", c.baseline)]; + for (aspect, module) in &c.modules { + parts.push(format!("{aspect}={module}")); + } + if let Some(p) = &c.profile { + parts.push(format!("profile={p}")); + } + parts.join(" ") +} + pub fn render(view: &GroundView) -> String { let mut out = String::new(); out.push_str(&format!( @@ -146,7 +158,12 @@ pub fn render(view: &GroundView) -> String { view.step, seat_name(view.lead), view.mode, - view.variant.id(), + // **Every aspect, not only the live ones** (ADR-0022 D3). This is + // the machine-facing view — a maintainer diffing two states — + // and a recording states the whole point in aspect space rather + // than the delta from an assumed origin. The HTML page shows the + // live modules, because a player is asking a different question. + full_configuration(&view.config), // CB-WP-0047: WHICH of the four boards. The id, not the title, // because this is the machine-facing view — `cb-play inspect` is // read by a maintainer diffing states, where the HTML page is @@ -502,8 +519,23 @@ mod tests { ("mode", "mode BondedCoalitions"), // CB-WP-0044: the inspector must say which rules it is replaying, // for the same reason the page must. - ("variant", "rules ground-darvo-r0"), ("scenario", "scenario SCN_"), + // One token per aspect, because the probe expands the map and + // each aspect must be shown to reach the reader on its own. + ("config.baseline", "baseline=ground-darvo-r0"), + ( + "config.modules.problem_stress", + "problem_stress=problem_stress.", + ), + ( + "config.modules.attack_relief", + "attack_relief=attack_relief.", + ), + ( + "config.modules.end_condition", + "end_condition=end_condition.", + ), + ("config.modules.problem_deal", "problem_deal=problem_deal."), ("viewer", "(you)"), ("solution_deck_len", "deck 11"), ("solution_discard.*.suit", "discard [Repair, Change]"), @@ -629,7 +661,7 @@ mod tests { ]), problems, problem_markers: Default::default(), - variant: games_ground::Variant::Baseline, + config: games_ground::config::Configuration::default(), focus: BTreeMap::from([(p1, p3)]), selections: BTreeMap::from([ ( @@ -780,7 +812,7 @@ mod tests { mode: games_ground::ScoringMode::SharedGround, scenario: "SCN_01".into(), pace: crate::table::Pace::Speed, - variant: games_ground::Variant::Baseline, + config: games_ground::config::Configuration::default(), }; let mut sink: Vec = Vec::new(); let summary = diff --git a/tools/cb-play/src/main.rs b/tools/cb-play/src/main.rs index 83a8dd8..fa340cf 100644 --- a/tools/cb-play/src/main.rs +++ b/tools/cb-play/src/main.rs @@ -55,7 +55,9 @@ not deal one, so a seed or a seat count would be silently ignored. /// can mean, and a flag that is accepted and ignored is worse than one /// that is refused. enum Mode { - Play(Config), + // Boxed since CB-WP-0048: `Config` carries a `Configuration`, which + // carries the module map, and the enum is otherwise 26 bytes. + Play(Box), Inspect { source: std::path::PathBuf, eyes: inspect::Eyes, @@ -206,9 +208,25 @@ fn parse_args(argv: &[String]) -> Result { config.scenario = normalise_scenario(&v)?; i += 2; } - "--variant" => { + // CB-WP-0048: legacy names ALIAS FOREVER (ADR-0022 D2). + // 26 recordings name them and the expansion is exact, so + // there is nothing to deprecate. + "--variant" | "--profile" => { play_flags.push(flag.into()); - config.variant = value(i, argv, flag)?.parse()?; + let v = value(i, argv, flag)?; + config.config = games_ground::config::Configuration::select(&v)?; + i += 2; + } + // A module directly, repeatable: one per aspect. + "--module" => { + play_flags.push(flag.into()); + let v = value(i, argv, flag)?; + let mut live = config.config.active_modules(); + live.push(v); + config.config = games_ground::config::Configuration::from_modules( + &config.config.baseline, + &live, + )?; i += 2; } "--pace" => { @@ -285,7 +303,7 @@ fn parse_args(argv: &[String]) -> Result { config.players )); } - Ok(Mode::Play(config)) + Ok(Mode::Play(Box::new(config))) } fn main() { @@ -381,7 +399,7 @@ mod tests { mode: games_ground::ScoringMode::SharedGround, scenario: "SCN_01".into(), pace: table::Pace::Speed, - variant: games_ground::Variant::Baseline, + config: games_ground::config::Configuration::default(), }; let script = "0\n".repeat(400); let mut out: Vec = Vec::new(); @@ -429,7 +447,7 @@ mod tests { mode: games_ground::ScoringMode::SharedGround, scenario: "SCN_01".into(), pace, - variant: games_ground::Variant::Baseline, + config: games_ground::config::Configuration::default(), }; let script = "0\n".repeat(400); let mut out: Vec = Vec::new(); @@ -499,7 +517,7 @@ mod tests { mode: games_ground::ScoringMode::SharedGround, scenario: "SCN_01".into(), pace: table::Pace::Speed, - variant: games_ground::Variant::Baseline, + config: games_ground::config::Configuration::default(), }; let mut out: Vec = Vec::new(); table::play(&config, "0\n".repeat(200).as_bytes(), &mut out).expect("game"); @@ -566,7 +584,7 @@ mod tests { mode: games_ground::ScoringMode::SharedGround, scenario: "SCN_01".into(), pace: table::Pace::Speed, - variant: games_ground::Variant::Baseline, + config: games_ground::config::Configuration::default(), }; let mut out: Vec = Vec::new(); let summary = table::play(&config, "".as_bytes(), &mut out).expect("bot game"); @@ -577,7 +595,7 @@ mod tests { /// flag that quietly switched modes could not pass for a play flag. fn play_args(list: &[&str]) -> Result { match parse_args(&args(list))? { - Mode::Play(c) => Ok(c), + Mode::Play(c) => Ok(*c), Mode::Inspect { .. } => panic!("{list:?} parsed as inspect, not play"), } } @@ -631,7 +649,7 @@ mod tests { mode: games_ground::ScoringMode::SharedGround, scenario: "SCN_01".into(), pace: table::Pace::Speed, - variant: games_ground::Variant::Baseline, + config: games_ground::config::Configuration::default(), }; let mut out: Vec = Vec::new(); let summary = table::play(&config, "".as_bytes(), &mut out).expect("game"); diff --git a/tools/cb-play/src/table.rs b/tools/cb-play/src/table.rs index dfefde6..c7d6368 100644 --- a/tools/cb-play/src/table.rs +++ b/tools/cb-play/src/table.rs @@ -67,7 +67,12 @@ pub struct Config { /// /// **Unlike `pace`, this changes the game** — outcome, state hash and /// recording all move, which is correct: a variant is mechanism. - pub variant: games_ground::Variant, + /// Which point in aspect space to play (ADR-0022). + /// + /// **Replaces `variant: Variant`**, which could not express a + /// configuration carrying two modules. Legacy names still select one + /// (`--variant h2`); `--profile` and `--module` name them directly. + pub config: games_ground::config::Configuration, } /// Speed or Interactive (Ornamentation §4). @@ -113,7 +118,7 @@ impl Default for Config { mode: games_ground::ScoringMode::SharedGround, scenario: "SCN_01".into(), pace: Pace::Speed, - variant: games_ground::Variant::Baseline, + config: games_ground::config::Configuration::default(), } } } @@ -417,10 +422,21 @@ fn run_game<'a, R: BufRead + 'a, W: Write + 'a>( initial.mode = config.mode; // CB-WP-0038: set beside `mode` and before the hash, so a recorded // session replays under the variant it was played under. - // `with_variant`, never a field write: H2 assigns Problem owners at + // `with_config`, never a field write: problem_stress.scoped assigns + // Problem owners at // setup, and the bare write would leave them unassigned — a silently // wrong game rather than a failing one. - let initial = initial.with_variant(config.variant); + // **Refused here, loudly** (ADR-0022 D1, CB-RES-0010 §5.6). + // + // `with_config` cannot return an error — it is a builder — so it + // falls back to the printed rules for a configuration it cannot + // resolve. That is the silent no-op this project calls `inert`, and + // `--module problem_deal.pressure_deck` played a full baseline game + // and reported success. The unit tests all passed: they called + // `resolve()` directly and never the path a player takes, which is + // CB-WP-0033's finding in a new place. + config.config.resolve()?; + let initial = initial.with_config(config.config.clone()); let initial_json = serde_json::to_value(&initial).map_err(|e| e.to_string())?; let initial_hash = cb_events::state_hash_hex(&initial); @@ -557,3 +573,71 @@ fn run_game<'a, R: BufRead + 'a, W: Write + 'a>( end_choice, )) } + +#[cfg(test)] +mod config_refusal_tests { + use super::*; + + /// **A module with no kernel path is refused by the DRIVER** + /// (ADR-0022 D1, CB-WP-0048 T03). + /// + /// Every unit test of `resolve()` passed while + /// `--module problem_deal.pressure_deck` played a full baseline game + /// and reported success: `with_config` is a builder, cannot return an + /// error, and fell back to the printed rules. **The helper was tested + /// and the path a player takes was not** — CB-WP-0033's finding, in a + /// new place. + #[test] + fn a_configuration_the_kernel_cannot_run_stops_the_game() { + let Some(module) = games_ground::catalog::catalog().ok().and_then(|c| { + c.modules.iter().find_map(|m| { + let mut cfg = games_ground::config::Configuration::default(); + cfg.modules.insert(m.aspect.clone(), m.module_id.clone()); + cfg.resolve().is_err().then(|| m.module_id.clone()) + }) + }) else { + // The kernel implements every module the catalog has. + return; + }; + let mut config = Config { + players: 3, + human_seats: vec![], + ..Default::default() + }; + config.config = games_ground::config::Configuration::from_modules( + "ground-darvo-r0", + std::slice::from_ref(&module), + ) + .expect("a catalog module must NAME a configuration"); + + let mut sink: Vec = Vec::new(); + let err = play(&config, "".as_bytes(), &mut sink) + .expect_err("a module with no kernel path must stop the game"); + assert!( + err.contains(&module) && err.contains("no kernel path"), + "refused, but not by name: {err}" + ); + // **And no game was played.** A refusal that still deals is the + // same silent no-op wearing an error message. + let out = String::from_utf8_lossy(&sink); + assert!(!out.contains("game over"), "the game ran anyway:\n{out}"); + } + + /// The baseline and a resolvable combination are NOT refused, or the + /// check above would pass by refusing everything. + #[test] + fn a_resolvable_configuration_still_plays() { + for name in ["ground-darvo-r0", "h2", "scoped_plus_attack_soothe"] { + let mut config = Config { + players: 3, + human_seats: vec![], + ..Default::default() + }; + config.config = games_ground::config::Configuration::select(name) + .unwrap_or_else(|e| panic!("{name}: {e}")); + let mut sink: Vec = Vec::new(); + play(&config, "".as_bytes(), &mut sink) + .unwrap_or_else(|e| panic!("{name} must play: {e}")); + } + } +} diff --git a/workplans/CB-WP-0048-a-configuration-not-a-variant.md b/workplans/CB-WP-0048-a-configuration-not-a-variant.md index 05d8132..7094d2a 100644 --- a/workplans/CB-WP-0048-a-configuration-not-a-variant.md +++ b/workplans/CB-WP-0048-a-configuration-not-a-variant.md @@ -110,7 +110,7 @@ could not express. ```task id: CB-WP-0048-T02 -status: todo +status: done priority: high state_hub_task_id: "3232a451-b60a-42f6-8200-b8115d4b35b9" ``` @@ -127,11 +127,23 @@ Per ADR-0022 D2, through the catalog's own `legacy_experiment_id`. - **the expansion comes from the catalog**, not a table in our source: a second copy of ground-game's mapping is F25's shape. +**Done 2026-08-08.** `GroundState.variant: Variant` became +`config: Configuration`, with `#[serde(alias = "variant")]` plus a +scalar-or-map deserialiser so a recording written before this loads by +its old name and old spelling. **`serde(default)` alone would have been a +silent migration bug**: every recording played under H1 or H2 would have +come back as the baseline. + +All 26 scenarios pass unchanged — none pins a state hash. `--variant h2`, +`--profile h2` and `--module problem_stress.scoped` all resolve to the +same point in aspect space, and `--variant h1` expands to **two** modules +on two aspects, which is the decomposition schema 2 exists for. + ## Task: the resolved configuration is recorded ```task id: CB-WP-0048-T03 -status: todo +status: done priority: high state_hub_task_id: "5be13b74-bcc9-4697-96bb-91f1966c760b" ``` @@ -147,11 +159,58 @@ logs and panel cells carry the **resolved** module list. - **a bare field write cannot leave a module inert** — the H2 defect had three call sites and this is the generalisation of it. +**Done 2026-08-08**, and the interesting part is what shipped broken +first. + +### `profile` was in the hash, and it should not have been + +The replay walk caught it within the hour: `with_config(select("ground-darvo-r0"))` +sets `profile = Some("baseline")` while `setup` alone leaves it `None`, +so **two states at the same point in aspect space hashed differently** and +a bundle stopped reproducing its own initial state. `profile` is now +`#[serde(skip)]`. A state hash should cover what determines play; +provenance of what was *requested* belongs in the recording's manifest. + +This was the open judgement flagged when T01 landed. It did not survive +contact with the replay path, which is the right way for it to have been +settled. + +### A YAML parse inside the event loop + +`rules()` called `resolve()` called `catalog()`, which re-parsed +`catalog.yaml` **per rule check**. AM-6 throughput fell to **9,345 +events/s** against a 100,000 target — a spec gate, caught the same run. +The catalog is an `include_str!` constant, so parsing it more than once +was never doing anything but work; it is a `OnceLock` now, and the aspect +validation moved out of `resolve` to where a configuration is *built*. + +**And then I nearly optimised a phantom.** 470k events/s still looked +like a 3x regression against the *"~1.7M on bnt-lap001"* reference in the +gate's own message. Making `rules()` free and re-measuring gave **491k** +— this machine's ceiling. The reference is another machine, and the only +real regression was the one already fixed. The control is cheap and worth +naming: **before optimising against a reference, measure the ceiling with +the suspect code removed.** + +### The refusal did not fire on the path a player takes + +`--module problem_deal.pressure_deck` **played a full baseline game and +reported success.** `with_config` is a builder, cannot return an error, +and fell back to the printed rules — the silent no-op ADR-0022 exists to +refuse. Every unit test of `resolve()` passed throughout. + +**The helper was tested and the driver was not**, which is exactly +CB-WP-0033's finding: *"the unit test proves the helper; only the +integration test proves the driver, and the driver was where the data +loss lived."* The driver resolves before dealing now, and the new test +asserts both that it refuses **by name** and that **no game was played** — +a refusal that still deals is the same no-op wearing an error message. + ## Task: the page and the panels speak aspects ```task id: CB-WP-0048-T04 -status: todo +status: done priority: normal state_hub_task_id: "ff135f86-2265-4db5-b412-23fcb68df489" ``` @@ -164,6 +223,18 @@ state_hub_task_id: "ff135f86-2265-4db5-b412-23fcb68df489" reconstructs; - **a module nothing measured is reported unmeasured**, not absent. +**Done 2026-08-08.** The page names every live module — +`attack_relief.self_soothe_ge4 + problem_stress.scoped` where `h2` could +only ever have said one thing — and names the baseline by id when none +are live, because "no modules" and "we could not read the catalog" must +not look the same on the line a player uses to tell rules sets apart. + +The four aspects sitting at their defaults are **declared omitted** from +the player page with a reason: an aspect at its default *is* the printed +game, and naming all four on every page is the wall of rules a player +stops reading. `cb-play inspect` prints all four, because a maintainer +diffing two states is asking a different question (ADR-0022 D3). + ## Not done here - **No policy reads the configuration.** F27 already records that the