Resolve manager id column in ETL readers

[stranske]

Closes #1302

Summary

• add a shared manager-id column resolver for SQLite/Postgres manager schemas
• use the resolver in ETL/news/daily diff/activism/diff holdings manager lookups
• add a regression test for the API-created SQLite managers table using id

Validation

• python -m pytest tests/test_etl_flows_additional.py::test_etl_manager_id_readers_support_api_created_sqlite_schema -q
• python -m pytest tests/test_etl_flows_additional.py tests/test_daily_diff.py tests/test_edgar_flow.py::test_fetch_and_store_uses_postgres_safe_persistence tests/test_edgar_flow.py::test_replace_holdings_for_filing_uses_postgres_transaction -q
• python -m ruff check adapters/base.py etl/news_flow.py etl/daily_diff_flow.py etl/activism_flow.py diff_holdings.py tests/test_etl_flows_additional.py
• black --fast --check --line-length 100 --exclude "(\.workflows-lib|node_modules)" adapters/base.py etl/news_flow.py etl/daily_diff_flow.py etl/activism_flow.py diff_holdings.py tests/test_etl_flows_additional.py
• git diff --check

Summary by CodeRabbit

• Bug Fixes
  • Improved manager lookups so they work across different database backends, including SQLite.
  • Fixed several data retrieval paths to use the correct manager identifier column automatically.
  • Improved entity matching and report generation when manager records use different ID formats.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 39 minutes

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c5b0d556-8abe-4ca8-ad4c-2f0aa82865c0

📥 Commits

Reviewing files that changed from the base of the PR and between 2ed2cba and daf9167.

📒 Files selected for processing (7)

• adapters/base.py
• api/managers.py
• diff_holdings.py
• etl/activism_flow.py
• etl/daily_diff_flow.py
• etl/news_flow.py
• tests/test_etl_flows_additional.py

📝 Walkthrough

Walkthrough

Adds helper functions in adapters/base.py (is_sqlite, get_table_columns, manager_id_column) to dynamically detect the managers table primary key column name. Updates diff_holdings.py and three ETL flow modules (activism_flow, daily_diff_flow, news_flow) to use this resolver instead of hard-coded column names. Adds a corresponding test.

Changes

Dynamic manager ID column resolution

Layer / File(s)	Summary
Database helper functions adapters/base.py	Adds is_sqlite, get_table_columns, and manager_id_column to detect connection dialect and resolve the managers table ID column, returning empty/None on errors.
diff_holdings.py manager ID resolution diff_holdings.py	_resolve_manager_id now computes the ID column dynamically via manager_id_column, falling back per dialect, instead of hard-coding manager_id.
activism_flow.py manager ID queries etl/activism_flow.py	_load_manager_row and _all_manager_ids use the dynamically resolved ID column in their SQL queries.
daily_diff_flow.py manager ID query etl/daily_diff_flow.py	_fetch_all_manager_ids selects the dynamically resolved ID column instead of a fixed manager_id.
news_flow.py manager ID query etl/news_flow.py	match_entities selects the dynamically resolved ID column when querying the managers table.
Cross-module integration test tests/test_etl_flows_additional.py	New test builds an SQLite schema via api.managers helpers and verifies consistent manager ID resolution across news_flow, diff_holdings, and activism_flow.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related issues

• [Audit] ETL hardcodes manager_id but API-created SQLite managers table uses id #1302: Adds a shared manager-id column resolver and updates ETL readers to use it, directly addressing the same SQLite manager_id vs id mismatch.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: resolving the manager ID column across ETL readers.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/issue-1302-manager-id-resolver

---

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

This PR fixes ETL manager lookups to work against SQLite databases created via the Manager API (where the primary key column is id), while keeping Postgres behavior intact (where the column is manager_id). It does this by introducing a shared resolver for the managers table PK column and updating multiple ETL readers to use it, plus adding a regression test that exercises the API-created SQLite schema.

Changes:

