Repository navigation
feat(finance): Ohio aggregators — direct contributions + 31-U outside spending (PR 6) - #542
Merged
Merged
Conversation
… aggregators Direct aggregator is a streaming accumulator (decision 10): the caller streams each CAC_CON file once and feeds every open accumulator. Size buckets are the only direct breakdown (decision 6 — no occupations); the SHORT_DESCRIPTION vocabulary is fail-closed (31-A / 31-E / 31-J-1 donor support, 31-A-2 receipts-only, unknown values counted). Summary receipts and disbursements come from the cover pages, cash from the latest filed report with negative balances written as NULL (decision 1) — on the real files cover satisfies receipts − disbursements + forward = cash to the cent, while itemized-only receipts would miss a $25.4M non-itemized transfer; the itemized sum stays as a diagnostic. Outside aggregator implements the two-stage 31-U model (decision 4): spender identity pinned from the annual (MASTER_KEY, REPORT_KEY) pair across all six expenditure files (candidate and party committees file 31-U too), every report gated by annual-vs-detail reconciliation plus the cover-page IE leg when a cover row exists (14 of 28 cycle report keys have none — structural, never a mismatch). Target matching per revised decision 5: strict unique name match via the resolver's shared helpers, office confirming-only but rejecting a contradicting stated office; unmatched/ambiguous targets quarantine with amounts. Real-data smoke reproduced the spike exactly: 13/13 bundle reports reconciled, Acton oppose $8,211,114.50, Ramaswamy support $600,000, Stephens oppose $217,704.16. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds Ohio direct-contribution and Form 31-U outside-spending aggregators. It adds shared candidate and office matching helpers, exports the new modules, adds comprehensive tests, and marks the implementation complete in the project plan. ChangesOhio finance aggregation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AnnualRows
participant aggregateOhioOutsideSpending
participant DetailReports
participant CoverPages
participant CandidateTargets
AnnualRows->>aggregateOhioOutsideSpending: provide annual expenditure rows
aggregateOhioOutsideSpending->>DetailReports: resolve report keys and process detail rows
aggregateOhioOutsideSpending->>CoverPages: reconcile independent-expenditure totals
aggregateOhioOutsideSpending->>CandidateTargets: match normalized names and office tokens
aggregateOhioOutsideSpending-->>AnnualRows: return candidate totals and diagnostics
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…gregators Replace the raw NUL separator bytes with the textual escape sequence (backslash-u0000) so Git stops classifying the two aggregator files as binary (behavior identical). Fully blank cover rows (every money column empty - 3 of 1,673 cycle rows on the real file, e-filing damage) are now excluded from the cover sums, the latest-report pick, and coverReportCount, and surfaced as blankCoverRowCount: a blank latest filing no longer nulls a real cash balance, and a committee with only blank rows falls back to itemized receipts instead of publishing fabricated zeroes. Partially blank rows keep their ?? 0 treatment deliberately - all 583 blank-contribution rows on the real file satisfy TOTAL_FUNDS = AMT_FORWARD + TOTAL_OTHER_INCOME exactly, so a blank cell on an otherwise-filled row provably means zero. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review 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: 1
🧹 Nitpick comments (3)
ohio_plan.md (1)
390-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the blank-cover-row rule in the PR 6 entry.
The entry states that receipts and disbursements come from cover sums and cash from the latest report. It omits the fully blank cover row exclusion that the implementation adds. In
ohioDirectContributionAggregator.tslines 64-69 and 310-311, a cover row with every money column null is excluded from the sums, from the latest-report pick, and fromcoverReportCount, and a committee with only blank rows falls back to itemized receipts with NULL disbursements and cash. That rule changes published output, so the plan should carry it.📝 Proposed plan addition
cover satisfies receipts − disbursements + forward = cash to the cent, while itemized-only receipts would miss Ramaswamy's $25.4M non-itemized - federal transfer (itemized sum kept as a diagnostic); 31-U spans all six + federal transfer (itemized sum kept as a diagnostic); cover rows with + every money column blank (3 of 1,673 on the 2026-cycle file, e-filing + damage) are excluded from the sums, the latest-report pick, and + `coverReportCount` and counted in `blankCoverRowCount`, so a + only-blank committee falls back to itemized receipts with NULL + disbursements/cash instead of fabricated zeroes; 31-U spans all six expenditure files (candidate and party committees file it too — 28🤖 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 `@ohio_plan.md` around lines 390 - 399, Update the PR 6 entry in ohio_plan.md to document that fully blank cover rows, with all money columns null, are excluded from cover sums, latest-report selection, and coverReportCount. Also record that committees containing only blank cover rows fall back to itemized receipts with NULL disbursements and cash.backend/tests/pipeline/ohioFinance/ohioDirectContributionAggregator.test.ts (1)
141-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
skippedContributionRowCountfor the description-classification paths.
addincrementsskippedContributionRowCountfor other-income rows and for unknown-description rows. No test asserts that behavior, so a change to the skip accounting would pass. Add the assertions to lock the contract that the diagnostics consumer depends on.💚 Proposed test additions
expect(result.otherIncomeRowCount).toBe(1); expect(result.unknownShortDescriptionRowCount).toBe(0); + expect(result.includedContributionRowCount).toBe(1); + expect(result.skippedContributionRowCount).toBe(1); expect(result.directBreakdowns).toHaveLength(1);expect(result.unknownShortDescriptionRowCount).toBe(1); + expect(result.includedContributionRowCount).toBe(0); + expect(result.skippedContributionRowCount).toBe(1); expect(result.directBreakdowns).toHaveLength(0);🤖 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/pipeline/ohioFinance/ohioDirectContributionAggregator.test.ts` around lines 141 - 164, Add assertions in both tests around createOhioDirectContributionAccumulator to verify skippedContributionRowCount is 1 for the other-income row and 1 for the unknown SHORT_DESCRIPTION row, while preserving the existing receipt, direct-total, bucket, and diagnostic assertions.backend/src/pipeline/ohioFinance/ohioDirectContributionAggregator.ts (1)
337-341: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the breakdown tie-break ordering deterministic.
localeComparewithout an explicit locale resolves against the runtime default locale and ICU data. If two buckets carry equalamountCents, the emitted order can differ between environments. The test at lines 108-130 asserts the full array, so a future tie would make ordering environment-dependent. Use a plain code-unit comparison, because the bucket labels are a fixed ASCII vocabulary.♻️ Proposed refactor
.sort( (left, right) => - right.amountCents - left.amountCents || left.categoryName.localeCompare(right.categoryName) + right.amountCents - left.amountCents || + (left.categoryName < right.categoryName ? -1 : left.categoryName > right.categoryName ? 1 : 0) )🤖 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/src/pipeline/ohioFinance/ohioDirectContributionAggregator.ts` around lines 337 - 341, Update the tie-break comparison in the directBreakdowns sort within the Ohio finance aggregator to use deterministic plain code-unit ordering for categoryName instead of localeCompare. Preserve descending amountCents ordering and the existing category-name tie-break behavior for the fixed ASCII vocabulary.
🤖 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 `@backend/src/pipeline/ohioFinance/ohioDirectContributionAggregator.ts`:
- Around line 52-62: Update the “Skip reasons” documentation near the aggregator
counters in
backend/src/pipeline/ohioFinance/ohioDirectContributionAggregator.ts:52-62 to
state that all five counters partition skippedContributionRowCount. In
backend/tests/pipeline/ohioFinance/ohioDirectContributionAggregator.test.ts:141-164,
add skippedContributionRowCount and matching includedContributionRowCount
assertions to both the other-income and unknown-description tests.
---
Nitpick comments:
In `@backend/src/pipeline/ohioFinance/ohioDirectContributionAggregator.ts`:
- Around line 337-341: Update the tie-break comparison in the directBreakdowns
sort within the Ohio finance aggregator to use deterministic plain code-unit
ordering for categoryName instead of localeCompare. Preserve descending
amountCents ordering and the existing category-name tie-break behavior for the
fixed ASCII vocabulary.
In `@backend/tests/pipeline/ohioFinance/ohioDirectContributionAggregator.test.ts`:
- Around line 141-164: Add assertions in both tests around
createOhioDirectContributionAccumulator to verify skippedContributionRowCount is
1 for the other-income row and 1 for the unknown SHORT_DESCRIPTION row, while
preserving the existing receipt, direct-total, bucket, and diagnostic
assertions.
In `@ohio_plan.md`:
- Around line 390-399: Update the PR 6 entry in ohio_plan.md to document that
fully blank cover rows, with all money columns null, are excluded from cover
sums, latest-report selection, and coverReportCount. Also record that committees
containing only blank cover rows fall back to itemized receipts with NULL
disbursements and cash.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: d15b69c0-cff8-437a-b930-3caba871bcd0
📒 Files selected for processing (7)
backend/src/pipeline/ohioFinance/index.tsbackend/src/pipeline/ohioFinance/ohioCandidateCommitteeResolver.tsbackend/src/pipeline/ohioFinance/ohioDirectContributionAggregator.tsbackend/src/pipeline/ohioFinance/ohioOutsideSpendingAggregator.tsbackend/tests/pipeline/ohioFinance/ohioDirectContributionAggregator.test.tsbackend/tests/pipeline/ohioFinance/ohioOutsideSpendingAggregator.test.tsohio_plan.md
…regator The result-type comment now states that matched = included + skipped and that the five reason counters partition skippedContributionRowCount, with the two description-classified reasons still counting toward itemized receipts. Tests assert the partition for the other-income and unknown-description cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
shu1513
added a commit
that referenced
this pull request
Aug 6, 2026
…rgets, fail-closed reader, cover-IE zero Four fixes from the Codex/CodeRabbit review of PR #545: - A 31-U detail bundle missing report keys the annual files discovered now fails the year closed to outside-unavailable instead of publishing partial totals and zeros. The real cache's spike bundle misses 15 keys carrying ~$5.9M of annual 31-U money, so this was production-relevant. - The outside target dedupe keys on candidateId + office, not display name: one person due for primary and general still shares a target, while two different people with the same name stay separate and the aggregator quarantines that name as ambiguous instead of paying both. - The version-1 bundle reader validates every field the aggregator consumes (direction pinned to support/oppose/null, integer cents, matching report keys) instead of casting - a bogus direction would have landed in the aggregator's oppose branch. - Cover-IE gate (leftover from the #542 review): a blank VALUE_IND_EXPENDITURES on an otherwise-filled cover row now counts as zero and feeds the third reconciliation leg; fully-blank rows stay ignored like missing ones (shared isBlankOhioSosCoverPageRow). A probe of all 28 cycle report keys found 13 exact IE matches, 15 no-cover-row, and 0 blank-on-filled, so this is added protection with no behavior change on the real files. Also: direct contribution rows route to their accumulator by MASTER_KEY instead of the rows x committees scan, and the outside catch warning no longer claims "artifacts unavailable" for what may be an aggregation failure. Declined from review: streaming JSON for the bundle (9.8 KB file - the 305 MB figure is the whole cache), shared CLI flag parser, and dropping force from the recurring scheduler payload (both verbatim the maryland/all-states template; repo-wide questions). Verified: typecheck green, 6,299 tests pass (10 new), and the real-data dry run now reports outside unavailable on the stale spike bundle with direct totals unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Ohio campaign-finance PR 6 of 9 (
ohio_plan.md). Capability-matrix row: standard state-finance tables, std direct categories (contribution_size only), donor/industry outside breakdowns.What
ohioDirectContributionAggregator.ts— streaming accumulator (decision 10: the ~90 MB CAC_CON files are never materialized; the caller streams each file once and feeds every open accumulator). Contribution-size buckets only (decision 6 — Ohio ships without occupations). Fail-closedSHORT_DESCRIPTIONvocabulary:31-A Stmt of Contribution/31-E FR Contributions/31-J-1 In-Kind Cont Rcvdare donor support;31-A-2 Other Incomecounts toward receipts only; anything new lands inunknownShortDescriptionRowCountinstead of a bucket.ohioOutsideSpendingAggregator.ts— two-stage Form 31-U (decision 4). Stage one pins each report's spender from the annual(MASTER_KEY, REPORT_KEY)pair — the detail export has no MASTER_KEY — across all six expenditure files (candidate and party committees file 31-U too, not just PACs). Stage two aggregates detail rows behind a per-report reconciliation gate: detail total must equal the annual total, and the cover page'sVALUE_IND_EXPENDITURESmust agree when a cover row exists. Target matching per revised decision 5: strict unique name match (shared resolver helpers, incl. the middle/suffix conflict guard), office as a confirming filter that also rejects a contradicting stated office; unmatched/ambiguous targets quarantine with dollar amounts. No dedup of identical rows (decision 3).ohioPersonNamesMatch,ohioSosOfficeTokenForOfficeName) so both matchers accept and reject exactly the same pairs.Design decisions grounded in the real 2026-cycle files
AMT_FORWARD= previousBALANCE_ON_HAND, zero duplicate report keys, zero AMENDED rows) and satisfy receipts − disbursements + forward = cash to the cent for all sampled committees. Itemized-only receipts would miss Ramaswamy's $25.4M non-itemized federal-account transfer ($30.3M itemized vs $55.1M actual). The itemized sum is kept as a reconciliation diagnostic. Negative balance → NULL + flag (decision 1).VALUE_IND_EXPENDITURESmatched the annual total exactly in all 14 cases.Verification
npm run typecheck+npm testgreen inbackend/(6,245 passed).JON HUSTED(federal race) correctly quarantined as unmatched; the 15 report keys the spike bundle predates are surfaced inmissingDetailReportKeys.Next: PR 7 sync + batchSync + npm scripts.
🤖 Generated with Claude Code
Summary by CodeRabbit