fix(adv): bound the ingest folder and keep the newest filing per CRD - #363
Merged
Conversation
`IngestAdvSnapshotTask` joined `SEC_RAW_DATA_FOLDER` with an unvalidated `folder`, and supplying it skipped `advArchiveFolder`'s check on `snapshot` too. The task is registered, so both inputs arrive from `sec-base task run`, the console form and the MCP tool surface: any local `*.csv` could be read into `adv_row`, and a chosen snapshot label wiped a real one through the `deleteSearch` that runs first. It now resolves and prefix-checks the directory the way `BootstrapDownloadTask` does, and validates `snapshot` whether or not `folder` overrides where the archive is read from. `adv_adviser` is keyed `(snapshot, crd_number)` while the base member is one row per filing, so the cumulative archive's thirteen years of amendments all collided on one key and the survivor was decided by CSV position rather than by `date_submitted` — leaving `sec show advisers --min-aum` filtering on an arbitrary historical AUM. Base rows are now reduced to one per CRD, newest `date_submitted` winning with `filing_id` as tie-break and an undated filing never displacing a dated one, and the reported `advisers:` count is what landed rather than what was pushed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T8DU24G9GWuRUJncc5TJFZ
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
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.
Two independent bugs in
IngestAdvSnapshotTask, one security and one correctness.1.
folderescapedSEC_RAW_DATA_FOLDER, and bypassedsnapshotvalidationWhat was wrong.
executedidjoin(root, input.folder)with no check on where that lands, and takinginput.folderalso skippedadvArchiveFolder(input.snapshot)— the only thing that validates the snapshot label — because the call was on the??right-hand side. So both the directory read and the label stamped on every row were caller-controlled.The task is in the curated
SEC_CLI_TASKS(src/config/registerTasks.ts:40), so those inputs are not internal: they arrive fromsec-base task run, the web console's task form, and the MCP tool surface, each carrying the task's own input schema. The siblingBootstrapDownloadTaskguards the identical shape atsrc/task/bootstrap/BootstrapDownloadTask.ts:221-229; the ingest simply omitted it.The concrete failure. Two, actually:
*.csvunder an arbitrary local directory is read whole and written intoadv_row.dataas JSON, then readable back through anyadv_rowquery. A test in this PR demonstrates it against the unfixed code: withfolderpointing outside the root the run returns{ success: true, tables: 1, … }having read a file placed there.executerunsrowRepo.deleteSearch({ snapshot })/adviserRepo.deleteSearch({ snapshot })before reading anything, so a call naming a real snapshot with a folder that turns out to hold nothing wipes that snapshot and then throws. The added test confirms the pre-fix behaviour drops all three rows of an already-ingested2026-06and reports "No CSV members under /".What changed.
advArchiveFolder(input.snapshot)is now called unconditionally, so the label is validated whether or notfolderoverrides the read path; the directory isresolved and prefix-checked againstresolve(root) + sep, the same guard and the same message shapeBootstrapDownloadTaskuses. Both checks run before the deletes.2.
adv_adviserkept whichever base filing came last in the CSVWhat was wrong. The primary key is
(snapshot, crd_number)(src/storage/adv/AdvAdviserSchema.ts:42), but the archive's base-filing member is one row per filing.ADV_CUMULATIVE_SNAPSHOT = "2011-2024"stamps thirteen years of filings with one label, so every annual amendment an adviser filed in that window collided on one key. Nothing ordered them: the dedupe that actually ran wasrunBulkPut's last-write-wins within a batch plusINSERT OR REPLACEacross batches — i.e. CSV position.date_submittedwas read and then never used to compare.The concrete failure.
sec load download adv && sec load ingest advreportsadvisers: <base rows pushed>whileadv_adviserholds one arbitrary historical row per CRD.sec show advisers --min-aum 1000000000then filters on, say, a 2013 AUM figure presented as the adviser's current description — a wrong answer with no signal that it is wrong. Nothing throws. The monthly path has the same shape at smaller scale (an adviser amending twice in one month).What changed. Base rows accumulate into a
Mapkeyed by CRD across all base members, and the later filing wins:date_submittedfirst,filing_idas tie-break (compared numerically where both parse — ADV writes the id unpadded), and an undated filing never displaces a dated one. The write happens once after every member, because the IA and ERA base members are one adviser set split in two and either can hold the later filing. The reportedadvisers:count is nowlandedAdvisers.length— rows that landed, not rows pushed at the key.Per-filing history is untouched:
adv_rowstill keeps every base row verbatim, which is where that time series already lived.Tests
Five new cases in
src/task/adv/IngestAdvSnapshotTask.test.ts. All five were run against the unfixed code first and all five failed (newest-per-CRD countexpected 2 to be 1; undated row winning by position; the traversal returningsuccess: trueafter reading an outside CSV; the bad snapshot label ingesting anyway; and the wipe-then-throw). All pass after.Verified
bunx vitest run src/task/adv/— 21 passedbunx vitest run(full suite) — 150 files passed, 3 skipped; 1488 tests passed, 21 skippedbun run typecheck— cleanbun run lint— cleanbun run format— applied, no further changesbun run build— both binaries bundleNothing was skipped. All checks were run in a fresh worktree after
bun install.Deliberately not changed
The reviewer's alternative — putting
filing_idin the primary key instead ofsnapshot— would change whatadv_advisermeans and whatsec show advisersreturns. The schema's own doc already describes the table as "one investment adviser, as its latest Form ADV Part 1A base filing describes it", so deduplicating to that is the fix that matches the declared contract; typed per-filing history would be a separate table and a separate change.🤖 Generated with Claude Code
https://claude.ai/code/session_01T8DU24G9GWuRUJncc5TJFZ
Generated by Claude Code