If access_token -> then app_key is not required#168
Conversation
- the fetch_llm_config call was synchronous, and if the server calling flo-ai and base_url are same, that was causing a deadlock.
# Conflicts: # flo_ai/flo_ai/llm/rootflo_llm.py
WalkthroughRelaxed validation for RootFlo access-token flows: JWT parameters ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LLMFactory
participant RootFloLLM
rect rgb(245,250,255)
Note over LLMFactory: Creation request
User->>LLMFactory: create(base_url, model_id, access_token?, app_key?, ...)
alt access_token provided
LLMFactory->>RootFloLLM: init(base_url, model_id, access_token, app_key=None...)
RootFloLLM->>RootFloLLM: _ensure_initialized(custom_headers={})
RootFloLLM->>RootFloLLM: include X-Rootflo-Key only if app_key present
RootFloLLM-->>User: instance (access-token flow)
else no access_token
LLMFactory->>RootFloLLM: init(base_url, model_id, app_key, app_secret, issuer, audience)
RootFloLLM->>RootFloLLM: _fetch_llm_config_async(base_url, model_id, api_token, app_key)
RootFloLLM-->>User: instance (JWT flow)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (5)
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: 0
🧹 Nitpick comments (1)
flo_ai/flo_ai/helpers/llm_factory.py (1)
205-228: Access-token flow correctly relaxes requirements to justbase_urlThe new access-token branch now only enforces
base_urland no longer requiresapp_key/JWT fields, which matches the PR objective and keeps the JWT path strict via theif not access_tokenbranch. The error message also clearly documents all sources forbase_url(model_config, kwargs, env). As a minor polish (optional), you might mirror the “model_config” wording in the JWT-branch error text for consistency, but functionally this looks solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flo_ai/flo_ai/helpers/llm_factory.py(1 hunks)flo_ai/flo_ai/llm/rootflo_llm.py(5 hunks)
🔇 Additional comments (3)
flo_ai/flo_ai/llm/rootflo_llm.py (3)
31-85: Constructor changes safely support access-token–only initializationMaking
app_key,app_secret,issuer, andaudienceOptional[str] = Nonewhile keeping theif not access_tokenvalidation preserves strict requirements for the JWT path but allows constructingRootFloLLMwith onlybase_url,model_id, and anaccess_token. This is backward compatible with existing call sites and aligns with the “access_token → app_key not required” goal.
116-146: Optionalapp_keyfor config fetch avoids bogus header valuesThe updated
_fetch_llm_config_asyncsignature and header construction only addX-Rootflo-Keywhenapp_keyis truthy. This prevents sending an invalidX-Rootflo-Key: Noneheader when using access-token–only auth, while still supporting scenarios where bothaccess_tokenandapp_keyare supplied.
233-235: Proxycustom_headersbehavior matches access-token semanticsBuilding
custom_headersas{'X-Rootflo-Key': self._app_key}only when_app_keyis set keeps downstream SDK wrappers consistent with the config-fetch behavior: access-token–only flows send no X-Rootflo-Key, while flows with an app_key continue to propagate it. This is a clean and minimal change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
flo_ai/flo_ai/llm/rootflo_llm.py (2)
68-84: Consider logging when JWT credentials are ignored.When both
access_tokenand JWT credentials are provided, the JWT credentials are silently ignored. While the docstring mentions this behavior (line 53), adding a debug log or warning could improve developer experience by making this precedence explicit at runtime.Example:
if not access_token: missing = [] if not app_key: missing.append('app_key') if not app_secret: missing.append('app_secret') if not issuer: missing.append('issuer') if not audience: missing.append('audience') if missing: raise ValueError( f'Missing required parameters for JWT generation: {", ".join(missing)}. ' f'Either provide these parameters or pass an access_token directly.' ) +else: + # Log if JWT credentials are provided but will be ignored + if any([app_key, app_secret, issuer, audience]): + import logging + logging.debug("JWT credentials provided but will be ignored because access_token is present")
233-238: Minor: Redundant parentheses in ternary expression.The outer parentheses in the ternary expression are unnecessary but don't affect functionality.
-custom_headers = ( - {'X-Rootflo-Key': self._app_key} - if (self._app_key and self._access_token is None) - else {} -) +custom_headers = ( + {'X-Rootflo-Key': self._app_key} + if self._app_key and self._access_token is None + else {} +)Note: The logic itself is correct and consistent with the header construction in
_fetch_llm_config_async.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flo_ai/flo_ai/helpers/llm_factory.py(1 hunks)flo_ai/flo_ai/llm/rootflo_llm.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flo_ai/flo_ai/helpers/llm_factory.py
🔇 Additional comments (3)
flo_ai/flo_ai/llm/rootflo_llm.py (3)
31-42: LGTM: Constructor signature correctly supports dual authentication paths.The optional JWT parameters (app_key, app_secret, issuer, audience) align with the PR objective and enable both JWT-based and access_token-based authentication flows.
192-207: LGTM: JWT generation is safely guarded by validation.The JWT generation code only executes when
access_tokenis not provided, and the constructor validation (lines 68-84) ensures all required JWT credentials are present in this scenario.
116-146: LGTM: Header construction correctly distinguishes authentication modes.The conditional logic for
X-Rootflo-Keyheader (lines 143-145) appropriately adds the header only in JWT authentication mode (whenapp_keyis provided andaccess_tokenis not used).
This reverts commit baedf01.
* rootflo_llm -> lazy fetching llm config - the fetch_llm_config call was synchronous, and if the server calling flo-ai and base_url are same, that was causing a deadlock. * llm_factory -> added openai_vllm * added OpenAIVLLM to rootflo_llm * resolved comments * resolved comments v2 * resolved comments v3 * if access_token -> then app_key not required * if access_token -> passing other jwt params as None * Revert "if access_token -> passing other jwt params as None" This reverts commit baedf01.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.