From 445ed9b0b40abc690ed82b551d5615a67d494705 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 15 May 2026 21:05:44 +0100 Subject: [PATCH] fix(auto-reply): clean up no-reply migration paths --- docs/.generated/config-baseline.sha256 | 4 +- docs/concepts/messages.md | 5 +- src/agents/subagent-registry.ts | 18 ---- src/auto-reply/reply/groups.test.ts | 9 +- .../shared/legacy-config-migrate.test.ts | 22 +++++ ...legacy-config-migrations.runtime.agents.ts | 14 ++- src/infra/outbound/payloads.test.ts | 87 +------------------ src/infra/outbound/payloads.ts | 33 ------- src/infra/outbound/pending-spawn-query.ts | 57 ------------ 9 files changed, 39 insertions(+), 210 deletions(-) delete mode 100644 src/infra/outbound/pending-spawn-query.ts diff --git a/docs/.generated/config-baseline.sha256 b/docs/.generated/config-baseline.sha256 index b02018b7947..1b0eb4b00eb 100644 --- a/docs/.generated/config-baseline.sha256 +++ b/docs/.generated/config-baseline.sha256 @@ -1,4 +1,4 @@ -11a321fb232fb821b47721055fb2efdc79921453bd045f64c1186316ab59515a config-baseline.json -dbb937da47062678c8305a3111b986ee331440f1d39c2cf95bd284550f0bbf61 config-baseline.core.json +a8d34475e1b9f6ef9407172ba2e96d7f5783784073dd0dd5d5c21190613aa002 config-baseline.json +60316e075939edea25391c4d5bd2bbc8205b16239d605ecf922033118ae1bfe5 config-baseline.core.json fe4f1cb00d7d1dee9746779ec3cf14236e5f672c91502268a12ad6e467a2c4ad config-baseline.channel.json e9049ce0154f484f44bb0ac174a44198269256044da5ba62a6e107e78bfd7a70 config-baseline.plugin.json diff --git a/docs/concepts/messages.md b/docs/concepts/messages.md index c49423fba89..f258bf8d655 100644 --- a/docs/concepts/messages.md +++ b/docs/concepts/messages.md @@ -202,9 +202,8 @@ raw runner details are shown only when `/verbose` is `on` or `full`. Defaults live under `agents.defaults.silentReply`; `surfaces..silentReply` can override group/internal policy per surface. -When the parent session has one or more pending spawned subagent runs, bare -silent replies are dropped on all surfaces, so the parent stays quiet until the -child completion event delivers the real reply. +Bare silent replies are dropped on all surfaces, so parent sessions stay quiet +instead of rewriting sentinel text into fallback chatter. ## Related diff --git a/src/agents/subagent-registry.ts b/src/agents/subagent-registry.ts index 931bea248c3..96112bc67b5 100644 --- a/src/agents/subagent-registry.ts +++ b/src/agents/subagent-registry.ts @@ -11,7 +11,6 @@ import type { ResolveContextEngineOptions } from "../context-engine/registry.js" import type { ContextEngine, SubagentEndReason } from "../context-engine/types.js"; import { callGateway } from "../gateway/call.js"; import { getAgentRunContext, onAgentEvent } from "../infra/agent-events.js"; -import { registerPendingSpawnedChildrenQuery } from "../infra/outbound/pending-spawn-query.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; import { createLazyImportLoader, createLazyPromiseLoader } from "../shared/lazy-promise.js"; import { importRuntimeModule } from "../shared/runtime-import.js"; @@ -1244,20 +1243,3 @@ export function initSubagentRegistry() { // Importing this module also registers the subagent maintenance preserve-key // provider as a side effect (see subagent-registry-maintenance.ts). export { listSessionMaintenanceProtectedSubagentSessionKeys } from "./subagent-registry-maintenance.js"; - -// Let the shared outbound plan treat bare silent replies as dropped (instead -// of rewriting them to visible fallback text) when the parent session has at -// least one pending spawned child whose completion will deliver the real -// reply. Uses the pending-descendant count so runs that have ended but whose -// announce/cleanup is still in flight continue to suppress rewriting; without -// this the window between `completeSubagentRun` setting `endedAt` and -// `startSubagentAnnounceCleanupFlow` finishing could briefly re-enable -// fallback chatter. Runtime-enforced, so it does not rely on agent prompt -// compliance. -registerPendingSpawnedChildrenQuery((sessionKey) => { - const key = sessionKey?.trim(); - if (!key) { - return false; - } - return countPendingDescendantRuns(key) > 0; -}); diff --git a/src/auto-reply/reply/groups.test.ts b/src/auto-reply/reply/groups.test.ts index 7c432dcadb7..6fed97955e4 100644 --- a/src/auto-reply/reply/groups.test.ts +++ b/src/auto-reply/reply/groups.test.ts @@ -63,7 +63,7 @@ describe("group runtime loading", () => { vi.doUnmock("./groups.runtime.js"); }); - it("builds direct chat context from the resolved silent reply policy", () => { + it("builds direct chat context without silent-token guidance", () => { expect( groups.buildDirectChatContext({ sessionCtx: { ChatType: "direct", Provider: "telegram" }, @@ -71,13 +71,6 @@ describe("group runtime loading", () => { ).toBe( "You are in a Telegram direct conversation. Your replies are automatically sent to this conversation.", ); - - expect( - groups.buildDirectChatContext({ - sessionCtx: { ChatType: "direct", Provider: "telegram" }, - }), - ).not.toContain("NO_REPLY"); - expect( groups.buildDirectChatContext({ sessionCtx: { ChatType: "direct", Provider: "telegram" }, diff --git a/src/commands/doctor/shared/legacy-config-migrate.test.ts b/src/commands/doctor/shared/legacy-config-migrate.test.ts index 0835ce08906..c4b169b5c46 100644 --- a/src/commands/doctor/shared/legacy-config-migrate.test.ts +++ b/src/commands/doctor/shared/legacy-config-migrate.test.ts @@ -69,6 +69,28 @@ describe("legacy silent reply config migrate", () => { "Removed surfaces.telegram.silentReplyRewrite", ]); }); + + it("removes malformed silent reply rewrite keys by presence", () => { + const res = migrateLegacyConfigForTest({ + agents: { + defaults: { + silentReplyRewrite: true, + }, + }, + surfaces: { + telegram: { + silentReplyRewrite: false, + }, + }, + }); + + expect(res.config?.agents?.defaults).not.toHaveProperty("silentReplyRewrite"); + expect(res.config?.surfaces?.telegram).not.toHaveProperty("silentReplyRewrite"); + expectMigrationChangesToIncludeFragments(res.changes, [ + "Removed agents.defaults.silentReplyRewrite", + "Removed surfaces.telegram.silentReplyRewrite", + ]); + }); }); describe("legacy session maintenance migrate", () => { diff --git a/src/commands/doctor/shared/legacy-config-migrations.runtime.agents.ts b/src/commands/doctor/shared/legacy-config-migrations.runtime.agents.ts index 87332ed4a6a..c5877eb67c7 100644 --- a/src/commands/doctor/shared/legacy-config-migrations.runtime.agents.ts +++ b/src/commands/doctor/shared/legacy-config-migrations.runtime.agents.ts @@ -241,13 +241,19 @@ function removeLegacyAgentRuntimePolicy( } } +function hasOwnRecordProperty(value: unknown, key: string): boolean { + const record = getRecord(value); + return Boolean(record && Object.prototype.hasOwnProperty.call(record, key)); +} + function hasSurfaceSilentReplyRewrite(value: unknown): boolean { const surfaces = getRecord(value); if (!surfaces) { return false; } - return Object.values(surfaces).some( - (surface) => getRecord(getRecord(surface)?.silentReplyRewrite) !== null, + return Object.entries(surfaces).some( + ([surfaceId, surface]) => + !isBlockedObjectKey(surfaceId) && hasOwnRecordProperty(surface, "silentReplyRewrite"), ); } @@ -271,7 +277,7 @@ function removeLegacySilentReplyConfig(raw: Record, changes: st delete defaultSilentReply.direct; changes.push("Removed agents.defaults.silentReply.direct; direct chats never use NO_REPLY."); } - if (defaults && getRecord(defaults.silentReplyRewrite) !== null) { + if (defaults && hasOwnRecordProperty(defaults, "silentReplyRewrite")) { delete defaults.silentReplyRewrite; changes.push("Removed agents.defaults.silentReplyRewrite."); } @@ -295,7 +301,7 @@ function removeLegacySilentReplyConfig(raw: Record, changes: st `Removed surfaces.${surfaceId}.silentReply.direct; direct chats never use NO_REPLY.`, ); } - if (getRecord(surface.silentReplyRewrite) !== null) { + if (hasOwnRecordProperty(surface, "silentReplyRewrite")) { delete surface.silentReplyRewrite; changes.push(`Removed surfaces.${surfaceId}.silentReplyRewrite.`); } diff --git a/src/infra/outbound/payloads.test.ts b/src/infra/outbound/payloads.test.ts index ea12a3c771e..b837060b4ed 100644 --- a/src/infra/outbound/payloads.test.ts +++ b/src/infra/outbound/payloads.test.ts @@ -1,7 +1,6 @@ import { resolveSendableOutboundReplyParts } from "openclaw/plugin-sdk/reply-payload"; import { describe, expect, it } from "vitest"; import type { ReplyPayload } from "../../auto-reply/types.js"; -import type { OpenClawConfig } from "../../config/types.openclaw.js"; import { typedCases } from "../../test-utils/typed-cases.js"; import { createOutboundPayloadPlan, @@ -15,7 +14,6 @@ import { projectOutboundPayloadPlanForOutbound, summarizeOutboundPayloadForTransport, } from "./payloads.js"; -import { registerPendingSpawnedChildrenQuery } from "./pending-spawn-query.js"; function resolveMirrorProjection(payloads: readonly ReplyPayload[]) { const normalized = normalizeReplyPayloadsForDelivery(payloads); @@ -191,45 +189,20 @@ describe("normalizeReplyPayloadsForDelivery", () => { }); it("drops bare silent replies for direct conversations", () => { - const cfg: OpenClawConfig = { - agents: { - defaults: { - silentReply: { - group: "allow", - internal: "allow", - }, - }, - }, - }; - - const sessionKey = "agent:main:telegram:direct:123"; expect( projectOutboundPayloadPlanForDelivery( createOutboundPayloadPlan([{ text: "NO_REPLY" }], { - cfg, - sessionKey, + sessionKey: "agent:main:telegram:direct:123", surface: "telegram", }), ), ).toStrictEqual([]); }); - it("drops bare silent replies for groups when policy allows silence", () => { - const cfg: OpenClawConfig = { - agents: { - defaults: { - silentReply: { - group: "allow", - internal: "allow", - }, - }, - }, - }; - + it("drops bare silent replies for groups", () => { expect( projectOutboundPayloadPlanForDelivery( createOutboundPayloadPlan([{ text: "NO_REPLY" }], { - cfg, sessionKey: "agent:main:telegram:group:123", surface: "telegram", }), @@ -238,20 +211,8 @@ describe("normalizeReplyPayloadsForDelivery", () => { }); it("does not add silent-reply chatter when visible content is already being delivered", () => { - const cfg: OpenClawConfig = { - agents: { - defaults: { - silentReply: { - group: "allow", - internal: "allow", - }, - }, - }, - }; - const delivery = projectOutboundPayloadPlanForDelivery( createOutboundPayloadPlan([{ text: "NO_REPLY" }, { text: "visible reply" }], { - cfg, sessionKey: "agent:main:telegram:direct:123", surface: "telegram", }), @@ -260,50 +221,6 @@ describe("normalizeReplyPayloadsForDelivery", () => { expect(delivery[0]?.text).toBe("visible reply"); }); - describe("pending spawned subagent children", () => { - const cfg: OpenClawConfig = { - agents: { - defaults: { - silentReply: { group: "allow", internal: "allow" }, - }, - }, - }; - const planSilent = (sessionKey: string, hasPendingSpawnedChildren?: boolean) => - projectOutboundPayloadPlanForDelivery( - createOutboundPayloadPlan([{ text: "NO_REPLY" }], { - cfg, - sessionKey, - surface: "telegram", - hasPendingSpawnedChildren, - }), - ); - - it("drops bare silent replies when the context flag is set", () => { - expect(planSilent("agent:main:telegram:direct:123", true)).toStrictEqual([]); - }); - - it("drops bare silent replies via the registered runtime query", () => { - const sessionKey = "agent:main:telegram:direct:456"; - const previousQuery = registerPendingSpawnedChildrenQuery((key) => key === sessionKey); - try { - expect(planSilent(sessionKey)).toStrictEqual([]); - } finally { - registerPendingSpawnedChildrenQuery(previousQuery); - } - }); - - it("still drops bare silent replies when the query throws", () => { - const previousQuery = registerPendingSpawnedChildrenQuery(() => { - throw new Error("registry unavailable"); - }); - try { - expect(planSilent("agent:main:telegram:direct:789")).toStrictEqual([]); - } finally { - registerPendingSpawnedChildrenQuery(previousQuery); - } - }); - }); - it("is idempotent for already-normalized delivery payloads", () => { const once = normalizeReplyPayloadsForDelivery([ { diff --git a/src/infra/outbound/payloads.ts b/src/infra/outbound/payloads.ts index cc5630f42a2..4cb509eb083 100644 --- a/src/infra/outbound/payloads.ts +++ b/src/infra/outbound/payloads.ts @@ -6,7 +6,6 @@ import { shouldSuppressReasoningPayload, } from "../../auto-reply/reply/reply-payloads.js"; import type { ReplyPayload } from "../../auto-reply/types.js"; -import { resolveSilentReplySettings } from "../../config/silent-reply.js"; import type { OpenClawConfig } from "../../config/types.openclaw.js"; import { hasInteractiveReplyBlocks, @@ -18,7 +17,6 @@ import { type ReplyPayloadDelivery, } from "../../interactive/payload.js"; import { type SilentReplyConversationType } from "../../shared/silent-reply-policy.js"; -import { resolvePendingSpawnedChildren } from "./pending-spawn-query.js"; export type NormalizedOutboundPayload = { text: string; @@ -57,13 +55,6 @@ type OutboundPayloadPlanContext = { sessionKey?: string; surface?: string; conversationType?: SilentReplyConversationType; - /** - * Set by callers that know the parent session has at least one pending - * spawned child whose completion will deliver the real reply. If omitted, the - * outbound plan consults the registered runtime query (see - * `pending-spawn-query.ts`). - */ - hasPendingSpawnedChildren?: boolean; extractMarkdownImages?: boolean; }; @@ -188,14 +179,6 @@ export function createOutboundPayloadPlan( // Intentionally scoped to channel-agnostic normalization and projection inputs. // Transport concerns (queueing, hooks, retries), channel transforms, and // heartbeat-specific token semantics remain outside this plan boundary. - const resolvedSilentReplySettings = resolveSilentReplySettings({ - cfg: context.cfg, - sessionKey: context.sessionKey, - surface: context.surface, - conversationType: context.conversationType, - }); - const hasPendingSpawnedChildren = - context.hasPendingSpawnedChildren ?? resolvePendingSpawnedChildren(context.sessionKey); const prepared: IndexedPreparedOutboundPayloadPlanEntry[] = []; for (const [sourceIndex, payload] of payloads.entries()) { const entry = createOutboundPayloadPlanEntry(payload, { @@ -206,16 +189,6 @@ export function createOutboundPayloadPlan( } prepared.push({ ...entry, sourceIndex }); } - const hasVisibleNonSilentContent = prepared.some((entry) => { - if (entry.isSilent) { - return false; - } - const parts = resolveSendableOutboundReplyParts(entry.payload); - return hasReplyPayloadContent( - { ...entry.payload, text: parts.text, mediaUrls: parts.mediaUrls }, - { hasChannelData: entry.hasChannelData }, - ); - }); const plan: OutboundPayloadPlan[] = []; for (const entry of prepared) { if (!entry.isSilent) { @@ -229,12 +202,6 @@ export function createOutboundPayloadPlan( }); continue; } - if (hasVisibleNonSilentContent || resolvedSilentReplySettings.policy === "allow") { - continue; - } - if (hasPendingSpawnedChildren) { - continue; - } } return plan; } diff --git a/src/infra/outbound/pending-spawn-query.ts b/src/infra/outbound/pending-spawn-query.ts deleted file mode 100644 index fff186e9a40..00000000000 --- a/src/infra/outbound/pending-spawn-query.ts +++ /dev/null @@ -1,57 +0,0 @@ -import { createHash } from "node:crypto"; -import { createSubsystemLogger } from "../../logging/subsystem.js"; - -/** - * Synchronous predicate: does `sessionKey` have pending spawned subagent runs? - * Runs on the outbound plan hot path, so implementations must be cheap/bounded - * (default in `subagent-registry.ts` is an in-memory map lookup). Internal to - * core; not re-exported through `openclaw/plugin-sdk`. - */ -export type PendingSpawnedChildrenQuery = (sessionKey?: string) => boolean; - -const log = createSubsystemLogger("outbound/pending-spawn"); -const THROW_LOG_INTERVAL_MS = 60_000; -let lastThrowLogAt = 0; -let pendingSpawnedChildrenQuery: PendingSpawnedChildrenQuery | undefined; - -export function registerPendingSpawnedChildrenQuery( - query: PendingSpawnedChildrenQuery | undefined, -): PendingSpawnedChildrenQuery | undefined { - const previous = pendingSpawnedChildrenQuery; - pendingSpawnedChildrenQuery = query; - return previous; -} - -function summarizeError(err: unknown): { name: string; message: string } { - if (err instanceof Error) { - return { name: err.name, message: err.message }; - } - return { name: "Unknown", message: typeof err === "string" ? err : "non-error throw" }; -} - -function hashSessionKey(key: string | undefined): string | undefined { - const trimmed = key?.trim(); - if (!trimmed) { - return undefined; - } - return createHash("sha256").update(trimmed).digest("hex").slice(0, 12); -} - -export function resolvePendingSpawnedChildren(sessionKey: string | undefined): boolean { - if (!pendingSpawnedChildrenQuery) { - return false; - } - try { - return pendingSpawnedChildrenQuery(sessionKey); - } catch (err) { - const now = Date.now(); - if (now - lastThrowLogAt >= THROW_LOG_INTERVAL_MS) { - lastThrowLogAt = now; - log.warn("pending-spawn query threw; defaulting to false", { - err: summarizeError(err), - sessionKeyHash: hashSessionKey(sessionKey), - }); - } - return false; - } -}