Repository navigation
feat(generated): AdminPortal, Agents, ApiKeys, AuditLogs, Authorization, ClientApi, Connect, DirectorySync, Events, FeatureFlags, Groups, MultiFactorAuth, OrganizationDomains, OrganizationMembership, Organizations, Pipes, PipesProvider, PlatformTeams, Radar, SSO, UserManagement, Vault, Webhooks, Widgets (batch aeefef11) - #752
Merged
Merged
Conversation
…iKeys, AuditLogs, Authorization, ClientApi, Connect, DirectorySync, Events, FeatureFlags, Groups, MultiFactorAuth, OrganizationDomains, OrganizationMembership, Organizations, Pipes, PipesProvider, PlatformTeams, Radar, SSO, UserManagement, Vault, Webhooks, Widgets
Contributor
 [Medium impact] Auto-generated SDK models and API methods for feature flags and agents. Fix the removed public export and required constructor argument before merging. Findings
Fix with agent prompt### Issue 1
src/workos/authorization/models/__init__.py:9
Removing `ListMetadata` breaks existing imports from `workos.authorization.models`. It also removes the name from `workos.authorization`, which forwards these exports. Applications using either import now fail with `ImportError`. Keep the existing export alongside the new export in `agents`.
```suggestion
from workos.common.models.list_metadata import ListMetadata as ListMetadata
from workos.common.models.slim_role import SlimRole as SlimRole
```
### Issue 2
src/workos/agents/models/agent_registration.py:30
Adding `revoked_at` without a default breaks existing `AgentRegistration(...)` calls. A caller supplying every previously required argument now gets `TypeError` because `revoked_at` is missing. Make the new field keyword-only with a default of `None`, or append it with that default, to preserve existing constructor calls.
### Issue 3
src/workos/common/models/flag_updated_context_previous_attribute_data.py:62-65
Serializing previous flag attributes now adds `"owner": null` even when the input omitted `owner`. These attributes describe fields that changed, so an update that only changed `enabled` should not gain a previous-owner entry. Callers that serialize parsed events receive a value the event never supplied. Preserve the difference between an omitted `owner` and an explicit null, and test a round trip where `owner` is absent.
### Issue 4
tests/test_agents.py:205-212
Both new revoke tests return an unrevoked registration fixture, and neither checks `revoked_at`. The added model tests also use only null timestamps, so the new timestamp parsing and formatting paths are never exercised. Return a non-null timestamp in the revoke tests and check both the parsed value and its serialized form.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds registration revocation to the sync and async SDK clients.
Reviews (1) · Last reviewed commit: "chore(generated): add release notes frag..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| AuthorizationPermission as AuthorizationPermission, | ||
| ) | ||
| from workos.common.models.list_metadata import ListMetadata as ListMetadata | ||
| from workos.common.models.slim_role import SlimRole as SlimRole |
Contributor
There was a problem hiding this comment.
Removing ListMetadata breaks existing imports from workos.authorization.models. It also removes the name from workos.authorization, which forwards these exports. Applications using either import now fail with ImportError. Keep the existing export alongside the new export in agents.
Suggested change
| from workos.common.models.slim_role import SlimRole as SlimRole | |
| from workos.common.models.list_metadata import ListMetadata as ListMetadata | |
| from workos.common.models.slim_role import SlimRole as SlimRole |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/workos/authorization/models/__init__.py
Line: 9
Comment:
**Existing authorization imports fail**
Removing `ListMetadata` breaks existing imports from `workos.authorization.models`. It also removes the name from `workos.authorization`, which forwards these exports. Applications using either import now fail with `ImportError`. Keep the existing export alongside the new export in `agents`.
```suggestion
from workos.common.models.list_metadata import ListMetadata as ListMetadata
from workos.common.models.slim_role import SlimRole as SlimRole
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| """Identifier of the organization the agent is registered to.""" | ||
| status: AgentRegistrationStatus | ||
| """The current verification status of the registration.""" | ||
| revoked_at: datetime | None |
Contributor
There was a problem hiding this comment.
Adding revoked_at without a default breaks existing AgentRegistration(...) calls. A caller supplying every previously required argument now gets TypeError because revoked_at is missing. Make the new field keyword-only with a default of None, or append it with that default, to preserve existing constructor calls.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/workos/agents/models/agent_registration.py
Line: 30
Comment:
**Existing registration constructors fail**
Adding `revoked_at` without a default breaks existing `AgentRegistration(...)` calls. A caller supplying every previously required argument now gets `TypeError` because `revoked_at` is missing. Make the new field keyword-only with a default of `None`, or append it with that default, to preserve existing constructor calls.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+62
to
+65
| if self.owner is not None: | ||
| result["owner"] = self.owner.to_dict() | ||
| else: | ||
| result["owner"] = None |
Contributor
There was a problem hiding this comment.
Serializing previous flag attributes now adds "owner": null even when the input omitted owner. These attributes describe fields that changed, so an update that only changed enabled should not gain a previous-owner entry. Callers that serialize parsed events receive a value the event never supplied. Preserve the difference between an omitted owner and an explicit null, and test a round trip where owner is absent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/workos/common/models/flag_updated_context_previous_attribute_data.py
Line: 62-65
Comment:
**Unchanged owners become null**
Serializing previous flag attributes now adds `"owner": null` even when the input omitted `owner`. These attributes describe fields that changed, so an update that only changed `enabled` should not gain a previous-owner entry. Callers that serialize parsed events receive a value the event never supplied. Preserve the difference between an omitted `owner` and an explicit null, and test a round trip where `owner` is absent.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+205
to
+212
| def test_revoke_registration(self, workos, httpx_mock): | ||
| httpx_mock.add_response( | ||
| json=load_fixture("agent_registration.json"), | ||
| ) | ||
| result = workos.agents.revoke_registration("test_id") | ||
| assert isinstance(result, AgentRegistration) | ||
| assert result.id == "agent_reg_01EHWNCE74X7JSDV0X3SZ3KJNY" | ||
| assert result.organization_id == "org_01EHQMYV6MBK39QC5PZXHY59C3" |
Contributor
There was a problem hiding this comment.
Both new revoke tests return an unrevoked registration fixture, and neither checks revoked_at. The added model tests also use only null timestamps, so the new timestamp parsing and formatting paths are never exercised. Return a non-null timestamp in the revoke tests and check both the parsed value and its serialized form.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_agents.py
Line: 205-212
Comment:
**Revoked timestamps go untested**
Both new revoke tests return an unrevoked registration fixture, and neither checks `revoked_at`. The added model tests also use only null timestamps, so the new timestamp parsing and formatting paths are never exercised. Return a non-null timestamp in the revoke tests and check both the parsed value and its serialized form.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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
Regenerated SDK from spec changes.
Triggered by workos/openapi-spec@26ae9d6