fix(cli): expand ~ in path arguments outside scan - #219
Conversation
The scan subcommand already expands a leading ~ in its path arguments via targets.ts's expandHome helper, but every other subcommand that takes a user-supplied path resolved it with a bare resolve(), so ~ was treated as a literal directory name (e.g. codex-security bulk-scan repos.csv --output-dir ~/security-scans created a stray literal ./~/security-scans directory instead of writing under $HOME). Add a resolveCliPath(directory, value) wrapper around the existing expandHome helper (imported from runtime.js) and use it at every affected call site: - scans list <repository> and --scan-root - install-hook <repository> - bulk-scan <input-csv> and --output-dir - export <scan-dir>, --output, and --source-root - the output directory prompted for in the interactive bulk-scan wizard (bulk-scan-discovery.ts), via a direct expandHome import scan's own repository/--path resolution, scan/bulk-scan's --output-dir, --plugin-path, and --python were verified to already expand ~ deeper in targets.ts/runtime.ts and did not need changes. Fixes openai#57
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25349d19b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| inputPath = resolveCliPath(currentDirectory, args.input); | ||
| outputDir = resolveCliPath(currentDirectory, options.outputDir); |
There was a problem hiding this comment.
Expand bulk-scan knowledge-base paths too
When a shell passes ~ literally, bulk-scan ... --knowledge-base ~/docs still forwards the option unchanged as knowledgeBasePaths. prepareKnowledgeBase() subsequently calls resolve(requested) without home expansion, so it looks for <cwd>/~/docs and the repository scans fail even though the CSV and output-directory arguments now resolve correctly. Apply the same home expansion to each bulk-scan knowledge-base path before invoking runMultiscan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right about the bug, but it belongs one layer down, and it is already fixed in #254 — including this exact bulk-scan path.
The bug reproduces exactly as described. On this branch:
ENOENT: no such file or directory, lstat '<cwd>/~/docs'
at prepareKnowledgeBase (src/knowledge-base.ts:41)
Why not patch it at the bulk-scan call site. --knowledge-base is not one entry point, it is three, and all three converge on the same consumer:
scan --knowledge-base—cli.ts:1143→runScan→cli.ts:2694→ScanOptionsbulk-scan --knowledge-base—cli.ts:1406→runMultiscan→multiscan.ts:185→security.runscans resumefrom a saved recipe —cli.ts:1827/cli.ts:1918→runScan→cli.ts:2694
All three land in api.ts:385, prepareKnowledgeBase(options.knowledgeBasePaths, signal), and nothing between the CLI and that call transforms the strings. I measured that rather than assuming it: driving main(["bulk-scan", …, "--knowledge-base", "~/docs"]) with a stubbed createSecurity shows security.run receiving knowledgeBasePaths: ["~/docs"] verbatim, and the committed multiscan.test.ts assertion at line 356 already pins that passthrough by round-tripping a relative path unchanged.
So expanding at cli.ts:1406 would fix exactly one of the three, and would leave scan --knowledge-base ~/docs and resume-from-recipe broken in the same way. That also cuts against the rule this PR already follows: where a path is already expanded deeper in the stack (validateOutputDir, resolvePluginPath, usablePython in runtime.ts; expandHome in targets.ts), the CLI does not expand it a second time.
#254 fixes it at that choke point — src/knowledge-base.ts line 40, resolve(requested) → resolve(expandHome(requested)). To confirm it genuinely covers the bulk-scan path and not just scan, I applied that one-line change to this branch's tree and re-ran the reproduction: prepareKnowledgeBase(["~/docs"]) went from the ENOENT above to resolving under $HOME, with no change to cli.ts or multiscan.ts. Then I reverted it, since it is #254's change to land.
No code change here. Full suite on this branch after the merge of main: 743 pass / 5 skip / 0 fail. This PR's own five regression tests are revert-proof — neutralising resolveCliPath's expandHome and the wizard's expandHome takes the four affected files from 127 pass / 0 fail to 122 pass / 5 fail, failing exactly the five expands ~ … tests and nothing else. types and format are clean.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Fixes #57
Problem
codex-security scanexpands a leading~in its path arguments, but every other subcommand called bareresolve()on user-supplied paths, so~was treated as a literal directory segment:So:
$ codex-security bulk-scan repos.csv --output-dir ~/security-scanswrites to
./~/security-scansand leaves a stray directory named~in the working directory. Beyond writing to the wrong place, resuming a bulk scan from a different working directory then fails to find the earlier ledger, because the literal~path does not resolve consistently.This affects any shell that does not pre-expand
~in a program's arguments, which is how the issue was hit on Windows.Change
A small wrapper next to the existing helper, as suggested in the issue:
applied at the call sites that genuinely lacked expansion:
scans list <repository>and--scan-root(two sites)install-hook <repository>bulk-scan <input-csv>and--output-direxport <scan-dir>,--output,--source-rootsrc/bulk-scan-discovery.tssrc/bulk-scan-discovery.tsimportsexpandHomefrom./runtime.jsdirectly.runtime.tsimports only node builtins,./config.js(type-only) and./trusted-executable.js, so this adds no import cycle.Why not expand at every site that takes a path
I checked each reported site against the code rather than taking the list as given, and three groups already expand:
scan's repository and--patharguments, throughexpandHomeinsrc/targets.ts(lines 122 and 243-245).--output-dir(forscan),--plugin-pathand--python, throughvalidateOutputDir,resolvePluginPathandusablePythoninsrc/runtime.ts(lines 690, 1221, 2192).CODEX_HOME, already handled atcli.ts:467.Expanding those a second time in the CLI would be redundant, and would put two places in charge of one rule.
Why not expand
bulk-scan --knowledge-basein the CLIReview raised that
bulk-scan --knowledge-base ~/docsstill fails. It does, and it is a real bug — but--knowledge-basehas three entry points, not one, and they all converge on a single consumer:scan --knowledge-base—cli.ts:1143→runScan→cli.ts:2694bulk-scan --knowledge-base—cli.ts:1406→runMultiscan→multiscan.ts:185→security.runscans resumefrom a saved recipe —cli.ts:1827/cli.ts:1918→runScan→cli.ts:2694All three reach
prepareKnowledgeBase()insrc/knowledge-base.ts, which is the only place these strings are turned into filesystem paths; nothing in between transforms them. Expanding at thebulk-scancall site would fix one of the three and leave the other two broken, so the fix belongs at that choke point. #254 makes it there, in one line. That keeps this PR to the sites the issue is about.Why not also change
validateandpatchBoth take a positional argument that is either literal finding text or a file path, and a stat miss safely falls back to treating it as text. Neither is in the reported list, and neither has the stray-directory failure mode, so widening scope there would add risk without fixing a reported bug.
Impact, stated plainly
Path arguments to
scans list,install-hook,bulk-scanandexportnow behave the way the same arguments already behave forscan. On shells that pre-expand~— the common POSIX case — nothing changes, because the CLI never sees a~. On Windows and anywhere else the shell passes~through, these commands stop creating a literal~directory and write where the user asked.The one behaviour this removes: a path argument can no longer address a directory literally named
~in the working directory. That is the intended trade and matches both shell semantics and whatscanalready does.bulk-scan --knowledge-base ~/docsis still broken on this branch. It is fixed in #254.Verification
Five regression tests, in
cli.test.ts,cli-workbench.test.ts,cli-export.test.tsandbulk-scan-discovery.test.ts.They are revert-proof, measured both ways. Neutralising the fix — dropping
expandHomefromresolveCliPathand from the wizard prompt, leaving every call site untouched — takes those four files from 127 pass / 0 fail to 122 pass / 5 fail, failing exactly the fiveexpands ~ …tests and nothing else. Restoring the fix returns them to 127 pass / 0 fail.Full suite, after the merge of
maininto this branch: 743 pass / 5 skip / 0 fail across 34 files.pnpm run typesandpnpm run formatare clean.The tests stub
node:oswithmock.modulerather than mutatingprocess.env.HOME, becauseos.homedir()does not re-read a live-mutatedHOMEin-process under Bun. That mirrors the existingmock.moduleusage intests-ts/api.test.ts.