Skip to content

fix(adv): bound the ingest folder and keep the newest filing per CRD - #363

Merged
sroussey merged 1 commit into
mainfrom
claude/amazing-fermat-y0d94j-adv-ingest
Sep 9, 2026
Merged

fix(adv): bound the ingest folder and keep the newest filing per CRD#363
sroussey merged 1 commit into
mainfrom
claude/amazing-fermat-y0d94j-adv-ingest

Conversation

@sroussey

@sroussey sroussey commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Two independent bugs in IngestAdvSnapshotTask, one security and one correctness.

1. folder escaped SEC_RAW_DATA_FOLDER, and bypassed snapshot validation

What was wrong. execute did join(root, input.folder) with no check on where that lands, and taking input.folder also skipped advArchiveFolder(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 from sec-base task run, the web console's task form, and the MCP tool surface, each carrying the task's own input schema. The sibling BootstrapDownloadTask guards the identical shape at src/task/bootstrap/BootstrapDownloadTask.ts:221-229; the ingest simply omitted it.

The concrete failure. Two, actually:

  • Every *.csv under an arbitrary local directory is read whole and written into adv_row.data as JSON, then readable back through any adv_row query. A test in this PR demonstrates it against the unfixed code: with folder pointing outside the root the run returns { success: true, tables: 1, … } having read a file placed there.
  • execute runs rowRepo.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-ingested 2026-06 and reports "No CSV members under /".

What changed. advArchiveFolder(input.snapshot) is now called unconditionally, so the label is validated whether or not folder overrides the read path; the directory is resolved and prefix-checked against resolve(root) + sep, the same guard and the same message shape BootstrapDownloadTask uses. Both checks run before the deletes.

2. adv_adviser kept whichever base filing came last in the CSV

What 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 was runBulkPut's last-write-wins within a batch plus INSERT OR REPLACE across batches — i.e. CSV position. date_submitted was read and then never used to compare.

The concrete failure. sec load download adv && sec load ingest adv reports advisers: <base rows pushed> while adv_adviser holds one arbitrary historical row per CRD. sec show advisers --min-aum 1000000000 then 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 Map keyed by CRD across all base members, and the later filing wins: date_submitted first, filing_id as 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 reported advisers: count is now landedAdvisers.length — rows that landed, not rows pushed at the key.

Per-filing history is untouched: adv_row still 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 count expected 2 to be 1; undated row winning by position; the traversal returning success: true after 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 passed
  • bunx vitest run (full suite) — 150 files passed, 3 skipped; 1488 tests passed, 21 skipped
  • bun run typecheck — clean
  • bun run lint — clean
  • bun run format — applied, no further changes
  • bun run build — both binaries bundle

Nothing was skipped. All checks were run in a fresh worktree after bun install.

Deliberately not changed

The reviewer's alternative — putting filing_id in the primary key instead of snapshot — would change what adv_adviser means and what sec show advisers returns. 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

`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
@sroussey
sroussey merged commit 532f61d into main Sep 9, 2026
1 check passed
@sroussey
sroussey deleted the claude/amazing-fermat-y0d94j-adv-ingest branch September 9, 2026 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants