From 37125092d8f6fccadea5acecd9ba22783f5f511c Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 13 Aug 2026 15:12:13 -0700 Subject: [PATCH] test(sessions): trim obsolete catalog scaffolding (#123212) --- .../sessions.acp-model-display.test.ts | 251 +++--------------- .../sessions.acp-runtime-metadata.test.ts | 172 +----------- .../sessions.kind-classification.test.ts | 177 +----------- 3 files changed, 51 insertions(+), 549 deletions(-) diff --git a/src/commands/sessions.acp-model-display.test.ts b/src/commands/sessions.acp-model-display.test.ts index f7ebea3e224b..6f2cdea60b7e 100644 --- a/src/commands/sessions.acp-model-display.test.ts +++ b/src/commands/sessions.acp-model-display.test.ts @@ -1,4 +1,4 @@ -// Sessions ACP model display tests cover model metadata rendering for ACP-backed sessions. +// Sessions ACP model display tests cover persisted control-plane metadata projection. import fs from "node:fs"; import os from "node:os"; import path from "node:path"; @@ -14,66 +14,21 @@ import { writeStore, } from "./sessions.test-helpers.js"; -/** - * Catalog #20 — `model` / `modelProvider` reported as agent-config, not ACP runtime actuals. - * - * Bug summary: For ACP-keyed sessions (e.g. `agent:copilot:acp:`), the - * `--json` listing reports the AGENT's configured model - * (e.g. `model: "gpt-5.3-codex"`, `modelProvider: "microsoft-foundry"`) — but - * those are the values the openclaw-agent-driven flow would have used. When - * the same agent runs as an ACP child via `copilot --acp --stdio`, the actual - * underlying model selection lives inside copilot CLI and is independent of - * the agent's configured model. The listing happily reports the agent default - * regardless of whether the session actually ran via ACP. - * - * `resolveSessionDisplayModelRef` (`src/commands/sessions-display-model.ts:123-148`) - * has zero ACP-awareness: it only consults the session entry's persisted - * `model` / `modelProvider` / `modelOverride` and the agent's configured - * default. It never inspects the session key or the persisted ACP metadata. - * - * Decided fix shape (catalog #20, mirrors #18): SENTINEL OVERLAY at the call - * site, gated on BOTH key shape AND persisted SQLite ACP metadata. Key shape - * alone is not sufficient because ACP bridge sessions (translator.ts) also use - * ACP-shaped keys without ever writing `SessionAcpMeta` — those sessions run - * the normal configured model and must not receive the sentinel. - * - * When `isAcpSessionKey(row.key)` is true AND SQLite ACP metadata exists, the - * JSON-emit path overlays `{ provider: "acpx", model: "-acp" }` on - * top of the resolver result. The resolver itself stays pure. - * - * NOTE ON DRIVING SURFACE: `resolveSessionDisplayModelRef` is exported, but - * the bug as observed by operators surfaces through `sessions --json`, so we - * drive the test end-to-end through `sessionsCommand --json` (mirroring the - * #19 test pattern). This proves the bug at the actual emit site that - * operators see, not just in the resolver in isolation. - */ - mockSessionsConfig(); const { sessionsCommand } = await import("./sessions.js"); -type SessionsJsonPayload = { - sessions?: Array<{ - key: string; - model?: string | null; - modelProvider?: string | null; - }>; -}; - const ACP_SESSION_KEY = "agent:copilot:acp:86b7b5af-3773-4a56-b244-069d6c5d3db9"; -const NON_ACP_SESSION_KEY = "agent:copilot:main"; - const AGENT_CONFIGURED_MODEL = "gpt-5.3-codex"; const AGENT_CONFIGURED_PROVIDER = "microsoft-foundry"; let originalStateDir: string | undefined; let tempStateDirs: string[] = []; -function useTempStateDir(): string { +function useTempStateDir(): void { const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-acp-sessions-state-")); tempStateDirs.push(stateDir); process.env.OPENCLAW_STATE_DIR = stateDir; - return stateDir; } function writeAcpRuntimeMeta(sessionKey: string): void { @@ -91,15 +46,6 @@ function writeAcpRuntimeMeta(sessionKey: string): void { }); } -/** - * Mock config with a `copilot` agent whose configured model is - * `microsoft-foundry/gpt-5.3-codex` (the deployed scenario from the catalog). - * - * Both the ACP and the non-ACP session entries below leave `model` / - * `modelProvider` unset, so `resolveSessionDisplayModelRef` falls through to - * the agent's configured default. That is precisely the path under test: - * for ACP sessions the agent default is the WRONG answer. - */ function mockAgentConfigWithCopilotModel(): void { setMockSessionsConfig(() => ({ agents: { @@ -109,43 +55,30 @@ function mockAgentConfigWithCopilotModel(): void { model: { primary: `${AGENT_CONFIGURED_PROVIDER}/${AGENT_CONFIGURED_MODEL}` }, }, ], - defaults: { - contextTokens: 200_000, - }, + defaults: { contextTokens: 200_000 }, }, })); } -/** - * ACP bridge session entry: ACP-shaped key but no ACP metadata. The ACP bridge - * (translator.ts) uses an in-memory-only session store and never writes - * `SessionAcpMeta` to disk. If a bridge client passes an explicit ACP-shaped - * key (e.g. `agent:copilot:acp:session-1`) and the Gateway persists the - * session, it will have an ACP key without ACP metadata. The overlay must NOT - * fire for these sessions — they ran the configured model. - */ function buildAcpBridgeSessionEntry(): SessionEntry { return { sessionId: "acp-bridge-session-id", updatedAt: Date.now() - 4 * 60_000, - // No `acp` field: this is a bridge session, not a control-plane child session. }; } -/** - * Minimal non-ACP session entry, same shape as the ACP bridge entry. Used as the - * GREEN-control case below. The agent default is the correct answer for - * non-ACP sessions — those run through the openclaw-agent-driven flow that - * actually uses the configured model. - */ -function buildNonAcpSessionEntry(): SessionEntry { - return { - sessionId: "non-acp-session-id", - updatedAt: Date.now() - 3 * 60_000, - }; +async function readSessionRow(sessionKey: string, store: string) { + const payload = await runSessionsJson<{ + sessions?: Array<{ + key: string; + model?: string | null; + modelProvider?: string | null; + }>; + }>(sessionsCommand, store); + return payload.sessions?.find((entry) => entry.key === sessionKey); } -describe("sessionsCommand model/modelProvider display for ACP sessions (catalog #20)", () => { +describe("sessionsCommand ACP model display", () => { beforeEach(() => { originalStateDir = process.env.OPENCLAW_STATE_DIR; mockAgentConfigWithCopilotModel(); @@ -165,168 +98,48 @@ describe("sessionsCommand model/modelProvider display for ACP sessions (catalog resetMockSessionsConfig(); }); - it("RED: ACP control-plane session must NOT report the agent-configured model", async () => { - // RED before fix. The session is a real ACP control-plane session - // (key has the `:acp:` segment AND SQLite ACP metadata is present), but - // `resolveSessionDisplayModelRef` ignores both and returns the agent - // default. Operators relying on `sessions --json` model fields see the - // model the openclaw-agent-driven flow would have used, NOT what copilot - // actually selected internally when it ran via ACP. - // - // The discriminator the fix uses: `isAcpSessionKey(row.key)` AND - // SQLite ACP metadata (persisted by the ACP control plane manager). + it("reports the ACP runtime sentinel for control-plane sessions", async () => { useTempStateDir(); writeAcpRuntimeMeta(ACP_SESSION_KEY); const store = await writeStore( { [ACP_SESSION_KEY]: buildAcpBridgeSessionEntry() }, - "sessions-acp-model-display-red", + "sessions-acp-model-display", { agentId: "copilot" }, ); - const payload = await runSessionsJson(sessionsCommand, store); - const row = payload.sessions?.find((entry) => entry.key === ACP_SESSION_KEY); + const row = await readSessionRow(ACP_SESSION_KEY, store); - expect( - row, - `Expected sessionsCommand --json to include a row for ${ACP_SESSION_KEY}; got none.`, - ).toBeDefined(); - expect( - row?.model, - `ACP session ${ACP_SESSION_KEY} reports model="${row?.model}" — that is the agent-configured ` + - `model (${AGENT_CONFIGURED_MODEL}), not what copilot actually used inside ACP. ` + - `resolveSessionDisplayModelRef (src/commands/sessions-display-model.ts:123) has zero ` + - `ACP-awareness; the call site at src/commands/sessions.ts should consult ` + - `isAcpSessionKey(row.key) AND SQLite ACP metadata exists, then overlay an ACP-runtime sentinel.`, - ).not.toBe(AGENT_CONFIGURED_MODEL); - expect( - row?.modelProvider, - `ACP session ${ACP_SESSION_KEY} reports modelProvider="${row?.modelProvider}" — the ` + - `agent-configured provider (${AGENT_CONFIGURED_PROVIDER}), not the ACP runtime. ` + - `Same fix site as above; the overlay must gate on persisted ACP metadata.`, - ).not.toBe(AGENT_CONFIGURED_PROVIDER); + expect(row).toMatchObject({ model: "copilot-acp", modelProvider: "acpx" }); }); - it("RED (fix-shape): ACP control-plane session should report the ACP runtime sentinel", async () => { - // RED before fix; GREEN once the catalog-#20 sentinel-overlay fix lands. - // - // The catalog's chosen fix shape: when `isAcpSessionKey(row.key)` is true - // AND persisted ACP metadata exists, overlay `{ provider: "acpx", model: "-acp" }`. - // This trades model-name accuracy for "this is ACP control-plane, not the - // agent default" clarity. Plumbing the actual copilot-side model selection - // into the openclaw record would require capturing ACP `session.model_change` - // events (catalog notes this as deferrable). + it("reads canonical ACP store keys before querying runtime metadata", async () => { useTempStateDir(); - writeAcpRuntimeMeta(ACP_SESSION_KEY); + const sessionKey = "agent:copilot:acp:binding:discord:default:feedface"; const store = await writeStore( - { [ACP_SESSION_KEY]: buildAcpBridgeSessionEntry() }, - "sessions-acp-model-display-fix-shape", - { agentId: "copilot" }, - ); - - const payload = await runSessionsJson(sessionsCommand, store); - const row = payload.sessions?.find((entry) => entry.key === ACP_SESSION_KEY); - - expect(row).toBeDefined(); - expect( - row?.model, - `ACP session ${ACP_SESSION_KEY} should resolve model to "copilot-acp" (the catalog-chosen ` + - `sentinel). Got "${row?.model}". Fix gates on isAcpSessionKey(row.key) and persisted ACP metadata ` + - `and overlays { provider: "acpx", model: "copilot-acp" }. Keeps resolveSessionDisplayModelRef pure.`, - ).toBe("copilot-acp"); - expect( - row?.modelProvider, - `ACP session ${ACP_SESSION_KEY} should resolve modelProvider to "acpx". Got ` + - `"${row?.modelProvider}". Same fix as the model assertion above; the overlay sets both ` + - `fields together so they remain internally consistent.`, - ).toBe("acpx"); - }); - - it("reads ACP runtime metadata from SQLite for the display overlay", async () => { - useTempStateDir(); - const store = await writeStore( - { [ACP_SESSION_KEY]: buildAcpBridgeSessionEntry() }, - "sessions-acp-model-display-sqlite", - { agentId: "copilot" }, - ); - writeAcpRuntimeMeta(ACP_SESSION_KEY); - - const payload = await runSessionsJson(sessionsCommand, store); - const row = payload.sessions?.find((entry) => entry.key === ACP_SESSION_KEY); - - expect(row).toBeDefined(); - expect(row?.model).toBe("copilot-acp"); - expect(row?.modelProvider).toBe("acpx"); - }); - - it("reads canonical ACP store keys before reading SQLite metadata", async () => { - useTempStateDir(); - const canonicalAcpKey = "agent:copilot:acp:binding:discord:default:feedface"; - const store = await writeStore( - { [canonicalAcpKey]: buildAcpBridgeSessionEntry() }, + { [sessionKey]: buildAcpBridgeSessionEntry() }, "sessions-acp-model-display-canonical", { agentId: "copilot" }, ); - writeAcpRuntimeMeta(canonicalAcpKey); + writeAcpRuntimeMeta(sessionKey); - const payload = await runSessionsJson(sessionsCommand, store); - const row = payload.sessions?.find((entry) => entry.key === canonicalAcpKey); + const row = await readSessionRow(sessionKey, store); - expect(row).toBeDefined(); - expect(row?.model).toBe("copilot-acp"); - expect(row?.modelProvider).toBe("acpx"); + expect(row).toMatchObject({ model: "copilot-acp", modelProvider: "acpx" }); }); - it("GREEN control: ACP bridge session (ACP key, no ACP metadata) reports the configured model", async () => { - // ACP bridge sessions (translator.ts) use ACP-shaped keys but never - // persist SessionAcpMeta. They run the normal configured model - // and must NOT receive the acpx sentinel. This guards against a regression - // where key-shape-only detection would misreport bridge sessions. - const ACP_BRIDGE_SESSION_KEY = "agent:copilot:acp:bridge-session-1"; + it("keeps the configured model for ACP-shaped bridge sessions without runtime metadata", async () => { + const sessionKey = "agent:copilot:acp:bridge-session-1"; const store = await writeStore( - { [ACP_BRIDGE_SESSION_KEY]: buildAcpBridgeSessionEntry() }, - "sessions-acp-model-display-bridge-control", + { [sessionKey]: buildAcpBridgeSessionEntry() }, + "sessions-acp-model-display-bridge", { agentId: "copilot" }, ); - const payload = await runSessionsJson(sessionsCommand, store); - const row = payload.sessions?.find((entry) => entry.key === ACP_BRIDGE_SESSION_KEY); + const row = await readSessionRow(sessionKey, store); - expect(row).toBeDefined(); - expect( - row?.model, - `ACP bridge session ${ACP_BRIDGE_SESSION_KEY} has an ACP-shaped key but no ACP metadata — ` + - `it ran the configured model. Got model="${row?.model}"; expected "${AGENT_CONFIGURED_MODEL}". ` + - `The overlay must gate on persisted ACP metadata, not key shape alone.`, - ).toBe(AGENT_CONFIGURED_MODEL); - expect( - row?.modelProvider, - `ACP bridge session ${ACP_BRIDGE_SESSION_KEY} should report the configured provider. ` + - `Got "${row?.modelProvider}"; expected "${AGENT_CONFIGURED_PROVIDER}".`, - ).toBe(AGENT_CONFIGURED_PROVIDER); - }); - - it("GREEN control: non-ACP session correctly reports the agent-configured model", async () => { - // GREEN today. The same agent configuration drives a non-ACP session - // (`agent:copilot:main`) — and for that session the agent-configured - // model IS the right answer because the openclaw-agent-driven flow - // actually runs that model. This control proves: - // 1. The test infrastructure is exercising the real resolver path - // (not a mock that would silently pass either way). - // 2. The configured-model branch of resolveSessionDisplayModelRef - // remains correct for non-ACP keys; the proposed sentinel overlay - // must NOT break this case (it should only fire when both - // isAcpSessionKey(row.key) is true AND ACP metadata is present). - const store = await writeStore( - { [NON_ACP_SESSION_KEY]: buildNonAcpSessionEntry() }, - "sessions-acp-model-display-green-control", - { agentId: "copilot" }, - ); - - const payload = await runSessionsJson(sessionsCommand, store); - const row = payload.sessions?.find((entry) => entry.key === NON_ACP_SESSION_KEY); - - expect(row).toBeDefined(); - expect(row?.model).toBe(AGENT_CONFIGURED_MODEL); - expect(row?.modelProvider).toBe(AGENT_CONFIGURED_PROVIDER); + expect(row).toMatchObject({ + model: AGENT_CONFIGURED_MODEL, + modelProvider: AGENT_CONFIGURED_PROVIDER, + }); }); }); diff --git a/src/commands/sessions.acp-runtime-metadata.test.ts b/src/commands/sessions.acp-runtime-metadata.test.ts index f507ac7dfddb..c1cda3b34121 100644 --- a/src/commands/sessions.acp-runtime-metadata.test.ts +++ b/src/commands/sessions.acp-runtime-metadata.test.ts @@ -1,93 +1,26 @@ -// Sessions ACP runtime metadata tests cover agent runtime metadata derived from model and session keys. +// Sessions ACP runtime metadata tests cover session-owned runtime overlays. import { describe, expect, it } from "vitest"; import { resolveModelAgentRuntimeMetadata } from "../agents/agent-runtime-metadata.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import { parseAgentSessionKey } from "../routing/session-key.js"; -/** - * Catalog #18 — `openclaw sessions --json` reports `agentRuntime.id: "openclaw"` for - * ACP sessions because the old metadata resolver only consulted agent-config - * policies (env / agent / defaults / implicit fallback to "openclaw"). The session - * key clearly carries the ACP runtime indicator (the `:acp:` segment), but - * `sessions.ts:294` used to ignore it. - * - * Empirical observation from a deployed openclaw container against a copilot - * agent that has no explicit `agentRuntime.id` policy: - * - * { - * "key": "agent:copilot:acp:86b7b5af-3773-4a56-b244-069d6c5d3db9", - * "agentId": "copilot", - * "agentRuntime": { "id": "openclaw", "source": "implicit" }, - * "kind": "direct" - * } - * - * That is wrong: this session is plainly ACP, not the native runtime. The runtime field is - * supposed to be a faithful classifier of how this session is actually being - * run; instead, every ACP session in the JSON output is mislabelled as the native runtime. - * - * This test mirrors the exact computation `sessionsCommand` performs at - * `src/commands/sessions.ts:294` and proves the bug in two parts: - * - * - RED: ACP-keyed session resolves to `id: "openclaw"`, `source: "implicit"`. - * - GREEN control: a non-ACP `agent:main:main` session resolves to the - * same implicit-native metadata, which IS correct in that case. The control - * proves the assertion infrastructure is not masking the RED case. - * - * Fix shape (see the third test): when the session key is ACP-style, - * agentRuntime.id should report `acpx` (or whatever runtime id is actually - * driving the session) so that the JSON faithfully classifies the session. - * The fix likely belongs at the caller (sessions.ts:294 and the other - * call sites in `src/gateway/server-methods/sessions-*.ts`, - * `src/gateway/session-utils.ts`) so it can pass session-key context to - * `resolveModelAgentRuntimeMetadata`. - */ - const ACP_SESSION_KEY = "agent:copilot:acp:86b7b5af-3773-4a56-b244-069d6c5d3db9"; const NON_ACP_SESSION_KEY = "agent:main:main"; -/** - * Build a minimal `OpenClawConfig` that mirrors the deployed scenario: - * - a copilot agent exists in the agents.list - * - it has NO explicit `agentRuntime.id` policy - * - no top-level `agents.defaults.agentRuntime` either - * - * Result: the old metadata resolver fell through to the implicit "openclaw" - * branch — which is the bug under test. - */ function buildConfigWithoutAgentRuntimePolicy(): OpenClawConfig { return { agents: { - list: [ - { - id: "copilot", - // Intentionally no `agentRuntime` field, no `runtime` descriptor. - }, - { - id: "main", - default: true, - }, - ], - // No `defaults.agentRuntime` either. + list: [{ id: "copilot" }, { id: "main", default: true }], defaults: {}, }, } as OpenClawConfig; } -/** - * Mirror the per-row computation from `src/commands/sessions.ts:290-298`: - * const agentId = parseAgentSessionKey(row.key)?.agentId ?? target.agentId; - * const agentRuntime = resolveModelAgentRuntimeMetadata({ cfg, agentId, sessionKey: row.key }); - * - * Returns the same shape that ends up serialized to `--json` output. - * After commit 02fe0d8978, the production path goes through resolveModelAgentRuntimeMetadata. - */ function computeSessionAgentRuntime(params: { cfg: OpenClawConfig; sessionKey: string; fallbackAgentId: string; - /** Mirrors `entry?.acp != null` passed from loaded session rows. */ acpRuntime?: boolean; - /** Mirrors `entry?.acp?.backend` passed from the session store entry. */ acpBackend?: string; }): ReturnType { const agentId = parseAgentSessionKey(params.sessionKey)?.agentId ?? params.fallbackAgentId; @@ -100,116 +33,33 @@ function computeSessionAgentRuntime(params: { }); } -describe("sessions --json agentRuntime classifier (catalog #18)", () => { - it("RED→GREEN: ACP session key is no longer misclassified (overlay applies)", () => { - const cfg = buildConfigWithoutAgentRuntimePolicy(); +describe("session ACP runtime metadata", () => { + it("prefers an explicit ACP backend", () => { const agentRuntime = computeSessionAgentRuntime({ - cfg, - sessionKey: ACP_SESSION_KEY, - fallbackAgentId: "copilot", - acpRuntime: true, - }); - - // The bug was: the session key plainly contains `:acp:` and yet the - // resolved metadata said id="openclaw", source="implicit". - // After the fix (applyAcpRuntimeOverlay in resolveModelAgentRuntimeMetadata), - // the ACP session key overrides the runtime to id="acpx", source="session-key". - expect( - agentRuntime.id, - `ACP session ${ACP_SESSION_KEY} should no longer be misclassified as "auto" or "openclaw". ` + - `Got "${agentRuntime.id}". resolveModelAgentRuntimeMetadata must pass sessionKey to ` + - `applyAcpRuntimeOverlay so ACP sessions are classified as "acpx".`, - ).not.toBe("auto"); - expect( - agentRuntime.source, - `ACP session ${ACP_SESSION_KEY} resolved with source="${agentRuntime.source}". ` + - `For an ACP-keyed session, the source should not be "implicit" — ` + - `the session key itself is an explicit signal that the runtime is ACP.`, - ).not.toBe("implicit"); - }); - - it("GREEN control: non-ACP session is NOT overridden by ACP overlay", () => { - const cfg = buildConfigWithoutAgentRuntimePolicy(); - const agentRuntime = computeSessionAgentRuntime({ - cfg, - sessionKey: NON_ACP_SESSION_KEY, - fallbackAgentId: "main", - }); - - // For a non-ACP session, the overlay must NOT fire — the result must - // not be "acpx" and source must not be "session-key". The control - // proves the overlay is gated on the `:acp:` segment in the session key. - // (The concrete id — "codex" for the default openai/gpt-5.5 provider — - // is determined by resolveAgentHarnessPolicy's Codex-routing rule; - // what matters here is the absence of the ACP override.) - expect(agentRuntime.id).not.toBe("acpx"); - expect(agentRuntime.source).not.toBe("session-key"); - }); - - it("FIX-SHAPE expectation: ACP session should resolve to 'acpx'", () => { - // What "fixed" should look like once the bug is addressed. - // RED today; GREEN once the fix lands. - // - // Note: the exact id ("acpx" vs another label) is a design choice for - // the fix author. What matters is that it is meaningfully different - // from "openclaw" and reflects the actual runtime driving the session. - // If the fix picks a different label, update this assertion to match — - // the structural point (session-key-aware classification) is the - // load-bearing part. - const cfg = buildConfigWithoutAgentRuntimePolicy(); - const agentRuntime = computeSessionAgentRuntime({ - cfg, - sessionKey: ACP_SESSION_KEY, - fallbackAgentId: "copilot", - acpRuntime: true, - }); - - expect( - agentRuntime.id, - `ACP session ${ACP_SESSION_KEY} should resolve to runtime id "acpx" (or the canonical ACP runtime label). ` + - `Got "${agentRuntime.id}". Fix candidates: ` + - `(a) override at the call site in src/commands/sessions.ts:294 once isAcpSessionKey(row.key) is true, or ` + - `make resolveModelAgentRuntimeMetadata apply the session-key-aware override centrally.`, - ).toBe("acpx"); - }); - - it("backend override: ACP session with entry.acp.backend set reports that backend id, NOT 'acpx'", () => { - // When the session entry carries an explicit acp.backend (e.g. a registered - // non-default backend), the overlay must reflect the actual backend instead - // of the generic "acpx" fallback. - const cfg = buildConfigWithoutAgentRuntimePolicy(); - const agentRuntime = computeSessionAgentRuntime({ - cfg, + cfg: buildConfigWithoutAgentRuntimePolicy(), sessionKey: ACP_SESSION_KEY, fallbackAgentId: "copilot", acpRuntime: true, acpBackend: "custom-backend", }); - expect(agentRuntime.id).toBe("custom-backend"); - expect(agentRuntime.source).toBe("session-key"); + expect(agentRuntime).toEqual({ id: "custom-backend", source: "session-key" }); }); - it("backend fallback: ACP session with entry.acp but no backend falls back to 'acpx'", () => { - // When the session entry has ACP metadata but no acp.backend, the overlay - // must fall back to the canonical "acpx" id. - const cfg = buildConfigWithoutAgentRuntimePolicy(); + it("falls back to acpx when ACP metadata has no backend", () => { const agentRuntime = computeSessionAgentRuntime({ - cfg, + cfg: buildConfigWithoutAgentRuntimePolicy(), sessionKey: ACP_SESSION_KEY, fallbackAgentId: "copilot", acpRuntime: true, - // acpBackend intentionally omitted — mirrors entry with no acp.backend }); - expect(agentRuntime.id).toBe("acpx"); - expect(agentRuntime.source).toBe("session-key"); + expect(agentRuntime).toEqual({ id: "acpx", source: "session-key" }); }); - it("GREEN control: ACP-shaped bridge session without entry.acp is NOT overridden", () => { - const cfg = buildConfigWithoutAgentRuntimePolicy(); + it("does not overlay ACP-shaped bridge sessions without ACP metadata", () => { const agentRuntime = computeSessionAgentRuntime({ - cfg, + cfg: buildConfigWithoutAgentRuntimePolicy(), sessionKey: ACP_SESSION_KEY, fallbackAgentId: "copilot", acpRuntime: false, diff --git a/src/commands/sessions.kind-classification.test.ts b/src/commands/sessions.kind-classification.test.ts index c553d624d408..f4bccd3f10b9 100644 --- a/src/commands/sessions.kind-classification.test.ts +++ b/src/commands/sessions.kind-classification.test.ts @@ -1,4 +1,4 @@ -// Session kind classification tests cover chat, ACP, and agent session metadata classification. +// Session kind classification tests cover ACP child session metadata. import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { SessionEntry } from "../config/sessions/types.js"; import { normalizeSessionDeliveryState } from "../utils/delivery-context.shared.js"; @@ -9,61 +9,13 @@ import { writeStore, } from "./sessions.test-helpers.js"; -/** - * Catalog #19 — `kind` misclassified as `"direct"` for ACP spawn-child sessions. - * - * Bug summary: `classifySessionKey` (defined twice — `src/commands/sessions.ts:136-152` - * and `src/status/summary.runtime.ts:129-145`) classifies a session - * based ONLY on the key shape (`:group:` / `:channel:` substrings) plus - * `entry.chatType`. It ignores `entry.spawnedBy` and `entry.deliveryContext`, - * so ACP spawn-child sessions (e.g., `agent:copilot:acp:` with - * `spawnedBy: "agent:main:telegram:group:..."` and - * `deliveryContext: { channel: "telegram", to: , threadId: }`) - * are misclassified as `kind: "direct"` even though they were spawned from a - * group/topic-bound parent. - * - * Available kinds today: - * "global" | "unknown" | "cron" | "group" | "direct" - * - * The fix shape proposed in the catalog is to add a new `"spawn-child"` kind - * (or, alternatively, fall through to the parent's classification — but the - * catalog calls out `"spawn-child"` as the cleanest minimal fix). - * - * NOTE ON DUPLICATION: the same logic lives in two places — - * - `src/commands/sessions.ts:136-152` (called by `sessionsCommand`, - * the path under test here) - * - `src/status/summary.runtime.ts:129-145` - * The eventual fix MUST update both, or extract a shared helper. - * - * NOTE ON SURFACE: `classifySessionKey` is private to each file (not exported), - * so this test drives the classification through the exposed seam: - * `sessionsCommand --json` and inspects the `kind` field of each session row - * (mirroring `src/commands/sessions.test.ts` and `sessions.acp-runtime-metadata.test.ts`). - */ - mockSessionsConfig(); const { sessionsCommand } = await import("./sessions.js"); -type SessionRowKind = "global" | "unknown" | "cron" | "group" | "direct" | "spawn-child"; - -type SessionsJsonPayload = { - sessions?: Array<{ - key: string; - kind: SessionRowKind; - }>; -}; - const ACP_SPAWN_CHILD_KEY = "agent:main:acp:7de23a0a-799d-4d63-b1b1-a7de9d4cd840"; -const ACP_DM_KEY = "agent:main:acp:86b7b5af-3773-4a56-b244-069d6c5d3db9"; const TELEGRAM_GROUP_KEY = "agent:main:telegram:group:-1003967207344:topic:1"; -/** - * SessionEntry shape mirroring the deployed-container record described in - * the catalog (a copilot ACP session spawned by a telegram supergroup parent). - * Only the fields the classifier and the JSON emit path care about are set; - * everything else stays unset / default. - */ function buildAcpSpawnChildEntry(): SessionEntry { return { sessionId: "spawn-child-session-id", @@ -76,44 +28,10 @@ function buildAcpSpawnChildEntry(): SessionEntry { threadId: 323, }, }), - // No chatType — ACP spawn-child entries don't carry one. The classifier - // must infer "this came from a group" from spawnedBy / deliveryContext. }; } -/** - * Plain DM-driven ACP session: same key shape (`agent:copilot:acp:`) - * but no `spawnedBy` and a direct delivery context. Today's classifier - * correctly reports `"direct"` for this; that behavior should be preserved - * after the fix. - */ -function buildAcpDirectEntry(): SessionEntry { - return { - sessionId: "dm-session-id", - updatedAt: Date.now() - 5 * 60_000, - delivery: normalizeSessionDeliveryState({ - context: { - channel: "telegram", - to: "+15555550123", - }, - }), - }; -} - -/** - * Group session with a key that explicitly embeds `:group:` — the - * classifier's existing key-shape branch picks this up correctly today - * and reports `"group"`. - */ -function buildTelegramGroupEntry(): SessionEntry { - return { - sessionId: "group-session-id", - updatedAt: Date.now() - 10 * 60_000, - chatType: "group", - }; -} - -describe("sessionsCommand kind classification (catalog #19)", () => { +describe("sessionsCommand kind classification", () => { beforeEach(() => { vi.useFakeTimers(); vi.setSystemTime(new Date("2025-12-06T00:00:00Z")); @@ -124,97 +42,18 @@ describe("sessionsCommand kind classification (catalog #19)", () => { vi.useRealTimers(); }); - it("RED: ACP spawn-child session must NOT be classified as 'direct'", async () => { - // RED today. The classifier ignores `spawnedBy` and `deliveryContext`, - // so an ACP key with no `:group:` substring and no `chatType` falls - // through to `"direct"`. Operators see this session in - // `openclaw sessions --json` as `kind: "direct"` even though it was - // plainly spawned from a group/topic. See `src/commands/sessions.ts:136-152`. + it("classifies ACP child sessions separately from direct sessions", async () => { const store = await writeStore( { [ACP_SPAWN_CHILD_KEY]: buildAcpSpawnChildEntry() }, - "sessions-kind-spawn-child-red", + "sessions-kind-spawn-child", ); - const payload = await runSessionsJson(sessionsCommand, store); - const row = payload.sessions?.find((entry) => entry.key === ACP_SPAWN_CHILD_KEY); - - expect( - row, - `Expected sessionsCommand --json to include a row for ${ACP_SPAWN_CHILD_KEY}; got none.`, - ).toBeDefined(); - expect( - row?.kind, - `ACP spawn-child session ${ACP_SPAWN_CHILD_KEY} is misclassified: kind="${row?.kind}". ` + - `It carries spawnedBy="${TELEGRAM_GROUP_KEY}" and deliveryContext.channel="telegram", ` + - `which clearly mark it as a non-direct origin. The classifier at ` + - `src/commands/sessions.ts:136-152 ignores these fields and returns "direct".`, - ).not.toBe("direct"); - }); - - it("RED (fix-shape): ACP spawn-child session should resolve to 'spawn-child'", async () => { - // RED today; flips GREEN once the proposed fix lands. - // - // The catalog's recommended fix introduces a new `"spawn-child"` kind - // checked BEFORE the key-shape branch so spawn-child ACP sessions take - // precedence over the fallback `"direct"` classification. - // - // If the fix author chooses a different label (e.g., `"acp-child"`) or - // a different shape (e.g., fall through to the parent's classification - // and report `"group"`), update this assertion to match. The structural - // point is that `entry.spawnedBy` / `entry.deliveryContext` MUST drive - // the classification for ACP children. - const store = await writeStore( - { [ACP_SPAWN_CHILD_KEY]: buildAcpSpawnChildEntry() }, - "sessions-kind-spawn-child-fix-shape", - ); - - const payload = await runSessionsJson(sessionsCommand, store); + const payload = await runSessionsJson<{ + sessions?: Array<{ key: string; kind: string }>; + }>(sessionsCommand, store); const row = payload.sessions?.find((entry) => entry.key === ACP_SPAWN_CHILD_KEY); expect(row).toBeDefined(); - expect( - row?.kind, - `ACP spawn-child session ${ACP_SPAWN_CHILD_KEY} should classify as "spawn-child" ` + - `(or whichever non-direct label the fix author chooses). Got "${row?.kind}". ` + - `Fix locations: src/commands/sessions.ts:136-152 AND ` + - `src/status/summary.runtime.ts:129-145 (the same logic is duplicated; ` + - `extract to a shared helper or update both).`, - ).toBe("spawn-child"); - }); - - it("GREEN control: non-spawn-child ACP DM session resolves to 'direct'", async () => { - // GREEN today. An ACP-keyed session WITHOUT `spawnedBy` and with a - // direct delivery context (or none) correctly resolves to `"direct"`. - // This control proves the test infrastructure exercises the real - // classification path; if it accidentally regressed to a different - // value, that would indicate the test harness was broken. - const store = await writeStore( - { [ACP_DM_KEY]: buildAcpDirectEntry() }, - "sessions-kind-acp-direct-control", - ); - - const payload = await runSessionsJson(sessionsCommand, store); - const row = payload.sessions?.find((entry) => entry.key === ACP_DM_KEY); - - expect(row).toBeDefined(); - expect(row?.kind).toBe("direct"); - }); - - it("GREEN control: telegram group key with chatType='group' resolves to 'group'", async () => { - // GREEN today. The classifier's key-shape branch (`:group:` substring) - // and the `chatType === "group"` branch both fire for this entry, - // yielding `"group"`. This control proves the existing happy-path - // classification still works and is not silently broken by the test - // harness. - const store = await writeStore( - { [TELEGRAM_GROUP_KEY]: buildTelegramGroupEntry() }, - "sessions-kind-group-control", - ); - - const payload = await runSessionsJson(sessionsCommand, store); - const row = payload.sessions?.find((entry) => entry.key === TELEGRAM_GROUP_KEY); - - expect(row).toBeDefined(); - expect(row?.kind).toBe("group"); + expect(row?.kind).toBe("spawn-child"); }); });