MCP tools fixes#253
Conversation
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe changes update compact inspect output so measures and aggregations retain full text, rename model inspection sample sections to “Data Profile,” and relax entity-reference validation to accept hyphens within identifier segments. ChangesInspect rendering
Entity resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
slayer/memories/resolver.py (1)
298-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse keyword arguments for the regex call.
Line 302 passes multiple arguments positionally, contrary to the Python guideline. Prefer
re.fullmatch(pattern=..., string=s)here; it preserves the intended validation and avoids relying on anchors.Suggested change
- if not all(re.match(r"^[a-zA-Z_][\w-]*$", s) for s in segments): + if not all( + re.fullmatch(pattern=r"[a-zA-Z_][\w-]*", string=s) + for s in segments + ):🤖 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 `@slayer/memories/resolver.py` around lines 298 - 302, Update the segment validation in the resolver to use re.fullmatch with explicit pattern and string keyword arguments instead of re.match with anchored positional arguments. Preserve the existing identifier rules, including leading letters or underscores and subsequent word characters or hyphens.Source: Coding guidelines
tests/test_entity_resolution.py (2)
320-330: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise a registered hyphenated datasource.
This test only proves that an unknown hyphenated segment bypasses the validation error. Add a fixture with a real hyphenated datasource and assert canonical resolution, or the datasource-specific missing-model error, so the intended lookup path is covered.
🤖 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 `@tests/test_entity_resolution.py` around lines 320 - 330, Extend test_hyphenated_datasource_segment_accepted with a registered datasource fixture whose name contains a hyphen, then resolve that datasource using the existing entity-resolution setup. Assert either the canonical resolved entity or the datasource-specific missing-model error, ensuring the request reaches datasource lookup rather than only avoiding the invalid identifier segment error.
320-330: 📐 Maintainability & Code Quality | 🔵 TrivialRun
poetry run ruff check slayer/ tests/before merge and fix any reported issues.🤖 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 `@tests/test_entity_resolution.py` around lines 320 - 330, Run ruff against the slayer/ and tests/ directories and resolve every reported issue, including any violations in test_hyphenated_datasource_segment_accepted, while preserving the test’s intended entity-resolution behavior.Source: Coding guidelines
🤖 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 `@slayer/memories/resolver.py`:
- Around line 298-302: Update the segment validation in the resolver to use
re.fullmatch with explicit pattern and string keyword arguments instead of
re.match with anchored positional arguments. Preserve the existing identifier
rules, including leading letters or underscores and subsequent word characters
or hyphens.
In `@tests/test_entity_resolution.py`:
- Around line 320-330: Extend test_hyphenated_datasource_segment_accepted with a
registered datasource fixture whose name contains a hyphen, then resolve that
datasource using the existing entity-resolution setup. Assert either the
canonical resolved entity or the datasource-specific missing-model error,
ensuring the request reaches datasource lookup rather than only avoiding the
invalid identifier segment error.
- Around line 320-330: Run ruff against the slayer/ and tests/ directories and
resolve every reported issue, including any violations in
test_hyphenated_datasource_segment_accepted, while preserving the test’s
intended entity-resolution behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4cc3373b-7512-42b9-a920-b968b0b73dd9
📒 Files selected for processing (4)
slayer/inspect/service.pyslayer/memories/resolver.pytests/test_entity_resolution.pytests/test_inspect.py
|
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests