fix: enqueue fallback event on delivery throw and align resolveGatewayTarget with callGateway URL resolution
Two regressions addressed (PR #34580 CR review): 1. server-restart-sentinel.ts – deliverOutboundPayloads catch block was silently swallowing errors and proceeding to agentCommand without any fallback. When delivery throws before bestEffort handling (e.g. channel plugin error), the user would receive neither the deterministic notice nor a system event if the resumed run emits no payloads. Fix: enqueue a system event in the catch block, mirroring prior behaviour. 2. gateway.ts resolveGatewayTarget – two mismatches with callGateway's actual URL resolution path: a. gateway.mode=remote + missing gateway.remote.url: callGateway falls back to local loopback, but resolveGatewayTarget returned 'remote', suppressing deliveryContext for what is actually a local call. b. Env URL overrides (OPENCLAW_GATEWAY_URL / CLAWDBOT_GATEWAY_URL) are picked up by callGateway but were ignored here, causing incorrect local/remote classification. Fix: check env overrides first, then require both mode=remote AND remote.url present for 'remote'. Tests: add regression coverage for both fixes.
This commit is contained in:
Bryan Marty
parent
f7132aeb79
commit
3c914b8f38
@ -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<string, unknown>,
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// 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<string, unknown>) {
|
||||
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");
|
||||
});
|
||||
});
|
||||
|
||||
@ -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,
|
||||
|
||||
@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
@ -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
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user