-
Notifications
You must be signed in to change notification settings - Fork 439
Improve code coverage of mcp-agent #215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c1dec1c
53564e3
9b0e076
8f05f91
b97fd35
da1554e
8779dfb
3f5d453
4edc57f
75c85e5
741cfbd
dadcf02
47a7d8e
9211913
2ceb8dc
d38b77c
1b39119
fcbc090
32745f4
25ea5ee
a2d1dcf
4589214
6319a8b
272fa69
ae4fa5d
f3a7e0c
ef01720
21465b6
2ca05d0
8d4386d
749cad4
e570ee7
6c3a9cb
f3ded33
77ab6b4
e13dec4
86c7e41
34fa51c
1b9ddc7
e7aa41f
6f5e24f
921a1bd
404c3a3
e5a96ce
d2b218c
c927399
63d3827
ac8fbb7
da76d19
8fefd4e
7f33dc1
fd6dffe
3b28ad2
f312076
2d995f6
5a85857
0fb1eaf
56a973d
c5a01be
ec5f367
31ce0e9
ec59981
66e1915
cf75ef5
bb74646
044a5fd
f75a5c6
e72b80f
0acd277
5881f5a
2ad4a63
65de0b2
4284d6d
bab2afa
672c895
7f6fa4b
20c083e
cff4a3e
91d96d6
2f2d3ec
fc23a56
8aac5c4
931c1e3
be8e799
c43e2b9
0c4a532
5afb263
8282269
c12c995
4d3ab76
5a56dff
847062f
34f29c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,6 +181,14 @@ def serialize_type(typ: Any) -> Dict[str, Any]: | |
origin = get_origin(typ) | ||
if origin is not None: | ||
args = get_args(typ) | ||
# Special handling for Literal: store raw values, not types | ||
if origin is Literal: | ||
return { | ||
"kind": "generic", | ||
"origin": "Literal", | ||
"literal_values": [make_serializable(a) for a in args], | ||
"repr": str(typ), | ||
} | ||
serialized_args = [ | ||
PydanticTypeSerializer.serialize_type(arg) for arg in args | ||
] | ||
|
@@ -292,7 +300,7 @@ def _serialize_validators(model_class: Type[BaseModel]) -> List[Dict[str, Any]]: | |
name, | ||
validator, | ||
) in model_class.__pydantic_decorators__.field_validators.items(): | ||
field_names = [str(f) for f in validator.fields] | ||
field_names = [str(f) for f in validator.info.fields] | ||
validators.append( | ||
{ | ||
"type": "field_validator", | ||
|
@@ -382,9 +390,11 @@ def _serialize_fields(model_class: Type[BaseModel]) -> Dict[str, Dict[str, Any]] | |
"description": make_serializable( | ||
getattr(field_info, "description", None) | ||
), | ||
"required": make_serializable( | ||
getattr(field_info, "required", True) | ||
), | ||
"required": getattr( | ||
field_info, | ||
"is_required", | ||
lambda: getattr(field_info, "required", True), | ||
)(), | ||
} | ||
|
||
# Add constraints if defined | ||
|
@@ -411,10 +421,10 @@ def _serialize_fields(model_class: Type[BaseModel]) -> Dict[str, Dict[str, Any]] | |
else: | ||
default = make_serializable(default) | ||
|
||
# Use type_ if available (Pydantic v2), else fallback to Any | ||
attr_type = getattr(private_attr, "type_", Any) | ||
private_attrs[name] = { | ||
"type": PydanticTypeSerializer.serialize_type( | ||
private_attr.annotation | ||
), | ||
"type": PydanticTypeSerializer.serialize_type(attr_type), | ||
"default": default, | ||
} | ||
|
||
|
@@ -436,7 +446,18 @@ def _serialize_config(model_class: Type[BaseModel]) -> Dict[str, Any]: | |
else: | ||
return config_dict | ||
|
||
# Extract serializable config values | ||
# If config_source is a dict or ConfigDict (Pydantic v2), just copy its items | ||
if isinstance(config_source, dict): | ||
for key, value in config_source.items(): | ||
if not str(key).startswith("_"): | ||
try: | ||
json.dumps({key: value}) | ||
config_dict[key] = value | ||
except (TypeError, OverflowError): | ||
config_dict[key] = str(value) | ||
return config_dict | ||
|
||
# Otherwise, use inspect.getmembers (for class-based config) | ||
for key, value in inspect.getmembers(config_source): | ||
if ( | ||
not key.startswith("_") | ||
|
@@ -516,6 +537,12 @@ def deserialize_type(serialized: Dict[str, Any]) -> Any: | |
elif kind == "generic": | ||
# Handle generics like List[int], Dict[str, Model], etc. | ||
origin_name = serialized["origin"] | ||
|
||
# Special handling for Literal: use literal_values if present | ||
if origin_name == "Literal" and "literal_values" in serialized: | ||
literal_values = serialized["literal_values"] | ||
return Literal.__getitem__(tuple(literal_values)) | ||
|
||
args = [ | ||
PydanticTypeSerializer.deserialize_type(arg) | ||
for arg in serialized["args"] | ||
|
@@ -621,15 +648,13 @@ def reconstruct_model(serialized: Dict[str, Any]) -> Type[BaseModel]: | |
# Get the field type | ||
field_type = PydanticTypeSerializer.deserialize_type(field_info["type"]) | ||
|
||
# Handle default values | ||
# Determine if the field is required | ||
is_required = field_info.get("required", True) | ||
default = field_info.get("default", ...) | ||
if default is None and not field_info.get("required", True): | ||
default = None | ||
|
||
# Handle default factories | ||
default_factory = field_info.get("default_factory") | ||
|
||
# This logic ensures that fields with a default or default_factory are not required | ||
if default_factory: | ||
# This is a simplification - ideally we'd recreate the actual factory | ||
if default_factory == "list": | ||
default_factory = list | ||
elif default_factory == "dict": | ||
|
@@ -658,25 +683,61 @@ def reconstruct_model(serialized: Dict[str, Any]) -> Type[BaseModel]: | |
|
||
# Add the field definition | ||
if constraints or default_factory: | ||
# If there is a default_factory, always use default=... and set default_factory | ||
field_definitions[field_name] = ( | ||
field_type, | ||
Field( | ||
default=default if default_factory is None else ..., | ||
default=... if default_factory is not None else default, | ||
default_factory=default_factory, | ||
**constraints, | ||
), | ||
) | ||
else: | ||
field_definitions[field_name] = (field_type, default) | ||
if is_required: | ||
field_definitions[field_name] = (field_type, Field(default=...)) | ||
else: | ||
field_definitions[field_name] = ( | ||
field_type, | ||
Field( | ||
default=default, | ||
), | ||
) | ||
|
||
Comment on lines
687
to
705
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Avoid passing
- Field(
- default=... if default_factory is not None else default,
- default_factory=default_factory,
+ kwargs = {**constraints}
+ if default_factory is not None:
+ kwargs["default_factory"] = default_factory
+ kwargs["default"] = ...
+ elif default is not ...:
+ kwargs["default"] = default
+ else:
+ kwargs["default"] = ...
+ Field(**kwargs),
🤖 Prompt for AI Agents
|
||
# Create model config | ||
model_config = ConfigDict(**config_dict) if config_dict else None | ||
|
||
# Create the basic model | ||
# Collect private attributes to pass to create_model | ||
private_attr_kwargs = {} | ||
if "__private_attrs__" in fields: | ||
for name, attr_info in fields["__private_attrs__"].items(): | ||
default = attr_info.get("default") | ||
if default == "None": | ||
default = None | ||
private_attr_kwargs[name] = PrivateAttr(default=default) | ||
|
||
# Create the basic model, including private attributes in the class namespace | ||
reconstructed_model = create_model( | ||
name, __config__=model_config, **field_definitions | ||
name, __config__=model_config, **field_definitions, **private_attr_kwargs | ||
) | ||
|
||
# Patch __init__ to ensure private attributes are initialized on instance | ||
private_attrs = getattr(reconstructed_model, "__private_attributes__", {}) | ||
if private_attrs: | ||
orig_init = reconstructed_model.__init__ | ||
|
||
def _init_with_private_attrs(self, *args, **kwargs): | ||
orig_init(self, *args, **kwargs) | ||
for attr_name, private_attr in private_attrs.items(): | ||
# Only set if not already set | ||
if not hasattr(self, attr_name): | ||
default = private_attr.default | ||
# If default is ... (Ellipsis), treat as None | ||
if default is ...: | ||
default = None | ||
setattr(self, attr_name, default) | ||
|
||
reconstructed_model.__init__ = _init_with_private_attrs | ||
Comment on lines
+723
to
+739
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Monkey-patching Overwriting
Prefer letting Pydantic handle private attributes: - private_attrs = getattr(reconstructed_model, "__private_attributes__", {})
- if private_attrs:
- orig_init = reconstructed_model.__init__
- def _init_with_private_attrs(self, *args, **kwargs):
- orig_init(self, *args, **kwargs)
- ...
- reconstructed_model.__init__ = _init_with_private_attrs
+ # Pydantic takes care of initialising PrivateAttr defaults; no patch needed. If a specific bug required this hook, add a comment with a minimal
🤖 Prompt for AI Agents
|
||
|
||
# Add validators (this gets complex and may require exec/eval) | ||
if validators: | ||
for validator in validators: | ||
|
@@ -719,17 +780,6 @@ def reconstruct_model(serialized: Dict[str, Any]) -> Type[BaseModel]: | |
except Exception as e: | ||
logger.error(f"Error recreating validator: {e}") | ||
|
||
# Add private attributes (simplified) | ||
if "__private_attrs__" in fields: | ||
for name, attr_info in fields["__private_attrs__"].items(): | ||
attr_type = PydanticTypeSerializer.deserialize_type(attr_info["type"]) | ||
default = attr_info.get("default") | ||
if default == "None": | ||
default = None | ||
reconstructed_model.__private_attributes__[name] = PrivateAttr( | ||
default=default, annotation=attr_type | ||
) | ||
|
||
return reconstructed_model | ||
|
||
@classmethod | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -253,7 +253,12 @@ def __init__( | |
|
||
self.agent = Agent( | ||
name=self.name, | ||
instruction=self.instruction, | ||
# Only pass instruction if it's not None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are there cases where instruction is None? Should we fail early in that case? |
||
**( | ||
{"instruction": self.instruction} | ||
if self.instruction is not None | ||
else {} | ||
), | ||
server_names=server_names or [], | ||
llm=self, | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean _handlers is a tuple?