From 595ac6eb2848dfd2fb209d2bd1d903f17584a029 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 10 Aug 2026 18:42:31 -0700 Subject: [PATCH] test(agents): remove aggregate command test facade (#121789) --- .../agent-command.ingress-diagnostics.test.ts | 144 +++--------------- .../agent-command.live-model-switch.test.ts | 19 --- src/agents/agent-command.ts | 21 +-- src/agents/command/ingress-diagnostics.ts | 2 +- src/agents/command/lifecycle.test.ts | 41 ++++- src/agents/command/lifecycle.ts | 17 +-- src/agents/command/prepare.ts | 2 +- src/commands/agent.test.ts | 7 +- src/commands/agent.worktree-race.test.ts | 9 +- 9 files changed, 74 insertions(+), 188 deletions(-) diff --git a/src/agents/agent-command.ingress-diagnostics.test.ts b/src/agents/agent-command.ingress-diagnostics.test.ts index f0336ee1447f..134de3713424 100644 --- a/src/agents/agent-command.ingress-diagnostics.test.ts +++ b/src/agents/agent-command.ingress-diagnostics.test.ts @@ -8,7 +8,8 @@ * - emitIngressModelUsageDiagnostic with null/missing usage */ -import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { emitIngressModelUsageDiagnostic } from "./command/ingress-diagnostics.js"; const mocks = vi.hoisted(() => ({ emitTrustedDiagnosticEvent: vi.fn(), @@ -43,13 +44,6 @@ vi.mock("../config/io.js", () => ({ getRuntimeConfig: () => mocks.getRuntimeConfig(), })); -let testing: typeof import("./agent-command.js").testing; - -beforeAll(async () => { - const mod = await import("./agent-command.js"); - testing = mod.testing; -}); - beforeEach(() => { vi.clearAllMocks(); mocks.isDiagnosticsEnabled.mockReturnValue(true); @@ -63,63 +57,6 @@ afterEach(() => { vi.clearAllMocks(); }); -describe("resolveAgentRunLifecycleEndLogLevel", () => { - it("logs successful stop and tool-use metadata at info", () => { - expect( - testing.resolveAgentRunLifecycleEndLogLevel({ - aborted: false, - stopReason: "stop", - }), - ).toBe("info"); - expect( - testing.resolveAgentRunLifecycleEndLogLevel({ - aborted: false, - stopReason: "toolUse", - }), - ).toBe("info"); - }); - - it("does not log ordinary end-turn completions", () => { - expect( - testing.resolveAgentRunLifecycleEndLogLevel({ - aborted: false, - stopReason: "end_turn", - }), - ).toBeUndefined(); - expect(testing.resolveAgentRunLifecycleEndLogLevel({ aborted: false })).toBeUndefined(); - }); - - it("keeps timeout metadata out of error severity", () => { - expect( - testing.resolveAgentRunLifecycleEndLogLevel({ - aborted: true, - stopReason: "timeout", - }), - ).toBe("warn"); - expect( - testing.resolveAgentRunLifecycleEndLogLevel({ - stopReason: "stop", - timeoutPhase: "provider", - providerStarted: true, - }), - ).toBe("warn"); - }); - - it("logs cancelled and failed endings at error", () => { - expect( - testing.resolveAgentRunLifecycleEndLogLevel({ - aborted: true, - stopReason: "stop", - }), - ).toBe("error"); - expect( - testing.resolveAgentRunLifecycleEndLogLevel({ - stopReason: "error", - }), - ).toBe("error"); - }); -}); - function makeResult(overrides?: Record) { return { payloads: [{ text: "hello", mediaUrl: "" }], @@ -160,58 +97,12 @@ function makeOpts(overrides?: Record) { }; } -// --------------------------------------------------------------------------- -// ingressDiagnosticChannel -// --------------------------------------------------------------------------- -describe("ingressDiagnosticChannel", () => { - it("returns runContext.messageChannel when set", () => { - const channel = testing.ingressDiagnosticChannel({ - message: "hi", - allowModelOverride: false, - runContext: { messageChannel: "discord" }, - messageChannel: "api", - channel: "http", - }); - expect(channel).toBe("discord"); - }); - - it("falls back to opts.messageChannel", () => { - const channel = testing.ingressDiagnosticChannel({ - message: "hi", - allowModelOverride: false, - messageChannel: "api", - channel: "http", - }); - expect(channel).toBe("api"); - }); - - it("falls back to opts.channel", () => { - const channel = testing.ingressDiagnosticChannel({ - message: "hi", - allowModelOverride: false, - channel: "webchat", - }); - expect(channel).toBe("webchat"); - }); - - it('defaults to "http" when no channel info is present', () => { - const channel = testing.ingressDiagnosticChannel({ - message: "hi", - allowModelOverride: false, - }); - expect(channel).toBe("http"); - }); -}); - -// --------------------------------------------------------------------------- -// emitIngressModelUsageDiagnostic -// --------------------------------------------------------------------------- describe("emitIngressModelUsageDiagnostic", () => { it("emits model.usage when diagnostics are enabled and result has usage", () => { const result = makeResult(); const opts = makeOpts(); - testing.emitIngressModelUsageDiagnostic(result, opts); + emitIngressModelUsageDiagnostic(result, opts); expect(mocks.emitTrustedDiagnosticEvent).toHaveBeenCalledTimes(1); const event = mocks.emitTrustedDiagnosticEvent.mock.calls[0]?.[0]; @@ -248,7 +139,7 @@ describe("emitIngressModelUsageDiagnostic", () => { }, }); - testing.emitIngressModelUsageDiagnostic(result, makeOpts()); + emitIngressModelUsageDiagnostic(result, makeOpts()); expect(mocks.estimateUsageCost).toHaveBeenCalledWith({ usage: { @@ -281,7 +172,7 @@ describe("emitIngressModelUsageDiagnostic", () => { const result = makeResult(); const opts = makeOpts(); - testing.emitIngressModelUsageDiagnostic(result, opts); + emitIngressModelUsageDiagnostic(result, opts); expect(mocks.emitTrustedDiagnosticEvent).not.toHaveBeenCalled(); }); @@ -295,7 +186,7 @@ describe("emitIngressModelUsageDiagnostic", () => { const opts = makeOpts(); - testing.emitIngressModelUsageDiagnostic(result, opts); + emitIngressModelUsageDiagnostic(result, opts); expect(mocks.emitTrustedDiagnosticEvent).not.toHaveBeenCalled(); }); @@ -305,7 +196,7 @@ describe("emitIngressModelUsageDiagnostic", () => { const result = makeResult(); const opts = makeOpts(); - testing.emitIngressModelUsageDiagnostic(result, opts); + emitIngressModelUsageDiagnostic(result, opts); expect(mocks.emitTrustedDiagnosticEvent).not.toHaveBeenCalled(); }); @@ -317,18 +208,29 @@ describe("emitIngressModelUsageDiagnostic", () => { messageChannel: "api", }); - testing.emitIngressModelUsageDiagnostic(result, opts); + emitIngressModelUsageDiagnostic(result, opts); expect(mocks.emitTrustedDiagnosticEvent).toHaveBeenCalledTimes(1); const event = mocks.emitTrustedDiagnosticEvent.mock.calls[0]?.[0]; expect(event.channel).toBe("discord"); }); + it("falls back to opts.channel when messageChannel is absent", () => { + const result = makeResult(); + const opts = makeOpts({ messageChannel: undefined, channel: "webchat" }); + + emitIngressModelUsageDiagnostic(result, opts); + + expect(mocks.emitTrustedDiagnosticEvent).toHaveBeenCalledTimes(1); + const event = mocks.emitTrustedDiagnosticEvent.mock.calls[0]?.[0]; + expect(event.channel).toBe("webchat"); + }); + it('defaults channel to "http" when no channel info is present', () => { const result = makeResult(); const opts = { message: "hi", allowModelOverride: false }; - testing.emitIngressModelUsageDiagnostic(result, opts); + emitIngressModelUsageDiagnostic(result, opts); expect(mocks.emitTrustedDiagnosticEvent).toHaveBeenCalledTimes(1); const event = mocks.emitTrustedDiagnosticEvent.mock.calls[0]?.[0]; @@ -339,7 +241,7 @@ describe("emitIngressModelUsageDiagnostic", () => { const result = makeResult(); const opts = makeOpts(); - testing.emitIngressModelUsageDiagnostic(result, opts); + emitIngressModelUsageDiagnostic(result, opts); expect(mocks.resolveModelCostConfig).toHaveBeenCalledWith({ provider: "openai", @@ -363,7 +265,7 @@ describe("emitIngressModelUsageDiagnostic", () => { }); const opts = makeOpts(); - testing.emitIngressModelUsageDiagnostic(result, opts); + emitIngressModelUsageDiagnostic(result, opts); expect(mocks.emitTrustedDiagnosticEvent).toHaveBeenCalledTimes(1); const event = mocks.emitTrustedDiagnosticEvent.mock.calls[0]?.[0]; @@ -390,7 +292,7 @@ describe("emitIngressModelUsageDiagnostic", () => { }); const opts = makeOpts(); - testing.emitIngressModelUsageDiagnostic(result, opts); + emitIngressModelUsageDiagnostic(result, opts); expect(mocks.emitTrustedDiagnosticEvent).toHaveBeenCalledTimes(1); const event = mocks.emitTrustedDiagnosticEvent.mock.calls[0]?.[0]; diff --git a/src/agents/agent-command.live-model-switch.test.ts b/src/agents/agent-command.live-model-switch.test.ts index 08fa94707065..046a51fe1c02 100644 --- a/src/agents/agent-command.live-model-switch.test.ts +++ b/src/agents/agent-command.live-model-switch.test.ts @@ -670,13 +670,11 @@ vi.mock("../acp/control-plane/manager.js", () => ({ let agentCommand: typeof import("./agent-command.js").agentCommand; let agentCommandFromSystem: typeof import("./agent-command.js").agentCommandFromSystem; -let agentCommandTesting: typeof import("./agent-command.js").testing; beforeAll(async () => { const mod = await import("./agent-command.js"); agentCommand ??= mod.agentCommand; agentCommandFromSystem ??= mod.agentCommandFromSystem; - agentCommandTesting ??= mod.testing; }); type FallbackRunnerParams = { @@ -2213,23 +2211,6 @@ describe("agentCommand – LiveSessionModelSwitchError retry", () => { }); }); - it("scopes explicit-agent sentinel store keys before command routing", () => { - expect( - agentCommandTesting.resolveExplicitAgentCommandSessionKey({ - rawExplicitSessionKey: "global", - agentIdOverride: "work", - cfg: {}, - }), - ).toBe("agent:work:global"); - expect( - agentCommandTesting.resolveExplicitAgentCommandSessionKey({ - rawExplicitSessionKey: "main", - agentIdOverride: "work", - cfg: {}, - }), - ).toBe("agent:work:main"); - }); - it("persists explicit overrides even when ingress skips the initial touch", async () => { setupSingleAttemptFallback(); state.runAgentAttemptMock.mockResolvedValue(makeSuccessResult("openai", "gpt-5.4")); diff --git a/src/agents/agent-command.ts b/src/agents/agent-command.ts index cd29ca32ba3e..52ee102436dd 100644 --- a/src/agents/agent-command.ts +++ b/src/agents/agent-command.ts @@ -30,20 +30,12 @@ import { shouldPersistRestartRecoveryCleanup, shouldPersistRestartRecoveryContextClaim, } from "./agent-command-restart-recovery.js"; -import { resolveAgentRuntimeConfig } from "./agent-runtime-config.js"; import { runAcpAgentCommand } from "./command/acp-execution.js"; import { repairPendingAssistantTranscriptTurns } from "./command/assistant-transcript-repair.js"; -import { - emitIngressModelUsageDiagnostic, - ingressDiagnosticChannel, -} from "./command/ingress-diagnostics.js"; -import { resolveAgentRunLifecycleEndLogLevel } from "./command/lifecycle.js"; +import { emitIngressModelUsageDiagnostic } from "./command/ingress-diagnostics.js"; import { resolveEmbeddedModelSelection } from "./command/model-selection.js"; import { finalizeEmbeddedAgentCommand } from "./command/post-run.js"; -import { - prepareAgentCommandExecution, - resolveExplicitAgentCommandSessionKey, -} from "./command/prepare.js"; +import { prepareAgentCommandExecution } from "./command/prepare.js"; import { runEmbeddedAgentAttempt } from "./command/run-embedded-attempt.js"; import { loadSessionStoreRuntime, resolveAgentCommandDeps } from "./command/runtime-loaders.js"; import { persistSessionEntry, prepareCurrentRunDelivery } from "./command/session-helpers.js"; @@ -694,12 +686,3 @@ export async function agentCommandFromGatewayIngress( ) { return await agentCommandFromIngressInternal(opts, runtime, deps, recovery); } - -export const testing = { - resolveAgentRuntimeConfig, - prepareAgentCommandExecution, - resolveExplicitAgentCommandSessionKey, - resolveAgentRunLifecycleEndLogLevel, - ingressDiagnosticChannel, - emitIngressModelUsageDiagnostic, -}; diff --git a/src/agents/command/ingress-diagnostics.ts b/src/agents/command/ingress-diagnostics.ts index 979d53f96705..abc51f376031 100644 --- a/src/agents/command/ingress-diagnostics.ts +++ b/src/agents/command/ingress-diagnostics.ts @@ -29,7 +29,7 @@ type AgentCommandResult = { }; /** Resolve the channel label for model.usage diagnostics from ingress run options. */ -export function ingressDiagnosticChannel(opts: AgentCommandIngressOpts): string { +function ingressDiagnosticChannel(opts: AgentCommandIngressOpts): string { return opts.runContext?.messageChannel ?? opts.messageChannel ?? opts.channel ?? "http"; } diff --git a/src/agents/command/lifecycle.test.ts b/src/agents/command/lifecycle.test.ts index d70e0e8a22b6..b7982b046400 100644 --- a/src/agents/command/lifecycle.test.ts +++ b/src/agents/command/lifecycle.test.ts @@ -2,14 +2,51 @@ import { describe, expect, it, vi } from "vitest"; import { buildAgentRunTerminalOutcome } from "../agent-run-terminal-outcome.js"; import { createAgentCommandLifecycle } from "./lifecycle.js"; -const emitAgentEvent = vi.hoisted(() => vi.fn()); +const { emitAgentEvent, lifecycleLog } = vi.hoisted(() => ({ + emitAgentEvent: vi.fn(), + lifecycleLog: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, +})); vi.mock("../../infra/agent-events.js", () => ({ emitAgentEvent })); vi.mock("../../logging/subsystem.js", () => ({ - createSubsystemLogger: () => ({ info: vi.fn(), warn: vi.fn(), error: vi.fn() }), + createSubsystemLogger: () => lifecycleLog, })); describe("createAgentCommandLifecycle", () => { + it.each([ + { name: "successful stops", status: "ok", stopReason: "stop", level: "info" }, + { name: "tool-use stops", status: "ok", stopReason: "toolUse", level: "info" }, + { name: "ordinary end turns", status: "ok", stopReason: "end_turn", level: undefined }, + { name: "timeouts", status: "timeout", stopReason: "timeout", level: "warn" }, + { name: "cancelled runs", status: "error", stopReason: "stop", level: "error" }, + { name: "failed runs", status: "error", stopReason: "error", level: "error" }, + ] as const)("logs $name at the expected severity", ({ status, stopReason, level }) => { + vi.clearAllMocks(); + const lifecycle = createAgentCommandLifecycle({ + runId: "logged-terminal-owner", + lifecycleGeneration: () => "test-generation", + startedAt: 100, + state: { + currentTurnUserMessagePersisted: true, + lifecycleFinishing: false, + lifecycleEnded: false, + }, + }); + + lifecycle.emitEnd({ + metadata: {}, + outcome: buildAgentRunTerminalOutcome({ status, stopReason }), + }); + + for (const candidate of ["info", "warn", "error"] as const) { + if (candidate === level) { + expect(lifecycleLog[candidate]).toHaveBeenCalledOnce(); + } else { + expect(lifecycleLog[candidate]).not.toHaveBeenCalled(); + } + } + }); + it.each(["finishing", "end", "error"] as const)( "preserves only canonical terminal facts on %s events", (phase) => { diff --git a/src/agents/command/lifecycle.ts b/src/agents/command/lifecycle.ts index ba0c414b9f21..50fe25178d17 100644 --- a/src/agents/command/lifecycle.ts +++ b/src/agents/command/lifecycle.ts @@ -2,10 +2,7 @@ import { emitAgentEvent } from "../../infra/agent-events.js"; import { formatErrorMessage } from "../../infra/errors.js"; import { createSubsystemLogger } from "../../logging/subsystem.js"; import { normalizeAgentRunTerminalDeliverySnapshot } from "../agent-run-terminal-delivery.js"; -import { - buildAgentRunTerminalOutcomeFromLifecycleEvent, - type AgentRunTerminalOutcome, -} from "../agent-run-terminal-outcome.js"; +import type { AgentRunTerminalOutcome } from "../agent-run-terminal-outcome.js"; import { normalizeAgentRunTerminalReceipt } from "../agent-run-terminal-receipt.js"; import type { EmbeddedAgentRunEntryTerminal } from "../embedded-agent-runner/run-entry.js"; import { @@ -30,18 +27,6 @@ function resolveTerminalLogLevel( return outcome.status === "timeout" ? "warn" : "error"; } -export function resolveAgentRunLifecycleEndLogLevel(meta: { - aborted?: unknown; - error?: unknown; - stopReason?: unknown; - livenessState?: unknown; - timeoutPhase?: unknown; - providerStarted?: unknown; -}): "info" | "warn" | "error" | undefined { - const outcome = buildAgentRunTerminalOutcomeFromLifecycleEvent({ phase: "end", data: meta }); - return resolveTerminalLogLevel(outcome); -} - export function applyAgentRunAbortMetadata( result: T, signal: AbortSignal | undefined, diff --git a/src/agents/command/prepare.ts b/src/agents/command/prepare.ts index ce3fc758a011..7924c0700430 100644 --- a/src/agents/command/prepare.ts +++ b/src/agents/command/prepare.ts @@ -94,7 +94,7 @@ export function normalizeExplicitOverrideInput(raw: string, kind: "provider" | " return trimmed; } -export function resolveExplicitAgentCommandSessionKey(params: { +function resolveExplicitAgentCommandSessionKey(params: { rawExplicitSessionKey?: string; agentIdOverride?: string; shouldScopeDefaultAgentKey?: boolean; diff --git a/src/commands/agent.test.ts b/src/commands/agent.test.ts index 82fe17ec813b..eabe873b34c0 100644 --- a/src/commands/agent.test.ts +++ b/src/commands/agent.test.ts @@ -12,6 +12,7 @@ import { executionIdentity } from "../agents/agent-command-execution-identity.js import * as authProfileStoreModule from "../agents/auth-profiles/store.js"; import * as attemptExecutionRuntime from "../agents/command/attempt-execution.runtime.js"; import { deliverAgentCommandResult } from "../agents/command/delivery.runtime.js"; +import { prepareAgentCommandExecution } from "../agents/command/prepare.js"; import { runEmbeddedAgent } from "../agents/embedded-agent.js"; import { loadManifestModelCatalog } from "../agents/model-catalog.js"; import * as modelSelectionModule from "../agents/model-selection.js"; @@ -46,7 +47,7 @@ import { normalizeSessionDeliveryState, } from "../utils/delivery-context.shared.js"; import { getAgentHarnessPluginMocks } from "./agent-command-state.test-mocks.js"; -import { agentCommand, agentCommandFromIngress, testing as agentCommandTesting } from "./agent.js"; +import { agentCommand, agentCommandFromIngress } from "./agent.js"; import { createThrowingTestRuntime } from "./test-runtime-config-helpers.js"; const configIoMocks = vi.hoisted(() => ({ @@ -1163,7 +1164,7 @@ describe("agentCommand", () => { }); mockConfig(home, store, { models: {} }); - const prepared = await agentCommandTesting.prepareAgentCommandExecution( + const prepared = await prepareAgentCommandExecution( { message: "prepare only", sessionKey, @@ -1210,7 +1211,7 @@ describe("agentCommand", () => { }); cfg.messages = { visibleReplies: "automatic" }; - const prepared = await agentCommandTesting.prepareAgentCommandExecution( + const prepared = await prepareAgentCommandExecution( { message: "child completed", sessionKey, diff --git a/src/commands/agent.worktree-race.test.ts b/src/commands/agent.worktree-race.test.ts index a0249ae22059..4f81ce937e32 100644 --- a/src/commands/agent.worktree-race.test.ts +++ b/src/commands/agent.worktree-race.test.ts @@ -6,6 +6,7 @@ import { promisify } from "node:util"; import { withTempHome } from "openclaw/plugin-sdk/test-env"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import "./agent-command.test-mocks.js"; +import { prepareAgentCommandExecution } from "../agents/command/prepare.js"; import { ensureAgentWorkspace } from "../agents/workspace.js"; import { getRegistryWorktree } from "../agents/worktrees/registry.js"; import { managedWorktrees } from "../agents/worktrees/service.js"; @@ -13,7 +14,6 @@ import { upsertSqliteSessionEntry } from "../config/sessions/session-accessor.sq import { clearSessionStoreCacheForTest } from "../config/sessions/store-writer-state.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import { closeOpenClawStateDatabaseForTest } from "../state/openclaw-state-db.js"; -import { testing as agentCommandTesting } from "./agent.js"; import { createThrowingTestRuntime } from "./test-runtime-config-helpers.js"; const configIoMocks = vi.hoisted(() => ({ @@ -132,7 +132,7 @@ describe("agent command worktree admission", () => { return { dir: params?.dir ?? "" }; }); - const preparing = agentCommandTesting.prepareAgentCommandExecution( + const preparing = prepareAgentCommandExecution( { message: "resume in worktree", sessionKey }, runtime, ); @@ -178,10 +178,7 @@ describe("agent command worktree admission", () => { let preparationResult: string; try { - await agentCommandTesting.prepareAgentCommandExecution( - { message: "resume in worktree", sessionKey }, - runtime, - ); + await prepareAgentCommandExecution({ message: "resume in worktree", sessionKey }, runtime); preparationResult = "preparation proceeded without its checkout"; } catch (error) { preparationResult = `preparation fails: ${(error as Error).message}`;