fix(cli): use shell-neutral guidance for overriding API key variables - #216
fix(cli): use shell-neutral guidance for overriding API key variables#216rohanpoudel2 wants to merge 5 commits into
Conversation
`codex-security login status` and `codex-security login` printed
POSIX-only `unset OPENAI_API_KEY CODEX_API_KEY` guidance when an
environment API key overrides (or would override) a stored ChatGPT
sign-in. `unset` is not a valid PowerShell command, so Windows users
received advice they could not act on.
Both call sites now describe the fix in shell-neutral terms ("remove
... from the environment, then run the command again") instead of
naming a POSIX shell built-in. The dynamic variant, which lists
whichever of OPENAI_API_KEY/CODEX_API_KEY are actually set, still
reads naturally for one or two variables.
Fixes openai#33
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
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. |
Both READMEs gave Windows readers a documented path for setting an environment API key but not for clearing one. `sdk/typescript/README.md` has a dedicated PowerShell block for `$env:OPENAI_API_KEY`, while the instructions for making a stored ChatGPT sign-in the default offered only a bash `unset OPENAI_API_KEY CODEX_API_KEY`. `unset` is not a PowerShell command, so the docs repeated the asymmetry openai#33 reported in the CLI's runtime guidance. Add a PowerShell block next to each POSIX one, mirroring the existing "On Windows, set the API key in PowerShell" pattern, and describe the step as removing the variables from the environment so the prose matches the shell-neutral wording the CLI now prints. Refs openai#33
slegarraga
left a comment
There was a problem hiding this comment.
Reviewed the diff. The CLI guidance is now shell-neutral, the README documents PowerShell removal, and the tests assert that POSIX-only commands no longer leak into status or login output. The change stays scoped and looks correct.
Fixes #33
Problem
When an API-key environment variable overrides a stored ChatGPT sign-in, the CLI suggests
unset, which PowerShell does not provide. Windows users are told to run a command that does not exist, precisely when they are trying to resolve credential precedence.There are two call sites in
src/cli.ts, both reported in #33:login status, which always names both variables:the
loginwarning that a configured environment API key will keep taking precedence, which names only the variables actually set:The same asymmetry appears in the docs.
sdk/typescript/README.mdgives Windows readers a PowerShell block for setting$env:OPENAI_API_KEY, but the instructions for making a stored ChatGPT sign-in the default offer only a bashunset OPENAI_API_KEY CODEX_API_KEY. Windows readers get a documented path in and no documented path out.Change
Both CLI messages now use shell-neutral wording, following the direction suggested in the issue:
Neither message names a shell built-in, so there is no shell in which it is wrong. Variable-name casing is passed through unchanged, as before.
The second message builds its variable list at runtime and names whichever of
OPENAI_API_KEY/CODEX_API_KEYare actually set, so the separator changed from" "to" and ". The environment lookup is case-insensitive (environmentApiKeyEntryinsrc/api.ts), so on a case-sensitive OS the list can in principle exceed two names and read "A and B and C". The message stays accurate and actionable in that case, and a general list formatter is not worth adding for it.Separately, both READMEs now carry a PowerShell block next to the existing bash one, mirroring the existing "On Windows, set the API key in PowerShell" pattern:
Why not platform-detected guidance
#33 suggests a formatting helper with an injected platform value, printing
unseton POSIX andRemove-Item Env:on Windows. That is more machinery for a worse result:process.platformdescribes the host, not the shell. It is wrong for PowerShell on Linux and macOS, for Git Bash, WSL, and MSYS on Windows, and for fish anywhere (fish has nounset; it usesset -e). Prose that names no shell is correct in all of them, and needs no injected platform to stay testable.Why not naming every shell inline
Spelling out
unset/Remove-Item Env:/set -e/set VAR=in one stderr line turns a two-line diagnostic into a shell cheat sheet, and still omits whichever shell the reader is using. The README is the right place for concrete per-shell commands; the CLI diagnostic states the goal.Impact, stated plainly
Message text only. No behavioural, exit-code, or credential-precedence change: the conditions that select each message, the
scanAuthenticationresult, and the exit codes are untouched. Anything grepping stderr for the literalunsetguidance will stop matching — that is the point of the fix, and the new test enforces it.Verification
tests-ts/cli-authentication.test.tsare updated to the new wording.uses shell-neutral guidance when an API key overrides the stored login, asserts that stderr fromlogin statusand fromlogin(with one and with two variables set) never matches/\bunset\b/,/\bexport\s+\w+=/, or/\$env:/i, so this cannot silently regress into a single-shell dialect.Measured on this branch:
Reverting only the
loginmessage (leavinglogin statusfixed) still fails the new test on its own, so it covers both call sites independently, not just the first one it reaches.pnpm run typesandpnpm run formatare clean.The PowerShell block is verified by reference to the
Remove-Item/Env:provider contract, not executed — no PowerShell is available in the environment these checks ran in.Left alone deliberately
#!/bin/shwrapper the CLI generates for the Git hook is a real POSIX script, not user-facing shell advice.src/api.tscredential errors already say "setOPENAI_API_KEYorCODEX_API_KEY" without naming a shell built-in, so they need no change.unset OPENAI_API_KEY CODEX_API_KEYin its explicitly labelledbashfence; the PowerShell block is added beside it rather than replacing it.cmd.exeis not covered, matching the existing house pattern of documenting Windows via PowerShell only.