fix(genbi): normalise null apps/schema_version in app index#2406
Conversation
load_index used data.setdefault('apps', {}), which is a no-op when the key
already exists. An apps.yml written with an explicit but empty 'apps:'
(hand-edit or truncated write) parses to {'apps': None}, so the key was
present and setdefault left None in place — then get_app/register_app/etc.
crashed with an opaque AttributeError, defeating MalformedIndexError's
purpose. Normalise null apps/schema_version with 'or', and raise
MalformedIndexError when apps is a non-mapping.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesNull-safe normalization and validation in
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@core/wren/src/wren/genbi/index.py`:
- Around line 58-65: The normalization in the index loading logic is too broad
because `or` in `index.py` converts any falsy `schema_version` or `apps` value
instead of only handling `None`. Update the `data["schema_version"]` and `apps`
handling in the same block to preserve valid falsy values like `0`, `false`,
`[]`, and `""`, while still defaulting only when the value is actually `None`;
keep the existing `MalformedIndexError` check in place for `apps` so non-mapping
inputs continue to fail in the `INDEX_SCHEMA_VERSION` / `apps` normalization
path.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cf3d22ec-9de7-45f4-8658-294ff6f81449
📒 Files selected for processing (2)
core/wren/src/wren/genbi/index.pycore/wren/tests/unit/test_genbi_index.py
Per review: check 'is None' rather than truthiness so valid falsy values (0, False, []) are preserved and surfaced for validation instead of being silently coerced to a default. Add a test for falsy schema_version (0).
|
Good catch, @coderabbitai — fixed in 75007cb. Switched both fields from |
|
Tip For best results, initiate chat on the files or code changes.
૮₍ ˶ᵔ ᵕ ᵔ˶ ₎ა If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
@Bartok9, please check the CI fails. Thanks. |
|
Thanks @goldmedal — looked into it. The crates.io dropped the connection mid-download while building |
|
@goldmedal CI is green now ✅ — all checks pass (unit, postgres, mysql, memory, ui tests, lint, CodeRabbit). The earlier failure has cleared on the latest run. Ready for review whenever you have a moment. Thanks! 🙏 |
Summary
load_indexnormalised a missingappskey withdata.setdefault("apps", {}), butsetdefaultis a no-op when the key already exists.apps.ymlwith an explicit-but-emptyapps:(a hand-edit or a truncated write) is valid YAML that parses to{"apps": None}— the key is present, sosetdefaultleavesNonein place. Every accessor (get_app,register_app,remove_app,update_app) then crashes with an opaqueAttributeError: 'NoneType' object has no attribute 'get'.This defeats the whole point of
MalformedIndexError, which exists precisely so a bad index file fails clearly instead of crashing deep in a command.Reproduction
Fix
Normalise null values with
orinstead ofsetdefault, so an explicit nullapps:/schema_version:is coerced to its default. Also raiseMalformedIndexError(not a later opaque crash) whenappsis present but is a non-mapping (e.g. a list).Real behavior proof
Real environment: Python 3.12,
core/wrenviauv run --no-sync, repomain.Commands:
AFTER fix:
BEFORE fix (new tests stashed onto unpatched index.py):
Tests
tests/unit/test_genbi_index.pygains coverage for nullapps, nullschema_version, and a non-mappingapps(raisesMalformedIndexError). The null-apps test fails without the fix.Scope / risk
Apache-2.0 path (
core/wren). Only changes index normalisation; well-formed index files behave identically (covered by the existing 10 index tests).Summary by CodeRabbit
appsfield isn’t a mapping, preventing downstream crashes.0.apps, nullschema_version, invalid (non-mapping)apps, and preservation ofschema_version: 0.