Yodan/connect eval service - #29
Open
yodanBargida wants to merge 15 commits into
Open
Conversation
…o yodan/connect-eval-service
📝 WalkthroughWalkthroughThis pull request updates Text-to-SQL prompts, agent schema handling, evaluation APIs and orchestration, OpenMetadata and Spider2 synchronization, Trino query execution, frontend evaluation controls, running-state displays, and related infrastructure configuration. ChangesAgent workflow
Evaluation backend and query services
Metadata and operations
Evaluation frontend
Supporting application edits
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
Collaborator
|
coderabbit review --dir agent |
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
frontend/src/components/tables/TableList.tsx (1)
189-196: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty state is misleading when all rows were filtered out by the Spider2 toggle.
datais post-filtered, so hiding Spider2 tables on a Spider2-only result set renders "No tables found — Click 'Add Table' to get started". Branch onrawData?.lengthto show a "no tables match the current filters" message instead.🤖 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/tables/TableList.tsx` around lines 189 - 196, Update the empty-state branch in the TableList rendering to distinguish an empty raw dataset from a non-empty rawData set whose post-filtered data is empty. Use rawData?.length to show a “no tables match the current filters” message for filtered-out results, while preserving the existing noData/Add Table message when no tables exist at all.frontend/src/components/monitoring/RunHistoryTable.tsx (1)
403-424: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFiltering is applied after paging state and after the empty check.
Two consequences: (1) if every fetched run is excluded,
runs.lengthis still non-zero so theEmptySlatebranch is skipped and an empty table body renders; (2)pageis never clamped whenexcludeTableIdschanges, so a user on page 3 can land pasttotalPagesand see no rows.🐛 Proposed fix
const visibleRuns = excludeTableIds?.size ? runs.filter((r) => !r.table_id || !excludeTableIds.has(r.table_id)) : runs; - const paged = visibleRuns.slice(page * pageSize, (page + 1) * pageSize); - const totalPages = Math.ceil(visibleRuns.length / pageSize); + const totalPages = Math.ceil(visibleRuns.length / pageSize); + const safePage = Math.min(page, Math.max(totalPages - 1, 0)); + const paged = visibleRuns.slice(safePage * pageSize, (safePage + 1) * pageSize); + + useEffect(() => { + setPage(0); + }, [excludeTableIds]); if (isLoading) return ( <div className="run-history-loading"> <Spinner size={24} /> </div> ); - if (!runs.length) + if (!visibleRuns.length)Note:
excludeTableIdsis a memoizedSetinfrontend/src/pages/EvaluationsPage.tsx, so the effect dependency is stable there; usesafePagein the pagination controls/label too.🤖 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/monitoring/RunHistoryTable.tsx` around lines 403 - 424, Base the empty-state check, pagination, and page count on filtered runs by moving the visibleRuns computation before those branches. Clamp the current page when visibleRuns or totalPages changes, derive a safePage value, and use it for slicing plus all pagination controls and page labels; ensure an entirely excluded result renders EmptySlate rather than an empty table.frontend/src/pages/SandboxPage.tsx (1)
11-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLoad
.score-ring--runningin SandboxPage.
EvaluationTab.cssis only imported fromEvaluationTab.tsx, whileSandboxPage.tsxmounts the same.score-ring score-ring--runningmarkup without a matching stylesheet. Add this modifier toglobals.cssor importEvaluationTab.cssfromSandboxPage.tsx; otherwise the running score ring renders unstyled.🤖 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/SandboxPage.tsx` around lines 11 - 22, Make the running ScoreRing markup in SandboxPage render with its required styles by either adding the .score-ring--running rules to globals.css or importing EvaluationTab.css from SandboxPage.tsx. Preserve the existing ScoreRing status handling and avoid duplicating styles across both locations.backend/app/routers/evaluation.py (1)
493-516: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead
table_namesaccumulation.
table_namesis built inside the loop at Lines 494-496 and then unconditionally reassigned at Line 514. Drop the loop-side accumulation.♻️ Proposed cleanup
all_production_questions: list[GoldenQuestion] = [] - table_names = [] for table in prod_tables: - table_names.append(table.name) qs = session.exec(🤖 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/evaluation.py` around lines 493 - 516, Remove the loop-side table_names initialization and append operation in the production-table processing block. Keep the later schema-qualified table_names comprehension unchanged, as it is the value used after the loop.agent/src/agent/nodes/query_builder.py (1)
13-36: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUser feedback and skill prompts are computed but never sent to the LLM.
feedback_str(including any appended skill-prompt text) is built but the corresponding chain input field is commented out, and the new query_builder prompt template has no{{feedback_str}}placeholder either. This silently breaks the "user rejects → regenerate with feedback" flow and the loaded-skills injection.Suggested fix
response = await chain.ainvoke( { "schema_plan": state.get("schema_plan"), "user_query": state.get("user_query"), - # "feedback_str": feedback_str, + "feedback_str": feedback_str, } )Also add a
{{feedback_str}}placeholder to thetext2sql/query_builderprompt inagent/scripts/upload_all_prompts.pyso the fed value is actually used.🤖 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/query_builder.py` around lines 13 - 36, Pass the computed feedback_str into the chain.ainvoke input in the query-builder node, and update the text2sql/query_builder prompt definition in upload_all_prompts.py to include a {{feedback_str}} placeholder so rejection feedback and loaded-skill prompts reach the LLM.
🤖 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/scripts/upload_all_prompts.py`:
- Around line 49-53: Update the constraints in the prompt-building logic near
the flat JSON schema to remove the stale references to an "english" key and
"both keys." Replace them with wording that consistently requires a valid flat
object mapping each extracted Hebrew location to its standard English
translation, and an empty object when no locations are found.
- Line 161: Update the geographic-distance instruction in the prompt content
handled by upload_all_prompts.py to name Trino’s function as
to_spherical_geography(), replacing toSphericalGeography(). Preserve the
surrounding WGS84 and spherical geography guidance.
In `@agent/src/agent/nodes/refiner.py`:
- Around line 126-134: Extract the duplicated datetime/date-to-ISO conversion
logic into a shared json_serial helper in
agent/src/agent/utils/serialization.py, preserving the TypeError behavior for
unsupported values. In agent/src/agent/nodes/refiner.py lines 126-134 and
agent/src/agent/nodes/finalizer.py lines 33-44, import and reuse the shared
helper, removing each local json_serial definition.
- Around line 159-173: The build_refiner_schema_context function must restore a
bounded schema-context representation for table_profiles instead of serializing
the entire profile blob. Apply the existing or appropriate profile-size/token
limit before json.dumps, while preserving the schema_plan fallback and “No
schema context available.” behavior.
In `@agent/src/agent/nodes/schema_explorer.py`:
- Around line 326-351: Update the Trino fallback in async function
get_table_profile to run execute_query_sync via await asyncio.to_thread, passing
the DESCRIBE SQL as its argument. Preserve the existing success handling, JSON
response, and exception logging while ensuring the event loop is not blocked.
In `@backend/app/config.py`:
- Around line 41-42: Verify the startup paths controlled by RUN_SEED and
RUN_INFRA_INIT are idempotent and safe to execute on every backend restart,
including production deployments with default configuration. Confirm repeated
seeding, OpenMetadata pipeline creation, and catalog verification do not
duplicate, corrupt, or otherwise disrupt existing resources; update those paths
if necessary to ensure safe re-runs.
In `@backend/app/main.py`:
- Line 136: Protect query execution by moving query.router from api_router to
private_router in backend/app/main.py so get_current_user applies; additionally,
in backend/app/routers/query.py around the query route, add an explicit
authentication dependency and reject non-read-only SQL statements, allowing only
read-only queries.
In `@backend/app/routers/evaluation.py`:
- Around line 382-390: The additional_tables contract uses inconsistent table
identifier formats across evaluation and orchestration callers. Introduce or
reuse one shared formatter that produces catalog.schema.table identifiers, then
apply it to the baseline table_names construction in evaluation.py, both
production and spider2 branches in orchestration.py, the regression path in
evaluation.py, and the single-table and candidate paths in evaluation.py; ensure
every caller sends the same fully qualified format.
- Around line 691-791: The evaluation post-processing logic is duplicated across
routers and should have one shared implementation. In
backend/app/routers/evaluation.py lines 691-791, extract the body of
_run_evaluation_pipeline into a shared service function, including regression
handling, alerts, and metric persistence, then call it from
_run_evaluation_pipeline. In backend/app/routers/orchestration.py lines 75-169,
replace the duplicated per-table loop body with that shared function. Move
REGRESSION_BLOCK_DELTA, REGRESSION_WARNING_DELTA, LOW_SCORE_THRESHOLD, and
_create_alert from backend/app/routers/evaluation.py lines 665-689 into the
shared module, and remove the duplicate definitions from
backend/app/routers/orchestration.py lines 187-205.
- Line 41: The router modules have a circular module-level import. In
backend/app/routers/evaluation.py:41-41, remove the top-level
get_orchestration_report import and import it inside the get_run_report handler
at its use site. In backend/app/routers/orchestration.py:35-40, retain the
execute_single_table_eval import only with the evaluation-side deferred;
otherwise move it into _run_full_pipeline, matching _run_dataset_pipeline’s
deferred-import pattern.
- Around line 193-206: Update the failure_breakdown construction in the
evaluation response handling so fixed counter keys cannot overwrite category
entries from eval_resp.failure_analysis.categories. Preserve both values by
merging without clobbering existing category keys or by placing fixed counters
under a distinct namespace, while keeping run.failure_breakdown populated with
all breakdown details.
- Around line 754-763: Update the low-score alert condition in
execute_single_table_eval to require a successful run status in addition to
score < LOW_SCORE_THRESHOLD. Preserve the existing alert creation for genuinely
low-scoring successful runs, while skipping it when the run is failed.
In `@backend/app/routers/orchestration.py`:
- Around line 322-326: Move the long-running request in _run_dataset_pipeline
out of FastAPI’s shared BackgroundTasks/AnyIO threadpool by dispatching dataset
evaluations through a dedicated worker or queue. Preserve the existing
evaluation payload, endpoint, and 600-second timeout while ensuring concurrent
runs cannot consume the shared sync-route worker slots.
- Around line 469-477: Extract the run-label fallback logic from the
orchestration endpoint into a shared helper, then update both this endpoint and
evaluation.py’s get_run to use it. Ensure identical records produce the same
labels, including table names, triggered-by dataset labels, and the Production
Baseline/Production Regression/Unknown cases.
- Around line 347-353: Validate dataset_name in trigger_dataset_run against an
explicit allowlist of supported datasets before creating the run or scheduling
background work. Reject unsupported values with the established client-error
response, and keep _run_dataset_pipeline reachable only for allowed dataset
names.
In `@backend/app/routers/query.py`:
- Around line 47-48: Replace the unbounded cur.fetchall() call in the query
handler with a server-side capped fetch using a defined MAX_ROWS limit, and
determine whether additional rows remain to set a truncation flag. Include that
flag in the API response while preserving the existing column extraction and
returned rows.
- Around line 33-41: Update the TRINO_ENABLED guard in execute_query to return
QueryResponse with success=False when Trino execution is disabled, while
preserving the existing error message and empty-result fields. Update
test_execute_query_disabled to assert the disabled execution is unsuccessful.
- Around line 43-71: Update the query execution handler in
backend/app/routers/query.py to delegate SQL execution to
core.trino.execute_query_sync instead of manually managing the Trino connection
and cursor. Preserve the existing QueryResponse success/error fields and
execution-time calculation while removing the local get_trino_connection,
cursor, fetch, and close logic.
In `@backend/app/spider2_questions.json`:
- Around line 1-1702: Remove the stale spider2_questions.json golden-question
file from the repository. Do not modify sync_om_metadata.py or add replacement
data, since no remaining references depend on this file.
In `@backend/app/sync_om_metadata.py`:
- Around line 90-92: Update the module docstring’s “Run with” command to
reference the file’s current backend/app/sync_om_metadata.py location while
preserving the existing optional flags.
- Around line 617-647: The table-selection loop in the question processing flow
should track when no extracted ref_tables match and the lowest-id catalog table
is used as a fallback. Add a separate fallback counter or equivalent summary
metric, increment it only for this fallback path, and include it in the run
summary alongside the existing inserted/skipped/failed counts.
- Around line 458-467: Parallelize the per-instance Spider2-Snow gold SQL
fetches in the loop around `_fetch_spider2_snow_gold_sql` using a small
`ThreadPoolExecutor`, submitting independent `sf_` instance requests and
collecting their results before updating metadata. Preserve the existing
`_fetch_spider2_snow_gold_sql` error handling and result mapping, while limiting
concurrency to avoid excessive GitHub requests.
- Around line 681-692: The tables_with_zero check currently performs one
GoldenQuestion query per table; replace this N+1 loop with a single query that
selects GoldenQuestion.table_id for all table IDs, build the returned ID set,
and identify tables whose IDs are absent while preserving the existing fully
qualified names and warning behavior.
- Around line 85-86: Align the environment-variable name used by the
documentation, runtime error, and lookup in the sync metadata flow. Update the
lookup associated with the OM_JWT_TOKEN variable to use the single canonical
name established elsewhere in the application, and ensure the docstring and
missing-variable error report that same name consistently.
In `@backend/tests/test_api.py`:
- Around line 263-283: Add resource-cleanup assertions to
test_execute_query_failure, verifying the mocked cursor and Trino connection are
closed after mock_cur.execute raises. Use the existing mock_cur and mock_conn
symbols and preserve all current response assertions.
In `@frontend/src/components/monitoring/RunHistoryTable.tsx`:
- Around line 396-400: Update the refetchInterval callback in RunHistoryTable so
it returns 5_000 while any run is running, but returns false when no run is
running, stopping unconditional polling and preserving manual refresh behavior.
In `@frontend/src/components/tables/EvaluationTab.tsx`:
- Around line 19-29: Remove the local ScoreRing definition from
frontend/src/components/tables/EvaluationTab.tsx lines 19-29 and import the
shared component from frontend/src/components/common/EvalUI.tsx. In
frontend/src/pages/SandboxPage.tsx lines 11-22, remove its duplicate ScoreRing
and import the same shared component; move the .score-ring styles into the
shared component’s styling so they load for both usages.
In `@frontend/src/pages/EvaluationsPage.tsx`:
- Around line 420-433: Update handleLaunch’s triggerDatasetMut.onError callback
to display an error through App.useApp()’s message.error, using the
server-provided detail as RunTriggerPanel does, while preserving the existing
runningDataset and runningRunId reset behavior.
- Around line 194-206: Replace the repeated `owner_id === 'spider2'` checks in
`filteredTables`, the other `EvaluationsPage` location, and `TableList` with a
shared `isSpider2Table` constant or predicate. Centralize the dataset-name
literal and owner-field access/casting there, then reuse it while preserving the
existing Spider2 filtering behavior.
- Around line 503-520: Keep the refetchInterval callback pure by only returning
false for completed or failed runs, without updating React state. Add an effect
tied to the running-run-status query data that clears runningDataset and
runningRunId when the run reaches a terminal status.
In `@scripts/generate_trino_catalogs.py`:
- Around line 102-126: Make get_question_referenced_db_ids() resilient to GitHub
fetch, HTTP, and parsing-related failures by catching the external-fetch
exception, logging a warning, and returning None as a no-filter fallback. Update
main() to treat referenced_db_ids is None as “all discovered non-denied
databases,” preserving the prior catalog-generation behavior while retaining
filtering when the fetch succeeds.
- Around line 102-126: The Spider2-Snow JSONL fetch and sf_ filtering are
duplicated across two files; extract them into one shared core helper. In
scripts/generate_trino_catalogs.py, update get_question_referenced_db_ids() to
call that helper and retain only this script’s uppercasing/set construction. In
backend/app/sync_om_metadata.py, refactor fetch_spider2_snow_questions() to use
the same helper while keeping gold-SQL downloading and translation local; update
both files as specified: scripts/generate_trino_catalogs.py lines 102-126 and
backend/app/sync_om_metadata.py lines 488-532.
---
Outside diff comments:
In `@agent/src/agent/nodes/query_builder.py`:
- Around line 13-36: Pass the computed feedback_str into the chain.ainvoke input
in the query-builder node, and update the text2sql/query_builder prompt
definition in upload_all_prompts.py to include a {{feedback_str}} placeholder so
rejection feedback and loaded-skill prompts reach the LLM.
In `@backend/app/routers/evaluation.py`:
- Around line 493-516: Remove the loop-side table_names initialization and
append operation in the production-table processing block. Keep the later
schema-qualified table_names comprehension unchanged, as it is the value used
after the loop.
In `@frontend/src/components/monitoring/RunHistoryTable.tsx`:
- Around line 403-424: Base the empty-state check, pagination, and page count on
filtered runs by moving the visibleRuns computation before those branches. Clamp
the current page when visibleRuns or totalPages changes, derive a safePage
value, and use it for slicing plus all pagination controls and page labels;
ensure an entirely excluded result renders EmptySlate rather than an empty
table.
In `@frontend/src/components/tables/TableList.tsx`:
- Around line 189-196: Update the empty-state branch in the TableList rendering
to distinguish an empty raw dataset from a non-empty rawData set whose
post-filtered data is empty. Use rawData?.length to show a “no tables match the
current filters” message for filtered-out results, while preserving the existing
noData/Add Table message when no tables exist at all.
In `@frontend/src/pages/SandboxPage.tsx`:
- Around line 11-22: Make the running ScoreRing markup in SandboxPage render
with its required styles by either adding the .score-ring--running rules to
globals.css or importing EvaluationTab.css from SandboxPage.tsx. Preserve the
existing ScoreRing status handling and avoid duplicating styles across both
locations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fcc587d4-0505-4b5b-9f35-e8d93b4cc834
⛔ 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 (54)
agent/scripts/upload_all_prompts.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/chat.pybackend/app/config.pybackend/app/infra_init.pybackend/app/main.pybackend/app/routers/__init__.pybackend/app/routers/agent.pybackend/app/routers/evaluation.pybackend/app/routers/flags.pybackend/app/routers/orchestration.pybackend/app/routers/query.pybackend/app/services/evaluator.pybackend/app/services/flag_service.pybackend/app/services/langfuse_client.pybackend/app/services/scheduler.pybackend/app/services/trino_client.pybackend/app/spider2_questions.jsonbackend/app/sync_om_metadata.pybackend/pyproject.tomlbackend/tests/test_api.pycore/src/core/models/models.pycore/src/core/trino.pydocker-compose.ymlfrontend/nginx.conffrontend/package.jsonfrontend/src/api/orchestration.tsfrontend/src/components/JsonTreeView.tsxfrontend/src/components/flags/FlagEditor.tsxfrontend/src/components/flags/ModeCard.tsxfrontend/src/components/monitoring/RunHistoryTable.cssfrontend/src/components/monitoring/RunHistoryTable.tsxfrontend/src/components/tables/EvaluationTab.cssfrontend/src/components/tables/EvaluationTab.tsxfrontend/src/components/tables/TableList.cssfrontend/src/components/tables/TableList.tsxfrontend/src/config/flagsConfig.tsfrontend/src/hooks/useEvaluations.tsfrontend/src/index.cssfrontend/src/pages/AgentTestingPage.module.cssfrontend/src/pages/ControlCenterPage.tsxfrontend/src/pages/EvaluationsPage.cssfrontend/src/pages/EvaluationsPage.tsxfrontend/src/pages/FlagsPage.cssfrontend/src/pages/SandboxPage.tsxfrontend/src/tests/components.test.tsxfrontend/tests/agent-testing.spec.tsfrontend/tests/real-agent.spec.tsinfra/trino/etc/jvm.configscripts/generate_trino_catalogs.pytext2sql_test
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
elirazpevz
reviewed
Jul 27, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
backend/app/routers/agent.py (1)
203-216: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftPaginate and avoid loading table embeddings for this listing endpoint.
select(Table).all()fetches everyTablerow, including itsVector(768)embedding, althoughTableReaddoes not return it. This creates unbounded database transfer and memory use; it will undermine the table-selection/infinite-scroll flow as metadata grows. Use cursor/limit pagination and project/load onlyTableReadfields.🤖 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/agent.py` around lines 203 - 216, Update get_agent_tables to accept cursor and limit parameters, apply deterministic cursor-based pagination with a bounded maximum limit, and return only the fields required by TableRead rather than full Table rows including embeddings. Preserve the optional status filter and ensure the query returns the expected TableRead response shape.backend/app/infra_init.py (1)
1282-1288: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate column-level failures into
is_partial.
run_table_profilingstores per-column failures inColumnStats.errors, notresult.errors, and still setsresult.successto true when rows exist. This can persist an incomplete profile ascompletedwithis_partial=False. Aggregate column errors here or propagate them intoresult.errorsupstream.🛠️ Proposed fix
+ has_column_errors = any( + bool(cs.errors) for cs in result.column_stats + ) - profile.is_partial = not result.success or bool(result.errors) + profile.is_partial = ( + not result.success + or bool(result.errors) + or has_column_errors + )🤖 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 1282 - 1288, Update the profiling result handling in run_table_profiling so profile.is_partial also reflects failures recorded in each ColumnStats.errors, rather than relying only on result.errors and result.success. Aggregate or propagate those column-level errors before assigning profile.status and profile.is_partial, preserving completed status only when the profile has no column or result errors.frontend/src/pages/EvaluationsPage.tsx (1)
588-591: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHide the Spider2 dataset run when Spider2 is disabled.
excludeTableIdscannot exclude the dataset-level Spider2 run because it hastable_id=None; its History row remains visible with the toggle off. Also excludetriggered_by === 'spider2'inRunHistoryTableor the backing API.🤖 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/EvaluationsPage.tsx` around lines 588 - 591, Update the RunHistoryTable filtering used by EvaluationsPage so disabling Spider2 excludes both spider2TableIds and history rows whose triggered_by value is "spider2", including the dataset-level run with no table_id. Apply this in RunHistoryTable or its backing API while preserving the existing behavior when showSpider2 is enabled.core/src/core/models/models.py (1)
152-159: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd the migration for model schema changes.
EvalRunRead.table_idalready acceptsNone, butprofiling_runsis a new persisted table and this model change can alter an existingeval_runs.table_idNOT NULL constraint. Add an Alembic migration creatingprofiling_runsand adjusting the existingeval_runsschema so upgraded deployments do not fail.🤖 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/models/models.py` around lines 152 - 159, Add an Alembic migration for the model changes represented by EvalRunRead.table_id and the new profiling_runs table: create profiling_runs with the required schema, and alter eval_runs.table_id to allow NULL while preserving its foreign-key behavior. Include the corresponding downgrade operations so upgraded deployments can migrate cleanly and revert safely.agent/src/agent/nodes/refiner.py (3)
104-104: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
json.dumps(error_history)escapes non-ASCII; passensure_ascii=False.Trino errors and satisfaction failures (Line 42) contain Hebrew, which becomes
\uXXXXescapes in the prompt — extra tokens and harder for the model to correlate with the user's question. Line 182 in this same file already usesensure_ascii=False.Proposed fix
- "error_history": json.dumps(error_history), + "error_history": json.dumps(error_history, ensure_ascii=False),🤖 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 104, Update the error_history serialization in the refiner node to call json.dumps with ensure_ascii=False, matching the existing serialization behavior elsewhere in the same file and preserving non-ASCII error content in prompts.
135-144: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGraceful ESCA degradation is incomplete downstream.
Not raising here is right, but
esca_write_failedisn't what the finalizer branches on:agent/src/agent/nodes/finalizer.pyLine 76 selects the inline-preview path only whenesca_write_enabledis false. With ESCA enabled and the write failed,raw_data_refisNone, so the finalizer callsget_esca_preview(None)and returns"No data reference found."— the user gets a contentless summary even thoughinline_result_rows/inline_result_columnswere populated at Lines 122-123.Gate the finalizer's inline path on
not esca_write_enabled or esca_write_failed(or onraw_data_refbeing falsy) so the fallback rows are actually used.🤖 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` around lines 135 - 144, The finalizer must use inline result rows when an ESCA write fails. Update the finalizer branch around its ESCA preview selection to take the inline path when ESCA is disabled or esca_write_failed is true (alternatively, when raw_data_ref is falsy), while preserving normal ESCA preview behavior for successful writes.
17-23: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
build_refiner_schema_contextis defined twice — this first definition is dead, and the surviving one can no longer find its inputs.Line 157 redefines
build_refiner_schema_contextat module scope, so the later definition wins and the call at Line 91 resolves to thetable_profiles/schema_planversion, not thisjeen_catalogone. Combined with theschema_explorer_noderewrite inagent/src/agent/nodes/schema_explorer.py(Lines 383-387), which now returns onlyjeen_catalogand no longer populatestable_profilesorschema_plan, the refiner will always inject the literal string"No schema context available."into the prompt — silently gutting the refinement loop while every retry still burns LLM calls.Also,
llm = get_llm("refiner")at Line 17 runs at import time, is never read (Line 88 builds_llmwithruntime_flags), and makes module import fail if LLM config is missing.Proposed fix
-from agent.utils.serialization import json_serial -import datetime - -llm = get_llm("refiner") - -def build_refiner_schema_context(state: AgentState) -> str: - catalog = state.get("jeen_catalog") - if not catalog: - return "No schema context available." - return catalog - +from agent.utils.serialization import json_serialAnd fold the
jeen_cataloglookup into the single remaining definition at Line 157:def build_refiner_schema_context(state: AgentState) -> str: catalog = state.get("jeen_catalog") if catalog: return catalog table_profiles = state.get("table_profiles") ...🤖 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` around lines 17 - 23, Remove the unused module-level `llm = get_llm("refiner")` initialization so importing the module does not require LLM configuration. Keep only one `build_refiner_schema_context` definition, updating the surviving definition to return `state["jeen_catalog"]` when present and otherwise preserve its existing `table_profiles`/`schema_plan` fallback behavior, ensuring the refiner uses the catalog produced by `schema_explorer_node`.agent/scripts/upload_all_prompts.py (4)
170-170: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSelf-contradictory constraint that may suppress required Hebrew literals.
UTF-8 is a Unicode encoding, so "NEVER use Unicode characters, only utf-8" is unenforceable as written. Worse, combined with the Hebrew user requests this agent handles, the model may read it as a ban on Hebrew string literals in
WHEREclauses, which are needed to filter Hebrew column values. Line 163/173 already covers the actual intent (no Hebrew identifiers), so this bullet can be dropped or restated precisely.Proposed fix
- * NEVER use Unicode characters, only utf-8. + * Non-ASCII characters (e.g. Hebrew) are allowed only inside string literal values, never in identifiers or aliases.🤖 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` at line 170, Remove the contradictory “NEVER use Unicode characters, only utf-8” instruction from the prompt near the Hebrew identifier guidance, preserving the existing rule that prohibits Hebrew identifiers while allowing Hebrew string literals in SQL filters.
300-316: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
data_previewis injected under the "SQL explanation:" label.Line 309-310 labels
{{data_preview}}as "SQL explanation", and there is no slot for the actual explanation.finalizer.py(Lines 106-113) passesdata_previewas the results preview, so the model receives result rows announced as an explanation — directly undermining the system-prompt rules "summarize them" and "NEVER rewrite the SQL query or its explanation".Also note the system prompt declares the Hebrew explanation as input
#2, butfinalizer.pycomputessql_explanationconcurrently with the summary (Lines 115-117), so it can never be passed here. Either drop that claim or sequence the two calls.Proposed fix
- SQL explanation: + Data preview: {{data_preview}}🤖 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 300 - 316, Correct the prompt template so the value passed as data_preview is labeled as the SQL results/data preview, not “SQL explanation.” In the finalizer flow around the concurrent sql_explanation and summary generation, either pass the actual sql_explanation into the prompt by sequencing the calls appropriately, or remove the SQL explanation input claim if it is not required; keep the prompt inputs consistent with the values supplied by finalizer.py.
102-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale
text2sql/schema_explorerupload.
schema_explorer_nodenow only fetches Jeen MCP’scatalog_prompt, and this prompt is only referenced by the upload script/config declaration. Drop the Langfuse prompt entry/name so the artifact upload doesn’t keep stale, unused prompt content in sync.🤖 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 102 - 123, Remove the stale `text2sql/schema_explorer` prompt entry from the upload script, including its content block and upload/config declaration. Keep the current `schema_explorer_node` flow that fetches Jeen MCP’s `catalog_prompt` unchanged, and ensure no remaining upload references use the removed prompt name.
400-449: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the duplicate “EXECUTION WORKFLOW” step list.
detect_ambiguityinjectscurrent_time,Current Agent SQL Attempt, andsql_explanationbefore formatting, so the missing-variable concern is covered. The remaining prompt issue is the 4-step list followed immediately by a different 5-step list; merge them so the same numbered step does not have competing definitions.🤖 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 400 - 449, In the prompt assembled by detect_ambiguity, remove the initial duplicate EXECUTION WORKFLOW four-step list and its associated repeated processing instructions. Retain and use the later five-step workflow, including Agent Proposal Audit and Final Decision, so each numbered step has one definition while preserving the injected current_time, Current Agent SQL Attempt, and sql_explanation context.agent/src/agent/nodes/schema_explorer.py (1)
301-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
loggerinstead ofThis module already has a
logger(used at Line 358/381).Proposed fix
- print(f"Trino DESCRIBE fallback failed for {table.name}: {e}") + logger.warning( + "Trino DESCRIBE fallback failed for %s: %s", table.name, e + )🤖 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` at line 301, Replace the print call in the Trino DESCRIBE fallback failure path with the module’s existing logger, preserving the table name and exception details in the logged message. Reuse the logger already used elsewhere in the module rather than introducing a new logging mechanism.docker-compose.yml (1)
1-7: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove hard-coded credentials from the production Compose path.
This file commits
postgres:postgres,admin/password123, andJWT_SECRET: dev-secret-change-in-productionwhileAPP_ENVis set toproduction. Use required environment substitutions or Docker secrets, and fail startup when credentials are missing.Also applies to: 497-520
🤖 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 1 - 7, Update the production Compose configuration, including the shared database environment and the services around the referenced production section, to replace hard-coded database credentials, admin credentials, and JWT_SECRET values with required environment substitutions or Docker secrets. Ensure startup fails when any required credential is unset, while preserving the existing production service wiring.Source: Linters/SAST tools
🤖 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/src/agent/utils/serialization.py`:
- Around line 4-8: The json_serial helper must support all raw Trino row value
types used by both preview paths: add handling for datetime.time, timedelta,
Decimal, UUID, and bytes before its TypeError fallback, using the appropriate
serialized representation. In agent/src/agent/utils/serialization.py lines 4-8
update json_serial; in agent/src/agent/nodes/finalizer.py lines 33-40 change the
preview serialization configuration from default=str to default=json_serial so
both preview paths use the same helper.
In `@backend/tests/test_evaluation_flow.py`:
- Around line 57-70: Update the test around execute_single_table_eval to stop
patching RunDatasetResponse, allowing the real DTO validation and parsing to
run. Configure mock_post_response.json.return_value with a realistic
evaluation-service payload matching the DTO fields, then assert the returned
score or persisted metrics while keeping unrelated mocks such as
_map_and_save_run_metrics as needed.
In `@core/src/core/spider2.py`:
- Line 2: Declare requests as a direct dependency of core in its package
dependency configuration, alongside the dependencies used by core.spider2. Keep
the existing import and backend dependency declarations unchanged.
In `@scripts/generate_trino_catalogs.py`:
- Around line 104-123: In get_question_referenced_db_ids, narrow the try/except
around fetch_spider2_snow_sf_questions to catch only the expected GitHub, HTTP,
and parsing-related exceptions that justify disabling filtering. Allow
programming and schema errors, including failures while processing q["db_id"],
to propagate instead of returning None; preserve the existing warning and
fallback for expected external failures.
---
Outside diff comments:
In `@agent/scripts/upload_all_prompts.py`:
- Line 170: Remove the contradictory “NEVER use Unicode characters, only utf-8”
instruction from the prompt near the Hebrew identifier guidance, preserving the
existing rule that prohibits Hebrew identifiers while allowing Hebrew string
literals in SQL filters.
- Around line 300-316: Correct the prompt template so the value passed as
data_preview is labeled as the SQL results/data preview, not “SQL explanation.”
In the finalizer flow around the concurrent sql_explanation and summary
generation, either pass the actual sql_explanation into the prompt by sequencing
the calls appropriately, or remove the SQL explanation input claim if it is not
required; keep the prompt inputs consistent with the values supplied by
finalizer.py.
- Around line 102-123: Remove the stale `text2sql/schema_explorer` prompt entry
from the upload script, including its content block and upload/config
declaration. Keep the current `schema_explorer_node` flow that fetches Jeen
MCP’s `catalog_prompt` unchanged, and ensure no remaining upload references use
the removed prompt name.
- Around line 400-449: In the prompt assembled by detect_ambiguity, remove the
initial duplicate EXECUTION WORKFLOW four-step list and its associated repeated
processing instructions. Retain and use the later five-step workflow, including
Agent Proposal Audit and Final Decision, so each numbered step has one
definition while preserving the injected current_time, Current Agent SQL
Attempt, and sql_explanation context.
In `@agent/src/agent/nodes/refiner.py`:
- Line 104: Update the error_history serialization in the refiner node to call
json.dumps with ensure_ascii=False, matching the existing serialization behavior
elsewhere in the same file and preserving non-ASCII error content in prompts.
- Around line 135-144: The finalizer must use inline result rows when an ESCA
write fails. Update the finalizer branch around its ESCA preview selection to
take the inline path when ESCA is disabled or esca_write_failed is true
(alternatively, when raw_data_ref is falsy), while preserving normal ESCA
preview behavior for successful writes.
- Around line 17-23: Remove the unused module-level `llm = get_llm("refiner")`
initialization so importing the module does not require LLM configuration. Keep
only one `build_refiner_schema_context` definition, updating the surviving
definition to return `state["jeen_catalog"]` when present and otherwise preserve
its existing `table_profiles`/`schema_plan` fallback behavior, ensuring the
refiner uses the catalog produced by `schema_explorer_node`.
In `@agent/src/agent/nodes/schema_explorer.py`:
- Line 301: Replace the print call in the Trino DESCRIBE fallback failure path
with the module’s existing logger, preserving the table name and exception
details in the logged message. Reuse the logger already used elsewhere in the
module rather than introducing a new logging mechanism.
In `@backend/app/infra_init.py`:
- Around line 1282-1288: Update the profiling result handling in
run_table_profiling so profile.is_partial also reflects failures recorded in
each ColumnStats.errors, rather than relying only on result.errors and
result.success. Aggregate or propagate those column-level errors before
assigning profile.status and profile.is_partial, preserving completed status
only when the profile has no column or result errors.
In `@backend/app/routers/agent.py`:
- Around line 203-216: Update get_agent_tables to accept cursor and limit
parameters, apply deterministic cursor-based pagination with a bounded maximum
limit, and return only the fields required by TableRead rather than full Table
rows including embeddings. Preserve the optional status filter and ensure the
query returns the expected TableRead response shape.
In `@core/src/core/models/models.py`:
- Around line 152-159: Add an Alembic migration for the model changes
represented by EvalRunRead.table_id and the new profiling_runs table: create
profiling_runs with the required schema, and alter eval_runs.table_id to allow
NULL while preserving its foreign-key behavior. Include the corresponding
downgrade operations so upgraded deployments can migrate cleanly and revert
safely.
In `@docker-compose.yml`:
- Around line 1-7: Update the production Compose configuration, including the
shared database environment and the services around the referenced production
section, to replace hard-coded database credentials, admin credentials, and
JWT_SECRET values with required environment substitutions or Docker secrets.
Ensure startup fails when any required credential is unset, while preserving the
existing production service wiring.
In `@frontend/src/pages/EvaluationsPage.tsx`:
- Around line 588-591: Update the RunHistoryTable filtering used by
EvaluationsPage so disabling Spider2 excludes both spider2TableIds and history
rows whose triggered_by value is "spider2", including the dataset-level run with
no table_id. Apply this in RunHistoryTable or its backing API while preserving
the existing behavior when showSpider2 is enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1add95bb-c114-4768-9338-4d0811fd13b4
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
agent/scripts/upload_all_prompts.pyagent/src/agent/nodes/finalizer.pyagent/src/agent/nodes/refiner.pyagent/src/agent/nodes/schema_explorer.pyagent/src/agent/routers/chat.pyagent/src/agent/utils/serialization.pybackend/app/config.pybackend/app/infra_init.pybackend/app/routers/agent.pybackend/app/routers/evaluation.pybackend/app/routers/orchestration.pybackend/app/services/langfuse_client.pybackend/app/sync_om_metadata.pybackend/pyproject.tomlbackend/tests/test_evaluation_flow.pycore/src/core/__init__.pycore/src/core/models/models.pycore/src/core/spider2.pydocker-compose.ymlfrontend/src/components/common/EvalUI.tsxfrontend/src/components/tables/EvaluationTab.cssfrontend/src/components/tables/EvaluationTab.tsxfrontend/src/components/tables/TableList.tsxfrontend/src/config/constants.tsfrontend/src/pages/EvaluationsPage.tsxfrontend/src/pages/SandboxPage.tsxfrontend/src/styles/globals.cssscripts/generate_trino_catalogs.py
💤 Files with no reviewable changes (3)
- agent/src/agent/routers/chat.py
- backend/app/services/langfuse_client.py
- frontend/src/components/tables/EvaluationTab.css
Comment on lines
+4
to
+8
| def json_serial(obj): | ||
| """JSON serializer for objects not serializable by default (e.g. datetime/date).""" | ||
| if isinstance(obj, (datetime.datetime, datetime.date)): | ||
| return obj.isoformat() | ||
| raise TypeError("Type %s not serializable" % type(obj)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Narrow json_serial breaks Trino result serialization at both call sites. The shared helper only maps datetime/date and raises TypeError for everything else, while both call sites serialize raw Trino rows that routinely contain Decimal, datetime.time, bytes, and UUID.
agent/src/agent/utils/serialization.py#L4-L8: extend the helper to handledatetime.time,timedelta,Decimal,UUID, andbytesbefore raising.agent/src/agent/nodes/finalizer.py#L33-L40: once the helper is broadened, this path stops silently collapsing into"Error retrieving data preview from Esca"; align Line 91'sdefault=strwithdefault=json_serialso both preview paths serialize identically.
📍 Affects 2 files
agent/src/agent/utils/serialization.py#L4-L8(this comment)agent/src/agent/nodes/finalizer.py#L33-L40
🤖 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/utils/serialization.py` around lines 4 - 8, The json_serial
helper must support all raw Trino row value types used by both preview paths:
add handling for datetime.time, timedelta, Decimal, UUID, and bytes before its
TypeError fallback, using the appropriate serialized representation. In
agent/src/agent/utils/serialization.py lines 4-8 update json_serial; in
agent/src/agent/nodes/finalizer.py lines 33-40 change the preview serialization
configuration from default=str to default=json_serial so both preview paths use
the same helper.
Comment on lines
+57
to
+70
| with patch("app.routers.evaluation.langfuse_client.ensure_dataset_synced"): | ||
| with patch("app.routers.evaluation._map_and_save_run_metrics") as mock_map: | ||
| with patch("app.routers.evaluation.RunDatasetResponse") as mock_response_class: | ||
|
|
||
| # Create a mock response object that has the fields accessed by execute_single_table_eval | ||
| mock_eval_resp = MagicMock() | ||
| mock_eval_resp.accuracy.contains_accuracy = 1.0 | ||
| mock_eval_resp.accuracy.execution_accuracy = 1.0 | ||
| mock_eval_resp.accuracy.sql_exact_match = 1.0 | ||
| mock_eval_resp.total_cases = 1 | ||
| mock_eval_resp.failure_rate = 0.0 | ||
| mock_response_class.return_value = mock_eval_resp | ||
|
|
||
| score = execute_single_table_eval(table_id, run_id, mock_session) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not mock the response DTO being validated.
Patching RunDatasetResponse bypasses response parsing entirely, so malformed or renamed evaluation-service fields still pass this flow. Configure mock_post_response.json.return_value with a realistic payload and use the real DTO; then assert the returned score or persisted metrics.
🤖 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/test_evaluation_flow.py` around lines 57 - 70, Update the test
around execute_single_table_eval to stop patching RunDatasetResponse, allowing
the real DTO validation and parsing to run. Configure
mock_post_response.json.return_value with a realistic evaluation-service payload
matching the DTO fields, then assert the returned score or persisted metrics
while keeping unrelated mocks such as _map_and_save_run_metrics as needed.
| @@ -0,0 +1,33 @@ | |||
| import json | |||
| import requests | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -eu
sed -n '/^\[project\]/,/^\[/p' core/pyproject.toml
rg -nP '^\s*"requests(?:\[.*\])?\s*(?:[<>=!~@]|")' core/pyproject.toml backend/pyproject.tomlRepository: StavPonte11/text2sql-onboarding
Length of output: 745
🏁 Script executed:
#!/usr/bin/env bash
set -eu
printf '\n--- candidate files ---\n'
fd '^(__init__\.py|spider2\.py)$' core
printf '\n--- core/__init__.py relevant imports ---\n'
rg -n 'spider2|import .*core|from .*core|import requests' core/src/core/__init__.py || true
printf '\n--- spider2.py imports ---\n'
sed -n '1,40p' core/src/core/spider2.py
printf '\n--- all core package imports in core/src ---\n'
rg -n '^\s*import\s+core\b|^\s*from\s+(?:core|.*\.core)\s' core/src || trueRepository: StavPonte11/text2sql-onboarding
Length of output: 1598
Declare requests as a direct core dependency.
core.__init__.py imports core.spider2, and core.spider2 uses requests; requests is only declared by backend, so an isolated core install can fail before unrelated exports are reachable.
🤖 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/spider2.py` at line 2, Declare requests as a direct dependency
of core in its package dependency configuration, alongside the dependencies used
by core.spider2. Keep the existing import and backend dependency declarations
unchanged.
Comment on lines
+104
to
+123
| def get_question_referenced_db_ids() -> set[str] | None: | ||
| """ | ||
| Fetch the Spider2-Snow sf_ question set and return the distinct db_id | ||
| values it references (uppercased, matching Snowflake's SHOW DATABASES | ||
| casing). Used to skip generating Trino catalogs for databases that will | ||
| never have a single golden question -- no point ingesting/syncing them | ||
| at all. | ||
| """ | ||
|
|
||
| try: | ||
| sf_questions = fetch_spider2_snow_sf_questions() | ||
| except Exception as exc: | ||
| logger.warning( | ||
| "Could not fetch golden-question db_id set from GitHub (%s); " | ||
| "will not filter target databases by referenced questions.", exc | ||
| ) | ||
| return None | ||
|
|
||
| return {q["db_id"].upper() for q in sf_questions if q.get("db_id")} | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Narrow the fallback exception boundary.
except Exception treats programming or schema errors the same as transient GitHub, HTTP, or parsing failures. That can silently disable filtering and cause main() to target every discovered database. Catch expected external/parsing exceptions and let unexpected errors surface, or explicitly document and test this broad fallback.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 115-115: 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 `@scripts/generate_trino_catalogs.py` around lines 104 - 123, In
get_question_referenced_db_ids, narrow the try/except around
fetch_spider2_snow_sf_questions to catch only the expected GitHub, HTTP, and
parsing-related exceptions that justify disabling filtering. Allow programming
and schema errors, including failures while processing q["db_id"], to propagate
instead of returning None; preserve the existing warning and fallback for
expected external failures.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes