From 4d92dfe0a24064d600d5a0890d41ad014ac598de Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Wed, 19 Aug 2026 11:08:34 -0300 Subject: [PATCH] fix: distinguish CLI-probe timeouts from not_found, resolve Hermes Agent keyId server-side (#10710+10711) (#10746) #10710: locateCommand() in cliRuntime.ts collapsed a genuine probe timeout (runProcess's timedOut flag) into the same reason:"not_found" as a truly absent binary, on both the where.exe and `command -v` branches. Give timeouts a distinct "timeout" reason, keep trying remaining command candidates in locateCommandCandidate instead of treating a timeout as terminal, and extend the settings-file fallback (cliInstallFallback.ts) to also cover the new "timeout" reason, matching the scenario it already existed for. #10711: the Hermes Agent dashboard "Apply" flow only ever sends `keyId` (never a raw `apiKey`), but the hermes-agent-settings POST handler never resolved it, so generateHermesAgentConfig() always fell through to the literal placeholder "YOUR_OMNIROUTE_API_KEY_HERE" for providers.omniroute.api_key, delegation.api_key, and every auxiliary.*.api_key. Resolve keyId server-side via getApiKeyById(), the same precedented pattern already used by claude-settings/route.ts and codex-settings/route.ts. Bug 2 from #10710 (hermes tool-detector configPath) was already fixed by commit 0a74bfbdeae4cc45b34c83d10348e33b7cdd85e4 -- confirmed still intact, no action needed. Co-authored-by: Markus Hartung --- ...10-10711-cli-tools-timeout-hermes-keyid.md | 1 + .../cli-tools/hermes-agent-settings/route.ts | 22 +++- src/shared/services/cliInstallFallback.ts | 16 ++- src/shared/services/cliRuntime.ts | 37 +++++- ...rmes-agent-keyid-placeholder-10711.test.ts | 63 +++++++++ ...ntime-locate-command-timeout-10710.test.ts | 121 ++++++++++++++++++ ...s-agent-settings-route-keyid-10711.test.ts | 116 +++++++++++++++++ 7 files changed, 367 insertions(+), 9 deletions(-) create mode 100644 changelog.d/fixes/10710-10711-cli-tools-timeout-hermes-keyid.md create mode 100644 tests/unit/cli-helper/hermes-agent-keyid-placeholder-10711.test.ts create mode 100644 tests/unit/cli-runtime-locate-command-timeout-10710.test.ts create mode 100644 tests/unit/hermes-agent-settings-route-keyid-10711.test.ts diff --git a/changelog.d/fixes/10710-10711-cli-tools-timeout-hermes-keyid.md b/changelog.d/fixes/10710-10711-cli-tools-timeout-hermes-keyid.md new file mode 100644 index 0000000000..4d9a6ba0b4 --- /dev/null +++ b/changelog.d/fixes/10710-10711-cli-tools-timeout-hermes-keyid.md @@ -0,0 +1 @@ +- fix(cli): distinguish a CLI-probe timeout from a genuinely absent binary in locateCommand, and resolve the Hermes Agent Apply flow's `keyId` server-side instead of writing the `YOUR_OMNIROUTE_API_KEY_HERE` placeholder (#10710, #10711) diff --git a/src/app/api/cli-tools/hermes-agent-settings/route.ts b/src/app/api/cli-tools/hermes-agent-settings/route.ts index 69c98f4303..bbbbe37512 100644 --- a/src/app/api/cli-tools/hermes-agent-settings/route.ts +++ b/src/app/api/cli-tools/hermes-agent-settings/route.ts @@ -10,6 +10,7 @@ import { getCurrentHermesAgentRoles, } from "@/lib/cli-helper/config-generator/hermes-agent"; import { getHermesConfigPath } from "@/lib/cli-helper/config-generator/hermesHome"; +import { getApiKeyById } from "@/lib/db/apiKeys"; import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error.ts"; const hermesAgentSettingsSchema = z.object({ @@ -99,10 +100,29 @@ export async function POST(request: Request) { await fs.mkdir(configDir, { recursive: true }); + // #10711: HermesAgentToolCard's "Apply" flow only ever sends `keyId` (never + // a raw `apiKey`) — the same precedented pattern as claude-settings/route.ts + // and codex-settings/route.ts. Resolve the real key by ID here so + // generateHermesAgentConfig() does not fall through to its + // "YOUR_OMNIROUTE_API_KEY_HERE" placeholder. Never trust a client-supplied + // key string directly: the /api/keys list endpoint returns masked values, + // so the only safe source of a usable key is resolving by ID from the DB. + let resolvedApiKey = apiKey ?? null; + if (keyId) { + try { + const keyRecord = await getApiKeyById(keyId); + if (keyRecord?.key) { + resolvedApiKey = keyRecord.key as string; + } + } catch { + // Non-critical: fall back to whatever apiKey (if any) was already provided. + } + } + const payload = { baseUrl, keyId, - apiKey, + apiKey: resolvedApiKey, selections, }; diff --git a/src/shared/services/cliInstallFallback.ts b/src/shared/services/cliInstallFallback.ts index dfe57c3bd8..dc290fca4d 100644 --- a/src/shared/services/cliInstallFallback.ts +++ b/src/shared/services/cliInstallFallback.ts @@ -18,11 +18,13 @@ import fsSync from "fs"; * for any CLI tool that declares a `settings` config path (currently * `claude` and `droid` — see `CLI_TOOLS` in `cliRuntime.ts`). * - * Only applies when the lookup's own reason is "not_found" — i.e. the binary - * genuinely couldn't be located on PATH/known install paths. Deliberate - * security rejections (unsafe/relative env override paths, symlink escapes, - * suspicious file sizes, etc.) must stay `installed:false` regardless of - * whether a settings file happens to exist. + * Only applies when the lookup's own reason is "not_found" or "timeout" — + * i.e. the binary genuinely couldn't be located on PATH/known install paths, + * or the probe never got a chance to answer (#10710: a probe timeout is one + * more variant of "not currently resolvable", the exact scenario this + * fallback exists for). Deliberate security rejections (unsafe/relative env + * override paths, symlink escapes, suspicious file sizes, etc.) must stay + * `installed:false` regardless of whether a settings file happens to exist. */ export interface NotInstalledResult { installed: false; @@ -54,7 +56,9 @@ export const withSettingsFallback = ( settingsPath: string | undefined, notInstalledResult: NotInstalledResult ): NotInstalledResult | SettingsFallbackResult => { - if (notInstalledResult.reason !== "not_found") return notInstalledResult; + if (notInstalledResult.reason !== "not_found" && notInstalledResult.reason !== "timeout") { + return notInstalledResult; + } if (!settingsPath || !fsSync.existsSync(settingsPath)) return notInstalledResult; return { diff --git a/src/shared/services/cliRuntime.ts b/src/shared/services/cliRuntime.ts index 9b47e68343..b8605deff1 100644 --- a/src/shared/services/cliRuntime.ts +++ b/src/shared/services/cliRuntime.ts @@ -856,6 +856,16 @@ export const locateCommand = async (command: string, env: Record winExt.test(l)) || lines[0]; return { installed: true, commandPath: normalizeMsys2Path(preferred), reason: null }; } + // #10710: a probe timeout is NOT the same fact as a genuinely absent binary + // -- runProcess sets `timedOut` when its own 3s timer SIGKILLs the child + // before it answered. Collapsing that into "not_found" makes an installed + // CLI starved under concurrent fan-out (see all-statuses route) look + // identical to one that was never installed. Surface a distinct reason so + // callers can decide (retry, remember-and-continue, etc.) instead of + // silently reporting a false negative. + if (located.timedOut) { + return { installed: false, commandPath: null, reason: "timeout" }; + } return { installed: false, commandPath: null, reason: "not_found" }; } @@ -866,6 +876,11 @@ export const locateCommand = async (command: string, env: Record { type KnownPathResult = Awaited>; -const locateCommandCandidate = async ( +export const locateCommandCandidate = async ( commands: string[], env: Record, toolId?: string @@ -975,13 +990,31 @@ const locateCommandCandidate = async ( // Always try PATH — a stray/broken known-path guess must never hide a genuinely // PATH-resolvable binary (#7774). User can also set CLI_EXTRA_PATHS if needed. + // + // #10710: "timeout" is deliberately NOT terminal like other failure reasons + // (unsafe_path, symlink_escape, ...). A timeout only proves the probe was + // too slow, not that the binary is absent, so remaining command aliases are + // still worth trying (the next one may resolve quickly). Remember the first + // timeout as a fallback so a genuine "not_found" for every alias doesn't + // silently swallow the fact that one probe never actually completed. + let bestTimeoutFailure: Awaited> | null = null; for (const command of commands) { const located = await locateCommand(command, env); - if (located.installed || located.reason !== "not_found") { + if (located.installed) { + return { command, ...located }; + } + if (located.reason === "timeout") { + if (!bestTimeoutFailure) bestTimeoutFailure = located; + continue; + } + if (located.reason !== "not_found") { return { command, ...located }; } } + if (bestTimeoutFailure) { + return { command: commands[0], ...bestTimeoutFailure }; + } if (bestKnownPathFailure) { return { command: commands[0], ...bestKnownPathFailure }; } diff --git a/tests/unit/cli-helper/hermes-agent-keyid-placeholder-10711.test.ts b/tests/unit/cli-helper/hermes-agent-keyid-placeholder-10711.test.ts new file mode 100644 index 0000000000..c4039f1557 --- /dev/null +++ b/tests/unit/cli-helper/hermes-agent-keyid-placeholder-10711.test.ts @@ -0,0 +1,63 @@ +/** + * Regression test for #10711. + * + * The Hermes Agent dashboard "Apply" flow (HermesAgentToolCard.tsx) only + * ever sends `keyId` (never a raw `apiKey`). generateHermesAgentConfig() + * used to never resolve `keyId` at all, so `providers.omniroute.api_key`, + * `delegation.api_key`, and every `auxiliary.*.api_key` were written with + * the hardcoded placeholder "YOUR_OMNIROUTE_API_KEY_HERE", yielding 401s + * against OmniRoute for every real user. + */ + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import * as yaml from "js-yaml"; +import { generateHermesAgentConfig } from "../../../src/lib/cli-helper/config-generator/hermes-agent.ts"; + +interface HermesAgentParsedConfig { + providers: { omniroute: { api_key: string } }; + delegation: { api_key: string }; + auxiliary: Record; +} + +test("#10711: generateHermesAgentConfig writes placeholder api_key when only keyId is supplied (no apiKey)", async () => { + const result = await generateHermesAgentConfig({ + baseUrl: "http://localhost:20128", + keyId: "some-stored-key-id", // what the real dashboard flow actually sends + apiKey: null, // never resolved server-side — this is the bug + selections: [ + { role: "default", model: "gpt-4o" }, + { role: "delegation", model: "gpt-4o" }, + { role: "vision", model: "gpt-4o-vision" }, + ], + }); + + assert.equal(result.error, undefined); + const parsed = yaml.load(result.yaml) as HermesAgentParsedConfig; + + // generateHermesAgentConfig() itself never resolves keyId (that now happens + // in the route handler before calling it) -- confirms the fallthrough this + // bug depends on still exists at this layer, and that an explicit apiKey + // (as the resolved route now passes) overrides the placeholder. + assert.equal(parsed.providers.omniroute.api_key, "YOUR_OMNIROUTE_API_KEY_HERE"); +}); + +test("#10711: an explicit apiKey (as resolved server-side from keyId) is written everywhere, never the placeholder", async () => { + const result = await generateHermesAgentConfig({ + baseUrl: "http://localhost:20128", + keyId: "some-stored-key-id", + apiKey: "sk-resolved-real-key-value", + selections: [ + { role: "default", model: "gpt-4o" }, + { role: "delegation", model: "gpt-4o" }, + { role: "vision", model: "gpt-4o-vision" }, + ], + }); + + assert.equal(result.error, undefined); + const parsed = yaml.load(result.yaml) as HermesAgentParsedConfig; + + assert.equal(parsed.providers.omniroute.api_key, "sk-resolved-real-key-value"); + assert.equal(parsed.delegation.api_key, "sk-resolved-real-key-value"); + assert.equal(parsed.auxiliary.vision.api_key, "sk-resolved-real-key-value"); +}); diff --git a/tests/unit/cli-runtime-locate-command-timeout-10710.test.ts b/tests/unit/cli-runtime-locate-command-timeout-10710.test.ts new file mode 100644 index 0000000000..981abbd725 --- /dev/null +++ b/tests/unit/cli-runtime-locate-command-timeout-10710.test.ts @@ -0,0 +1,121 @@ +/** + * Regression test for #10710. + * + * locateCommand() (src/shared/services/cliRuntime.ts) used to collapse a + * genuine probe timeout (runProcess's internal 3s timer SIGKILLing the + * child) into the exact same reason:"not_found" as a truly-absent binary. + * That made an installed CLI whose where.exe/`command -v` probe was starved + * (e.g. under the all-statuses route's wide, undeduped ~30-tool fan-out) + * indistinguishable from one that was never installed at all. + */ + +import test from "node:test"; +import assert from "node:assert/strict"; +import path from "node:path"; +import { createRequire, syncBuiltinESMExports } from "node:module"; +import { pathToFileURL } from "node:url"; +import { EventEmitter } from "node:events"; + +const require = createRequire(import.meta.url); +const childProcess = require("node:child_process"); +const modulePath = path.join(process.cwd(), "src/shared/services/cliRuntime.ts"); + +const originalSpawn = childProcess.spawn; + +async function importFresh(label: string) { + return import(`${pathToFileURL(modulePath).href}?case=${label}-${Date.now()}-${Math.random()}`); +} + +type FakeChildProcess = EventEmitter & { + stdout: EventEmitter; + stderr: EventEmitter; + kill: (signal?: string) => boolean; +}; + +function installTimeoutSpawn() { + childProcess.spawn = () => { + const child = new EventEmitter() as FakeChildProcess; + child.stdout = new EventEmitter(); + child.stderr = new EventEmitter(); + // Model a real SIGKILL: the OS delivers 'close' shortly after the kill, + // not synchronously -- but not before the internal 3s timeout fires. + child.kill = () => { + setImmediate(() => child.emit("close", null)); + return true; + }; + return child; + }; + syncBuiltinESMExports(); +} + +test.afterEach(() => { + childProcess.spawn = originalSpawn; + syncBuiltinESMExports(); +}); + +test("#10710: locateCommand surfaces a distinct 'timeout' reason, not 'not_found'", async () => { + installTimeoutSpawn(); + + const cliRuntime = await importFresh("locate-timeout"); + const start = Date.now(); + const result = await cliRuntime.locateCommand("codex", { PATH: process.env.PATH }); + const elapsedMs = Date.now() - start; + + assert.ok(elapsedMs >= 2900, `expected the internal 3s timeout to fire, took ${elapsedMs}ms`); + assert.equal(result.installed, false); + assert.equal( + result.reason, + "timeout", + "a probe timeout must not be relabeled as not_found -- indistinguishable from a genuinely absent binary" + ); +}); + +test("#10710: a timed-out first candidate does not hide a second candidate that resolves", async () => { + let call = 0; + childProcess.spawn = (_command: string) => { + call += 1; + const child = new EventEmitter() as FakeChildProcess; + child.stdout = new EventEmitter(); + child.stderr = new EventEmitter(); + if (call === 1) { + // First candidate (e.g. "codex-preview") times out. + child.kill = () => { + setImmediate(() => child.emit("close", null)); + return true; + }; + } else { + // Second candidate (e.g. "codex") resolves quickly and successfully. + child.kill = () => true; + setImmediate(() => { + child.stdout.emit("data", Buffer.from("/usr/local/bin/codex\n")); + child.emit("close", 0); + }); + } + return child; + }; + syncBuiltinESMExports(); + + const cliRuntime = await importFresh("locate-candidate-fallthrough"); + const result = await cliRuntime.locateCommandCandidate( + ["codex-preview", "codex"], + { PATH: process.env.PATH }, + undefined + ); + + assert.equal(result.installed, true, "the second, quickly-resolving candidate must still be found"); + assert.equal(result.command, "codex"); +}); + +test("#10710: when every candidate times out, the caller sees 'timeout' rather than 'not_found'", async () => { + installTimeoutSpawn(); + + const cliRuntime = await importFresh("locate-candidate-all-timeout"); + const result = await cliRuntime.locateCommandCandidate( + ["codex-preview", "codex"], + { PATH: process.env.PATH }, + undefined + ); + + assert.equal(result.installed, false); + assert.equal(result.reason, "timeout"); +}); diff --git a/tests/unit/hermes-agent-settings-route-keyid-10711.test.ts b/tests/unit/hermes-agent-settings-route-keyid-10711.test.ts new file mode 100644 index 0000000000..6651b8bdc4 --- /dev/null +++ b/tests/unit/hermes-agent-settings-route-keyid-10711.test.ts @@ -0,0 +1,116 @@ +/** + * Regression test for #10711. + * + * The Hermes Agent dashboard "Apply" flow (HermesAgentToolCard.tsx) only ever + * sends `{ keyId, selections }` — never a raw `apiKey` — because resolving a + * real key from a stored keyId is expected to happen server-side, mirroring + * claude-settings/route.ts and codex-settings/route.ts. The POST handler for + * hermes-agent-settings never resolved `keyId` before this fix, so it always + * fell through to the literal placeholder "YOUR_OMNIROUTE_API_KEY_HERE" in + * providers.omniroute.api_key, delegation.api_key, and every auxiliary.*.api_key. + * + * This test drives the real POST handler end-to-end (real DB-backed API key, + * real JWT auth cookie, preview mode so nothing is written to disk) and + * asserts the generated YAML carries the real resolved key. + */ + +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { SignJWT } from "jose"; +import * as yaml from "js-yaml"; + +interface HermesAgentParsedConfig { + providers: { omniroute: { api_key: string } }; + delegation: { api_key: string }; + auxiliary: Record; +} + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omr-hermes-agent-10711-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "hermes-agent-10711-api-secret"; +process.env.JWT_SECRET = "hermes-agent-10711-jwt-secret"; +process.env.CLI_ALLOW_CONFIG_WRITES = "true"; + +const core = await import("../../src/lib/db/core.ts"); +const apiKeysDb = await import("../../src/lib/db/apiKeys.ts"); +const route = await import("../../src/app/api/cli-tools/hermes-agent-settings/route.ts"); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +async function authCookie(): Promise { + const secret = new TextEncoder().encode(process.env.JWT_SECRET); + const jwt = await new SignJWT({ sub: "admin" }) + .setProtectedHeader({ alg: "HS256" }) + .setExpirationTime("1h") + .sign(secret); + return `auth_token=${jwt}`; +} + +test("#10711: POST hermes-agent-settings resolves keyId server-side instead of writing the placeholder", async () => { + const created = await apiKeysDb.createApiKey("hermes-agent-10711-key", "hermes-agent-10711-machine"); + const realKey = created.key; + assert.ok(realKey && realKey.length > 0, "createApiKey must return the real plaintext key"); + + const response = await route.POST( + new Request("http://localhost/api/cli-tools/hermes-agent-settings", { + method: "POST", + headers: { + cookie: await authCookie(), + "Content-Type": "application/json", + }, + body: JSON.stringify({ + baseUrl: "http://localhost:20128", + keyId: created.id, + selections: [ + { role: "default", model: "gpt-4o" }, + { role: "delegation", model: "gpt-4o" }, + { role: "vision", model: "gpt-4o-vision" }, + ], + preview: true, + }), + }) + ); + + assert.equal(response.status, 200); + const body = await response.json(); + assert.equal(body.success, true); + + const parsed = yaml.load(body.yaml) as HermesAgentParsedConfig; + assert.notEqual( + parsed.providers.omniroute.api_key, + "YOUR_OMNIROUTE_API_KEY_HERE", + "providers.omniroute.api_key must not be the unresolved placeholder" + ); + assert.equal(parsed.providers.omniroute.api_key, realKey); + assert.equal(parsed.delegation.api_key, realKey); + assert.equal(parsed.auxiliary.vision.api_key, realKey); +}); + +test("#10711: POST hermes-agent-settings falls back gracefully when keyId does not resolve", async () => { + const response = await route.POST( + new Request("http://localhost/api/cli-tools/hermes-agent-settings", { + method: "POST", + headers: { + cookie: await authCookie(), + "Content-Type": "application/json", + }, + body: JSON.stringify({ + baseUrl: "http://localhost:20128", + keyId: "does-not-exist-in-db", + selections: [{ role: "default", model: "gpt-4o" }], + preview: true, + }), + }) + ); + + // Must not crash the Apply flow — still succeeds, just without a resolved key. + assert.equal(response.status, 200); + const body = await response.json(); + assert.equal(body.success, true); +});