fix(agents): make Agent classes frozen/slots dataclasses with required fields #2376
Conversation
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
…d fields - Add frozen=True and slots=True to AgentCore, AgentUpsert, and Agent - Make Agent read-class fields non-optional (API always returns them) - Declare _unknown_properties as dataclass field for slots compatibility - Use object.__setattr__ for frozen dataclass attribute assignment - Fix typo: 'writeableinstance' -> 'writeable instance' - Remove redundant 'Always present in API responses' from docstrings
- Remove test_load_dump_minimal (tests impossible API scenario) - Remove agent_minimal_dump fixture - Remove test_agent_with_none_tools test - Update all Agent constructor calls to provide all required parameters - Update test_load_dump_maintain_unknown_properties with required fields - Update test_tools_handling, test_agent_with_empty_tools_list - Update test_post_init_tools_validation, test_as_write - Update test_agent_labels_forward_compatibility - Update TestAgentList.test_as_write with all required fields
andeplane
changed the title
- Add @DataClass(frozen=True, slots=True) decorators to AgentCore, AgentUpsert, and Agent - Declare all fields including inherited ones (required when inheriting from regular class) - Implement __post_init__ in AgentUpsert to transform tools using object.__setattr__() - Update dump() methods to manually construct dicts (required for slots=True) - Use object.__setattr__() in _load() methods for _unknown_properties - Remove obsolete test_post_init_tools_validation test - Add labels field to test fixtures Benefits: - Memory optimization with slots=True (~40% reduction) - Immutable instances with frozen=True (thread-safe, hashable) - Maintains forward compatibility with _unknown_properties
andeplane
reviewed
Oct 17, 2025
| ) | ||
|
|
||
|
|
||
| @dataclass |
Contributor
There was a problem hiding this comment.
I think this is the pattern we have used elsewhere. Base class is not dataclass, but subclasses are.
andeplane
reviewed
Oct 17, 2025
| description (str | None): The human readable description of the agent. | ||
| instructions (str | None): Instructions for the agent. | ||
| model (str | None): Name of the language model to use. For example, "azure/gpt-4o", "gcp/gemini-2.0" or "aws/claude-3.5-sonnet". | ||
| model (str | None): Name of the language model to use. For example, "azure/gpt-4.1", "gcp/gemini-2.5-flash" or "aws/claude-4.0-sonnet". |
Contributor
There was a problem hiding this comment.
Just updated to correct and better examples
andeplane
reviewed
Oct 17, 2025
Comment on lines
+85
to
+98
| result: dict[str, Any] = {} | ||
| if self.external_id is not None: | ||
| result["externalId" if camel_case else "external_id"] = self.external_id | ||
| if self.name is not None: | ||
| result["name"] = self.name | ||
| if self.description is not None: | ||
| result["description"] = self.description | ||
| if self.instructions is not None: | ||
| result["instructions"] = self.instructions | ||
| if self.model is not None: | ||
| result["model"] = self.model | ||
| if self.labels is not None: | ||
| result["labels"] = self.labels | ||
| if self.tools is not None: |
Contributor
There was a problem hiding this comment.
Need a custom dump because of dataclass being slots=True.
The API always returns owner_id, so it should be required (str) not optional (str | None).
Changes:
- Changed owner_id type from str | None to str
- Updated dump() to always include owner_id
- Updated _load() to use resource['ownerId'] instead of resource.get('ownerId')
- Updated all tests to provide owner_id value
andeplane
reviewed
Oct 17, 2025
Comment on lines
+163
to
+174
| result: dict[str, Any] = { | ||
| "externalId" if camel_case else "external_id": self.external_id, | ||
| "name": self.name, | ||
| "description": self.description, | ||
| "instructions": self.instructions, | ||
| "model": self.model, | ||
| "labels": self.labels, | ||
| "tools": [item.dump(camel_case=camel_case) for item in self.tools], | ||
| "createdTime" if camel_case else "created_time": self.created_time, | ||
| "lastUpdatedTime" if camel_case else "last_updated_time": self.last_updated_time, | ||
| "ownerId" if camel_case else "owner_id": self.owner_id, | ||
| } |
Contributor
There was a problem hiding this comment.
Custom dump function because of slots=True.
erlendvollset
commented
Oct 17, 2025
Collaborator
Author
erlendvollset
left a comment
erlendvollset
left a comment
There was a problem hiding this comment.
Overall looks good. Just one small question on the __post_init__ method of AgentUpsert.
| # This is useful while the API is evolving and new fields are added. | ||
| self._unknown_properties: dict[str, object] = {} | ||
| def __post_init__(self) -> None: | ||
| # Transform tools sequence to AgentToolUpsertList |
Collaborator
Author
There was a problem hiding this comment.
Why is this necessary?
Contributor
There was a problem hiding this comment.
So if we don't this test fails basically assert isinstance(agent.tools, AgentToolUpsertList). It is to have agent.tools as an actual AgentToolUpsertList type and not whatever the user put in (e.g. [tool1, tool2]). We use the same in e.g. sequences.py afaik.
Collaborator
Author
There was a problem hiding this comment.
But the type is wrong now.. it's typed as a Sequence[AgentToolUpsert]. It's better then to type it as AgentToolUpsertList and add a custom __init__() which accepts Sequence[AgentToolUpsert] | AgentToolUpsertList and transforms it to AgentToolUpsertList
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This PR fixes the Agent and AgentUpsert data classes to follow SDK best practices and correct typing:
Changes Made
Added
frozen=Trueandslots=Trueto dataclassesMade Agent read-class fields non-optional
description,instructions,model,labels,tools,created_time,last_updated_timeare now required (non-optional)Properly declared
_unknown_propertiesfieldUpdated attribute assignments for frozen dataclasses
object.__setattr__()for frozen dataclass compatibilityRemoved redundant docstring text
Fixed typo: "writeableinstance" → "writeable instance"
Updated tests
Checklist: