feat(attribution): position-level returns since disclosure (#1465) - #1503
Conversation
stranske
added
autofix
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 23 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 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 (6)
📝 WalkthroughWalkthroughAdds point-in-time, position-level attribution from disclosure through an as-of date. The change stores position results, aggregates manager returns and hit rates, exposes a read-only API endpoint, and adds migration, schema parity, persistence, and acceptance tests. ChangesPosition Attribution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AttributionWorkflow
participant HoldingsAsOf
participant PriceAdapter
participant Database
participant SignalsAPI
AttributionWorkflow->>HoldingsAsOf: Select holdings visible at disclosure
AttributionWorkflow->>PriceAdapter: Retrieve disclosure and as-of prices
PriceAdapter-->>AttributionWorkflow: Return price data
AttributionWorkflow->>Database: Persist attribution report
SignalsAPI->>Database: Query manager attribution rows
Database-->>SignalsAPI: Return stored position records
SignalsAPI-->>SignalsAPI: Compute manager summary and response
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@alembic/versions/019_manager_attribution.py`:
- Around line 45-51: Fix NULL filing_id idempotency across
alembic/versions/019_manager_attribution.py lines 45-51, schema.sql line 326,
and etl/attribution_flow.py lines 56 and 346-360: retain the composite
UniqueConstraint, add and remove the partial unique index on (manager_id,
security_key, as_of_date) where filing_id IS NULL in the migration, add the
matching runtime SQLite index and schema DDL, and select the appropriate ON
CONFLICT target in the attribution insert based on whether position.filing_id is
present. Add an idempotency test in tests/test_attribution_flow.py covering
repeated runs with NULL filing_id.
- Around line 45-51: Replace the nullable-column UniqueConstraint in the manager
attribution migration with the partial unique-index pattern used by
016_activism_campaigns: retain uniqueness for non-NULL filing_id rows and add a
predicate-based index covering filing_id IS NULL with the appropriate key
columns. Add the matching op.drop_index call in downgrade(), and update
persist_attribution to handle filing-less rows through a delete-then-insert path
or a dedicated conflict target so repeated runs do not accumulate duplicates.
In `@api/signals.py`:
- Line 512: Update the response as_of_date construction in
query_manager_attribution to return a value only when all returned positions
share the same as_of_date; when rows span multiple as-of windows, return None
instead of positions[0].as_of_date. Preserve the caller-provided as_of_date when
it is explicitly supplied.
- Around line 536-541: Remove the duplicate _coerce_date_value helper, update
_to_date to truncate string inputs with [:10] before date.fromisoformat, and
replace the call sites around the existing _coerce_date_value usage with
_to_date. Preserve _to_date’s handling of date and datetime values.
- Around line 469-490: Change get_manager_attribution and the neighboring signal
handlers that perform synchronous database operations from async def to def,
preserving their existing request parameters and response behavior so
connect_db, query_manager_attribution, and related calls do not block the event
loop.
In `@etl/attribution_flow.py`:
- Around line 262-274: Update _aggregate to delegate realized_return and
hit_rate calculation to summarize_manager_attribution instead of recomputing the
metrics locally. Ensure summarize_manager_attribution is defined earlier or
otherwise available at call time, and preserve the existing empty/no-return
handling and report field assignments.
- Around line 241-258: Update the price validation in the attribution flow
around close_on_or_before and position_return to reject non-finite disclosure
and as-of prices by reusing the imported finite_float_or_none helper. Preserve
the existing skipped-position behavior and missing_price reason for invalid
values, and add a test covering a NaN close that asserts the position is
skipped.
- Around line 188-196: Update the visible-holdings filtering logic in the
function containing _filing_id_for_disclosure and visible_holdings so that when
filing_id is identified but period_rows is empty, it returns an empty list
rather than the full visible set. Preserve the existing full visible-set
fallback only when filing_id is None, and avoid attributing rows from other
filings to the identified filing_id.
- Around line 304-318: Update the deduplication in the disclosure-processing
loop around _holdings_for_disclosure to key only by security key, preserving the
existing ascending dates order so the first disclosure is retained and later
occurrences are skipped. Add a test covering two disclosure dates exposing the
same security and assert the generated report contains exactly one row for that
security.
In `@schema.sql`:
- Around line 319-322: Update the declarations for disclosure_price,
as_of_price, position_return, and value_usd in schema.sql from real to double
precision so they match the FLOAT types emitted by migration
019_manager_attribution.py. Preserve the existing column names and ordering, and
do not switch value_usd to numeric unless the migration and SQLite runtime DDL
are updated consistently.
In `@tests/test_attribution_flow.py`:
- Around line 100-127: Add persistence coverage alongside
test_two_positions_yield_hand_computed_returns by running run_attribution twice
on the same connection with default persistence and asserting
query_manager_attribution still returns exactly two rows. Add skip-path tests
for positions without resolved_ticker and tickers without prices, asserting
skip_reason is respectively "unresolved_ticker" and "missing_price", and verify
skipped positions do not affect realized_return or hit_rate.
In `@tests/test_schema.py`:
- Around line 297-329: Extract the duplicated schema-parity logic shared with
test_backtest_schema_contract_stays_in_sync_across_all_three_definitions into a
helper accepting the table set and runtime setup callable, then update both
tests to use it. In that helper, collect each table’s column name and declared
type from PRAGMA table_info rows 1 and 2, and compare the resulting contracts
across migrated, runtime, and schema.sql definitions.
🪄 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: 43c8fa31-768d-4ac0-9f6d-ec0d3bbda687
📒 Files selected for processing (6)
alembic/versions/019_manager_attribution.pyapi/signals.pyetl/attribution_flow.pyschema.sqltests/test_attribution_flow.pytests/test_schema.py
Resolves all 11 unresolved review threads. Data integrity - NULL filing_id escaped the four-column UNIQUE constraint, so every run re-inserted filing-less rows and drifted the manager aggregates. Adds a partial unique index on (manager_id, security_key, as_of_date) WHERE filing_id IS NULL across migration 019, schema.sql, and the runtime SQLite DDL, and selects the matching ON CONFLICT target per row. - _holdings_for_disclosure no longer falls back to the full visible set for an identified filing with no rows; that persisted a filing_id the holdings did not belong to and surfaced it through the API. - schema.sql declared prices as `real` (4-byte) while migration 019 used sa.Float (double precision). Both now use double precision. Correctness and stability - Prices go through finite_float_or_none: NaN passed the `<= 0` guard and poisoned position_return, the aggregates, and JSON serialization. - Dedupe by security alone, not (filing, security), so a security re-disclosed in a later filing is attributed once rather than double-counted. - The response as_of_date is None when rows span multiple windows instead of reporting the first row's window as if it covered all of them. Maintainability - _aggregate delegates to summarize_manager_attribution so one definition drives both the stored report and the API. - Drops _coerce_date_value in favour of _to_date, which gains the [:10] slice. - get_manager_attribution is sync so its blocking DB work leaves the event loop. - Extracts the schema-parity helper and pins declared SQLite types for manager_attribution. Tests: persistence idempotency (with and without filing_id), skip paths, non-finite price, re-disclosure dedupe, and empty identified filings.
Add manager_attribution over holdings_as_of + the price adapter so each
disclosed position gets a buy-and-hold forward return, with manager-level
realized return and hit-rate aggregates. Expose derived returns via
/api/signals/attribution/{manager_id}. Migration 019 + schema.sql +
runtime SQLite DDL stay in sync; deliberate-break proves starting the
return window before disclosure corrupts the hand-computed path.
Co-authored-by: Cursor <cursoragent@cursor.com>
Resolves all 11 unresolved review threads. Data integrity - NULL filing_id escaped the four-column UNIQUE constraint, so every run re-inserted filing-less rows and drifted the manager aggregates. Adds a partial unique index on (manager_id, security_key, as_of_date) WHERE filing_id IS NULL across migration 019, schema.sql, and the runtime SQLite DDL, and selects the matching ON CONFLICT target per row. - _holdings_for_disclosure no longer falls back to the full visible set for an identified filing with no rows; that persisted a filing_id the holdings did not belong to and surfaced it through the API. - schema.sql declared prices as `real` (4-byte) while migration 019 used sa.Float (double precision). Both now use double precision. Correctness and stability - Prices go through finite_float_or_none: NaN passed the `<= 0` guard and poisoned position_return, the aggregates, and JSON serialization. - Dedupe by security alone, not (filing, security), so a security re-disclosed in a later filing is attributed once rather than double-counted. - The response as_of_date is None when rows span multiple windows instead of reporting the first row's window as if it covered all of them. Maintainability - _aggregate delegates to summarize_manager_attribution so one definition drives both the stored report and the API. - Drops _coerce_date_value in favour of _to_date, which gains the [:10] slice. - get_manager_attribution is sync so its blocking DB work leaves the event loop. - Extracts the schema-parity helper and pins declared SQLite types for manager_attribution. Tests: persistence idempotency (with and without filing_id), skip paths, non-finite price, re-disclosure dedupe, and empty identified filings.
#1504 merged `019_activism_campaign_returns` to main while this branch also used revision 019, so the merged tree had two heads and every alembic `upgrade("head")` call failed with: alembic.util.exc.CommandError: Multiple head revisions are present for given argument 'head' Renumbers this branch's migration to 020 with down_revision 019 and rebases onto current main. `ScriptDirectory.get_heads()` now returns ['020'].
5246301 to
525eb44
Compare
|
Closer update — rebased onto current Why: #1504 merged Fix: Validation at All 11 CodeRabbit threads from the 17:45Z review were answered individually and resolved in the prior push. |
Provider Comparison ReportProvider Summary
📋 Full Provider Details (click to expand)openai
anthropic
Agreement
Disagreement
Unique Insights
🔍 LangSmith Traces |
|
Workflow state fingerprint for Agents Verifier. Do not edit. |
|
Closer verifier disposition — anthropic openai's concern: the nullable- Accurate as a reading of the index, but it is the intended grain rather than a defect. # Deduplicate by security alone so a security is attributed once for this as-of
# window. ``dates`` is ascending, so the earliest disclosure wins; keying by
# (filing, security) would attribute the same security once per filing that
# re-discloses it and double-count it in the manager aggregates.That dedupe was added deliberately in this PR's review round: keying storage per disclosure period would let a security that is re-disclosed across several filings enter the manager-level realized-return and hit-rate aggregates once per filing, weighting securities by disclosure frequency instead of by position. Because the write path can never produce two rows for one Adding Recording rather than silently accepting the residual tension: issue #1465's Tasks section phrases the computation as "per (manager, security, disclosure period)", while the shipped aggregate is per security per as-of window. The three acceptance criteria — hand-computed two-position returns, no look-ahead, and the deliberate-break on a pre-disclosure entry offset — are all satisfied by the shipped behaviour, so this is a wording mismatch and not an unmet criterion. If manager aggregates are ever meant to weight by disclosure frequency, that is a follow-on schema-grain change, not a fix to this PR. anthropic's concerns are review-context limitations rather than named defects: the Source #1465 stays open until #1507 merges and |
1 participant
Summary
Implements #1465 / design #1402: position-level performance attribution as a view over
holdings_as_of+ the existing free-source price adapter (no parallel price/as-of system).What landed
etl/attribution_flow.py: per (manager, security, disclosure period), buy-and-hold return from disclosure date to as-of; manager realized return + hit-rate aggregates; persistsmanager_attribution.019+schema.sql+ runtime SQLite DDL kept in sync.GET /api/signals/attribution/{manager_id}(derived returns only; raw prices not redistributed).Test gate
tests/test_attribution_flow.py:entry_date_offset_days=-1fails the hand-computed assertion; restoring offset 0 returns greenAlso: migration autoincrement + three-way schema contract tests for
manager_attribution.Validation
Closes #1465
Summary by CodeRabbit
New Features
Tests