Surface meta in inspect_model output (DEV-1332)#87
Merged
ZmeiGorynych merged 3 commits intoMay 4, 2026
Merged
Conversation
PR #83 added `meta: Optional[Dict[str, Any]]` to `ModelMeasure` and `Aggregation` and storage round-trips it correctly, but `inspect_model` never rendered the field. Agents calling inspect_model to verify their bookkeeping saw no meta column and concluded the value had been dropped — the symptom DEV-1332 reports. Surface meta in `inspect_model`: - Markdown columns / measures / aggregations tables get a `meta` cell (compact JSON via a new `_format_meta` helper). The existing `_markdown_table` all-empty-column pruner hides the column when no entity in the section uses meta, so output is unchanged for callers that don't set it. - Markdown model header gets a `**meta:**` bullet when `model.meta` is non-None. - JSON form emits `meta` on every column / measure / aggregation entry and at the top level of the payload (unconditional, matching how `label` / `description` / `hidden` are emitted). Add 11 tests in tests/test_mcp_server.py: - 5 in TestInspectModel pinning meta rendering for columns, measures, aggregations, the model header, and the prune-when-absent contract. - 6 in TestEditModel pinning end-to-end edit_model -> storage round-trip for ModelMeasure.meta and Aggregation.meta (create-new and update-existing paths), plus replacement-not-merge semantics on update and the omitted-key-preserves-existing semantics of `_upsert_entity`. Docs: add `meta` row to the ModelMeasure fields table in docs/concepts/models.md, note meta on aggregations under Custom Aggregations, extend the Column entry in docs/concepts/terminology.md, update the inspect_model row in docs/reference/mcp.md and docs/interfaces/mcp.md, and update the meta bullet in CLAUDE.md to include aggregations and the inspect_model rendering. ruff clean, 1532 unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR extends the optional opaque JSON ChangesMeta Field Documentation, Rendering & Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 1/5 review remaining, refill in 47 minutes and 3 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_mcp_server.py (1)
2216-2230: ⚡ Quick winMake the aggregation create-path test explicit (custom name + formula).
test_edit_persists_aggregation_meta_create_pathcurrently uses"name": "sum"and no formula, which makes the create-path contract ambiguous. Prefer a custom aggregation name with formula so this test only verifies meta persistence, not reserved-name/formula edge behavior.♻️ Suggested adjustment
- result = await _call(mcp_server, name="edit_model", arguments={ - "model_name": "orders", - "aggregations": [{"name": "sum", "meta": {"owner": "x"}}], - }) + result = await _call(mcp_server, name="edit_model", arguments={ + "model_name": "orders", + "aggregations": [{ + "name": "my_agg", + "formula": "SUM({expr})", + "meta": {"owner": "x"}, + }], + }) assert json.loads(result)["success"] is True model = await storage.get_model("orders") assert model.aggregations[0].meta == {"owner": "x"} + assert model.aggregations[0].formula == "SUM({expr})"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_server.py` around lines 2216 - 2230, The test test_edit_persists_aggregation_meta_create_path is ambiguous because it uses the reserved name "sum" and no formula; change the aggregation payload in the call to edit_model to use a custom name (e.g., "custom_sum" or similar) and include an explicit "formula" value so the test exercises only meta persistence. Update the _call invocation arguments for "aggregations" to include {"name": "<custom_name>", "formula": "<some_formula>", "meta": {"owner": "x"}} and keep the rest (storage.save_model, assertion on model.aggregations[0].meta) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_mcp_server.py`:
- Around line 2216-2230: The test
test_edit_persists_aggregation_meta_create_path is ambiguous because it uses the
reserved name "sum" and no formula; change the aggregation payload in the call
to edit_model to use a custom name (e.g., "custom_sum" or similar) and include
an explicit "formula" value so the test exercises only meta persistence. Update
the _call invocation arguments for "aggregations" to include {"name":
"<custom_name>", "formula": "<some_formula>", "meta": {"owner": "x"}} and keep
the rest (storage.save_model, assertion on model.aggregations[0].meta)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9f452a1-e8d4-4c7c-b598-71d110baac00
📒 Files selected for processing (7)
CLAUDE.mddocs/concepts/models.mddocs/concepts/terminology.mddocs/interfaces/mcp.mddocs/reference/mcp.mdslayer/mcp/server.pytests/test_mcp_server.py
- CodeRabbit nitpick: rewrite test_edit_persists_aggregation_meta_create_path
to use a custom aggregation name + formula (`my_agg` / `SUM({expr})`)
instead of the built-in `sum`. The previous shape passed even if formula
round-trip silently dropped the value, since `sum` is a built-in and the
formula is optional. Now pins both meta and formula persistence on the
create path, mirroring the matching update-path test.
- Sonar S5886 (storage fixture): annotate the `storage` pytest fixture
return type as `Generator[YAMLStorage, None, None]` so pyright/Sonar
recognise the yield correctly. Pre-existing issue surfaced during the
PR review; trivial enough to address inline.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mcp-meta-field-on-modelmeasure-and-aggregation-doesnt # Conflicts: # tests/test_mcp_server.py
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
inspect_modelnow rendersmetaforColumn,ModelMeasure,Aggregation, and the model header itself — Markdown tables get ametacell (compact JSON, hidden by_markdown_table's all-empty-column pruner when no entity uses meta), the model-header gets a**meta:**bullet, and the JSON form emitsmetaon every entity entry and at the top level of the payload.metacorrectly post-PR Add meta field to ModelMeasure and Aggregation #83 — the visible defect was purely rendering, so agents callinginspect_modelto verify their bookkeeping saw no meta column and concluded the field had been dropped.TestInspectModel) and theedit_model→ storage round-trip forModelMeasure/Aggregation(6 inTestEditModel, including replacement-not-merge semantics and the omitted-key-preserves-existing-meta contract of_upsert_entity).models.mdModelMeasure fields table + Custom Aggregations note,terminology.mdColumn entry,inspect_modelrows indocs/reference/mcp.md+docs/interfaces/mcp.md, and the meta bullet inCLAUDE.md(now includes aggregations).Closes DEV-1332.
Test plan
poetry run ruff check slayer/ tests/— cleanpoetry run pytest -m "not integration"— 1532 passedmain's rendering branches (rebased onto current HEAD before the fix); pass after🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
meta) for models, columns, measures (named formulas), and aggregations in both markdown and JSON outputs.metais supported on measures and custom aggregations for caller bookkeeping.Bug Fixes
metabullet when no entities includemeta.Documentation
metaas opaque JSON persisted with entities.Tests