fix(odoo): codex review on #532/#533 — selection replace/extend/poison + mro rustfmt#536
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 6 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
Changesspo_enrich Selection Domain Semantics Fix
mro.rs Formatting Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca67c5d5fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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) |
There was a problem hiding this comment.
When a selection_add extension is scanned before the base/static declaration, this replacement assignment discards the accumulated extension keys. build_all_facts walks addon files with glob.iglob, so the scanner can see an _inherit class adding ('reviewed', ...) before the class that declares selection=[('draft', ...)]; the final map then becomes only ['draft'], and the emitted ASSERT IN rejects the legal added value. The replace and extend state needs to be tracked separately or extensions reapplied after replacements so selection_add remains an extension independent of traversal order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 618d1c6. Correct and subtle — glob.iglob gives no order guarantee, and my replace/extend/poison fix collapsed the three into one accumulated list during the scan, so a selection_add seen before the base selection= got clobbered by the REPLACE.
Now replace / extend / poison are tracked separately per (model, field) for the whole scan (_new_sel_acc) and collapsed only at the end (_resolve_selections in build_all_facts): None if poisoned, else replace ++ extend de-duped (base first). selection_add lives in its own bucket and is never overwritten by a replace — order-independent.
Regression test test_selection_add_before_base_is_order_independent declares the extension class before the base and asserts the resolved domain is still [draft, posted, reviewed]. 62/62.
…ce/extend/poison + mro rustfmt 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.
AdaWorldAPI
force-pushed
the
claude/odoo-codex-followups
branch
from
ca67c5d to
ab385e6
Compare
) 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.py (1)
545-552:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
build_all_factslocalselectionsannotation is now inconsistent with the return type.At Line 551 the function returns
Optional[List[str]]values, but the local variable is still declared asDict[Tuple[str, str], List[str]](Line 564)._scan_filecan writeNone, so this type hint is stale and misleading.Proposed fix
- selections: Dict[Tuple[str, str], List[str]] = {} + selections: Dict[Tuple[str, str], Optional[List[str]]] = {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.py` around lines 545 - 552, The local variable `selections` in the `build_all_facts` function has an inconsistent type annotation. The function's return type declares the fourth element as `Dict[Tuple[str, str], Optional[List[str]]]` to account for None values that `_scan_file` can write, but the `selections` variable is still annotated as `Dict[Tuple[str, str], List[str]]` without Optional. Update the type annotation of the `selections` variable to `Dict[Tuple[str, str], Optional[List[str]]]` to match the return type annotation and accurately reflect that the dictionary values can be None or a List of strings.
🧹 Nitpick comments (1)
tools/odoo-blueprint-extractor/tests/test_spo_enrich.py (1)
689-766: ⚡ Quick winAdd regression cases for
related=and dynamicselection_addopen domains.Current coverage (Line 709 onward) validates dynamic base poisoning, but it does not pin the same open-domain outcome for
related=and non-literalselection_add. Adding both keeps the “never close unknown domain” contract explicit.Suggested tests
class TestSelectionScan(unittest.TestCase): @@ def test_dynamic_base_poisons_field_to_none(self): @@ self.assertEqual(sel, {("account_move", "s"): None}) + def test_related_field_poisons_domain(self): + sel = _scan_sel( + "class Base:\n" + " _name = 'account.move'\n" + " state = fields.Selection([('draft','Draft')])\n" + "\n" + "class Ext:\n" + " _inherit = 'account.move'\n" + " state = fields.Selection(related='x.y')\n" + ) + self.assertEqual(sel, {("account_move", "state"): None}) + + def test_dynamic_selection_add_poisons_domain(self): + sel = _scan_sel( + "class Base:\n" + " _name = 'account.move'\n" + " state = fields.Selection([('draft','Draft')])\n" + "\n" + "class Ext:\n" + " _inherit = 'account.move'\n" + " state = fields.Selection(selection_add=STATE_CONST)\n" + ) + self.assertEqual(sel, {("account_move", "state"): None})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/odoo-blueprint-extractor/tests/test_spo_enrich.py` around lines 689 - 766, The TestSelectionScan class is missing regression test cases to explicitly verify that Selection fields with related= parameters and dynamic selection_add (non-literal expressions) both result in poisoned (None) domains. Add two new test methods following the existing pattern (test_dynamic_base_poisons_field_to_none, test_dynamic_base_plus_add_stays_poisoned) that use the _scan_sel helper to validate these scenarios. The first should test a Selection field with related='some_method' and assert the domain is None, and the second should test a Selection field with selection_add=some_variable (dynamic expression) and assert the domain is None, ensuring the "never close unknown domain" contract is maintained for these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.py`:
- Around line 191-229: The _extract_selection function incompletely detects
open-domain conditions. The function needs to be enhanced to: first, check for a
related= keyword argument in the loop over call.keywords and set base_dynamic to
True if found since related= makes domains dynamic; second, detect when add_node
exists but is not a static list or tuple (dynamic selection_add), which also
creates an open domain and should be reflected in the return value or in how
base_dynamic is calculated; and third, ensure that empty static selections like
selection=[] are properly handled as valid replacements rather than being
filtered out due to the non-empty base_keys gate. These edge cases mean the
poisoning logic driven only by base_dynamic is insufficient.
---
Outside diff comments:
In `@tools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.py`:
- Around line 545-552: The local variable `selections` in the `build_all_facts`
function has an inconsistent type annotation. The function's return type
declares the fourth element as `Dict[Tuple[str, str], Optional[List[str]]]` to
account for None values that `_scan_file` can write, but the `selections`
variable is still annotated as `Dict[Tuple[str, str], List[str]]` without
Optional. Update the type annotation of the `selections` variable to
`Dict[Tuple[str, str], Optional[List[str]]]` to match the return type annotation
and accurately reflect that the dictionary values can be None or a List of
strings.
---
Nitpick comments:
In `@tools/odoo-blueprint-extractor/tests/test_spo_enrich.py`:
- Around line 689-766: The TestSelectionScan class is missing regression test
cases to explicitly verify that Selection fields with related= parameters and
dynamic selection_add (non-literal expressions) both result in poisoned (None)
domains. Add two new test methods following the existing pattern
(test_dynamic_base_poisons_field_to_none,
test_dynamic_base_plus_add_stays_poisoned) that use the _scan_sel helper to
validate these scenarios. The first should test a Selection field with
related='some_method' and assert the domain is None, and the second should test
a Selection field with selection_add=some_variable (dynamic expression) and
assert the domain is None, ensuring the "never close unknown domain" contract is
maintained for these cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a19a61e-560a-4d18-b5cd-00e88969d0b0
📒 Files selected for processing (4)
.claude/board/EPIPHANIES.mdcrates/lance-graph-ontology/src/odoo_blueprint/mro.rstools/odoo-blueprint-extractor/odoo_blueprint_extractor/spo_enrich.pytools/odoo-blueprint-extractor/tests/test_spo_enrich.py
2 participants
Summary
Addresses the codex P2 review left on merged #532 (two real semantic bugs now live in
main) and the rustfmt miss on merged #533.Both #532 findings produce a wrong
ASSERT $value IN [...]— the silent-data-rejection class that matters for "Odoo as faithful DDL."#532 finding 1 —
selection_addmust not close a dynamic domainSelection(selection_add=[('x','X')])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 projectedASSERT $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.Selectiondeclaration, so an_inheritclass that redeclares the field withselection=[...](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):selection=[...]selection_add=[...]None; never closed; stickyA closed
ASSERT INis emitted only when the whole domain is statically known. A poisoned field is recorded asNonein theselectionsmap and skipped at emission (selection_skip_open_domainstat).#533 —
mro.rsrustfmtmro.rslanded 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(dynamic, base, add)contractcargo test -p lance-graph-ontology --lib mro— 7/7rustfmt --check mro.rs— cleanEPIPHANIES.mdprependsE-ODOO-SELECTION-REPLACE-EXTEND-POISON.Note on the OGAR /
ar_shapediscussionA parallel thread raised whether OGAR producer types should move into
lance-graph-contract(alance-graph-contract::ar_shapemirror, droppingogar-proposal's ownedMappingProposalmirror, addingogar-from-ruff/ogar-from-rails). That's a separate architectural proposal — not in this PR, which is scoped to the concrete codex review feedback. Happy to take it up separately if wanted; #534'sclass_id_for_guidwiring is the natural predecessor.Summary by CodeRabbit
Bug Fixes