Treat explicit null defaults as nullable in MCP schema translation#1665
Conversation
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDetects fields that explicitly default to None and, for optional fields, widens their inferred type to include nullability so the model annotation permits None; adds a test verifying this behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ok to test fa66f98 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/nvidia_nat_mcp/tests/client/test_mcp_schema.py (1)
884-890: Consider adding a test case for omitted field defaulting to None.The test validates explicit
Noneand string values, but doesn't verify that omitting the field entirely uses theNonedefault. This would make the test more complete.🧪 Suggested additional assertion
# Test string value validates m2 = _model.model_validate({"cursor": "abc123"}) assert m2.cursor == "abc123" + + # Test omitted field defaults to None + m3 = _model.model_validate({}) + assert m3.cursor is None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nvidia_nat_mcp/tests/client/test_mcp_schema.py` around lines 884 - 890, Add a test that verifies omitting the "cursor" field results in the default None value: call _model.model_validate with an empty dict (or without "cursor"), assign to e.g. m3, and assert m3.cursor is None so the schema defaults missing cursor to None; update the test near the existing m1/m2 assertions in test_mcp_schema.py to include this omitted-field check referencing _model and model_validate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/nvidia_nat_mcp/tests/client/test_mcp_schema.py`:
- Around line 884-890: Add a test that verifies omitting the "cursor" field
results in the default None value: call _model.model_validate with an empty dict
(or without "cursor"), assign to e.g. m3, and assert m3.cursor is None so the
schema defaults missing cursor to None; update the test near the existing m1/m2
assertions in test_mcp_schema.py to include this omitted-field check referencing
_model and model_validate.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/utils.pypackages/nvidia_nat_mcp/tests/client/test_mcp_schema.py
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
|
/ok to test 1bc3274 |
|
/merge |
…VIDIA#1665) Adds a temporary MCP schema-translation workaround so fields with an explicit default: null are treated as nullable, even when the schema type does not explicitly include null. ## By Submitting this PR I confirm: - I am familiar with the [Contributing Guidelines](https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/docs/source/resources/contributing/index.md). - We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license. - Any contribution which contains commits that are not Signed-Off will not be accepted. - When the PR is ready for review, new or existing tests cover these changes. - When the PR is ready for review, the documentation is up to date with these changes. ## Summary by CodeRabbit * **Bug Fixes** * Schema processing now treats optional fields that explicitly default to null as nullable, so validators accept both the original type and null as valid values. * **Tests** * New tests confirm optional fields with explicit null defaults validate correctly for null and for values of the declared type. Authors: - Anuradha Karuppiah (https://github.com/AnuradhaKaruppiah) Approvers: - Will Killian (https://github.com/willkill07) URL: NVIDIA#1665
Description
Adds a temporary MCP schema-translation workaround so fields with an explicit default: null are treated as nullable, even when the schema type does not explicitly include null.
By Submitting this PR I confirm:
Summary by CodeRabbit
Bug Fixes
Tests