From ab385e6507e2618584072fd905979466f94e2130 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 18 Jun 2026 07:54:48 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(odoo):=20address=20codex=20review=20on?= =?UTF-8?q?=20merged=20#532/#533=20=E2=80=94=20selection=20replace/extend/?= =?UTF-8?q?poison=20+=20mro=20rustfmt?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two codex P2 findings on merged #532 were real semantic bugs (live in main), both producing a WRONG `ASSERT $value IN [...]` — the silent-data-rejection class that matters for faithful DDL — plus a rustfmt miss on merged #533. # #532 finding 1 — selection_add must not close a dynamic domain A `Selection(selection_add=[('x','X')])` extension onto a base that is DYNAMIC (`Selection(STATE_CONST)` / `selection='_compute_x'`) was emitting `['x']` as if it were the complete closed set, so the projected `ASSERT $value IN ['x']` would reject every legal base value. # #532 finding 2 — full redeclarations must replace, not union The cross-class merge unioned EVERY `fields.Selection` declaration, so an `_inherit` class that REDECLARES the whole field with `selection=[...]` (replace/reorder) left stale keys from the earlier definition in the set. # The correct three-way semantics `_extract_selection(call) -> (base_dynamic, base_keys, add_keys)`: - static `selection=` → REPLACE (a redeclaration drops earlier keys) - `selection_add=` → EXTEND (unions across classes) - provided-but-dynamic → POISON (sentinel `None`; never closed; sticky) A closed `ASSERT IN` is emitted only when the whole domain is statically known. Poison is recorded as `None` in the `selections` map and skipped at emission (`selection_skip_open_domain` stat). # #533 — mro.rs rustfmt `mro.rs` landed un-`cargo fmt`'d (codex flagged the diff). Reformatted; 7/7 mro tests still pass. # Tests python3 -m unittest tests.test_spo_enrich : 61/61 (was 57) + test_dynamic_base_plus_add_stays_poisoned (finding 1) + test_full_redeclaration_replaces_not_unions (finding 2) + test_open_domain_none_emits_nothing + rewrote extraction tests for the (dynamic, base, add) contract cargo test -p lance-graph-ontology --lib mro : 7/7 rustfmt --check mro.rs : clean EPIPHANIES: E-ODOO-SELECTION-REPLACE-EXTEND-POISON. --- .claude/board/EPIPHANIES.md | 6 + .../src/odoo_blueprint/mro.rs | 23 ++- .../odoo_blueprint_extractor/spo_enrich.py | 142 ++++++++++------ .../tests/test_spo_enrich.py | 154 ++++++++++++------ 4 files changed, 215 insertions(+), 110 deletions(-) diff --git a/.claude/board/EPIPHANIES.md b/.claude/board/EPIPHANIES.md index cd9e9a2d..735d3c9e 100644 --- a/.claude/board/EPIPHANIES.md +++ b/.claude/board/EPIPHANIES.md @@ -9,6 +9,12 @@ **Why it matters beyond the bug:** the F32-17D tenant is the **single bit-exact ground-truth** the rest of the SoA migration (S3 driver-flip, future quantization tiers bf16/i4) measures against — every step can diff against it. Alternatives (bf16/f16-17D) are deferred; bf16 is integer-exact only to 256, f16 to 2048, and neither is f32-bit-exact, so they could not satisfy the bit-exact tests as the anchor. **Tests:** singleton `--features with-engine` 6/6 (the 3 un-ignored + 2 + the C8 corner corpus `codebook_index ∈ {0,255,256,1234,4095,65535}`); mailbox-arm `--features with-engine,mailbox-thoughtspace` 5/5 (headline+energy bit-exact via the f32 tenant; non-headline top_k=0 by design, C5/D-DIST-5 preserved); default lib 20/20 (cfg field absent — no hot-path change). fmt clean; my code clippy-clean. +## 2026-06-18 — E-ODOO-SELECTION-REPLACE-EXTEND-POISON — `selection=` closes, `selection_add=` extends, a dynamic base poisons; conflating them produces wrong `ASSERT IN` + +**Status:** FINDING (codex review on merged #532; fixed forward). The first `selection_value` cut conflated two Odoo idioms into one flat value list, producing two ways to emit a **wrong** `ASSERT $value IN [...]` — the exact silent-data-rejection class that matters for "Odoo as faithful DDL": (1) a static `selection_add=` onto a **dynamic** base (`Selection(STATE_CONST)` / `selection='_compute_x'`) wrongly *closed* an open domain; (2) the cross-class merge unioned **every** declaration, so an `_inherit` class that *redeclares* `selection=[…]` left stale keys from the replaced set. + +**The correct three-way semantics** (`spo_enrich._extract_selection` → `(base_dynamic, base_keys, add_keys)`): a full static `selection=` **REPLACES** the domain (a redeclaration drops earlier keys); `selection_add=` **EXTENDS** (unions across classes); a provided-but-unresolvable base **POISONS** the field — recorded as a sentinel `None` in the `selections` map and **never** emitted as a closed set, sticky across classes. The `ASSERT IN` is only emitted when the *whole* domain is statically known. 61 python tests (was 57); the two findings have paired regression tests (`test_dynamic_base_plus_add_stays_poisoned`, `test_full_redeclaration_replaces_not_unions`). Also: `mro.rs` was landed un-`cargo fmt`'d on #533 — fixed in the same follow-up. General lesson: a "merge across classes" on a field whose declaration can either *replace* or *extend* must distinguish the two, and an open/dynamic domain must never be silently closed. + ## 2026-06-18 — E-ODOO-VIRTUALLY-OVERRIDES-COMPUTED — the wishlist's last item is a *derived* ClassView relation, not harvest predicate #6 **Status:** FINDING. `virtually_overrides` — the one open odoo-rs wishlist item — lands as a **computed** Core/ClassView capability (`odoo_blueprint::mro`), closing the wishlist consistently with the Core-first correction (E-ODOO-CORE-FIRST-STRUCTURAL) directly above. Per the doctrine the `(has_function / inherits_from / virtually_overrides)` triad is the ClassView method-resolution manifest: `has_function` + `inherits_from` are *facts*; `virtually_overrides` is the **derivation** (`M.m` overrides `B.m` iff `M` declares `m`, `B` is up `M`'s `_inherit` chain, and `B` also declares `m`). Adding a `virtually_overrides` AST pass to `spo_enrich.py` would have been the drift; computing it from the manifest is the Core-correct move. diff --git a/crates/lance-graph-ontology/src/odoo_blueprint/mro.rs b/crates/lance-graph-ontology/src/odoo_blueprint/mro.rs index 0c358f77..b0b5546e 100644 --- a/crates/lance-graph-ontology/src/odoo_blueprint/mro.rs +++ b/crates/lance-graph-ontology/src/odoo_blueprint/mro.rs @@ -105,12 +105,8 @@ fn nearest_base_declaring( seen.insert(start.to_string()); // Frontier holds (model) at increasing chain distance; a BTreeSet per level // gives the sorted tie-break, then feeds the next level. - let mut frontier: VecDeque = bases_of - .get(start) - .into_iter() - .flatten() - .cloned() - .collect(); + let mut frontier: VecDeque = + bases_of.get(start).into_iter().flatten().cloned().collect(); while let Some(node) = frontier.pop_front() { if !seen.insert(node.clone()) { @@ -220,11 +216,7 @@ mod tests { #[test] fn nearest_base_wins_over_farther_base() { // child → mid → far ; all declare _x. Override targets `mid` (nearest). - let m = methods(&[ - ("child", &["_x"]), - ("mid", &["_x"]), - ("far", &["_x"]), - ]); + let m = methods(&[("child", &["_x"]), ("mid", &["_x"]), ("far", &["_x"])]); let b = bases(&[("child", &["mid"]), ("mid", &["far"])]); let ov = resolve_overrides(&m, &b); assert_eq!(ov.len(), 2, "child→mid and mid→far both resolve"); @@ -249,7 +241,9 @@ mod tests { let m = methods(&[("a", &["_x"]), ("b", &["_x"])]); let b = bases(&[("a", &["b"]), ("b", &["a"])]); let ov = resolve_overrides(&m, &b); // must not hang - assert!(ov.iter().any(|o| o.child_model == "a" && o.base_model == "b")); + assert!(ov + .iter() + .any(|o| o.child_model == "a" && o.base_model == "b")); } #[test] @@ -279,7 +273,10 @@ mod tests { // non-trivial and the call is total, not that overrides exist. let (m, b) = manifest_from_curated_core(); assert!(!m.is_empty(), "curated core declares methods"); - assert!(b.contains_key("account_move"), "account_move has _inherit bases"); + assert!( + b.contains_key("account_move"), + "account_move has _inherit bases" + ); let _ = resolve_overrides(&m, &b); // terminates, deterministic } } diff --git a/tools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.py b/tools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.py index 4fe598d9..f5e83312 100644 --- a/tools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.py +++ b/tools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.py @@ -72,14 +72,20 @@ Enables the consumer to lower a Selection field to `DEFINE FIELD state … ASSERT $value IN ['draft','posted','cancel']`. - Both the base list (positional arg 0 or `selection=`) AND a - `_inherit` addon's `selection_add=[…]` extension are read and unioned — - an extension's keys MERGE onto the field's accumulated set across - classes, so the `ASSERT` set includes legal extension-added states (codex - #530). Dynamic selections (`selection='_compute_states'`, a bare Name - constant, or `related=`) are skipped — their values aren't statically - knowable. Source order is preserved. Selection fields bind to the same - `model_names` as relational fields (`_name`, else `_inherit[0]`). + The two Odoo idioms are kept distinct (codex #532 follow-ups): + + - a full static `selection=[…]` (positional or kwarg) **REPLACES** the + domain — a later redeclaration drops the earlier keys, not unions them; + - an `_inherit` addon's `selection_add=[…]` **EXTENDS** it — its keys + union onto the field's accumulated set across classes, so the `ASSERT` + includes legal extension-added states; + - a **dynamic** base (`selection='_compute_states'`, a bare Name constant, + `related=`) **POISONS** the field — its domain is open and is **never** + emitted as a closed `ASSERT IN`, even if a `selection_add` is statically + known. Poison is sticky across classes. + + Source order is preserved within a domain. Selection fields bind to the + same `model_names` as relational fields (`_name`, else `_inherit[0]`). This is *in addition* to the existing shallow relation read `(_compute_amount, reads_field, odoo:account_move.line_ids)`. The deep @@ -182,34 +188,45 @@ def _selection_tuple_keys(node: Optional[ast.expr], out: List[str], seen: Set[st out.append(key) -def _extract_selection_values(call: ast.Call) -> List[str]: - """Pull the value KEYS from a `fields.Selection(...)` declaration. +def _extract_selection(call: ast.Call) -> "Tuple[bool, List[str], List[str]]": + """Resolve a `fields.Selection(...)` into `(base_dynamic, base_keys, add_keys)`. - Reads BOTH the base list — first positional arg OR `selection=` kwarg — - AND the `selection_add=` kwarg (the Odoo idiom for an `_inherit` addon - that *extends* an existing Selection field's domain without redeclaring - it). Keys from both are unioned, base first, in source order, de-duped. + Distinguishes the two Odoo idioms a single flat list conflated (codex #530 + follow-ups): - Returns `[]` when neither source is a static list of 2-tuples — a dynamic - `selection='_compute_x'` (str), a bare Name constant, or `related=`. - A pure `selection_add=` extension (no base list) yields just the added - keys, which the caller MERGES onto the field's accumulated set. + - `base_dynamic` — a base `selection=`/positional arg was **provided** but is + not a static list of 2-tuples (a `'_compute_x'` method-ref, a bare `Name` + constant, `related=`). The field's value domain is then **open** and + cannot be closed into an `ASSERT $value IN [...]`. This is distinct from + *no base arg at all* (a pure `selection_add=` extension). + - `base_keys` — a static base list. A full `selection=[...]` **REPLACES** the + domain (so a later redeclaration drops the earlier keys, not unions them). + Empty when there is no static base. + - `add_keys` — `selection_add=[...]` keys. An `_inherit` addon **EXTENDS** + the domain (unions onto whatever the field accumulates across classes). """ - base: Optional[ast.expr] = None - add: Optional[ast.expr] = None + base_node: Optional[ast.expr] = None + base_provided = False + add_node: Optional[ast.expr] = None for kw in call.keywords: if kw.arg == "selection": - base = kw.value + base_node = kw.value + base_provided = True elif kw.arg == "selection_add": - add = kw.value - if base is None and call.args: - base = call.args[0] + add_node = kw.value + if base_node is None and call.args: + base_node = call.args[0] + base_provided = True + + base_static = isinstance(base_node, (ast.List, ast.Tuple)) + base_dynamic = base_provided and not base_static - out: List[str] = [] - seen: Set[str] = set() - _selection_tuple_keys(base, out, seen) - _selection_tuple_keys(add, out, seen) - return out + base_keys: List[str] = [] + add_keys: List[str] = [] + if base_static: + _selection_tuple_keys(base_node, base_keys, set()) + _selection_tuple_keys(add_node, add_keys, set()) + return base_dynamic, base_keys, add_keys def _is_uniqueness_call(node: ast.expr) -> bool: @@ -318,7 +335,7 @@ def _scan_file( relmap: Dict[Tuple[str, str], Tuple[str, Optional[str]]], inherits: Optional[Dict[str, Set[str]]] = None, constrains: Optional[Dict[Tuple[str, str], Set[str]]] = None, - selections: Optional[Dict[Tuple[str, str], List[str]]] = None, + selections: Optional[Dict[Tuple[str, str], Optional[List[str]]]] = None, ) -> None: """Parse one .py file; record every relational field on every named model. Optionally also record `inherits_from` edges and `@api.constrains` @@ -359,7 +376,12 @@ def _scan_file( inherits_models: List[str] = [] # _inherits dict keys (delegation) local_fields: Dict[str, Tuple[str, Optional[str]]] = {} local_constrains: List[Tuple[str, ast.FunctionDef]] = [] - local_selections: Dict[str, List[str]] = {} + # Selection domains split by idiom (codex #530 follow-ups): a static + # `selection=` REPLACES, `selection_add=` EXTENDS, a dynamic base + # POISONS (open domain, never closable into ASSERT IN). + local_sel_replace: Dict[str, List[str]] = {} + local_sel_extend: Dict[str, List[str]] = {} + local_sel_poison: Set[str] = set() for stmt in node.body: # Method definitions — gather @api.constrains methods for P2. @@ -409,11 +431,17 @@ def _scan_file( ): continue field_type = stmt.value.func.attr - # Selection field — extract statically-known value keys (P3). + # Selection field — split into REPLACE / EXTEND / POISON (P3). if field_type == "Selection" and selections is not None: - vals = _extract_selection_values(stmt.value) - if vals: - local_selections[target_name] = vals + base_dynamic, base_keys, add_keys = _extract_selection(stmt.value) + if base_dynamic: + # A provided-but-unresolvable base = open domain: do not + # let any `selection_add` masquerade as a closed set. + local_sel_poison.add(target_name) + if base_keys: + local_sel_replace[target_name] = base_keys + if add_keys: + local_sel_extend[target_name] = add_keys continue if field_type not in RELATIONAL_KINDS: continue @@ -464,14 +492,26 @@ def _scan_file( # comodel for a given (model, field) is stable in practice. relmap[(mu, field_name)] = (comodel, inverse) if selections is not None: - for field_name, vals in local_selections.items(): - # MERGE, not overwrite: the base `selection=` declaration - # and any `_inherit` addon's `selection_add=` extensions - # live in DIFFERENT classes, so a given (model, field) - # accumulates across scans (codex #530). Order-preserving, - # de-duplicated union. + # Sentinel `None` value = POISONED (open domain): a dynamic + # base anywhere means the field can never be a closed + # `ASSERT IN [...]`. Poison is STICKY and wins over any keys. + for field_name in local_sel_poison: + selections[(mu, field_name)] = None + # A full static `selection=` REPLACES the domain (drops the + # earlier declaration's stale keys — codex #532 finding #2), + # unless the field is already poisoned. + for field_name, keys in local_sel_replace.items(): + if selections.get((mu, field_name), 0) is None: + continue + selections[(mu, field_name)] = list(keys) + # `selection_add=` EXTENDS (unions onto the accumulated set), + # unless poisoned (codex #532 finding #1 — never close a + # dynamic-base domain from selection_add alone). + for field_name, keys in local_sel_extend.items(): + if selections.get((mu, field_name), 0) is None: + continue acc = selections.setdefault((mu, field_name), []) - for v in vals: + for v in keys: if v not in acc: acc.append(v) @@ -508,7 +548,7 @@ def build_all_facts( Dict[Tuple[str, str], Tuple[str, Optional[str]]], Dict[str, Set[str]], Dict[Tuple[str, str], Set[str]], - Dict[Tuple[str, str], List[str]], + Dict[Tuple[str, str], Optional[List[str]]], ]: """Single-pass scan that populates (relmap, inherits, constrains, selections). @@ -586,7 +626,7 @@ def enrich( relmap: Dict[Tuple[str, str], Tuple[str, Optional[str]]], inherits: Optional[Dict[str, Set[str]]] = None, constrains: Optional[Dict[Tuple[str, str], Set[str]]] = None, - selections: Optional[Dict[Tuple[str, str], List[str]]] = None, + selections: Optional[Dict[Tuple[str, str], Optional[List[str]]]] = None, ) -> Tuple[List[str], dict]: """Compute the additive enrichment lines + a stats dict. @@ -665,6 +705,7 @@ def enrich( "validation_skip_unknown_method": 0, "selection_value": 0, "selection_skip_unknown_model": 0, + "selection_skip_open_domain": 0, } # ── P1: target / inverse_name ───────────────────────────────────────── @@ -786,8 +827,15 @@ def enrich( # One triple per statically-known enum key on a Selection field. Scoped to # corpus-declared ObjectTypes (the additive boundary): never invent a # selection on an unknown model. Value order preserved (source order). + # A `None` value is a POISONED (open) domain — a dynamic base means the + # field is not statically closable, so emit nothing (codex #532 finding #1). if selections: - for (model_us, field_name), vals in sorted(selections.items()): + for (model_us, field_name), vals in sorted( + selections.items(), key=lambda kv: kv[0] + ): + if vals is None: + stats["selection_skip_open_domain"] += 1 + continue if model_us not in object_type_models: stats["selection_skip_unknown_model"] += 1 continue @@ -815,7 +863,9 @@ def run(corpus_path: str, addons_root: str, out_path: str) -> dict: stats["relmap_entries"] = len(relmap) stats["inherits_entries"] = sum(len(v) for v in inherits.values()) stats["constrains_entries"] = sum(len(v) for v in constrains.values()) - stats["selections_entries"] = sum(len(v) for v in selections.values()) + stats["selections_entries"] = sum( + len(v) for v in selections.values() if v is not None + ) stats["new_triples"] = len(new_lines) # Preserve the original corpus lines verbatim, then append the sorted new diff --git a/tools/odoo-blueprint-extractor/tests/test_spo_enrich.py b/tools/odoo-blueprint-extractor/tests/test_spo_enrich.py index 0ba9b268..9ecd2739 100644 --- a/tools/odoo-blueprint-extractor/tests/test_spo_enrich.py +++ b/tools/odoo-blueprint-extractor/tests/test_spo_enrich.py @@ -613,74 +613,84 @@ def _scan_sel(src): def _classify_sel(src): - """Classify a single synthetic `fields.Selection(...)` call.""" + """Resolve a single synthetic `fields.Selection(...)` → + `(base_dynamic, base_keys, add_keys)`.""" import ast as _ast - from odoo_blueprint_extractor.spo_enrich import _extract_selection_values + from odoo_blueprint_extractor.spo_enrich import _extract_selection tree = _ast.parse(src) call = next(n for n in _ast.walk(tree) if isinstance(n, _ast.Call)) - return _extract_selection_values(call) + return _extract_selection(call) class TestSelectionExtraction(unittest.TestCase): - def test_list_of_tuples_positional(self): - vals = _classify_sel( - "fields.Selection([('draft','Draft'),('posted','Posted'),('cancel','Cancel')])" + def test_static_list_positional_is_base(self): + self.assertEqual( + _classify_sel( + "fields.Selection([('draft','Draft'),('posted','Posted'),('cancel','Cancel')])" + ), + (False, ["draft", "posted", "cancel"], []), ) - self.assertEqual(vals, ["draft", "posted", "cancel"]) - def test_selection_kwarg(self): - vals = _classify_sel( - "fields.Selection(selection=[('a','A'),('b','B')], string='X')" + def test_selection_kwarg_is_base(self): + self.assertEqual( + _classify_sel("fields.Selection(selection=[('a','A'),('b','B')], string='X')"), + (False, ["a", "b"], []), ) - self.assertEqual(vals, ["a", "b"]) - def test_dynamic_method_name_skipped(self): - # selection='_compute_states' is a compute method ref → no static values. - vals = _classify_sel("fields.Selection(selection='_compute_states')") - self.assertEqual(vals, []) + def test_dynamic_method_name_is_base_dynamic(self): + # selection='_compute_states' = provided-but-unresolvable → POISON signal. + self.assertEqual( + _classify_sel("fields.Selection(selection='_compute_states')"), (True, [], []) + ) - def test_bare_name_constant_skipped(self): - # selection=SOME_CONSTANT — not statically resolvable from this AST. - vals = _classify_sel("fields.Selection(STATE_VALUES)") - self.assertEqual(vals, []) + def test_bare_name_constant_is_base_dynamic(self): + self.assertEqual(_classify_sel("fields.Selection(STATE_VALUES)"), (True, [], [])) - def test_dedup_preserves_first_order(self): - vals = _classify_sel( - "fields.Selection([('a','A'),('b','B'),('a','A2')])" - ) - self.assertEqual(vals, ["a", "b"]) + def test_no_selection_arg_is_not_dynamic(self): + # No selection arg at all is NOT a provided-but-dynamic base. + self.assertEqual(_classify_sel("fields.Selection(string='X')"), (False, [], [])) + + def test_dedup_within_base(self): + _, base, _ = _classify_sel("fields.Selection([('a','A'),('b','B'),('a','A2')])") + self.assertEqual(base, ["a", "b"]) def test_non_tuple_entries_skipped(self): - # A malformed entry (not a 2-tuple) is skipped, others survive. - vals = _classify_sel("fields.Selection([('a','A'), 'bogus', ('b','B')])") - self.assertEqual(vals, ["a", "b"]) + _, base, _ = _classify_sel("fields.Selection([('a','A'), 'bogus', ('b','B')])") + self.assertEqual(base, ["a", "b"]) - # codex #530 — selection_add (the _inherit extension idiom) - def test_selection_add_kwarg_alone(self): - # A pure extension: `selection_add=` with no base list → the added keys. - vals = _classify_sel( - "fields.Selection(selection_add=[('reviewed','Reviewed'),('void','Void')])" + # codex #530 — selection_add is EXTEND, kept separate from base + def test_selection_add_alone_is_extend(self): + self.assertEqual( + _classify_sel( + "fields.Selection(selection_add=[('reviewed','Reviewed'),('void','Void')])" + ), + (False, [], ["reviewed", "void"]), ) - self.assertEqual(vals, ["reviewed", "void"]) - def test_selection_base_plus_add_union_base_first(self): - vals = _classify_sel( - "fields.Selection(selection=[('draft','Draft'),('posted','Posted')], " - "selection_add=[('reviewed','Reviewed')])" + def test_base_and_add_kept_separate(self): + self.assertEqual( + _classify_sel( + "fields.Selection(selection=[('draft','Draft'),('posted','Posted')], " + "selection_add=[('reviewed','Reviewed')])" + ), + (False, ["draft", "posted"], ["reviewed"]), ) - self.assertEqual(vals, ["draft", "posted", "reviewed"]) - def test_selection_add_dedups_against_base(self): - vals = _classify_sel( - "fields.Selection(selection=[('a','A')], selection_add=[('a','A2'),('b','B')])" + # codex #532 #1 — dynamic base + add must flag dynamic (caller poisons) + def test_dynamic_base_with_add_flags_dynamic(self): + dyn, base, add = _classify_sel( + "fields.Selection(STATE_CONST, selection_add=[('reviewed','Reviewed')])" ) - self.assertEqual(vals, ["a", "b"]) + self.assertTrue(dyn, "provided-but-dynamic base must flag poison") + self.assertEqual(base, []) + self.assertEqual(add, ["reviewed"]) # parsed, but caller drops under poison class TestSelectionScan(unittest.TestCase): - """`_scan_file` binds Selection values to model_names (per #525 rule).""" + """`_scan_file` binds Selection domains: static REPLACE, add EXTEND, + dynamic POISON (sentinel `None`).""" - def test_selection_bound_to_name_model(self): + def test_static_base_bound_to_name_model(self): sel = _scan_sel( "class M:\n" " _name = 'account.move'\n" @@ -688,7 +698,7 @@ def test_selection_bound_to_name_model(self): ) self.assertEqual(sel, {("account_move", "state"): ["draft", "posted"]}) - def test_selection_on_inherit_only_binds_to_inherit_zero(self): + def test_static_base_on_inherit_only_binds_inherit_zero(self): sel = _scan_sel( "class Ext:\n" " _inherit = ['account.move', 'mail.thread']\n" @@ -696,19 +706,19 @@ def test_selection_on_inherit_only_binds_to_inherit_zero(self): ) self.assertEqual(sel, {("account_move", "kind"): ["x", "y"]}) - def test_dynamic_selection_field_records_nothing(self): + def test_dynamic_base_poisons_field_to_none(self): + # codex #532 #1: a dynamic base records the field as OPEN (None), + # so enrich emits no closed `ASSERT IN` for it. sel = _scan_sel( "class M:\n" " _name = 'account.move'\n" " s = fields.Selection(selection='_compute_s')\n" ) - self.assertEqual(sel, {}) + self.assertEqual(sel, {("account_move", "s"): None}) def test_selection_add_merges_across_classes(self): - # codex #530: base declaration in the _name class + a selection_add - # extension in a separate _inherit class must ACCUMULATE, not - # overwrite — else the ASSERT $value IN [...] rejects legal added - # states. + # codex #530: base in the _name class + a selection_add extension in a + # separate _inherit class must ACCUMULATE, not overwrite. sel = _scan_sel( "class Base:\n" " _name = 'account.move'\n" @@ -722,6 +732,38 @@ def test_selection_add_merges_across_classes(self): sel, {("account_move", "state"): ["draft", "posted", "reviewed"]} ) + def test_dynamic_base_plus_add_stays_poisoned(self): + # codex #532 #1: a dynamic base in one class POISONS the domain even if + # another class statically adds — never close an open domain. + sel = _scan_sel( + "class Base:\n" + " _name = 'account.move'\n" + " state = fields.Selection(STATE_CONST)\n" + "\n" + "class Ext:\n" + " _inherit = 'account.move'\n" + " state = fields.Selection(selection_add=[('reviewed','Reviewed')])\n" + ) + self.assertEqual(sel, {("account_move", "state"): None}) + + def test_full_redeclaration_replaces_not_unions(self): + # codex #532 #2: a full static `selection=` redeclaration REPLACES the + # domain; stale keys from the earlier declaration must not survive. + sel = _scan_sel( + "class Base:\n" + " _name = 'account.move'\n" + " state = fields.Selection([('draft','Draft'),('posted','Posted')])\n" + "\n" + "class Ext:\n" + " _inherit = 'account.move'\n" + " state = fields.Selection([('draft','Draft'),('open','Open')])\n" + ) + keys = sel[("account_move", "state")] + self.assertIn("open", keys) + self.assertNotIn( + "posted", keys, "stale key from the replaced declaration must be dropped" + ) + class TestP3SelectionValueEmission(unittest.TestCase): def _triples(self, field_iri="odoo:account_move.state"): @@ -763,6 +805,16 @@ def test_dedup_against_existing(self): # 'draft' already present → only 'posted' is new. self.assertEqual(stats["selection_value"], 1) + def test_open_domain_none_emits_nothing(self): + # codex #532 #1: a poisoned (None) domain → no selection_value triples. + triples = [t("odoo:account_move", "rdf:type", "ogit:ObjectType")] + lines, stats = enrich( + triples, RELMAP, selections={("account_move", "state"): None} + ) + self.assertEqual(stats["selection_value"], 0) + self.assertEqual(stats["selection_skip_open_domain"], 1) + self.assertEqual(lines, []) + if __name__ == "__main__": unittest.main(verbosity=2) From 618d1c6429df59c8f6d45beff222a99202c0672b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 18 Jun 2026 08:09:57 +0000 Subject: [PATCH 2/2] fix(odoo-spo): make selection domains scan-order-independent (codex #536) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third codex P2 finding on this PR: my replace/extend/poison fix collapsed the three into a single accumulated list DURING the scan, so under glob.iglob's unordered file walk a `selection_add=` extension scanned BEFORE the base `selection=[...]` was clobbered by the REPLACE assignment — losing the legal added key, so the emitted `ASSERT $value IN [...]` would reject it. Fix: track replace / extend / poison SEPARATELY per `(model, field)` for the whole scan, collapse only at the end. - `_new_sel_acc()` — per-field record `{replace, extend, poison}`. - `_scan_file` accumulates into it: poison sets the flag; a static `selection=` sets `replace` (last full decl wins, drops stale keys); `selection_add=` unions into `extend` and is NEVER overwritten by a replace. - `_resolve_selections()` (run once at the end of `build_all_facts`) collapses to the final domain: `None` if poisoned, else `replace ++ extend` de-duped, base first. Order-independent. `enrich` is unchanged — it still receives the resolved `Optional[List[str]]` map. `_scan_sel` test helper resolves before asserting (mirrors `build_all_facts`). Tests (61 -> 62): + `test_selection_add_before_base_is_order_independent` (the EXTENSION class declared BEFORE the base; resolved domain still [draft, posted, reviewed] regardless of order). All prior selection tests still green. python3 -m unittest tests.test_spo_enrich : 62/62 EPIPHANIES E-ODOO-SELECTION-REPLACE-EXTEND-POISON updated with the order-independence refinement. --- .claude/board/EPIPHANIES.md | 2 +- .../odoo_blueprint_extractor/spo_enrich.py | 83 ++++++++++++++----- .../tests/test_spo_enrich.py | 30 +++++-- 3 files changed, 88 insertions(+), 27 deletions(-) diff --git a/.claude/board/EPIPHANIES.md b/.claude/board/EPIPHANIES.md index 735d3c9e..21633761 100644 --- a/.claude/board/EPIPHANIES.md +++ b/.claude/board/EPIPHANIES.md @@ -13,7 +13,7 @@ **Status:** FINDING (codex review on merged #532; fixed forward). The first `selection_value` cut conflated two Odoo idioms into one flat value list, producing two ways to emit a **wrong** `ASSERT $value IN [...]` — the exact silent-data-rejection class that matters for "Odoo as faithful DDL": (1) a static `selection_add=` onto a **dynamic** base (`Selection(STATE_CONST)` / `selection='_compute_x'`) wrongly *closed* an open domain; (2) the cross-class merge unioned **every** declaration, so an `_inherit` class that *redeclares* `selection=[…]` left stale keys from the replaced set. -**The correct three-way semantics** (`spo_enrich._extract_selection` → `(base_dynamic, base_keys, add_keys)`): a full static `selection=` **REPLACES** the domain (a redeclaration drops earlier keys); `selection_add=` **EXTENDS** (unions across classes); a provided-but-unresolvable base **POISONS** the field — recorded as a sentinel `None` in the `selections` map and **never** emitted as a closed set, sticky across classes. The `ASSERT IN` is only emitted when the *whole* domain is statically known. 61 python tests (was 57); the two findings have paired regression tests (`test_dynamic_base_plus_add_stays_poisoned`, `test_full_redeclaration_replaces_not_unions`). Also: `mro.rs` was landed un-`cargo fmt`'d on #533 — fixed in the same follow-up. General lesson: a "merge across classes" on a field whose declaration can either *replace* or *extend* must distinguish the two, and an open/dynamic domain must never be silently closed. +**The correct three-way semantics** (`spo_enrich._extract_selection` → `(base_dynamic, base_keys, add_keys)`): a full static `selection=` **REPLACES** the domain (a redeclaration drops earlier keys); `selection_add=` **EXTENDS** (unions across classes); a provided-but-unresolvable base **POISONS** the field (never emitted as a closed set, sticky). **Critically, replace/extend/poison must be tracked SEPARATELY through the whole scan and collapsed only at the end** (`_new_sel_acc` per field + `_resolve_selections` in `build_all_facts`) — a third codex finding (#536 follow-up) caught that the first fix collapsed them per-class, so under `glob.iglob`'s unordered walk a `selection_add` scanned *before* the base `selection=` was clobbered by the REPLACE and the legal added key was lost. Collapsing-during-scan is order-dependent; collapsing-at-resolve is not. The `ASSERT IN` is only emitted when the *whole* domain is statically known. 62 python tests (was 57); three findings have paired regression tests (`test_dynamic_base_plus_add_stays_poisoned`, `test_full_redeclaration_replaces_not_unions`, `test_selection_add_before_base_is_order_independent`). Also: `mro.rs` was landed un-`cargo fmt`'d on #533 — fixed in the same follow-up. General lesson: a "merge across classes" on a field whose declaration can either *replace* or *extend* must (a) distinguish the two, (b) keep their state separate until a final resolve so the result is scan-order-independent, and (c) never silently close an open/dynamic domain. ## 2026-06-18 — E-ODOO-VIRTUALLY-OVERRIDES-COMPUTED — the wishlist's last item is a *derived* ClassView relation, not harvest predicate #6 diff --git a/tools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.py b/tools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.py index f5e83312..bbba009b 100644 --- a/tools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.py +++ b/tools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.py @@ -172,6 +172,42 @@ _VALIDATION_KINDS = ("presence", "uniqueness", "range", "format", "lookup") +def _new_sel_acc() -> Dict[str, object]: + """An order-independent Selection-domain accumulator per `(model, field)`. + + `replace` and `extend` are kept SEPARATE through the whole scan so the + final domain does not depend on the order `glob.iglob` happens to walk the + addon files in (codex follow-up on #536): a `selection_add=` extension + scanned BEFORE the base `selection=[…]` must not be clobbered by the + base's REPLACE. + + - `replace`: keys from the last static `selection=[…]` (full declaration — + a redeclaration supersedes the earlier one). `None` until one is seen. + - `extend`: the union of every `selection_add=[…]` (order-preserving). + - `poison`: any dynamic base seen → the domain is open and never closable. + """ + return {"replace": None, "extend": [], "poison": False} + + +def _resolve_selections( + acc: "Dict[Tuple[str, str], Dict[str, object]]", +) -> "Dict[Tuple[str, str], Optional[List[str]]]": + """Collapse the per-field accumulators into the final domain map: + `None` = poisoned (open), else `replace ++ extend` (de-duped, base first). + Order-independent: replace and extend were tracked separately.""" + out: "Dict[Tuple[str, str], Optional[List[str]]]" = {} + for key, rec in acc.items(): + if rec["poison"]: + out[key] = None + continue + domain: List[str] = list(rec["replace"] or []) # type: ignore[arg-type] + for v in rec["extend"]: # type: ignore[union-attr] + if v not in domain: + domain.append(v) + out[key] = domain + return out + + def _selection_tuple_keys(node: Optional[ast.expr], out: List[str], seen: Set[str]) -> None: """Append the first-element string key of each 2-tuple in `node` (a list or tuple of `('key','Label')` pairs) to `out`, de-duplicating against @@ -335,7 +371,7 @@ def _scan_file( relmap: Dict[Tuple[str, str], Tuple[str, Optional[str]]], inherits: Optional[Dict[str, Set[str]]] = None, constrains: Optional[Dict[Tuple[str, str], Set[str]]] = None, - selections: Optional[Dict[Tuple[str, str], Optional[List[str]]]] = None, + selections: Optional[Dict[Tuple[str, str], Dict[str, object]]] = None, ) -> None: """Parse one .py file; record every relational field on every named model. Optionally also record `inherits_from` edges and `@api.constrains` @@ -492,28 +528,29 @@ def _scan_file( # comodel for a given (model, field) is stable in practice. relmap[(mu, field_name)] = (comodel, inverse) if selections is not None: - # Sentinel `None` value = POISONED (open domain): a dynamic - # base anywhere means the field can never be a closed - # `ASSERT IN [...]`. Poison is STICKY and wins over any keys. + # Accumulate REPLACE / EXTEND / POISON SEPARATELY per field so + # the resolved domain is independent of file scan order (codex + # follow-up on #536). `_resolve_selections` collapses these at + # the end of `build_all_facts`. + # - poison: a dynamic base → open domain, sticky, never closed + # - replace: last static `selection=` wins (drops stale keys + # from a superseded full redeclaration — #532 finding #2) + # - extend: union of every `selection_add=` (order-preserving; + # NEVER clobbered by a replace — the #536 fix) for field_name in local_sel_poison: - selections[(mu, field_name)] = None - # A full static `selection=` REPLACES the domain (drops the - # earlier declaration's stale keys — codex #532 finding #2), - # unless the field is already poisoned. + selections.setdefault((mu, field_name), _new_sel_acc())[ + "poison" + ] = True for field_name, keys in local_sel_replace.items(): - if selections.get((mu, field_name), 0) is None: - continue - selections[(mu, field_name)] = list(keys) - # `selection_add=` EXTENDS (unions onto the accumulated set), - # unless poisoned (codex #532 finding #1 — never close a - # dynamic-base domain from selection_add alone). + selections.setdefault((mu, field_name), _new_sel_acc())[ + "replace" + ] = list(keys) for field_name, keys in local_sel_extend.items(): - if selections.get((mu, field_name), 0) is None: - continue - acc = selections.setdefault((mu, field_name), []) + acc = selections.setdefault((mu, field_name), _new_sel_acc()) + ext = acc["extend"] for v in keys: - if v not in acc: - acc.append(v) + if v not in ext: + ext.append(v) # ── P1b: inherits_from edges ───────────────────────────────────── # Only when the class declares a NEW model (_name is set). An @@ -561,10 +598,14 @@ def build_all_facts( relmap: Dict[Tuple[str, str], Tuple[str, Optional[str]]] = {} inherits: Dict[str, Set[str]] = {} constrains: Dict[Tuple[str, str], Set[str]] = {} - selections: Dict[Tuple[str, str], List[str]] = {} + # Raw per-field accumulators (replace/extend/poison kept separate); resolved + # to the final `Optional[List[str]]` domain map AFTER the full scan, so the + # result is independent of glob walk order. + sel_acc: "Dict[Tuple[str, str], Dict[str, object]]" = {} pattern = os.path.join(addons_root, "**", "*.py") for path in glob.iglob(pattern, recursive=True): - _scan_file(path, relmap, inherits, constrains, selections) + _scan_file(path, relmap, inherits, constrains, sel_acc) + selections = _resolve_selections(sel_acc) return relmap, inherits, constrains, selections diff --git a/tools/odoo-blueprint-extractor/tests/test_spo_enrich.py b/tools/odoo-blueprint-extractor/tests/test_spo_enrich.py index 9ecd2739..90f7fbe8 100644 --- a/tools/odoo-blueprint-extractor/tests/test_spo_enrich.py +++ b/tools/odoo-blueprint-extractor/tests/test_spo_enrich.py @@ -599,17 +599,19 @@ def test_name_plus_inherit_binds_to_name_only(self): def _scan_sel(src): - """Run `_scan_file` over a synthetic module; return the selections dict.""" - from odoo_blueprint_extractor.spo_enrich import _scan_file - relmap, inherits, constrains, selections = {}, {}, {}, {} + """Run `_scan_file` over a synthetic module; return the RESOLVED selections + map (`None` = open domain, else the closed value list) — mirroring what + `build_all_facts` returns.""" + from odoo_blueprint_extractor.spo_enrich import _scan_file, _resolve_selections + relmap, inherits, constrains, sel_acc = {}, {}, {}, {} with tempfile.NamedTemporaryFile("w", suffix=".py", delete=False) as f: f.write(src) path = f.name try: - _scan_file(path, relmap, inherits, constrains, selections) + _scan_file(path, relmap, inherits, constrains, sel_acc) finally: os.unlink(path) - return selections + return _resolve_selections(sel_acc) def _classify_sel(src): @@ -764,6 +766,24 @@ def test_full_redeclaration_replaces_not_unions(self): "posted", keys, "stale key from the replaced declaration must be dropped" ) + def test_selection_add_before_base_is_order_independent(self): + # codex #536 follow-up: the `selection_add` EXTENSION class is scanned + # BEFORE the base `selection=` class (glob.iglob gives no order). The + # base's REPLACE must NOT clobber the already-accumulated extension — + # the resolved domain includes the added key regardless of order. + sel = _scan_sel( + "class Ext:\n" + " _inherit = 'account.move'\n" + " state = fields.Selection(selection_add=[('reviewed','Reviewed')])\n" + "\n" + "class Base:\n" + " _name = 'account.move'\n" + " state = fields.Selection([('draft','Draft'),('posted','Posted')])\n" + ) + self.assertEqual( + sel, {("account_move", "state"): ["draft", "posted", "reviewed"]} + ) + class TestP3SelectionValueEmission(unittest.TestCase): def _triples(self, field_iri="odoo:account_move.state"):