• Added manager_id_column() (and supporting schema-introspection helpers) to resolve the managers PK column across SQLite/Postgres.
• Updated manager ID reads in news_flow, daily_diff_flow, activism_flow, and diff_holdings to use the resolved column.
• Added a regression test that creates the managers table via the API path and asserts ETL readers can load/resolve the inserted manager.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
adapters/base.py	Adds manager_id_column() and table column introspection utilities used by ETL readers.
etl/news_flow.py	Uses resolved manager PK column when selecting managers for entity matching.
etl/daily_diff_flow.py	Uses resolved manager PK column when enumerating managers.
etl/activism_flow.py	Uses resolved manager PK column for manager row lookup and ID enumeration.
diff_holdings.py	Uses resolved manager PK column when resolving a manager by CIK.
tests/test_etl_flows_additional.py	Adds regression coverage for API-created SQLite managers schema (id PK).

[coderabbitai]

Actionable comments posted: 5

🤖 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 `@adapters/base.py`:
- Around line 114-115: The exception handling in the table/column lookup logic
is too broad and silently hides real failures. Update the `except Exception`
block in the `adapters.base` lookup path to emit a debug or warning log before
returning `set()`, using the surrounding method name and the exception details
so connectivity, permission, or query errors are visible instead of being
treated like a missing table.
- Around line 101-116: The SQLite branch in get_table_columns still manually
escapes table_name before querying pragma_table_info, which should be replaced
with a bound parameter using the table-valued pragma_table_info(?) form. Update
the is_sqlite(conn) path in get_table_columns to pass table_name as a parameter
to conn.execute instead of interpolating it into the SQL string, and remove the
escaped_table handling entirely.

In `@diff_holdings.py`:
- Around line 69-73: The fallback logic for selecting the manager ID column is
duplicated across multiple call sites and should be centralized. Add a single
helper such as resolve_manager_id_column in adapters/base.py that wraps
manager_id_column(conn) with the sqlite3-vs-default fallback, then replace the
inline fallback in diff_holdings.py and the ETL flows (activism_flow,
daily_diff_flow, news_flow) with that helper so all callers share one consistent
behavior.

In `@etl/news_flow.py`:
- Around line 71-74: `match_entities` is doing repeated schema introspection for
`id_column` on every source, which duplicates the `manager_id_column` lookup and
`managers` query during `news_flow` runs. Resolve the manager ID column once in
`news_flow` (where `match_entities.fn` is invoked per source) and pass that
value into `match_entities`, or add connection-level caching so
`manager_id_column` is not re-run for each call. Keep the change centered around
`news_flow`, `match_entities`, and `manager_id_column`.

In `@tests/test_etl_flows_additional.py`:
- Around line 55-83: The sqlite connection in
test_etl_manager_id_readers_support_api_created_sqlite_schema is only closed at
the end of the test, so a failed assertion can leave it open. Wrap the body of
the test in a try/finally (or use a context-managed connection) so conn.close()
is guaranteed to run even if one of the assertions involving
news_flow.match_entities.fn, daily_flow._fetch_all_manager_ids,
diff_holdings_module._resolve_manager_id, or activism_flow._load_manager_row
fails.

🪄 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: a2350eba-07c6-4e98-b101-fc1baa69bd76

📥 Commits

Reviewing files that changed from the base of the PR and between f6a84c0 and 2ed2cba.

📒 Files selected for processing (6)

• adapters/base.py
• diff_holdings.py
• etl/activism_flow.py
• etl/daily_diff_flow.py
• etl/news_flow.py
• tests/test_etl_flows_additional.py

[stranske]

Closer pushed 823b220 after rebasing #1364 onto current main (including merged #1361) and addressing the review findings.

Changes:

• Added shared resolve_manager_id_column() so API/ETL callers use one manager-id fallback path; non-SQLite/Postgres callers keep the canonical manager_id path without schema-probe side effects, while SQLite resolves id vs manager_id from the live table.
• Switched SQLite column introspection to SELECT name FROM pragma_table_info(?) and added debug logging before returning an empty column set on introspection errors.
• Replaced duplicated inline manager-id fallback logic in diff_holdings, ETL flows, and api.managers; kept the [Audit] ETL hardcodes manager_id but API-created SQLite managers table uses id #1302 SQLite API-created schema regression coverage and made the test close its connection in finally.
• Rebased the branch onto Consolidate duplicated dialect and chain helpers #1361 merge commit 06eb505, resolving the shared-helper overlap.

