Consolidate duplicated dialect and chain helpers - #1361
Merged
Merged
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 1 minute Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR consolidates duplicated database-dialect helpers (SQLite/Postgres detection, placeholder selection, table/column introspection, api_usage schema DDL) and chain utility functions (JSON extraction, row conversion, context guarding, connection acquisition) plus CIK normalization into shared modules ( ChangesShared helper consolidation
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates previously duplicated DB dialect/table helpers and chain utility helpers into shared modules (adapters.base, chains.utils, and utils.identifiers), updates call sites to use the shared implementations, and adds a regression test intended to prevent helper duplication from reappearing.
Changes:
- Added shared DB helpers (
is_sqlite,get_placeholder,table_exists,get_table_columns,ensure_api_usage_schema) inadapters/base.pyand replaced many local copies across ETL/API/LLM code. - Added shared chain helpers (
rows_to_dicts,extract_json_text,guard_context_values,acquire_connection) inchains/utils.pyand replaced local chain implementations. - Added
normalize_cikinutils/identifiers.pyand updated API/scripts to import it; introduced a regression test to detect redefinitions.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/identifiers.py | Introduces shared CIK normalization helper. |
| tests/test_shared_helper_consolidation.py | Adds regression test intended to prevent helper redefinitions. |
| scripts/seed_universe.py | Switches to shared normalize_cik. |
| scripts/resolve_aliases.py | Switches to shared normalize_cik. |
| llm/cost_tracking.py | Uses shared placeholder/dialect + shared api_usage schema helper. |
| etl/news_flow.py | Replaces local placeholder/sqlite checks with shared helpers. |
| etl/ingest_flow.py | Replaces local dialect/columns helpers with shared helpers. |
| etl/evaluation_flow.py | Uses shared placeholder + shared api_usage schema helper. |
| etl/edgar_flow.py | Replaces local columns/placeholder helpers with shared helpers. |
| etl/digest_flow.py | Replaces local table/columns/placeholder helpers with shared helpers. |
| etl/daily_diff_flow.py | Replaces local placeholder/sqlite checks with shared helpers. |
| etl/conviction_flow.py | Uses shared placeholder + api_usage schema helper inside the flow. |
| etl/activism_flow.py | Replaces local placeholder/sqlite checks with shared helpers. |
| etl/activism_detection.py | Replaces local placeholder/sqlite checks with shared helpers. |
| chains/utils.py | Adds shared chain parsing/connection/context-guard helpers. |
| chains/rag_search.py | Adopts shared chain helpers + shared DB helpers for dialect/table checks. |
| chains/nl_query.py | Adopts shared chain helpers + shared DB dialect detection. |
| chains/holdings_analysis.py | Uses shared JSON extraction/row mapping + shared api_usage schema helper. |
| chains/filing_summary.py | Uses shared JSON extraction/row mapping + shared api_usage schema helper. |
| api/signals.py | Replaces local table/columns/placeholder helpers with shared helpers. |
| api/search.py | Replaces local table/columns helpers with shared helpers. |
| api/managers.py | Switches to shared normalize_cik. |
| api/chat.py | Replaces local placeholder/columns/sqlite checks with shared helpers. |
| api/activism.py | Replaces local placeholder/table/sqlite checks with shared helpers. |
| adapters/base.py | Adds shared DB dialect/table helpers and consolidates api_usage schema creation. |
Owner
Author
|
Closer pushed Changes:
Validation from disposable checkout
Fresh GitHub checks are expected on the new head. |
Contributor
|
Runner dispatch state for codex on PR #1361. Do not edit. |
Contributor
🤖 Keepalive Loop StatusPR #1361 | Agent: Codex | Iteration 0/12 Current State
🔍 Failure Classification| Error type | infrastructure | 🧠 Task Analysis| Provider | ✅ GitHub Models (primary) |
|
Contributor
Keepalive Work Log (click to expand)
|
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
etl/digest_flow.py (1)
164-175: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMissed consolidation: raw
isinstancecheck alongside newly-imported shared helpers.
get_table_columns/get_placeholder/table_existsare now imported fromadapters.base, but line 174 still doesFalse if not isinstance(conn, sqlite3.Connection) else 0instead of usingis_sqlite(conn). This keeps a directsqlite3dependency in this function even though dialect detection was supposed to be centralized.♻️ Suggested fix
-from adapters.base import connect_db, get_placeholder, get_table_columns, table_exists +from adapters.base import connect_db, get_placeholder, get_table_columns, is_sqlite, table_exists ... - (False if not isinstance(conn, sqlite3.Connection) else 0, window_start.isoformat()), + (False if not is_sqlite(conn) else 0, window_start.isoformat()),🤖 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 `@etl/digest_flow.py` around lines 164 - 175, Replace the remaining direct sqlite3 type check in the alert_history query path with the shared dialect helper so dialect detection stays centralized. In digest_flow.py, update the logic in the alert_history fetch block that currently uses isinstance(conn, sqlite3.Connection) to instead use is_sqlite(conn), matching the imported helpers from adapters.base. Keep the query and parameter selection behavior the same, just route the SQLite-specific placeholder/value choice through the shared helper.etl/evaluation_flow.py (1)
362-381: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSame missed consolidation as
etl/digest_flow.py: rawisinstanceinstead ofis_sqlite.
ensure_api_usage_schemaandget_placeholderare pulled fromadapters.base, but the commit decision at line 379 still usesisinstance(conn, sqlite3.Connection)directly rather than the sharedis_sqlite(conn)helper used consistently elsewhere in this PR (e.g.etl/daily_diff_flow.pyline 368).♻️ Suggested fix
-from adapters.base import connect_db, ensure_api_usage_schema, get_placeholder +from adapters.base import connect_db, ensure_api_usage_schema, get_placeholder, is_sqlite ... - if isinstance(conn, sqlite3.Connection): + if is_sqlite(conn): conn.commit()🤖 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 `@etl/evaluation_flow.py` around lines 362 - 381, The commit check in log_evaluation_summary still uses a raw sqlite3.Connection type test instead of the shared adapter helper. Replace the direct isinstance(conn, sqlite3.Connection) call with is_sqlite(conn) so this path matches the same consolidation used elsewhere and stays consistent with ensure_api_usage_schema and get_placeholder from adapters.base.
🤖 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 `@api/chat.py`:
- Around line 571-573: The _manager_id_column helper in chat.py has drifted from
the shared version used in api/signals.py, api/managers.py, etl/ingest_flow.py,
and scripts/resolve_aliases.py. Update _manager_id_column to match the
consolidated behavior: add the table_exists guard, return None when the managers
table is absent, and align the return type with the shared helper so the
duplicated logic stays consistent across callers.
- Around line 870-877: The local `_postgres_table_exists` helper duplicates the
shared `table_exists` logic this PR is consolidating, so
`_ensure_chat_feedback_table` should use the shared `adapters.base.table_exists`
helper instead. Remove the local PostgreSQL-only existence check and update the
chat feedback table setup to call `table_exists(conn, "chat_feedback")` like the
other shared-helper call sites in `api/search.py` and `api/signals.py`, unless a
schema-qualified lookup is strictly required.
In `@api/signals.py`:
- Around line 132-141: The helper _manager_id_column is duplicated across
multiple modules and is not covered by the new consolidation gate, so it can
still drift in behavior. Move the shared logic into a single consolidated
location such as adapters.base alongside get_table_columns, or register
_manager_id_column in tests/test_shared_helper_consolidation.py’s
CONSOLIDATED_HELPERS map so all copies are enforced. Update the call sites in
api/managers.py, etl/ingest_flow.py, scripts/resolve_aliases.py, and the
simplified api/chat.py variant to reference the shared definition or match the
consolidated implementation.
In `@chains/utils.py`:
- Around line 121-134: guard_context_values currently only checks direct string
items in lists, so nested dicts inside list values can bypass the
prompt-injection guard. Update guard_context_values to recurse into nested
containers within the list branch as well, applying guard_input to every string
found inside items like dicts or deeper lists, while keeping the existing
behavior for top-level strings and dict values.
In `@etl/activism_detection.py`:
- Around line 44-45: The _is_postgres helper in activism_detection.py is still a
file-local duplicate and has drifted from the same logic in activism_flow.py and
daily_diff_flow.py. Move the dialect check into adapters.base as a shared
is_postgres helper alongside is_sqlite and get_placeholder, then replace each
local _is_postgres usage/import with the centralized is_postgres symbol so all
ETL flows share one implementation.
In `@etl/activism_flow.py`:
- Around line 36-41: The `_is_postgres` helper logic is duplicated and already
drifting across flows, so centralize it instead of keeping local copies. Hoist a
single `is_postgres` helper into `adapters.base`, then update
`etl/activism_flow.py` (and the matching helpers in `etl/activism_detection.py`
and `etl/daily_diff_flow.py`) to use that shared symbol rather than
reimplementing `not is_sqlite(conn)` / `hasattr(conn, "execute")` checks
locally.
In `@etl/daily_diff_flow.py`:
- Around line 30-31: The _is_postgres helper in daily_diff_flow.py has drifted
from the matching helpers in activism_detection.py and activism_flow.py by
dropping the hasattr(conn, "execute") guard. Update _is_postgres to use the same
logic as those other copies, or better, replace all three with a single shared
helper so the behavior stays consistent and the duplicated implementations do
not diverge again.
In `@tests/test_shared_helper_consolidation.py`:
- Around line 39-43: The duplicate scan in the shared-helper consolidation test
is too broad because `ast.walk` matches class methods as well as top-level
helpers, causing false positives for generic names like `_columns` and
`_placeholder`. Tighten the check in the test logic around
`CONSOLIDATED_HELPERS` so it only considers module-level `FunctionDef` and
`AsyncFunctionDef` nodes from the file’s top-level body, not methods nested
inside classes, while keeping the existing duplicate reporting format intact.
- Around line 10-24: The consolidation gate still keys off legacy helper names
instead of the current canonical shared names, so update CONSOLIDATED_HELPERS to
reference the real exported helpers in adapters/base.py and chains/utils.py (for
example the current table/connection and row/context helpers) rather than the
old private aliases, and make sure the scan covers the utils package too. Also
add utils to SCANNED_DIRS so duplicates of the shared identifier helper in
utils/identifiers.py are detected by the test.
In `@utils/identifiers.py`:
- Around line 8-16: normalize_cik is incorrectly deriving CIK digits from
stringified floats and can return more than 10 digits for malformed inputs.
Update the helper to normalize numeric inputs before digit extraction so values
like 1067983.0 preserve the intended integer CIK, and ensure any extracted digit
string is constrained to the documented 10-digit form. Keep the fix localized in
normalize_cik, since its output is consumed by api/managers.py,
scripts/resolve_aliases.py, and scripts/seed_universe.py for uniqueness and
upsert behavior.
---
Outside diff comments:
In `@etl/digest_flow.py`:
- Around line 164-175: Replace the remaining direct sqlite3 type check in the
alert_history query path with the shared dialect helper so dialect detection
stays centralized. In digest_flow.py, update the logic in the alert_history
fetch block that currently uses isinstance(conn, sqlite3.Connection) to instead
use is_sqlite(conn), matching the imported helpers from adapters.base. Keep the
query and parameter selection behavior the same, just route the SQLite-specific
placeholder/value choice through the shared helper.
In `@etl/evaluation_flow.py`:
- Around line 362-381: The commit check in log_evaluation_summary still uses a
raw sqlite3.Connection type test instead of the shared adapter helper. Replace
the direct isinstance(conn, sqlite3.Connection) call with is_sqlite(conn) so
this path matches the same consolidation used elsewhere and stays consistent
with ensure_api_usage_schema and get_placeholder from adapters.base.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8325172e-2592-4184-8e2f-bdf915e253fe
📒 Files selected for processing (26)
adapters/base.pyapi/activism.pyapi/chat.pyapi/managers.pyapi/search.pyapi/signals.pychains/filing_summary.pychains/holdings_analysis.pychains/nl_query.pychains/rag_search.pychains/utils.pyetl/activism_detection.pyetl/activism_flow.pyetl/conviction_flow.pyetl/daily_diff_flow.pyetl/digest_flow.pyetl/edgar_flow.pyetl/evaluation_flow.pyetl/ingest_flow.pyetl/news_flow.pyllm/cost_tracking.pyscripts/resolve_aliases.pyscripts/seed_universe.pytests/test_cost_script_dialect_portability.pytests/test_shared_helper_consolidation.pyutils/identifiers.py
Owner
Author
|
Closer pushed Changes:
Validation before push:
Resolved the addressed review threads. Fresh Gate/review checks are async on the new head. |
This was referenced Jul 1, 2026
Contributor
Provider Comparison ReportProvider Summary
📋 Full Provider Details (click to expand)openai
anthropic
Agreement
DisagreementNo major disagreements detected. Unique Insights
🔍 LangSmith Traces |
Contributor
|
Workflow state fingerprint for Agents Verifier. Do not edit. |
7 tasks
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
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.
Closes #1313
Summary
adapters.baseand replace local placeholder/table-column/api_usage copiesValidation
python -m pytest tests/test_shared_helper_consolidation.py tests/test_llm_cost_tracking.py tests/test_chain_dialect_portability.py tests/test_filing_summary_chain.py tests/test_holdings_analysis_chain.py tests/test_adapter_base.py -qpython -m ruff check ...focused touched filesblack --fast --check --line-length 100 --exclude '(\\.workflows-lib|node_modules)' ...focused touched files\n-git diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Tests