Conversation
This reverts commit b905d6d.
…-models' into 112-update-analysis-hub-response-models # Conflicts: # poetry.lock # pyproject.toml
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors authentication from RobotAuth to ClientAuth with env var renames (HUB_ROBOT_* → HUB_NODE_CLIENT_*), renames pod statuses (RUNNING→EXECUTING, FINISHED→EXECUTED), updates autostart status criteria, synchronizes docs/tests, updates API metadata/port and minor route/path changes, and bumps client dependency/tooling metadata. Changes
Sequence Diagram(s)(Skipped — changes are refactors, renames, metadata and tests updates, not a new multi-component sequential flow requiring visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
hub_adapter/routers/hub.py (1)
69-69:⚠️ Potential issue | 🔴 CriticalPre-existing security risk:
eval()on user-supplied query parameter.
eval(page_params)can execute arbitrary Python code if a malicious string is passed as thepagequery parameter. This is a code injection vulnerability. Consider replacing withjson.loads()or a safer parser.🔒 Proposed fix
+import json + ... if page_params: - page_param_dict: dict = eval(page_params) + page_param_dict: dict = json.loads(page_params) limit = page_param_dict.get("limit") or 50 offset = page_param_dict.get("offset") or 0 formatted_query_params["page"] = {"limit": limit, "offset": offset}tests/test_dependencies.py (1)
192-206:⚠️ Potential issue | 🟠 MajorAssertions inside
pytest.raisescontext managers may be silently skipped.Lines 195-196, 205-206, and 213-214 place
assertstatements insidewith pytest.raises(...)blocks after the call that raises. Once the exception is raised, the remaining lines in thewithblock are skipped. These assertions will never execute.Move assertions outside the
withblock using theasalias (already captured in some cases, e.g.,badUser), similar to how it's done intest_hub_auth_flow(lines 93-94).🐛 Example fix for lines 192-196
- with pytest.raises(HTTPException) as err: - get_registry_metadata_for_url(TEST_MOCK_NODE, self.cc) - - assert err.value.status_code == status.HTTP_400_BAD_REQUEST - assert err.value.detail["message"] == "No external name for node found" + with pytest.raises(HTTPException) as err: + get_registry_metadata_for_url(TEST_MOCK_NODE, self.cc) + + assert err.value.status_code == status.HTTP_400_BAD_REQUEST + assert err.value.detail["message"] == "No external name for node found"The same pattern needs to be fixed for the blocks at lines 202-206, 210-214, and 150-154.
hub_adapter/dependencies.py (1)
243-252:⚠️ Potential issue | 🟡 MinorUpdated error message is semantically inaccurate.
The error at line 247 fires when
account_nameoraccount_secretis missing from the registry project. The message "Unable to retrieve node client ID or secret from the registry" conflates registry account credentials with node client credentials (HUB_NODE_CLIENT_ID/HUB_NODE_CLIENT_SECRET), which are unrelated. A more precise message would be something like "Unable to retrieve registry account credentials."📝 Suggested message fix
- "message": "Unable to retrieve node client ID or secret from the registry", + "message": "Unable to retrieve registry account name or secret",
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Line 14: The pyproject dependency constraint for flame-hub-client is incorrect
(references nonexistent 0.2.11); update the version specifier in pyproject.toml
for the flame-hub-client dependency from "^0.2.11" to "^0.2.10" so the project
uses the latest available release and resolves installation failures.
🧹 Nitpick comments (4)
hub_adapter/errors.py (1)
162-173: Consider usinglogger.exceptioninstead oflogger.errorto capture the traceback.When catching
pydantic.ValidationError, the traceback is valuable for debugging which field/model failed validation.logger.exceptionautomatically includes it.Proposed fix
except pydantic.ValidationError as e: - logger.error(f"Pydantic type error: {e.errors()}") + logger.exception(f"Pydantic type error: {e.errors()}") raise HubTypeError(tests/constants.py (1)
56-102: Test data centralization is a good refactor. Consider using distinct UUIDs for different fields.The same UUID (
ac776c7f-...) is reused forid,realm_id,user_id,analysis_realm_id, andnode_realm_id. While this works for current tests, it could mask bugs where the wrong field is accidentally used (e.g.,analysis_realm_idinstead ofid). Using distinct UUIDs for semantically different fields would make tests more robust.tests/test_dependencies.py (1)
11-11: Importing from a private module (_auth_flows) is fragile.
ClientAuthis only available from the privateflame_hub._auth_flowsmodule—it is not re-exported from public APIs likeflame_huborflame_hub.models. This applies to both the test file andhub_adapter/dependencies.py(line 16). Sinceflame_hubis a pinned dependency with^0.2.11allowance for minor updates, internal module reorganization in a future version could break these imports.Consider whether
ClientAuthcould be inferred from type stubs, documentation, or future library updates that expose it publicly.hub_adapter/dependencies.py (1)
16-17:ClientAuthis imported from a private module.
flame_hub._auth_flowsis a private module (indicated by the leading underscore in the module name). The flame-hub-client library's public API exposes authentication viaPasswordAuthandRobotAuthinstead. When flame-hub updates its API to exposeClientAuthpublicly, prefer importing from the public path to avoid breaking changes.
# Conflicts: # README.md # hub_adapter/conf.py # hub_adapter/dependencies.py # poetry.lock # pyproject.toml # tests/router_tests/test_hub.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/conftest.py (1)
1-1:⚠️ Potential issue | 🟠 MajorPipeline failure: missing
node_event_loggingdependency.The CI is failing with
ImportError: failed to import hub_adapter.server due to missing dependency 'node_event_logging'. This is likely unrelated to the auth migration but will block merging.
🤖 Fix all issues with AI agents
In `@hub_adapter/errors.py`:
- Around line 33-35: The file defines HubTypeError twice which shadows the
original class used by catch_hub_errors; remove the duplicate class definition
so only the first HubTypeError (subclassing HTTPException) remains and ensure
catch_hub_errors continues to reference that single definition (no other symbol
changes required).
🧹 Nitpick comments (1)
hub_adapter/dependencies.py (1)
48-48: Stale docstring still references "robot token".The docstring says "getting a robot token from the central Hub service" but this function now uses
ClientAuthwith node client credentials.Proposed fix
- """Automated method for getting a robot token from the central Hub service.""" + """Automated method for getting a client token from the central Hub service."""
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Line 29: Update the ruff version constraint in pyproject.toml: replace the
incorrect "ruff = \"^0.1.8\"" with the correct modern constraint (e.g., "ruff =
\"^0.15.0\"" or at minimum the previous "0.14.x" line) so the dependency
resolves to the current ruff 0.x release; locate the ruff entry in
pyproject.toml and change the version string accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@hub_adapter/server.py`:
- Line 143: The default port was changed in the deploy function (async def
deploy(host: str = "127.0.0.1", port: int = 5000, reload: bool = False)) but
documentation and test artifacts still reference 8081; update README.md to use
http://127.0.0.1:5000, update tests/constants.py to replace all occurrences of
http://localhost:8081/ (lines noted in the review) with http://localhost:5000/
(or use the deploy default via a shared constant/env var), and adjust
docker/test-realm.json entries that embed port 8081 to 5000 (or make them
configurable) so docs/tests match the new deploy default.
🧹 Nitpick comments (1)
hub_adapter/server.py (1)
60-76: Consider makingserversconfigurable and using an organizational email.Two minor observations:
Hardcoded server URL (line 71):
http://localhost:5000is fine for local dev, but in deployed environments this will appear in the OpenAPI spec and could confuse consumers. Consider populating this from settings or omitting it so FastAPI defaults to the request's origin.Personal email in contact (line 64): Using a personal Gmail in public API metadata may not age well if maintainership changes. Consider using a project or organization email.
# Conflicts: # pyproject.toml
# Conflicts: # poetry.lock # pyproject.toml
# Conflicts: # hub_adapter/autostart.py # hub_adapter/conf.py # hub_adapter/dependencies.py # hub_adapter/routers/kong.py # hub_adapter/server.py # tests/conftest.py # tests/test_dependencies.py # uv.lock
1 participant
Summary by CodeRabbit
Documentation
Changes