Reconcile amended 13F filings before diffing

[stranske]

Closes #1324

Summary

• select one authoritative filing per manager reporting period before holdings diffs
• prefer amended 13F filings while preserving raw original/amended filings for provenance
• add regression coverage, including a deliberate-break selector that reproduces the old double-count behavior

Validation

• PYTHONPYCACHEPREFIX=/tmp/pycache-manager-1324 python -m pytest tests/test_diff_holdings.py -q
• python -m ruff check diff_holdings.py tests/test_diff_holdings.py
• python -m black --target-version py312 --check diff_holdings.py tests/test_diff_holdings.py
• PYTHONPYCACHEPREFIX=/tmp/pycache-manager-1324-mypy python -m mypy diff_holdings.py
• git diff --check

Summary by CodeRabbit

• Bug Fixes

  • Improved holdings comparison to use the latest authoritative filing for each period, including amended filings, before calculating changes.
  • Updated period selection to handle either reported period end dates or filing dates when needed.
  • Fixed comparison behavior so the latest and prior holdings are chosen more accurately for each manager.

• Tests

  • Added regression coverage for amendment-aware holdings diffs.
  • Strengthened database/query test coverage for more realistic SQL execution behavior.

[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: 42 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: 4c4d3ba5-8df5-435e-bef2-0b5e58409aff

📥 Commits

Reviewing files that changed from the base of the PR and between 1b89d8c and 595f212.

📒 Files selected for processing (2)

• diff_holdings.py
• tests/test_diff_holdings.py

📝 Walkthrough

Walkthrough

diff_holdings.py now selects authoritative filings per period, preferring 13F-HR/A amendments over originals, using period_end or filed_date as the period key. Holdings fetching is refactored to per-filing lookups. Tests are extended to cover amendment reconciliation and updated placeholder handling.

Changes

13F Amendment Reconciliation

Layer / File(s)	Summary
Authoritative filing selection diff_holdings.py	Adds _is_amendment, _sort_key, and _select_authoritative_filings to group filings by period (using period_end or filed_date) and prefer amendments over originals, replacing the prior latest-by-filed_date logic.
Holdings fetch and diff wiring diff_holdings.py	Adds _fetch_holdings_for_filing for per-filing CUSIP-keyed holdings retrieval, and rewrites _fetch_latest_sets to build current/prior sets from the new selection and fetch helpers.
Tests for reconciliation and SQL placeholders tests/test_diff_holdings.py	Adds a regression test verifying amendments are reconciled before diffing, and reworks the Postgres placeholder test with fakes tracking all execute() calls across varying queries.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DiffHoldings as diff_holdings()
  participant Selector as _select_authoritative_filings
  participant Fetcher as _fetch_holdings_for_filing
  participant DB as Database

  DiffHoldings->>Selector: request current/prior filing ids
  Selector->>DB: query filings (period_end or filed_date)
  DB-->>Selector: filing rows
  Selector->>Selector: group by period, prefer /A amendments
  Selector-->>DiffHoldings: current_id, prior_id
  DiffHoldings->>Fetcher: fetch holdings(current_id)
  Fetcher->>DB: query holdings for filing_id
  DB-->>Fetcher: holdings rows
  Fetcher-->>DiffHoldings: current holdings by CUSIP
  DiffHoldings->>Fetcher: fetch holdings(prior_id)
  Fetcher->>DB: query holdings for filing_id
  DB-->>Fetcher: holdings rows
  Fetcher-->>DiffHoldings: prior holdings by CUSIP
  DiffHoldings->>DiffHoldings: compute INCREASE/EXIT/ADD/DECREASE deltas

Loading

Related issues: #1324

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The diff logic and tests cover amendment reconciliation, but the PR doesn't show the ingest/alert path or deliberate-break demo required by #1324.	Add reconciliation in the ingest/alert path, preserve superseded filings explicitly, and include a break-test proving double-counting returns when supersede logic is disabled.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The changes stay focused on amendment reconciliation and test updates; no unrelated features or broad refactors are evident.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: reconciling amended 13F filings before diffing.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/issue-1324-13f-amendment-reconciliation

---

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

[Copilot]

Pull request overview

This PR adds an explicit “authoritative filing selection” stage to diff_holdings so that (for a given manager and reporting period) amended 13F filings supersede originals before holdings are diffed, with regression coverage that demonstrates both the fixed behavior and the prior double-count behavior when reconciliation is disabled.

Changes:

• Add _select_authoritative_filings() to reconcile 13F amendments and pick one filing per reporting period before diffing.
• Refactor _fetch_latest_sets() to fetch holdings for the latest two authoritative periods (instead of “latest two filing dates from holdings join”).
• Add regression tests covering amendment reconciliation and tighten the Postgres-placeholder test to account for multiple DB calls.

Reviewed changes

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

File	Description
diff_holdings.py	Introduces authoritative-per-period filing selection (preferring amendments) and refactors latest-set fetching to use the reconciled filing IDs.
tests/test_diff_holdings.py	Adds regression coverage for amendment reconciliation (including a deliberate “broken selector” path) and updates placeholder assertions for the refactor.

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@diff_holdings.py`:
- Around line 48-51: The period selection in the holdings grouping loop is
mixing `period_key` and `filed_date`, which can split related filings into
different groups when `period_end` is missing on one row. Update the logic in
the cursor iteration to use a single canonical reporting-period value
consistently (for example in `diff_holdings`), or ignore rows that do not have a
valid reporting period, so amendments and originals stay grouped together.

🪄 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: e514fa4b-f954-444a-a282-022c60e1265e

📥 Commits

Reviewing files that changed from the base of the PR and between 28cb852 and 1b89d8c.

📒 Files selected for processing (2)

• diff_holdings.py
• tests/test_diff_holdings.py

[stranske]

Closer review fix pushed in b981834.

Addressed the CodeRabbit period-grouping comment by making _select_authoritative_filings() use a single reporting-period key consistently: when the schema has period_end, rows missing that reporting period are ignored instead of falling back to filed_date and splitting originals/amendments into separate groups. Schemas without period_end continue to use filed_date as the period key.

Validation:

• PYTHONPYCACHEPREFIX=/tmp/pycache-manager-1381-reviewfix python -m pytest tests/test_diff_holdings.py -q -> 11 passed
• python -m ruff check diff_holdings.py tests/test_diff_holdings.py -> passed
• python -m black --target-version py312 --check diff_holdings.py tests/test_diff_holdings.py -> passed
• PYTHONPYCACHEPREFIX=/tmp/pycache-manager-1381-reviewfix-mypy python -m mypy diff_holdings.py -> passed
• git diff --check -> passed

[stranske]

Additional closer review fix pushed in 595f212.

Covered the remaining unresolved review threads:

• filings now must have at least one holdings row before they can be selected for diffing
• within a reporting period, selector ranking now uses filed_date first, then amendment status as a tie-breaker, then filing id
• rows missing period_end are ignored when the schema has a reporting-period column, avoiding mixed period_end/filed_date grouping

Validation after the full patch:

• PYTHONPYCACHEPREFIX=/tmp/pycache-manager-1381-reviewfix python -m pytest tests/test_diff_holdings.py -q -> 13 passed
• python -m ruff check diff_holdings.py tests/test_diff_holdings.py -> passed
• python -m black --target-version py312 --check diff_holdings.py tests/test_diff_holdings.py -> passed
• PYTHONPYCACHEPREFIX=/tmp/pycache-manager-1381-reviewfix-mypy python -m mypy diff_holdings.py -> passed
• git diff --check -> passed

[github-actions]

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

[agents-workflows-bot]

🤖 Keepalive Loop Status

PR #1381 | 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	3m elapsed (7%, 42m 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 before any repo command can run. Every shell invocation fails with: text bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted This happens even for true in /tmp, so it is not a project/test failure. Because of that I could not inspect files, ap...

🔍 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 |

[stranske-keepalive]

Keepalive Work Log (click to expand)

#	Time (UTC)	Agent	Action	Result	Files	Tasks	Progress	Commit	Gate
0	2026-07-03 10:05:03	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	87%	The merged changes appear to implement the core requirement: before diffing, the code now selects one authoritative filing per manager/reporting period, preferring the latest amended 13F filing whe...
anthropic	claude-sonnet-4-6	PASS	78%	The implementation correctly adds a reconciliation stage that selects authoritative filings per manager-period, preferring 13F-HR/A amendments over originals while preserving raw filings for proven...

📋 Full Provider Details (click to expand)

openai

• Model: gpt-5.4
• Verdict: PASS
• Confidence: 87%
• Scores:
  • Correctness: 8.0/10
  • Completeness: 9.0/10
  • Quality: 8.0/10
  • Testing: 9.0/10
  • Risks: 7.0/10
• Summary: The merged changes appear to implement the core requirement: before diffing, the code now selects one authoritative filing per manager/reporting period, preferring the latest amended 13F filing when present and otherwise the latest original, while leaving raw filings/holdings intact for provenance. The tests were substantially expanded and, based on the described additions, cover amended-vs-original reconciliation plus a deliberate-break path reproducing the prior double-count behavior. Code quality is solid and the approach is maintainable, though it does not persist an explicit superseded marker and uses somewhat format-sensitive ordering logic. Overall, the acceptance criteria for reconciliation and regression coverage are met.
• Concerns:
  • Authoritative filing selection is implemented inside diffing logic rather than by explicitly marking superseded filings in storage, so provenance is preserved implicitly by leaving raw rows queryable but there is no durable superseded flag/state.
  • Tie-breaking relies on string sorting of period/filed_date values and then amendment preference; this is probably fine for ISO-formatted dates but could be brittle if stored formats vary.
  • The reconciliation scope appears targeted to holdings diff behavior; if other alert paths do not reuse this selector, the broader 'daily diff and large-delta alerts read the reconciled set' requirement depends on existing call paths outside the shown diff.

anthropic

• Model: claude-sonnet-4-6
• Verdict: PASS
• Confidence: 78%
• Scores:
  • Correctness: 8.0/10
  • Completeness: 8.0/10
  • Quality: 8.0/10
  • Testing: 8.0/10
  • Risks: 7.0/10
• Summary: The implementation correctly adds a reconciliation stage that selects authoritative filings per manager-period, preferring 13F-HR/A amendments over originals while preserving raw filings for provenance. The ranking logic is sound: later filed_date > amendment status > higher filing_id. The refactoring cleanly separates filing selection from holdings fetching. Tests were substantially expanded (+244 lines). The main concerns are the truncated diff preventing full verification of test scenarios, and minor edge cases around NULL periods and date format assumptions. Overall the implementation fulfills the stated acceptance criteria.
• Concerns:
  • Full diff was truncated - cannot verify complete test implementations for all three acceptance criteria scenarios (amendment supersedes, double-count regression, provenance preservation)
  • New implementation fetches ALL filings for a manager before selecting top 2 periods, removing the early-exit optimization; minor performance regression at scale
  • Filings with NULL period_key are silently skipped - could drop valid filings if period_end and filed_date are both NULL
  • Dependency on get_table_columns from adapters.base cannot be fully verified from the diff alone
  • The ranking by (filed_date, is_amendment, filing_id) assumes filed_date strings are lexicographically sortable as dates - could fail with inconsistent date formats

Agreement

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

Disagreement

No major disagreements detected.

Unique Insights

• openai: Authoritative filing selection is implemented inside diffing logic rather than by explicitly marking superseded filings in storage, so provenance is preserved implicitly by leaving raw rows queryable but there is no durable superseded flag/state.; Tie-breaking relies on string sorting of period/filed_date values and then amendment preference; this is probably fine for ISO-formatted dates but could be brittle if stored formats vary.; The reconciliation scope appears targeted to holdings diff behavior; if other alert paths do not reuse this selector, the broader 'daily diff and large-delta alerts read the reconciled set' requirement depends on existing call paths outside the shown diff.
• anthropic: Full diff was truncated - cannot verify complete test implementations for all three acceptance criteria scenarios (amendment supersedes, double-count regression, provenance preservation); New implementation fetches ALL filings for a manager before selecting top 2 periods, removing the early-exit optimization; minor performance regression at scale; Filings with NULL period_key are silently skipped - could drop valid filings if period_end and filed_date are both NULL; Dependency on get_table_columns from adapters.base cannot be fully verified from the diff alone; The ranking by (filed_date, is_amendment, filing_id) assumes filed_date strings are lexicographically sortable as dates - could fail with inconsistent date formats

🔍 LangSmith Traces

• openai
• anthropic

[github-actions]

Workflow state fingerprint for Agents Verifier. Do not edit.