mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 21:10:43 +00:00
fix(routing): address three review P1s for bound-account lookup
1. Preserve requester account for same-agent subagent spawns (src/agents/subagent-spawn.ts) resolveRequesterOriginForChild previously overwrote the caller's active inbound accountId with a fresh binding lookup even when the target agent equaled the requester. In multi-account channels where the same agent has multiple bindings, that could switch the child to a different account from the one the parent is actively using. Now the lookup only runs when targetAgentId differs from requesterAgentId; same-agent spawns keep the caller's accountId unchanged. 2. Skip wildcard peer matches when peerId is absent (src/routing/bound-account-read.ts) A wildcard peer binding (peer.id: "*") represents "match any peer", so it is only meaningful when the caller supplies a peer. For peerless callers (e.g. cron delivery resolution) a wildcard binding no longer beats a channel-only binding. It is instead used as a last-resort peer-ish fallback (see #3 below). 3. Preserve old first-match semantics for peerless callers with only peer-specific bindings (src/routing/bound-account-read.ts) The previous iteration skipped peer-specific bindings outright when no peerId was supplied, which silently dropped the account for operators whose only binding was peer-specific (for example cron jobs targeting a specific room). Peerless callers now fall back to the first peer-specific or wildcard binding they find (peerlessPeerSpecificFallback) after channel-only bindings, restoring the prior cron semantics without overriding explicit channel-only configuration. Final three-tier precedence summary: - peerId supplied: exact peer match > wildcard peer > channel-only. Non-matching peer-specific bindings are skipped. - peerId absent: channel-only > first peer-specific or wildcard binding found. Regression coverage: - src/routing/bound-account-read.test.ts (new): six unit tests covering exact peer match, wildcard-wins-over-channel-only for peer-present callers, channel-only-wins-over-wildcard for peerless callers, peer-specific fallback for peerless callers with no channel-only binding, skipping non-matching peer-specific bindings, and the agent-on-different-channel no-match case. - src/agents/openclaw-tools.subagents.sessions-spawn.lifecycle.test.ts: new test asserting that a same-agent sessions_spawn (no explicit agentId) preserves the caller's accountId rather than re-resolving via bindings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
committed by
Gustavo Madeira Santana
parent
de20d381bc
commit
46708707f6
@@ -652,6 +652,44 @@ describe("openclaw-tools: subagents (sessions_spawn lifecycle)", () => {
|
||||
expect(spawnAccountId).toBe("bot-alpha-room-a");
|
||||
});
|
||||
|
||||
it("sessions_spawn preserves the caller's account for same-agent subagent spawns", async () => {
|
||||
let spawnAccountId: string | undefined;
|
||||
const room = "!someRoom:example.org";
|
||||
setSessionsSpawnConfigOverride({
|
||||
session: { mainKey: "main", scope: "per-sender" },
|
||||
messages: { queue: { debounceMs: 0 } },
|
||||
agents: { defaults: { subagents: { allowAgents: ["bot-alpha"] } } },
|
||||
bindings: [
|
||||
{
|
||||
type: "route",
|
||||
agentId: "bot-alpha",
|
||||
match: { channel: "matrix", accountId: "bot-alpha-default" },
|
||||
},
|
||||
],
|
||||
});
|
||||
setupSessionsSpawnGatewayMock({
|
||||
onAgentSubagentSpawn: (params) => {
|
||||
const rec = params as { accountId?: string } | undefined;
|
||||
spawnAccountId = rec?.accountId;
|
||||
},
|
||||
});
|
||||
|
||||
const tool = await getSessionsSpawnTool({
|
||||
agentSessionKey: "agent:bot-alpha:session:main",
|
||||
agentChannel: "matrix",
|
||||
agentAccountId: "bot-alpha-adhoc",
|
||||
agentTo: room,
|
||||
});
|
||||
|
||||
// Spawn a child of the same agent (no explicit agentId → defaults to requester).
|
||||
const result = await tool.execute("call-same-agent", {
|
||||
task: "do thing",
|
||||
cleanup: "keep",
|
||||
});
|
||||
expect(result.details).toMatchObject({ status: "accepted", runId: expect.any(String) });
|
||||
expect(spawnAccountId).toBe("bot-alpha-adhoc");
|
||||
});
|
||||
|
||||
it("sessions_spawn announces with requester accountId", async () => {
|
||||
const ctx = setupSessionsSpawnGatewayMock({});
|
||||
|
||||
|
||||
@@ -350,6 +350,10 @@ function resolveRequesterOriginForChild(params: {
|
||||
requesterTo?: string;
|
||||
requesterThreadId?: string | number;
|
||||
}) {
|
||||
// Same-agent spawns (a child of the same agent) must keep the caller's active
|
||||
// inbound account, not re-resolve via bindings — the caller is already acting
|
||||
// as that agent with a specific account, and a lookup could pick a different
|
||||
// binding when the same agent has multiple accounts configured.
|
||||
const { peerId: normalizedPeerId, peerKind: inferredPeerKind } = extractRequesterPeer(
|
||||
params.requesterChannel,
|
||||
params.requesterTo,
|
||||
|
||||
150
src/routing/bound-account-read.test.ts
Normal file
150
src/routing/bound-account-read.test.ts
Normal file
@@ -0,0 +1,150 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import type { AgentRouteBinding } from "../config/types.agents.js";
|
||||
import type { OpenClawConfig } from "../config/types.openclaw.js";
|
||||
import { resolveFirstBoundAccountId } from "./bound-account-read.js";
|
||||
|
||||
function cfgWithBindings(bindings: AgentRouteBinding[]): OpenClawConfig {
|
||||
return { bindings } as unknown as OpenClawConfig;
|
||||
}
|
||||
|
||||
describe("resolveFirstBoundAccountId", () => {
|
||||
it("returns exact peer match when caller supplies a matching peerId", () => {
|
||||
const cfg = cfgWithBindings([
|
||||
{
|
||||
type: "route",
|
||||
agentId: "bot-alpha",
|
||||
match: { channel: "matrix", accountId: "bot-alpha-default" },
|
||||
},
|
||||
{
|
||||
type: "route",
|
||||
agentId: "bot-alpha",
|
||||
match: {
|
||||
channel: "matrix",
|
||||
peer: { kind: "channel", id: "!roomA:example.org" },
|
||||
accountId: "bot-alpha-room-a",
|
||||
},
|
||||
},
|
||||
]);
|
||||
expect(
|
||||
resolveFirstBoundAccountId({
|
||||
cfg,
|
||||
channelId: "matrix",
|
||||
agentId: "bot-alpha",
|
||||
peerId: "!roomA:example.org",
|
||||
}),
|
||||
).toBe("bot-alpha-room-a");
|
||||
});
|
||||
|
||||
it("prefers wildcard peer binding over channel-only when caller supplies any peerId", () => {
|
||||
const cfg = cfgWithBindings([
|
||||
{
|
||||
type: "route",
|
||||
agentId: "bot-alpha",
|
||||
match: { channel: "matrix", accountId: "bot-alpha-default" },
|
||||
},
|
||||
{
|
||||
type: "route",
|
||||
agentId: "bot-alpha",
|
||||
match: {
|
||||
channel: "matrix",
|
||||
peer: { kind: "channel", id: "*" },
|
||||
accountId: "bot-alpha-wildcard",
|
||||
},
|
||||
},
|
||||
]);
|
||||
expect(
|
||||
resolveFirstBoundAccountId({
|
||||
cfg,
|
||||
channelId: "matrix",
|
||||
agentId: "bot-alpha",
|
||||
peerId: "!anyRoom:example.org",
|
||||
}),
|
||||
).toBe("bot-alpha-wildcard");
|
||||
});
|
||||
|
||||
it("prefers channel-only over wildcard peer binding when caller supplies no peerId", () => {
|
||||
const cfg = cfgWithBindings([
|
||||
{
|
||||
type: "route",
|
||||
agentId: "bot-alpha",
|
||||
match: {
|
||||
channel: "matrix",
|
||||
peer: { kind: "channel", id: "*" },
|
||||
accountId: "bot-alpha-wildcard",
|
||||
},
|
||||
},
|
||||
{
|
||||
type: "route",
|
||||
agentId: "bot-alpha",
|
||||
match: { channel: "matrix", accountId: "bot-alpha-default" },
|
||||
},
|
||||
]);
|
||||
expect(
|
||||
resolveFirstBoundAccountId({
|
||||
cfg,
|
||||
channelId: "matrix",
|
||||
agentId: "bot-alpha",
|
||||
}),
|
||||
).toBe("bot-alpha-default");
|
||||
});
|
||||
|
||||
it("falls back to peer-specific binding for peerless callers when no channel-only or wildcard binding exists", () => {
|
||||
const cfg = cfgWithBindings([
|
||||
{
|
||||
type: "route",
|
||||
agentId: "bot-alpha",
|
||||
match: {
|
||||
channel: "matrix",
|
||||
peer: { kind: "channel", id: "!specificRoom:example.org" },
|
||||
accountId: "bot-alpha-specific",
|
||||
},
|
||||
},
|
||||
]);
|
||||
expect(
|
||||
resolveFirstBoundAccountId({
|
||||
cfg,
|
||||
channelId: "matrix",
|
||||
agentId: "bot-alpha",
|
||||
}),
|
||||
).toBe("bot-alpha-specific");
|
||||
});
|
||||
|
||||
it("skips non-matching peer-specific bindings when caller supplies a different peerId", () => {
|
||||
const cfg = cfgWithBindings([
|
||||
{
|
||||
type: "route",
|
||||
agentId: "bot-alpha",
|
||||
match: {
|
||||
channel: "matrix",
|
||||
peer: { kind: "channel", id: "!otherRoom:example.org" },
|
||||
accountId: "bot-alpha-other",
|
||||
},
|
||||
},
|
||||
]);
|
||||
expect(
|
||||
resolveFirstBoundAccountId({
|
||||
cfg,
|
||||
channelId: "matrix",
|
||||
agentId: "bot-alpha",
|
||||
peerId: "!differentRoom:example.org",
|
||||
}),
|
||||
).toBeUndefined();
|
||||
});
|
||||
|
||||
it("returns undefined when the agent has no binding on the channel", () => {
|
||||
const cfg = cfgWithBindings([
|
||||
{
|
||||
type: "route",
|
||||
agentId: "bot-alpha",
|
||||
match: { channel: "whatsapp", accountId: "bot-alpha-whatsapp" },
|
||||
},
|
||||
]);
|
||||
expect(
|
||||
resolveFirstBoundAccountId({
|
||||
cfg,
|
||||
channelId: "matrix",
|
||||
agentId: "bot-alpha",
|
||||
}),
|
||||
).toBeUndefined();
|
||||
});
|
||||
});
|
||||
@@ -58,6 +58,7 @@ export function resolveFirstBoundAccountId(params: {
|
||||
const normalizedPeerId = params.peerId?.trim() || undefined;
|
||||
let wildcardPeerMatch: string | undefined;
|
||||
let channelOnlyFallback: string | undefined;
|
||||
let peerlessPeerSpecificFallback: string | undefined;
|
||||
for (const binding of listRouteBindings(params.cfg)) {
|
||||
const resolved = resolveNormalizedBindingMatch(binding);
|
||||
if (
|
||||
@@ -68,14 +69,27 @@ export function resolveFirstBoundAccountId(params: {
|
||||
continue;
|
||||
}
|
||||
if (resolved.peerId === "*") {
|
||||
wildcardPeerMatch ??= resolved.accountId;
|
||||
if (normalizedPeerId) {
|
||||
wildcardPeerMatch ??= resolved.accountId;
|
||||
} else {
|
||||
// Caller supplied no peer — a wildcard binding has no peer to match against,
|
||||
// so treat it as a last-resort peer-ish fallback rather than letting it
|
||||
// override channel-only bindings.
|
||||
peerlessPeerSpecificFallback ??= resolved.accountId;
|
||||
}
|
||||
} else if (resolved.peerId) {
|
||||
if (normalizedPeerId && resolved.peerId === normalizedPeerId) {
|
||||
return resolved.accountId;
|
||||
}
|
||||
if (!normalizedPeerId) {
|
||||
// Preserves the pre-existing "first match wins" semantics for peerless
|
||||
// callers (e.g. cron delivery resolution) whose only bindings are
|
||||
// peer-specific; otherwise they would silently regress to undefined.
|
||||
peerlessPeerSpecificFallback ??= resolved.accountId;
|
||||
}
|
||||
} else {
|
||||
channelOnlyFallback ??= resolved.accountId;
|
||||
}
|
||||
}
|
||||
return wildcardPeerMatch ?? channelOnlyFallback;
|
||||
return wildcardPeerMatch ?? channelOnlyFallback ?? peerlessPeerSpecificFallback;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user