fix(genbi): treat non-dict apps.yml entries as missing - #2583
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe GenBI app index now handles non-dictionary entries during lookup, registration, updates, and listing. Unit tests cover malformed entries, valid entries, persistence, replacement, rejection, and CLI output. ChangesApp Entry Validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/wren/tests/unit/test_genbi_get_app_guard.py (1)
8-13: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for
update_app()’s new contract.This test validates
get_app()only, while the PR also changesupdate_app()to raiseKeyErrorfor missing or non-dictionary entries. Add cases for malformed, missing, and valid entries, including persistence of the valid update.🤖 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 `@core/wren/tests/unit/test_genbi_get_app_guard.py` around lines 8 - 13, Add tests for update_app’s new contract alongside test_get_app_skips_non_dict_entry: assert KeyError for both missing and non-dictionary app entries, and verify a valid dictionary entry updates successfully and persists when reloaded through get_app.
🤖 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.
Nitpick comments:
In `@core/wren/tests/unit/test_genbi_get_app_guard.py`:
- Around line 8-13: Add tests for update_app’s new contract alongside
test_get_app_skips_non_dict_entry: assert KeyError for both missing and
non-dictionary app entries, and verify a valid dictionary entry updates
successfully and persists when reloaded through get_app.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 179b6acf-b8ae-4817-8ee2-3795453bbf15
📒 Files selected for processing (2)
core/wren/src/wren/genbi/index.pycore/wren/tests/unit/test_genbi_get_app_guard.py
ReviewReal bug, right direction, nicely restrained diff, CI green. My concern is that the fix is too narrow: two sibling paths in the same file share the same root cause and are untouched — and the new 1.
|
Address goldmedal review on Canner#2583: register_app no longer TypeErrors on truthy non-dict entries, list skips malformed entries, fold get_app guard tests into test_genbi_index and cover update_app contract.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tests/unit/test_genbi_index.py`:
- Around line 210-223: Update test_register_app_replaces_non_dict_entry to
reload the saved index after register_app completes, then assert apps["bad"]
contains the replacement entry and its expected fields, confirming the repair
persisted to .wren/apps.yml in addition to validating the returned dictionary.
- Around line 251-274: Update test_list_skips_non_dict_entries to assert that
the malformed app name "bad" is absent from result.output, alongside the
existing assertions for successful listing and valid app output, verifying
list_apps excludes skipped malformed entries.
🪄 Autofix
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 Plus
Run ID: 9c0ca78b-521c-41d7-b579-a485ccad8822
📒 Files selected for processing (3)
core/wren/src/wren/genbi/cli.pycore/wren/src/wren/genbi/index.pycore/wren/tests/unit/test_genbi_index.py
CodeRabbit: reload after register_app repair; assert list omits "bad".
|
Thanks for the thorough review — agreed the first pass was too narrow. Addressed in 721ba0f / d3b1718:
Design call (#3): kept the lenient stance for individual app entries ( On bare CI was green on the prior commit; re-running on the test tighten-up. |
goldmedal
left a comment
goldmedal
left a comment
There was a problem hiding this comment.
CI is green and the tests do call real functions rather than matching source text, but two items block: this duplicates an open PR against the same file, and the list hardening doesn't actually hold.
Blocking
1. Duplicate PR against the same file. #2587 changes the same list_apps in core/wren/src/wren/genbi/cli.py for the same non-dict-entry bug. The contribution bar says duplicate PRs against the same file are closed without review. Fold them into one PR or withdraw one — and note #2587 is strictly better on that hunk, see (2).
2. The list_apps guard is incomplete — the same crash survives one level down. Verified on this branch: a valid dict entry whose deploy is a scalar still kills the command.
apps: {good: {source: apps/good, status: deployed, deploy: "https://x"}}
$ wren genbi list
AttributeError: 'str' object has no attribute 'get' # exit 1, no output
deploy = entry.get("deploy") or {} → deploy.get("last_url"). If the premise is "a hand-edited apps.yml must not crash the CLI", this PR does not deliver it. #2587 covers exactly this case ("Non-dict deploy blocks are treated as empty").
3. The update_app hunk is not a fix. Both CLI callers (verify, deploy) reach update_app only after _require_registered, which already guarantees a dict via get_app. Neither catches KeyError or AttributeError. So the change swaps one uncaught traceback for another — no user-visible difference. Under "Label honestly" this hunk is refactor:, not fix:.
4. Dead assertion. assert "Traceback" not in result.output cannot fail: Click's CliRunner puts the exception in result.exception and leaves output empty. Confirmed in the scenario that does crash (case 2) — result.output == '' and the assertion still passes. "Every assertion must be capable of failing." Drop it; assert result.exit_code == 0 is the real check.
Design
5. Validation is scattered instead of done at the edge, with four different policies. load_index is already this module's validation boundary — it raises MalformedIndexError naming the path for a non-mapping top level and for a non-mapping apps. This PR adds four more guards downstream, each with different semantics: get_app → silently None, list_apps → silently skip, register_app → silently overwrite, update_app → KeyError. core/wren/.claude/CLAUDE.md says validate once, at the edge, and for a genuine input boundary "validate and report rather than crash".
For the shape to follow here, see 9bdae39 (#2604), merged after this branch was opened: the loader drops malformed entries silently and validate_project is the single place that reports them. One entry-level check inside load_index covers all four callers and matches that precedent.
6. Silent skip produces an inconsistent surface. With bad: "not-a-map", wren genbi list shows nothing, but wren genbi remove bad then succeeds and prints Removed bad from the index. The user is told it doesn't exist, then removes it. remove_app still uses name in index["apps"] and wasn't touched. At minimum report the bad key on stderr.
7. register_app silently discards hand-written data and prints the same Registered X message as a normal create, so there is no signal the entry was replaced.
Description
The states are reachable — confirmed on the base commit: register_app → TypeError: 'str' object does not support item assignment, get_app → returns 'not-a-map', update_app → AttributeError — but the description asserts this instead of showing it, which is what the pre-merge check flagged. Please paste the actual errors.
Also address the module docstring: it says apps.yml is "machine-written ... never hand-rolled", which is a recorded decision this PR reverses. load_index already contradicts it, so either argue it is wrong or update it.
Rebase
This branch is 33 commits behind main and its CI ran against a base that predates 9bdae39. It merges cleanly, but a textually clean merge is not a semantically correct one — please rebase and re-run.
What's good
The register_app fix is genuinely correct — falsy non-dicts were already handled by or, truthy ones were not. The get_app guard turns an AttributeError into a clean "not registered" message via _require_registered.
- list: report invalid entries; treat non-dict deploy as empty - get_app: non-dict → None (not registered) - register_app: replace truthy non-dict instead of TypeError - update_app: KeyError on missing/non-dict Addresses goldmedal review on Canner#2583/Canner#2587 (single PR).
2 participants
Summary
Hand-edited
.wren/apps.ymlcan place a scalar under an app key;get_appreturned it and callers crashed on mapping ops.Changes
get_appreturns None for non-dict entriesupdate_appraises KeyError instead of AttributeErrorSummary by CodeRabbit