fix(xbrl): XbrlFactRepo.replaceForAccession no-ops on 0 rows unless intentionalClear#177
Closed
sroussey wants to merge 1 commit into
Closed
fix(xbrl): XbrlFactRepo.replaceForAccession no-ops on 0 rows unless intentionalClear#177sroussey wants to merge 1 commit into
sroussey wants to merge 1 commit into
Conversation
…ntentionalClear
A re-extract that legitimately yields 0 facts (parser regression, XBRL
disabled, or a degrade path returning `[]`) would otherwise wipe every
prior fact for the accession. The current sole caller
(`s1/xbrlEnrichment.ts`) early-returns `NO_XBRL` before touching the
repo, but the hole is one refactor away.
Move the invariant into the repo: `replaceForAccession` refuses a 0-row
input with a `console.warn` and returns, unless the caller opts in via
`{ intentionalClear: true }`. Add `clearForAccession` as the explicit
purge path for operator-driven resets.
Tests cover no-op-on-empty, opt-in clear, non-empty replacement, and
explicit clear.
Contributor
Author
|
Integrated into #176 (the consolidated hardening PR). The Generated by Claude Code |
sroussey
pushed a commit
that referenced
this pull request
Jul 2, 2026
Correctness: - parseDate: restore the literal year via setUTCFullYear before the calendar probe. Date.UTC remaps years 0-99 to 1900-1999, so a valid 4-digit year like '0099-01-01' was wrongly rejected as an invalid calendar date. Regression test added; Feb-30-style rollover detection is unaffected. Resilience regression from #178's stricter parseDate throw: - FetchQuarterlyIndexTask / FetchQuarterlyFormIdxTask: wrap the per-row secDate() in try/catch so a single calendar-invalid EDGAR 'Date Filed' row is skipped+warned instead of aborting the whole quarter's batch (parseDate now throws where it previously rolled forward). Cleanup: - XbrlFactRepo.clearForAccession delegates to replaceForAccession(acc, [], { intentionalClear: true }), removing a byte-identical duplicate delete loop. Skipped (documented in review): the 4-way junction-repo copy-paste (would need a shared CanonicalJunctionRepo base — larger refactor beyond this diff); the CLI --date throw (operator fail-fast is acceptable); wiring clearForAccession into xbrlEnrichment (pre-existing behavior #177 deliberately left unchanged); and 5 concurrency observations that are pre-existing #175 reap-path limitations or documented single-process scope, not wave-2 regressions.
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
2 participants
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.
Summary
XbrlFactRepo.replaceForAccessiondeletes any prior fact for the accession whosefact_indexis not in the incoming set. When the incoming set is empty, that means every prior fact — a legitimate re-extract that yields 0 facts (parser regression, XBRL disabled, or a degrade path returning[]) silently wipes the filing.The sole production caller (
src/sec/forms/registration-statements/s1/xbrlEnrichment.ts) early-returnsNO_XBRLbefore touching the repo, so the wipe is not currently reachable in prod — but the invariant lives in the caller instead of the repo, and one refactor away from re-appearing.Fix
Move the invariant into the repo:
replaceForAccession(accession_number, rows, opts?)— refuses a 0-row input withconsole.warnand returns. Callers that genuinely want to purge (an operator forcing a reset) pass{ intentionalClear: true }.clearForAccession(accession_number)— new explicit purge path.Existing call sites all pass non-empty rows and need no changes; behavior is unchanged for them.
Tests
Added to
src/storage/xbrl/XbrlFactRepo.test.ts:intentionalClear(spiesconsole.warn, seeds 2 facts, asserts 2 remain).intentionalClear: true.clearForAccessionremoves all facts for the target accession only.Test plan
bun test src/storage/xbrl/— 8 pass (4 new + 4 existing)bun test src/sec/forms/registration-statements/s1/— 72 passbun test src/cli/queries/XbrlQuery.test.ts— 5 passnpx tsc --noEmit— cleanGenerated by Claude Code