fix(genbi): harden non-dict apps.yml entries in list/get/register - #2587
fix(genbi): harden non-dict apps.yml entries in list/get/register#2587Bartok9 wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe GenBI index validates app entries and normalizes deployment state. CLI listing skips malformed apps and safely reads deployment URLs. Provider deployment receives normalized existing deployment data. Tests cover malformed app and deployment entries. ChangesGenBI malformed data handling
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant GenBI CLI
participant GenBI index
participant Deployment provider
GenBI CLI->>GenBI index: Read app and deploy_state
GenBI index-->>GenBI CLI: Return normalized deployment mapping
GenBI CLI->>Deployment provider: Pass existing deployment linkage
Deployment provider-->>GenBI CLI: Return deployment record
GenBI CLI->>GenBI index: Persist deploy_record
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_list_nonduct.py (1)
13-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover malformed
deployvalues.This test verifies invalid app entries but not the new non-dictionary
deployhandling incore/wren/src/wren/genbi/cli.pylines 210-214. Add a valid app whosedeployvalue is a scalar and assert listing succeeds without a URL suffix.Proposed test extension
"apps": { "good": {"data_mode": "snapshot", "status": "ready"}, "bad": "not-a-mapping", + "bad-deploy": { + "data_mode": "snapshot", + "status": "ready", + "deploy": "not-a-mapping", + }, }, ... assert "good" in result.stdout + assert "bad-deploy [snapshot, ready]" in result.stdout assert "invalid entry" in result.stdout🤖 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_list_nonduct.py` around lines 13 - 27, Extend the test around save_index and the genbi_app list invocation with a valid app whose deploy field is a scalar rather than a mapping. Keep the successful listing assertions, and verify that this app is listed without a URL suffix while the existing malformed app still produces the invalid-entry output.
🤖 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_list_nonduct.py`:
- Around line 13-27: Extend the test around save_index and the genbi_app list
invocation with a valid app whose deploy field is a scalar rather than a
mapping. Keep the successful listing assertions, and verify that this app is
listed without a URL suffix while the existing malformed app still produces the
invalid-entry output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c0ae7cda-7e1c-4dd0-8e53-a4f05b649f76
📒 Files selected for processing (2)
core/wren/src/wren/genbi/cli.pycore/wren/tests/unit/test_genbi_list_nonduct.py
- 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).
Bartok9
changed the title
9921cdd to
0fc97c3
Compare
|
@goldmedal folded the #2583 + #2587 work into this single PR and rebased on current
Closing #2583 as the duplicate against the same files. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/tests/unit/test_genbi_list_nonduct.py`:
- Around line 44-46: Update the test around the genbi_app list invocation to
assert that the invalid non-dictionary deploy value, including “https://x”, is
absent from result.stdout while preserving the existing successful exit-code and
app-name assertions.
🪄 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: 471b8ba2-d888-4d21-b972-8bbec0268150
📒 Files selected for processing (4)
core/wren/src/wren/genbi/cli.pycore/wren/src/wren/genbi/index.pycore/wren/tests/unit/test_genbi_index.pycore/wren/tests/unit/test_genbi_list_nonduct.py
🚧 Files skipped from review as they are similar to previous changes (1)
- core/wren/src/wren/genbi/cli.py
CodeRabbit: non-dict deploy must not render last_url.
goldmedal
left a comment
goldmedal
left a comment
There was a problem hiding this comment.
Nice catch on the or fall-through in register_app — the truthy-non-dict case is genuinely easy to miss, and folding #2583 in here was the right call. The entry-level hardening looks complete to me. One gap on a nested field, plus some minors.
🔴 Blocking — the deploy command still AttributeErrors on a non-dict deploy block
test_genbi_list_nonduct.py constructs {"deploy": "https://x"} as the realistic malformed shape, and list now tolerates it. But cli.py also consumes that field, unguarded:
# core/wren/src/wren/genbi/cli.py:358
deployment = adapter.deploy(..., link=entry.get("deploy"))Both providers declare link: dict | None and immediately call .get() on it:
providers/vercel.py:86—if link and link.get("org_id")providers/cloudflare.py:62—(link or {}).get("account_id")
E2E against this branch (register an app → hand-edit deploy to a scalar → run both commands):
LIST exit: 0 ← fixed by this PR
DEPLOY exit: 1 AttributeError("'str' object has no attribute 'get'")
Same exception class the PR description cites as the bug. It's a worse failure on this path than it was on list: it fires after token resolution, and it surfaces as an uncaught traceback rather than a DeployError (the only exception the deploy command catches). In Vercel's case it's also before _request, so it's a deterministic local crash, not a network edge case.
🟠 Suggestion — the nested-field guard wants to be shared, not inlined
Worth separating the two levels, because they're in different shape:
- Entry-level (
apps.<name>itself is not a mapping) — centralized and complete. All three consumers incli.pyreach entries through_require_registered→get_app, andregister_appcovers the create path. Nothing to add. - Nested-field level (
entry["deploy"]is not a mapping) — ad-hoc. Guarded inline inlist_apps, missing indeploy. That's the blocking item above.
The exposure here is bounded, which makes this cheap to close properly: deploy is the only entry field consumed as a mapping. data_mode and status are read via .get() with defaults and only string-compared or formatted, so a stray mapping there is ugly output rather than a crash; entry['data_mode']/entry['status'] at line 192 read a dict register_app just built; source isn't read in cli.py at all.
So one small accessor next to get_app in index.py covers it, and both call sites share it:
def deploy_state(entry: dict) -> dict:
"""The entry's deploy block, or {} when hand-edited to a non-mapping."""
d = entry.get("deploy")
return d if isinstance(d, dict) else {}list_apps:last_url = deploy_state(entry).get("last_url")— also retires the now-redundantor {}on line 207, which the followingisinstancecheck makes dead.deploy:link=deploy_state(entry) or None— keeps the providers' declareddict | Nonecontract intact.
To be explicit about a road not taken: I don't think this should move into load_index the way _load_views_v1/_load_relationships filter non-mappings. Those loaders return a fresh list that's never written back, so filtering is free. index.py is read-modify-write — register_app/remove_app/update_app all save_index(index) the whole dict — so filtering at load would mean an unrelated wren genbi register otherapp silently deletes a malformed bad entry from disk, and would break remove_app, which today can still delete a non-dict entry and is the only way a user has to clean one up. Per-call-site is the right call for this file. Just worth making the one shared helper explicit so the next nested field doesn't repeat this.
🟡 Minor
- The list test doesn't lock in the behavior it exists to protect.
assert "invalid entry" in (result.stdout + result.stderr)still passes if someone flipserr=Truetoerr=False. Streams are cleanly separated under the locked click 8.3.1 — I checked: stdout is'good [snapshot, ready]\n', stderr is'bad [invalid entry: str]\n'— so this can assertin result.stderrandnot in result.stdout. registersilently clobbers wherelistreports. Same malformed entry, two policies:listwrites[invalid entry: str]to stderr,register_appreplaces it with no notice. The replacement itself is fine — it's an explicit user command naming that app — but a one-line stderr note ("replacing malformed entry forbad") would make the two consistent and leave a trace.- Contradictory diagnosis between commands.
listsaysbad [invalid entry: str];verify badsaysnot registered. Run 'wren genbi register bad' first. The suggested remedy does work, so this is cosmetic — but the message is misleading about why, andupdate_app's new docstring already gestures at this ("callers that need a friendlier message"). Could be a follow-up. - Filename typo:
test_genbi_list_nonduct.py→nondict. It's also in the branch name (fix/genbi-list-nonduct-entries), which stays visible in the PR compare header and merge metadata even after the branch is deleted — so at minimum worth renaming the file. - PR body is self-referential: "folds #2583 + prior #2587 list work" — #2587 is this PR.
- Branch is a couple of commits behind
main; neither touchesgenbi/, so no conflict — just noting it.
Verification I ran: the PR's own suite (36 passed), ruff check src clean, line-level and end-to-end repro of the deploy crash, and the stdout/stderr separation check above. ruff check tests reports 45 pre-existing errors including function-level imports throughout the suite, and CI only lints src — so the import pytest inside test_update_app_rejects_non_dict_entry matches existing convention and isn't a finding.
goldmedal review on Canner#2587: list already tolerated scalar deploy, but deploy still passed entry.get("deploy") into providers that call .get(). - add index.deploy_state(entry) -> dict - list_apps and deploy both use it (link=deploy_state(entry) or None) - rename test_genbi_list_nonduct.py -> nondict; lock stderr-only invalid entry
|
@goldmedal thanks — great catch on the deploy path. Addressed:
Left as follow-up (cosmetic): register stderr notice on replace, and friendlier verify message when entry exists but is non-dict. PR body self-ref cleaned up. |
goldmedal
left a comment
goldmedal
left a comment
There was a problem hiding this comment.
Fix looks right and I verified it end-to-end — thanks for the quick turnaround. Two new things introduced by this commit, one of which I'd want fixed before merge.
✅ Resolved
- Deploy path — confirmed against
b2eb996a: register an app, hand-editdeployto a scalar, runwren genbi deploy. No moreAttributeError; it now fails cleanly through the normal error path instead of an uncaught traceback. or {}+ inlineisinstanceretired fromlist_apps;link=deploy_state(entry) or Nonepreserves the providers'dict | Nonecontract (behaviour is identical for the missing-key and valid-dict cases, and an empty{}now normalizes toNone, which both providers already treat the same).- File renamed to
test_genbi_list_nondict.py; the stderr assertion is now precise; PR body self-reference cleaned up. CI 11/11 green.
🟠 The new test doesn't actually guard the fix
test_deploy_state_normalizes_non_dict tests the helper in isolation, but nothing asserts that the deploy command routes through it. I mutation-checked this — reverting just line 361 back to the original bug:
- link=deploy_state(entry) or None,
+ link=entry.get("deploy"),...and the whole suite still passes, 37/37. So the regression this PR exists to fix is currently unprotected: any future refactor can reintroduce it silently.
Worth a command-level test in test_genbi_deploy.py, which already has the _make_deployable_project fixture and the _isolate_env_loading autouse fixture to make it hermetic. Roughly: build the project, hand-edit .wren/apps.yml so deploy is a scalar, invoke genbi deploy, and assert not isinstance(result.exception, AttributeError). That fails on the mutated line and passes on the fix.
🟠 deploy_state is shadowed by a local dict inside deploy()
Both bindings live in the same function scope:
def deploy(...):
from wren.genbi.index import deploy_state, update_app # line 317 — binds the function
...
link=deploy_state(entry) or None, # line 361 — calls it
...
deploy_state = { # line 367 — rebinds to a dict
"provider": adapter.name, ...
}
update_app(project_path, name, status="deployed", deploy=deploy_state)This works today purely because the call at 361 precedes the rebind at 367. It's a trap for the next person: reorder those blocks, or add a second deploy_state(...) call anywhere below 367, and you get TypeError: 'dict' object is not callable. Ruff doesn't catch it either — I ran ruff check src on this head and it's clean, because the import is used before the rebind, so F811 doesn't fire.
The local dict predates the helper, so renaming the local is the smaller change — deploy_record, or new_state. Either way the two shouldn't share a name in one scope.
🟡 Minor
test_deploy_state_normalizes_non_dictlives intest_genbi_list_nondict.py, but it exercisesindex.deploy_state, notlist.test_genbi_index.pyis the natural home, next to the other index-level tests.
Verification: ruff check src clean, genbi suite 37 passed, E2E confirmation that the deploy crash is gone, and the mutation check above.
goldmedal review on Canner#2587: - rename local deploy_state dict to deploy_record in deploy() - add test_deploy_tolerates_scalar_deploy_block (guards link= path) - move deploy_state unit test into test_genbi_index.py
|
@goldmedal thanks — both items addressed on this head:
Local: genbi index/list/deploy unit suite 38 passed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/wren/tests/unit/test_genbi_deploy.py (1)
119-121: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the normalized provider argument directly.
The test claims that the provider receives
link=None, but_FakeTransportrecords only the HTTP request.assert fake.callsdoes not verify thelinkargument. Spy on the provider adapter or patchget_provider, then assertlink is Nonefor the scalardeployentry.🤖 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_deploy.py` around lines 119 - 121, Update the test around the scalar deploy entry and provider invocation so it spies on or patches get_provider (or the provider adapter) and captures the link argument. Assert directly that the provider receives link=None, while retaining the existing fake.calls assertion only if it still verifies request execution.
🤖 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_deploy.py`:
- Around line 119-121: Update the test around the scalar deploy entry and
provider invocation so it spies on or patches get_provider (or the provider
adapter) and captures the link argument. Assert directly that the provider
receives link=None, while retaining the existing fake.calls assertion only if it
still verifies request execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 852cfc07-57e4-4bb6-b6bd-75d6d211e166
📒 Files selected for processing (4)
core/wren/src/wren/genbi/cli.pycore/wren/tests/unit/test_genbi_deploy.pycore/wren/tests/unit/test_genbi_index.pycore/wren/tests/unit/test_genbi_list_nondict.py
🚧 Files skipped from review as they are similar to previous changes (3)
- core/wren/tests/unit/test_genbi_list_nondict.py
- core/wren/tests/unit/test_genbi_index.py
- core/wren/src/wren/genbi/cli.py
Spy get_provider so the regression test captures the provider link arg directly (CodeRabbit nit on Canner#2587).
|
@coderabbitai good catch — tightened |
|
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! You are interacting with an AI system. |
2 participants
Summary
Single PR for hand-edited
apps.ymlnon-dict entries (folds #2583 list/index hardening):listdeployviadeploy_stateget_appNone(CLI "not registered")register_appTypeErroron item assignmentupdate_appKeyErrorwhen missing/non-dictdeploylink=deploy_state(entry) or Noneso providers never.geton a scalarReproduction (base / before)
Verification
Summary by CodeRabbit
Bug Fixes
Tests