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 ｜ 𝕏
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 ｜ 𝕏
d632733 to
efd8ecb
Compare
|
Your Claude subscription has hit its usage limit. It resets at 3:10am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
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 (2)
packages/foundation/schema/src/wire/message.ts:16
- The PR description says this change bumps the wire protocol from 73 → 74, but this diff bumps it from 76 → 77 (and the floor is 76). Please reconcile the PR description with the actual wire versions so reviewers/users don’t get misled about compatibility impact.
export const WIRE_PROTOCOL_VERSION = 77 as const;
/** The oldest `v` this build still accepts. Bump only for a breaking change — a variant or field
* removed, renamed, or given a new meaning; additive changes leave it alone. */
export const MIN_COMPATIBLE_WIRE_VERSION = 76 as const;
packages/presentation/ui/src/chat/tests/user-message.test.tsx:159
- This test asserts on CSS serialization details (
chip.style.backgroundColorcontaining anrgb(...)substring). Since the implementation setsbackgroundColorto acolor-mix(...)expression, the exact string returned byelement.style.*is not stable across DOM/CSS implementations. Prefer asserting on the presence of thecolor-mixstyle (and optionally the brand hex) rather than a specific serialization format.
| 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 { | ||
| return undefined; | ||
| } | ||
| } |
…s with integration icons
…ated activity groups
efd8ecb to
e4a17a9
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 (2)
packages/foundation/schema/src/wire/message.ts:16
- PR description says
WIRE_PROTOCOL_VERSIONbumps 73 → 74, but the code change here sets it to 77 (withMIN_COMPATIBLE_WIRE_VERSIONstill 76). Please update the PR description (or, if the intent really was 73 → 74, adjust the constants accordingly) so reviewers/users don’t get misled about wire compatibility.
/** Stamped on every frame this build sends; bump on any wire schema change. */
export const WIRE_PROTOCOL_VERSION = 77 as const;
/** The oldest `v` this build still accepts. Bump only for a breaking change — a variant or field
* removed, renamed, or given a new meaning; additive changes leave it alone. */
export const MIN_COMPATIBLE_WIRE_VERSION = 76 as const;
packages/presentation/ui/src/chat/tests/user-message.test.tsx:159
- This assertion depends on how the CSSOM serializes
color-mix(...)(e.g. whether the embedded hex color is preserved vs normalized), which can vary across test environments and make the test brittle. Prefer asserting that the style usescolor-mix(or at least that the backgroundColor is non-empty) rather than matching a particularrgb(...)substring.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The head move since my last review (d6327339 → e4a17a95) carries no PR-own content changes — it is a pure rebase of the same 15 commits from base 794b9355 onto 6c83b89c.
- Evidence.
git range-diff 794b9355..d6327339 6c83b89c..e4a17a95marks 13 of 15 commits=(byte-identical). The three deltas are all rebase artifacts: the wire re-stamp (73→74became76→77because the base moved the number), and context-only churn instart-options-resolver.tsandpackages/host/agent-adapter/AGENTS.mdwhere the base independently edited neighboring lines. WIRE_PROTOCOL_VERSION76→77, floor untouched.MIN_COMPATIBLE_WIRE_VERSIONis 76 and was set by the base, not by this PR. Still the right call —displayName/iconDataUri/brandColorremain optional and additive, so nothing forces a lockstep upgrade.
Because the rebase merged textually across files the base also touched, I verified the merged result rather than trusting the clean git exit:
- No TDZ in
engine.ts. Construction order holds after the base's newadoptDetectedLoginsblock landed at:121-136:customMcp(:88) →plugins(:104) →translator(:105) →SessionStartOptionsResolver(:106) →HistoryService(:113, closing overstartOptions.injectedMcpServerNames). Both dependencies are initialized before the resolver. injectedMcpServerNamesstill agrees with its neighbor.start-options-resolver.ts:101readsthis.customMcp?.listEnabled()the same way the base-reshapedwithCustomMcpServers(:110) does — the rebase didn't split them apart.conversation-surface.tsxprovider wrapping intact.CommandCatalogProviderimported at:5, wrapping at:146/:164,composer.directiveControls.slashat:148-149,directiveControlsstill threaded at:211.AgentCommandSchemaadditions coexist.input.ts:95-114— the base's fields and this PR's three optional fields merged without collision or duplication.
Verification: pnpm typecheck (whole-solution tsc --build --noEmit) → 0 errors. pnpm exec vitest run over the four touched packages → 208 files / 1896 tests passed. (pnpm install --frozen-lockfile was required first — @iconify-json/simple-icons is new on this branch and wasn't present in the pre-installed tree.)
Both threads from my earlier reviews are resolved, and re-reading the adapter and UI regions turned up nothing rounds 1–4 hadn't already cleared.
ℹ️ The PR description still cites the pre-rebase wire versions
The body says 73 → 74 in two places. After the rebase the actual bump is 76 → 77 (packages/foundation/schema/src/wire/message.ts). Purely a description drift — the code is correct — but worth correcting before merge so the historical record doesn't disagree with the commit.
ℹ️ The "ran the affected surface" checkbox is unchecked
This PR is almost entirely visual (skill brand icons, display names, command-catalog chips, activity-run glyphs). Type-checking and the unit suite pass, but neither exercises rendering. Since the change is user-visible by nature, a quick pass over the command catalog and a codex session with plugin skills is worth doing before merge — particularly to confirm embedded data:image/* icons actually render under the desktop CSP and that the color-mix chip style resolves.
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)