mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
refactor(site/src/pages/AgentsPage/components/ChatElements/tools): replace label/icon switches with tables and registry invariants (#27706)
Stacked on #27697. Do not merge before it; this diff is against that branch, not main. Replaces the `ToolLabel` and `ToolIcon` string switches with lookup tables, and makes the registry/table agreement a CI failure instead of a manual audit. No behaviour change; every label and icon renders identically. ## Why Three enumerations of the same tool-name set (`toolRenderers`, the label switch, the icon switch) were kept in agreement by convention alone, and drifted repeatedly (#27684, #27687, #27697 each deleted arms a registered renderer had silently shadowed). Switches are unenumerable, so the drift was invisible to both tsc and tests. ## What changed - `genericToolLabels` (ToolLabel.tsx): the four generic-rendered labels (`process_signal`, `process_list`, `attach_file`, `advisor`) as a `Partial<Record<string, FC>>`. `ToolLabel` is now a table lookup plus the MCP/raw-name fallback. - `toolIcons` (ToolIcon.tsx): all 19 built-in icon names as a `Partial<Record<string, LucideIcon>>`, same pattern. Unknown/MCP names still fall to `WrenchIcon`. - `Tool.tsx`: exports `toolRenderers` (it is the single source of truth for dispatch; the tests read it directly). - `toolLabelVisibility.test.ts`: fails if a registered renderer that does not delegate to `GenericToolRenderer` shadows a `genericToolLabels` entry, naming the arm. `process_signal` (known delegator) and `advisor` (consumed directly by `AdvisorTool`) are allowlisted in the test. This is the tripwire requested in review on #27697, as a CI failure rather than a comment. - `toolIconsCoverage.test.ts`: fails if a registered renderer has no dedicated icon, with `read_skill_file` allowlisted for its `read_skill` icon alias. ## Design notes - The invariant checks live in tests, not in the type system. TypeScript cannot assert a runtime object's key set against an independent intent without either re-listing the names (the `satisfies Record<union, ...>` approach, rejected as ugly repetition) or abusing conditional types. The tables are plain objects; the tests own the invariant. A generated union from the Go `chattool` constants is the proper long-term fix and is deliberately out of scope. - No `as const` / union key types: they added annotation without buying safety the tests don't already provide, and nothing consumes `keyof typeof` here. ## Validation - `tsc --noEmit`, `biome check --error-on-warnings`, knip: clean. - Unit (`--project=unit src/pages/AgentsPage`): 1502 passed, 2 skipped (base: 1500/2; +2 are the new invariant tests). - Storybook (`--project=storybook src/pages/AgentsPage`): 949 passed, 2 failed, identical to base: `AgentChatPageView.stories.tsx > Scroll To Bottom Button Works With Inverse Scroll` and `Tool.stories.tsx > MCP Tool Completed`. Both reproduce on unmodified main, so pre-existing and unrelated. One additional flake (`AgentChatPage.stories.tsx > Slash Compact Yields To Personal Skill`) failed once under parallel load and passed in isolation on the final code. - Line delta vs #27697: +148 / -98 across 5 files. Generated by Coder Agents.
This commit is contained in:
@@ -1016,7 +1016,7 @@ const StartWorkspaceRenderer: FC<ToolRendererProps> = ({
|
||||
// Renderer lookup map for tool names and specialized renderers.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const toolRenderers: Record<string, FC<ToolRendererProps>> = {
|
||||
export const toolRenderers: Record<string, FC<ToolRendererProps>> = {
|
||||
execute: ExecuteRenderer,
|
||||
process_output: ProcessOutputRenderer,
|
||||
process_signal: ProcessSignalRenderer,
|
||||
|
||||
@@ -5,6 +5,7 @@ import {
|
||||
FilePenLineIcon,
|
||||
FileTextIcon,
|
||||
LightbulbIcon,
|
||||
type LucideIcon,
|
||||
MonitorIcon,
|
||||
PowerIcon,
|
||||
RouteIcon,
|
||||
@@ -22,6 +23,28 @@ import {
|
||||
} from "#/components/Tooltip/Tooltip";
|
||||
import { cn } from "#/utils/cn";
|
||||
|
||||
export const toolIcons: Partial<Record<string, LucideIcon>> = {
|
||||
execute: TerminalIcon,
|
||||
process_output: TerminalIcon,
|
||||
process_list: TerminalIcon,
|
||||
process_signal: TerminalIcon,
|
||||
read_file: FileTextIcon,
|
||||
read_skill: FileTextIcon,
|
||||
write_file: FilePenLineIcon,
|
||||
edit_files: FilePenLineIcon,
|
||||
list_templates: ServerIcon,
|
||||
read_template: ServerIcon,
|
||||
create_workspace: ServerIcon,
|
||||
start_workspace: PowerIcon,
|
||||
chat_summarized: BotIcon,
|
||||
list_agents: BotIcon,
|
||||
thinking: LightbulbIcon,
|
||||
propose_plan: RouteIcon,
|
||||
ask_user_question: BadgeQuestionMarkIcon,
|
||||
advisor: CompassIcon,
|
||||
computer: MonitorIcon,
|
||||
};
|
||||
|
||||
export const ToolIcon: React.FC<{
|
||||
name: string;
|
||||
iconUrl?: string;
|
||||
@@ -73,39 +96,6 @@ export const ToolIcon: React.FC<{
|
||||
return img;
|
||||
}
|
||||
|
||||
switch (name) {
|
||||
case "execute":
|
||||
case "process_output":
|
||||
case "process_list":
|
||||
case "process_signal":
|
||||
return <TerminalIcon className={base} />;
|
||||
case "read_file":
|
||||
case "read_skill":
|
||||
return <FileTextIcon className={base} />;
|
||||
case "write_file":
|
||||
case "edit_files":
|
||||
return <FilePenLineIcon className={base} />;
|
||||
case "list_templates":
|
||||
case "read_template":
|
||||
case "create_workspace":
|
||||
return <ServerIcon className={base} />;
|
||||
case "start_workspace":
|
||||
return <PowerIcon className={base} />;
|
||||
case "chat_summarized":
|
||||
case "list_agents":
|
||||
return <BotIcon className={base} />;
|
||||
case "thinking":
|
||||
return <LightbulbIcon className={base} />;
|
||||
case "propose_plan":
|
||||
return <RouteIcon className={base} />;
|
||||
case "ask_user_question":
|
||||
return <BadgeQuestionMarkIcon className={base} />;
|
||||
case "advisor":
|
||||
return <CompassIcon className={base} />;
|
||||
case "computer":
|
||||
return <MonitorIcon className={base} />;
|
||||
|
||||
default:
|
||||
return <WrenchIcon className={base} />;
|
||||
}
|
||||
const Icon = toolIcons[name] ?? WrenchIcon;
|
||||
return <Icon className={base} />;
|
||||
};
|
||||
|
||||
@@ -2,75 +2,86 @@ import type React from "react";
|
||||
import { getPathBasename } from "../../../utils/path";
|
||||
import { asRecord, asString, humanizeMCPToolName, parseArgs } from "./utils";
|
||||
|
||||
export const ToolLabel: React.FC<{
|
||||
type ToolLabelProps = {
|
||||
name: string;
|
||||
args: unknown;
|
||||
result: unknown;
|
||||
mcpSlug?: string;
|
||||
}> = ({ name, args, result, mcpSlug }) => {
|
||||
};
|
||||
|
||||
const ProcessSignalLabel: React.FC<ToolLabelProps> = ({ args, result }) => {
|
||||
const parsed = parseArgs(args);
|
||||
const parsedResult = asRecord(result);
|
||||
const signal = parsed ? asString(parsed.signal) : "";
|
||||
const processId = parsed ? asString(parsed.process_id) : "";
|
||||
const shortId = processId ? processId.slice(0, 8) : "";
|
||||
const suffix = shortId ? ` ${shortId}` : "";
|
||||
const isKill = signal === "kill";
|
||||
const isTerminate = signal === "terminate";
|
||||
|
||||
switch (name) {
|
||||
case "process_signal": {
|
||||
const signal = parsed ? asString(parsed.signal) : "";
|
||||
const processId = parsed ? asString(parsed.process_id) : "";
|
||||
const shortId = processId ? processId.slice(0, 8) : "";
|
||||
const hasResult = result !== undefined && result !== null;
|
||||
const success = parsedResult ? Boolean(parsedResult.success) : false;
|
||||
if (hasResult && success) {
|
||||
const verb = signal === "kill" ? "Killed" : "Terminated";
|
||||
return (
|
||||
<span className="truncate text-[13px]">
|
||||
{verb} process{shortId ? ` ${shortId}` : ""}
|
||||
</span>
|
||||
);
|
||||
}
|
||||
if (hasResult && !success) {
|
||||
const verb =
|
||||
signal === "kill"
|
||||
? "kill"
|
||||
: signal === "terminate"
|
||||
? "terminate"
|
||||
: "signal";
|
||||
return (
|
||||
<span className="truncate text-[13px]">
|
||||
Failed to {verb} process{shortId ? ` ${shortId}` : ""}
|
||||
</span>
|
||||
);
|
||||
}
|
||||
return (
|
||||
<span className="truncate text-[13px]">
|
||||
{signal === "kill"
|
||||
? "Killing process…"
|
||||
: signal === "terminate"
|
||||
? "Terminating process…"
|
||||
: "Sending signal…"}
|
||||
</span>
|
||||
);
|
||||
}
|
||||
case "process_list":
|
||||
return <span className="truncate text-[13px]">Listing processes</span>;
|
||||
case "attach_file": {
|
||||
const attachedName =
|
||||
(parsedResult ? asString(parsedResult.name) : "") ||
|
||||
(parsed ? asString(parsed.name) : "") ||
|
||||
(parsed ? getPathBasename(asString(parsed.path)) : "") ||
|
||||
"file";
|
||||
return (
|
||||
<span className="truncate text-[13px]">{`Attached ${attachedName}`}</span>
|
||||
);
|
||||
}
|
||||
case "advisor":
|
||||
return (
|
||||
<span className="truncate text-[13px] leading-4 text-content-secondary">
|
||||
Advisor
|
||||
</span>
|
||||
);
|
||||
|
||||
default: {
|
||||
const displayName = mcpSlug ? humanizeMCPToolName(mcpSlug, name) : name;
|
||||
return <span className="truncate text-[13px]">{displayName}</span>;
|
||||
}
|
||||
const hasResult = result !== undefined && result !== null;
|
||||
if (!hasResult) {
|
||||
const inFlightVerb = isKill
|
||||
? "Killing process…"
|
||||
: isTerminate
|
||||
? "Terminating process…"
|
||||
: "Sending signal…";
|
||||
return <span className="truncate text-[13px]">{inFlightVerb}</span>;
|
||||
}
|
||||
|
||||
const success = parsedResult ? Boolean(parsedResult.success) : false;
|
||||
if (success) {
|
||||
const verb = isKill ? "Killed" : "Terminated";
|
||||
return (
|
||||
<span className="truncate text-[13px]">
|
||||
{verb} process{suffix}
|
||||
</span>
|
||||
);
|
||||
}
|
||||
|
||||
const failedVerb = isKill ? "kill" : isTerminate ? "terminate" : "signal";
|
||||
return (
|
||||
<span className="truncate text-[13px]">
|
||||
Failed to {failedVerb} process{suffix}
|
||||
</span>
|
||||
);
|
||||
};
|
||||
|
||||
const AttachFileLabel: React.FC<ToolLabelProps> = ({ args, result }) => {
|
||||
const parsed = parseArgs(args);
|
||||
const parsedResult = asRecord(result);
|
||||
const resultName = parsedResult ? asString(parsedResult.name) : "";
|
||||
const argName = parsed ? asString(parsed.name) : "";
|
||||
const argPath = parsed ? asString(parsed.path) : "";
|
||||
const attachedName =
|
||||
resultName || argName || getPathBasename(argPath) || "file";
|
||||
return (
|
||||
<span className="truncate text-[13px]">{`Attached ${attachedName}`}</span>
|
||||
);
|
||||
};
|
||||
|
||||
export const genericToolLabels: Partial<
|
||||
Record<string, React.FC<ToolLabelProps>>
|
||||
> = {
|
||||
process_signal: ProcessSignalLabel,
|
||||
process_list: () => (
|
||||
<span className="truncate text-[13px]">Listing processes</span>
|
||||
),
|
||||
attach_file: AttachFileLabel,
|
||||
advisor: () => (
|
||||
<span className="truncate text-[13px] leading-4 text-content-secondary">
|
||||
Advisor
|
||||
</span>
|
||||
),
|
||||
};
|
||||
|
||||
export const ToolLabel: React.FC<ToolLabelProps> = (props) => {
|
||||
const Label = genericToolLabels[props.name];
|
||||
if (Label) {
|
||||
return <Label {...props} />;
|
||||
}
|
||||
const displayName = props.mcpSlug
|
||||
? humanizeMCPToolName(props.mcpSlug, props.name)
|
||||
: props.name;
|
||||
return <span className="truncate text-[13px]">{displayName}</span>;
|
||||
};
|
||||
|
||||
@@ -0,0 +1,18 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { toolRenderers } from "./Tool";
|
||||
import { toolIcons } from "./ToolIcon";
|
||||
|
||||
// ReadSkillFileRenderer renders ReadSkillTool, which passes the
|
||||
// hardcoded iconName "read_skill" for read_skill_file calls.
|
||||
const aliasedIconNames = new Set(["read_skill_file"]);
|
||||
|
||||
describe("toolIcons coverage", () => {
|
||||
// A registered tool name without a table entry silently degrades to
|
||||
// WrenchIcon, unless its renderer passes an aliased fixed iconName.
|
||||
it("resolves an icon for every registered renderer", () => {
|
||||
const missing = Object.keys(toolRenderers).filter(
|
||||
(name) => !aliasedIconNames.has(name) && toolIcons[name] === undefined,
|
||||
);
|
||||
expect(missing).toEqual([]);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,26 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { toolRenderers } from "./Tool";
|
||||
import { genericToolLabels } from "./ToolLabel";
|
||||
|
||||
// process_signal's renderer delegates to GenericToolRenderer.
|
||||
const genericDelegatingToolNames = new Set(["process_signal"]);
|
||||
|
||||
// advisor's registered renderer does not delegate; AdvisorTool renders
|
||||
// ToolLabel directly with a hardcoded name instead.
|
||||
const directToolLabelConsumers = new Set(["advisor"]);
|
||||
|
||||
// genericToolLabels holds the label for every tool name that can reach
|
||||
// GenericToolRenderer. A name with a registered renderer only reaches it
|
||||
// when that renderer delegates, so a name that is registered but does not
|
||||
// delegate can never use its genericToolLabels entry.
|
||||
describe("genericToolLabels visibility", () => {
|
||||
it("contains no arm shadowed by a non-delegating registered renderer", () => {
|
||||
const shadowed = Object.keys(genericToolLabels).filter(
|
||||
(name) =>
|
||||
!genericDelegatingToolNames.has(name) &&
|
||||
!directToolLabelConsumers.has(name) &&
|
||||
toolRenderers[name] !== undefined,
|
||||
);
|
||||
expect(shadowed).toEqual([]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user