Validation from disposable checkout /tmp/imi-manager-1364-fix.OLjvU8/Manager-Database:

• python -m pytest tests/test_etl_flows_additional.py::test_etl_manager_id_readers_support_api_created_sqlite_schema -q -> passed.
• python -m pytest tests/test_etl_flows_additional.py tests/test_daily_diff.py tests/test_manager_api.py tests/test_shared_helper_consolidation.py -q -> 64 passed.
• python -m ruff check adapters/base.py api/managers.py diff_holdings.py etl/activism_flow.py etl/daily_diff_flow.py etl/news_flow.py tests/test_etl_flows_additional.py -> passed.
• python -m black --check adapters/base.py api/managers.py diff_holdings.py etl/activism_flow.py etl/daily_diff_flow.py etl/news_flow.py tests/test_etl_flows_additional.py -> passed.
• git diff --check -> passed.

Fresh GitHub checks are expected on the new head.

[stranske]

Follow-up push daf9167 fixes the fresh Gate failure from the prior head.

Root cause:

• The remote dialect portability gate rejects SQLite ? placeholders in audited shared code, including pragma_table_info(?).

Change:

• Switched SQLite column introspection to the table-valued pragma with a named parameter: SELECT name FROM pragma_table_info(:table_name).

Additional validation from /tmp/imi-manager-1364-fix.OLjvU8/Manager-Database:

• python -m pytest tests/test_dialect_portability_gate.py::test_dialect_gate_accepts_current_audited_repo_state tests/test_app_health.py::test_health_app_ok tests/test_etl_flows_additional.py::test_etl_manager_id_readers_support_api_created_sqlite_schema -q -> 3 passed.
• python -m pytest tests/test_etl_flows_additional.py tests/test_daily_diff.py tests/test_manager_api.py tests/test_shared_helper_consolidation.py tests/test_dialect_portability_gate.py -q -> 86 passed.
• Focused Ruff, Black check, and git diff --check -> passed.

Fresh GitHub checks are async on the new head.

[github-actions]

Runner dispatch state for codex on PR #1364. Do not edit.

[agents-workflows-bot]

🤖 Keepalive Loop Status

PR #1364 | Agent: Codex | Iteration 0/12

Current State

Metric	Value
Iteration progress	[----------] 0/12
Action	run (agent-run-failed)
Agent status	❌ AGENT FAILED
Gate	success
Tasks	0/8 complete
Timeout	45 min (default)
Timeout usage	2m elapsed (6%, 43m remaining)
Keepalive	✅ enabled
Autofix	❌ disabled

Last Codex Run

Result	Value
Status	❌ AGENT FAILED
Reason	agent-run-failed
Exit code	0
Failures	1/3 before pause

To retry immediately:

• Add the agent:retry label to this PR

Or wait for the next successful Gate run to automatically retry.

Codex output:

Blocked by the execution environment. Every shell command, including pwd, fails before execution with: text bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted Because of that I cannot safely inspect the repo, edit the correct files, run the required tests/formatters, or create the...

🔍 Failure Classification

| Error type | infrastructure |
| Error category | transient |
| Suggested recovery | Capture logs and context; retry once and escalate if the issue persists. |

🧠 Task Analysis

| Provider | ✅ GitHub Models (primary) |
| Confidence | 40% |

⚠️ Failure Tracking

| Consecutive failures | 1/3 |
| Reason | agent-run-failed |

[agents-workflows-bot]

Keepalive Work Log (click to expand)

#	Time (UTC)	Agent	Action	Result	Files	Tasks	Progress	Commit	Gate
0	2026-07-01 22:05:17	Codex	run (agent-run-failed)	failure	5 file(s)	0	0/8	—	success

[github-actions]

Provider Comparison Report

Provider Summary

Provider	Model	Verdict	Confidence	Summary
openai	gpt-5.4	PASS	94%	The merged changes satisfy the documented acceptance criteria. A shared resolver was added in adapters/base.py via resolve_manager_id_column(), built on existing schema introspection helpers, and a...
anthropic	claude-sonnet-4-6	PASS	88%	The PR correctly implements a shared resolve_manager_id_column() resolver in adapters/base.py and applies it across all four ETL locations identified in issue #1302 (news_flow, daily_diff_flow,...

