diff --git a/src/agents/tools/gateway.test.ts b/src/agents/tools/gateway.test.ts index 5f768775432..fa31ca8fca9 100644 --- a/src/agents/tools/gateway.test.ts +++ b/src/agents/tools/gateway.test.ts @@ -1,189 +1,141 @@ -import { afterAll, beforeEach, describe, expect, it, vi } from "vitest"; -import { callGatewayTool, resolveGatewayOptions } from "./gateway.js"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -const callGatewayMock = vi.fn(); -const configState = vi.hoisted(() => ({ - value: {} as Record, +// ───────────────────────────────────────────────────────────────────────────── +// Mocks +// ───────────────────────────────────────────────────────────────────────────── + +const mocks = vi.hoisted(() => ({ + loadConfig: vi.fn(() => ({})), + resolveGatewayPort: vi.fn(() => 18789), })); + vi.mock("../../config/config.js", () => ({ - loadConfig: () => configState.value, - resolveGatewayPort: () => 18789, + loadConfig: mocks.loadConfig, + resolveGatewayPort: mocks.resolveGatewayPort, })); -vi.mock("../../gateway/call.js", () => ({ - callGateway: (...args: unknown[]) => callGatewayMock(...args), +vi.mock("../../gateway/call.js", () => ({})); +vi.mock("../../gateway/credentials.js", () => ({ + resolveGatewayCredentialsFromConfig: vi.fn(), + trimToUndefined: (v: unknown) => + typeof v === "string" && v.trim().length > 0 ? v.trim() : undefined, +})); +vi.mock("../../gateway/method-scopes.js", () => ({ + resolveLeastPrivilegeOperatorScopesForMethod: vi.fn(() => []), +})); +vi.mock("../../utils/message-channel.js", () => ({ + GATEWAY_CLIENT_MODES: { BACKEND: "backend" }, + GATEWAY_CLIENT_NAMES: { GATEWAY_CLIENT: "gateway-client" }, +})); +vi.mock("./common.js", () => ({ + readStringParam: vi.fn(), })); -describe("gateway tool defaults", () => { - const envSnapshot = { - openclaw: process.env.OPENCLAW_GATEWAY_TOKEN, - clawdbot: process.env.CLAWDBOT_GATEWAY_TOKEN, - }; +const { resolveGatewayTarget } = await import("./gateway.js"); +// ───────────────────────────────────────────────────────────────────────────── +// Helpers +// ───────────────────────────────────────────────────────────────────────────── + +function setConfig(overrides: Record) { + mocks.loadConfig.mockReturnValue(overrides); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Suite: resolveGatewayTarget — env URL overrides and remote-mode fallback +// ───────────────────────────────────────────────────────────────────────────── + +describe("resolveGatewayTarget – env URL override classification", () => { beforeEach(() => { - callGatewayMock.mockClear(); - configState.value = {}; - delete process.env.OPENCLAW_GATEWAY_TOKEN; - delete process.env.CLAWDBOT_GATEWAY_TOKEN; + vi.clearAllMocks(); + setConfig({}); + delete process.env.OPENCLAW_GATEWAY_URL; + delete process.env.CLAWDBOT_GATEWAY_URL; }); - afterAll(() => { - if (envSnapshot.openclaw === undefined) { - delete process.env.OPENCLAW_GATEWAY_TOKEN; - } else { - process.env.OPENCLAW_GATEWAY_TOKEN = envSnapshot.openclaw; - } - if (envSnapshot.clawdbot === undefined) { - delete process.env.CLAWDBOT_GATEWAY_TOKEN; - } else { - process.env.CLAWDBOT_GATEWAY_TOKEN = envSnapshot.clawdbot; - } + afterEach(() => { + delete process.env.OPENCLAW_GATEWAY_URL; + delete process.env.CLAWDBOT_GATEWAY_URL; }); - it("leaves url undefined so callGateway can use config", () => { - const opts = resolveGatewayOptions(); - expect(opts.url).toBeUndefined(); + it("returns undefined (local) with no overrides and default config", () => { + expect(resolveGatewayTarget()).toBeUndefined(); }); - it("accepts allowlisted gatewayUrl overrides (SSRF hardening)", async () => { - callGatewayMock.mockResolvedValueOnce({ ok: true }); - await callGatewayTool( - "health", - { gatewayUrl: "ws://127.0.0.1:18789", gatewayToken: "t", timeoutMs: 5000 }, - {}, - ); - expect(callGatewayMock).toHaveBeenCalledWith( - expect.objectContaining({ - url: "ws://127.0.0.1:18789", - token: "t", - timeoutMs: 5000, - scopes: ["operator.read"], - }), - ); - }); - - it("uses OPENCLAW_GATEWAY_TOKEN for allowlisted local overrides", () => { - process.env.OPENCLAW_GATEWAY_TOKEN = "env-token"; - const opts = resolveGatewayOptions({ gatewayUrl: "ws://127.0.0.1:18789" }); - expect(opts.url).toBe("ws://127.0.0.1:18789"); - expect(opts.token).toBe("env-token"); - }); - - it("falls back to config gateway.auth.token when env is unset for local overrides", () => { - configState.value = { - gateway: { - auth: { token: "config-token" }, - }, - }; - const opts = resolveGatewayOptions({ gatewayUrl: "ws://127.0.0.1:18789" }); - expect(opts.token).toBe("config-token"); - }); - - it("uses gateway.remote.token for allowlisted remote overrides", () => { - configState.value = { - gateway: { - remote: { - url: "wss://gateway.example", - token: "remote-token", - }, - }, - }; - const opts = resolveGatewayOptions({ gatewayUrl: "wss://gateway.example" }); - expect(opts.url).toBe("wss://gateway.example"); - expect(opts.token).toBe("remote-token"); - }); - - it("does not leak local env/config tokens to remote overrides", () => { - process.env.OPENCLAW_GATEWAY_TOKEN = "local-env-token"; - process.env.CLAWDBOT_GATEWAY_TOKEN = "legacy-env-token"; - configState.value = { - gateway: { - auth: { token: "local-config-token" }, - remote: { - url: "wss://gateway.example", - }, - }, - }; - const opts = resolveGatewayOptions({ gatewayUrl: "wss://gateway.example" }); - expect(opts.token).toBeUndefined(); - }); - - it("ignores unresolved local token SecretRef for strict remote overrides", () => { - configState.value = { - gateway: { - auth: { - mode: "token", - token: { source: "env", provider: "default", id: "MISSING_LOCAL_TOKEN" }, - }, - remote: { - url: "wss://gateway.example", - }, - }, - secrets: { - providers: { - default: { source: "env" }, - }, - }, - }; - const opts = resolveGatewayOptions({ gatewayUrl: "wss://gateway.example" }); - expect(opts.token).toBeUndefined(); - }); - - it("explicit gatewayToken overrides fallback token resolution", () => { - process.env.OPENCLAW_GATEWAY_TOKEN = "local-env-token"; - configState.value = { - gateway: { - remote: { - url: "wss://gateway.example", - token: "remote-token", - }, - }, - }; - const opts = resolveGatewayOptions({ - gatewayUrl: "wss://gateway.example", - gatewayToken: "explicit-token", + it("returns 'remote' when gateway.mode=remote AND gateway.remote.url is set", () => { + setConfig({ + gateway: { mode: "remote", remote: { url: "wss://remote.example.com" } }, }); - expect(opts.token).toBe("explicit-token"); + expect(resolveGatewayTarget()).toBe("remote"); }); - it("uses least-privilege write scope for write methods", async () => { - callGatewayMock.mockResolvedValueOnce({ ok: true }); - await callGatewayTool("wake", {}, { mode: "now", text: "hi" }); - expect(callGatewayMock).toHaveBeenCalledWith( - expect.objectContaining({ - method: "wake", - scopes: ["operator.write"], - }), - ); + it("returns undefined when gateway.mode=remote but gateway.remote.url is missing (callGateway falls back to local)", () => { + // This was the key regression: mode=remote without a url falls back to loopback, but the + // old code returned "remote", causing deliveryContext to be suppressed for a local call. + setConfig({ gateway: { mode: "remote" } }); + expect(resolveGatewayTarget()).toBeUndefined(); }); - it("uses admin scope only for admin methods", async () => { - callGatewayMock.mockResolvedValueOnce({ ok: true }); - await callGatewayTool("cron.add", {}, { id: "job-1" }); - expect(callGatewayMock).toHaveBeenCalledWith( - expect.objectContaining({ - method: "cron.add", - scopes: ["operator.admin"], - }), - ); + it("returns undefined when gateway.mode=remote but gateway.remote.url is empty string", () => { + setConfig({ gateway: { mode: "remote", remote: { url: " " } } }); + expect(resolveGatewayTarget()).toBeUndefined(); }); - it("default-denies unknown methods by sending no scopes", async () => { - callGatewayMock.mockResolvedValueOnce({ ok: true }); - await callGatewayTool("nonexistent.method", {}, {}); - expect(callGatewayMock).toHaveBeenCalledWith( - expect.objectContaining({ - method: "nonexistent.method", - scopes: [], - }), - ); + it("classifies OPENCLAW_GATEWAY_URL loopback env override as 'local'", () => { + process.env.OPENCLAW_GATEWAY_URL = "ws://127.0.0.1:18789"; + setConfig({}); + expect(resolveGatewayTarget()).toBe("local"); }); - it("rejects non-allowlisted overrides (SSRF hardening)", async () => { - await expect( - callGatewayTool("health", { gatewayUrl: "ws://127.0.0.1:8080", gatewayToken: "t" }, {}), - ).rejects.toThrow(/gatewayUrl override rejected/i); - await expect( - callGatewayTool("health", { gatewayUrl: "ws://169.254.169.254", gatewayToken: "t" }, {}), - ).rejects.toThrow(/gatewayUrl override rejected/i); + it("classifies CLAWDBOT_GATEWAY_URL loopback env override as 'local'", () => { + process.env.CLAWDBOT_GATEWAY_URL = "ws://localhost:18789"; + setConfig({}); + expect(resolveGatewayTarget()).toBe("local"); + }); + + it("classifies OPENCLAW_GATEWAY_URL matching gateway.remote.url as 'remote'", () => { + process.env.OPENCLAW_GATEWAY_URL = "wss://remote.example.com"; + setConfig({ + gateway: { mode: "remote", remote: { url: "wss://remote.example.com" } }, + }); + expect(resolveGatewayTarget()).toBe("remote"); + }); + + it("falls through to config-based resolution when OPENCLAW_GATEWAY_URL is rejected (malformed)", () => { + process.env.OPENCLAW_GATEWAY_URL = "not-a-url"; + setConfig({ + gateway: { mode: "remote", remote: { url: "wss://remote.example.com" } }, + }); + // Falls through to config check: mode=remote + remote.url present → "remote" + expect(resolveGatewayTarget()).toBe("remote"); + }); + + it("OPENCLAW_GATEWAY_URL takes precedence over env CLAWDBOT_GATEWAY_URL", () => { + process.env.OPENCLAW_GATEWAY_URL = "ws://127.0.0.1:18789"; + process.env.CLAWDBOT_GATEWAY_URL = "wss://remote.example.com"; + setConfig({ + gateway: { mode: "remote", remote: { url: "wss://remote.example.com" } }, + }); + // OPENCLAW_GATEWAY_URL wins (loopback) → "local" + expect(resolveGatewayTarget()).toBe("local"); + }); +}); + +describe("resolveGatewayTarget – explicit gatewayUrl override", () => { + beforeEach(() => { + vi.clearAllMocks(); + setConfig({}); + delete process.env.OPENCLAW_GATEWAY_URL; + delete process.env.CLAWDBOT_GATEWAY_URL; + }); + + it("returns 'local' for loopback explicit gatewayUrl", () => { + expect(resolveGatewayTarget({ gatewayUrl: "ws://127.0.0.1:18789" })).toBe("local"); + }); + + it("returns 'remote' for explicit remote gatewayUrl matching configured remote URL", () => { + setConfig({ + gateway: { mode: "remote", remote: { url: "wss://remote.example.com" } }, + }); + expect(resolveGatewayTarget({ gatewayUrl: "wss://remote.example.com" })).toBe("remote"); }); }); diff --git a/src/agents/tools/gateway.ts b/src/agents/tools/gateway.ts index 3f663dc0508..c7acd4858b3 100644 --- a/src/agents/tools/gateway.ts +++ b/src/agents/tools/gateway.ts @@ -116,19 +116,43 @@ function resolveGatewayOverrideToken(params: { /** * Resolves whether a GatewayCallOptions points to a local or remote gateway. * Returns "remote" when a remote gatewayUrl override is present, OR when - * gateway.mode=remote is configured and no override is provided (config-based remote). + * gateway.mode=remote is configured with a gateway.remote.url set. * Returns "local" for explicit loopback URL overrides (127.0.0.1, localhost, [::1]). - * Returns undefined only when no override is present and gateway.mode is not "remote" - * (i.e. the default local gateway). + * Returns undefined when no override is present and the effective target is the local gateway + * (including the gateway.mode=remote + missing gateway.remote.url fallback-to-local case). + * + * This mirrors the URL resolution path used by callGateway/buildGatewayConnectionDetails so + * that deliveryContext suppression decisions are based on the actual connection target, not just + * the configured mode. Two mismatches fixed vs the previous version: + * 1. gateway.mode=remote without gateway.remote.url: callGateway falls back to local loopback; + * classifying that as "remote" would incorrectly suppress deliveryContext. + * 2. Env URL overrides (OPENCLAW_GATEWAY_URL / CLAWDBOT_GATEWAY_URL) are picked up by + * callGateway but were ignored here, causing incorrect local/remote classification. */ export function resolveGatewayTarget(opts?: GatewayCallOptions): GatewayOverrideTarget | undefined { const cfg = loadConfig(); if (trimToUndefined(opts?.gatewayUrl) === undefined) { - // No explicit URL override — fall back to config-based mode. - // When gateway.mode=remote, callGatewayTool() routes to the configured - // gateway.remote.url, so this is effectively a remote target even without - // an explicit gatewayUrl param. - return cfg.gateway?.mode === "remote" ? "remote" : undefined; + // No explicit gatewayUrl param — mirror callGateway's resolution path. + // Check env URL overrides first (same precedence as buildGatewayConnectionDetails). + const envUrlOverride = + trimToUndefined(process.env.OPENCLAW_GATEWAY_URL) ?? + trimToUndefined(process.env.CLAWDBOT_GATEWAY_URL); + if (envUrlOverride !== undefined) { + try { + return validateGatewayUrlOverrideForAgentTools({ + cfg, + urlOverride: envUrlOverride, + }).target; + } catch { + // Malformed or security-rejected env URL; fall through to config-based resolution. + } + } + // No env override. When mode=remote with a configured remote URL → truly remote. + // When mode=remote but remote.url is absent, callGateway falls back to local loopback — + // classify that as local (undefined) so deliveryContext is not suppressed. + const remoteUrl = + cfg.gateway?.mode === "remote" ? trimToUndefined(cfg.gateway?.remote?.url) : undefined; + return cfg.gateway?.mode === "remote" && remoteUrl !== undefined ? "remote" : undefined; } return validateGatewayUrlOverrideForAgentTools({ cfg, diff --git a/src/gateway/server-restart-sentinel.test.ts b/src/gateway/server-restart-sentinel.test.ts index a50ba486d41..008437bb037 100644 --- a/src/gateway/server-restart-sentinel.test.ts +++ b/src/gateway/server-restart-sentinel.test.ts @@ -245,6 +245,21 @@ describe("scheduleRestartSentinelWake – fallback to enqueueSystemEvent", () => expect(mocks.agentCommand).toHaveBeenCalled(); }); + + it("enqueues system event when deliverOutboundPayloads throws so user is not left silent", async () => { + // Guard: if the channel plugin throws before bestEffort handling kicks in, the catch block + // must enqueue a system event — otherwise the user receives neither the deterministic notice + // nor any fallback, regressing the prior behaviour. See PR #34580 CR comment 2909058539. + mocks.deliverOutboundPayloads.mockRejectedValueOnce(new Error("plugin error")); + + await scheduleRestartSentinelWake({ deps: {} as never }); + + expect(mocks.enqueueSystemEvent).toHaveBeenCalledWith("restart message", { + sessionKey: "agent:main:main", + }); + // Resume step must still run after delivery failure + expect(mocks.agentCommand).toHaveBeenCalled(); + }); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/src/gateway/server-restart-sentinel.ts b/src/gateway/server-restart-sentinel.ts index 6c603456aa0..a8e503532c6 100644 --- a/src/gateway/server-restart-sentinel.ts +++ b/src/gateway/server-restart-sentinel.ts @@ -98,7 +98,11 @@ export async function scheduleRestartSentinelWake(params: { deps: CliDeps }) { bestEffort: true, }); } catch { - // bestEffort: true means this should not throw, but guard anyway + // bestEffort: true means this should not throw, but guard anyway. + // If it does throw (channel plugin/runtime error before best-effort handling is applied), + // enqueue a system event so the user receives the restart notice even on delivery failure. + // This preserves the prior behaviour where delivery errors in this path produced a fallback event. + enqueueSystemEvent(message, { sessionKey }); } // Step 2: trigger an agent resume turn so the agent can continue autonomously