add sso and use util package#12
Closed
elirazpevz wants to merge 21 commits into
Closed
Conversation
Yuval/agent to ts
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds session-based SSO auth, a shared backend core package, embedding and extractor data models, a new agent service and workflow, frontend auth and form-state refactors, and updated Docker, compose, Trino, and build wiring. ChangesShared backend data platform
Session-based authentication
Agent workflow and extractors
Frontend transport and form UX
Container and local runtime wiring
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend
participant API as Agent API
participant Graph as Agent Graph
participant Trino as Trino
participant Esca as Esca
UI->>API: POST /api/v1/agent/chat
API->>Graph: ainvoke(user_query, allowed_tables, active_extractors)
Graph->>Trino: execute_query_sync(sql_query)
Trino-->>Graph: rows or error
Graph->>Esca: save_data/load_head
Esca-->>Graph: raw_data_ref or preview
Graph-->>API: summary, sql_query, sql_explanation
API-->>UI: ChatResponse
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/infra_init.py (1)
509-511:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unreachable code after final Trino readiness check.
Line 511 is unreachable: if line 510 succeeds, the loop would have already returned during one of the 20 iterations; if line 510 fails, it raises an exception. Consider removing the redundant final attempt or restructuring the retry logic.
♻️ Suggested fix
time.sleep(_TRINO_READY_INTERVAL) - # Final attempt — raise on failure - _trino_exec("SHOW SCHEMAS FROM minio") - logger.info("[InfraInit] Trino minio catalog is ready ✓") + # All attempts exhausted + raise Exception(f"Trino minio catalog not ready after {_TRINO_READY_RETRIES} attempts")🤖 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 `@backend/app/infra_init.py` around lines 509 - 511, The logger.info call following the final _trino_exec("SHOW SCHEMAS FROM minio") statement is unreachable code. If the Trino readiness check succeeds, the function would have already returned during one of the retry iterations in the preceding loop; if it fails, an exception is raised. Remove the unreachable logger.info statement that logs the Trino minio catalog readiness message after the final _trino_exec call, or restructure the retry logic to ensure success cases properly log completion before returning.backend/app/main.py (1)
69-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAudit rows are still attributed to a fake user.
The PR now authenticates protected routes with
get_current_user, butaudit_middlewarestill persistsuser_id="user-1". That will corrupt the audit trail for every real request and make per-user investigations/compliance reporting unusable.Also applies to: 111-125
🤖 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 `@backend/app/main.py` around lines 69 - 90, The audit_middleware function is using a hardcoded fake user ID "user-1" instead of extracting the actual authenticated user from the request. Replace the hardcoded user_id assignment in the audit_middleware function with code that retrieves the current authenticated user (using the same authentication mechanism like get_current_user that is used in other protected routes) and uses their actual user ID in the audit log. This ensures audit records are correctly attributed to real users rather than a fake placeholder user.
🧹 Nitpick comments (8)
backend/app/infra_init.py (1)
1015-1026: ⚡ Quick winConsider limiting concurrent profiling threads to prevent resource exhaustion.
Each table spawns its own background thread without a concurrency cap. If
_AIRLINES_TABLESgrows to dozens of tables, this could exhaust thread resources or overwhelm the database. Consider using aThreadPoolExecutorwith a fixed pool size to limit concurrent profiling operations.♻️ Example refactor using ThreadPoolExecutor
+ from concurrent.futures import ThreadPoolExecutor + for table_id, tname, sname, cat in table_meta: - t = threading.Thread( - target=_run_profile, - args=(table_id, tname, sname, cat), - daemon=True, - name=f"profile-{cat}.{sname}.{tname}", - ) - t.start() - threads_started += 1 logger.info( "[InfraInit] Started profiling thread for '%s.%s.%s' ✓", cat, sname, tname ) + + # Submit all profiling jobs to a bounded thread pool + with ThreadPoolExecutor(max_workers=5, thread_name_prefix="airlines-profile") as executor: + for table_id, tname, sname, cat in table_meta: + executor.submit(_run_profile, table_id, tname, sname, cat) + threads_started += 1🤖 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 `@backend/app/infra_init.py` around lines 1015 - 1026, The current implementation spawns one thread per table without any concurrency limit, which could exhaust system resources if the number of tables grows large. Replace the unbounded thread creation in the loop that processes table_meta by creating a ThreadPoolExecutor with a fixed pool size at the start of the function, then use executor.submit() to submit _run_profile tasks to the pool instead of creating individual threading.Thread objects. This ensures only a bounded number of profiling threads run concurrently while still maintaining the same logging and functionality.infra/trino/etc/catalog/airlines.properties (1)
2-3: Account identifier and username are hardcoded in this development configuration—consider parameterizing if this catalog is reused across environments.This is a local development Snowflake catalog configuration. While the hardcoded account (
RSRSBDK-YDB67606) and username (BARVAZ_HAMOOD) are acceptable for a demo/test setup, parameterizing them via environment variables (like the password is already doing) would align with best practices and make the configuration more portable across development, staging, and production environments.🤖 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 `@infra/trino/etc/catalog/airlines.properties` around lines 2 - 3, The connection-url and connection-user properties in the Snowflake catalog configuration are hardcoded with specific account identifier and username values. Replace the hardcoded account identifier in the connection-url property (currently RSRSBDK-YDB67606) and the connection-user property value (currently BARVAZ_HAMOOD) with environment variable references using the same parameterization pattern already applied to the password, ensuring the configuration can be easily reused across different environments by adjusting environment variables rather than modifying the file.frontend/src/components/wizard/OnboardingWizard.tsx (1)
91-98: ⚡ Quick winOptimize watch dependencies to prevent unnecessary effect runs.
watch('columns')andwatch('questions')return new array references on every render, causing the effect to run unnecessarily. Consider usingwatch()with a subscription or comparing a stable identifier.♻️ Suggested optimization
- const watchOasisSourceId = watch('oasis_source_id'); - const watchTableDescription = watch('table_description'); - const watchColumns = watch('columns') || []; - const watchQuestions = watch('questions') || []; - - useEffect(() => { - setSubmitError(null); - }, [watchOasisSourceId, watchTableDescription, watchColumns, watchQuestions]); + const watchedValues = watch(); + useEffect(() => { + setSubmitError(null); + }, [watchedValues.oasis_source_id, watchedValues.table_description, watchedValues.columns?.length, watchedValues.questions?.length]);Or use a subscription-based approach:
+ useEffect(() => { + const subscription = watch(() => setSubmitError(null)); + return () => subscription.unsubscribe(); + }, [watch]);🤖 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 `@frontend/src/components/wizard/OnboardingWizard.tsx` around lines 91 - 98, The useEffect hook in OnboardingWizard has a performance issue where watch('columns') and watch('questions') return new array references on every render, causing the effect with setSubmitError to run unnecessarily even when the actual data hasn't changed. To fix this, replace the direct watch calls for columns and questions in the dependency array with a subscription-based approach using watch() with a callback function, or use the field names as stable identifiers instead of the array values themselves. This will ensure the effect only runs when the actual field values change, not on every render due to reference changes.frontend/src/components/layout/AuthProvider.tsx (1)
11-20: ⚡ Quick winRedundant
setLoading(false)in finally block.The
setAuthfunction already setsisLoading: false(authStore.ts line 18), so thesetLoading(false)call in the finally block is redundant. Both the success and error branches callsetAuth, which handles the loading state.♻️ Suggested cleanup
authApi.getMe() .then((user) => { if (mounted) setAuth(user); }) .catch(() => { if (mounted) setAuth(null); - }) - .finally(() => { - if (mounted) setLoading(false); });Alternatively, if you want to decouple loading state from auth state, consider having
setAuthnot modifyisLoading, and rely solely on the explicitsetLoadingcalls.🤖 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 `@frontend/src/components/layout/AuthProvider.tsx` around lines 11 - 20, The finally block in the promise chain for authApi.getMe() contains a redundant setLoading(false) call. Since the setAuth function already sets isLoading to false in authStore.ts (line 18), and both the success and error branches call setAuth, the explicit setLoading(false) in the finally block is unnecessary. Remove the finally block entirely, including the setLoading(false) call and the mounted check associated with it.frontend/src/components/layout/Sidebar.tsx (2)
107-108: ⚡ Quick winReplace string-based admin check with a more robust approach.
The check
group.label === 'Administration'is fragile. If someone renames the label for i18n or UX reasons, the admin navigation will become visible to non-admins (though actual route protection viaProtectedAdminRoutewould still apply).♻️ Recommended fix
Add an
adminOnlyflag to the group definition:{ label: 'Administration', + adminOnly: true, items: [{ to: '/admin', icon: Shield, key: 'nav.admin', label: 'Admin Panel' }], },Then update the rendering logic:
- {NAV_GROUPS.map((group) => { - if (group.label === 'Administration' && !user?.is_admin) return null; + {NAV_GROUPS.map((group) => { + if (group.adminOnly && !user?.is_admin) return null; return (🤖 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 `@frontend/src/components/layout/Sidebar.tsx` around lines 107 - 108, The admin check in the Sidebar component uses a fragile string comparison on group.label === 'Administration' which will break if the label is renamed for i18n or other reasons. Add an adminOnly boolean property to each group definition in NAV_GROUPS (set to true for the Administration group and false or omitted for others), then replace the string-based conditional check in the map function to use this new adminOnly flag instead of comparing the group.label.
136-213: ⚖️ Poor tradeoffConsider extracting inline styles to CSS classes.
The user panel contains extensive inline style objects (lines 137-212) that make the component harder to read and maintain. While functional, extracting these to CSS classes or a styled component would improve maintainability.
🤖 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 `@frontend/src/components/layout/Sidebar.tsx` around lines 136 - 213, Extract the extensive inline styles from the user panel section in the Sidebar component (the container div, avatar circle div, text wrapper, name div, email div, and logout button) into CSS classes or a styled component. Replace all inline style objects with appropriate className references. Additionally, convert the dynamic styles applied via onMouseEnter and onMouseLeave event handlers on the logout button to CSS pseudo-classes like :hover to eliminate the need for JavaScript event handlers managing inline styles.frontend/src/api/auth.ts (1)
7-7: ⚡ Quick winRemove duplicate User type export.
The
Usertype is already exported fromauthStore.ts(line 4). Having duplicate type exports can cause import confusion and maintenance overhead.♻️ Suggested fix
-export type User = components["schemas"]["SecurityUserRead"]; - export const authApi = {Then update imports in other files to use
UserfromauthStore.tsconsistently.🤖 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 `@frontend/src/api/auth.ts` at line 7, The User type is being exported from both auth.ts and authStore.ts, creating duplication. Remove the `export type User = components["schemas"]["SecurityUserRead"];` export statement from auth.ts, and then update all imports throughout the codebase that currently import User from auth.ts to instead import it from authStore.ts where it is already exported.backend/app/main.py (1)
61-66: ⚡ Quick winMake the session cookie security attributes explicit.
Auth now depends on this cookie for every protected API call, but the middleware is still relying on implicit cookie flags. Please wire the intended
https_onlyandsame_sitepolicy from config so production does not silently ship a weaker or incompatible session cookie.🤖 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 `@backend/app/main.py` around lines 61 - 66, The SessionMiddleware initialization is missing explicit security attributes for the session cookie. Add the https_only and same_site parameters to the app.add_middleware call for SessionMiddleware, sourcing these values from the auth_settings configuration object (similar to how SESSION_SECRET_KEY, SESSION_COOKIE_NAME, and SESSION_COOKIE_MAX_AGE are already being pulled). This ensures that production deployments use the intended security policy for session cookies rather than relying on implicit defaults.
🤖 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 @.gitignore:
- Line 37: The `*.conf` pattern is too broad and may inadvertently exclude
configuration files that should be version-controlled. Replace this overly
general pattern with a more specific one that targets only the files you intend
to ignore, such as `snowflake*.conf`, or alternatively move this rule into a
subdirectory-specific .gitignore file if it only applies to local environment
configurations in a particular directory. This ensures legitimate config files
remain tracked in version control while still excluding the intended temporary
or local configuration files.
In `@backend/alembic/versions/f4d69a17d156_add_organization_and_sso_fields.py`:
- Around line 43-47: The SSO migration in
f4d69a17d156_add_organization_and_sso_fields.py contains destructive cleanup
operations (dropping http_extractors table, admins table, and embedding column)
that are unrelated to SSO functionality and have no data preservation mechanism.
Remove all the drop_index, drop_table, and drop_column calls from this migration
file, and instead create a separate dedicated retirement migration that includes
explicit data archival or backfill steps before performing these destructive
operations. This ensures that rolling out the SSO migration does not
inadvertently delete unrelated data.
- Line 53: In the op.add_column call for the embedding column in the tables
table, replace sa.NullType() with the actual original column type that was used
when the embedding column was first created. Investigate the database schema
history or earlier migration files to determine the correct type (such as
sa.String(), sa.LargeBinary(), sa.Text(), etc.) and update the column definition
to use that type instead of NullType(), ensuring that downgrades can properly
restore the column with the correct schema.
In `@backend/app/api/auth.py`:
- Around line 14-16: The auth callback code directly uses a module-level
`engine` import to create the database session, but this binding happens at
module load time and prevents tests from swapping in a test database engine.
Replace the direct `engine` reference in the Session creation (where
sync_user_from_payload is called) with a dynamic resolution at call time, such
as accessing the engine through the app.db.engine module path. This ensures that
when tests swap out the engine for testing purposes, the auth callback will use
the correct test database instead of the original engine that was captured at
module import time.
In `@backend/app/core/auth.py`:
- Around line 38-53: The sync_user_from_payload function only appends
organizations to the user but never removes stale memberships. When the groups
claim is present, treat it as the source of truth for which organizations the
user should belong to. After processing the cleaned_groups and adding missing
organizations, you must also remove any organizations from user.organizations
that are not in the reconciled set of organizations derived from the groups
claim. This ensures that when a user is removed from a group in the IdP, their
stale organization membership is properly cleaned up in the database.
- Around line 14-31: The code validates that either email or sso_id is provided,
but still creates a new SecurityUser object even when email is None and only
sso_id exists. Since SecurityUser.email is a required field, this causes a
database integrity error at commit time. Before the SecurityUser object creation
block, add a check that ensures email is present (not None/empty) when creating
a new user. If email is missing, raise a ValueError to prevent creating an
incomplete user record that will fail at db.commit().
In `@backend/app/infra_init.py`:
- Around line 1016-1022: The thread created in the profiling initialization (the
one launched with target=_run_profile and daemon=True) will be forcibly
terminated when the main process exits, leaving TableProfile records stuck in
"running" status. To fix this, either change daemon=True to daemon=False and
implement proper thread shutdown handling to ensure graceful termination of
profiling tasks, or add startup initialization logic that queries for and resets
any stale TableProfile records that remain in "running" state from previous
interrupted runs back to "failed" state. If choosing the second approach, this
cleanup should execute early in the application startup sequence before normal
operations begin.
- Around line 938-942: In the code that retrieves a TableProfile using
session.get(TableProfile, profile_id), when the profile is not found and the
function returns early, the previously created profile record remains in
"running" status indefinitely. Before the early return statement when profile is
None, either log a warning message to indicate the missing profile or update the
profile status to "failed" to prevent the orphaned running status from
persisting during concurrent operations or external deletions.
- Around line 47-48: The `_SYSTEM_OWNER_ID` constant defined here is used as the
owner_id for Airlines tables later in the code (around line 896 in
infra_init.py), but no corresponding `SecurityUser` record with id="system" is
created during database initialization, leaving orphaned references. Add
creation of a `SecurityUser` record with id="system" either in the security
users migration code or directly in the initialization logic in `infra_init.py`
before the Airlines tables are inserted to ensure referential integrity is
maintained.
In `@backend/app/routers/tables.py`:
- Around line 35-40: The OpenMetadata login credentials in the httpx.post call
are hard-coded with a fixed email "admin@open-metadata.org" and base64-encoded
password "admin", which creates a security vulnerability and assumes every
deployment uses identical credentials. Replace the hard-coded email and password
values with configuration/secrets settings (similar to how
settings.OPENMETADATA_URL is already used), or use an existing token-based
authentication mechanism if available. Remove the base64 encoding of "admin" and
instead reference the credential values from the settings configuration.
- Around line 53-63: The om_get_request function has hardcoded verify=False in
both httpx.get calls, creating a MITM vulnerability. Add an OPENMETADATA_VERIFY
configuration option (similar to the existing TRINO_VERIFY pattern) and use it
instead of the hardcoded verify=False in both httpx.get calls within the
om_get_request function to allow standard SSL verification or custom CA bundle
paths. Apply the same configurable verification pattern to any other
OpenMetadata requests, including the TRINO_SERVICE_URL call mentioned in the
review, to maintain consistent secure certificate handling across all external
HTTP requests.
In `@backend/Dockerfile`:
- Line 8: The ssh-keyscan command on line 8 creates a TOFU vulnerability by
dynamically discovering GitHub's host key during the build, which could be
intercepted by a MITM attacker before line 14 uses --mount=type=ssh to forward
SSH credentials for dependency fetching. Replace the dynamic ssh-keyscan
approach with a pinned GitHub host key that is either committed to the
repository (as a file to be copied into the Docker image) or injected as a build
secret, ensuring the SSH host key verification cannot be compromised during the
build process.
In `@backend/pyproject.toml`:
- Line 26: In the backend/pyproject.toml file, replace the mutable feature
branch reference `feat/google-sso` in the python-core-utils dependency URL with
the specific commit hash `49c9ecc832cb74e750da2e6f858d62df36f2fbe2`. This
ensures that the dependency resolution remains stable and predictable,
preventing issues from branch force-pushes or deletions. Change the git URL
portion from `@feat/google-sso` to `@49c9ecc832cb74e750da2e6f858d62df36f2fbe2`
while keeping the rest of the dependency specification intact.
In `@backend/tests/conftest.py`:
- Around line 118-125: The override_get_current_user function in the test
fixture returns a detached SecurityUser object that does not exist in the
database, which bypasses session-backed lookups and prevents tests from catching
real session handling issues or DB-backed relationship failures. Instead of
returning a hardcoded SecurityUser, seed an actual auth user in the database
(using the appropriate database session/transaction available in the fixture),
retrieve that user through the normal session-backed lookup mechanism, and
return that real persisted user object so tests properly validate session
handling and database relationships.
In `@docker-compose.yml`:
- Around line 426-437: The environment variables in the `environment` block are
being interpolated by Docker Compose to empty strings because they are not
defined in a project-level `.env` file at the Docker Compose root. To fix this,
create a `.env` file in the same directory as docker-compose.yml and define all
the required variables (LANGFUSE_PUBLIC_KEY, LANGFUSE_SECRET_KEY, LANGFUSE_HOST,
ENABLE_GOOGLE, GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET, ENABLE_KEYCLOAK,
KEYCLOAK_SERVER_URL, KEYCLOAK_CLIENT_ID, KEYCLOAK_CLIENT_SECRET,
FRONTEND_DEFAULT_REDIRECT) with appropriate values. Alternatively, if these
variables should come from an `env_file` directive (which typically appears
after the `environment` block), remove the `${...}` interpolation syntax from
the `environment` block and rely solely on the `env_file` approach, since
`environment` takes precedence and overrides `env_file` values.
In `@frontend/src/App.tsx`:
- Around line 52-67: The ProtectedAdminRoute and ProtectedRoute functions in
frontend/src/App.tsx lines 52-67 currently discard the user's originally
requested URL when redirecting to login or control-center. To fix this, in the
Navigate components, pass the current location (pathname + search + hash)
through the state parameter so the destination page knows where to redirect back
to after authentication. Then, in frontend/src/pages/LoginPage.tsx lines 51-66,
read that stored destination from the location state and use it to construct the
next_url parameter for the SSO provider instead of hardcoding it to
"/control-center", ensuring users return to the page they actually requested.
In `@frontend/src/pages/LoginPage.tsx`:
- Around line 15-19: The LoginPage is masking getConfig() errors by treating
them the same as "no providers configured", which hides login buttons on
transient failures and delays authenticated user redirects. Add an explicit
error state variable to track getConfig() failures separately from
configLoading. In the useEffect hook at the getConfig() call, update the catch
block to set this error state instead of just logging. Then at the authenticated
user redirect logic (around lines 22-31), check if the user is already
authenticated and redirect them immediately without waiting for configLoading to
complete. Finally, update the "no providers configured" fallback (around lines
73-75) to only display when there is actually no config AND no error state,
allowing transient errors to be distinguished from missing providers.
---
Outside diff comments:
In `@backend/app/infra_init.py`:
- Around line 509-511: The logger.info call following the final
_trino_exec("SHOW SCHEMAS FROM minio") statement is unreachable code. If the
Trino readiness check succeeds, the function would have already returned during
one of the retry iterations in the preceding loop; if it fails, an exception is
raised. Remove the unreachable logger.info statement that logs the Trino minio
catalog readiness message after the final _trino_exec call, or restructure the
retry logic to ensure success cases properly log completion before returning.
In `@backend/app/main.py`:
- Around line 69-90: The audit_middleware function is using a hardcoded fake
user ID "user-1" instead of extracting the actual authenticated user from the
request. Replace the hardcoded user_id assignment in the audit_middleware
function with code that retrieves the current authenticated user (using the same
authentication mechanism like get_current_user that is used in other protected
routes) and uses their actual user ID in the audit log. This ensures audit
records are correctly attributed to real users rather than a fake placeholder
user.
---
Nitpick comments:
In `@backend/app/infra_init.py`:
- Around line 1015-1026: The current implementation spawns one thread per table
without any concurrency limit, which could exhaust system resources if the
number of tables grows large. Replace the unbounded thread creation in the loop
that processes table_meta by creating a ThreadPoolExecutor with a fixed pool
size at the start of the function, then use executor.submit() to submit
_run_profile tasks to the pool instead of creating individual threading.Thread
objects. This ensures only a bounded number of profiling threads run
concurrently while still maintaining the same logging and functionality.
In `@backend/app/main.py`:
- Around line 61-66: The SessionMiddleware initialization is missing explicit
security attributes for the session cookie. Add the https_only and same_site
parameters to the app.add_middleware call for SessionMiddleware, sourcing these
values from the auth_settings configuration object (similar to how
SESSION_SECRET_KEY, SESSION_COOKIE_NAME, and SESSION_COOKIE_MAX_AGE are already
being pulled). This ensures that production deployments use the intended
security policy for session cookies rather than relying on implicit defaults.
In `@frontend/src/api/auth.ts`:
- Line 7: The User type is being exported from both auth.ts and authStore.ts,
creating duplication. Remove the `export type User =
components["schemas"]["SecurityUserRead"];` export statement from auth.ts, and
then update all imports throughout the codebase that currently import User from
auth.ts to instead import it from authStore.ts where it is already exported.
In `@frontend/src/components/layout/AuthProvider.tsx`:
- Around line 11-20: The finally block in the promise chain for authApi.getMe()
contains a redundant setLoading(false) call. Since the setAuth function already
sets isLoading to false in authStore.ts (line 18), and both the success and
error branches call setAuth, the explicit setLoading(false) in the finally block
is unnecessary. Remove the finally block entirely, including the
setLoading(false) call and the mounted check associated with it.
In `@frontend/src/components/layout/Sidebar.tsx`:
- Around line 107-108: The admin check in the Sidebar component uses a fragile
string comparison on group.label === 'Administration' which will break if the
label is renamed for i18n or other reasons. Add an adminOnly boolean property to
each group definition in NAV_GROUPS (set to true for the Administration group
and false or omitted for others), then replace the string-based conditional
check in the map function to use this new adminOnly flag instead of comparing
the group.label.
- Around line 136-213: Extract the extensive inline styles from the user panel
section in the Sidebar component (the container div, avatar circle div, text
wrapper, name div, email div, and logout button) into CSS classes or a styled
component. Replace all inline style objects with appropriate className
references. Additionally, convert the dynamic styles applied via onMouseEnter
and onMouseLeave event handlers on the logout button to CSS pseudo-classes like
:hover to eliminate the need for JavaScript event handlers managing inline
styles.
In `@frontend/src/components/wizard/OnboardingWizard.tsx`:
- Around line 91-98: The useEffect hook in OnboardingWizard has a performance
issue where watch('columns') and watch('questions') return new array references
on every render, causing the effect with setSubmitError to run unnecessarily
even when the actual data hasn't changed. To fix this, replace the direct watch
calls for columns and questions in the dependency array with a
subscription-based approach using watch() with a callback function, or use the
field names as stable identifiers instead of the array values themselves. This
will ensure the effect only runs when the actual field values change, not on
every render due to reference changes.
In `@infra/trino/etc/catalog/airlines.properties`:
- Around line 2-3: The connection-url and connection-user properties in the
Snowflake catalog configuration are hardcoded with specific account identifier
and username values. Replace the hardcoded account identifier in the
connection-url property (currently RSRSBDK-YDB67606) and the connection-user
property value (currently BARVAZ_HAMOOD) with environment variable references
using the same parameterization pattern already applied to the password,
ensuring the configuration can be easily reused across different environments by
adjusting environment variables rather than modifying the file.
🪄 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 Plus
Run ID: b5374745-3a68-47e4-925d-94fb37413d19
⛔ Files ignored due to path filters (2)
backend/uv.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.gitignorebackend/Dockerfilebackend/alembic/versions/f4d69a17d156_add_organization_and_sso_fields.pybackend/app/api/auth.pybackend/app/config.pybackend/app/core/auth.pybackend/app/infra_init.pybackend/app/main.pybackend/app/models/models.pybackend/app/routers/admin_approval.pybackend/app/routers/tables.pybackend/pyproject.tomlbackend/tests/conftest.pydocker-compose.ymlfrontend/package.jsonfrontend/src/App.tsxfrontend/src/api/auth.tsfrontend/src/api/client.tsfrontend/src/api/orchestration.tsfrontend/src/api/schema.d.tsfrontend/src/components/layout/AuthProvider.tsxfrontend/src/components/layout/Sidebar.tsxfrontend/src/components/tables/TableList.tsxfrontend/src/components/wizard/OnboardingWizard.tsxfrontend/src/pages/LandingPage.tsxfrontend/src/pages/LoginPage.tsxfrontend/src/store/authStore.tsfrontend/src/styles/globals.cssinfra/trino/etc/catalog/airlines.propertiesinfra/trino/etc/jvm.config
|
|
||
| # Snowflake local config | ||
| snowflake.conf | ||
| *.conf |
There was a problem hiding this comment.
Overly broad ignore pattern may hide legitimate config files.
The *.conf pattern ignores all .conf files throughout the repository, which may inadvertently exclude configuration files that should be version-controlled. Consider using a more specific pattern like snowflake*.conf or placing this rule in a subdirectory-specific .gitignore if the intent is to ignore only local Snowflake configuration files.
📝 Suggested refinement
# Snowflake local config
snowflake.conf
-*.conf
+snowflake*.conf🤖 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 @.gitignore at line 37, The `*.conf` pattern is too broad and may
inadvertently exclude configuration files that should be version-controlled.
Replace this overly general pattern with a more specific one that targets only
the files you intend to ignore, such as `snowflake*.conf`, or alternatively move
this rule into a subdirectory-specific .gitignore file if it only applies to
local environment configurations in a particular directory. This ensures
legitimate config files remain tracked in version control while still excluding
the intended temporary or local configuration files.
Comment on lines
+43
to
+47
| op.drop_index('ix_http_extractors_name', table_name='http_extractors') | ||
| op.drop_table('http_extractors') | ||
| op.drop_index('ix_admins_username', table_name='admins') | ||
| op.drop_table('admins') | ||
| op.drop_column('tables', 'embedding') |
There was a problem hiding this comment.
Split the destructive cleanup out of this SSO migration.
This upgrade drops http_extractors, admins, and tables.embedding outright with no archive/backfill step. If any environment still has live data there, rolling out SSO will also irreversibly delete unrelated data. Please move that cleanup into a dedicated retirement migration with an explicit data plan.
🤖 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 `@backend/alembic/versions/f4d69a17d156_add_organization_and_sso_fields.py`
around lines 43 - 47, The SSO migration in
f4d69a17d156_add_organization_and_sso_fields.py contains destructive cleanup
operations (dropping http_extractors table, admins table, and embedding column)
that are unrelated to SSO functionality and have no data preservation mechanism.
Remove all the drop_index, drop_table, and drop_column calls from this migration
file, and instead create a separate dedicated retirement migration that includes
explicit data archival or backfill steps before performing these destructive
operations. This ensures that rolling out the SSO migration does not
inadvertently delete unrelated data.
Comment on lines
+14
to
+16
| with Session(engine) as db: | ||
| user = sync_user_from_payload(db, userinfo, provider) | ||
| request.session["user_id"] = user.id |
There was a problem hiding this comment.
Avoid binding auth writes to the module-level engine import.
backend/tests/conftest.py only swaps app.db.engine.engine; this alias keeps the original engine object. Any test that exercises the auth callback will write to the wrong database unless you resolve the engine/session at call time instead of capturing it here.
🤖 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 `@backend/app/api/auth.py` around lines 14 - 16, The auth callback code
directly uses a module-level `engine` import to create the database session, but
this binding happens at module load time and prevents tests from swapping in a
test database engine. Replace the direct `engine` reference in the Session
creation (where sync_user_from_payload is called) with a dynamic resolution at
call time, such as accessing the engine through the app.db.engine module path.
This ensures that when tests swap out the engine for testing purposes, the auth
callback will use the correct test database instead of the original engine that
was captured at module import time.
Comment on lines
+14
to
+31
| if not email and not sso_id: | ||
| raise ValueError("Invalid user payload") | ||
|
|
||
| user = None | ||
| if sso_id: | ||
| user = db.exec(select(SecurityUser).where(SecurityUser.sso_id == sso_id)).first() | ||
|
|
||
| if not user and email: | ||
| user = db.exec(select(SecurityUser).where(SecurityUser.email == email)).first() | ||
|
|
||
| if not user: | ||
| user = SecurityUser( | ||
| email=email, | ||
| name=name, | ||
| sso_id=sso_id, | ||
| provider=provider | ||
| ) | ||
| db.add(user) |
There was a problem hiding this comment.
Guard the new-user path when the IdP omits email.
SecurityUser.email is required, but this branch still creates a new row when only sub is present. The first such login will fail on db.commit() with an integrity error and turn authentication into a 500.
🤖 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 `@backend/app/core/auth.py` around lines 14 - 31, The code validates that
either email or sso_id is provided, but still creates a new SecurityUser object
even when email is None and only sso_id exists. Since SecurityUser.email is a
required field, this causes a database integrity error at commit time. Before
the SecurityUser object creation block, add a check that ensures email is
present (not None/empty) when creating a new user. If email is missing, raise a
ValueError to prevent creating an incomplete user record that will fail at
db.commit().
| "passlib[bcrypt]==1.7.4", | ||
| "requests==2.32.3", | ||
| "minio>=7.2.0", | ||
| "python-core-utils[keycloak] @ git+ssh://git@github.com/matzpen-agency/python-core-utils.git@feat/google-sso#egg=python-core-utils", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "pyproject.toml" -path "*/backend/*" -type fRepository: StavPonte11/text2sql-onboarding
Length of output: 100
🏁 Script executed:
find . -name "uv.lock" -path "*/backend/*" -type fRepository: StavPonte11/text2sql-onboarding
Length of output: 93
🏁 Script executed:
sed -n '20,35p' ./backend/pyproject.tomlRepository: StavPonte11/text2sql-onboarding
Length of output: 534
🏁 Script executed:
grep -A 10 "python-core-utils" ./backend/uv.lock | head -20Repository: StavPonte11/text2sql-onboarding
Length of output: 707
Pin the python-core-utils dependency to the reviewed commit instead of the mutable feature branch.
The manifest currently points to feat/google-sso, which is a mutable reference. Although the lock file resolves to commit 49c9ecc832cb74e750da2e6f858d62df36f2fbe2, any future uv lock or fresh resolution can pick up force-pushed changes or fail if the branch is deleted. Pin the dependency to the commit hash (or a release tag) to keep main stable and predictable.
Suggested fix
- "python-core-utils[keycloak] @ git+ssh://git@github.com/matzpen-agency/python-core-utils.git@feat/google-sso#egg=python-core-utils",
+ "python-core-utils[keycloak] @ git+ssh://git@github.com/matzpen-agency/python-core-utils.git@49c9ecc832cb74e750da2e6f858d62df36f2fbe2#egg=python-core-utils",📝 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
| "python-core-utils[keycloak] @ git+ssh://git@github.com/matzpen-agency/python-core-utils.git@feat/google-sso#egg=python-core-utils", | |
| "python-core-utils[keycloak] @ git+ssh://git@github.com/matzpen-agency/python-core-utils.git@49c9ecc832cb74e750da2e6f858d62df36f2fbe2#egg=python-core-utils", |
🤖 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 `@backend/pyproject.toml` at line 26, In the backend/pyproject.toml file,
replace the mutable feature branch reference `feat/google-sso` in the
python-core-utils dependency URL with the specific commit hash
`49c9ecc832cb74e750da2e6f858d62df36f2fbe2`. This ensures that the dependency
resolution remains stable and predictable, preventing issues from branch
force-pushes or deletions. Change the git URL portion from `@feat/google-sso` to
`@49c9ecc832cb74e750da2e6f858d62df36f2fbe2` while keeping the rest of the
dependency specification intact.
Comment on lines
+118
to
+125
| def override_get_current_user(): | ||
| return SecurityUser( | ||
| id="test-user-id", | ||
| email="test-user@example.com", | ||
| name="Test User", | ||
| is_active=True, | ||
| is_admin=True, | ||
| ) |
There was a problem hiding this comment.
Seed a real auth user for tests instead of returning a detached model.
This override bypasses the new session-backed lookup entirely and returns a SecurityUser that does not exist in security.users. Tests using client will miss broken session handling, and anything that later depends on DB-backed relationships or org memberships can pass here but fail in production.
🤖 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 `@backend/tests/conftest.py` around lines 118 - 125, The
override_get_current_user function in the test fixture returns a detached
SecurityUser object that does not exist in the database, which bypasses
session-backed lookups and prevents tests from catching real session handling
issues or DB-backed relationship failures. Instead of returning a hardcoded
SecurityUser, seed an actual auth user in the database (using the appropriate
database session/transaction available in the fixture), retrieve that user
through the normal session-backed lookup mechanism, and return that real
persisted user object so tests properly validate session handling and database
relationships.
Comment on lines
+426
to
437
| - LANGFUSE_PUBLIC_KEY=${LANGFUSE_PUBLIC_KEY} | ||
| - LANGFUSE_SECRET_KEY=${LANGFUSE_SECRET_KEY} | ||
| - LANGFUSE_HOST=${LANGFUSE_HOST} | ||
| - ENABLE_GOOGLE=${ENABLE_GOOGLE} | ||
| - GOOGLE_CLIENT_ID=${GOOGLE_CLIENT_ID} | ||
| - GOOGLE_CLIENT_SECRET=${GOOGLE_CLIENT_SECRET} | ||
| - ENABLE_KEYCLOAK=${ENABLE_KEYCLOAK} | ||
| - KEYCLOAK_SERVER_URL=${KEYCLOAK_SERVER_URL} | ||
| - KEYCLOAK_CLIENT_ID=${KEYCLOAK_CLIENT_ID} | ||
| - KEYCLOAK_CLIENT_SECRET=${KEYCLOAK_CLIENT_SECRET} | ||
| - FRONTEND_DEFAULT_REDIRECT=${FRONTEND_DEFAULT_REDIRECT} | ||
| depends_on: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "docker-compose.yml" -o -name "docker-compose.yaml" | head -5Repository: StavPonte11/text2sql-onboarding
Length of output: 96
🏁 Script executed:
cat -n docker-compose.yml | sed -n '400,450p'Repository: StavPonte11/text2sql-onboarding
Length of output: 2184
🏁 Script executed:
cat -n backend/.env 2>/dev/null | head -100Repository: StavPonte11/text2sql-onboarding
Length of output: 57
🏁 Script executed:
cat -n backend/.env 2>/dev/null || echo "backend/.env is empty or does not exist"Repository: StavPonte11/text2sql-onboarding
Length of output: 115
🏁 Script executed:
find . -name ".env" -o -name ".env.example" | head -10Repository: StavPonte11/text2sql-onboarding
Length of output: 114
🏁 Script executed:
cat -n .env 2>/dev/null | head -50Repository: StavPonte11/text2sql-onboarding
Length of output: 57
🏁 Script executed:
# Check if there's documentation or example files
ls -la | grep -i env
cat -n .env.example 2>/dev/null | head -50Repository: StavPonte11/text2sql-onboarding
Length of output: 57
🌐 Web query:
Docker Compose environment variable precedence env_file vs environment block
💡 Result:
When configuring environment variables in Docker Compose, the environment block has explicit precedence over the env_file attribute [1][2][3]. If the same variable is defined in both, the value specified in the environment block will be used within the container [1][3]. It is important to distinguish between how environment variables are set inside the container and how variable interpolation works within the compose.yaml file itself [4][5]. Container Environment Precedence The following hierarchy determines the final value of an environment variable inside a container, from highest to lowest precedence [1]: 1. Command line: Using docker compose run -e VARIABLE=value. 2. Environment block: Variables defined directly in the environment section of the compose.yaml file [1][3]. 3. Env file: Variables loaded via the env_file attribute in the compose.yaml file [1][2][3]. 4. Dockerfile: Values defined in the image using the ENV directive. Note that variables defined in the environment section or loaded via env_file override any values set in the container image's Dockerfile [1]. Important Distinction: Interpolation vs. Container Environment The env_file attribute and the environment block are used to set variables inside the container; they do not perform variable interpolation for the compose.yaml file itself [4]. Interpolation (e.g., using ${VARIABLE} in your config) is handled separately by Docker Compose, typically using the .env file in your project directory or shell environment variables [6][4][7]. Values used for interpolation are resolved before the container configuration is fully processed [7].
Citations:
- 1: https://docs.docker.com/compose/how-tos/environment-variables/envvars-precedence/
- 2: https://github.com/docker/compose/blob/a6649b308e05a7a0bc65b607930e2aac5265c3fc/docs/compose-file.md
- 3: https://github.com/compose-spec/compose-spec/blob/a4a7e7c/spec.md
- 4: [BUG] environment variable not correctly overload when using multiple .env files docker/compose#11870
- 5: https://docs.docker.com/compose/how-tos/environment-variables/
- 6: https://docs.docker.com/compose/how-tos/environment-variables/variable-interpolation/
- 7: https://docs.docker.com/reference/compose-file/interpolation/
🏁 Script executed:
# Check if there's a project-level .env file
cat -n .env 2>/dev/null || echo "No .env file found at project root"Repository: StavPonte11/text2sql-onboarding
Length of output: 110
🏁 Script executed:
# Check backend/.env.example to see what variables should be defined
cat -n backend/.env.example 2>/dev/null | head -100Repository: StavPonte11/text2sql-onboarding
Length of output: 1039
🏁 Script executed:
# Search for where these variables might be defined
rg "LANGFUSE_PUBLIC_KEY|LANGFUSE_SECRET_KEY|ENABLE_GOOGLE" --type-list 2>/dev/null | head -20Repository: StavPonte11/text2sql-onboarding
Length of output: 57
🏁 Script executed:
# Check git history to see if this environment block is new
git log --oneline -20 -- docker-compose.ymlRepository: StavPonte11/text2sql-onboarding
Length of output: 112
🏁 Script executed:
# Check if backend/.env is gitignored
cat .gitignore 2>/dev/null | grep -i "\.env"Repository: StavPonte11/text2sql-onboarding
Length of output: 81
🏁 Script executed:
# Check git status for backend/.env
git status backend/.env 2>&1 || echo "Checking git log for .env deletion..."
git log --all --full-history -- backend/.env | head -20Repository: StavPonte11/text2sql-onboarding
Length of output: 496
🏁 Script executed:
# Verify the actual file structure
ls -la backend/ | grep -E "\.env|Dockerfile"Repository: StavPonte11/text2sql-onboarding
Length of output: 203
🏁 Script executed:
# Check if there's CI configuration that might set these variables
find . -name "*.yml" -o -name "*.yaml" | grep -E "(github|gitlab|circleci|jenkins)" | head -10
ls -la .github/ 2>/dev/null || ls -la .gitlab-ci.yml 2>/dev/null || echo "No CI config found"Repository: StavPonte11/text2sql-onboarding
Length of output: 94
🏁 Script executed:
# Check if there's any documentation about env setup
find . -name "README*" -o -name "SETUP*" -o -name "ENV*" | xargs cat 2>/dev/null | head -100Repository: StavPonte11/text2sql-onboarding
Length of output: 3987
🏁 Script executed:
# Look for any compose override files or documentation
ls -la docker-compose* .env*Repository: StavPonte11/text2sql-onboarding
Length of output: 215
Move interpolated environment variables to project-level .env or document shell-level setup.
The ${...} expressions in the environment block (lines 426-436) are resolved by Docker Compose before any service configuration is applied. These variables are not in a project-level .env file, so they interpolate to empty strings. Since environment takes precedence over env_file, this sets LANGFUSE_PUBLIC_KEY, LANGFUSE_SECRET_KEY, ENABLE_GOOGLE, and related settings to empty values in the container, preventing Langfuse and SSO from functioning. Either move these variables to a project-level .env file (which Docker Compose will use for interpolation), set them in the shell environment before running docker-compose up, or hardcode non-sensitive defaults in the environment block.
🤖 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 `@docker-compose.yml` around lines 426 - 437, The environment variables in the
`environment` block are being interpolated by Docker Compose to empty strings
because they are not defined in a project-level `.env` file at the Docker
Compose root. To fix this, create a `.env` file in the same directory as
docker-compose.yml and define all the required variables (LANGFUSE_PUBLIC_KEY,
LANGFUSE_SECRET_KEY, LANGFUSE_HOST, ENABLE_GOOGLE, GOOGLE_CLIENT_ID,
GOOGLE_CLIENT_SECRET, ENABLE_KEYCLOAK, KEYCLOAK_SERVER_URL, KEYCLOAK_CLIENT_ID,
KEYCLOAK_CLIENT_SECRET, FRONTEND_DEFAULT_REDIRECT) with appropriate values.
Alternatively, if these variables should come from an `env_file` directive
(which typically appears after the `environment` block), remove the `${...}`
interpolation syntax from the `environment` block and rely solely on the
`env_file` approach, since `environment` takes precedence and overrides
`env_file` values.
Comment on lines
52
to
+67
| function ProtectedAdminRoute({ children }: { children: React.ReactNode }) { | ||
| const isAuthenticated = useAdminStore((state) => state.isAuthenticated); | ||
| const { user, isAuthenticated, isLoading } = useAuthStore(); | ||
|
|
||
| if (isLoading) return <div style={{ padding: '2rem', textAlign: 'center' }}>Loading...</div>; | ||
| if (!isAuthenticated || !user?.is_admin) { | ||
| return <Navigate to="/control-center" replace />; | ||
| } | ||
| return <>{children}</>; | ||
| } | ||
|
|
||
| function ProtectedRoute({ children }: { children: React.ReactNode }) { | ||
| const { isAuthenticated, isLoading } = useAuthStore(); | ||
|
|
||
| if (isLoading) return <div style={{ padding: '2rem', textAlign: 'center' }}>Loading...</div>; | ||
| if (!isAuthenticated) { | ||
| return <Navigate to="/admin/login" replace />; | ||
| return <Navigate to="/login" replace />; |
There was a problem hiding this comment.
Preserve the originally requested route across the SSO flow. The current guards discard the active URL before redirecting, and LoginPage always sends the provider back to /control-center. Together, that means bookmarks, shared links, and expired-session reloads never return to the page the user actually asked for.
frontend/src/App.tsx#L52-L67: carrypathname + search + hashinto the redirect state for unauthenticated users instead of replacing everything with fixed destinations.frontend/src/pages/LoginPage.tsx#L51-L66: read that stored destination and buildnext_urlfrom it instead of hardcoding/control-center.
Suggested direction
--- a/frontend/src/App.tsx
+++ b/frontend/src/App.tsx
@@
-import { BrowserRouter, Navigate, Route, Routes } from 'react-router-dom';
+import { BrowserRouter, Navigate, Route, Routes, useLocation } from 'react-router-dom';
@@
function ProtectedAdminRoute({ children }: { children: React.ReactNode }) {
+ const location = useLocation();
const { user, isAuthenticated, isLoading } = useAuthStore();
if (isLoading) return <div style={{ padding: '2rem', textAlign: 'center' }}>Loading...</div>;
- if (!isAuthenticated || !user?.is_admin) {
+ if (!isAuthenticated) {
+ return <Navigate to="/login" replace state={{ from: location }} />;
+ }
+ if (!user?.is_admin) {
return <Navigate to="/control-center" replace />;
}
return <>{children}</>;
}
function ProtectedRoute({ children }: { children: React.ReactNode }) {
+ const location = useLocation();
const { isAuthenticated, isLoading } = useAuthStore();
if (isLoading) return <div style={{ padding: '2rem', textAlign: 'center' }}>Loading...</div>;
if (!isAuthenticated) {
- return <Navigate to="/login" replace />;
+ return <Navigate to="/login" replace state={{ from: location }} />;
}
return <>{children}</>;
}--- a/frontend/src/pages/LoginPage.tsx
+++ b/frontend/src/pages/LoginPage.tsx
@@
-import { Navigate } from 'react-router-dom';
+import { Navigate, useLocation } from 'react-router-dom';
@@
export function LoginPage() {
+ const location = useLocation();
const { isAuthenticated, isLoading } = useAuthStore();
@@
+ const from = (location.state as {
+ from?: { pathname: string; search?: string; hash?: string };
+ } | null)?.from;
+ const nextPath = from
+ ? `${from.pathname}${from.search ?? ''}${from.hash ?? ''}`
+ : '/control-center';
@@
- const nextUrl = encodeURIComponent(window.location.origin + '/control-center');
+ const nextUrl = encodeURIComponent(new URL(nextPath, window.location.origin).toString());
window.location.href = `${API_BASE_URL}/api/v1/auth/login/google?next_url=${nextUrl}`;
@@
- const nextUrl = encodeURIComponent(window.location.origin + '/control-center');
+ const nextUrl = encodeURIComponent(new URL(nextPath, window.location.origin).toString());
window.location.href = `${API_BASE_URL}/api/v1/auth/login/keycloak?next_url=${nextUrl}`;📝 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
| function ProtectedAdminRoute({ children }: { children: React.ReactNode }) { | |
| const isAuthenticated = useAdminStore((state) => state.isAuthenticated); | |
| const { user, isAuthenticated, isLoading } = useAuthStore(); | |
| if (isLoading) return <div style={{ padding: '2rem', textAlign: 'center' }}>Loading...</div>; | |
| if (!isAuthenticated || !user?.is_admin) { | |
| return <Navigate to="/control-center" replace />; | |
| } | |
| return <>{children}</>; | |
| } | |
| function ProtectedRoute({ children }: { children: React.ReactNode }) { | |
| const { isAuthenticated, isLoading } = useAuthStore(); | |
| if (isLoading) return <div style={{ padding: '2rem', textAlign: 'center' }}>Loading...</div>; | |
| if (!isAuthenticated) { | |
| return <Navigate to="/admin/login" replace />; | |
| return <Navigate to="/login" replace />; | |
| import { BrowserRouter, Navigate, Route, Routes, useLocation } from 'react-router-dom'; | |
| function ProtectedAdminRoute({ children }: { children: React.ReactNode }) { | |
| const location = useLocation(); | |
| const { user, isAuthenticated, isLoading } = useAuthStore(); | |
| if (isLoading) return <div style={{ padding: '2rem', textAlign: 'center' }}>Loading...</div>; | |
| if (!isAuthenticated) { | |
| return <Navigate to="/login" replace state={{ from: location }} />; | |
| } | |
| if (!user?.is_admin) { | |
| return <Navigate to="/control-center" replace />; | |
| } | |
| return <>{children}</>; | |
| } | |
| function ProtectedRoute({ children }: { children: React.ReactNode }) { | |
| const location = useLocation(); | |
| const { isAuthenticated, isLoading } = useAuthStore(); | |
| if (isLoading) return <div style={{ padding: '2rem', textAlign: 'center' }}>Loading...</div>; | |
| if (!isAuthenticated) { | |
| return <Navigate to="/login" replace state={{ from: location }} />; | |
| } | |
| return <>{children}</>; | |
| } |
Suggested change
| function ProtectedAdminRoute({ children }: { children: React.ReactNode }) { | |
| const isAuthenticated = useAdminStore((state) => state.isAuthenticated); | |
| const { user, isAuthenticated, isLoading } = useAuthStore(); | |
| if (isLoading) return <div style={{ padding: '2rem', textAlign: 'center' }}>Loading...</div>; | |
| if (!isAuthenticated || !user?.is_admin) { | |
| return <Navigate to="/control-center" replace />; | |
| } | |
| return <>{children}</>; | |
| } | |
| function ProtectedRoute({ children }: { children: React.ReactNode }) { | |
| const { isAuthenticated, isLoading } = useAuthStore(); | |
| if (isLoading) return <div style={{ padding: '2rem', textAlign: 'center' }}>Loading...</div>; | |
| if (!isAuthenticated) { | |
| return <Navigate to="/admin/login" replace />; | |
| return <Navigate to="/login" replace />; | |
| import { Navigate, useLocation } from 'react-router-dom'; | |
| export function LoginPage() { | |
| const location = useLocation(); | |
| const { isAuthenticated, isLoading } = useAuthStore(); | |
| const from = (location.state as { | |
| from?: { pathname: string; search?: string; hash?: string }; | |
| } | null)?.from; | |
| const nextPath = from | |
| ? `${from.pathname}${from.search ?? ''}${from.hash ?? ''}` | |
| : '/control-center'; | |
| onClick={() => { | |
| const nextUrl = encodeURIComponent(new URL(nextPath, window.location.origin).toString()); | |
| window.location.href = `${API_BASE_URL}/api/v1/auth/login/google?next_url=${nextUrl}`; | |
| }} | |
| > | |
| Sign in with Google | |
| </Button> | |
| )} | |
| {config?.ENABLE_KEYCLOAK && ( | |
| <Button | |
| size="large" | |
| block | |
| onClick={() => { | |
| const nextUrl = encodeURIComponent(new URL(nextPath, window.location.origin).toString()); | |
| window.location.href = `${API_BASE_URL}/api/v1/auth/login/keycloak?next_url=${nextUrl}`; |
📍 Affects 2 files
frontend/src/App.tsx#L52-L67(this comment)frontend/src/pages/LoginPage.tsx#L51-L66
🤖 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 `@frontend/src/App.tsx` around lines 52 - 67, The ProtectedAdminRoute and
ProtectedRoute functions in frontend/src/App.tsx lines 52-67 currently discard
the user's originally requested URL when redirecting to login or control-center.
To fix this, in the Navigate components, pass the current location (pathname +
search + hash) through the state parameter so the destination page knows where
to redirect back to after authentication. Then, in
frontend/src/pages/LoginPage.tsx lines 51-66, read that stored destination from
the location state and use it to construct the next_url parameter for the SSO
provider instead of hardcoding it to "/control-center", ensuring users return to
the page they actually requested.
Comment on lines
+15
to
+19
| useEffect(() => { | ||
| authApi.getConfig() | ||
| .then(setConfig) | ||
| .catch(console.error) | ||
| .finally(() => setConfigLoading(false)); |
There was a problem hiding this comment.
Don't collapse auth-config failures into "no providers configured".
getConfig() errors are only logged, then the page falls through to the "No authentication providers configured." branch. A transient network/backend failure therefore hides every login button, and an already-authenticated user still waits on that request before the redirect can happen. Track an explicit error state and short-circuit the authenticated redirect before depending on configLoading.
Also applies to: 22-31, 73-75
🤖 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 `@frontend/src/pages/LoginPage.tsx` around lines 15 - 19, The LoginPage is
masking getConfig() errors by treating them the same as "no providers
configured", which hides login buttons on transient failures and delays
authenticated user redirects. Add an explicit error state variable to track
getConfig() failures separately from configLoading. In the useEffect hook at the
getConfig() call, update the catch block to set this error state instead of just
logging. Then at the authenticated user redirect logic (around lines 22-31),
check if the user is already authenticated and redirect them immediately without
waiting for configLoading to complete. Finally, update the "no providers
configured" fallback (around lines 73-75) to only display when there is actually
no config AND no error state, allowing transient errors to be distinguished from
missing providers.
…d prefixed API routers
…orwarding in download_images.sh
… location errors occur in Trino
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/main.py (1)
96-97:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAudit middleware uses hardcoded
user_idinstead of the authenticated session user.The comment acknowledges this is a placeholder, but now that session auth is wired, the middleware should extract the real user. Since middleware runs before route handlers, you can read directly from the session:
user_id = request.session.get("user_id", "anonymous")This ensures audit logs reflect actual authenticated users.
🤖 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 `@backend/app/main.py` around lines 96 - 97, The audit middleware is using a hardcoded placeholder for user_id instead of reading from the authenticated session. Replace the hardcoded user_id assignment with a session lookup that reads the user_id from the request session object, providing "anonymous" as a fallback value if the user_id is not present in the session. This ensures audit logs capture the actual authenticated user rather than the placeholder value.backend/app/routers/tables.py (1)
149-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn server-side status codes for upstream metadata failures.
Both fallback
except Exceptionblocks convert upstream/network failures into400, which misclassifies server-side outages as client input errors and harms client retry behavior.Suggested fix
- except Exception as e: - raise HTTPException( - status_code=400, detail=f"Failed to fetch table metadata: {e!s}" - ) + except httpx.RequestError as e: + raise HTTPException( + status_code=502, detail=f"Metadata service unavailable: {e!s}" + ) + except Exception as e: + raise HTTPException( + status_code=500, detail=f"Unexpected metadata parsing error: {e!s}" + )- except Exception as e: - raise HTTPException( - status_code=400, detail=f"Failed to refetch table metadata: {e!s}" - ) + except httpx.RequestError as e: + raise HTTPException( + status_code=502, detail=f"Metadata service unavailable: {e!s}" + ) + except Exception as e: + raise HTTPException( + status_code=500, detail=f"Unexpected metadata parsing error: {e!s}" + )Also applies to: 248-251
🤖 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 `@backend/app/routers/tables.py` around lines 149 - 152, The except Exception blocks at lines 149-152 and 248-251 in backend/app/routers/tables.py are returning HTTP 400 status codes for all exceptions, which incorrectly classifies upstream/network failures as client errors. Replace the 400 status code with an appropriate 5xx status code (such as 500 or 503) in both exception handlers to properly indicate server-side outages, ensuring clients receive the correct signal for retry behavior.
♻️ Duplicate comments (2)
backend/Dockerfile (1)
19-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin GitHub host keys instead of discovering them during secret-backed dependency sync.
Using
ssh-keyscanin the same step as SSH-key-authenticated fetch keeps a TOFU window open during build-time secret use. Use a pinnedknown_hostsentry (checked into repo or mounted as a separate secret) and fail closed.#!/bin/bash set -euo pipefail echo "Inspect current Dockerfile SSH trust flow:" cat -n backend/Dockerfile | sed -n '15,30p' echo echo "Find any pinned known_hosts artifacts:" fd -HI "known_hosts" . echo echo "Locate dynamic host-key discovery usage:" rg -n "ssh-keyscan github.com|known_hosts|--mount=type=secret,id=deploy_key" backend/Dockerfile🤖 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 `@backend/Dockerfile` around lines 19 - 22, The RUN command in backend/Dockerfile is using ssh-keyscan to dynamically discover GitHub's host keys during the build step when the SSH secret is mounted, creating a security vulnerability. Remove the ssh-keyscan github.com line and replace it with a pinned known_hosts file that either already exists in the repository or is mounted as a separate secret. Update the mkdir and subsequent lines to copy or use the pre-existing known_hosts file instead of generating it at build time, ensuring the SSH trust verification happens with a fixed, auditable set of host keys rather than dynamically discovered ones.docker-compose.yml (1)
437-447:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
${...}interpolation here unless these vars are guaranteed at Compose parse time.These explicit
environmententries can overrideenv_filewith empty values when root-level interpolation sources are missing, which can silently disable Langfuse/SSO config.#!/bin/bash set -euo pipefail vars=( LANGFUSE_PUBLIC_KEY LANGFUSE_SECRET_KEY LANGFUSE_HOST ENABLE_GOOGLE GOOGLE_CLIENT_ID GOOGLE_CLIENT_SECRET ENABLE_KEYCLOAK KEYCLOAK_SERVER_URL KEYCLOAK_CLIENT_ID KEYCLOAK_CLIENT_SECRET FRONTEND_DEFAULT_REDIRECT ) echo "Compose references:" rg -n 'LANGFUSE_PUBLIC_KEY|LANGFUSE_SECRET_KEY|LANGFUSE_HOST|ENABLE_GOOGLE|GOOGLE_CLIENT_ID|GOOGLE_CLIENT_SECRET|ENABLE_KEYCLOAK|KEYCLOAK_SERVER_URL|KEYCLOAK_CLIENT_ID|KEYCLOAK_CLIENT_SECRET|FRONTEND_DEFAULT_REDIRECT' docker-compose.yml echo echo "Root .env interpolation source check:" if [[ -f .env ]]; then for v in "${vars[@]}"; do grep -n "^${v}=" .env || echo "MISSING .env value for ${v}" done else echo "MISSING root .env file" fi echo echo "backend/.env presence (note: does not drive compose-file interpolation):" if [[ -f backend/.env ]]; then for v in "${vars[@]}"; do grep -n "^${v}=" backend/.env || true done fi🤖 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 `@docker-compose.yml` around lines 437 - 447, The docker-compose.yml file contains explicit environment variable entries using ${...} interpolation syntax for Langfuse and SSO configuration variables (LANGFUSE_PUBLIC_KEY, LANGFUSE_SECRET_KEY, LANGFUSE_HOST, ENABLE_GOOGLE, GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET, ENABLE_KEYCLOAK, KEYCLOAK_SERVER_URL, KEYCLOAK_CLIENT_ID, KEYCLOAK_CLIENT_SECRET, FRONTEND_DEFAULT_REDIRECT). When these variables are not defined at Compose parse time, they interpolate to empty strings and silently override any values set in env_file directives, disabling critical Langfuse and SSO functionality. Either ensure all these variables are guaranteed to exist in the root .env file before docker-compose execution, or remove the explicit environment entries and rely solely on env_file configuration to avoid silent configuration loss.
🧹 Nitpick comments (12)
backend/app/config.py (1)
55-57: 💤 Low valueDefault embedder URL uses unencrypted HTTP.
The
EMBEDDER_URLdefault points to an HTTP endpoint. While this is acceptable for local development with Ollama onhost.docker.internal, ensure production deployments override this with an HTTPS endpoint or use network-level encryption (e.g., service mesh).🤖 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 `@backend/app/config.py` around lines 55 - 57, The EMBEDDER_URL configuration variable defaults to an unencrypted HTTP endpoint, which is unsafe for production environments. Add clear documentation in the form of a comment above the EMBEDDER_URL variable definition that explicitly states this is for local development with Ollama only and that production deployments must override this value with an HTTPS endpoint or implement network-level encryption such as a service mesh. This ensures developers are aware of the security implications and take appropriate action for production deployments.Source: Linters/SAST tools
backend/app/main.py (1)
147-149: 💤 Low valueDuplicate health endpoint outside
/apiprefix.A root
/healthendpoint exists here whilehealth.routeris also included at/api/health(line 122). If intentional (e.g., for load balancers expecting/), consider adding a comment. Otherwise, remove one to avoid confusion.🤖 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 `@backend/app/main.py` around lines 147 - 149, The application has duplicate health check endpoints: one at the root path `/health` defined by the `health_check` function and another at `/api/health` provided by the included `health.router`. Remove the duplicate root `/health` endpoint (the `health_check` function with the `@app.get("/health")` decorator) unless there is a specific requirement for it to exist outside the `/api` prefix, in which case add a clarifying comment explaining why both endpoints are necessary.scripts/download_images.sh (1)
1-36: ⚡ Quick winDeduplicate this script with
build_openshift.shto prevent drift.This is functionally the same build/export flow; keep one canonical implementation and make the other a thin wrapper or remove it.
🤖 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 `@scripts/download_images.sh` around lines 1 - 36, The download_images.sh script contains the same build and export workflow as build_openshift.sh, creating unnecessary duplication that can lead to maintenance drift. Consolidate these scripts by keeping one as the canonical implementation and either remove the duplicate script entirely or refactor it as a thin wrapper that calls the canonical implementation. If keeping both files, ensure the wrapper script delegates to the main script rather than duplicating the build logic for the Frontend, Backend, and Agent images along with the tar file creation and output instructions.backend/Dockerfile (1)
5-5: ⚡ Quick winAdd
--no-install-recommendsto package install.This reduces unnecessary packages and runtime attack surface in the final image.
Suggested patch
-RUN apt-get update && apt-get install -y git openssh-client && rm -rf /var/lib/apt/lists/* +RUN apt-get update && apt-get install -y --no-install-recommends git openssh-client && rm -rf /var/lib/apt/lists/*🤖 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 `@backend/Dockerfile` at line 5, The RUN command in the Dockerfile is missing the --no-install-recommends flag when installing packages with apt-get. Add the --no-install-recommends flag to the apt-get install command that installs git and openssh-client to reduce unnecessary dependencies and the runtime attack surface of the final image. The flag should be placed between apt-get install and the -y flag.Source: Linters/SAST tools
backend/app/infra_init.py (1)
986-988: 💤 Low valueConsider validating catalog names before SQL interpolation.
While
catalogcomes from Trino'sSHOW CATALOGSresult (trusted internal data), directly interpolating it into SQL at line 988 bypasses parameterization. For defense-in-depth, consider validating that catalog names match an expected pattern (e.g., alphanumeric with underscores).🤖 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 `@backend/app/infra_init.py` around lines 986 - 988, The catalog variable at line 988 is being directly interpolated into the SQL query without validation. Add a validation check before the _trino_exec call to ensure the catalog name matches an expected pattern (such as alphanumeric characters and underscores only). This validation should occur within the for loop that iterates over custom_catalogs, and should raise an appropriate exception or log an error if a catalog name fails validation, preventing potentially unsafe SQL interpolation.core/src/core/config.py (2)
21-21: 💤 Low valueModule-level settings instantiation may load before environment is configured.
settings = CoreSettings()at module level means configuration is loaded at import time. If the module is imported before environment variables are set (e.g., in certain test fixtures or app bootstrap sequences), settings will use defaults. Consider using a lazy singleton pattern or factory function if this causes issues.🤖 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 `@core/src/core/config.py` at line 21, The `settings` variable instantiates `CoreSettings()` at module import time, which means configuration is loaded before environment variables may be set, causing the system to use default values instead of environment-specific configuration. Replace the direct module-level instantiation of `settings = CoreSettings()` with a lazy singleton pattern or factory function that defers the creation of the CoreSettings instance until it is first accessed, ensuring environment variables are properly configured at that time.
17-17: 💤 Low valueConsider safer default for
TRINO_VERIFYin production contexts.
TRINO_VERIFYdefaults toFalse, which disables SSL certificate verification. While acceptable for local development, this could mask misconfiguration in production deployments where TLS should be enforced. Consider documenting that this must be explicitly set toTrue(or a CA bundle path) in production, or defaulting toTrue.🤖 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 `@core/src/core/config.py` at line 17, The TRINO_VERIFY configuration attribute currently defaults to False, which disables SSL certificate verification and could mask security misconfigurations in production environments. Change the default value of TRINO_VERIFY from False to True to enforce certificate verification by default, making production deployments more secure. Additionally, add documentation or comments explaining that this setting controls SSL certificate verification and should be carefully configured based on the deployment environment, with explicit guidance that production environments should use True or provide a path to a valid CA bundle.backend/app/seed.py (1)
28-30: ⚡ Quick winLog failed embedding requests at warning level instead of silent fallback.
Catching all exceptions and silently returning a zero vector could mask configuration errors (wrong URL, missing model) or service unavailability. Consider logging at warning level to aid debugging in non-development environments.
♻️ Proposed improvement
+import logging +logger = logging.getLogger(__name__) + def get_embedding(text: str) -> list[float]: url = settings.EMBEDDER_URL data = json.dumps({"model": settings.EMBEDDER_MODEL, "prompt": text}).encode() req = urllib.request.Request(url, data=data, headers={"Content-Type": "application/json"}) try: - with urllib.request.urlopen(req) as res: + with urllib.request.urlopen(req, timeout=30) as res: return json.loads(res.read().decode())["embedding"] except Exception as e: - print(f"Error getting embedding: {e}") + logger.warning("Failed to get embedding from %s: %s. Using zero vector.", url, e) return [0.0] * 768🤖 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 `@backend/app/seed.py` around lines 28 - 30, The exception handler that catches embedding errors is using print() which provides insufficient logging for production environments and masks configuration or service issues. Replace the print statement with a proper logger.warning() call to ensure embedding failures are logged at the appropriate warning level, making it easier to debug configuration errors, wrong URLs, missing models, or service unavailability in non-development environments while keeping the fallback to the zero vector as a safe default.agent/src/agent/nodes/schema_explorer.py (2)
276-283: ⚡ Quick winReplace print with logger in LLM error handling paths.
Multiple exception handlers use
print()instead of the logging module, making production debugging difficult.Also applies to: 299-306
🤖 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 `@agent/src/agent/nodes/schema_explorer.py` around lines 276 - 283, Replace the print() statement in the exception handler at lines 276-283 with a proper logger.error() call that includes the error details. Also apply the same fix at the consolidated site at lines 299-306, replacing any print() statement with logger.error() to ensure consistent logging across all exception handlers in the schema_explorer module for proper production debugging.
48-53: ⚡ Quick winUse logger instead of print for embedding failures.
The
print()statement won't integrate with structured logging. Also, returning a zero vector silently degrades search quality without visibility.♻️ Proposed fix
Add a logger at the top of the file and use it:
+import logging +logger = logging.getLogger(__name__) + def get_query_embedding(text: str) -> list[float]: """Generate 768-dimensional embedding from nomic-embed-text.""" # TODO: support secret url = f"{settings.EMBEDDER_URL}/api/embeddings" data = json.dumps({"model": settings.EMBEDDER_MODEL, "prompt": text}).encode() req = urllib.request.Request(url, data=data, headers={"Content-Type": "application/json"}) try: with urllib.request.urlopen(req) as res: return json.loads(res.read().decode())["embedding"] except Exception as e: - print(f"Error getting query embedding: {e}") + logger.warning(f"Embedding service unavailable, falling back to zero vector: {e}") return [0.0] * 768🤖 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 `@agent/src/agent/nodes/schema_explorer.py` around lines 48 - 53, In the exception handler for the query embedding function (where urllib.request.urlopen is called), replace the print statement with proper logger usage. First, add a logger import and initialization at the top of the schema_explorer.py file (typically using logging.getLogger). Then, in the exception handler that currently catches the exception and prints the error, use the logger instance with an appropriate log level (e.g., logger.error) to log the error message instead of using print. This will ensure the embedding failure is captured in the structured logging system rather than being printed to stdout, providing better visibility and integration with the application's logging infrastructure.agent/src/agent/nodes/refiner.py (1)
2-2: ⚡ Quick winRemove unused import.
uuidis imported but never used in this module.♻️ Proposed fix
import json -import uuid from agent.state import AgentState🤖 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 `@agent/src/agent/nodes/refiner.py` at line 2, Remove the unused uuid import statement from the refiner module. The uuid import is declared but never referenced or used anywhere in the module, so it should be deleted to keep the imports clean and reduce unnecessary dependencies.agent/tests/test_extractor.py (1)
31-46: ⚡ Quick winAssert
raise_for_status()is called in the success-path HTTP extractor test.This strengthens the test against regressions that accidentally remove status validation.
Test assertion addition
assert len(entries) == 1 assert entries[0].term == "test" assert entries[0].context == "test context" mock_post.assert_called_once_with("http://test-url", json={"query": "test query"}, timeout=50) + mock_response.raise_for_status.assert_called_once()🤖 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 `@agent/tests/test_extractor.py` around lines 31 - 46, The test for the HTTP extractor's success path is missing an assertion to verify that raise_for_status() is called on the response object. Add an assertion after the existing mock_post.assert_called_once_with() call to verify that mock_response.raise_for_status() was called exactly once. This ensures the extractor properly validates the HTTP response status and prevents regressions if that validation is accidentally removed.
🤖 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 `@agent/Dockerfile`:
- Around line 1-4: The Dockerfile currently runs as root, which is a security
risk, and the apt-get command is not optimized for image size. Modify the
apt-get install line to include the `--no-install-recommends` flag and add
cleanup of apt lists with `rm -rf /var/lib/apt/lists/*` to reduce the final
image size. Additionally, add a RUN instruction to create a non-root user with
appropriate directory permissions, and then add a USER instruction before any
CMD directive to switch to that non-root user, ensuring any SSH key mounts and
`.venv` directory ownership are properly configured for the non-root user.
In `@agent/scripts/upload_all_prompts.py`:
- Around line 175-185: The exception handler in the for loop iterating over
prompts_to_upload currently catches exceptions from
langfuse_client.create_prompt and only prints an error message before continuing
with the next prompt. This allows the script to exit successfully even when some
prompts fail to upload. Instead of silently continuing after catching the
exception, the script should fail immediately when any prompt upload fails.
After printing the error message for the failed upload, either re-raise the
exception or call sys.exit(1) to terminate the script and prevent partial
uploads from being treated as success.
- Around line 7-9: Remove the hardcoded credential assignments for
LANGFUSE_PUBLIC_KEY, LANGFUSE_SECRET_KEY, and LANGFUSE_BASE_URL from the source
file. Instead, load these three values from environment variables using the os
module (or similar). Set reasonable defaults if the environment variables are
not present, or raise an error if they are required. This eliminates the risk of
accidentally committing real secrets by following the hardcoded credentials
pattern.
In `@agent/src/agent/graph.py`:
- Around line 80-81: The MemorySaver instance on line 80 is a development-only
in-memory checkpointer that does not persist state across process restarts or
crashes. Replace the MemorySaver initialization with a production-grade
database-backed checkpointer such as PostgresSaver, which provides durable state
persistence required for reliable thread resumption in production environments.
Update both the instantiation of the checkpointer (currently MemorySaver()) and
ensure the workflow.compile() call uses the new database-backed checkpointer
instance.
- Around line 38-40: The exception handler in the structured output parsing
block (lines 38-40) silently catches all exceptions and defaults to the
"extractor" route without proper error handling or logging. Replace the print
statement with appropriate logging infrastructure that captures and surfaces the
actual error details, and either propagate critical errors rather than silently
falling back, or implement conditional fallback logic that only applies to
specific, expected error cases—ensuring that unexpected errors like API failures
or prompt issues are not masked by the unconditional route assignment.
In `@agent/src/agent/nodes/extractor.py`:
- Around line 81-83: The HTTP request in the extractor is using a timeout of 50
seconds, which is too long and can stall the entire node if a single endpoint is
degraded. Replace the single long timeout parameter with tighter, separate
connect and read timeouts (for example, using a tuple like
timeout=(connect_timeout, read_timeout)) to fail fast and prevent a single slow
extractor from blocking the whole pipeline. Apply this same timeout adjustment
to all locations where HTTP requests are made to extractors, including the
additional occurrence noted in the comment.
- Around line 56-58: The exception handler in the structured output parsing
block catches the overly broad `Exception` class, which masks real programming
and schema errors by silently returning empty results. Replace the broad
`Exception` catch in the except block with specific expected exception classes
(such as those raised by JSON parsing or validation failures), and either allow
unexpected exceptions to propagate naturally or log them with a full traceback
to surface actual defects. Apply this same fix to all similar exception handlers
in the file that currently catch broad Exception classes.
- Around line 75-82: The HTTPExtractor class accepts extractor URLs without
validation, creating an SSRF vulnerability where unvalidated URLs could target
internal hosts. Add strict URL validation in the __init__ method to verify the
scheme (allowlist http/https), validate the host against an allowlist, and block
private/loopback/link-local addresses. Additionally, add a runtime guard in the
extract method immediately before the requests.post call to re-verify the URL
safety before dispatching the request. This prevents malicious or compromised
extractors from reaching internal services.
In `@agent/src/agent/nodes/finalizer.py`:
- Around line 68-82: The asyncio.gather call in the finalizer function uses
summary_task and sql_task without failure isolation, causing the entire
finalization to fail if either task encounters a transient error. Modify the
asyncio.gather call to set return_exceptions=True so that exceptions are
returned as results instead of being raised, then check the results from both
summary_task and sql_task for exceptions. For each task, provide a sensible
fallback value (such as an empty or default response) when an exception is
returned, and return these values along with any successful responses in the
final return dictionary.
In `@agent/src/agent/nodes/refiner.py`:
- Around line 45-51: The try-finally block in the refiner.py file lacks an
except clause to handle exceptions from client.save_data(). If save_data()
raises an exception, raw_ref is never assigned, causing a NameError when the
return statement tries to access it on line 51. Add an except clause that
catches exceptions from the save_data call, initializes raw_ref to an
appropriate default value (such as None), and ensures the trino_error field in
the return dictionary is set to capture the exception details so the error is
properly communicated to the caller instead of crashing.
In `@agent/src/agent/nodes/schema_explorer.py`:
- Around line 42-50: The urllib.request.urlopen call in the get_query_embedding
function lacks a timeout parameter, which could cause the node to hang
indefinitely if the embedder service becomes unresponsive. Add a timeout
parameter to the urlopen call to specify a maximum duration the request should
wait before raising an exception. Consider using a reasonable timeout value such
as 30 seconds, potentially configurable through settings if available.
- Around line 203-210: The exception handler for client.save_data() in the
try-except block silently swallows the error without logging it, allowing the
LLM to proceed with a None esca_id unaware of the failure. Add a logging import
at the top of the file and log the actual exception details when
client.save_data() fails in the except Exception as e block. This ensures that
save failures are visible in logs rather than hidden, allowing for proper error
tracking and debugging.
In `@agent/src/agent/routers/chat.py`:
- Around line 84-117: The chat_endpoint function is defined as async but uses
synchronous Session(engine) calls that block the event loop. Replace both
instances of Session(engine) with AsyncSession from SQLModel, ensure the engine
is created using create_async_engine for async compatibility, and use await
syntax when opening the AsyncSession context manager. This will allow the async
endpoint to properly yield control to the event loop and scale correctly with
concurrent requests.
In `@backend/alembic/env.py`:
- Line 21: The wildcard import `from core.models.models import *` on line 21
triggers a Ruff F403 linting violation and obscures the intent of importing for
side effects. Replace this wildcard import with an explicit module-level import
(such as `import core.models.models`) that clearly indicates the module is being
imported solely to populate SQLModel.metadata as a side effect, making the
intent more explicit and resolving the linting violation.
In `@backend/alembic/versions/f4d69a17d156_add_organization_and_sso_fields.py`:
- Around line 3-4: The docstring at the top of the file in
backend/alembic/versions/f4d69a17d156_add_organization_and_sso_fields.py (lines
3-4) states "Revises: d35412e44b34" but the actual down_revision variable at
line 17 is set to 'aeb0e2695517'. Update the docstring's Revises line to match
the actual down_revision value of 'aeb0e2695517' to ensure consistency and
prevent confusion when tracing the migration chain.
In `@backend/app/seed.py`:
- Around line 24-27: The urllib.request.urlopen(req) call in the try block lacks
a timeout parameter, which can cause the seed process to hang indefinitely if
the embedder service becomes unresponsive. Add a timeout parameter to the
urlopen call with a reasonable value (such as 30 seconds) to ensure the request
does not block indefinitely.
In `@backend/tests/conftest.py`:
- Around line 95-99: The monkeypatch of core.db.engine.engine in the fixture
happens too late because app.main is imported on line 14 before the fixture
reassigns core.db.engine.engine on line 96. Any module that executed from
core.db.engine import engine during the earlier import will retain a direct
reference to the original engine, bypassing the test database. Move the
core.db.engine.engine monkeypatch to occur before the app.main import, so all
subsequent module imports receive the test_engine reference. This ensures all DB
access paths are properly isolated to the test database.
In `@core/pyproject.toml`:
- Line 15: The python-core-utils dependency is pinned to the feature branch
`@feat/google-sso`, which creates non-reproducible builds and causes issues in CI
environments without SSH access. Replace the feature branch reference in the
git+ssh URL with either a specific commit SHA (e.g., @<commit_hash>) for
immediate reproducibility, or wait for the feature to be merged and released,
then update to a release tag (e.g., `@v`<version>) instead. This ensures builds
are reproducible and CI can function in restricted environments.
In `@core/src/core/trino.py`:
- Around line 71-79: The `fetchall()` call on line 71 of the
TrinoExecutionResult method can load unlimited rows into memory when processing
agent-generated SQL queries that lack strict LIMIT clauses, causing memory
issues. Replace the unbounded `fetchall()` approach with a bounded result set
strategy by either enforcing a maximum limit on rows fetched using `fetchmany()`
with a configured max size parameter, or by adding automatic LIMIT clause
injection to agent-generated SQL queries to prevent loading excessive result
sets into memory.
- Around line 68-72: The database connection obtained from
get_trino_connection() and the cursor created via conn.cursor() are not being
closed, causing resource leaks on both success and failure paths. Wrap the
cursor and connection operations in a try-finally block (or use context managers
with the 'with' statement if supported) to ensure cur.close() is called followed
by conn.close() in the finally block, regardless of whether the SQL execution
succeeds or raises an exception. Apply this fix to both affected code blocks:
the main block at lines 68-72 and the similar code block at lines 82-92.
---
Outside diff comments:
In `@backend/app/main.py`:
- Around line 96-97: The audit middleware is using a hardcoded placeholder for
user_id instead of reading from the authenticated session. Replace the hardcoded
user_id assignment with a session lookup that reads the user_id from the request
session object, providing "anonymous" as a fallback value if the user_id is not
present in the session. This ensures audit logs capture the actual authenticated
user rather than the placeholder value.
In `@backend/app/routers/tables.py`:
- Around line 149-152: The except Exception blocks at lines 149-152 and 248-251
in backend/app/routers/tables.py are returning HTTP 400 status codes for all
exceptions, which incorrectly classifies upstream/network failures as client
errors. Replace the 400 status code with an appropriate 5xx status code (such as
500 or 503) in both exception handlers to properly indicate server-side outages,
ensuring clients receive the correct signal for retry behavior.
---
Duplicate comments:
In `@backend/Dockerfile`:
- Around line 19-22: The RUN command in backend/Dockerfile is using ssh-keyscan
to dynamically discover GitHub's host keys during the build step when the SSH
secret is mounted, creating a security vulnerability. Remove the ssh-keyscan
github.com line and replace it with a pinned known_hosts file that either
already exists in the repository or is mounted as a separate secret. Update the
mkdir and subsequent lines to copy or use the pre-existing known_hosts file
instead of generating it at build time, ensuring the SSH trust verification
happens with a fixed, auditable set of host keys rather than dynamically
discovered ones.
In `@docker-compose.yml`:
- Around line 437-447: The docker-compose.yml file contains explicit environment
variable entries using ${...} interpolation syntax for Langfuse and SSO
configuration variables (LANGFUSE_PUBLIC_KEY, LANGFUSE_SECRET_KEY,
LANGFUSE_HOST, ENABLE_GOOGLE, GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET,
ENABLE_KEYCLOAK, KEYCLOAK_SERVER_URL, KEYCLOAK_CLIENT_ID,
KEYCLOAK_CLIENT_SECRET, FRONTEND_DEFAULT_REDIRECT). When these variables are not
defined at Compose parse time, they interpolate to empty strings and silently
override any values set in env_file directives, disabling critical Langfuse and
SSO functionality. Either ensure all these variables are guaranteed to exist in
the root .env file before docker-compose execution, or remove the explicit
environment entries and rely solely on env_file configuration to avoid silent
configuration loss.
---
Nitpick comments:
In `@agent/src/agent/nodes/refiner.py`:
- Line 2: Remove the unused uuid import statement from the refiner module. The
uuid import is declared but never referenced or used anywhere in the module, so
it should be deleted to keep the imports clean and reduce unnecessary
dependencies.
In `@agent/src/agent/nodes/schema_explorer.py`:
- Around line 276-283: Replace the print() statement in the exception handler at
lines 276-283 with a proper logger.error() call that includes the error details.
Also apply the same fix at the consolidated site at lines 299-306, replacing any
print() statement with logger.error() to ensure consistent logging across all
exception handlers in the schema_explorer module for proper production
debugging.
- Around line 48-53: In the exception handler for the query embedding function
(where urllib.request.urlopen is called), replace the print statement with
proper logger usage. First, add a logger import and initialization at the top of
the schema_explorer.py file (typically using logging.getLogger). Then, in the
exception handler that currently catches the exception and prints the error, use
the logger instance with an appropriate log level (e.g., logger.error) to log
the error message instead of using print. This will ensure the embedding failure
is captured in the structured logging system rather than being printed to
stdout, providing better visibility and integration with the application's
logging infrastructure.
In `@agent/tests/test_extractor.py`:
- Around line 31-46: The test for the HTTP extractor's success path is missing
an assertion to verify that raise_for_status() is called on the response object.
Add an assertion after the existing mock_post.assert_called_once_with() call to
verify that mock_response.raise_for_status() was called exactly once. This
ensures the extractor properly validates the HTTP response status and prevents
regressions if that validation is accidentally removed.
In `@backend/app/config.py`:
- Around line 55-57: The EMBEDDER_URL configuration variable defaults to an
unencrypted HTTP endpoint, which is unsafe for production environments. Add
clear documentation in the form of a comment above the EMBEDDER_URL variable
definition that explicitly states this is for local development with Ollama only
and that production deployments must override this value with an HTTPS endpoint
or implement network-level encryption such as a service mesh. This ensures
developers are aware of the security implications and take appropriate action
for production deployments.
In `@backend/app/infra_init.py`:
- Around line 986-988: The catalog variable at line 988 is being directly
interpolated into the SQL query without validation. Add a validation check
before the _trino_exec call to ensure the catalog name matches an expected
pattern (such as alphanumeric characters and underscores only). This validation
should occur within the for loop that iterates over custom_catalogs, and should
raise an appropriate exception or log an error if a catalog name fails
validation, preventing potentially unsafe SQL interpolation.
In `@backend/app/main.py`:
- Around line 147-149: The application has duplicate health check endpoints: one
at the root path `/health` defined by the `health_check` function and another at
`/api/health` provided by the included `health.router`. Remove the duplicate
root `/health` endpoint (the `health_check` function with the
`@app.get("/health")` decorator) unless there is a specific requirement for it
to exist outside the `/api` prefix, in which case add a clarifying comment
explaining why both endpoints are necessary.
In `@backend/app/seed.py`:
- Around line 28-30: The exception handler that catches embedding errors is
using print() which provides insufficient logging for production environments
and masks configuration or service issues. Replace the print statement with a
proper logger.warning() call to ensure embedding failures are logged at the
appropriate warning level, making it easier to debug configuration errors, wrong
URLs, missing models, or service unavailability in non-development environments
while keeping the fallback to the zero vector as a safe default.
In `@backend/Dockerfile`:
- Line 5: The RUN command in the Dockerfile is missing the
--no-install-recommends flag when installing packages with apt-get. Add the
--no-install-recommends flag to the apt-get install command that installs git
and openssh-client to reduce unnecessary dependencies and the runtime attack
surface of the final image. The flag should be placed between apt-get install
and the -y flag.
In `@core/src/core/config.py`:
- Line 21: The `settings` variable instantiates `CoreSettings()` at module
import time, which means configuration is loaded before environment variables
may be set, causing the system to use default values instead of
environment-specific configuration. Replace the direct module-level
instantiation of `settings = CoreSettings()` with a lazy singleton pattern or
factory function that defers the creation of the CoreSettings instance until it
is first accessed, ensuring environment variables are properly configured at
that time.
- Line 17: The TRINO_VERIFY configuration attribute currently defaults to False,
which disables SSL certificate verification and could mask security
misconfigurations in production environments. Change the default value of
TRINO_VERIFY from False to True to enforce certificate verification by default,
making production deployments more secure. Additionally, add documentation or
comments explaining that this setting controls SSL certificate verification and
should be carefully configured based on the deployment environment, with
explicit guidance that production environments should use True or provide a path
to a valid CA bundle.
In `@scripts/download_images.sh`:
- Around line 1-36: The download_images.sh script contains the same build and
export workflow as build_openshift.sh, creating unnecessary duplication that can
lead to maintenance drift. Consolidate these scripts by keeping one as the
canonical implementation and either remove the duplicate script entirely or
refactor it as a thin wrapper that calls the canonical implementation. If
keeping both files, ensure the wrapper script delegates to the main script
rather than duplicating the build logic for the Frontend, Backend, and Agent
images along with the tar file creation and output instructions.
🪄 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 Plus
Run ID: 50f7cb49-0293-48ba-9c50-8d900f98bdd9
⛔ Files ignored due to path filters (4)
agent/uv.lockis excluded by!**/*.lockbackend/uv.lockis excluded by!**/*.lockexternal_extractor/uv.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (95)
.dockerignore.gitignoreagent/Dockerfileagent/README.mdagent/pyproject.tomlagent/scripts/upload_all_prompts.pyagent/src/agent/__init__.pyagent/src/agent/config.pyagent/src/agent/graph.pyagent/src/agent/langfuse_client.pyagent/src/agent/main.pyagent/src/agent/nodes/extractor.pyagent/src/agent/nodes/finalizer.pyagent/src/agent/nodes/query_builder.pyagent/src/agent/nodes/refiner.pyagent/src/agent/nodes/schema_explorer.pyagent/src/agent/routers/__init__.pyagent/src/agent/routers/chat.pyagent/src/agent/state.pyagent/tests/__init__.pyagent/tests/test_extractor.pybackend/.env.examplebackend/Dockerfilebackend/alembic/env.pybackend/alembic/versions/781457ead87d_add_httpextractor.pybackend/alembic/versions/aeb0e2695517_add_status_to_httpextractor.pybackend/alembic/versions/e738b4f2c9c2_add_table_embedding.pybackend/alembic/versions/f4d69a17d156_add_organization_and_sso_fields.pybackend/app/api/auth.pybackend/app/config.pybackend/app/core/auth.pybackend/app/db/engine.pybackend/app/infra_init.pybackend/app/main.pybackend/app/routers/admin_approval.pybackend/app/routers/admin_auth.pybackend/app/routers/audit.pybackend/app/routers/enrichment.pybackend/app/routers/evaluation.pybackend/app/routers/extractors.pybackend/app/routers/feedback.pybackend/app/routers/health.pybackend/app/routers/orchestration.pybackend/app/routers/profiling.pybackend/app/routers/publish.pybackend/app/routers/questions.pybackend/app/routers/scopes.pybackend/app/routers/tables.pybackend/app/seed.pybackend/app/services/auth.pybackend/app/services/evaluator.pybackend/app/services/join_detection.pybackend/app/services/scheduler.pybackend/app/services/trino_client.pybackend/e2e_test.pybackend/pyproject.tomlbackend/tests/conftest.pybackend/tests/test_api.pybuild_openshift.shcore/README.mdcore/pyproject.tomlcore/src/core/__init__.pycore/src/core/config.pycore/src/core/db/__init__.pycore/src/core/db/engine.pycore/src/core/models/__init__.pycore/src/core/models/models.pycore/src/core/py.typedcore/src/core/trino.pydocker-compose.ymlexternal_extractor/README.mdexternal_extractor/main.pyexternal_extractor/pyproject.tomlfrontend/Dockerfilefrontend/nginx.conffrontend/package.jsonfrontend/src/App.tsxfrontend/src/api/auth.tsfrontend/src/api/client.tsfrontend/src/api/orchestration.tsfrontend/src/api/schema.d.tsfrontend/src/components/layout/AuthProvider.tsxfrontend/src/components/layout/Sidebar.tsxfrontend/src/components/tables/TableList.tsxfrontend/src/components/wizard/OnboardingWizard.tsxfrontend/src/config/constants.tsfrontend/src/main.tsxfrontend/src/pages/LandingPage.tsxfrontend/src/pages/LoginPage.tsxfrontend/src/store/authStore.tsfrontend/src/styles/globals.cssfrontend/vite.config.tsinfra/trino/etc/catalog/airlines.propertiesinfra/trino/etc/jvm.configscripts/download_images.sh
💤 Files with no reviewable changes (1)
- backend/app/db/engine.py
✅ Files skipped from review due to trivial changes (13)
- .dockerignore
- frontend/src/config/constants.ts
- backend/app/routers/admin_auth.py
- .gitignore
- agent/src/agent/main.py
- external_extractor/pyproject.toml
- agent/pyproject.toml
- backend/alembic/versions/e738b4f2c9c2_add_table_embedding.py
- frontend/src/main.tsx
- backend/app/services/auth.py
- agent/src/agent/init.py
- agent/src/agent/routers/init.py
- backend/app/routers/feedback.py
🚧 Files skipped from review as they are similar to previous changes (17)
- infra/trino/etc/catalog/airlines.properties
- frontend/src/api/client.ts
- frontend/src/pages/LandingPage.tsx
- frontend/src/store/authStore.ts
- frontend/src/components/layout/AuthProvider.tsx
- infra/trino/etc/jvm.config
- frontend/src/api/auth.ts
- frontend/src/styles/globals.css
- frontend/src/App.tsx
- frontend/package.json
- backend/app/api/auth.py
- frontend/src/components/tables/TableList.tsx
- frontend/src/components/layout/Sidebar.tsx
- frontend/src/pages/LoginPage.tsx
- frontend/src/components/wizard/OnboardingWizard.tsx
- backend/app/routers/admin_approval.py
- backend/app/core/auth.py
Comment on lines
+1
to
+4
| FROM python:3.12-slim | ||
|
|
||
| RUN pip install uv | ||
| RUN apt-get update && apt-get install -y git openssh-client |
There was a problem hiding this comment.
Add a non-root user and optimize apt-get.
Running as root in the container is a security risk. Also, add --no-install-recommends and clean up apt lists to reduce image size.
🔧 Proposed fix
FROM python:3.12-slim
+RUN useradd -m -s /bin/bash agent
+
RUN pip install uv
-RUN apt-get update && apt-get install -y git openssh-client
+RUN apt-get update && apt-get install -y --no-install-recommends git openssh-client \
+ && rm -rf /var/lib/apt/lists/*Then at the end of the Dockerfile, before CMD:
+USER agent
CMD ["uvicorn", "agent.main:app", "--host", "0.0.0.0", "--port", "8001"]Note: You may need to adjust the SSH key mount path and .venv ownership if switching to a non-root user during build.
🧰 Tools
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[error] 4-4: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'apt-get update && apt-get install -y git openssh-client'
Rule: DS-0029
(IaC/Dockerfile)
🤖 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 `@agent/Dockerfile` around lines 1 - 4, The Dockerfile currently runs as root,
which is a security risk, and the apt-get command is not optimized for image
size. Modify the apt-get install line to include the `--no-install-recommends`
flag and add cleanup of apt lists with `rm -rf /var/lib/apt/lists/*` to reduce
the final image size. Additionally, add a RUN instruction to create a non-root
user with appropriate directory permissions, and then add a USER instruction
before any CMD directive to switch to that non-root user, ensuring any SSH key
mounts and `.venv` directory ownership are properly configured for the non-root
user.
Source: Linters/SAST tools
Comment on lines
+175
to
+185
| for p in prompts_to_upload: | ||
| try: | ||
| print(f"Uploading prompt: {p['name']}...") | ||
| langfuse_client.create_prompt( | ||
| name=p["name"], | ||
| prompt=p["prompt"], | ||
| type=p["type"] | ||
| ) | ||
| print(f" ✓ Successfully uploaded {p['name']}") | ||
| except Exception as e: | ||
| print(f" ✗ Failed to upload {p['name']}: {e}") |
There was a problem hiding this comment.
Fail the script when any prompt upload fails.
Lines 184-185 swallow errors and continue, so the script can exit successfully with a partially uploaded prompt set.
Suggested fix
- for p in prompts_to_upload:
+ failed = []
+ for p in prompts_to_upload:
try:
print(f"Uploading prompt: {p['name']}...")
langfuse_client.create_prompt(
name=p["name"],
prompt=p["prompt"],
type=p["type"]
)
print(f" ✓ Successfully uploaded {p['name']}")
except Exception as e:
print(f" ✗ Failed to upload {p['name']}: {e}")
+ failed.append(p["name"])
+
+ if failed:
+ raise SystemExit(f"Upload failed for prompts: {', '.join(failed)}")📝 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
| for p in prompts_to_upload: | |
| try: | |
| print(f"Uploading prompt: {p['name']}...") | |
| langfuse_client.create_prompt( | |
| name=p["name"], | |
| prompt=p["prompt"], | |
| type=p["type"] | |
| ) | |
| print(f" ✓ Successfully uploaded {p['name']}") | |
| except Exception as e: | |
| print(f" ✗ Failed to upload {p['name']}: {e}") | |
| failed = [] | |
| for p in prompts_to_upload: | |
| try: | |
| print(f"Uploading prompt: {p['name']}...") | |
| langfuse_client.create_prompt( | |
| name=p["name"], | |
| prompt=p["prompt"], | |
| type=p["type"] | |
| ) | |
| print(f" ✓ Successfully uploaded {p['name']}") | |
| except Exception as e: | |
| print(f" ✗ Failed to upload {p['name']}: {e}") | |
| failed.append(p["name"]) | |
| if failed: | |
| raise SystemExit(f"Upload failed for prompts: {', '.join(failed)}") |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 184-184: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@agent/scripts/upload_all_prompts.py` around lines 175 - 185, The exception
handler in the for loop iterating over prompts_to_upload currently catches
exceptions from langfuse_client.create_prompt and only prints an error message
before continuing with the next prompt. This allows the script to exit
successfully even when some prompts fail to upload. Instead of silently
continuing after catching the exception, the script should fail immediately when
any prompt upload fails. After printing the error message for the failed upload,
either re-raise the exception or call sys.exit(1) to terminate the script and
prevent partial uploads from being treated as success.
Comment on lines
+38
to
+40
| except Exception as e: | ||
| print(f"Structured output parsing failed: {e}") | ||
| route = "extractor" # Fallback |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -50 agent/src/agent/graph.py | cat -nRepository: StavPonte11/text2sql-onboarding
Length of output: 2478
🏁 Script executed:
rg -n "rejection_router_node|rejection_router|except Exception" agent/src/agent/graph.py -A 3 -B 3Repository: StavPonte11/text2sql-onboarding
Length of output: 1749
🏁 Script executed:
wc -l agent/src/agent/graph.pyRepository: StavPonte11/text2sql-onboarding
Length of output: 103
Do not silently swallow all rejection-routing failures.
Lines 38–40 catch all exceptions from the LLM chain invocation and unconditionally fall back to the "extractor" route. This masks real errors (LLM failures, prompt issues, parsing errors, API faults) and silently causes incorrect control flow—user feedback is ignored without notification. The print() statement provides no actionable logging. When structured output parsing fails, the route should either surface the error explicitly or log it properly, not default to a fixed fallback.
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 38-38: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@agent/src/agent/graph.py` around lines 38 - 40, The exception handler in the
structured output parsing block (lines 38-40) silently catches all exceptions
and defaults to the "extractor" route without proper error handling or logging.
Replace the print statement with appropriate logging infrastructure that
captures and surfaces the actual error details, and either propagate critical
errors rather than silently falling back, or implement conditional fallback
logic that only applies to specific, expected error cases—ensuring that
unexpected errors like API failures or prompt issues are not masked by the
unconditional route assignment.
Source: Linters/SAST tools
Comment on lines
+80
to
+81
| memory = MemorySaver() | ||
| agent_graph = workflow.compile(checkpointer=memory) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and surrounding context
cat -n agent/src/agent/graph.py | head -100 | tail -30Repository: StavPonte11/text2sql-onboarding
Length of output: 1512
🏁 Script executed:
# Check the imports to understand which MemorySaver is being used
head -30 agent/src/agent/graph.pyRepository: StavPonte11/text2sql-onboarding
Length of output: 1447
🌐 Web query:
LangChain MemorySaver checkpointer storage durability
💡 Result:
LangGraph's checkpointer durability determines how the system persists graph state snapshots and handles execution flow [1]. Key durability and storage considerations include: 1. Storage Persistence: - InMemorySaver: Designed exclusively for development, testing, and debugging [2][1]. It is ephemeral and will not survive process restarts, crashes, or multi-replica deployments [2][3]. - Production Backends: For production, you must use a database-backed checkpointer such as PostgresSaver (using PostgreSQL) [4][5][1]. These implementations support persistent storage, allowing threads to resume after interruptions or failures [6][7][1]. 2. Durability Modes: - sync (Default): Checkpoints the state before each superstep [1]. This mode provides inherent backpressure because the graph loop awaits the completion of each checkpoint write before proceeding to the next step, ensuring higher consistency and reliability at the cost of slight latency [8]. - async: Attempts to checkpoint asynchronously [1]. While this can improve performance by not blocking the main execution loop, it lacks backpressure [8]. Under high load or with slow write latency, this may lead to memory accumulation (e.g., coroutine chains) if the system generates state faster than it can persist it [8]. 3. Production Best Practices: - Always use connection pooling (e.g., via psycopg_pool) when using PostgreSQL to handle concurrent database operations efficiently [5][1]. - Perform database migrations (calling checkpointer.setup()) to initialize the necessary schema, typically on application startup [2][5][1]. - Configure checkpointers within the application lifespan (e.g., inside a FastAPI lifespan block) to ensure proper lifecycle management of connection pools [2]. - Distinguish between checkpointers (short-term, thread-scoped state) and stores (long-term, cross-thread data); most production applications require both for full functionality [6][7][2].
Citations:
- 1: https://langchain-ai-langgraph-40.mintlify.app/guides/persistence
- 2: https://www.learnwithparam.com/blog/langgraph-persistence-thread-models-production
- 3: https://langchain-ai.github.io/langmem/hot_path_quickstart/
- 4: https://langchain-ai.github.io/langgraph/how-tos/persistence/#use-in-production
- 5: https://langchain-ai-langgraph-40.mintlify.app/guides/deployment
- 6: https://docs.langchain.com/oss/javascript/langgraph/persistence
- 7: https://docs.langchain.com/oss/python/langgraph/persistence
- 8: Memory leak: _checkpointer_put_after_previous coroutine chains accumulate with default durability="async" langchain-ai/langgraph#7094
MemorySaver is not suitable for production deployments.
Line 80 uses an in-memory checkpointer designed exclusively for development and testing. It does not persist state across process restarts, crashes, or multiple service instances, causing thread resumption to fail unpredictably in production. Use a database-backed checkpointer like PostgresSaver for durable state persistence.
🤖 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 `@agent/src/agent/graph.py` around lines 80 - 81, The MemorySaver instance on
line 80 is a development-only in-memory checkpointer that does not persist state
across process restarts or crashes. Replace the MemorySaver initialization with
a production-grade database-backed checkpointer such as PostgresSaver, which
provides durable state persistence required for reliable thread resumption in
production environments. Update both the instantiation of the checkpointer
(currently MemorySaver()) and ensure the workflow.compile() call uses the new
database-backed checkpointer instance.
Comment on lines
+24
to
+27
| req = urllib.request.Request(url, data=data, headers={"Content-Type": "application/json"}) | ||
| try: | ||
| with urllib.request.urlopen(req) as res: | ||
| return json.loads(res.read().decode())["embedding"] |
There was a problem hiding this comment.
Add timeout to urlopen to prevent indefinite blocking.
urllib.request.urlopen(req) has no timeout, which can cause the seed process to hang indefinitely if the embedder service is unresponsive. Add a reasonable timeout.
🛡️ Proposed fix
- with urllib.request.urlopen(req) as res:
+ with urllib.request.urlopen(req, timeout=30) as res:📝 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
| req = urllib.request.Request(url, data=data, headers={"Content-Type": "application/json"}) | |
| try: | |
| with urllib.request.urlopen(req) as res: | |
| return json.loads(res.read().decode())["embedding"] | |
| req = urllib.request.Request(url, data=data, headers={"Content-Type": "application/json"}) | |
| try: | |
| with urllib.request.urlopen(req, timeout=30) as res: | |
| return json.loads(res.read().decode())["embedding"] |
🤖 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 `@backend/app/seed.py` around lines 24 - 27, The urllib.request.urlopen(req)
call in the try block lacks a timeout parameter, which can cause the seed
process to hang indefinitely if the embedder service becomes unresponsive. Add a
timeout parameter to the urlopen call with a reasonable value (such as 30
seconds) to ensure the request does not block indefinitely.
Comment on lines
+95
to
+99
| """Globally monkeypatches the core.db.engine.engine to isolate all routers to the test database.""" | ||
| original_engine = core.db.engine.engine | ||
| core.db.engine.engine = test_engine | ||
| yield | ||
| app.db.engine.engine = original_engine | ||
| core.db.engine.engine = original_engine |
There was a problem hiding this comment.
Global engine monkeypatch is too late to isolate all DB access paths.
Line 14 imports app.main before Line 96 reassigns core.db.engine.engine. Modules that already did from core.db.engine import engine keep the old reference, so some code paths can bypass the test DB.
Suggested fix
-from app.main import app as fastapi_app
+# Import lazily inside the client fixture, after engine patching is active.
@@
-@pytest.fixture
-def client(test_engine):
+@pytest.fixture
+def client(test_engine, patch_global_engine):
@@
- fastapi_app.dependency_overrides[get_session] = override_get_session
+ from app.main import app as fastapi_app
+ fastapi_app.dependency_overrides[get_session] = override_get_session🤖 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 `@backend/tests/conftest.py` around lines 95 - 99, The monkeypatch of
core.db.engine.engine in the fixture happens too late because app.main is
imported on line 14 before the fixture reassigns core.db.engine.engine on line
96. Any module that executed from core.db.engine import engine during the
earlier import will retain a direct reference to the original engine, bypassing
the test database. Move the core.db.engine.engine monkeypatch to occur before
the app.main import, so all subsequent module imports receive the test_engine
reference. This ensures all DB access paths are properly isolated to the test
database.
Comment on lines
+71
to
+79
| rows = cur.fetchall() | ||
| columns = [desc[0] for desc in cur.description] if cur.description else [] | ||
| execution_time_ms = int((time.time() - start_time) * 1000) | ||
| logger.info(f"[CoreTrinoClient] OK — {len(rows)} rows in {execution_time_ms}ms") | ||
| return TrinoExecutionResult( | ||
| success=True, | ||
| rows=[list(r) for r in rows], | ||
| columns=columns, | ||
| row_count=len(rows), |
There was a problem hiding this comment.
Avoid unbounded result materialization.
Line 71 uses fetchall() for agent-generated SQL. A query without a strict LIMIT can load huge result sets into memory and degrade API availability.
🤖 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 `@core/src/core/trino.py` around lines 71 - 79, The `fetchall()` call on line
71 of the TrinoExecutionResult method can load unlimited rows into memory when
processing agent-generated SQL queries that lack strict LIMIT clauses, causing
memory issues. Replace the unbounded `fetchall()` approach with a bounded result
set strategy by either enforcing a maximum limit on rows fetched using
`fetchmany()` with a configured max size parameter, or by adding automatic LIMIT
clause injection to agent-generated SQL queries to prevent loading excessive
result sets into memory.
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
3 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.
Summary by CodeRabbit
Release Notes
New Features
/meand auth config endpoints, and session-based access control.Improvements
/apiand updated frontend/backend request handling.Bug Fixes
Tests