fix(cli): don't provision STORAGE_ENCRYPTION_KEY on informational commands - #3129
Conversation
… commands Running any CLI command — even `omniroute --version` or `--help` — generated a 32-byte STORAGE_ENCRYPTION_KEY and created `~/.omniroute/.env` (or DATA_DIR/.env). A read-only command should never mutate the data dir. Gate the provisioning behind shouldProvisionStorageKey(): skip for --version/--help/help/completion and bare invocations; still provision for real commands (serve, keys, …) so the encryption key persists before storage is accessed (#1622).
There was a problem hiding this comment.
Code Review
This pull request introduces a utility function shouldProvisionStorageKey to prevent the CLI from provisioning a storage encryption key and creating a .env file during purely informational invocations (such as --help, --version, or help). This utility is integrated into bin/omniroute.mjs and verified with unit tests. The reviewer identified an issue where global options specified before an informational command (e.g., --lang en help) would bypass the check and incorrectly trigger key provisioning. The reviewer suggested a more robust argument parsing implementation and an accompanying test case to address this edge case.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function shouldProvisionStorageKey(argv) { | ||
| const args = Array.isArray(argv) ? argv.slice(2) : []; | ||
| if (args.length === 0) return false; | ||
| if (args.some((a) => INFO_FLAGS.has(a))) return false; | ||
| if (INFO_COMMANDS.has(args[0])) return false; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The current implementation only checks args[0] to identify informational commands. However, if global options (such as --lang <lang> or --output <format>) are specified before the command (e.g., omniroute --lang en help), args[0] will be "--lang" instead of "help". This causes the function to return true and incorrectly provision a storage key for an informational command.
We should parse the arguments more robustly by skipping known options and their arguments to find the actual command.
export function shouldProvisionStorageKey(argv) {
const args = Array.isArray(argv) ? argv.slice(2) : [];
if (args.length === 0) return false;
if (args.some((a) => INFO_FLAGS.has(a))) return false;
// Find the first positional argument (the command) by skipping options
let command = null;
for (let i = 0; i < args.length; i++) {
const arg = args[i];
if (arg.startsWith("-")) {
// Skip the option's argument if it takes one
if (arg === "--lang" || arg === "--output") {
i++;
}
continue;
}
command = arg;
break;
}
if (!command || INFO_COMMANDS.has(command)) return false;
return true;
}| test("storage key: --help/--version anywhere in the args still skips", () => { | ||
| assert.equal(shouldProvisionStorageKey(argv("serve", "--help")), false); | ||
| assert.equal(shouldProvisionStorageKey(argv("keys", "list", "-h")), false); | ||
| assert.equal(shouldProvisionStorageKey(argv("--lang", "en", "--version")), false); | ||
| }); |
There was a problem hiding this comment.
Add a test case to verify that informational commands like help are correctly skipped even when global options (such as --lang) are passed before them.
| test("storage key: --help/--version anywhere in the args still skips", () => { | |
| assert.equal(shouldProvisionStorageKey(argv("serve", "--help")), false); | |
| assert.equal(shouldProvisionStorageKey(argv("keys", "list", "-h")), false); | |
| assert.equal(shouldProvisionStorageKey(argv("--lang", "en", "--version")), false); | |
| }); | |
| test("storage key: --help/--version anywhere in the args still skips", () => { | |
| assert.equal(shouldProvisionStorageKey(argv("serve", "--help")), false); | |
| assert.equal(shouldProvisionStorageKey(argv("keys", "list", "-h")), false); | |
| assert.equal(shouldProvisionStorageKey(argv("--lang", "en", "--version")), false); | |
| assert.equal(shouldProvisionStorageKey(argv("--lang", "en", "help")), false); | |
| }); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c749037eb
ℹ️ 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".
| * @param {string[]} argv - process.argv (node + script + args). | ||
| * @returns {boolean} | ||
| */ | ||
| const INFO_FLAGS = new Set(["-h", "--help", "-V", "--version"]); |
There was a problem hiding this comment.
Add the actual -v version flag
When users run omniroute -v, Commander treats it as the version-only command because bin/cli/program.mjs registers .version(..., "-v, --version", ...), but this helper only recognizes -V. In that scenario shouldProvisionStorageKey() returns true, so the read-only version command still creates DATA_DIR/.env and a storage key, which is the side effect this change is meant to prevent.
Useful? React with 👍 / 👎.
| const args = Array.isArray(argv) ? argv.slice(2) : []; | ||
| if (args.length === 0) return false; | ||
| if (args.some((a) => INFO_FLAGS.has(a))) return false; | ||
| if (INFO_COMMANDS.has(args[0])) return false; |
There was a problem hiding this comment.
Detect help subcommands after global options
This only skips help/completion when they are the first CLI token, but Commander allows global options before the subcommand, e.g. omniroute --lang en help or omniroute --output json completion. Those informational invocations currently return true here and provision DATA_DIR/.env, so users can still get the unwanted mutation whenever they combine global options with help/completion.
Useful? React with 👍 / 👎.
…PTION_KEY My #3129 gate wrongly skipped provisioning for a bare `omniroute` invocation — but `serve` is isDefault:true, so bare runs the server, which needs the key. Only --version/--help/help/completion skip now. Realigns with #1622: its bootstrap test invoked `--help` (now correctly skipped), so it's switched to `config list --json` (a real, fast, offline command) to exercise the provisioning path.
… commands (diegosouzapw#3129) Running any CLI command — even `omniroute --version` or `--help` — generated a 32-byte STORAGE_ENCRYPTION_KEY and created `~/.omniroute/.env` (or DATA_DIR/.env). A read-only command should never mutate the data dir. Gate the provisioning behind shouldProvisionStorageKey(): skip for --version/--help/help/completion and bare invocations; still provision for real commands (serve, keys, …) so the encryption key persists before storage is accessed (diegosouzapw#1622).
…ors hall Adds entries for diegosouzapw#3097, diegosouzapw#3101 (deepseek-web diegosouzapw#2942/diegosouzapw#2820), diegosouzapw#3104, diegosouzapw#3105, diegosouzapw#3107, diegosouzapw#3109, diegosouzapw#3111, diegosouzapw#3113, diegosouzapw#3115, diegosouzapw#3122, diegosouzapw#3125, diegosouzapw#3127, diegosouzapw#3129, plus a Contributors section crediting all v3.8.9 contributors. Stamps the 3.8.9 release date.
…PTION_KEY My diegosouzapw#3129 gate wrongly skipped provisioning for a bare `omniroute` invocation — but `serve` is isDefault:true, so bare runs the server, which needs the key. Only --version/--help/help/completion skip now. Realigns with diegosouzapw#1622: its bootstrap test invoked `--help` (now correctly skipped), so it's switched to `config list --json` (a real, fast, offline command) to exercise the provisioning path.
… commands (diegosouzapw#3129) Running any CLI command — even `omniroute --version` or `--help` — generated a 32-byte STORAGE_ENCRYPTION_KEY and created `~/.omniroute/.env` (or DATA_DIR/.env). A read-only command should never mutate the data dir. Gate the provisioning behind shouldProvisionStorageKey(): skip for --version/--help/help/completion and bare invocations; still provision for real commands (serve, keys, …) so the encryption key persists before storage is accessed (diegosouzapw#1622).
…ors hall Adds entries for diegosouzapw#3097, diegosouzapw#3101 (deepseek-web diegosouzapw#2942/diegosouzapw#2820), diegosouzapw#3104, diegosouzapw#3105, diegosouzapw#3107, diegosouzapw#3109, diegosouzapw#3111, diegosouzapw#3113, diegosouzapw#3115, diegosouzapw#3122, diegosouzapw#3125, diegosouzapw#3127, diegosouzapw#3129, plus a Contributors section crediting all v3.8.9 contributors. Stamps the 3.8.9 release date.
…PTION_KEY My diegosouzapw#3129 gate wrongly skipped provisioning for a bare `omniroute` invocation — but `serve` is isDefault:true, so bare runs the server, which needs the key. Only --version/--help/help/completion skip now. Realigns with diegosouzapw#1622: its bootstrap test invoked `--help` (now correctly skipped), so it's switched to `config list --json` (a real, fast, offline command) to exercise the provisioning path.
… commands (diegosouzapw#3129) Running any CLI command — even `omniroute --version` or `--help` — generated a 32-byte STORAGE_ENCRYPTION_KEY and created `~/.omniroute/.env` (or DATA_DIR/.env). A read-only command should never mutate the data dir. Gate the provisioning behind shouldProvisionStorageKey(): skip for --version/--help/help/completion and bare invocations; still provision for real commands (serve, keys, …) so the encryption key persists before storage is accessed (diegosouzapw#1622).
…ors hall Adds entries for diegosouzapw#3097, diegosouzapw#3101 (deepseek-web diegosouzapw#2942/diegosouzapw#2820), diegosouzapw#3104, diegosouzapw#3105, diegosouzapw#3107, diegosouzapw#3109, diegosouzapw#3111, diegosouzapw#3113, diegosouzapw#3115, diegosouzapw#3122, diegosouzapw#3125, diegosouzapw#3127, diegosouzapw#3129, plus a Contributors section crediting all v3.8.9 contributors. Stamps the 3.8.9 release date.
…PTION_KEY My diegosouzapw#3129 gate wrongly skipped provisioning for a bare `omniroute` invocation — but `serve` is isDefault:true, so bare runs the server, which needs the key. Only --version/--help/help/completion skip now. Realigns with diegosouzapw#1622: its bootstrap test invoked `--help` (now correctly skipped), so it's switched to `config list --json` (a real, fast, offline command) to exercise the provisioning path.
Problem
Running any CLI command — including read-only ones like
omniroute --versionandomniroute --help— generated a 32-byteSTORAGE_ENCRYPTION_KEYand created~/.omniroute/.env(orDATA_DIR/.env). The key-provisioning block ran unconditionally at CLI entry, before Commander parsed the command. A read-only command should never mutate the data dir.Fix
Gate provisioning behind a pure, unit-tested predicate
shouldProvisionStorageKey(argv):omniroute,--version/-V,--help/-h(anywhere in args), and thehelp/completionsubcommands.serve,keys,providers, …) so the encryption key is persisted before encrypted storage is accessed — preserving the [BUG] 3.7.0 install regenerates .env (STORAGE_ENCRYPTION_KEY/JWT_SECRET/API_KEY_SECRET) → all stored credentials become un-decryptable, every chat 401, every OAuth refresh 400 invalid_grant #1622 fix.Verification
omniroute --versionin an isolatedDATA_DIRno longer creates.env(was:✨ Generated STORAGE_ENCRYPTION_KEY).omniroute keys liststill provisions the key.tests/unit/cli-storage-key-provision.test.ts(4 cases) covers info commands,--helpanywhere, real commands, and--lang en serve.🤖 Generated with Claude Code