From 840ae79612683c62ec59537e2d920edb2dd77e93 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Thu, 17 Sep 2026 07:02:19 +0700 Subject: [PATCH] fix(cli): read commander's negated --no-* flags as opts. === false (#13322) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commander exposes a negated `--no-x` flag as `opts.x === false`, not as `opts.noX`, so every `--no-*` flag on `chat`, `contexts` and `serve` was being read as undefined and silently ignored. Probe on your head: 5/5 pass in `tests/unit/cli-negated-flags.test.ts` — a real Commander parser with mocked fetch and a temp `DATA_DIR`, exercising all three commands end to end rather than asserting on the parser in isolation. Thanks, @datrixlab. **Batch validation** — boarded with the other 10 PRs of your batch into one worktree cut from `release/v3.8.51`; every PR verified as an ancestor of the combined HEAD before validating. - Focused tests across all 11 PRs: **104/104 pass** on the combined tree. - Gates on the combined tree: `check-changelog-integrity` PASS, `check-complexity` PASS, `check-cognitive-complexity` PASS, `typecheck:core` PASS, `check:open-sse-typecheck` PASS. - `check-file-size` is red, but reproduces with byte-identical line counts on the pure `release/v3.8.51` tip (`open-sse/handlers/imageGeneration.ts` 3304, `open-sse/services/combo/roundRobinCombo.ts` 1221, `open-sse/utils/stream.ts` 3115). Inherited base-red, nothing added by this batch — it is also why this PR's "Fast Quality Gates" check was red. --- bin/cli/commands/chat.mjs | 3 +- bin/cli/commands/contexts.mjs | 4 +- bin/cli/commands/serve.mjs | 3 +- changelog.d/fixes/13322-cli-negated-flags.md | 1 + tests/unit/cli-negated-flags.test.ts | 147 +++++++++++++++++++ 5 files changed, 155 insertions(+), 3 deletions(-) create mode 100644 changelog.d/fixes/13322-cli-negated-flags.md create mode 100644 tests/unit/cli-negated-flags.test.ts diff --git a/bin/cli/commands/chat.mjs b/bin/cli/commands/chat.mjs index f5d6ba6006..9f66e4298a 100644 --- a/bin/cli/commands/chat.mjs +++ b/bin/cli/commands/chat.mjs @@ -79,7 +79,8 @@ export async function runChatCommand(promptArg, opts, cmd) { const data = await response.json(); const text = extractText(data, opts.responsesApi); - if (!opts.noHistory) { + // Commander stores `--no-history` as `history === false`, never as `noHistory`. + if (opts.history !== false && opts.noHistory !== true) { appendHistory({ prompt, model: opts.model, latencyMs, usage: data.usage, response: text }); } diff --git a/bin/cli/commands/contexts.mjs b/bin/cli/commands/contexts.mjs index 5577a08220..865f120b6c 100644 --- a/bin/cli/commands/contexts.mjs +++ b/bin/cli/commands/contexts.mjs @@ -248,7 +248,9 @@ export function registerContexts(program) { .option("--no-secrets", "Omit API keys from export") .action(async (opts, cmd) => { const cfg = loadContexts(); - const out = opts.noSecrets ? redactContextSecrets(cfg) : JSON.parse(JSON.stringify(cfg)); + // Commander stores `--no-secrets` as `secrets === false`, never as `noSecrets`. + const redact = opts.secrets === false || opts.noSecrets === true; + const out = redact ? redactContextSecrets(cfg) : JSON.parse(JSON.stringify(cfg)); const json = JSON.stringify(out, null, 2); if (opts.out) { const { writeFileSync } = await import("node:fs"); diff --git a/bin/cli/commands/serve.mjs b/bin/cli/commands/serve.mjs index 95538e6c20..ff7edb7b98 100644 --- a/bin/cli/commands/serve.mjs +++ b/bin/cli/commands/serve.mjs @@ -281,7 +281,8 @@ export async function runServe(opts = {}) { return runDaemon(serverJs, env, memoryLimit, dashboardPort, apiPort); } - if (opts.noRecovery) { + // Commander stores `--no-recovery` as `recovery === false`, never as `noRecovery`. + if (opts.recovery === false || opts.noRecovery === true) { return runWithoutRecovery( serverJs, env, diff --git a/changelog.d/fixes/13322-cli-negated-flags.md b/changelog.d/fixes/13322-cli-negated-flags.md new file mode 100644 index 0000000000..b4e6216c60 --- /dev/null +++ b/changelog.d/fixes/13322-cli-negated-flags.md @@ -0,0 +1 @@ +- **fix(cli):** `contexts export --no-secrets` now leaves the access tokens and API keys out, and `chat --no-history` and `serve --no-recovery` take effect; all three flags were accepted and ignored ([#13322](https://github.com/diegosouzapw/OmniRoute/pull/13322)) diff --git a/tests/unit/cli-negated-flags.test.ts b/tests/unit/cli-negated-flags.test.ts new file mode 100644 index 0000000000..a392f4d8ad --- /dev/null +++ b/tests/unit/cli-negated-flags.test.ts @@ -0,0 +1,147 @@ +/** + * Commander stores a negated option such as `--no-history` as `history === false`; it never + * sets `noHistory`. `chat --no-history`, `contexts export --no-secrets` and + * `serve --no-recovery` read the `noX` name, so each flag was accepted and did nothing: + * the prompt was still written to cli-history.jsonl, and the export still carried the + * access tokens and API keys. These tests drive the real Commander parser instead of + * hand-building an options object Commander never produces. + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { Command } from "commander"; + +const FAKE_RESPONSE = { + id: "chatcmpl-abc", + model: "claude-sonnet-4-6", + choices: [{ message: { role: "assistant", content: "Hello!" } }], + usage: { prompt_tokens: 5, completion_tokens: 10, total_tokens: 15 }, +}; + +async function withDataDir(prefix: string, fn: (dir: string) => Promise) { + const dir = mkdtempSync(join(tmpdir(), prefix)); + const prevDataDir = process.env.DATA_DIR; + const prevKeychain = process.env.OMNIROUTE_KEYCHAIN_DISABLED; + process.env.DATA_DIR = dir; + process.env.OMNIROUTE_KEYCHAIN_DISABLED = "1"; + try { + await fn(dir); + } finally { + if (prevDataDir === undefined) delete process.env.DATA_DIR; + else process.env.DATA_DIR = prevDataDir; + if (prevKeychain === undefined) delete process.env.OMNIROUTE_KEYCHAIN_DISABLED; + else process.env.OMNIROUTE_KEYCHAIN_DISABLED = prevKeychain; + rmSync(dir, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + } +} + +async function quietly(fn: () => Promise) { + const out = process.stdout.write.bind(process.stdout); + const err = process.stderr.write.bind(process.stderr); + process.stdout.write = () => true; + process.stderr.write = () => true; + try { + await fn(); + } finally { + process.stdout.write = out; + process.stderr.write = err; + } +} + +async function runChat(args: string[]) { + const origFetch = globalThis.fetch; + globalThis.fetch = (() => + Promise.resolve( + new Response(JSON.stringify(FAKE_RESPONSE), { + status: 200, + headers: { "content-type": "application/json" }, + }) + )) as typeof fetch; + try { + const { registerChat } = await import("../../bin/cli/commands/chat.mjs"); + const program = new Command().exitOverride(); + registerChat(program); + await quietly(() => program.parseAsync(["chat", ...args], { from: "user" })); + } finally { + globalThis.fetch = origFetch; + } +} + +test("chat --no-history does not write cli-history.jsonl", async () => { + await withDataDir("chat-no-history-", async (dir) => { + await runChat(["secret prompt", "--no-history"]); + assert.equal(existsSync(join(dir, "cli-history.jsonl")), false); + }); +}); + +test("chat without --no-history still records the exchange", async () => { + await withDataDir("chat-history-", async (dir) => { + await runChat(["keep this"]); + const line = JSON.parse(readFileSync(join(dir, "cli-history.jsonl"), "utf8").trim()); + assert.equal(line.prompt, "keep this"); + }); +}); + +async function exportContexts(dir: string, args: string[]) { + const { saveContexts } = await import("../../bin/cli/contexts.mjs"); + saveContexts({ + currentContext: "remote", + contexts: { + remote: { + baseUrl: "https://omniroute.example", + accessToken: "oma_SECRET_TOKEN", + apiKey: "sk-SECRET-KEY", + }, + }, + }); + const { registerContexts } = await import("../../bin/cli/commands/contexts.mjs"); + const program = new Command().exitOverride(); + registerContexts(program); + const outFile = join(dir, "export.json"); + await quietly(() => + program.parseAsync(["contexts", "export", "--out", outFile, ...args], { from: "user" }) + ); + return readFileSync(outFile, "utf8"); +} + +test("contexts export --no-secrets leaves the token and API key out", async () => { + await withDataDir("ctx-no-secrets-", async (dir) => { + const json = await exportContexts(dir, ["--no-secrets"]); + assert.doesNotMatch(json, /oma_SECRET_TOKEN|sk-SECRET-KEY/); + assert.equal(JSON.parse(json).contexts.remote.baseUrl, "https://omniroute.example"); + }); +}); + +test("contexts export without --no-secrets keeps the full config", async () => { + await withDataDir("ctx-secrets-", async (dir) => { + const json = await exportContexts(dir, []); + assert.match(json, /sk-SECRET-KEY/); + }); +}); + +test("serve --no-recovery reaches runServe as recovery === false", async () => { + const { registerServe } = await import("../../bin/cli/commands/serve.mjs"); + const program = new Command().exitOverride(); + registerServe(program); + let parsed: Record | undefined; + program.commands + .find((cmd) => cmd.name() === "serve")! + .action((opts: Record) => { + parsed = opts; + }); + + await program.parseAsync(["serve", "--no-recovery"], { from: "user" }); + + assert.equal(parsed?.recovery, false); + assert.equal(parsed?.noRecovery, undefined); + + const fs = await import("node:fs"); + const path = await import("node:path"); + const source = fs.readFileSync( + path.resolve(import.meta.dirname, "../../bin/cli/commands/serve.mjs"), + "utf-8" + ); + assert.match(source, /if \(opts\.recovery === false[^)]*\) \{\s*return runWithoutRecovery\(/); +});