Conversation
…returned by the Hub for registry projects
In a new realm the admin user does not have the permissions to create robots or clients.
📝 WalkthroughWalkthroughThis pull request refactors core Pydantic models in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/test_core.py (1)
363-363: Consider linking a tracking issue for the newxfailreasons.The existing
xfailat line 714 references a specific Hub issue (PrivateAIM/hub#1181). The four newxfailmarkers added here state "X is not included in this case" without a tracking link. If this is a Hub-side limitation (single-resource GET not returning related entities whenincludeis requested), please add a link to the upstream issue — otherwise these tests risk silently passing once the Hub is fixed, with no trail back to the root cause.If no upstream issue exists yet, would you like me to draft one?
Also applies to: 506-506, 553-553, 586-586
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_core.py` at line 363, Update the new pytest.mark.xfail decorators that currently use the generic reasons like "node and project are not included in this case" to include a link to the upstream tracking issue (or create one and include its URL) so future fixes don’t silently remove the regression context; locate each pytest.mark.xfail instance in tests/test_core.py (the four newly added xfail markers) and replace the reason text with a string that includes the Hub issue reference (e.g., "… (see PrivateAIM/hub#NNNN)") or add the issue URL if it’s external, and if no upstream issue exists open one and use its link in the reason.tests/test_auth.py (2)
177-177: Same xfail-tracking suggestion as intests/test_core.py.The "realm is not included in this case" reason would benefit from a linked upstream issue, consistent with the existing
xfailat line 211 (authup#2660) and friends. That way these tests will surface an actionable lead when they start unexpectedly passing after a Hub/Authup fix.Also applies to: 473-473
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_auth.py` at line 177, Update the pytest.mark.xfail decorator in tests/test_auth.py (the pytest.mark.xfail applied where the reason is "realm is not included in this case") to include a linked upstream issue identifier (e.g. "authup#2660" or a URL) in the reason string so the metadata matches the other xfail entries (like the one referenced at line 211); locate the pytest.mark.xfail usage and change its reason to something like "realm is not included in this case (authup#XXXX)" so future unexpected passes point to an actionable upstream bug.
29-30: Fixture switch tomaster_realm— heads-up on test isolation.Swapping the throwaway
realmfixture for the sharedmaster_realmmeans robot/client tests no longer create their own realm. That's fine for the current assertions, but if any future test mutates or deletes entities undermaster_realm, it could affect other tests in the same session. Consider keeping a comment in the fixture about whymaster_realmis preferred (presumably Hub v0.8.34 requires robots/clients in the master realm for the auth flows being exercised).Also applies to: 142-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_auth.py` around lines 29 - 30, The robot fixture now uses the shared master_realm (auth_client.create_robot(..., master_realm, ...)), which can harm test isolation; update the robot fixture to include a clear comment above the creation explaining why master_realm is used (e.g., Hub v0.8.34 requires robots/clients in master realm) and warning that tests must not mutate or delete master_realm entities, and add the same explanatory comment at the other place in the file where master_realm is substituted (the similar fixture/usage around lines 142-143).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flame_hub/_auth_client.py`:
- Line 77: CreateRobot.secret and CreateClient.secret lost their default None
which makes them required; restore their optional default to preserve prior
behavior by reintroducing the default value and optional marker (i.e. change the
type/annotation back to t.Annotated[str | None, IsOptionalField] = None) for the
secret field used by the CreateRobot and CreateClient models so direct
construction like CreateClient(name=..., realm_id=...) does not raise
ValidationError; apply the same fix where the other occurrence is referenced
(the second spot mentioned around the other occurrence).
- Line 506: The default for name_locked in the public API AuthClient.create_user
was flipped to False (name_locked: bool = False) which breaks existing behavior;
revert the default back to True or explicitly gate the change behind a
documented API/version bump. Update AuthClient.create_user to restore the
original default (name_locked: bool = True) or, if the False default is
intentional, add clear version-checking or a feature flag and document the
breaking change in the release notes/CHANGELOG and coordinate with the Hub
v0.8.34 server release so clients are aware of the new default.
In `@flame_hub/_core_client.py`:
- Line 199: The type of ProjectNode.approval_status was relaxed to
ProjectNodeApprovalStatus | None, which means callers must handle a None value;
update the release notes for v0.8.34 to explicitly state that approval_status
can be None and that downstream code should guard against None or adjust
signatures that currently expect a non-optional ProjectNodeApprovalStatus.
Reference the ProjectNode.approval_status field (in _core_client.py) and the
re-export via flame_hub.models when adding the note so users can find and update
any call sites that pass approval_status into functions typed as
ProjectNodeApprovalStatus.
---
Nitpick comments:
In `@tests/test_auth.py`:
- Line 177: Update the pytest.mark.xfail decorator in tests/test_auth.py (the
pytest.mark.xfail applied where the reason is "realm is not included in this
case") to include a linked upstream issue identifier (e.g. "authup#2660" or a
URL) in the reason string so the metadata matches the other xfail entries (like
the one referenced at line 211); locate the pytest.mark.xfail usage and change
its reason to something like "realm is not included in this case (authup#XXXX)"
so future unexpected passes point to an actionable upstream bug.
- Around line 29-30: The robot fixture now uses the shared master_realm
(auth_client.create_robot(..., master_realm, ...)), which can harm test
isolation; update the robot fixture to include a clear comment above the
creation explaining why master_realm is used (e.g., Hub v0.8.34 requires
robots/clients in master realm) and warning that tests must not mutate or delete
master_realm entities, and add the same explanatory comment at the other place
in the file where master_realm is substituted (the similar fixture/usage around
lines 142-143).
In `@tests/test_core.py`:
- Line 363: Update the new pytest.mark.xfail decorators that currently use the
generic reasons like "node and project are not included in this case" to include
a link to the upstream tracking issue (or create one and include its URL) so
future fixes don’t silently remove the regression context; locate each
pytest.mark.xfail instance in tests/test_core.py (the four newly added xfail
markers) and replace the reason text with a string that includes the Hub issue
reference (e.g., "… (see PrivateAIM/hub#NNNN)") or add the issue URL if it’s
external, and if no upstream issue exists open one and use its link in the
reason.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0ded6b5-d2d1-48a2-8764-48c7fa1f9e37
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.env.testflame_hub/_auth_client.pyflame_hub/_core_client.pytests/test_auth.pytests/test_core.py
| name: str | ||
| realm_id: t.Annotated[uuid.UUID, Field(), WrapValidator(uuid_validator)] | ||
| secret: t.Annotated[str, IsOptionalField] = None | ||
| secret: str | None |
There was a problem hiding this comment.
secret lost its default value — direct model construction now raises ValidationError.
Previously both CreateRobot.secret and CreateClient.secret were t.Annotated[str | None, IsOptionalField] = None. After this change they are declared as str | None with no default, making them required fields at the Pydantic level.
Inside this module the impact is nil (AuthClient.create_robot requires secret, and create_client always forwards secret=secret with its own default of None). However, CreateRobot and CreateClient are exported via flame_hub.models, so any external consumer doing e.g. CreateClient(name=..., realm_id=...) without secret= will now get a ValidationError.
♻️ Minimal fix to preserve prior behavior for direct model users
class CreateRobot(BaseModel):
name: str
realm_id: t.Annotated[uuid.UUID, Field(), WrapValidator(uuid_validator)]
- secret: str | None
+ secret: str | None = None
display_name: str | None class CreateClient(BaseModel):
name: str
- secret: str | None
+ secret: str | None = None
display_name: str | NoneIf the intent is specifically to make secret required at construction time, please ignore — but then it's worth noting in release notes alongside name_locked.
Also applies to: 238-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flame_hub/_auth_client.py` at line 77, CreateRobot.secret and
CreateClient.secret lost their default None which makes them required; restore
their optional default to preserve prior behavior by reintroducing the default
value and optional marker (i.e. change the type/annotation back to
t.Annotated[str | None, IsOptionalField] = None) for the secret field used by
the CreateRobot and CreateClient models so direct construction like
CreateClient(name=..., realm_id=...) does not raise ValidationError; apply the
same fix where the other occurrence is referenced (the second spot mentioned
around the other occurrence).
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
approval_statusfield optional for project nodes.Tests