fix(knowledge-base): expand ~ in --knowledge-base paths - #254
Open
rohanpoudel2 wants to merge 2 commits into
Open
fix(knowledge-base): expand ~ in --knowledge-base paths#254rohanpoudel2 wants to merge 2 commits into
rohanpoudel2 wants to merge 2 commits into
Conversation
prepareKnowledgeBase() called resolve() on each requested path, so a literal ~/docs resolved against the current working directory and the scan failed with ENOENT on <cwd>/~/docs before any work started. It is reachable from a quoted argument and from anything that does not go through shell word expansion at all: a config file, a CI variable, a Makefile. Expand where the path is resolved, reusing the exported expandHome from src/runtime.ts rather than adding another copy. That is where the sibling path options already expand -- --plugin-path in resolvePluginPath, --python in usablePython, --output-dir in validateOutputDir -- and one site covers every producer: scan --knowledge-base, bulk-scan --knowledge-base, and SDK callers passing knowledgeBasePaths. Replayed scan recipes store realpath values, and expandHome returns an absolute path unchanged, so the recipe path is unaffected.
`mock.module` hot-swaps the live `node:os` namespace object, so
`mock.module("node:os", () => os)` re-installed the already-mocked
`homedir`. The restore was a no-op and every later test in the same bun
process saw `homedir()` pointing at a temporary directory that `afterEach`
had deleted.
Snapshot the original exports before the first mock and restore from the
snapshot, then assert the restore. Also cover bare `~` and `~other/docs`,
which stays literal because another account's home cannot be resolved
portably.
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.
Fixes #247
Problem
prepareKnowledgeBase()resolves each requested path with no home expansion:so a literal
~is treated as a directory segment under the current working directory and the scan fails before any work starts. CallingprepareKnowledgeBase(["~/docs"])from the package root, with a real~/docscontaining one Markdown file:A bare
~/docson the command line is expanded by the shell, so this hides until the value arrives from somewhere that does not do word expansion: a quoted argument, a config file, a CI variable, a Makefile.Change
One line, at the point where the path is resolved:
reusing the exported
expandHomefromsrc/runtime.ts— the same helper--plugin-path,--pythonand--output-dirgo through, including its~\handling for Windows. No new helper, and no import cycle:src/runtime.tsdoes not importsrc/knowledge-base.ts, andsrc/api.tsalready imports both.Why not expand at the CLI option boundary
The repo's convention for path options is to expand inside the consuming function, not in
src/cli.ts:--plugin-pathexpands inresolvePluginPath,--pythoninusablePython,--output-dirinvalidateOutputDir. Expanding at the CLI boundary would also have to be done twice, because — contrary to the issue's "bulk-scanhas no--knowledge-baseoption at all" — it does have one, and it reaches the same function.bulk-scanis covered by this one-line change. The full chain, read from the code on this branch:src/cli.ts:1281defines the option,src/cli.ts:1405passes it torunMultiscanon both the CSV and the wizard branch,src/multiscan.ts:185forwards it verbatim into each per-repositorysecurity.run, andsrc/api.ts:385callsprepareKnowledgeBase. Nothing in between re-resolves or validates the strings, andsrc/api.ts:385is the onlyprepareKnowledgeBasecall site insrc/. Each link already has a test.tests-ts/cli.test.ts:663drives the realmain()withbulk-scan repositories.csv --output-dir results --knowledge-base /shared/architecture.pdf --knowledge-base=/shared/threat-modelsand asserts the per-repository scan options carry both values verbatim, through the realrunMultiscan;tests-ts/multiscan.test.ts:359and:480pin the same forwarding directly; and the new test below covers the expansion itself. Fixing the shared callee coversscan,bulk-scan, and SDK callers passingknowledgeBasePathsdirectly.The third producer is scan-recipe replay, and expansion there is a verified no-op.
src/api.ts:615builds the recipe fromknowledgeBase?.sources, which arerealpath()results and therefore absolute;expandHomereturns any value not equal to~and not starting with~/or~\unchanged, which I confirmed by running it over absolute inputs including/Users/someone/~docsand/Users/someone/docs/~/nested.Why not mutate
process.env.HOMEin the testos.homedir()does not re-read a live-mutatedHOMEin-process under Bun; I measured it — settingprocess.env.HOMEand callinghomedir()in the same test returns the unchanged real home. The test stubsnode:oswithmock.moduleinstead.That stub needs care.
mock.modulehot-swaps the livenode:osnamespace object, so restoring withmock.module("node:os", () => os)re-installs the already-mockedhomedirand leaves it mocked for the rest of the bun process — measured: after that "restore",homedir()still returns the stub, and a second test file loaded afterwards sees the stub too. The test therefore snapshots the original exports before the first mock, restores from the snapshot, and assertsos.homedir()is back to the real value.Impact, stated plainly
--knowledge-base ~/docs(quoted, or supplied by anything other than a shell) now reads<home>/docsinstead of failing with ENOENT, forscanand forbulk-scanalike. Absolute and ordinary relative paths resolve exactly as before, and the symlink, extension, permission and extraction checks are untouched — only the string handed toresolve()changes.~other/docsis deliberately not expanded: there is no portable way to resolve another account's home from Node, so it stays a literal path segment under the working directory and fails with ENOENT naming~other. That matches every other path option in the repo.One behavior change worth naming: a relative path whose first segment is literally
~, i.e. an actual directory named~in the working directory, is now read as the home directory. That is the same trade the other path options already made, and such a directory is almost always the residue of this bug rather than something intentional.scan --dry-runstill echoes--knowledge-basevalues unexpanded, becausepreflight()returnsoptions.knowledgeBasePathsverbatim (src/api.ts:312) and never callsprepareKnowledgeBase. That is pre-existing — a dry run has never validated knowledge-base paths — and is left alone rather than widened into this PR.Recipe replay,
scan --path, and thebulk-scanpositional/--output-dirarguments are unchanged by this PR.Verification
One test in
tests-ts/knowledge-base.test.tscovering~/docs, bare~, an absolute path, and the unexpanded~other/docs, withnode:osrestored from a pre-mock snapshot and the restore asserted.expect()calls),bun test --timeout 30000 ./tests-ts.mock.modulecalls pointed back at the live namespace): 7 pass / 1 fail intests-ts/knowledge-base.test.ts, onexpect(os.homedir()).toBe(realHomeDirectory)receiving the deleted temporary home.pnpm --dir sdk/typescript run types(generate:models --checkplustsc --noEmit) exits 0, andpnpm --dir sdk/typescript run formatreports all matched files use Prettier code style.