📋 Full Provider Details (click to expand)

openai

• Model: gpt-5.4
• Verdict: PASS
• Confidence: 94%
• Scores:
  • Correctness: 9.0/10
  • Completeness: 9.0/10
  • Quality: 9.0/10
  • Testing: 9.0/10
  • Risks: 8.0/10
• Summary: The merged changes satisfy the documented acceptance criteria. A shared resolver was added in adapters/base.py via resolve_manager_id_column(), built on existing schema introspection helpers, and api/managers.py was updated to reuse it. The ETL/lookup paths called out in the issue and PR scope were updated to stop hardcoding manager_id and instead use the resolver for SQLite/Postgres compatibility, covering news_flow, daily_diff_flow, activism_flow, and diff_holdings manager lookups. The added regression test in tests/test_etl_flows_additional.py directly addresses the failure mode described in the issue: an API-created SQLite managers table using id now works with the ETL reader path. Code quality is good: the resolver is centralized, duplication is reduced, and get_table_columns() was improved with safer parameter handling and defensive filtering/logging. Risk is low; the main behavioral change is the SQLite fallback to id, which matches the documented API-created schema while preserving Postgres behavior as manager_id.

anthropic

• Model: claude-sonnet-4-6
• Verdict: PASS
• Confidence: 88%
• Scores:
  • Correctness: 9.0/10
  • Completeness: 9.0/10
  • Quality: 9.0/10
  • Testing: 8.0/10
  • Risks: 8.0/10
• Summary: The PR correctly implements a shared resolve_manager_id_column() resolver in adapters/base.py and applies it across all four ETL locations identified in issue [Audit] ETL hardcodes manager_id but API-created SQLite managers table uses id #1302 (news_flow, daily_diff_flow, diff_holdings, activism_flow). The API's _manager_id_column() is simplified to delegate to the same resolver, eliminating duplication. A regression test is added that creates an API-schema SQLite DB and verifies ETL readers work without raising no such column: manager_id. Additional quality improvements (parameterized pragma query, null-safe filtering, debug logging) are included. All acceptance criteria appear satisfied and CI passed.
• Concerns:
  • resolve_manager_id_column() falls back to 'id' when table introspection fails entirely (manager_id_column returns None), which could silently mask schema errors rather than raising them — acceptable given existing API behavior but worth noting
  • Diff was truncated so exact ETL call-site changes in diff_holdings.py, etl/activism_flow.py, etl/daily_diff_flow.py could not be fully verified, though file-level change counts are consistent with the expected single-site fix per file
  • Test coverage for the 'deliberate-break demonstration' (reverting to hardcoded manager_id fails the test) is implicit rather than explicit, but the regression test itself is sufficient per the acceptance criteria

Agreement

• Verdict: PASS (all providers)
• Correctness: scores within 1 point (avg 9.0/10, range 9.0-9.0)
• Completeness: scores within 1 point (avg 9.0/10, range 9.0-9.0)
• Quality: scores within 1 point (avg 9.0/10, range 9.0-9.0)
• Testing: scores within 1 point (avg 8.5/10, range 8.0-9.0)
• Risks: scores within 1 point (avg 8.0/10, range 8.0-8.0)

Disagreement

No major disagreements detected.

Unique Insights

• openai: The merged changes satisfy the documented acceptance criteria. A shared resolver was added in adapters/base.py via resolve_manager_id_column(), built on existing schema introspection helpers, and api/managers.py was updated to reuse it. The ETL/lookup paths called out in the issue and PR scope we...
• anthropic: resolve_manager_id_column() falls back to 'id' when table introspection fails entirely (manager_id_column returns None), which could silently mask schema errors rather than raising them — acceptable given existing API behavior but worth noting; Diff was truncated so exact ETL call-site changes in diff_holdings.py, etl/activism_flow.py, etl/daily_diff_flow.py could not be fully verified, though file-level change counts are consistent with the expected single-site fix per file; Test coverage for the 'deliberate-break demonstration' (reverting to hardcoded manager_id fails the test) is implicit rather than explicit, but the regression test itself is sufficient per the acceptance criteria

🔍 LangSmith Traces

• openai
• anthropic

[github-actions]

Workflow state fingerprint for Agents Verifier. Do not edit.