Skip to content

Commit 2d8524c

Browse files
SDSLeonSDSLeon
authored
fix(codex): normalize MCP, skill, and collab agent tool items (#201)
- Map Codex mcpToolCall and collabAgentToolCall notifications to canonical payloads - Classify skills, MCP servers, and subagents in usage recording - Show SKILL.md reads as skill displays in chat tool UI - Unwrap MCP result content text blocks for ACP tool output
1 parent ac98a4a commit 2d8524c

11 files changed

Lines changed: 517 additions & 28 deletions

File tree

‎src/main/projectDirectory.test.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { createProjectDirectory, describeMkdirError } from "./projectDirectory";
66

77
describe("createProjectDirectory", () => {
88
let root: string;
9+
const nativeKind = process.platform === "win32" ? "windows" : "posix";
910

1011
beforeEach(() => {
1112
root = mkdtempSync(join(tmpdir(), "lc-create-project-"));
@@ -16,7 +17,11 @@ describe("createProjectDirectory", () => {
1617
});
1718

1819
test("creates the folder under the parent and returns its path", async () => {
19-
const result = await createProjectDirectory({ parent: root, name: "new-app", kind: "posix" });
20+
const result = await createProjectDirectory({
21+
parent: root,
22+
name: "new-app",
23+
kind: nativeKind,
24+
});
2025

2126
const expected = join(root, "new-app");
2227
expect(result.path).toBe(expected);
@@ -25,16 +30,16 @@ describe("createProjectDirectory", () => {
2530
});
2631

2732
test("throws when a folder with that name already exists", async () => {
28-
await createProjectDirectory({ parent: root, name: "dup", kind: "posix" });
33+
await createProjectDirectory({ parent: root, name: "dup", kind: nativeKind });
2934

3035
await expect(
31-
createProjectDirectory({ parent: root, name: "dup", kind: "posix" }),
36+
createProjectDirectory({ parent: root, name: "dup", kind: nativeKind }),
3237
).rejects.toThrow(/already exists/i);
3338
});
3439

3540
test("surfaces a friendly message when the parent does not exist", async () => {
3641
await expect(
37-
createProjectDirectory({ parent: join(root, "missing"), name: "app", kind: "posix" }),
42+
createProjectDirectory({ parent: join(root, "missing"), name: "app", kind: nativeKind }),
3843
).rejects.toThrow(/no longer exists/i);
3944
});
4045
});

‎src/renderer/components/thread/ChatPane/parts/items/acpToolPayload.test.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,21 @@ describe("acpToolPayload", () => {
4040
});
4141
});
4242

43+
it("unwraps MCP content text blocks from result objects", () => {
44+
expect(
45+
extractAcpResultPart({
46+
result: {
47+
content: [{ type: "text", text: '{"count":1}' }],
48+
structuredContent: null,
49+
_meta: null,
50+
},
51+
}),
52+
).toEqual({
53+
text: '{\n "count": 1\n}',
54+
language: "json",
55+
});
56+
});
57+
4358
it("synthesizes a unified diff from replacement-style edit args", () => {
4459
expect(
4560
extractAcpDiffResultPart({

‎src/renderer/components/thread/ChatPane/parts/items/acpToolPayload.ts‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -63,18 +63,26 @@ export function extractAcpResultPart(payload: unknown): ExtractedPart {
6363
return asPart(prettyIfJson(r.detailedContent));
6464
if (typeof r.text === "string" && r.text.length > 0) return asPart(prettyIfJson(r.text));
6565
if (typeof r.content === "string" && r.content.length > 0) return asPart(prettyIfJson(r.content));
66+
if (Array.isArray(r.content)) {
67+
const part = extractTextBlockPart(r.content);
68+
if (part.text.length > 0) return part;
69+
}
6670
if (Array.isArray(r.contents)) {
67-
const parts = r.contents
68-
.map((c) => (c && typeof c.text === "string" ? prettyIfJson(c.text) : ""))
69-
.filter((t) => t.length > 0);
70-
if (parts.length > 0) {
71-
const joined = parts.join("\n\n");
72-
return { text: joined, language: parts.every(isJsonText) ? "json" : "plain" };
73-
}
71+
const part = extractTextBlockPart(r.contents);
72+
if (part.text.length > 0) return part;
7473
}
7574
return { text: safeJson(result), language: "json" };
7675
}
7776

77+
function extractTextBlockPart(blocks: readonly ({ text?: unknown } | undefined)[]): ExtractedPart {
78+
const parts = blocks
79+
.map((c) => (c && typeof c.text === "string" ? prettyIfJson(c.text) : ""))
80+
.filter((t) => t.length > 0);
81+
if (parts.length === 0) return emptyPart();
82+
const joined = parts.join("\n\n");
83+
return { text: joined, language: parts.every(isJsonText) ? "json" : "plain" };
84+
}
85+
7886
/**
7987
* Read-tool result serializer. Unwraps OpenCode's
8088
* `<path>…</path><type>file</type><content>…</content>` wrapper and strips

‎src/renderer/components/thread/ChatPane/parts/items/toolDisplay.test.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { describe, expect, it } from "vitest";
2-
import { Eye, GitBranch, ImageIcon, Pencil, SearchCode, Terminal } from "lucide-react";
2+
import { Eye, GitBranch, ImageIcon, Pencil, SearchCode, Sparkles, Terminal } from "lucide-react";
33
import type { ToolCallPayload } from "@/shared/contracts";
44
import { deriveToolDisplay, isSubAgentTool } from "./toolDisplay";
55

@@ -134,6 +134,19 @@ describe("deriveToolDisplay", () => {
134134
expect(display.Icon).toBe(Eye);
135135
});
136136

137+
it("normalizes skill file reads to skill displays", () => {
138+
const display = deriveToolDisplay(
139+
makePayload({
140+
name: "Read",
141+
args: { file_path: String.raw`C:\Users\sdsle\.codex\skills\.system\imagegen\SKILL.md` },
142+
}),
143+
);
144+
145+
expect(display.title).toBe("Skill: imagegen");
146+
expect(display.parts).toBeUndefined();
147+
expect(display.Icon).toBe(Sparkles);
148+
});
149+
137150
it("includes line ranges for read tools that provide offsets", () => {
138151
const display = deriveToolDisplay(
139152
makePayload({

‎src/renderer/components/thread/ChatPane/parts/items/toolDisplay.ts‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ export function deriveToolDisplay(payload: ToolCallPayload): ToolDisplay {
108108
}
109109

110110
if (isSkillTool(payload)) {
111-
const skill = readStr(args, "skill") ?? readStr(args, "name");
111+
const skill = readStr(args, "skill") ?? readStr(args, "name") ?? readSkillName(payload);
112112
return { title: skill ? i18n._(msg`Skill: ${skill}`) : payload.name, Icon: Sparkles };
113113
}
114114

@@ -458,7 +458,30 @@ export function isSkillTool(payload: ToolCallPayload): boolean {
458458
const n = payload.name.trim();
459459
if (n === "Skill" || /^(loaded|using) skill\b/i.test(n)) return true;
460460
const args = readArgsObject(payload);
461-
return readStr(args, "skill") !== undefined;
461+
return readStr(args, "skill") !== undefined || readSkillName(payload) !== undefined;
462+
}
463+
464+
function readSkillName(payload: ToolCallPayload): string | undefined {
465+
const args = readArgsObject(payload);
466+
return (
467+
readSkillNameFromPath(readPathArg(args)) ??
468+
readSkillNameFromPath(payload.title) ??
469+
readSkillNameFromPath(payload.name)
470+
);
471+
}
472+
473+
function readSkillNameFromPath(value: string | undefined): string | undefined {
474+
if (!value) return undefined;
475+
const cleaned = value
476+
.replace(/^(?:view(?:\s+\d+(?::\d+)?)?|read(?:ing)?|open(?:ing)?)[:\s]+/i, "")
477+
.trim()
478+
.replace(/^["'`]+|["'`]+$/g, "");
479+
const parts = cleaned.split(/[\\/]+/).filter(Boolean);
480+
if (parts.at(-1)?.toLowerCase() !== "skill.md") return undefined;
481+
const skillsIndex = parts.findLastIndex((part) => part.toLowerCase() === "skills");
482+
if (skillsIndex === -1 || skillsIndex >= parts.length - 2) return undefined;
483+
const skill = parts.at(-2);
484+
return skill && !skill.startsWith(".") ? skill : undefined;
462485
}
463486

464487
function formatAcpPathDisplay(

‎src/renderer/state/usageRecorder.test.ts‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,10 @@ function emittedTokenValues(provider: string): number[] {
3737
.map((event) => event.value ?? 0);
3838
}
3939

40+
function emittedEvents(): UsageEventInputPayload[] {
41+
return bridgeMock.appendUsageEvents.mock.calls.flatMap((call) => call[0].events);
42+
}
43+
4044
describe("usageRecorder token baseline", () => {
4145
beforeEach(() => {
4246
flushNow(); // drain anything a prior test left buffered
@@ -74,3 +78,113 @@ describe("usageRecorder token baseline", () => {
7478
expect(emittedTokenValues(provider)).toEqual([1_200]);
7579
});
7680
});
81+
82+
describe("usageRecorder item classification", () => {
83+
beforeEach(() => {
84+
flushNow();
85+
bridgeMock.appendUsageEvents.mockReset();
86+
bridgeMock.appendUsageEvents.mockResolvedValue(undefined);
87+
});
88+
afterEach(() => {
89+
flushNow();
90+
});
91+
92+
it("records Codex skill calls with lower-case skill names", () => {
93+
const thread = makeThread("skill-thread", "codex");
94+
recordRuntimeUsage(
95+
"skill-thread",
96+
[
97+
{
98+
type: "item.started",
99+
threadId: "skill-thread",
100+
itemId: "skill-codex",
101+
itemType: "tool_call",
102+
payload: { name: "skill", args: { skill: "imagegen" }, status: "running" },
103+
},
104+
],
105+
[thread],
106+
);
107+
108+
flushNow();
109+
expect(emittedEvents()).toContainEqual(
110+
expect.objectContaining({ kind: "skill", provider: "codex", name: "imagegen" }),
111+
);
112+
});
113+
114+
it("records Codex skill file reads as skills", () => {
115+
const thread = makeThread("skill-file-thread", "codex");
116+
recordRuntimeUsage(
117+
"skill-file-thread",
118+
[
119+
{
120+
type: "item.started",
121+
threadId: "skill-file-thread",
122+
itemId: "skill-file-codex",
123+
itemType: "dynamic_tool_call",
124+
payload: {
125+
name: "Read",
126+
args: {
127+
file_path: String.raw`C:\Users\sdsle\.codex\skills\.system\imagegen\SKILL.md`,
128+
},
129+
status: "running",
130+
},
131+
},
132+
],
133+
[thread],
134+
);
135+
136+
flushNow();
137+
expect(emittedEvents()).toContainEqual(
138+
expect.objectContaining({ kind: "skill", provider: "codex", name: "imagegen" }),
139+
);
140+
});
141+
142+
it("records Codex collab agent tool calls as subagents", () => {
143+
const thread = makeThread("subagent-thread", "codex");
144+
recordRuntimeUsage(
145+
"subagent-thread",
146+
[
147+
{
148+
type: "item.started",
149+
threadId: "subagent-thread",
150+
itemId: "collab-agent",
151+
itemType: "tool_call",
152+
payload: {
153+
name: "spawn_agent",
154+
isSubAgent: true,
155+
args: { prompt: "inspect one thing", agentType: "worker" },
156+
status: "running",
157+
},
158+
},
159+
],
160+
[thread],
161+
);
162+
163+
flushNow();
164+
expect(emittedEvents()).toContainEqual(
165+
expect.objectContaining({ kind: "subagent", provider: "codex", name: "worker" }),
166+
);
167+
});
168+
169+
it("records Codex MCP calls by server instead of generic mcp", () => {
170+
const thread = makeThread("mcp-thread", "codex");
171+
recordRuntimeUsage(
172+
"mcp-thread",
173+
[
174+
{
175+
type: "item.started",
176+
threadId: "mcp-thread",
177+
itemId: "mcp-codex",
178+
itemType: "mcp_tool_call",
179+
payload: { name: "mcpToolCall", server: "browser", status: "running" },
180+
},
181+
],
182+
[thread],
183+
);
184+
185+
flushNow();
186+
expect(emittedEvents()).toContainEqual(
187+
expect.objectContaining({ kind: "mcp", provider: "codex", name: "browser" }),
188+
);
189+
});
190+
});

‎src/renderer/state/usageRecorder.ts‎

Lines changed: 63 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -167,46 +167,100 @@ function classifyItem(itemType: string, payload: unknown): ItemHit | undefined {
167167
const name = str(p, "name") ?? "";
168168
const title = str(p, "title") ?? "";
169169
const args = asRecord(p?.["args"]);
170+
const lowerName = name.toLowerCase();
170171

171172
if (itemType === "mcp_tool_call" || /^mcp__/.test(name)) {
172173
const match = /^mcp__(.+?)__/.exec(name);
173-
return { kind: "mcp", name: match?.[1] ?? str(p, "serverId") ?? "mcp" };
174+
return {
175+
kind: "mcp",
176+
name:
177+
match?.[1] ??
178+
str(p, "serverId") ??
179+
str(p, "server") ??
180+
str(args, "serverId") ??
181+
str(args, "server") ??
182+
"mcp",
183+
};
174184
}
175185
if (
176-
name === "Skill" ||
186+
lowerName === "skill" ||
177187
/^(loaded|using) skill\b/i.test(name) ||
178-
/^(loaded|using) skill\b/i.test(title)
188+
/^(loaded|using) skill\b/i.test(title) ||
189+
readSkillName(p, args) !== undefined
179190
) {
180191
const skill =
181192
str(args, "skill") ??
182193
str(args, "name") ??
194+
readSkillName(p, args) ??
183195
title
184196
.replace(/^(loaded|using) skill[:\s]*/i, "")
185197
.replace(/^skill:\s*/i, "")
186198
.trim();
187199
return { kind: "skill", name: skill || "skill" };
188200
}
189-
const subagentType = str(args, "subagent_type");
201+
const subagentType =
202+
str(args, "subagent_type") ?? str(args, "agent_type") ?? str(args, "agentType");
190203
if (
191204
p?.["isSubAgent"] === true ||
192-
name === "Task" ||
193-
name === "Workflow" ||
194-
name === "Agent" ||
205+
lowerName === "task" ||
206+
lowerName === "workflow" ||
207+
lowerName === "agent" ||
208+
lowerName === "collabagenttoolcall" ||
209+
lowerName === "collab agent tool call" ||
195210
subagentType
196211
) {
197212
// Prefer the agent type (Task/Agent); for workflows use the saved name or
198213
// description (inline script workflows carry neither, so they bucket under a
199214
// generic "workflow"); otherwise the task description.
200215
const agent =
201216
subagentType ??
202-
(name === "Workflow"
217+
(lowerName === "workflow"
203218
? (str(args, "name") ?? str(args, "description") ?? "workflow")
204-
: (str(args, "description") ?? "subagent"));
219+
: (str(args, "description") ?? str(args, "prompt") ?? "subagent"));
205220
return { kind: "subagent", name: agent };
206221
}
207222
return undefined;
208223
}
209224

225+
function readSkillName(
226+
payload: Record<string, unknown> | undefined,
227+
args: Record<string, unknown> | undefined,
228+
): string | undefined {
229+
return (
230+
str(args, "skill") ??
231+
str(args, "name") ??
232+
readSkillNameFromPath(readPathArg(args)) ??
233+
readSkillNameFromPath(str(payload, "title")) ??
234+
readSkillNameFromPath(str(payload, "name"))
235+
);
236+
}
237+
238+
function readPathArg(args: Record<string, unknown> | undefined): string | undefined {
239+
return (
240+
str(args, "file_path") ??
241+
str(args, "filePath") ??
242+
str(args, "path") ??
243+
str(args, "relative_path") ??
244+
str(args, "relativePath") ??
245+
str(args, "notebook_path") ??
246+
str(args, "notebookPath")
247+
);
248+
}
249+
250+
function readSkillNameFromPath(value: string | undefined): string | undefined {
251+
if (!value) return undefined;
252+
const cleaned = value
253+
.replace(/^(?:view(?:\s+\d+(?::\d+)?)?|read(?:ing)?|open(?:ing)?)[:\s]+/i, "")
254+
.trim()
255+
.replace(/^["'`]+|["'`]+$/g, "");
256+
const parts = cleaned.split(/[\\/]+/).filter(Boolean);
257+
if (parts.at(-1)?.toLowerCase() !== "skill.md") return undefined;
258+
const skillsIndex = parts.findLastIndex((part) => part.toLowerCase() === "skills");
259+
if (skillsIndex === -1 || skillsIndex >= parts.length - 2) return undefined;
260+
const skill = parts.at(-2);
261+
return skill && !skill.startsWith(".") ? skill : undefined;
262+
}
263+
210264
/** Record an AI-performed git action (commit / PR / conflict) into the buffer. */
211265
export function recordAiAction(type: AiActionType, provider: string, model: string): void {
212266
push({ ts: Date.now(), kind: `ai_${type}`, provider, model, value: 1 });

0 commit comments

Comments
 (0)