feat(agent-adapter,ui): surface codex skill brand icons and display names in the command catalog - #431
feat(agent-adapter,ui): surface codex skill brand icons and display names in the command catalog#431Zerlight wants to merge 15 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR carries provider-supplied command branding (display names, icons, brand colors) end-to-end—starting from codex skills/list through the schema and wire protocol—so the UI can render branded command rows/chips. It also normalizes opencode MCP tool titles to the shared mcp__<server>__<tool> slug and adds integration brand glyphs + per-brand activity grouping in the chat timeline.
Changes:
- Extend
AgentCommand(schema + wire v74) withdisplayName,iconDataUri, andbrandColor, and propagate these through codex command ingest (with per-icon + total payload caps). - Add UI command catalog context + branded glyph/tint rendering across the composer, transcript command echoes, tool rows, and activity-run headers/groups.
- Normalize opencode MCP tool names to the shared MCP slug for both live streaming and cold history reads, including an engine-supplied injected-server-name hint.
Reviewed changes
Copilot reviewed 48 out of 49 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Add @iconify-json/simple-icons to the shared catalog for brand glyph imports. |
| pnpm-lock.yaml | Lockfile updates for @iconify-json/simple-icons. |
| package.json | Add @iconify-json/simple-icons workspace dependency. |
| eslint.config.cjs | Allow unplugin-icons virtual module usage in the new integration brand component. |
| apps/webview/package.json | Add @iconify-json/simple-icons dependency for webview build. |
| apps/desktop/package.json | Add @iconify-json/simple-icons dependency for desktop build. |
| packages/presentation/ui/src/tool-utils.ts | Update MCP slug parsing doc comment to reflect shared cross-adapter usage. |
| packages/presentation/ui/src/shell/conversation-surface.tsx | Provide command catalog context to the conversation surface for transcript echo chips. |
| packages/presentation/ui/src/shell/composer-editor/directive-state.ts | Add commandFor lookup for branded composer directive chips. |
| packages/presentation/ui/src/shell/composer-editor/chips.tsx | Render branded command chip glyphs/tints in the composer. |
| packages/presentation/ui/src/shell/composer-command.tsx | Show command display names in the command catalog and render branded glyphs for command entries. |
| packages/presentation/ui/src/chat/user-message.tsx | Detect /command args echoes and render a branded chip when the command is in the catalog. |
| packages/presentation/ui/src/chat/tool.tsx | Prefer caller/plugin-provided glyphs even when a tool call fails. |
| packages/presentation/ui/src/chat/tool-call-item.tsx | Add integration brand glyph fallback for MCP tool headers. |
| packages/presentation/ui/src/chat/integration-brand.tsx | New: map MCP server-name tokens to known integration glyphs + labels. |
| packages/presentation/ui/src/chat/command-catalog.ts | New: build/consume an O(1) command lookup for transcript echo branding. |
| packages/presentation/ui/src/chat/command-brand.tsx | New: branded command glyph rendering + runtime tint style + catalog provider. |
| packages/presentation/ui/src/chat/activity-summary.ts | Add brand resolution for activity items/runs based on MCP server name. |
| packages/presentation/ui/src/chat/activity-run.tsx | Render branded activity run headers/icons and add i18n keys for branded wording. |
| packages/presentation/ui/src/chat/activity-groups.ts | Split activity runs by integration brand; glue thinking into surrounding runs. |
| packages/presentation/ui/src/chat/tests/user-message.test.tsx | Test transcript echo branding behavior for known vs unknown leading slashes. |
| packages/presentation/ui/src/chat/tests/tool-call-metadata.test.tsx | Test that known integration MCP calls wear a brand glyph in the header icon slot. |
| packages/presentation/ui/src/chat/tests/integration-brand.test.tsx | New: unit tests for server-name token matching + glyph rendering. |
| packages/presentation/ui/src/chat/tests/activity-run.test.tsx | Test branded run naming/icon behavior, including failure-state tint rules. |
| packages/presentation/ui/src/tests/composer-command.test.ts | Test displayName query matching and hint formatting. |
| packages/presentation/ui/src/tests/activity-summary.test.ts | Add tests for activityRunBrand resolution rules. |
| packages/presentation/ui/src/tests/activity-groups.test.ts | Add tests for per-brand grouping and glue behavior. |
| packages/presentation/i18n/src/locales/zh-cn.ts | Add branded integration activity strings. |
| packages/presentation/i18n/src/locales/en.ts | Add branded integration activity strings. |
| packages/host/engine/src/session/start-options-resolver.ts | Provide engine-side injected MCP server-name hint for cold history reads. |
| packages/host/engine/src/session/history-service.ts | Thread injected server-name hint into adapter history reads (adapter-local context). |
| packages/host/engine/src/engine.ts | Wire the injected-server-name hint provider into HistoryService. |
| packages/host/engine/src/tests/start-options-mcp.test.ts | Test injected MCP server-name enumeration. |
| packages/host/engine/src/tests/history-service.test.ts | Test that injected MCP server names are passed through on cold reads. |
| packages/host/engine/src/tests/fixtures/history-adapter.ts | Update test adapter to accept AgentHistoryReadContext. |
| packages/host/agent-adapter/src/native/opencode/history.ts | Normalize opencode MCP tool names to mcp__<server>__<tool> when resolvable. |
| packages/host/agent-adapter/src/native/opencode/adapter.ts | Use injected/configured server-name sets to retitle opencode MCP tools live + in history. |
| packages/host/agent-adapter/src/native/codex/adapter.ts | Ingest codex skill brand identity and embed/cap icon payloads as data URIs. |
| packages/host/agent-adapter/src/base.ts | Update base adapter history read signature to AgentHistoryReadContext. |
| packages/host/agent-adapter/src/adapter.ts | Introduce AgentHistoryReadContext (wire options + engine-local hints). |
| packages/host/agent-adapter/src/tests/opencode.test.ts | Test live normalization using injected + config-declared MCP servers. |
| packages/host/agent-adapter/src/tests/opencode-history.test.ts | Test opencode MCP prefix splitting + retitling behavior. |
| packages/host/agent-adapter/src/tests/opencode-history-adapter.test.ts | Test cold-read retitling using engine hint + config servers. |
| packages/host/agent-adapter/src/tests/codex-commands.test.ts | Test codex brand identity projection + icon embedding + payload budget cap behavior. |
| packages/foundation/schema/src/wire/message.ts | Bump WIRE_PROTOCOL_VERSION 73 → 74 for additive wire schema changes. |
| packages/foundation/schema/src/model/agent/input.ts | Extend AgentCommand schema with displayName, iconDataUri, brandColor. |
| packages/client/workbench/tests/integration/dev-mock-transport.test.ts | Update mock transport expectations to include branded commands. |
| packages/client/workbench/src/mock/data/showcase.ts | Add branded MCP call examples for the UI showcase. |
| packages/client/workbench/src/mock/data/commands.ts | Add branded mock command fixtures with data-URI icon + brandColor. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/host/agent-adapter/src/native/opencode/adapter.ts:731
readHistoryalso treats MCP server-name resolution as best-effort, butclient.config.get(...)is awaited without error handling. A rejected config read would fail the entire history read (and thus transcript loading) even though only MCP retitling depends on it. Swallow config-get failures and proceed with just the injected/engine-providedmcpServerNames.
// Best-effort MCP server names so replayed MCP tool titles converge with live ones — a
// pure config read, never `mcp.status` (its lazy init would spawn/dial every configured
// server from the shared history instance). Config-declared servers resolve, including
// disabled ones; engine-injected servers exist only on a session's own live instance, so
// the caller's `mcpServerNames` hint is the only way their calls resolve here.
const names = new Set(opts.mcpServerNames);
const config = await client.config.get({ directory: got.data.directory });
for (const name of Object.keys(config.data?.mcp ?? {})) names.add(name);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
- Schema is correctly additive.
displayName/iconDataUri/brandColorare all optional, andWIRE_PROTOCOL_VERSION73→74 withMIN_COMPATIBLE_WIRE_VERSIONuntouched is the right call — nothing is removed, renamed, or re-meant, so no lockstep upgrade is forced. - Codex icon embedding looks sound. Extension→MIME allowlist, 32 KiB per-icon cap,
statbeforereadFile, andcapSkillIconPayloaddropping (not truncating) over-budget icons. The refresh-generation staleness re-check is correctly placed after the awaited icon reads, so a concurrent refresh can't emit a stale catalog. - opencode retitling traced end-to-end. Longest-sanitized-prefix matching for
sanitize(server)_sanitize(tool)is the right disambiguator given opencode's flat naming, and routing both live parts and permission asks throughopencodeMcpTitlekeeps the header consistent.AgentHistoryReadContextstaying adapter-local rather than on the wire is the right boundary. - Engine reorder is safe.
SessionStartOptionsResolvermoves aboveHistoryService, and bothcustomMcpandpluginsare already initialized at that point — no TDZ. - Checked and cleared: the desktop CSP already allows
img-src … data:so embedded icons render;--foregroundresolves to a real color so thecolor-mixchip style is valid; the un-guardedclient.config.get()calls are fine because the generated opencode client resolves{ error }rather than throwing (as the existing comment atopencode/adapter.ts:124documents) and both sites null-check.data?.mcp. ACTIVITY_RUN_GLUE_KEYmechanics verified against the new tests — glue items attach to an open run without opening one themselves, andflushRunresetsrunKeyso a trailing glue item can't leak into the next group.
ℹ️ .claude/rules/frontend.md now contradicts the dependency set
The renderer rule still reads:
Brand / agent icons:
@proj-airi/lobe-iconsviaunplugin-icons— chosen oversimple-icons, which at adoption time (2026-07) lacked usableclaudecode/opencodeglyphs after trademark removals.
This PR adds @iconify-json/simple-icons as a second brand-icon collection (catalog, root/desktop/webview manifests, eslint override, /// <reference> — all wired correctly). The wiring is fine; the guidance isn't. As written, the next contributor reaching for an integration glyph is told not to use the collection this PR just standardized on. Worth a line in that rule saying which collection owns which case — lobe-icons for agent/model brands, simple-icons for third-party MCP integrations, or whatever the intended split is.
Nitpicks (not blocking)
toolCallFromPartnow deriveskindfrom the retitled slug rather than the raw provider name.tool-utils.ts's updated JSDoc acknowledges that a server key containing__mis-splits — previously that was cosmetic (a wrong header label), but now the same mis-split also feedstoolKindFromName, so it can flip a call's kind and kind icon. Still low-impact, just a slightly wider blast radius than the JSDoc implies.HistoryService.injectedMcpServerNamesresolves today's injected server set and applies it to historical sessions. A server that was enabled when the session ran but has since been disabled won't retitle on a cold read. Probably acceptable — just noting the reads aren't point-in-time.- Skill icon reads fan out through an unbounded
Promise.all. Fine for realistic catalog sizes; only a concern if a workspace ever carries hundreds of skills.
Claude Opus ｜ 𝕏
a730f54 to
8cb9c94
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/presentation/ui/src/chat/integration-brand.tsx:88
- IntegrationIcon renders a decorative SVG glyph but does not mark it as decorative. Other icon components in this codebase set
aria-hiddenon glyph SVGs (e.g. AgentIcon), and leaving it out can cause screen readers to announce the icon redundantly in tool headers and activity runs.
|
frontend.md now records the collection split (8cb9c94): lobe-icons stays the agent/model brand collection; third-party integration glyphs use static The three nitpicks are acknowledged and deliberately left as-is: the |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior review at a730f54f the branch was rebased onto a moved ruocheng/code-412 (which itself gained 9a3fcb97 and c4fa4a17) and picked up two commits of its own. The automatic range-diff failed to compute, so I isolated this PR's delta from the base-branch churn commit-by-commit before reviewing — the base commits' edits to codex/tool-view.ts, codex/history-tools.ts, tool-search.tsx, and tool-utils.ts's search case belong to the parent PR and are deliberately out of scope here.
-
Guarded opencode's best-effort config reads against transport rejections (
65d548bd). BothcollectMcpServerNamesandreadHistorynow wrapclient.config.getintry/catch. I checked the load-bearing claim against the installed SDK rather than taking the comment's word for it:@opencode-ai/sdk@1.18.3'sdist/gen/client/client.gen.jsdoeslet response = await _fetch(request)with no wrapper, andwrapClientErroronly converts whenthrowOnErroris set — so a dead-server fetch genuinely rejects and these catches are load-bearing, not dead code. This also corrects my own prior "checked and cleared" call, which read the{error}contract as covering transport failures too. The live path matters most:collectMcpServerNamessits inonStart'sPromise.allalongside three siblings that already swallow their own failures, so an unguarded rejection there would have failed session start outright. -
Gated the transcript command-echo chip to single-line messages (
8cb9c949).commandEchonow returnsundefinedfor any text containing\n, so multi-line arguments fall back toContentBlockViewblock rendering. This also makes thecollapsibleline-clamp interaction moot by construction — an echo can no longer exceed one line. The existingitem.blocks.length === 1guard atuser-message.tsx:112already kept mixed text+image messages off the chip path. -
Corrected the failed-call glyph comments to match shipped behavior. Rather than narrowing
ToolIcon'sicon ?? <CircleXIcon>back to the brand path, the comments attool.tsx:157andtool-call-item.tsx:107now state that any caller-supplied glyph survives a failure — a deliberate widening, with the stale "state glyphs still override" line fixed. -
Reconciled
.claude/rules/frontend.mdwith the two-collection icon split, naming lobe-icons as the agent/model collection and~icons/simple-icons/*(viachat/integration-brand.tsx) as the third-party MCP integration collection. That closes the guidance/dependency contradiction from the prior review.
Both new tests are real regression guards, not theatre: drop the try/catch and the opencode test's readHistory rejects; drop the newline gate and the user-message test's img//documents assertions both fire. Ran locally — 21 tests across the two files pass.
Claude Opus ｜ 𝕏
…s with integration icons
…ated activity groups
8cb9c94 to
7473d53
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/foundation/schema/src/model/agent/input.ts:103
iconDataUriis validated only by prefix (data:image/) with no length cap. Given this field is carried on the wire, an unexpectedly large data URI (from a buggy/malicious adapter or future provider) can bloat frames and stress transport reassembly/memory. Other wire payloads (e.g. attachments) enforce.max(...)limits; consider doing the same here for defense-in-depth.
/** Small brand icon embedded as a data URI — size-capped at adapter ingest (per icon and in
* aggregate per catalog), so consumers can render it directly (no asset endpoint exists for
* command icons). */
iconDataUri: z.string().startsWith('data:image/').optional(),
/** Brand accent for icon fallbacks (menu initial chips). */
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior review at 8cb9c949 this PR added exactly one commit. The base branch also moved (c4fa4a17 → 794b9355), and the automatic range-diff again failed to compute, so I separated the two before reviewing: the only tree difference besides the commit below is codex/tool-view.ts un-exporting CODEX_PLUGIN_APPS_SERVER, which is parent-PR churn (the file is absent from this PR's 51-file diff) and has no consumer outside its own module.
- Marked the integration brand glyph decorative (
7473d532).IntegrationIconnow passesaria-hiddento thesimple-iconsSVG, matching the sibling glyph components in this package (agent-icon.tsx,service-icon.tsx,link-icon.tsx,file-identity-icon.tsx) and completing the brand-glyph family —CommandBrandGlyphalready hadaria-hiddenon its initial chip andalt=""on its data-URIimg.
No information is lost at either call site, which is the thing worth checking on an aria-hidden: tool-call-item.tsx:120 renders the glyph into the header icon slot beside the visible title, and activity-run.tsx:237 renders it beside a run label whose text already carries the brand name via INTEGRATION_LABELS (activity-run.tsx:53). Screen readers keep "Linear"; they just stop announcing the logo twice.
The 35 tests across integration-brand, tool-call-metadata, and activity-run pass at this tip. Worth noting for anyone reproducing locally: these three files fail with Icon 'simple-icons/asana' not found until pnpm install runs against this branch's manifests — @iconify-json/simple-icons is new here, and a node_modules tree from before the branch makes the failure look like broken ~icons wiring rather than a missing dependency.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/host/agent-adapter/src/native/codex/adapter.ts:163
skillIconDataUrienforces the size cap viastat()beforereadFile(), but the file can change between the two (TOCTOU) and exceedSKILL_ICON_MAX_BYTES, resulting in a larger-than-intended payload being embedded and sent over the wire. Re-checkdata.byteLengthafter reading (and early-return if it exceeds the cap) so the cap is enforced on the actual bytes embedded.
export async function skillIconDataUri(iconPath: string): Promise<string | undefined> {
const mime = SKILL_ICON_MIME[extname(iconPath).toLowerCase()];
if (!mime) return undefined;
try {
const info = await stat(iconPath);
if (!info.isFile() || info.size === 0 || info.size > SKILL_ICON_MAX_BYTES) return undefined;
const data = await readFile(iconPath);
if (mime === 'image/svg+xml' && SVG_ACTIVE_CONTENT_RE.test(data.toString('utf8'))) {
return undefined;
}
return `data:${mime};base64,${data.toString('base64')}`;
} catch {
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior review at 7473d532 this PR added exactly one commit. Unlike rounds 2 and 3, the range-diff computed cleanly and held nothing but that commit — no base-branch churn needed separating this time.
- Screened SVG skill icons for active content at codex ingest (
d6327339).skillIconDataUrinow rejects an.svgwhose bytes matchSVG_ACTIVE_CONTENT_RE—<script>,<foreignObject>,on*=handlers,javascript:, and non-fragmenthref/xlink:href— and theAgentCommand.iconDataUridocstring gained the render contract it depends on:<img>only, never inlined markup, "the screen is depth, not a sanitizer."
I checked the load-bearing premise rather than taking the docstring's word for it, since the whole design rests on it. SVG 2 §2.2.4/§2.3 normatively requires an SVG embedded via <img> to be processed in secure animated mode — script execution: no, external references: no, interactivity: no — and MDN's SVG as an image confirms JavaScript is disabled and external subresources are not fetched. Chromium's own position matches: issue 40058212 was closed Won't Fix with "Only SVGs in image tags are prevented from running JS", and the <foreignObject>-escapes-<img> report (499559357) was closed not-reproducible. The desktop renderer adds nothing that widens this (webSecurity, sandbox, contextIsolation on; nodeIntegration off). So the framing is right, and the gaps a byte-level regex must have — a UTF-16-encoded SVG read through toString('utf8'), entity-encoded payloads, SMIL <animate> triggers — are not exploitable behind <img>. That matters because the docstring is what stops a future contributor from reaching for dangerouslySetInnerHTML; command-brand.tsx:18 is the only iconDataUri render site repo-wide today, and adapter.ts:1002 (iconSmall ?? iconLarge) is the only producer, so the screen sits on a single funnel.
The false-positive cost is measured rather than assumed: I ran the regex over 281 real .svg files ≤32 KiB under node_modules, and exactly one was rejected — a design-tool export carrying <image href="data:image/png;base64,…">. So a skill shipping a logo with an embedded raster silently loses its icon and falls back to the brandColor chip, which is the documented graceful degradation. Worth leaving alone: relaxing href= to permit data: would re-open nested data:image/svg+xml references, and the fallback already reads fine.
The new test is a real guard, not theatre. Four exact toBeUndefined() rejections plus a toBeDefined() positive control that pins the legitimate fragment-ref idiom (<use xlink:href="#g"> alongside fill="url(#g)") as still allowed — delete the screen and the four rejections fail; over-tighten it to any href at all and both the positive control and the existing catalog test's <svg xmlns="…"/> fixture fail. All 15 tests in codex-commands.test.ts pass at this tip, eslint reports 0 errors on the touched files (one pre-existing consistent-function-scoping warning on the older dataUri helper), and biome reports no format changes.
Claude Opus ｜ 𝕏
Summary
Stacked on the CODE-412 PR (base
ruocheng/code-412); only the 11 commits above it are new here.Codex plugin skills ship brand identity on
skills/list(interface.displayName/iconSmall/brandColor— verified live on codex 0.144.6); carry it through instead of dropping it (Closes CODE-525):AgentCommandfields (displayName,iconDataUri,brandColor); icons travel as data URIs, size-capped per icon and in aggregate per catalog.WIRE_PROTOCOL_VERSION73 → 74, floor untouched — old clients unaffected.mcp__<server>__<tool>slug, live and on cold history reads. Cold reads resolve server names from the session directory's config plus a new engine-suppliedmcpServerNameshint, so calls to engine-injected servers (managed connectors, the daemon's simulator endpoint) retitle too. The hint is adapter-local context (AgentHistoryReadContext), deliberately off the wire.Verification
pnpm check:ci(0 errors) and fullpnpm exec vitest run(2764 passed) at this tip.skills/listinterface fields probed against a livecodex app-server(0.144.6); findings recorded in CODE-525.sanitize(server)_sanitize(tool)join verified against anomalyco/opencode 1.18.15 source; cold reads use a pureconfig.get— nevermcp.status, whose lazy init spawns/dials every configured server (noted inreadHistory).history-service,start-options-mcp).Checklist
pnpm check:ciandpnpm testboth pass (pluscargo fmt/clippy/testfor Rust changes)WIRE_PROTOCOL_VERSIONis bumped (73 → 74)