Skip to content

Commit 207fb99

Browse files
efpivaefpiva
authored
fix(sessions): display ACP runtime sentinel for ACP sessions (#79543)
Display the ACP runtime sentinel for ACP control-plane session rows in openclaw sessions output, while preserving configured model/provider display for direct sessions. Verified with focused sessions tests, touched-file oxlint, check:test-types, Crabbox after-fix proof, and exact-head GitHub CI.
1 parent b8ea609 commit 207fb99

3 files changed

Lines changed: 334 additions & 5 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ Docs: https://docs.openclaw.ai
105105
- Plugins/runtime: attribute deprecated runtime config load/write warnings to the plugin id and source that triggered them so logs and plugin doctor runs are actionable. Refs #81394. (#81425) Thanks @BKF-Gitty.
106106
- Memory/LanceDB: make auto-capture recognize short CJK memory phrases and configurable literal triggers, so Chinese, Japanese, and Korean users can capture memories without regex or LLM intent detection. Fixes #75680. Thanks @vyctorbrzezowski and @guokewuming.
107107
- Plugins doctor: report stale plugin config warnings and avoid claiming full plugin health when config warnings remain. (#81515) Thanks @BKF-Gitty.
108+
- Sessions: display `model: "<agentId>-acp"` / `modelProvider: "acpx"` (ACP-runtime sentinel) for ACP control-plane sessions in `openclaw sessions` output, instead of the agent's configured model which was misleading. Catalog finding 20. (#79543)
108109

109110
### Changes
110111

‎src/commands/sessions.acp-model-display.test.ts‎

Lines changed: 274 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,274 @@
1+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
2+
import type { SessionEntry } from "../config/sessions/types.js";
3+
import {
4+
mockSessionsConfig,
5+
resetMockSessionsConfig,
6+
runSessionsJson,
7+
setMockSessionsConfig,
8+
writeStore,
9+
} from "./sessions.test-helpers.js";
10+
11+
/**
12+
* Catalog #20 — `model` / `modelProvider` reported as agent-config, not ACP runtime actuals.
13+
*
14+
* Bug summary: For ACP-keyed sessions (e.g. `agent:copilot:acp:<uuid>`), the
15+
* `--json` listing reports the AGENT's configured model
16+
* (e.g. `model: "gpt-5.3-codex"`, `modelProvider: "microsoft-foundry"`) — but
17+
* those are the values the openclaw-agent-driven flow would have used. When
18+
* the same agent runs as an ACP child via `copilot --acp --stdio`, the actual
19+
* underlying model selection lives inside copilot CLI and is independent of
20+
* the agent's configured model. The listing happily reports the agent default
21+
* regardless of whether the session actually ran via ACP.
22+
*
23+
* `resolveSessionDisplayModelRef` (`src/commands/sessions-display-model.ts:123-148`)
24+
* has zero ACP-awareness: it only consults the session entry's persisted
25+
* `model` / `modelProvider` / `modelOverride` and the agent's configured
26+
* default. It never inspects the session key or the persisted ACP metadata.
27+
*
28+
* Decided fix shape (catalog #20, mirrors #18): SENTINEL OVERLAY at the call
29+
* site, gated on BOTH key shape AND persisted `entry.acp` metadata. Key shape
30+
* alone is not sufficient because ACP bridge sessions (translator.ts) also use
31+
* ACP-shaped keys without ever writing `SessionAcpMeta` — those sessions run
32+
* the normal configured model and must not receive the sentinel.
33+
*
34+
* When `isAcpSessionKey(row.key)` is true AND `entry.acp != null`, the
35+
* JSON-emit path overlays `{ provider: "acpx", model: "<agentId>-acp" }` on
36+
* top of the resolver result. The resolver itself stays pure.
37+
*
38+
* NOTE ON DRIVING SURFACE: `resolveSessionDisplayModelRef` is exported, but
39+
* the bug as observed by operators surfaces through `sessions --json`, so we
40+
* drive the test end-to-end through `sessionsCommand --json` (mirroring the
41+
* #19 test pattern). This proves the bug at the actual emit site that
42+
* operators see, not just in the resolver in isolation.
43+
*/
44+
45+
mockSessionsConfig();
46+
47+
const { sessionsCommand } = await import("./sessions.js");
48+
49+
type SessionsJsonPayload = {
50+
sessions?: Array<{
51+
key: string;
52+
model?: string | null;
53+
modelProvider?: string | null;
54+
}>;
55+
};
56+
57+
const ACP_SESSION_KEY = "agent:copilot:acp:86b7b5af-3773-4a56-b244-069d6c5d3db9";
58+
const NON_ACP_SESSION_KEY = "agent:copilot:main";
59+
60+
const AGENT_CONFIGURED_MODEL = "gpt-5.3-codex";
61+
const AGENT_CONFIGURED_PROVIDER = "microsoft-foundry";
62+
63+
/**
64+
* Mock config with a `copilot` agent whose configured model is
65+
* `microsoft-foundry/gpt-5.3-codex` (the deployed scenario from the catalog).
66+
*
67+
* Both the ACP and the non-ACP session entries below leave `model` /
68+
* `modelProvider` unset, so `resolveSessionDisplayModelRef` falls through to
69+
* the agent's configured default. That is precisely the path under test:
70+
* for ACP sessions the agent default is the WRONG answer.
71+
*/
72+
function mockAgentConfigWithCopilotModel(): void {
73+
setMockSessionsConfig(() => ({
74+
agents: {
75+
list: [
76+
{
77+
id: "copilot",
78+
model: { primary: `${AGENT_CONFIGURED_PROVIDER}/${AGENT_CONFIGURED_MODEL}` },
79+
},
80+
],
81+
defaults: {
82+
contextTokens: 200_000,
83+
},
84+
},
85+
}));
86+
}
87+
88+
/**
89+
* ACP control-plane session entry: includes `entry.acp` as persisted by
90+
* `src/acp/control-plane/manager.core.ts:365` during acpx child init. The
91+
* presence of `entry.acp` is the discriminator the overlay uses to distinguish
92+
* real ACP child-runtime sessions from ACP bridge sessions.
93+
*
94+
* No `model` / `modelProvider` set on the entry — the listing falls through
95+
* to the agent's configured default, which is the buggy path for ACP keys.
96+
*/
97+
function buildAcpSessionEntry(): SessionEntry {
98+
return {
99+
sessionId: "acp-session-id",
100+
updatedAt: Date.now() - 2 * 60_000,
101+
acp: {
102+
backend: "copilot",
103+
agent: "copilot",
104+
runtimeSessionName: "acp-runtime-session-1",
105+
mode: "persistent",
106+
state: "idle",
107+
lastActivityAt: Date.now() - 2 * 60_000,
108+
},
109+
};
110+
}
111+
112+
/**
113+
* ACP bridge session entry: ACP-shaped key but no `entry.acp`. The ACP bridge
114+
* (translator.ts) uses an in-memory-only session store and never writes
115+
* `SessionAcpMeta` to disk. If a bridge client passes an explicit ACP-shaped
116+
* key (e.g. `agent:copilot:acp:session-1`) and the Gateway persists the
117+
* session, it will have an ACP key without `entry.acp`. The overlay must NOT
118+
* fire for these sessions — they ran the configured model.
119+
*/
120+
function buildAcpBridgeSessionEntry(): SessionEntry {
121+
return {
122+
sessionId: "acp-bridge-session-id",
123+
updatedAt: Date.now() - 4 * 60_000,
124+
// No `acp` field: this is a bridge session, not a control-plane child session.
125+
};
126+
}
127+
128+
/**
129+
* Minimal non-ACP session entry, same shape as the ACP bridge entry. Used as the
130+
* GREEN-control case below. The agent default is the correct answer for
131+
* non-ACP sessions — those run through the openclaw-agent-driven flow that
132+
* actually uses the configured model.
133+
*/
134+
function buildNonAcpSessionEntry(): SessionEntry {
135+
return {
136+
sessionId: "non-acp-session-id",
137+
updatedAt: Date.now() - 3 * 60_000,
138+
};
139+
}
140+
141+
describe("sessionsCommand model/modelProvider display for ACP sessions (catalog #20)", () => {
142+
beforeEach(() => {
143+
vi.useFakeTimers();
144+
vi.setSystemTime(new Date("2025-12-06T00:00:00Z"));
145+
mockAgentConfigWithCopilotModel();
146+
});
147+
148+
afterEach(() => {
149+
resetMockSessionsConfig();
150+
vi.useRealTimers();
151+
});
152+
153+
it("RED: ACP control-plane session must NOT report the agent-configured model", async () => {
154+
// RED before fix. The session is a real ACP control-plane session
155+
// (key has the `:acp:` segment AND entry.acp is present), but
156+
// `resolveSessionDisplayModelRef` ignores both and returns the agent
157+
// default. Operators relying on `sessions --json` model fields see the
158+
// model the openclaw-agent-driven flow would have used, NOT what copilot
159+
// actually selected internally when it ran via ACP.
160+
//
161+
// The discriminator the fix uses: `isAcpSessionKey(row.key)` AND
162+
// `entry.acp != null` (persisted by the ACP control plane manager).
163+
const store = writeStore(
164+
{ [ACP_SESSION_KEY]: buildAcpSessionEntry() },
165+
"sessions-acp-model-display-red",
166+
);
167+
168+
const payload = await runSessionsJson<SessionsJsonPayload>(sessionsCommand, store);
169+
const row = payload.sessions?.find((entry) => entry.key === ACP_SESSION_KEY);
170+
171+
expect(
172+
row,
173+
`Expected sessionsCommand --json to include a row for ${ACP_SESSION_KEY}; got none.`,
174+
).toBeDefined();
175+
expect(
176+
row?.model,
177+
`ACP session ${ACP_SESSION_KEY} reports model="${row?.model}" — that is the agent-configured ` +
178+
`model (${AGENT_CONFIGURED_MODEL}), not what copilot actually used inside ACP. ` +
179+
`resolveSessionDisplayModelRef (src/commands/sessions-display-model.ts:123) has zero ` +
180+
`ACP-awareness; the call site at src/commands/sessions.ts should consult ` +
181+
`isAcpSessionKey(row.key) AND entry.acp != null, then overlay an ACP-runtime sentinel.`,
182+
).not.toBe(AGENT_CONFIGURED_MODEL);
183+
expect(
184+
row?.modelProvider,
185+
`ACP session ${ACP_SESSION_KEY} reports modelProvider="${row?.modelProvider}" — the ` +
186+
`agent-configured provider (${AGENT_CONFIGURED_PROVIDER}), not the ACP runtime. ` +
187+
`Same fix site as above; the overlay must gate on entry.acp presence.`,
188+
).not.toBe(AGENT_CONFIGURED_PROVIDER);
189+
});
190+
191+
it("RED (fix-shape): ACP control-plane session should report the ACP runtime sentinel", async () => {
192+
// RED before fix; GREEN once the catalog-#20 sentinel-overlay fix lands.
193+
//
194+
// The catalog's chosen fix shape: when `isAcpSessionKey(row.key)` is true
195+
// AND `entry.acp != null`, overlay `{ provider: "acpx", model: "<agentId>-acp" }`.
196+
// This trades model-name accuracy for "this is ACP control-plane, not the
197+
// agent default" clarity. Plumbing the actual copilot-side model selection
198+
// into the openclaw record would require capturing ACP `session.model_change`
199+
// events (catalog notes this as deferrable).
200+
const store = writeStore(
201+
{ [ACP_SESSION_KEY]: buildAcpSessionEntry() },
202+
"sessions-acp-model-display-fix-shape",
203+
);
204+
205+
const payload = await runSessionsJson<SessionsJsonPayload>(sessionsCommand, store);
206+
const row = payload.sessions?.find((entry) => entry.key === ACP_SESSION_KEY);
207+
208+
expect(row).toBeDefined();
209+
expect(
210+
row?.model,
211+
`ACP session ${ACP_SESSION_KEY} should resolve model to "copilot-acp" (the catalog-chosen ` +
212+
`sentinel). Got "${row?.model}". Fix gates on isAcpSessionKey(row.key) AND entry.acp != null ` +
213+
`and overlays { provider: "acpx", model: "copilot-acp" }. Keeps resolveSessionDisplayModelRef pure.`,
214+
).toBe("copilot-acp");
215+
expect(
216+
row?.modelProvider,
217+
`ACP session ${ACP_SESSION_KEY} should resolve modelProvider to "acpx". Got ` +
218+
`"${row?.modelProvider}". Same fix as the model assertion above; the overlay sets both ` +
219+
`fields together so they remain internally consistent.`,
220+
).toBe("acpx");
221+
});
222+
223+
it("GREEN control: ACP bridge session (ACP key, no entry.acp) reports the configured model", async () => {
224+
// ACP bridge sessions (translator.ts) use ACP-shaped keys but never
225+
// persist SessionAcpMeta to disk. They run the normal configured model
226+
// and must NOT receive the acpx sentinel. This guards against a regression
227+
// where key-shape-only detection would misreport bridge sessions.
228+
const ACP_BRIDGE_SESSION_KEY = "agent:copilot:acp:bridge-session-1";
229+
const store = writeStore(
230+
{ [ACP_BRIDGE_SESSION_KEY]: buildAcpBridgeSessionEntry() },
231+
"sessions-acp-model-display-bridge-control",
232+
);
233+
234+
const payload = await runSessionsJson<SessionsJsonPayload>(sessionsCommand, store);
235+
const row = payload.sessions?.find((entry) => entry.key === ACP_BRIDGE_SESSION_KEY);
236+
237+
expect(row).toBeDefined();
238+
expect(
239+
row?.model,
240+
`ACP bridge session ${ACP_BRIDGE_SESSION_KEY} has an ACP-shaped key but no entry.acp — ` +
241+
`it ran the configured model. Got model="${row?.model}"; expected "${AGENT_CONFIGURED_MODEL}". ` +
242+
`The overlay must gate on entry.acp != null, not key shape alone.`,
243+
).toBe(AGENT_CONFIGURED_MODEL);
244+
expect(
245+
row?.modelProvider,
246+
`ACP bridge session ${ACP_BRIDGE_SESSION_KEY} should report the configured provider. ` +
247+
`Got "${row?.modelProvider}"; expected "${AGENT_CONFIGURED_PROVIDER}".`,
248+
).toBe(AGENT_CONFIGURED_PROVIDER);
249+
});
250+
251+
it("GREEN control: non-ACP session correctly reports the agent-configured model", async () => {
252+
// GREEN today. The same agent configuration drives a non-ACP session
253+
// (`agent:copilot:main`) — and for that session the agent-configured
254+
// model IS the right answer because the openclaw-agent-driven flow
255+
// actually runs that model. This control proves:
256+
// 1. The test infrastructure is exercising the real resolver path
257+
// (not a mock that would silently pass either way).
258+
// 2. The configured-model branch of resolveSessionDisplayModelRef
259+
// remains correct for non-ACP keys; the proposed sentinel overlay
260+
// must NOT break this case (it should only fire when both
261+
// isAcpSessionKey(row.key) is true AND entry.acp is present).
262+
const store = writeStore(
263+
{ [NON_ACP_SESSION_KEY]: buildNonAcpSessionEntry() },
264+
"sessions-acp-model-display-green-control",
265+
);
266+
267+
const payload = await runSessionsJson<SessionsJsonPayload>(sessionsCommand, store);
268+
const row = payload.sessions?.find((entry) => entry.key === NON_ACP_SESSION_KEY);
269+
270+
expect(row).toBeDefined();
271+
expect(row?.model).toBe(AGENT_CONFIGURED_MODEL);
272+
expect(row?.modelProvider).toBe(AGENT_CONFIGURED_PROVIDER);
273+
});
274+
});

0 commit comments

Comments
 (0)