Add alternative to proprietary firebase dependency#984
Open
nikochiko wants to merge 23 commits into
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR adds a FIREBASE_ENABLED toggle and conditionally initializes Firebase. It implements a filesystem-backed storage fallback and updates image/GCS helpers to use the new storage backend. AppUser gains localauth_password and unique email, with creation logic branching on FIREBASE_ENABLED; migrations added. The auth backend now verifies either Firebase session cookies or local session cookies. A new auth/auth_ui module provides Firebase and local auth routers; routers/root conditionally includes the appropriate router. Templates and header include Firebase scripts only when enabled. Other edits: server static mounting when Firebase is off, workspace URL construction changes, minor UI/style tweaks, and related provider config checks, tests, and documentation. Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
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)
auth/auth_backend.py (1)
68-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winException handling is incomplete for
verify_session_cookie.The current exception handler catches
InvalidSessionCookieError, which coversExpiredSessionCookieErrorandRevokedSessionCookieError(both subclasses). However, it missesCertificateFetchError(network/certificate issues) andUserDisabledError(when check_revoked=True), which would propagate as unhandled exceptions.Consider adding the missing exception types:
Proposed fix
try: decoded_claims = auth.verify_session_cookie(firebase_cookie) return AppUser.objects.get_or_create_from_uid(decoded_claims["uid"])[0] - except (auth.UserNotFoundError, auth.InvalidSessionCookieError): + except ( + auth.UserNotFoundError, + auth.InvalidSessionCookieError, + auth.UserDisabledError, + auth.CertificateFetchError, + ): # Session cookie is invalid, expired or revoked. Force user to login. return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/auth_backend.py` around lines 68 - 72, The current exception handler around auth.verify_session_cookie in auth_backend.py only catches auth.UserNotFoundError and auth.InvalidSessionCookieError; update the except tuple to also catch auth.CertificateFetchError and auth.UserDisabledError so network/certificate fetch failures and disabled-user errors are handled the same way (returning None). Locate the block that calls verify_session_cookie and modify the except clause that references auth.UserNotFoundError and auth.InvalidSessionCookieError to include auth.CertificateFetchError and auth.UserDisabledError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app_users/models.py`:
- Around line 50-60: The branch that constructs a new user with
self.model(**kwargs) is missing the required BooleanField is_anonymous (causing
IntegrityError); update the user creation in that branch (where user =
self.model(**kwargs) / user.save()) to explicitly set is_anonymous=False
(matching get_or_create_from_email behavior), e.g. add is_anonymous=False into
the kwargs or pass it when instantiating the model so new local-auth users are
created with is_anonymous set.
---
Outside diff comments:
In `@auth/auth_backend.py`:
- Around line 68-72: The current exception handler around
auth.verify_session_cookie in auth_backend.py only catches
auth.UserNotFoundError and auth.InvalidSessionCookieError; update the except
tuple to also catch auth.CertificateFetchError and auth.UserDisabledError so
network/certificate fetch failures and disabled-user errors are handled the same
way (returning None). Locate the block that calls verify_session_cookie and
modify the except clause that references auth.UserNotFoundError and
auth.InvalidSessionCookieError to include auth.CertificateFetchError and
auth.UserDisabledError.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2afe772-482e-4522-a930-5bca9ad3bbc7
📒 Files selected for processing (9)
app_users/models.pyauth/auth_backend.pydaras_ai_v2/db.pydaras_ai_v2/settings.pygooey-gui/app/styles/app.cssrouters/root.pytemplates/header.htmltemplates/login_scripts.htmlworkspaces/models.py
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
daras_ai_v2/storage_backends.py (2)
59-69: ⚖️ Poor tradeoffConsider streaming hash for large files.
reload()reads the entire file into memory to compute the MD5 etag. For very large files this could cause memory pressure. Consider streaming the hash computation if large file support is expected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@daras_ai_v2/storage_backends.py` around lines 59 - 69, The reload() method currently calls full_path.read_bytes() and hashes the whole file in memory; change it to stream the file for MD5 by opening full_path in binary mode and feeding chunks (e.g., 64KB) into hashlib.md5().update() to compute etag, keep using stat() to set size and updated, and remove the full in-memory read; ensure the logic that sets self.size, self.etag, self.updated, and self.content_type (using mimetypes.guess_type(self.name)[0]) remains intact and reference full_path, reload(), self.size, self.etag, and hashlib.md5() when making the change.
48-48: 💤 Low valueFix implicit
Optionaltype hints.Ruff reports PEP 484 violations for parameters with
= Nonedefaults lacking explicit| Noneannotations.Proposed fix
- def upload_from_string(self, data: bytes, content_type: str = None): + def upload_from_string(self, data: bytes, content_type: str | None = None):def generate_signed_url( self, - version: str = None, + version: str | None = None, expiration=None, method: str = "GET", - content_type: str = None, + content_type: str | None = None, ) -> str:-def get_storage_bucket(bucket_name: str = None): +def get_storage_bucket(bucket_name: str | None = None):Also applies to: 84-90, 132-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@daras_ai_v2/storage_backends.py` at line 48, Function parameter type hints that use a None default must be annotated explicitly as optional; update upload_from_string to annotate content_type as str | None = None (i.e., change content_type: str = None to content_type: str | None = None) and do the same for the other functions in this module that have parameters defaulting to None (the methods referenced at lines 84-90 and 132); search for any parameters written as "<name>: Type = None" and change them to use the PEP 604 union syntax "Type | None = None" to satisfy Ruff/PEP 484.auth/auth_ui.py (1)
1-1: ⚡ Quick winRemove postponed annotations from this FastAPI router module.
Line [1] adds
from __future__ import annotationsin a module defining FastAPI endpoints. Please drop it here to avoid runtime signature/typing resolution surprises in router inspection.As per coding guidelines, “Do NOT use
from __future__ import annotationsin FastAPI router modules: FastAPI inspects endpoint signatures at import time to derive dependency injection, request parsing, and OpenAPI schemas, and postponed string annotations can break or change that runtime type resolution”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/auth_ui.py` at line 1, Remove the postponed annotations import line "from __future__ import annotations" from this FastAPI router module so endpoint signatures are evaluated at import time; edit auth_ui.py to delete that import and ensure all endpoint function/type hints (the router and any endpoint callables) use normal annotations so FastAPI can inspect them during import for DI, request parsing, and OpenAPI generation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app_users/migrations/0032_appuser_email_unique.py`:
- Around line 17-21: The migration currently applies migrations.AlterField for
model_name="appuser" name="email" with unique=True without cleaning existing
data; add a pre-migration step (e.g., a migrations.RunPython named function)
that normalizes emails (trim and lower-case), identifies non-null duplicate
emails and resolves them (choose a strategy: set duplicates to NULL, append a
deterministic suffix, or merge related user data) before running AlterField,
then include a reversible no-op or reverse operation; ensure the RunPython is
placed immediately before the AlterField entry in the migration so the unique
constraint will not fail on deploy.
In `@auth/auth_ui.py`:
- Line 37: The code directly uses attacker-controlled next values from
request.query_params in RedirectResponse (e.g., request.query_params.get("next",
DEFAULT_LOGOUT_REDIRECT)) which allows open-redirects; add a single reusable
sanitizer (e.g., is_safe_next or sanitize_next) that validates the next URL is
either a local relative path (starts with "/") or matches an allowlist of hosts,
and change every redirect site that uses request.query_params.get("next", ...)
or builds redirects from that value to call this sanitizer and fall back to
DEFAULT_LOGOUT_REDIRECT when invalid; update all RedirectResponse and local-flow
redirect usages to use the sanitizer before returning.
- Around line 222-236: Wrap the AppUser.objects.create call in a try/except that
catches django.db.IntegrityError (to handle the unique-email race) and, on
IntegrityError, use the same error path as the prior exists-check (enter
error_slot and call gui.error("An account with this email already exists.")) so
concurrent signups produce the same UX instead of a 500; ensure you import
IntegrityError and do not swallow other exceptions.
In `@daras_ai_v2/storage_backends.py`:
- Around line 105-124: list_blobs yields FilesystemBlob instances without etag,
causing inconsistency with reload(); fix list_blobs (function list_blobs) to
populate blob.etag before yielding by computing a stable etag (e.g., MD5 hex of
the file contents or a composite of stat fields like size+mtime) using the file
path (entry) and assign it to blob.etag so callers see the same identifier they
would get from reload() or GCS.
---
Nitpick comments:
In `@auth/auth_ui.py`:
- Line 1: Remove the postponed annotations import line "from __future__ import
annotations" from this FastAPI router module so endpoint signatures are
evaluated at import time; edit auth_ui.py to delete that import and ensure all
endpoint function/type hints (the router and any endpoint callables) use normal
annotations so FastAPI can inspect them during import for DI, request parsing,
and OpenAPI generation.
In `@daras_ai_v2/storage_backends.py`:
- Around line 59-69: The reload() method currently calls full_path.read_bytes()
and hashes the whole file in memory; change it to stream the file for MD5 by
opening full_path in binary mode and feeding chunks (e.g., 64KB) into
hashlib.md5().update() to compute etag, keep using stat() to set size and
updated, and remove the full in-memory read; ensure the logic that sets
self.size, self.etag, self.updated, and self.content_type (using
mimetypes.guess_type(self.name)[0]) remains intact and reference full_path,
reload(), self.size, self.etag, and hashlib.md5() when making the change.
- Line 48: Function parameter type hints that use a None default must be
annotated explicitly as optional; update upload_from_string to annotate
content_type as str | None = None (i.e., change content_type: str = None to
content_type: str | None = None) and do the same for the other functions in this
module that have parameters defaulting to None (the methods referenced at lines
84-90 and 132); search for any parameters written as "<name>: Type = None" and
change them to use the PEP 604 union syntax "Type | None = None" to satisfy
Ruff/PEP 484.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d022233-b424-414e-97a0-3da65be98208
📒 Files selected for processing (15)
app_users/migrations/0031_appuser_localauth_password.pyapp_users/migrations/0032_appuser_email_unique.pyapp_users/models.pyauth/auth_ui.pydaras_ai/image_input.pydaras_ai_v2/base.pydaras_ai_v2/gcs_v2.pydaras_ai_v2/settings.pydaras_ai_v2/storage_backends.pyfiles/signals.pygooey_gui/components/modal.pyrouters/root.pyserver.pyworkspaces/views.pyworkspaces/widgets.py
✅ Files skipped from review due to trivial changes (1)
- gooey_gui/components/modal.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app_users/models.py (1)
36-48:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
is_anonymous=Falsein local-auth branch causesIntegrityError.When
FIREBASE_ENABLEDisFalse, line 37 createsself.model(**kwargs)wherekwargsonly containsuid. Theis_anonymousfield is a requiredBooleanFieldwith no default (line 132), souser.save()at line 41 will fail with anIntegrityErrordue to the NOT NULL constraint.Compare with
get_or_create_from_emailat lines 67-68, which correctly setsis_anonymous=False.🐛 Proposed fix
try: - user = self.model(**kwargs) + user = self.model(is_anonymous=False, **kwargs) if settings.FIREBASE_ENABLED: user.copy_from_firebase_user(firebase_auth.get_user(uid)) else: user.save()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app_users/models.py` around lines 36 - 48, The branch that creates a local user when settings.FIREBASE_ENABLED is False does not set the required is_anonymous field, causing a NOT NULL IntegrityError on user.save(); modify the local-auth branch in the method shown so that after instantiating user = self.model(**kwargs) you set user.is_anonymous = False (or include is_anonymous=False in the kwargs) before calling user.save(), leaving the firebase branch (user.copy_from_firebase_user(firebase_auth.get_user(uid))) unchanged and preserving the existing IntegrityError/DoesNotExist handling (references: self.model, settings.FIREBASE_ENABLED, firebase_auth.get_user, user.save(), self.get, self.model.DoesNotExist).auth/auth_ui.py (2)
229-247:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSignup race condition on unique email constraint.
The
exists()check at line 229 andcreate()at line 235 are not atomic. Concurrent signups with the same email can pass the check but fail onIntegrityErrorat the DB level due to the unique constraint, resulting in a 500 error.🛠️ Proposed fix
+from django.db import IntegrityError @@ elif AppUser.objects.filter(email=email).exists(): with error_slot: gui.error( "An account with this email already exists." ) else: - user = AppUser.objects.create( - uid=str(uuid.uuid4()), - email=email, - display_name=email.split("@")[0], - is_anonymous=False, - balance=settings.VERIFIED_EMAIL_USER_FREE_CREDITS, - localauth_password=make_password(password), - last_login_at=timezone.now(), - ) - request.session.clear() - request.session[LOCAL_AUTH_SESSION_COOKIE] = user.pk - user.get_or_create_personal_workspace() - return RedirectResponse(next_url, status_code=303) + try: + user = AppUser.objects.create( + uid=str(uuid.uuid4()), + email=email, + display_name=email.split("@")[0], + is_anonymous=False, + balance=settings.VERIFIED_EMAIL_USER_FREE_CREDITS, + localauth_password=make_password(password), + last_login_at=timezone.now(), + ) + request.session.clear() + request.session[LOCAL_AUTH_SESSION_COOKIE] = user.pk + user.get_or_create_personal_workspace() + return RedirectResponse(next_url, status_code=303) + except IntegrityError: + with error_slot: + gui.error( + "An account with this email already exists." + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/auth_ui.py` around lines 229 - 247, The exists() + create() sequence for AppUser (AppUser.objects.filter(email=email).exists() and AppUser.objects.create(...)) can race and raise a django.db.IntegrityError; wrap the create call in a try/except that catches IntegrityError (imported from django.db) and handles it the same way as the pre-check by rendering the gui.error("An account with this email already exists.") inside the error_slot and returning/aborting the signup flow, ensuring the session is not created on failure; keep the optimistic exists() check but use the except block to handle the concurrent-creation case.
35-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOpen redirect protection in
_safe_next_urlcan be bypassed.The check
next_url.startswith("/") or next_url.startswith("//")doesn't account for protocol-relative URLs that browsers may interpret. For example,/\example.comor/%2f%2fexample.com(URL-encoded//) could potentially bypass this check depending on browser behavior.A more robust check would validate that the path doesn't contain a host component after URL parsing.
🔒 Proposed fix
def _safe_next_url(query_params, *, default: str) -> str: next_url = query_params.get("next") or default - if not next_url.startswith("/") or next_url.startswith("//"): + # Reject if not starting with single slash, or contains backslash (IE compat), + # or has a scheme/netloc after parsing + if not next_url.startswith("/") or next_url.startswith("//") or "\\" in next_url: + return default + parsed = furl(next_url) + if parsed.scheme or parsed.host: return default return next_url🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@auth/auth_ui.py` around lines 35 - 39, The _safe_next_url function's simple startswith checks can be bypassed by protocol-relative or encoded host components; replace the logic with proper URL parsing (e.g., using urllib.parse.urlparse) inside _safe_next_url: parse the candidate next_url, ensure its scheme is empty, its netloc is empty, and that the path begins with a single '/' (and does not start with '//'); if any of those validations fail, return the provided default. Also decode/normalize percent-encodings before validation to prevent encoded "//" or similar bypasses.
🧹 Nitpick comments (1)
daras_ai_v2/storage_backends.py (1)
11-12: ⚡ Quick winGuard
firebase_storageimport for import-time decoupling
firebase-adminis a required Poetry dependency (pyproject.toml), and other modules import Firebase SDK unconditionally, soFIREBASE_ENABLED=Falseis unlikely to break startup solely due to the top-levelfrom firebase_admin import storage as firebase_storageindaras_ai_v2/storage_backends.py. Keeping the import behind thesettings.FIREBASE_ENABLEDbranch would still reduce unnecessary coupling/import-time work.Suggested minimal fix
-from firebase_admin import storage as firebase_storage from furl import furl @@ def get_storage_bucket(bucket_name: str = None): from django.conf import settings if settings.FIREBASE_ENABLED: + from firebase_admin import storage as firebase_storage return firebase_storage.bucket(bucket_name or settings.GS_BUCKET_NAME) else: return _get_filesystem_bucket(bucket_name or settings.GS_BUCKET_NAME)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@daras_ai_v2/storage_backends.py` around lines 11 - 12, Top-level import of firebase_admin.storage (firebase_storage) causes import-time coupling; guard it by moving "from firebase_admin import storage as firebase_storage" behind the conditional that checks settings.FIREBASE_ENABLED. Specifically, remove the top-level import and import firebase_storage inside the branch or function that constructs/uses the Firebase backend (referencing settings.FIREBASE_ENABLED and the code that initializes the Firebase storage client), so the import only happens when FIREBASE_ENABLED is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@daras_ai_v2/gcs_v2.py`:
- Line 5: The module currently imports firebase_admin.storage at import time
which breaks imports when FIREBASE_ENABLED=False; move the import into the
private_url_to_blob function so Firebase is only imported when needed, and
remove or replace any runtime-only return type annotation referencing
firebase_admin (e.g., change return annotation on private_url_to_blob to a
generic type like "Any" or omit it) so the module has no hard dependency on
firebase_admin at import time; update references in private_url_to_blob to use
the locally imported storage variable.
---
Duplicate comments:
In `@app_users/models.py`:
- Around line 36-48: The branch that creates a local user when
settings.FIREBASE_ENABLED is False does not set the required is_anonymous field,
causing a NOT NULL IntegrityError on user.save(); modify the local-auth branch
in the method shown so that after instantiating user = self.model(**kwargs) you
set user.is_anonymous = False (or include is_anonymous=False in the kwargs)
before calling user.save(), leaving the firebase branch
(user.copy_from_firebase_user(firebase_auth.get_user(uid))) unchanged and
preserving the existing IntegrityError/DoesNotExist handling (references:
self.model, settings.FIREBASE_ENABLED, firebase_auth.get_user, user.save(),
self.get, self.model.DoesNotExist).
In `@auth/auth_ui.py`:
- Around line 229-247: The exists() + create() sequence for AppUser
(AppUser.objects.filter(email=email).exists() and AppUser.objects.create(...))
can race and raise a django.db.IntegrityError; wrap the create call in a
try/except that catches IntegrityError (imported from django.db) and handles it
the same way as the pre-check by rendering the gui.error("An account with this
email already exists.") inside the error_slot and returning/aborting the signup
flow, ensuring the session is not created on failure; keep the optimistic
exists() check but use the except block to handle the concurrent-creation case.
- Around line 35-39: The _safe_next_url function's simple startswith checks can
be bypassed by protocol-relative or encoded host components; replace the logic
with proper URL parsing (e.g., using urllib.parse.urlparse) inside
_safe_next_url: parse the candidate next_url, ensure its scheme is empty, its
netloc is empty, and that the path begins with a single '/' (and does not start
with '//'); if any of those validations fail, return the provided default. Also
decode/normalize percent-encodings before validation to prevent encoded "//" or
similar bypasses.
---
Nitpick comments:
In `@daras_ai_v2/storage_backends.py`:
- Around line 11-12: Top-level import of firebase_admin.storage
(firebase_storage) causes import-time coupling; guard it by moving "from
firebase_admin import storage as firebase_storage" behind the conditional that
checks settings.FIREBASE_ENABLED. Specifically, remove the top-level import and
import firebase_storage inside the branch or function that constructs/uses the
Firebase backend (referencing settings.FIREBASE_ENABLED and the code that
initializes the Firebase storage client), so the import only happens when
FIREBASE_ENABLED is true.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc9d1f29-f683-46e3-9b4d-608eefeb47c8
📒 Files selected for processing (6)
app_users/models.pyauth/auth_backend.pyauth/auth_ui.pydaras_ai_v2/gcs_v2.pydaras_ai_v2/storage_backends.pyworkspaces/migrations/0016_alter_workspacemembership_balance.py
Contributor
Author
|
@devxpy please review! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_apis_without_keys.py (1)
133-135: ⚡ Quick winNarrow the expected exception set in this test.
Catching
Exceptionhere weakens the test signal and can hide unrelated failures. Keep only credential/auth-related exception types.Suggested patch
- with pytest.raises( - (GoogleAPICallError, DefaultCredentialsError, Exception) - ) as exc_info: + with pytest.raises( + (GoogleAPICallError, DefaultCredentialsError) + ) as exc_info:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_apis_without_keys.py` around lines 133 - 135, The test currently broadens failure masking by using pytest.raises((GoogleAPICallError, DefaultCredentialsError, Exception)) in the with pytest.raises(...) block; remove the generic Exception from that tuple so the test only expects credential/auth-related errors (e.g., GoogleAPICallError and DefaultCredentialsError, or add any specific auth-related exceptions like RefreshError if used elsewhere) to avoid hiding unrelated failures—update the pytest.raises invocation in tests/test_apis_without_keys.py accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@daras_ai_v2/language_model.py`:
- Line 1043: The match guard currently indexes part["image_url"] directly which
can raise KeyError for malformed parts; change the guard to use
part.get("image_url") (e.g., replace the condition in the match-case line `case
"image_url" if isinstance(part["image_url"], dict):` with a guard that calls
isinstance(part.get("image_url"), dict)) so missing keys return None and do not
crash; keep the rest of the case body unchanged and ensure any subsequent uses
of part["image_url"] are either guarded or use .get() as well.
- Around line 318-336: The current logic in local_image_urls_to_base64 rebuilds
message content from get_entry_images() and get_entry_text(), which drops other
content parts; instead, iterate the original message["content"] and transform
only items where item.get("type") == "image_url" (use
get_entry_images/get_entry_text only if needed), fetching the image bytes with
requests and converting via bytes_to_cv2_img and cv2_img_to_bytes, then replace
the image_url value in-place with the base64 data URI; finally append the
modified message (not a reconstructed one) to ret so all non-image parts are
preserved.
- Line 320: Requests fetches in language_model.py must handle timeouts and HTTP
errors and image-to-base64 conversion must preserve non-image parts: update all
requests.get(...) calls used to fetch images (e.g., the list comprehension
producing img_bytes and the similar call near line ~613) to pass a sensible
timeout and call response.raise_for_status() before using .content; in
local_image_urls_to_base64() stop rebuilding message["content"] as only
image_url parts plus a single text part—instead iterate message["content"] and
replace only parts that contain an image URL with a new base64 part while
retaining all other parts (e.g., input_audio) unchanged so multi-part messages
are preserved; finally, in chat_completion_msg_to_responses_input_msg() use
part.get("image_url") (and similar .get for optional keys) when checking fields
to avoid KeyError on malformed parts.
---
Nitpick comments:
In `@tests/test_apis_without_keys.py`:
- Around line 133-135: The test currently broadens failure masking by using
pytest.raises((GoogleAPICallError, DefaultCredentialsError, Exception)) in the
with pytest.raises(...) block; remove the generic Exception from that tuple so
the test only expects credential/auth-related errors (e.g., GoogleAPICallError
and DefaultCredentialsError, or add any specific auth-related exceptions like
RefreshError if used elsewhere) to avoid hiding unrelated failures—update the
pytest.raises invocation in tests/test_apis_without_keys.py accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 826fce9a-987e-42ac-a087-b556663a5506
📒 Files selected for processing (5)
.env.exampleREADME.mdconfiguration.mddaras_ai_v2/language_model.pytests/test_apis_without_keys.py
✅ Files skipped from review due to trivial changes (2)
- README.md
- configuration.md
| for message in messages: | ||
| content = [] | ||
| if img_urls := get_entry_images(message): | ||
| img_bytes = [requests.get(img_url).content for img_url in img_urls] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify requests calls in this file that do not set timeout.
rg -nP 'requests\.(get|post)\(' daras_ai_v2/language_model.py
rg -nP 'requests\.(get|post)\([^)]*timeout\s*=' daras_ai_v2/language_model.pyRepository: GooeyAI/gooey-server
Length of output: 292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="daras_ai_v2/language_model.py"
# Show the flagged image fetch at/around line 320
nl -ba "$FILE" | sed -n '280,360p'
# Show the other image fetch at ~613
nl -ba "$FILE" | sed -n '560,660p'
# Locate and show local_image_urls_to_base64
rg -n "def local_image_urls_to_base64|local_image_urls_to_base64" "$FILE"
LOC=$(rg -n "def local_image_urls_to_base64" "$FILE" | head -n1 | cut -d: -f1)
if [[ -n "${LOC}" ]]; then
START=$((LOC-60)); END=$((LOC+120))
nl -ba "$FILE" | sed -n "${START},${END}p"
fi
# Locate chat_completion_msg_to_responses_input_msg and show relevant content reconstruction
rg -n "def chat_completion_msg_to_responses_input_msg|chat_completion_msg_to_responses_input_msg" "$FILE"
LOC2=$(rg -n "def chat_completion_msg_to_responses_input_msg" "$FILE" | head -n1 | cut -d: -f1)
if [[ -n "${LOC2}" ]]; then
START=$((LOC2-80)); END=$((LOC2+220))
nl -ba "$FILE" | sed -n "${START},${END}p"
fi
# Find any part["image_url"] indexing and show surrounding code
rg -n "part\\[\"image_url\"\\]" "$FILE"
rg -n "isinstance\\(part\\[\"image_url\"\\], dict\\)" "$FILE"
nl -ba "$FILE" | sed -n '1,220p' | head -n 1 >/dev/null || trueRepository: GooeyAI/gooey-server
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="daras_ai_v2/language_model.py"
# Show the flagged image fetch at/around line 320
cat -n "$FILE" | sed -n '280,360p'
# Show the other image fetch at ~613
cat -n "$FILE" | sed -n '560,660p'
# Locate and show local_image_urls_to_base64
rg -n "def local_image_urls_to_base64" "$FILE"
LOC=$(rg -n "def local_image_urls_to_base64" "$FILE" | head -n1 | cut -d: -f1)
if [[ -n "${LOC}" ]]; then
START=$((LOC-60)); END=$((LOC+140))
cat -n "$FILE" | sed -n "${START},${END}p"
fi
# Locate chat_completion_msg_to_responses_input_msg and show relevant content reconstruction
rg -n "def chat_completion_msg_to_responses_input_msg" "$FILE"
LOC2=$(rg -n "def chat_completion_msg_to_responses_input_msg" "$FILE" | head -n1 | cut -d: -f1)
if [[ -n "${LOC2}" ]]; then
START=$((LOC2-60)); END=$((LOC2+260))
cat -n "$FILE" | sed -n "${START},${END}p"
fi
# Find any part["image_url"] indexing and show surrounding code
rg -n "part\\[\"image_url\"\\]" "$FILE"
rg -n "isinstance\\(part\\[\"image_url\"\\], dict\\)" "$FILE"Repository: GooeyAI/gooey-server
Length of output: 29579
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="daras_ai_v2/language_model.py"
# Find where local_image_urls_to_base64 is called
rg -n "local_image_urls_to_base64\\(" "$FILE"
# Show the function call context
CALL_LINE=$(rg -n "local_image_urls_to_base64\\(" "$FILE" | head -n1 | cut -d: -f1 || true)
if [[ -n "${CALL_LINE}" ]]; then
START=$((CALL_LINE-40)); END=$((CALL_LINE+80))
sed -n "${START},${END}p" "$FILE" | cat -n
fi
# Locate and show get_entry_text/get_entry_images implementations
rg -n "def get_entry_text\\b|def get_entry_images\\b" "$FILE"
GT=$(rg -n "def get_entry_text\\b" "$FILE" | head -n1 | cut -d: -f1 || true)
if [[ -n "${GT}" ]]; then
START=$((GT-30)); END=$((GT+120))
sed -n "${START},${END}p" "$FILE" | cat -n
fi
GI=$(rg -n "def get_entry_images\\b" "$FILE" | head -n1 | cut -d: -f1 || true)
if [[ -n "${GI}" ]]; then
START=$((GI-30)); END=$((GI+160))
sed -n "${START},${END}p" "$FILE" | cat -n
fiRepository: GooeyAI/gooey-server
Length of output: 19915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="daras_ai_v2/language_model.py"
# Find all requests.get/post usages
rg -nP 'requests\.(get|post)\(' "$FILE"
echo "----"
# Check for any timeout= in requests.get/post calls
rg -nP 'requests\.(get|post)\([^)]*timeout\s*=' "$FILE" || true
echo "----"
# Check whether raise_for_status is used for requests.get/post
rg -n "raise_for_status\(" "$FILE" || trueRepository: GooeyAI/gooey-server
Length of output: 418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="daras_ai_v2/language_model.py"
# Find definition/import of ChatCompletionContentPartParam and allowed part types
rg -n "ChatCompletionContentPartParam" "$FILE"
# Show nearby type/aliases if defined in-file
LOC=$(rg -n "ChatCompletionContentPartParam" "$FILE" | head -n1 | cut -d: -f1 || true)
if [[ -n "${LOC}" ]]; then
START=$((LOC-60)); END=$((LOC+120))
sed -n "${START},${END}p" "$FILE" | cat -n
fi
# Locate any code that builds 'content' parts with more than 'text'/'image_url'
rg -n "content.*\\[" "$FILE" | head -n 50
rg -n "\"type\": \"(text|image_url)\"" "$FILE"
# Find get_entry_images/get_entry_text usage context (only for vision/local-base64 path already seen, but we'll confirm)
rg -n "local_image_urls_to_base64\\(messages\\)|local_image_urls_to_base64\\(" "$FILE"Repository: GooeyAI/gooey-server
Length of output: 2004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="daras_ai_v2/language_model.py"
# Search how content part types are constructed in this file
rg -n '"type"\s*:\s*"(image_url|text|input_image|input_text|image|audio|function_call|function_call_output|tool)"' "$FILE"
# Search for generic "type": usage to see other part types
rg -n '"type"\s*:' "$FILE" | head -n 80
# Search for creation of "image_url" parts
rg -n '"type"\s*:\s*"image_url"' "$FILE"
# Search for usage of local_image_urls_to_base64 output later / how chat_completion_msg_to_responses_input_msg expects it
rg -n 'chat_completion_msg_to_responses_input_msg\\(|local_image_urls_to_base64\\(' "$FILE"Repository: GooeyAI/gooey-server
Length of output: 2397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="daras_ai_v2/language_model.py"
# View around requests.post and any timeout usage
cat -n "$FILE" | sed -n '1400,1620p'
# View around _run_anthropic_chat requests.get (line ~613)
cat -n "$FILE" | sed -n '580,660p'
# Check how raise_for_status is defined/imported
rg -n "raise_for_status\\(" "$FILE"
rg -n "def raise_for_status\\b|from .* import raise_for_status\\b|import .*raise_for_status\\b" "$FILE"
# Check if any requests.* call uses timeout= elsewhere in the file
rg -n "requests\\.(get|post)\\([^\\)]*timeout\\s*=" "$FILE" || trueRepository: GooeyAI/gooey-server
Length of output: 11686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="daras_ai_v2/language_model.py"
# Show top of file to inspect ChatCompletionContentPartParam definition
sed -n '1,220p' "$FILE" | cat -n
echo "----"
# Show around where "input_images" and "input_audio" are built
# (we saw hits around ~1804 and ~1815)
cat -n "$FILE" | sed -n '1760,1845p'
echo "----"
# Show around raise_for_status import area
sed -n '1,80p' "$FILE" | cat -nRepository: GooeyAI/gooey-server
Length of output: 13966
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Python call sites that pass input_audio= (likely via format_chat_entry)
rg -n "input_audio\s*=" --glob='*.py' . | head -n 80
echo "----"
# For any matches, show a bit of surrounding context to see whether input_images= is also passed
# (we'll re-run with context for the top matches)
rg -n "input_audio\s*=" --glob='*.py' . | head -n 20 | cut -d: -f1-2 | while read -r f l; do
start=$((l-10))
end=$((l+10))
echo "### $f:$l"
sed -n "${start},${end}p" "$f" | cat -n
echo "----"
doneRepository: GooeyAI/gooey-server
Length of output: 3614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find format_chat_entry calls that pass input_audio and/or input_images
rg -n "format_chat_entry\(" recipes daras_ai_v2 bots ai_models --glob='*.py'
echo "----"
# 2) Narrow: calls that include input_audio=
rg -n "format_chat_entry\([^)]*input_audio\s*=" recipes daras_ai_v2 bots --glob='*.py'
echo "----"
# 3) Narrow: calls that include both input_audio= and input_images=
rg -n "format_chat_entry\([^)]*input_audio\s*=.*,\s*input_images\s*=" -S recipes daras_ai_v2 bots --glob='*.py' || true
rg -n "format_chat_entry\([^)]*input_images\s*=.*,\s*input_audio\s*=" -S recipes daras_ai_v2 bots --glob='*.py' || true
echo "----"
# 4) Inspect likely message building spots around VideoBots
cat -n recipes/VideoBots.py | sed -n '1320,1385p'
cat -n recipes/VideoBots.py | sed -n '360,580p'
echo "----"
# 5) Search for dicts/literals with type "image_url" missing "image_url" key
# (best-effort textual search)
rg -n '"type"\s*:\s*"image_url"' daras_ai_v2 bots recipes --glob='*.py'
# 6) Search for any content parts manipulation using "type": "image_url" without "image_url":
# (best-effort heuristic: show a small window around occurrences)
rg -n --context=2 '"type"\s*:\s*"image_url"' daras_ai_v2 bots recipes --glob='*.py'Repository: GooeyAI/gooey-server
Length of output: 1030
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect likely call sites with input_audio in VideoBots
sed -n '520,620p' recipes/VideoBots.py | cat -n
echo "----"
sed -n '1270,1380p' recipes/VideoBots.py | cat -n
echo "----"
sed -n '1480,1565p' recipes/VideoBots.py | cat -n
echo "===================="
# Inspect convo_msg builder around input_audio handling
sed -n '380,560p' bots/models/convo_msg.py | cat -n
echo "===================="
# Specifically find any format_chat_entry call that passes input_audio=
rg -n "format_chat_entry\\([^\\n]*input_audio" recipes/VideoBots.py bots/models/convo_msg.py daras_ai_v2/bots.py --glob='*.py' || trueRepository: GooeyAI/gooey-server
Length of output: 23160
Fix image fetch robustness (timeouts/status) and prevent losing non-image content when converting images to base64.
daras_ai_v2/language_model.pyhasrequests.get(...)with notimeoutand no status validation at lines ~320 and ~613; addtimeout=...and callraise_for_status(...)on the response.local_image_urls_to_base64()rebuildsmessage["content"]asimage_urlparts + a single text part, dropping other content parts liketype: "input_audio"when bothinput_imagesandinput_audioare present (e.g., VideoBots builds chat entries withinput_images=request.input_imagesandinput_audio=include_input_audio and request.input_audio, andrun_language_modelcallslocal_image_urls_to_base64()whennot settings.FIREBASE_ENABLED).
Suggested patch
- img_bytes = [requests.get(img_url).content for img_url in img_urls]
+ img_bytes = []
+ for img_url in img_urls:
+ resp = requests.get(img_url, timeout=10)
+ raise_for_status(resp)
+ img_bytes.append(resp.content)- Minor:
chat_completion_msg_to_responses_input_msg()pattern guard usespart["image_url"], which can raiseKeyErroron malformed parts; preferpart.get("image_url").
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| img_bytes = [requests.get(img_url).content for img_url in img_urls] | |
| img_bytes = [] | |
| for img_url in img_urls: | |
| resp = requests.get(img_url, timeout=10) | |
| resp.raise_for_status() | |
| img_bytes.append(resp.content) |
🧰 Tools
🪛 Ruff (0.15.14)
[error] 320-320: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@daras_ai_v2/language_model.py` at line 320, Requests fetches in
language_model.py must handle timeouts and HTTP errors and image-to-base64
conversion must preserve non-image parts: update all requests.get(...) calls
used to fetch images (e.g., the list comprehension producing img_bytes and the
similar call near line ~613) to pass a sensible timeout and call
response.raise_for_status() before using .content; in
local_image_urls_to_base64() stop rebuilding message["content"] as only
image_url parts plus a single text part—instead iterate message["content"] and
replace only parts that contain an image URL with a new base64 part while
retaining all other parts (e.g., input_audio) unchanged so multi-part messages
are preserved; finally, in chat_completion_msg_to_responses_input_msg() use
part.get("image_url") (and similar .get for optional keys) when checking fields
to avoid KeyError on malformed parts.
Contributor
Author
There was a problem hiding this comment.
what's the committable suggestion here?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
recipes/TextToSpeech.py (1)
181-195:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate both Uberduck credentials before calling the API.
This branch now checks
UBERDUCK_KEY, but the request still authenticates with bothsettings.UBERDUCK_KEYandsettings.UBERDUCK_SECRET. If the secret is missing, users get a downstream auth failure instead of a clear config error.Possible fix
case TextToSpeechProviders.UBERDUCK: ensure_config_key("UBERDUCK_KEY") + ensure_config_key("UBERDUCK_SECRET") voicemodel_uuid = ( UBERDUCK_VOICES.get(state.get("uberduck_voice_name")) or UBERDUCK_VOICES["Aiden Botha"] ).strip()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@recipes/TextToSpeech.py` around lines 181 - 195, The Uberduck branch (case TextToSpeechProviders.UBERDUCK) currently only ensures UBERDUCK_KEY but then calls requests.post with both settings.UBERDUCK_KEY and settings.UBERDUCK_SECRET; add a validation for UBERDUCK_SECRET before making the API call (e.g., call ensure_config_key("UBERDUCK_SECRET") or similar) so both credentials are present and raise a clear config error rather than letting the request fail, updating the Uberduck branch that currently sets voicemodel_uuid, pace and calls requests.post to include this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ai_models/models.py`:
- Around line 75-76: The current provider-level check in ModelProvider
(returning ("AZURE_OPENAI_KEY_CA","AZURE_OPENAI_KEY_EASTUS2")) makes any
azure_openai model appear available regardless of its region; change
availability to be model-specific by moving the Azure key/region check into
AIModelSpec so it inspects the spec's model_id (e.g., look for "-ca-" vs
"-eastus2-" tokens) and maps to the correct env var (AZURE_OPENAI_KEY_CA or
AZURE_OPENAI_KEY_EASTUS2) before declaring availability; update
filter_available() / is_available callers to use AIModelSpec.is_available (or
equivalent instance method) so availability is determined per-model-id/region
rather than per-provider.
In `@daras_ai_v2/language_model.py`:
- Around line 369-373: The code assumes api/model.provider is a ModelProvider
enum but sometimes receives a raw int; update _run_text_model and
_run_chat_model to coerce the incoming api or model.provider to the enum before
using helpers: call api = ModelProvider(api) in _run_text_model (or ensure
run_language_model passes ModelProvider(model.provider)), and in _run_chat_model
use provider = ModelProvider(model.provider) before accessing
provider.required_keys[0] or provider.keys_available(); adjust any checks that
call api.keys_available() or access required_keys to use the enum instance
instead of a bare int so the enum helper methods work correctly.
- Around line 432-436: The unavailable-model error path and the text-model
invocation use model.provider directly but Django IntegerChoices yields an int
at runtime, causing AttributeError; fix by normalizing/casting model.provider to
the ModelProvider enum (e.g. provider = ModelProvider(model.provider)) before
accessing provider.required_keys or passing it to _run_text_model, then use
provider in the UserError message and in the subsequent match/logic and ensure
_run_text_model receives the enum (not an int) so api.keys_available() and
api.required_keys work correctly.
In `@daras_ai_v2/text_to_speech_settings_widgets.py`:
- Around line 34-36: The Eleven Labs provider is being marked as unavailable
because TTSProvider is created with required_key="ELEVEN_LABS_API_KEY", which
makes _unavailable() hide the provider when the default key is unset; remove or
avoid using required_key for ELEVEN_LABS (and the other ELEVEN_LABS occurrences
at the other instantiations around lines 58-62 and 188) so the provider remains
selectable when only workspace-managed/custom secrets are present, and ensure
elevenlabs_selector() and any availability checks use a separate, explicit check
for a server-wide key vs workspace secret rather than relying on required_key.
In `@recipes/VideoBots.py`:
- Around line 1086-1097: The current guard skips validating output_glossary when
gui.session_state["asr_task"] == "translate", but output_translation_step()
still uses output_glossary when should_translate_lang(user_language) is true;
update the logic so validate_glossary_document(output_glossary) always runs
whenever should_translate_lang(user_language) is true. Concretely, keep or add a
check for input_glossary validation that skips when asr_task == "translate", but
remove the asr_task check for output_glossary (or split into two conditionals)
so validate_glossary_document(output_glossary) is called whenever
should_translate_lang(user_language) and output_glossary is present; reference:
user_language, should_translate_lang, gui.session_state["asr_task"],
input_glossary, output_glossary, validate_glossary_document, and
output_translation_step.
In `@tests/test_token_counter.py`:
- Around line 13-15: The test currently depends on runtime model availability
because it iterates AIModelSpec objects with .filter_available(), which can skip
all models when OpenAI credentials are missing; change the test to exercise the
offline logic by removing the .filter_available() call (or replace it with a
deterministic queryset such as .all() or a fixed test fixture) so
default_length_function() is exercised regardless of API keys; update the loop
in tests/test_token_counter.py that references AIModelSpec.objects.filter(...,
provider=ModelProvider.openai).filter_available() to use a non-runtime-dependent
source (e.g., AIModelSpec.objects.filter(..., provider=ModelProvider.openai) or
a created test AIModelSpec instance) and keep assertions that validate
default_length_function().
---
Outside diff comments:
In `@recipes/TextToSpeech.py`:
- Around line 181-195: The Uberduck branch (case TextToSpeechProviders.UBERDUCK)
currently only ensures UBERDUCK_KEY but then calls requests.post with both
settings.UBERDUCK_KEY and settings.UBERDUCK_SECRET; add a validation for
UBERDUCK_SECRET before making the API call (e.g., call
ensure_config_key("UBERDUCK_SECRET") or similar) so both credentials are present
and raise a clear config error rather than letting the request fail, updating
the Uberduck branch that currently sets voicemodel_uuid, pace and calls
requests.post to include this check.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d537a68-0fb2-4746-893d-cc8a0cfcefdd
📒 Files selected for processing (15)
ai_models/models.pydaras_ai_v2/asr.pydaras_ai_v2/azure_asr.pydaras_ai_v2/embedding_model.pydaras_ai_v2/enum_selector_widget.pydaras_ai_v2/exceptions.pydaras_ai_v2/fal_ai.pydaras_ai_v2/language_model.pydaras_ai_v2/language_model_openai_audio.pydaras_ai_v2/mistral_ocr.pydaras_ai_v2/text_to_speech_settings_widgets.pyrecipes/TextToSpeech.pyrecipes/VideoBots.pyserver.pytests/test_token_counter.py
✅ Files skipped from review due to trivial changes (2)
- daras_ai_v2/azure_asr.py
- daras_ai_v2/enum_selector_widget.py
🚧 Files skipped from review as they are similar to previous changes (1)
- server.py
Comment on lines
+75
to
+76
| case ModelProvider.azure_openai: | ||
| return ("AZURE_OPENAI_KEY_CA", "AZURE_OPENAI_KEY_EASTUS2") |
Contributor
There was a problem hiding this comment.
Make Azure availability checks model-specific.
This treats any Azure key as sufficient for every azure_openai spec, but the runtime client picks a region-specific key from model_id (*-ca-* uses AZURE_OPENAI_KEY_CA, *-eastus2-* uses AZURE_OPENAI_KEY_EASTUS2). With only one region configured, filter_available()/is_available will still expose models for the other region that cannot run. Consider moving this check to AIModelSpec so it can inspect the specific model id instead of the provider alone.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ai_models/models.py` around lines 75 - 76, The current provider-level check
in ModelProvider (returning ("AZURE_OPENAI_KEY_CA","AZURE_OPENAI_KEY_EASTUS2"))
makes any azure_openai model appear available regardless of its region; change
availability to be model-specific by moving the Azure key/region check into
AIModelSpec so it inspects the spec's model_id (e.g., look for "-ca-" vs
"-eastus2-" tokens) and maps to the correct env var (AZURE_OPENAI_KEY_CA or
AZURE_OPENAI_KEY_EASTUS2) before declaring availability; update
filter_available() / is_available callers to use AIModelSpec.is_available (or
equivalent instance method) so availability is determined per-model-id/region
rather than per-provider.
Comment on lines
+1086
to
+1097
| user_language = gui.session_state.get("user_language") | ||
| if ( | ||
| user_language | ||
| and should_translate_lang(user_language) | ||
| and not gui.session_state.get("asr_task") == "translate" | ||
| ): | ||
| input_glossary = gui.session_state.get("input_glossary_document", "") | ||
| output_glossary = gui.session_state.get("output_glossary_document", "") | ||
| if input_glossary: | ||
| validate_glossary_document(input_glossary) | ||
| if output_glossary: | ||
| validate_glossary_document(output_glossary) |
Contributor
There was a problem hiding this comment.
Still validate the output glossary for asr_task == "translate" runs.
output_translation_step() uses output_glossary_document whenever should_translate_lang(user_language) is true, even when the ASR step already translated the input. The new guard skips validation for that case, so invalid output glossaries are no longer caught before execution.
Possible fix
user_language = gui.session_state.get("user_language")
- if (
- user_language
- and should_translate_lang(user_language)
- and not gui.session_state.get("asr_task") == "translate"
- ):
- input_glossary = gui.session_state.get("input_glossary_document", "")
- output_glossary = gui.session_state.get("output_glossary_document", "")
- if input_glossary:
- validate_glossary_document(input_glossary)
- if output_glossary:
- validate_glossary_document(output_glossary)
+ if user_language and should_translate_lang(user_language):
+ output_glossary = gui.session_state.get("output_glossary_document", "")
+ if output_glossary:
+ validate_glossary_document(output_glossary)
+
+ if gui.session_state.get("asr_task") != "translate":
+ input_glossary = gui.session_state.get("input_glossary_document", "")
+ if input_glossary:
+ validate_glossary_document(input_glossary)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@recipes/VideoBots.py` around lines 1086 - 1097, The current guard skips
validating output_glossary when gui.session_state["asr_task"] == "translate",
but output_translation_step() still uses output_glossary when
should_translate_lang(user_language) is true; update the logic so
validate_glossary_document(output_glossary) always runs whenever
should_translate_lang(user_language) is true. Concretely, keep or add a check
for input_glossary validation that skips when asr_task == "translate", but
remove the asr_task check for output_glossary (or split into two conditionals)
so validate_glossary_document(output_glossary) is called whenever
should_translate_lang(user_language) and output_glossary is present; reference:
user_language, should_translate_lang, gui.session_state["asr_task"],
input_glossary, output_glossary, validate_glossary_document, and
output_translation_step.
Contributor
Author
There was a problem hiding this comment.
add a committable suggestion
Contributor
There was a problem hiding this comment.
@nikochiko Here's the committable suggestion:
Suggested change
| user_language = gui.session_state.get("user_language") | |
| if ( | |
| user_language | |
| and should_translate_lang(user_language) | |
| and not gui.session_state.get("asr_task") == "translate" | |
| ): | |
| input_glossary = gui.session_state.get("input_glossary_document", "") | |
| output_glossary = gui.session_state.get("output_glossary_document", "") | |
| if input_glossary: | |
| validate_glossary_document(input_glossary) | |
| if output_glossary: | |
| validate_glossary_document(output_glossary) | |
| def validate_form_v2(self): | |
| user_language = gui.session_state.get("user_language") | |
| if user_language and should_translate_lang(user_language): | |
| output_glossary = gui.session_state.get("output_glossary_document", "") | |
| if output_glossary: | |
| validate_glossary_document(output_glossary) | |
| if gui.session_state.get("asr_task") != "translate": | |
| input_glossary = gui.session_state.get("input_glossary_document", "") | |
| if input_glossary: | |
| validate_glossary_document(input_glossary) |
Contributor
Author
There was a problem hiding this comment.
just remove the line and not gui.session_state.get("asr_task") == "translate"` instead
Contributor
There was a problem hiding this comment.
@nikochiko That's a cleaner approach — removing that line validates both glossaries whenever translation is active, which fully addresses the output-glossary concern with minimal change. Here's the committable suggestion:
Suggested change
| user_language = gui.session_state.get("user_language") | |
| if ( | |
| user_language | |
| and should_translate_lang(user_language) | |
| and not gui.session_state.get("asr_task") == "translate" | |
| ): | |
| input_glossary = gui.session_state.get("input_glossary_document", "") | |
| output_glossary = gui.session_state.get("output_glossary_document", "") | |
| if input_glossary: | |
| validate_glossary_document(input_glossary) | |
| if output_glossary: | |
| validate_glossary_document(output_glossary) | |
| def validate_form_v2(self): | |
| user_language = gui.session_state.get("user_language") | |
| if user_language and should_translate_lang(user_language): | |
| input_glossary = gui.session_state.get("input_glossary_document", "") | |
| output_glossary = gui.session_state.get("output_glossary_document", "") | |
| if input_glossary: | |
| validate_glossary_document(input_glossary) | |
| if output_glossary: | |
| validate_glossary_document(output_glossary) |
nikochiko
force-pushed
the
local-auth
branch
2 times, most recently
from
efa9c7c to
602b6a4
Compare
…evel import Deduplicate IntegrityError handling in get_or_create_from_uid/email by moving the FIREBASE_ENABLED branch inside a single try/except block. Replace all lazy firebase_admin imports with a top-level import. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ckend Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ering Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…em storage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-hosting guide Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Verifies that calling Google TTS with no service account configured raises a credential-related error, not an unexpected crash. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Skip Azure image moderation entirely when AZURE_IMAGE_MODERATION_KEY is not set - Hide "Add Integrations" button when COMPOSIO_API_KEY is not configured Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
SOVEREIGN_DEPLOY=True now means self-hosted/local mode (no Firebase/GCS). Default is False so existing deployments continue using Firebase/GCS unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nfigured Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… secrets, bot guards - Guard AI model selectors/runners behind API key availability - Hide managed secrets UI when Azure Key Vault is not configured - Guard Stripe buttons/tasks, Composio, photo intelligence behind their keys - Guard bot integration creation (Slack/WhatsApp/Facebook/Twilio/Telegram) behind required keys - Rename FIREBASE_ENABLED → SOVEREIGN_DEPLOY (inverted: True = no Firebase/GCS) - Skip Azure image moderation when key is absent - Fix _unavailable() condition ordering to avoid TypeError on None keys - Check os.environ as fallback in is_key_set for env-only vars - Expand .env.example with all available settings and fix SOVEREIGN_DEPLOY comments - Add Sovereign Deployment section to README with Deno self-hosting instructions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
2 participants
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.
Q/A checklist
How to check import time?
You can visualize this using tuna:
To measure import time for a specific library:
To reduce import times, import libraries that take a long time inside the functions that use them instead of at the top of the file:
Legal Boilerplate
Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.