Repository navigation
feat(finance): Ohio SoS artifact cache, bulk parsers, and acquisition script (PR 4) - #538
Conversation
…tion script PR 4 of the Ohio campaign-finance sequence (ohio_plan.md). Adds the read path for the Ohio SoS bulk exports plus the attended acquisition step that fills the cache. No database writes and no schema change; capability matrix row unchanged (Ohio stays on the shared factories with zero new factory capability). - ohioSosCsv.ts: chunk-feedable CSV core for the portal's quirks (decision 10) — Windows-1252 bytes, CR-only row separators, quoted currency amounts, header whitespace/underscore drift, HTML entities, missing-final-separator treated as truncation, malformed rows skipped and counted on the big files. - ohioSosBulkFiles.ts: 11 pinned family schemas with typed row mappers, a streaming reader (the ~90 MB contribution files are never read whole), and manifest stats including 31-U report-key discovery. - ohioSos31uDetail.ts: stage two of the two-stage independent-expenditure model (decision 4). Parses both the exported CSV and the scraped page table, keeps direction fail-closed (decision 3), and reconciles detail against annual totals without ever summing the two. - ohioSosArtifactCache.ts: SHA-256, manifests per decision 11, validate-then- atomically-install, and per-cycle artifact status. - ohioSosChromeClient.ts + ohioSosArtifactAcquisition.ts + refreshOhioSosCampaignFinanceRawData.ts: label-to-ID discovery on CFDISCLOSURE:73, strictly sequential paced downloads from :72 (the portal 429s on rapid requests), and page-48 31-U detail scrapes. The portal refuses scripted HTTP, headless Chrome, and fresh-profile automated Chrome alike, so the script attaches to a Chrome the user started with their own profile over the DevTools protocol (Node's built-in WebSocket, no new dependency). It solves no challenges and spoofs nothing. Validated against all 17 real cycle-2026 files (305 MB): every file parses with 0 malformed rows, and the 31-U reconciliation reproduces the acquisition spike exactly — 13 report keys, 43 rows, $9,800,170.34 total, 0 mismatches, $9,401,220.34 directional, 8 blank-direction rows excluding $398,950. Three data findings folded into the plan and the code: filer year typos (0202, 3036, 5025) are common enough that manifest date ranges now ignore implausible dates and count them instead; blank AMOUNT is real (11 rows) and is parsed as null, never zero; and the scraped 31-U table renders empty cells as a bare "-" where the CSV export leaves them blank. Also carries the acquisition spike's committed output: real-byte parser fixtures and the gitignore rule keeping the 305 MB cache out of the repo. backend: npm run typecheck clean, npm test 6180 passed / 24 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 34 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 (9)
📝 WalkthroughWalkthroughChangesOhio finance pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant RefreshScript
participant OhioSosChromeSession
participant OhioSosPortal
participant OhioSosArtifactCache
participant OhioSos31uDetail
Operator->>RefreshScript: run raw refresh command
RefreshScript->>OhioSosChromeSession: connect to attended Chrome
OhioSosChromeSession->>OhioSosPortal: list and download cycle artifacts
OhioSosArtifactCache-->>RefreshScript: return manifests and cache status
RefreshScript->>OhioSos31uDetail: parse and reconcile 31-U details
OhioSos31uDetail-->>RefreshScript: return detail rows and reconciliation results
RefreshScript-->>Operator: emit structured JSON result
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
backend/tests/pipeline/ohioFinance/ohioSosCsv.test.ts (2)
148-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign this test name with the error it verifies, or remove the unreachable branch.
The test name says "no header at all", but the assertion matches the truncation error. For
"",end()reaches theendedWithSeparatorcheck first and throwsfinal row separator is missing, so the header check never runs.The
has no headererror atohioSosCsv.tslines 156-158 is in fact unreachable. It needsendedWithSeparator === truetogether withheaderSeen === false.endedWithSeparatorbecomes true only on the lines that immediately follow afinishRow()call, andfinishRow()setsheaderSeen = trueon the first row it completes.Either rename this test to describe truncation, or delete the unreachable branch so the file does not advertise a validation that never runs.
♻️ Proposed test rename
- it("rejects a file with no header at all", () => { + it("rejects an empty file as truncated", () => { expect(() => collect("")).toThrow(/final row separator is missing/); });🤖 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/ohioSosCsv.test.ts` around lines 148 - 150, Rename the test around collect("") to describe the missing final row separator/truncated input it actually verifies, rather than absence of a header. Alternatively, remove the unreachable no-header validation branch in the Ohio SOS CSV parser, but keep the test and parser behavior consistent.
212-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin both sides of the two-digit-year pivot.
The assertions cover
90and00. Neither sits on the boundary. The rule atohioSosCsv.tsline 358 switches century at exactly 70, so69and70are the values that detect an off-by-one change.💚 Proposed boundary assertions
it("parses cover-page dates in DD-MON-YY, pivoting two-digit years at 70", () => { expect(parseOhioSosDateIso("26-APR-90")).toBe("1990-04-26"); expect(parseOhioSosDateIso("15-DEC-00")).toBe("2000-12-15"); + expect(parseOhioSosDateIso("01-JAN-70")).toBe("1970-01-01"); + expect(parseOhioSosDateIso("01-JAN-69")).toBe("2069-01-01"); });🤖 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/ohioSosCsv.test.ts` around lines 212 - 215, Update the test case for parseOhioSosDateIso to assert both sides of the two-digit-year pivot: add DD-MON-YY inputs using years 69 and 70, expecting 2069 and 1970 respectively. Keep the existing 90 and 00 assertions unchanged.backend/src/pipeline/ohioFinance/ohioSosBulkFiles.ts (2)
558-560: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the 31-U classification through one predicate.
isOhioSos31uExpenditureRowclassifies a mapped row. Lines 876-877 instreamOhioSosBulkFilerepeat the same rule on the raw row. The two agree today, because both normalize withnormalizeOhioSosTextand both compare againstOHIO_SOS_31U_SHORT_DESCRIPTION_PREFIX.The two results are consumed by different halves of the same reconciliation:
storeOhioSosArtifactpersistsstats.reportKeys31uas the detail-fetch worklist, andcollectOhioSos31uAnnualTotalssums rows selected byisOhioSos31uExpenditureRow. If the rule drifts in one place, the worklist and the annual totals disagree and the three-way reconciliation reports phantom mismatches.Extract the string test into one helper and call it from both sites.
♻️ Proposed shared predicate
+export function isOhioSos31uShortDescription(rawShortDescription: string | undefined): boolean { + return normalizeOhioSosText(rawShortDescription).startsWith(OHIO_SOS_31U_SHORT_DESCRIPTION_PREFIX); +} + export function isOhioSos31uExpenditureRow(row: Pick<OhioSosExpenditureRow, "shortDescription">): boolean { return row.shortDescription?.startsWith(OHIO_SOS_31U_SHORT_DESCRIPTION_PREFIX) ?? false; }Then in
streamOhioSosBulkFile:if (fileFamily.collect31uReportKeys) { - const shortDescription = normalizeOhioSosText(row[fileFamily.collect31uReportKeys.shortDescriptionColumn]); - if (shortDescription.startsWith(OHIO_SOS_31U_SHORT_DESCRIPTION_PREFIX)) { + if (isOhioSos31uShortDescription(row[fileFamily.collect31uReportKeys.shortDescriptionColumn])) {🤖 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/ohioSosBulkFiles.ts` around lines 558 - 560, Extract the shared 31-U short-description check into a helper near isOhioSos31uExpenditureRow, applying the existing normalizeOhioSosText and OHIO_SOS_31U_SHORT_DESCRIPTION_PREFIX rule. Update isOhioSos31uExpenditureRow and the raw-row classification in streamOhioSosBulkFile to call this helper, ensuring both reconciliation paths use the same predicate.
236-273: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe 12 cover-page money columns are mapped by position and only partly verified.
coverPageMapperdestructures the 12 parsed values out ofindexes.moneyby array position, so the field-to-column mapping depends entirely on the declaration order ofCOVER_MONEY_COLUMNS. The fixture test verifies 4 of those 12 fields. A future reorder or insertion therefore misattributes dollar amounts with no type error and no test failure.
backend/src/pipeline/ohioFinance/ohioSosBulkFiles.ts#L236-L273: keyindexes.moneyby column name and read each money field by that key, so the compiler checks the mapping.backend/tests/pipeline/ohioFinance/ohioSosBulkFiles.test.ts#L84-L97: assert all 12 money fields, either withtoEqualon the full row or by extending thetoMatchObjectargument with the 8 currently unverified fields.🤖 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/ohioSosBulkFiles.ts` around lines 236 - 273, Update coverPageMapper in backend/src/pipeline/ohioFinance/ohioSosBulkFiles.ts (lines 236-273) to access parsed money values by each COVER_MONEY_COLUMNS name instead of destructuring indexes.money by position, preserving the existing field mappings while making them compiler-checked. Extend the fixture assertions in backend/tests/pipeline/ohioFinance/ohioSosBulkFiles.test.ts (lines 84-97) to verify all 12 money fields, including the 8 currently unverified fields.backend/tests/pipeline/ohioFinance/ohioSosArtifactCache.test.ts (1)
26-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the temporary cache directories after each test.
beforeEachcreates a newmkdtempdirectory for every test in this file. No hook removes it. Each run leaves directories in the system temp location, and one of them holds a copy of the fixture CSV.♻️ Proposed cleanup hook
-import { mkdtemp, readFile, writeFile } from "node:fs/promises"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; @@ -import { beforeEach, describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; @@ beforeEach(async () => { cacheDir = await mkdtemp(join(tmpdir(), "ohio-sos-cache-")); }); + +afterEach(async () => { + await rm(cacheDir, { recursive: true, force: true }); +});🤖 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/ohioSosArtifactCache.test.ts` around lines 26 - 30, Clean up the temporary directory created by the ohioSosArtifactCache.test.ts beforeEach hook after every test. Add an afterEach cleanup hook that removes the cacheDir recursively and tolerates already-absent paths, ensuring the fixture CSV copy and directory are deleted even when a test fails.backend/src/pipeline/ohioFinance/ohioSosArtifactCache.ts (2)
310-345: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winInstall the manifest atomically, and write it before the data file becomes visible.
The data file is renamed into place at Line 314, but the manifest is written last at Line 343 with a direct
writeFile. Two failure windows follow:
- If the process stops between Line 314 and Line 343, the cached bytes no longer match the previous manifest.
getOhioSosArtifactStatusthen reportsstale.writeFileonpaths.manifestPathis not atomic. A partial write leaves truncated JSON, soreadOhioSosArtifactManifestreturnsnulland the status becomesmissing.Both states resolve only by re-downloading the product, which costs up to ~90 MB per file. Write the manifest to a temporary path and rename it, so each install publishes a complete pair.
♻️ Proposed atomic manifest install
- await writeFile(paths.manifestPath, `${JSON.stringify(manifest, null, 2)}\n`, "utf8"); + const tmpManifestPath = `${paths.manifestPath}.tmp-${process.pid}-${retrievedAt.getTime()}`; + await writeFile(tmpManifestPath, `${JSON.stringify(manifest, null, 2)}\n`, "utf8"); + try { + await rename(tmpManifestPath, paths.manifestPath); + } catch (error) { + await rm(tmpManifestPath, { force: true }).catch(() => {}); + throw error; + } return manifest;🤖 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/ohioSosArtifactCache.ts` around lines 310 - 345, Update the artifact installation flow around the manifest construction and direct writeFile call to write the manifest to a unique temporary path, then atomically rename that temporary manifest into paths.manifestPath. Publish the manifest before renaming the data file into paths.filePath, and clean up the temporary manifest on failure so readers never observe partial JSON or a data file without its matching manifest.
248-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider validating the numeric and enum manifest fields before the cast.
The guard checks
version,productKey,fileName,filePath,sha256,byteSize,rowCount,retrievedAt, and thatreportKeys31uis an array. It does not checktransactionYear,encoding,rowSeparator, the date fields, the count fields, or the element type ofreportKeys31u. The cast at Line 265 therefore promises more than the guard proves. A hand-edited manifest can produce a value that reads asOhioSosArtifactManifestbut carriesundefinedcounts.The current consumers read only validated fields, so this is a typing risk rather than a live defect.
🤖 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/ohioSosArtifactCache.ts` around lines 248 - 273, Strengthen the validation guard in readOhioSosArtifactManifest before the OhioSosArtifactManifest cast to validate every required numeric, enum, date, and count field, plus the element type of reportKeys31u. Reuse the existing schema constants or type guards where available, and only return the cast manifest after all required fields are proven valid.backend/src/pipeline/ohioFinance/ohioSosArtifactAcquisition.ts (1)
387-391: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReport the expenditure files that were skipped as absent.
The
ENOENTbranch swallows a missing cached expenditure file. The reconciliation baseline for every report key in that year then stays absent, andreconcileOhioSos31uReportlater compares detail rows against a total of0. Nothing in the returned value records the gap, so a mismatch report cannot distinguish "no 31-U rows" from "file not downloaded".Return the skipped product and year alongside the totals, or accept a
logcallback and emit one line per skipped file.🤖 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/ohioSosArtifactAcquisition.ts` around lines 387 - 391, The ENOENT branch in the acquisition flow must record each missing cached expenditure file instead of silently continuing. Update the surrounding function’s returned result to include the skipped product and year, or pass a log callback that emits one entry per skipped file, while preserving rethrow behavior for non-ENOENT errors and ensuring reconcileOhioSos31uReport can distinguish missing files from zero totals.backend/tests/pipeline/ohioFinance/ohioSosArtifactAcquisition.test.ts (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the orchestration functions.
This file tests only the pure helpers: the URL builders,
planOhioSosCycleDownloads, andohioSos31uDetailCachePath.listOhioSosPortalFiles,downloadOhioSosCycleArtifacts, andfetchOhioSos31uDetailsacceptsession,tab, andsleepas parameters, so a fake session that replays CDP events covers them without a browser.Two of the defects raised in
ohioSosArtifactAcquisition.tssit in exactly these untested paths: the droppeddownloadProgressevent inwatchOhioSosDownload, and the unconditional detail-bundle write infetchOhioSos31uDetails. Tests that emit acompletedevent beforewait()and that run a cycle where every report fails would pin both.🤖 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/ohioSosArtifactAcquisition.test.ts` around lines 1 - 9, Add orchestration coverage in the Ohio SOS acquisition tests for listOhioSosPortalFiles, downloadOhioSosCycleArtifacts, and fetchOhioSos31uDetails using a fake CDP session that replays events, with stubbed tab and sleep dependencies. Include a download test that emits a completed downloadProgress event before wait() returns, and a detail-fetch cycle where every report fails, verifying the expected handling without browser dependencies.
🤖 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/ohioSosArtifactAcquisition.ts`:
- Around line 476-493: Update the detail-bundle write flow around
ohioSos31uDetailCachePath and writeFile to create input.cacheDir before writing,
then write the payload to a temporary file and atomically rename it into place.
When reports is empty and failures is non-empty, skip replacing the existing
detail bundle; preserve normal replacement for successful or partially
successful runs.
- Around line 206-242: Update watchOhioSosDownload to accept the expected
download URL and filter Browser.downloadWillBegin events by that URL before
recording guid, preventing unrelated profile downloads from hijacking the
watcher. Persist the terminal completed or canceled result when settle/fail are
unavailable, then have wait replay the stored outcome immediately after
registering its handlers while preserving timeout behavior for pending
downloads. Update the watcher call site to pass the URL that must match.
In `@backend/src/pipeline/ohioFinance/ohioSosChromeClient.ts`:
- Around line 42-43: Guard JSON parsing in the socket message listener by
catching invalid frame errors and routing the failure through the command’s
existing rejection/error path instead of letting the listener throw. Update the
handler around JSON.parse in the message listener while preserving normal
CdpMessage processing for valid JSON frames.
- Around line 72-82: Update OhioSosChromeClient.send to apply a finite deadline
to every pending CDP command, ensuring timed-out promises reject and their
pending entries are removed. Also pass an AbortSignal.timeout(...) to the fetch
call in connectOhioSosChrome so stalled DevTools connections settle; preserve
existing close and response handling while allowing callers’ error paths to run.
In `@backend/src/scripts/refreshOhioSosCampaignFinanceRawData.ts`:
- Around line 202-237: Recheck required artifact readiness after acquisition in
the refresh flow before calling fetchOhioSos31uDetails; do not derive totals or
write detail cache data while any required artifact is missing. When artifacts
are incomplete, preserve the existing detail cache and return an explicit
details-skipped result, while retaining the current detail metadata path only
when every artifact is ready.
- Around line 152-154: Separate the cache-state set used for reporting from the
download-selection set in the refresh flow. Preserve the actual ready-file names
from cachedBefore for the dry-run cached output, while keeping skip empty under
options.force so force still downloads every artifact; update the output logic
around the skip usage near lines 179–184 accordingly.
- Around line 104-130: Update the argument parsing flow around the visible
cycleYear and boolean option handling to validate every token, rejecting unknown
options and value-bearing forms such as --dry-run=true or misspelled boolean
flags instead of silently ignoring them. Detect and reject simultaneous
--cycle-year and --year inputs rather than preferring one, while preserving
existing valid option parsing. Add regression tests covering unknown options,
boolean values, misspelled flags, and conflicting cycle-year aliases.
---
Nitpick comments:
In `@backend/src/pipeline/ohioFinance/ohioSosArtifactAcquisition.ts`:
- Around line 387-391: The ENOENT branch in the acquisition flow must record
each missing cached expenditure file instead of silently continuing. Update the
surrounding function’s returned result to include the skipped product and year,
or pass a log callback that emits one entry per skipped file, while preserving
rethrow behavior for non-ENOENT errors and ensuring reconcileOhioSos31uReport
can distinguish missing files from zero totals.
In `@backend/src/pipeline/ohioFinance/ohioSosArtifactCache.ts`:
- Around line 310-345: Update the artifact installation flow around the manifest
construction and direct writeFile call to write the manifest to a unique
temporary path, then atomically rename that temporary manifest into
paths.manifestPath. Publish the manifest before renaming the data file into
paths.filePath, and clean up the temporary manifest on failure so readers never
observe partial JSON or a data file without its matching manifest.
- Around line 248-273: Strengthen the validation guard in
readOhioSosArtifactManifest before the OhioSosArtifactManifest cast to validate
every required numeric, enum, date, and count field, plus the element type of
reportKeys31u. Reuse the existing schema constants or type guards where
available, and only return the cast manifest after all required fields are
proven valid.
In `@backend/src/pipeline/ohioFinance/ohioSosBulkFiles.ts`:
- Around line 558-560: Extract the shared 31-U short-description check into a
helper near isOhioSos31uExpenditureRow, applying the existing
normalizeOhioSosText and OHIO_SOS_31U_SHORT_DESCRIPTION_PREFIX rule. Update
isOhioSos31uExpenditureRow and the raw-row classification in
streamOhioSosBulkFile to call this helper, ensuring both reconciliation paths
use the same predicate.
- Around line 236-273: Update coverPageMapper in
backend/src/pipeline/ohioFinance/ohioSosBulkFiles.ts (lines 236-273) to access
parsed money values by each COVER_MONEY_COLUMNS name instead of destructuring
indexes.money by position, preserving the existing field mappings while making
them compiler-checked. Extend the fixture assertions in
backend/tests/pipeline/ohioFinance/ohioSosBulkFiles.test.ts (lines 84-97) to
verify all 12 money fields, including the 8 currently unverified fields.
In `@backend/tests/pipeline/ohioFinance/ohioSosArtifactAcquisition.test.ts`:
- Around line 1-9: Add orchestration coverage in the Ohio SOS acquisition tests
for listOhioSosPortalFiles, downloadOhioSosCycleArtifacts, and
fetchOhioSos31uDetails using a fake CDP session that replays events, with
stubbed tab and sleep dependencies. Include a download test that emits a
completed downloadProgress event before wait() returns, and a detail-fetch cycle
where every report fails, verifying the expected handling without browser
dependencies.
In `@backend/tests/pipeline/ohioFinance/ohioSosArtifactCache.test.ts`:
- Around line 26-30: Clean up the temporary directory created by the
ohioSosArtifactCache.test.ts beforeEach hook after every test. Add an afterEach
cleanup hook that removes the cacheDir recursively and tolerates already-absent
paths, ensuring the fixture CSV copy and directory are deleted even when a test
fails.
In `@backend/tests/pipeline/ohioFinance/ohioSosCsv.test.ts`:
- Around line 148-150: Rename the test around collect("") to describe the
missing final row separator/truncated input it actually verifies, rather than
absence of a header. Alternatively, remove the unreachable no-header validation
branch in the Ohio SOS CSV parser, but keep the test and parser behavior
consistent.
- Around line 212-215: Update the test case for parseOhioSosDateIso to assert
both sides of the two-digit-year pivot: add DD-MON-YY inputs using years 69 and
70, expecting 2069 and 1970 respectively. Keep the existing 90 and 00 assertions
unchanged.
🪄 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: 21b386e4-fbc8-464b-9054-458bdb031482
⛔ Files ignored due to path filters (5)
backend/tests/fixtures/ohioFinance/31u_detail_sample.csvis excluded by!**/*.csvbackend/tests/fixtures/ohioFinance/act_can_list_sample.csvis excluded by!**/*.csvbackend/tests/fixtures/ohioFinance/cac_con_sample.csvis excluded by!**/*.csvbackend/tests/fixtures/ohioFinance/can_cover_sample.csvis excluded by!**/*.csvbackend/tests/fixtures/ohioFinance/pac_exp_31u_sample.csvis excluded by!**/*.csv
📒 Files selected for processing (17)
.gitignorebackend/package.jsonbackend/src/pipeline/ohioFinance/index.tsbackend/src/pipeline/ohioFinance/ohioSos31uDetail.tsbackend/src/pipeline/ohioFinance/ohioSosArtifactAcquisition.tsbackend/src/pipeline/ohioFinance/ohioSosArtifactCache.tsbackend/src/pipeline/ohioFinance/ohioSosBulkFiles.tsbackend/src/pipeline/ohioFinance/ohioSosChromeClient.tsbackend/src/pipeline/ohioFinance/ohioSosCsv.tsbackend/src/scripts/refreshOhioSosCampaignFinanceRawData.tsbackend/tests/pipeline/ohioFinance/ohioSos31uDetail.test.tsbackend/tests/pipeline/ohioFinance/ohioSosArtifactAcquisition.test.tsbackend/tests/pipeline/ohioFinance/ohioSosArtifactCache.test.tsbackend/tests/pipeline/ohioFinance/ohioSosBulkFiles.test.tsbackend/tests/pipeline/ohioFinance/ohioSosCsv.test.tsbackend/tests/scripts/refreshOhioSosCampaignFinanceRawData.test.tsohio_plan.md
| function watchOhioSosDownload(session: OhioSosChromeSession, stagingDir: string): DownloadWatcher { | ||
| let guid: string | null = null; | ||
| let settle: ((result: { filePath: string }) => void) | null = null; | ||
| let fail: ((error: Error) => void) | null = null; | ||
|
|
||
| const off = session.on((event) => { | ||
| if (event.method === "Browser.downloadWillBegin") { | ||
| guid = String(event.params.guid); | ||
| return; | ||
| } | ||
| if (event.method !== "Browser.downloadProgress" || String(event.params.guid) !== guid) { | ||
| return; | ||
| } | ||
| const state = String(event.params.state); | ||
| if (state === "completed" && settle) { | ||
| settle({ filePath: join(stagingDir, guid!) }); | ||
| } else if (state === "canceled" && fail) { | ||
| fail(new Error("Chrome canceled the download")); | ||
| } | ||
| }); | ||
|
|
||
| return { | ||
| wait: ({ timeoutMs }) => | ||
| new Promise<{ filePath: string }>((resolve, reject) => { | ||
| const timeout = setTimeout(() => reject(new Error("Timed out waiting for the download to finish")), timeoutMs); | ||
| settle = (result) => { | ||
| clearTimeout(timeout); | ||
| resolve(result); | ||
| }; | ||
| fail = (error) => { | ||
| clearTimeout(timeout); | ||
| reject(error); | ||
| }; | ||
| }), | ||
| dispose: off, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Fix two event-handling defects in watchOhioSosDownload.
1. A completion event that arrives before wait() is lost. The watcher registers the session listener at Line 300, but settle and fail stay null until wait() runs its executor at Line 231. wait() is only called after await session.send("Page.navigate", ...) resolves at Line 302. Chrome can emit Browser.downloadProgress with state === "completed" in that window for a small file. Line 220 then evaluates settle as null and drops the event. The download waits the full downloadTimeoutMs (300 s by default) and is recorded as a failure even though the file arrived. Record the terminal state in the watcher and replay it inside wait().
2. Any download in the attached profile can hijack the watcher. Line 212 accepts every Browser.downloadWillBegin event and overwrites guid. The module header states that the session attaches to the user's own long-lived Chrome profile, and Line 282 sets Browser.setDownloadBehavior browser-wide with downloadPath: stagingDir. If the user starts any download during the run, its event replaces guid. The watcher then resolves with that unrelated file, storeOhioSosArtifact reads it in place of the portal artifact, and Line 320 deletes the user's file. Filter downloadWillBegin on the expected frameId or url before accepting the guid.
🐛 Proposed watcher rewrite
-function watchOhioSosDownload(session: OhioSosChromeSession, stagingDir: string): DownloadWatcher {
+function watchOhioSosDownload(
+ session: OhioSosChromeSession,
+ stagingDir: string,
+ expectedUrl: string
+): DownloadWatcher {
let guid: string | null = null;
let settle: ((result: { filePath: string }) => void) | null = null;
let fail: ((error: Error) => void) | null = null;
+ // Terminal state can arrive before wait() registers its callbacks.
+ let pendingResult: { filePath: string } | null = null;
+ let pendingError: Error | null = null;
const off = session.on((event) => {
if (event.method === "Browser.downloadWillBegin") {
- guid = String(event.params.guid);
+ // Only the navigation this watcher started may claim the guid; the
+ // attached profile is the user's own and can start other downloads.
+ if (guid === null && String(event.params.url) === expectedUrl) {
+ guid = String(event.params.guid);
+ }
return;
}
if (event.method !== "Browser.downloadProgress" || String(event.params.guid) !== guid) {
return;
}
const state = String(event.params.state);
- if (state === "completed" && settle) {
- settle({ filePath: join(stagingDir, guid!) });
- } else if (state === "canceled" && fail) {
- fail(new Error("Chrome canceled the download"));
+ if (state === "completed") {
+ const result = { filePath: join(stagingDir, guid) };
+ if (settle) settle(result);
+ else pendingResult = result;
+ } else if (state === "canceled") {
+ const error = new Error("Chrome canceled the download");
+ if (fail) fail(error);
+ else pendingError = error;
}
});
return {
wait: ({ timeoutMs }) =>
new Promise<{ filePath: string }>((resolve, reject) => {
+ if (pendingResult) return resolve(pendingResult);
+ if (pendingError) return reject(pendingError);
const timeout = setTimeout(() => reject(new Error("Timed out waiting for the download to finish")), timeoutMs);Update the call site to pass the URL that the watcher must match:
- const watcher = watchOhioSosDownload(input.session, stagingDir);
+ const downloadUrl = ohioSosFileDownloadUrl(entry.downloadId);
+ const watcher = watchOhioSosDownload(input.session, stagingDir, downloadUrl);
try {
await input.session.send(
"Page.navigate",
- { url: ohioSosFileDownloadUrl(entry.downloadId) },
+ { url: downloadUrl },
input.tab.sessionId
);🤖 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/ohioSosArtifactAcquisition.ts` around lines
206 - 242, Update watchOhioSosDownload to accept the expected download URL and
filter Browser.downloadWillBegin events by that URL before recording guid,
preventing unrelated profile downloads from hijacking the watcher. Persist the
terminal completed or canceled result when settle/fail are unavailable, then
have wait replay the stored outcome immediately after registering its handlers
while preserving timeout behavior for pending downloads. Update the watcher call
site to pass the URL that must match.
- Skip decisions now compare the portal's date-modified against the cached manifest, so a default refresh picks up updated portal files instead of permanently trusting the first download (--force unchanged) - Download watcher stores terminal events that fire before wait() is armed, and only claims downloads from its own tab's main frame, so a fast portal download is not lost and a user-initiated download cannot be hijacked - 31-U detail bundle: cache dir created, written via tmp+rename, and an existing bundle is preserved when any report scrape failed - Form 31-U stage is skipped (and says so in the result) unless every cycle artifact is ready, so partial acquisitions cannot shrink the bundle - CDP client: per-command deadline, JSON frame parse guard, and a timeout on the /json/version probe, so a silent Chrome stall cannot hang the run - Manifest writes go through tmp+rename so a crash cannot leave a torn manifest - Argument parser rejects unknown options, value-bearing boolean flags, and conflicting --cycle-year/--year; dry-run reports cached and would_download separately Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
backend/src/pipeline/ohioFinance/ohioSosCsv.ts (1)
209-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared parser construction.
parseOhioSosCsvTextandparseOhioSosCsvFilerepeat the same parser setup and the same malformed-row counting wrapper. Extract one helper that returns the parser and a counter accessor.🤖 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/ohioSosCsv.ts` around lines 209 - 247, Extract the duplicated OhioSosCsvParser construction and malformed-row counting wrapper from parseOhioSosCsvText and parseOhioSosCsvFile into a shared helper that returns the parser plus a malformed-row counter accessor. Update both functions to use the helper while preserving their existing input options, parsing behavior, and returned count.backend/tests/pipeline/ohioFinance/ohioSosBulkFiles.test.ts (1)
212-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the untested bulk-file families.
Four of the eleven families are exercised here.
pac_list,pac_cover,party_cover,pac_contributions,party_contributions,candidate_expenditures, andparty_expenditureshave no test. The derivedOHIO_SOS_PARTY_EXPENDITURES_HEADERis the highest-risk gap, because a header mismatch throws at first production read. One small fixture per family, or one header-shape assertion per family, closes the gap.🤖 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/ohioSosBulkFiles.test.ts` around lines 212 - 247, Add focused coverage for the seven untested Ohio SoS bulk-file families: pac_list, pac_cover, party_cover, pac_contributions, party_contributions, candidate_expenditures, and party_expenditures. Extend the relevant tests around readFixture and OHIO_SOS_PAC_EXPENDITURES_FAMILY with either a minimal fixture per family or header-shape assertions, explicitly validating OHIO_SOS_PARTY_EXPENDITURES_HEADER to catch mismatches during reads.backend/src/pipeline/ohioFinance/ohioSosBulkFiles.ts (1)
855-885: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParse each date and amount once per row.
The visitor parses every date column and the amount column, then
mapRowparses the same cells again. On the ~90 MB transaction files this doubles the regex and date work for every caller that suppliesvisit. Consider mapping the row first and reading the typed fields for the diagnostics.
missingDateRowCountandimplausibleDateRowCountalso increment once per date column. Every family declares one date column today, so the totals agree. If a family ever declares two date columns, one row can add two to a counter named "RowCount". Count per row, or rename the fields.🤖 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/ohioSosBulkFiles.ts` around lines 855 - 885, Update the visitor in the file-family processing flow to call fileFamily.mapRow once, reuse its typed date and amount values for diagnostics, and pass that same mapped result to visit instead of reparsing raw cells. Preserve min/max date tracking across valid dates, while counting missingDateRowCount and implausibleDateRowCount at most once per row even when multiple date columns exist.backend/src/scripts/refreshOhioSosCampaignFinanceRawData.ts (2)
368-377: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet a non-zero exit code when the refresh reports failures.
mainprints the result and returns. The process exits with code 0 even whenacquisition.failuresis non-empty, when required artifacts are missing, or when Form 31-U details were skipped. A caller ofnpm run ohio-candidates:finance:raw:refreshcannot detect a partial refresh from the exit status.♻️ Proposed change
const output = await runRefreshOhioSosCampaignFinanceRawDataScript({ options }); console.log(JSON.stringify(output, null, 2)); + const failed = + ("failures" in output && output.failures.length > 0) || + ("missing_file_names" in output && output.missing_file_names.length > 0); + if (failed) { + process.exitCode = 1; + } }🤖 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/scripts/refreshOhioSosCampaignFinanceRawData.ts` around lines 368 - 377, Update main and the refresh result handling around runRefreshOhioSosCampaignFinanceRawDataScript so the process exits with a non-zero status whenever acquisition.failures is non-empty, required artifacts are missing, or Form 31-U details were skipped; preserve the existing output logging and disabled-refresh behavior.
204-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider declaring an explicit return type for this exported entrypoint.
The function returns two different object shapes: the dry-run summary at Line 246 and the full result at Line 339. The return type is inferred, so external consumers depend on structural inference across both branches. Declare a discriminated union type keyed on
dry_runand annotate the return type. This keeps the script output contract stable when either branch changes.🤖 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/scripts/refreshOhioSosCampaignFinanceRawData.ts` around lines 204 - 208, Define an explicit discriminated-union return type for runRefreshOhioSosCampaignFinanceRawDataScript, keyed by dry_run, covering both the dry-run summary and full-result shapes. Annotate the exported function with this type and ensure both return branches conform to their corresponding union member.backend/tests/scripts/refreshOhioSosCampaignFinanceRawData.test.ts (1)
106-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the Form 31-U details gate.
This file tests argument parsing and
selectOhioSosDownloadSkips. It does not testrunRefreshOhioSosCampaignFinanceRawDataScript. The branch at Line 286 through Line 301 ofbackend/src/scripts/refreshOhioSosCampaignFinanceRawData.tsprevents an incomplete cycle from replacing a complete Form 31-U detail cache. That behavior currently has no regression test. Add a test that mocks the acquisition module and assertsfetchOhioSos31uDetailsis not called when a cycle artifact status is notready.🤖 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/scripts/refreshOhioSosCampaignFinanceRawData.test.ts` around lines 106 - 179, Add a regression test for the Form 31-U completeness gate in runRefreshOhioSosCampaignFinanceRawDataScript, mocking the acquisition module and configuring a cycle artifact status other than “ready”; assert that fetchOhioSos31uDetails is not called. Keep the test focused on preventing incomplete cycle data from triggering Form 31-U detail acquisition.backend/tests/pipeline/ohioFinance/ohioSosArtifactAcquisition.test.ts (1)
53-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the listing-to-file-name step.
planOhioSosCycleDownloadsis tested with hand-builtOhioSosListedFilevalues, so the step that produces them is untested.listOhioSosPortalFilesresolves each file name throughfileNameFromListing, which prefers the anchor label, falls back to the row cells, and upper-cases the match. It also picksdateModifiedwithlooksLikeDate.FakeChromeSessionalready supports a cannedRuntime.evaluatevalue, so a test can drivelistOhioSosPortalFileswith a synthetic listing and assert the extracted name, id, and date. A test for the empty-listing error would also pin the Cloudflare-interstitial message.🤖 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/ohioSosArtifactAcquisition.test.ts` around lines 53 - 126, Add tests for listOhioSosPortalFiles using FakeChromeSession’s canned Runtime.evaluate response and a synthetic portal listing, covering fileNameFromListing’s anchor-label preference, row-cell fallback, upper-casing, and looksLikeDate-based dateModified extraction while asserting each file’s name and id. Also add an empty-listing case that verifies the expected Cloudflare-interstitial error message.
🤖 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/ohioSosArtifactAcquisition.ts`:
- Around line 510-523: Update the result returned by the failure branch in
ohioSos31uDetailCachePath’s acquisition flow to include a discriminator
indicating that the existing bundle was preserved rather than written. Ensure
the normal write result also provides the corresponding written state, and
update the caller’s handling in refreshOhioSosCampaignFinanceRawData to use this
discriminator when labeling the reported bundle while retaining the scraped
report metadata.
- Around line 313-320: Restore the browser’s default download behavior after the
download run and before removing stagingDir. Update the flow surrounding
Browser.setDownloadBehavior to reset the globally applied behavior/path,
ensuring no stale stagingDir remains configured for subsequent downloads while
preserving the existing download setup.
In `@backend/src/pipeline/ohioFinance/ohioSosBulkFiles.ts`:
- Around line 517-520: Update OHIO_SOS_PARTY_EXPENDITURES_HEADER to use the
pinned PARTY_EXP column list captured during acquisition-spike research, rather
than spreading OHIO_SOS_PAC_EXPENDITURES_HEADER. Preserve the captured column
order and values as the payload contract, using the voteapp-manual-research
reference documentation to verify the header.
In `@backend/src/pipeline/ohioFinance/ohioSosChromeClient.ts`:
- Around line 152-171: Update the WebSocket connection handshake in
connectOhioSosChrome so both timeout and error rejection paths close the socket
before rejecting. Preserve the existing successful open behavior, and ensure
cleanup also handles a socket that is still connecting or already open.
In `@backend/src/pipeline/ohioFinance/ohioSosCsv.ts`:
- Around line 153-158: Update ohioSosCsv.ts in end() to evaluate the
!this.headerSeen validation before !this.endedWithSeparator, ensuring empty or
header-less files report “has no header.” Update
backend/tests/pipeline/ohioFinance/ohioSosCsv.test.ts lines 148-150 to assert
collect("") matches /has no header/.
- Around line 296-307: Update parseOhioSosAmountCents to derive cents from the
normalized decimal text using exact scaled-value arithmetic, especially for
three- and four-decimal inputs, instead of relying on Math.round(amount * 100).
Preserve validation, negative-value handling, and the existing safe-integer/null
behavior while ensuring values such as .145 round to 15 cents.
- Around line 248-253: Address the ICU runtime requirement for the CSV decoding
path around TextDecoder in ohioSosCsv.ts: either document full ICU as a required
runtime configuration in the existing backend package/runtime documentation, or
add an early startup validation that constructs the required non-Unicode decoder
and throws a clear actionable error when unsupported. Ensure the check occurs
before pipeline CSV processing begins.
---
Nitpick comments:
In `@backend/src/pipeline/ohioFinance/ohioSosBulkFiles.ts`:
- Around line 855-885: Update the visitor in the file-family processing flow to
call fileFamily.mapRow once, reuse its typed date and amount values for
diagnostics, and pass that same mapped result to visit instead of reparsing raw
cells. Preserve min/max date tracking across valid dates, while counting
missingDateRowCount and implausibleDateRowCount at most once per row even when
multiple date columns exist.
In `@backend/src/pipeline/ohioFinance/ohioSosCsv.ts`:
- Around line 209-247: Extract the duplicated OhioSosCsvParser construction and
malformed-row counting wrapper from parseOhioSosCsvText and parseOhioSosCsvFile
into a shared helper that returns the parser plus a malformed-row counter
accessor. Update both functions to use the helper while preserving their
existing input options, parsing behavior, and returned count.
In `@backend/src/scripts/refreshOhioSosCampaignFinanceRawData.ts`:
- Around line 368-377: Update main and the refresh result handling around
runRefreshOhioSosCampaignFinanceRawDataScript so the process exits with a
non-zero status whenever acquisition.failures is non-empty, required artifacts
are missing, or Form 31-U details were skipped; preserve the existing output
logging and disabled-refresh behavior.
- Around line 204-208: Define an explicit discriminated-union return type for
runRefreshOhioSosCampaignFinanceRawDataScript, keyed by dry_run, covering both
the dry-run summary and full-result shapes. Annotate the exported function with
this type and ensure both return branches conform to their corresponding union
member.
In `@backend/tests/pipeline/ohioFinance/ohioSosArtifactAcquisition.test.ts`:
- Around line 53-126: Add tests for listOhioSosPortalFiles using
FakeChromeSession’s canned Runtime.evaluate response and a synthetic portal
listing, covering fileNameFromListing’s anchor-label preference, row-cell
fallback, upper-casing, and looksLikeDate-based dateModified extraction while
asserting each file’s name and id. Also add an empty-listing case that verifies
the expected Cloudflare-interstitial error message.
In `@backend/tests/pipeline/ohioFinance/ohioSosBulkFiles.test.ts`:
- Around line 212-247: Add focused coverage for the seven untested Ohio SoS
bulk-file families: pac_list, pac_cover, party_cover, pac_contributions,
party_contributions, candidate_expenditures, and party_expenditures. Extend the
relevant tests around readFixture and OHIO_SOS_PAC_EXPENDITURES_FAMILY with
either a minimal fixture per family or header-shape assertions, explicitly
validating OHIO_SOS_PARTY_EXPENDITURES_HEADER to catch mismatches during reads.
In `@backend/tests/scripts/refreshOhioSosCampaignFinanceRawData.test.ts`:
- Around line 106-179: Add a regression test for the Form 31-U completeness gate
in runRefreshOhioSosCampaignFinanceRawDataScript, mocking the acquisition module
and configuring a cycle artifact status other than “ready”; assert that
fetchOhioSos31uDetails is not called. Keep the test focused on preventing
incomplete cycle data from triggering Form 31-U detail acquisition.
🪄 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: e21647ce-d7b4-415b-8d9d-a5adce209c80
⛔ Files ignored due to path filters (5)
backend/tests/fixtures/ohioFinance/31u_detail_sample.csvis excluded by!**/*.csvbackend/tests/fixtures/ohioFinance/act_can_list_sample.csvis excluded by!**/*.csvbackend/tests/fixtures/ohioFinance/cac_con_sample.csvis excluded by!**/*.csvbackend/tests/fixtures/ohioFinance/can_cover_sample.csvis excluded by!**/*.csvbackend/tests/fixtures/ohioFinance/pac_exp_31u_sample.csvis excluded by!**/*.csv
📒 Files selected for processing (17)
.gitignorebackend/package.jsonbackend/src/pipeline/ohioFinance/index.tsbackend/src/pipeline/ohioFinance/ohioSos31uDetail.tsbackend/src/pipeline/ohioFinance/ohioSosArtifactAcquisition.tsbackend/src/pipeline/ohioFinance/ohioSosArtifactCache.tsbackend/src/pipeline/ohioFinance/ohioSosBulkFiles.tsbackend/src/pipeline/ohioFinance/ohioSosChromeClient.tsbackend/src/pipeline/ohioFinance/ohioSosCsv.tsbackend/src/scripts/refreshOhioSosCampaignFinanceRawData.tsbackend/tests/pipeline/ohioFinance/ohioSos31uDetail.test.tsbackend/tests/pipeline/ohioFinance/ohioSosArtifactAcquisition.test.tsbackend/tests/pipeline/ohioFinance/ohioSosArtifactCache.test.tsbackend/tests/pipeline/ohioFinance/ohioSosBulkFiles.test.tsbackend/tests/pipeline/ohioFinance/ohioSosCsv.test.tsbackend/tests/scripts/refreshOhioSosCampaignFinanceRawData.test.tsohio_plan.md
| export const OHIO_SOS_PARTY_EXPENDITURES_HEADER = [ | ||
| ...OHIO_SOS_PAC_EXPENDITURES_HEADER, | ||
| "PARTY", | ||
| ] as const; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether a party-expenditure capture or fixture exists in the repository.
fd -i -t f 'par.*exp|party.*exp' | head -50
fd -t d 'ohioFinance' | head -20
rg -n 'PARTY_EXPENDITURES_HEADER|par_exp|PAR_EXP' --type=ts -C2Repository: shu1513/voteapp
Length of output: 259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== file header and constants/usages ==="
sed -n '1,80p' backend/src/pipeline/ohioFinance/ohioSosBulkFiles.ts
echo
rg -n 'OHIO_SOS_(PAC|PARTY)_EXPENDITURES_HEADER|OHIO_SOS_.*EXPENDITURES_HEADERS|expenditureMapper|ohioSosBulkFiles' backend/src/backend/src backend/src backend/tests -g '*.ts' -C 2
echo
echo "=== ohio tests/fixtures relevant files ==="
git ls-files backend/tests/pipeline/ohioFinance backend/tests/fixtures/ohioFinance | sort
echo
for f in $(git ls-files backend/tests/fixtures/ohioFinance); do
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,220p' "$f"
done
echo
echo "=== relevant source outline ==="
ast-grep outline backend/src/pipeline/ohioFinance/ohioSosBulkFiles.ts || trueRepository: shu1513/voteapp
Length of output: 9205
🌐 Web query:
voteapp-manual-research Ohio SoS PARTY_EXP header party expenditures
💡 Result:
The header PARTY_EXP is not associated with official Ohio Secretary of State campaign finance data files [1][2][3]. Based on a broad search of technical and governmental resources, the term PARTY_EXP is primarily found in documentation for computer network packet structures, specifically in relation to game server protocols such as those used by the OpenKore project, where it identifies packets related to party experience or party group information [4][5][6][7][8]. It also appears as a parameter name in certain legacy SAP ABAP function modules [9]. Official Ohio campaign finance data made available by the Ohio Secretary of State is typically provided in standardized formats for public disclosure, governed by Ohio Revised Code Chapter 3517 [10][11]. If you are encountering a file with this header in the context of Ohio campaign data, it is likely a non-standard or custom-generated file, as it does not correspond to the standard naming conventions or data dictionaries documented for the Secretary of State's electronic filing systems [1][2][3].
Citations:
- 1: https://data.ohiosos.gov/portal/campaign-finance
- 2: https://www.ohiosos.gov/campaign-finance/search/
- 3: https://www.ohiosos.gov/elections/campaign-finance
- 4: https://github.com/OpenKore/openkore/blob/master/src/Network/Receive/ServerType0.pm
- 5: https://github.com/OpenKore/openkore/blob/master/tables/packetlist.txt
- 6: https://github.com/OpenKore/openkore/blob/746bbb7a/src/Network/Receive/kRO/Sakexe_0.pm
- 7: OpenKore/openkore@84e1834
- 8: http://svn.code.sf.net/p/openkore/code/tablepack/trunk/tables/bRO/recvpackets.txt
- 9: https://www.sapdatasheet.org/abap/func/bbp_common_esa_cvt_shiploc_out.html
- 10: https://oh.elaws.us/orc/3517.106
- 11: https://oh.elaws.us/orc/3517.08
Pin the party expenditure header from the acquisition-spike capture.
OHIO_SOS_PARTY_EXPENDITURES_HEADER is derived from the PAC header instead of using a pinned PARTY_EXP column list. Use the captured party-expenditure header and the voteapp-manual-research reference documentation as the column contract before accepting this payload.
🤖 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/ohioSosBulkFiles.ts` around lines 517 - 520,
Update OHIO_SOS_PARTY_EXPENDITURES_HEADER to use the pinned PARTY_EXP column
list captured during acquisition-spike research, rather than spreading
OHIO_SOS_PAC_EXPENDITURES_HEADER. Preserve the captured column order and values
as the payload contract, using the voteapp-manual-research reference
documentation to verify the header.
Source: Coding guidelines
| const decoder = new TextDecoder("windows-1252"); | ||
| const stream = createReadStream(path); | ||
| for await (const chunk of stream) { | ||
| parser.push(decoder.decode(chunk as Buffer, { stream: true })); | ||
| } | ||
| parser.push(decoder.decode()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Node.js 20 TextDecoder windows-1252 support small-icu full-icu
💡 Result:
In Node.js 20, the support for the windows-1252 encoding in the TextDecoder API depends on the ICU data configuration used to build the Node.js binary [1][2]. Full ICU support (the default in official Node.js binaries) provides full support for most encodings, including windows-1252 [1][3]. In builds configured with small-icu, the native TextDecoder API is generally limited to Unicode encodings (such as UTF-8, UTF-16LE, and UTF-16BE), and it often does not natively support windows-1252 without additional ICU data or polyfills [1][3][4]. Technical context and recent developments: - Historical Implementation: Traditionally, TextDecoder in Node.js relied on the ICU library to support the WHATWG Encoding Standard [5]. When full-icu was not present, support for non-Unicode encodings was restricted [1][3]. - Regression and Fixes (Late 2025): Issues have occurred regarding the implementation of windows-1252, particularly confusion between it and the Latin-1 (ISO-8859-1) encoding [6][7]. As of late 2025, changes were made in the Node.js codebase (including patches applied to the v20.x maintenance branch) to correctly implement windows-1252 support and fix regressions where it was incorrectly treated as a subset of other encodings [6][8]. - Current Status: While the internal implementation has been refined to correctly handle windows-1252 mappings (especially the 0x80-0x9F range that differentiates it from Latin-1), the requirement for full-icu remains the standard way to ensure all encoding features are available in official builds [1][8]. If you are using a custom or minimal build without full-icu, you may need to use a third-party library or polyfill to handle windows-1252 decoding [4].
Citations:
- 1: https://nodejs.org/download/release/latest-v20.x/docs/api/intl.html
- 2: https://nodejs.org/download/release/v20.19.0/docs/api/intl.html
- 3: http://nodejs.org/docs/latest/api/intl.html
- 4: https://github.com/ChALkeR/text-codec
- 5: buffer: implement WHATWG Encoding Standard API nodejs/node#13644
- 6: TextDecoder behavior is wrong since 20.18.3/22.13.0 nodejs/node#60888
- 7: TextDecoder incorrectly decodes 0x92 and several other characters for Windows-1252 nodejs/node#56542
- 8: src: implement Windows-1252 encoding support and update related tests nodejs/node#60893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked package/runtime declarations:"
git ls-files | rg '(^backend/package\.json$|(^|/)package\.json$|Dockerfile$|docker-compose|README|pnpm|yarn|package-lock|pnpm-lock|yarn.lock)' | sort
echo
echo "backend package relevant fields:"
if [ -f backend/package.json ]; then
node - <<'JS'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('backend/package.json','utf8'));
const keys = ['engines','scripts'];
for (const k of Object.keys(pkg).filter(x => keys.includes(x) || ['volta','volta-bin','volta-lifecycle-scripts','volta-workspaces','volta-cache'].includes(x))) {
console.log(`${k}: ${pkg[k]}`);
}
JS
fi
echo
echo "Search for Node engine/runtime constraints:"
rg -n --hidden --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' '"node"|engines|NODE_VERSION|node:|from node|NODE_ENV|Dockerfile|docker-compose|small-icu|full-icu|`@actions/setup-node`|setup-node' backend Dockerfile docker-compose* 2>/dev/null || trueRepository: shu1513/voteapp
Length of output: 50371
Add ICU coverage for non-Unicode CSV decodings.
backend/package.json declares node >=20.12.0, but that range can include a custom small-icu build where new TextDecoder("windows-1252") throws RangeError. Document full ICU as a runtime requirement, or add an early startup check with a clear error before pipeline CSV decoding fails.
🤖 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/ohioSosCsv.ts` around lines 248 - 253,
Address the ICU runtime requirement for the CSV decoding path around TextDecoder
in ohioSosCsv.ts: either document full ICU as a required runtime configuration
in the existing backend package/runtime documentation, or add an early startup
validation that constructs the required non-Unicode decoder and throws a clear
actionable error when unsupported. Ensure the check occurs before pipeline CSV
processing begins.
- Skip Form 31-U details whenever the acquisition itself had download failures or portal-missing files, not just when the cache is unready — a failed download leaves the old snapshot 'ready' and would feed the detail stage stale, mixed-vintage report keys - Reset Browser.setDownloadBehavior to default after the run; the setting outlives the CDP connection and would keep routing the user's own downloads into the deleted staging directory - Cancel the pending load-event waiter when Page.navigate itself fails, so its timer cannot fire later as an unhandled rejection - Close the WebSocket on a failed DevTools handshake so the process can exit - 31-U result carries written:false when a prior bundle was preserved, and the script reports bundle_written so counts aren't mistaken for file contents - Derive amount cents from digits instead of Math.round(x*100), which loses a cent on 3-4 decimal inputs like .145 - Empty CSV downloads now report 'has no header' instead of claiming a missing final separator - Document the Chrome 136+ default-profile remote-debugging restriction in the script header, connect error, and ohio_plan.md; pin the rationale for the party-expenditure header spread; make the manifest crash-window comment honest about same-size replacements Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
PR 4 of the Ohio campaign-finance sequence (
ohio_plan.md). Adds the read path for the Ohio SoS bulk exports plus the attended acquisition step that fills the cache.No migration, no DB writes, no schema change. Capability matrix row unchanged — Ohio stays on the shared factories with zero new factory capability.
What's here
ohioSosCsv.tsohioSosBulkFiles.tsohioSos31uDetail.tsohioSosArtifactCache.tsohioSosChromeClient.ts+ohioSosArtifactAcquisition.ts+refreshOhioSosCampaignFinanceRawData.tsParsers handle Windows-1252 bytes, CR-only row separators, quoted currency amounts, header whitespace/underscore drift, HTML entities, the duplicated
OFFICEheader, and truncation. The ~90 MB contribution files are never read whole.Validated against real data
Ran the pinned schemas against all 17 real cycle-2026 files (305 MB from the spike): every file parsed with 0 malformed rows.
The 31-U two-stage reconciliation reproduces the acquisition spike exactly:
Three data findings, folded into code and plan
0202,0206,2926,3026,3036,5025all appear (~194 rows). Manifest date ranges now ignore implausible dates and reportimplausibleDateRowCountinstead of widening the range.AMOUNTis real — 11 rows across the cycle (blank in-kind amounts, a literalTESTrow). Parsed asnulland counted; never read as zero.-where the CSV export leaves them blank. Pinned by a test asserting the scrape and the export produce identical rows.Acquisition mechanism
The portal is behind Cloudflare and refuses scripted HTTP, headless Chrome, and fresh-profile automated Chrome alike. So this is an attended step: the user starts their own Chrome with
--remote-debugging-port=9222, then runswhich attaches over the DevTools protocol using Node's built-in WebSocket — no new dependency. It does label→ID discovery on
CFDISCLOSURE:73(ids are non-sequential and get reissued, so they're never hardcoded), strictly sequential paced downloads from:72(the portal 429s on rapid requests; 8s spacing is the default), then page-48 31-U detail scrapes.It solves no challenges and spoofs nothing. If Cloudflare interstitials, the script stops and says so. No unattended server job.
Also carries the acquisition spike's committed output: real-byte parser fixtures and the gitignore rule keeping the 305 MB cache out of the repo.
Checks
npm run typecheckclean.npm test— 6180 passed / 24 skipped, 811 files. 54 of those tests are new.🤖 Generated with Claude Code
Summary by CodeRabbit