Skip to content

fix(mcp): harden list_cubes member arrays - #2617

Open
Bartok9 wants to merge 2 commits into
Canner:mainfrom
Bartok9:fix/mcp-list-cubes-member-guards
Open

fix(mcp): harden list_cubes member arrays#2617
Bartok9 wants to merge 2 commits into
Canner:mainfrom
Bartok9:fix/mcp-list-cubes-member-guards

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What failure does this repair?

MCP list_cubes raised AttributeError: 'str' object has no attribute 'get' when a cube's measures/dimensions/time_dimensions was non-list or held non-dict members. Repro: call the list_cubes tool against a manifest whose cube has "measures": ["revenue"] — name extraction called .get("name") on the string and the tool failed.

Summary

list_cubes now ignores invalid cube entries and extracts names only from valid dictionary members.

Verification

pytest tests/unit/test_mcp_server.py::test_list_cubes_skips_non_dict_members -q
# 1 passed

Duplicate check

Searched open/closed PRs; #2615 hardens related cube listing but not the member-array normalization in this path. No overlap.

list_cubes assumed cube members were always lists of dicts. Skip
non-dict cubes and member rows so malformed cube YAML cannot crash MCP.
@github-actions github-actions Bot added python Pull requests that update Python code core labels Jul 29, 2026
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

list_cubes now handles malformed cube and member structures defensively, while a unit test verifies filtering and name extraction behavior.

Changes

Cube enumeration robustness

Layer / File(s) Summary
Normalize cube members and validate malformed input
core/wren/src/wren/mcp_server.py, core/wren/tests/unit/test_mcp_server.py
list_cubes ignores invalid cube entries and extracts names only from valid dictionary members; the unit test covers malformed measures and mixed dimensions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • Canner/WrenAI#2615: Related hardening of cube listing and tests for malformed cube members.

Poem

I hop through cubes, both neat and strange,
And skip the shapes that drift or change.
Valid names sparkle in my tray,
While malformed bits bounce away.
Tests thump softly—hip-hop, hooray!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and accurately describes the main change: hardening list_cubes member array handling.
Description check ✅ Passed The description covers the required sections with a clear failure, summary, test verification, and duplicate check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@goldmedal

Copy link
Copy Markdown
Collaborator

Reviewing this together with #2615 — the MCP half of one change.

#2615 and #2617 harden the same cube listing behind two entry points — the wren cube list CLI and the list_cubes MCP tool. They should be one PR, because right now each covers a case the other misses and one of them guards states its data source cannot produce.

The two data sources differ, and that decides which guards are live

MCP list_cubes reads context.load_cubes, which already normalises. Both _load_cubes_v1 and _load_cubes_v2 do if isinstance(data, dict) before appending, and both always return a list. Measured against hand-edited cube YAML (a file that is a bare scalar, a file that is a list, and a valid cube with measures: nope):

load_cubes -> [{'name': 'orders', 'measures': 'nope', 'dimensions': [{'name': 'id'}, 'x'], ...}]
  container is list?        True     # `isinstance(cubes, list)` always holds
  any non-dict cube?        False    # `isinstance(cube, dict)` always holds
  any non-list 'measures'?  True     # member arrays ARE unvalidated

So in #2617 the two cube-level guards are dead and only _member_names is real. The test reaches the dead branches only by monkeypatching wren.context.load_cubes to return ["bad", ...] — a value the real loader cannot emit.

The CLI reads mdl.json through json.loads (_load_manifest_dict), which validates only that the top level is an object. Everything below it is genuinely untrusted, so #2615's cube-level guard is live. But the container is not guarded, and that is a silent-wrong-output bug. Measured on main + #2615:

cubes: "abc"        -> exit 0, output ''                      # neither listed nor "No cubes defined."
cubes: []           -> exit 0, output 'No cubes defined.'
cubes: ["bad", {…}] -> exit 0, lists orders                   # fixed by this PR

A scalar cubes prints a blank screen that is indistinguishable from neither-of-the-above. #2617 guards exactly this container case; #2615 does not.

What to do

Fold the two into one PR that:

  • guards the container and the member arrays in both entry points, so the CLI and the MCP tool agree
  • drops the cube-level guard and its monkeypatched test from the MCP side, where load_cubes already guarantees list[dict]
  • states in the description which side of the wren-core deserialization boundary each callsite sits on — the CLI path is upstream (json.loads), the MCP path is behind a loader that normalises

Better still, follow 9bdae39 (#2604): if member arrays should be trustworthy, normalise them once in load_cubes and report malformed ones from validate_project, rather than re-checking at every consumer.

Rebase

Both branches are 17 commits behind main and their CI ran against a base that predates 9bdae39. They merge cleanly, but a textually clean merge is not a semantically correct one — please rebase and re-run.

@Bartok9

Bartok9 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @goldmedal — agreed on the data-source boundary analysis.

Plan:

  1. Close this PR and fix(cube): harden list against non-dict cube/measure rows #2615 in favor of one combined follow-up that covers both entry points.
  2. Prefer the fix(context): filter non-mapping entries in _load_views_v1 #2604 / 9bdae39c pattern: normalise member arrays in load_cubes (and guard the CLI mdl.json container/list[dict] path where json.loads is upstream of normalisation), with validate_project reporting malformed member arrays rather than re-checking at every consumer.
  3. Drop dead cube-level guards + monkeypatched load_cubes returns on the MCP side; keep live member-array handling and the CLI container guard.
  4. Rebase onto current main (post-9bdae39c) and re-run CI before opening the replacement.

Will open the consolidated PR shortly and link both of these from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants