mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 05:40:44 +00:00
fix(agents): gate sessions_send A2A skip on requester ownership
Greptile/Codex review follow-ups on #69817: - Narrow skipA2AFlow from target-only detection to a combined check that the caller is the parent of the target (new isRequesterParentOfBackgroundAcpSession helper). Under tools.sessions.visibility=all a non-parent sender can see the same oneshot ACP session; the previous guard would have suppressed their only follow-up delivery path. With requester ownership required, those senders continue through the normal A2A flow. - When the A2A flow is skipped, return delivery.status="skipped" instead of "pending" so the parent LLM does not wait for a second result that will never arrive. - Add unit tests for resolveAcpSessionInteractionMode and isRequesterParentOfBackgroundAcpSession covering both the new ownership gate and the existing target-type branches.
This commit is contained in:
committed by
Peter Steinberger
parent
1c3fbbd72a
commit
8a7c21407a
98
src/acp/session-interaction-mode.test.ts
Normal file
98
src/acp/session-interaction-mode.test.ts
Normal file
@@ -0,0 +1,98 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import {
|
||||
isParentOwnedBackgroundAcpSession,
|
||||
isRequesterParentOfBackgroundAcpSession,
|
||||
resolveAcpSessionInteractionMode,
|
||||
} from "./session-interaction-mode.js";
|
||||
|
||||
const parentKey = "agent:main:main";
|
||||
const otherKey = "agent:peer:some-other";
|
||||
|
||||
describe("resolveAcpSessionInteractionMode", () => {
|
||||
it("returns interactive when entry is undefined", () => {
|
||||
expect(resolveAcpSessionInteractionMode(undefined)).toBe("interactive");
|
||||
});
|
||||
|
||||
it("returns interactive for non-oneshot ACP sessions", () => {
|
||||
expect(
|
||||
resolveAcpSessionInteractionMode({
|
||||
acp: { mode: "persistent" } as never,
|
||||
spawnedBy: parentKey,
|
||||
}),
|
||||
).toBe("interactive");
|
||||
});
|
||||
|
||||
it("returns parent-owned-background for oneshot sessions with spawnedBy set", () => {
|
||||
expect(
|
||||
resolveAcpSessionInteractionMode({
|
||||
acp: { mode: "oneshot" } as never,
|
||||
spawnedBy: parentKey,
|
||||
}),
|
||||
).toBe("parent-owned-background");
|
||||
});
|
||||
|
||||
it("returns parent-owned-background for oneshot sessions with parentSessionKey set", () => {
|
||||
expect(
|
||||
resolveAcpSessionInteractionMode({
|
||||
acp: { mode: "oneshot" } as never,
|
||||
parentSessionKey: parentKey,
|
||||
}),
|
||||
).toBe("parent-owned-background");
|
||||
});
|
||||
|
||||
it("returns interactive for a oneshot session without any parent linkage", () => {
|
||||
expect(
|
||||
resolveAcpSessionInteractionMode({
|
||||
acp: { mode: "oneshot" } as never,
|
||||
}),
|
||||
).toBe("interactive");
|
||||
});
|
||||
});
|
||||
|
||||
describe("isRequesterParentOfBackgroundAcpSession", () => {
|
||||
const backgroundEntry = {
|
||||
acp: { mode: "oneshot" } as never,
|
||||
spawnedBy: parentKey,
|
||||
parentSessionKey: parentKey,
|
||||
};
|
||||
|
||||
it("returns true when requester matches spawnedBy", () => {
|
||||
expect(
|
||||
isRequesterParentOfBackgroundAcpSession(
|
||||
{ acp: { mode: "oneshot" } as never, spawnedBy: parentKey },
|
||||
parentKey,
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("returns true when requester matches parentSessionKey", () => {
|
||||
expect(
|
||||
isRequesterParentOfBackgroundAcpSession(
|
||||
{ acp: { mode: "oneshot" } as never, parentSessionKey: parentKey },
|
||||
parentKey,
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("returns false when requester is a different session (not the parent)", () => {
|
||||
expect(isRequesterParentOfBackgroundAcpSession(backgroundEntry, otherKey)).toBe(false);
|
||||
});
|
||||
|
||||
it("returns false when requester key is missing", () => {
|
||||
expect(isRequesterParentOfBackgroundAcpSession(backgroundEntry, undefined)).toBe(false);
|
||||
expect(isRequesterParentOfBackgroundAcpSession(backgroundEntry, "")).toBe(false);
|
||||
});
|
||||
|
||||
it("returns false when target is not a parent-owned background ACP session", () => {
|
||||
expect(
|
||||
isRequesterParentOfBackgroundAcpSession(
|
||||
{ acp: { mode: "persistent" } as never, spawnedBy: parentKey },
|
||||
parentKey,
|
||||
),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("delegates to isParentOwnedBackgroundAcpSession for target-only checks", () => {
|
||||
expect(isParentOwnedBackgroundAcpSession(backgroundEntry)).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -23,3 +23,30 @@ export function resolveAcpSessionInteractionMode(
|
||||
export function isParentOwnedBackgroundAcpSession(entry?: SessionInteractionEntry | null): boolean {
|
||||
return resolveAcpSessionInteractionMode(entry) === "parent-owned-background";
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns true when `entry` is a parent-owned background ACP session AND the
|
||||
* given `requesterSessionKey` is the session that spawned/owns it. This is a
|
||||
* strictly narrower check than {@link isParentOwnedBackgroundAcpSession}: the
|
||||
* target must match *and* the caller must be the parent.
|
||||
*
|
||||
* Used to gate behaviors that only make sense for the parent↔own-child pair
|
||||
* (e.g. skipping the A2A ping-pong flow in `sessions_send`), so that an
|
||||
* unrelated session with broad visibility (e.g. `tools.sessions.visibility=all`)
|
||||
* sending to the same target is still routed through the normal A2A path.
|
||||
*/
|
||||
export function isRequesterParentOfBackgroundAcpSession(
|
||||
entry: SessionInteractionEntry | null | undefined,
|
||||
requesterSessionKey: string | null | undefined,
|
||||
): boolean {
|
||||
if (!isParentOwnedBackgroundAcpSession(entry)) {
|
||||
return false;
|
||||
}
|
||||
const requester = normalizeOptionalString(requesterSessionKey);
|
||||
if (!requester) {
|
||||
return false;
|
||||
}
|
||||
const spawnedBy = normalizeOptionalString(entry?.spawnedBy);
|
||||
const parentSessionKey = normalizeOptionalString(entry?.parentSessionKey);
|
||||
return requester === spawnedBy || requester === parentSessionKey;
|
||||
}
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import crypto from "node:crypto";
|
||||
import { Type } from "@sinclair/typebox";
|
||||
import { isParentOwnedBackgroundAcpSession } from "../../acp/session-interaction-mode.js";
|
||||
import { isRequesterParentOfBackgroundAcpSession } from "../../acp/session-interaction-mode.js";
|
||||
import type { OpenClawConfig } from "../../config/types.openclaw.js";
|
||||
import { callGateway } from "../../gateway/call.js";
|
||||
import { formatErrorMessage } from "../../infra/errors.js";
|
||||
@@ -290,18 +290,33 @@ export function createSessionsSendTool(opts?: {
|
||||
const requesterSessionKey = opts?.agentSessionKey;
|
||||
const requesterChannel = opts?.agentChannel;
|
||||
const maxPingPongTurns = resolvePingPongTurns(cfg);
|
||||
const delivery = { status: "pending", mode: "announce" as const };
|
||||
|
||||
// Skip the A2A ping-pong + announce flow when the target is a
|
||||
// parent-owned background ACP subagent. Such sessions already report
|
||||
// their results back to the parent through the
|
||||
// `[Internal task completion event]` announcement path, and treating
|
||||
// Skip the A2A ping-pong + announce flow when the current caller is the
|
||||
// parent of a parent-owned background ACP subagent it spawned itself.
|
||||
// Such sessions already report their results back to the parent through
|
||||
// the `[Internal task completion event]` announcement path, and treating
|
||||
// them as a peer agent causes the parent to be woken with the child's
|
||||
// reply, generate a user-facing response, and have that response
|
||||
// forwarded back to the child as a new message — producing an infinite
|
||||
// ping-pong loop between parent and ACP child.
|
||||
// forwarded back to the child as a new message — producing a
|
||||
// ping-pong loop between parent and ACP child (bounded by
|
||||
// maxPingPongTurns, but user-visible as a runaway conversation).
|
||||
//
|
||||
// The skip is gated on requester ownership, not just target type: an
|
||||
// unrelated sender that can see the same target (e.g. under
|
||||
// `tools.sessions.visibility=all`) must still go through the normal A2A
|
||||
// path so it actually receives a follow-up delivery.
|
||||
const targetSessionEntry = loadSessionEntryByKey(resolvedKey);
|
||||
const skipA2AFlow = isParentOwnedBackgroundAcpSession(targetSessionEntry);
|
||||
const skipA2AFlow = isRequesterParentOfBackgroundAcpSession(
|
||||
targetSessionEntry,
|
||||
effectiveRequesterKey,
|
||||
);
|
||||
// When the A2A flow is skipped, no follow-up announcement will fire and
|
||||
// the reply (when present) is returned inline via the `reply` field.
|
||||
// Reflect that in the metadata so the parent LLM does not wait for a
|
||||
// second result that will never arrive.
|
||||
const delivery = skipA2AFlow
|
||||
? ({ status: "skipped", mode: "announce" } as const)
|
||||
: ({ status: "pending", mode: "announce" } as const);
|
||||
|
||||
const startA2AFlow = (roundOneReply?: string, waitRunId?: string) => {
|
||||
if (skipA2AFlow) {
|
||||
|
||||
Reference in New Issue
Block a user