fix(sec): tighten parseIntOption + fix bundled import.meta.dir asset paths#206
Open
sroussey wants to merge 2 commits into
Open
fix(sec): tighten parseIntOption + fix bundled import.meta.dir asset paths#206sroussey wants to merge 2 commits into
sroussey wants to merge 2 commits into
Conversation
parseInt silently accepted values like "123abc" (→ 123) and "1e5" (→ 1) and handed a plausible-but-wrong CIK to downstream tasks, so queries returned rows for a different issuer. The ad-hoc /^\d+$/ trims scattered above the query tasks caught it for the ones I noticed, but the primary parser was still the hole. Tightening parseIntOption to reject anything that isn't an all-digit, safe-integer string closes it for every commander option in one place and lets the query commands drop their bespoke guards. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UC9LGu3iokhuhAB46yt1ee
…te-side cache paths
`import.meta.dir` in dev points at `src/…/`, but after `bun build` it points
at `dist/…/` — so:
- the CSV / HTML fixtures the eval harness reads weren't in `dist/` at all
(never copied by the build), and `sec eval unit-terms` / `sec eval s1`
on the packaged binary failed to load them; and
- `sec fetch fixtures` / `sec fetch s1-fixtures` were writing their
"gitignored cache" into the packaged binary's directory instead of
somewhere in the user's project.
`resolveAsset` centralizes the dev-vs-dist candidate walk for read-only
assets, the build now copies the fixture trees into `dist/`, and the two
fetch tasks resolve their output under `SEC_FIXTURES_DIR ?? cwd` and log
where they landed so the user can find them.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UC9LGu3iokhuhAB46yt1ee
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
parseIntOption silently accepts non-integers. The commander parser used
parseInt(value, 10), which happily returned123for"123abc"and1for"1e5"— the CLI then handed a plausible-but-wrong CIK to downstream tasks, sosec query facts 123abcandsec query xbrl --cik 1e5returned rows for a different issuer than the user asked about. The two known callsites (sec query facts <cik>andsec query xbrl --cik) had ad-hoc/^\d+$/guards duct-taped on top; every other numeric option (year, limit, offset, sic, portal…) was still exposed. This tightensparseIntOptionat the parser boundary — trim, require all-digit +Number.isSafeInteger, throw commander'sInvalidArgumentErrorwith the original value — and drops the redundant per-command guards. Every commander option usingparseIntOptionis now protected in one place.Bundled asset paths broke on the packaged binary.
import.meta.dirresolves tosrc/…/in dev but todist/…/afterbun build, so on the shippedsecbinary the eval harness'smock_data/embarc-spac-unit-terms.csvand committed S-1 HTML weren't found at all (the build script never copied them intodist/), and — worse — thesec fetch fixtures/sec fetch s1-fixtures"gitignored cache" was silently writing back into the packaged binary's own directory. This introducesresolveAsset()for the dev-vs-dist candidate walk on read-only fixtures, adds acopy-assetsbuild step that stages both fixture trees intodist/, and reroutes the two fetch tasks toSEC_FIXTURES_DIR ?? cwd— logging where output landed so the user can find it.realSections.tsalso picks up a documented 4-tier resolution order (--dir→SEC_S1_MOCK_DIRenv → dist candidate → source candidate) and lists every candidate in its not-found error.Verification
bun run build— clean;dist/eval/mock_data/embarc-spac-unit-terms.csvanddist/sec/html/mock_data/s1/*.htmpresent after build.npx tsc --noEmit— clean (this repo has notypesscript; equivalent).bun test src/cli/— 273 pass, 0 fail (includes the newparseIntOptionblock: 3 accept cases + 9 reject cases, plus the pre-existingquery.test.tsguards updated for the new error message).bun test src/eval/— 40 pass, 0 fail.bun test src/util/— 50 pass, 0 fail.bun test src/task/fixtures/— 14 pass, 0 fail.Notes
SEC_FIXTURES_DIRandSEC_S1_MOCK_DIRare documented under Environment Variables inCLAUDE.md, and.sec-fixtures/is added to.gitignoreso the default (cwd-relative) output stays untracked.embarcUnitTermsReference.tsresolves its CSV lazily insideloadEmbarcUnitTermsReference()rather than at module-load time, so a missing asset doesn't crash import for code paths that don't need the eval reference.parseIntOptionargument validation andimport.meta.dirasset-path resolution surfaced in review.🤖 Generated with Claude Code
https://claude.ai/code/session_01UC9LGu3iokhuhAB46yt1ee
Generated by Claude Code