mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-20 22:22:57 +03:00
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 0a74bfbdea -- confirmed still intact,
no action needed.
Co-authored-by: Markus Hartung <mail@hartmark.se>
This commit is contained in:
committed by
GitHub
parent
e1c2425ed7
commit
4d92dfe0a2
@@ -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,
|
||||
};
|
||||
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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 };
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user