fix: distinguish CLI-probe timeouts from not_found, resolve Hermes Agent keyId server-side (#10710+10711)

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)

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,
   };
 

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 {

src/shared/services/cliRuntime.ts

@@ -856,6 +856,16 @@ export const locateCommand = async (command: string, env: Record<string, string
       const preferred = lines.find((l: string) => 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<string, string
   if (located.ok && located.stdout) {
     return { installed: true, commandPath: command, reason: null };
   }
+  // #10710: see the matching Windows branch above -- a timeout must not be
+  // reported as "not_found".
+  if (located.timedOut) {
+    return { installed: false, commandPath: null, reason: "timeout" };
+  }
   return { installed: false, commandPath: null, reason: "not_found" };
 };
 
@@ -945,7 +960,7 @@ export const checkKnownPath = async (commandPath: string) => {
 
 type KnownPathResult = Awaited<ReturnType<typeof checkKnownPath>>;
 
-const locateCommandCandidate = async (
+export const locateCommandCandidate = async (
   commands: string[],
   env: Record<string, string | undefined>,
   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<ReturnType<typeof locateCommand>> | 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 };
   }

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<string, { api_key: string }>;
+}
+
+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");
+});

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");
+});