From 66f809388eb861bf9865bb0304c4415c0e242ad5 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Wed, 18 Mar 2026 11:27:51 +0200 Subject: [PATCH] refactor: make ChatMessagePart a discriminated union in TypeScript (#23168) The flat ChatMessagePart interface had 20+ optional fields, preventing TypeScript from narrowing types on switch(part.type). Each consumer needed runtime validation, type assertions, or defensive ?. chains. Add `variants` struct tags to ChatMessagePart fields declaring which union variants include each field. A codegen mutation in apitypings reads these tags via reflect and generates per-variant sub-interfaces (ChatTextPart, ChatReasoningPart, etc.) plus a union type alias. A test validates every field has a variants tag or is explicitly excluded, and every part type is covered. Remove dead frontend code: normalizeBlockType, alias case branches ("thinking", "toolcall", "toolresult"), legacy field fallbacks (line_number, typedBlock.name/id/input/output), and result_delta handling. Add test coverage for args_delta streaming, provider_executed skip logic, and source part parsing. --- codersdk/chats.go | 57 +++--- codersdk/chats_test.go | 81 +++++++++ scripts/apitypings/main.go | 167 ++++++++++++++++++ site/src/api/typesGenerated.ts | 116 ++++++++---- .../pages/AgentsPage/AgentDetail.stories.tsx | 16 +- .../ConversationTimeline.stories.tsx | 2 +- .../AgentDetail/messageParsing.test.ts | 120 +++++++------ .../AgentsPage/AgentDetail/messageParsing.ts | 43 ++--- .../AgentDetail/streamState.test.ts | 139 ++++++++++++--- .../AgentsPage/AgentDetail/streamState.ts | 14 +- .../pages/AgentsPage/AgentDetailContent.tsx | 9 +- 11 files changed, 568 insertions(+), 196 deletions(-) diff --git a/codersdk/chats.go b/codersdk/chats.go index ce228bb492..3255cf3d05 100644 --- a/codersdk/chats.go +++ b/codersdk/chats.go @@ -98,6 +98,19 @@ const ( ChatMessagePartTypeFileReference ChatMessagePartType = "file-reference" ) +// AllChatMessagePartTypes returns all known ChatMessagePartType values. +func AllChatMessagePartTypes() []ChatMessagePartType { + return []ChatMessagePartType{ + ChatMessagePartTypeText, + ChatMessagePartTypeReasoning, + ChatMessagePartTypeToolCall, + ChatMessagePartTypeToolResult, + ChatMessagePartTypeSource, + ChatMessagePartTypeFile, + ChatMessagePartTypeFileReference, + } +} + // ChatMessagePart is a structured chunk of a chat message. // // WARNING: This type is both an API wire type and a database @@ -106,37 +119,41 @@ const ( // changes, and omitempty behavior all affect backward-compatible // deserialization of stored rows. Treat changes to this struct // with the same care as a database migration. +// +// The variants struct tag declares which discriminated-union +// variants include each field in the generated TypeScript. Bare +// name = required, ? suffix = optional. Fields without a variants +// tag are excluded from the generated union. See +// scripts/apitypings/main.go for the codegen that reads these. type ChatMessagePart struct { Type ChatMessagePartType `json:"type"` - Text string `json:"text,omitempty"` + Text string `json:"text,omitempty" variants:"text,reasoning"` Signature string `json:"signature,omitempty"` - ToolCallID string `json:"tool_call_id,omitempty"` - ToolName string `json:"tool_name,omitempty"` - Args json.RawMessage `json:"args,omitempty"` - ArgsDelta string `json:"args_delta,omitempty"` - Result json.RawMessage `json:"result,omitempty"` + ToolCallID string `json:"tool_call_id,omitempty" variants:"tool-call,tool-result"` + ToolName string `json:"tool_name,omitempty" variants:"tool-call,tool-result"` + Args json.RawMessage `json:"args,omitempty" variants:"tool-call?"` + ArgsDelta string `json:"args_delta,omitempty" variants:"tool-call?"` + Result json.RawMessage `json:"result,omitempty" variants:"tool-result?"` ResultDelta string `json:"result_delta,omitempty"` - IsError bool `json:"is_error,omitempty"` - SourceID string `json:"source_id,omitempty"` - URL string `json:"url,omitempty"` - Title string `json:"title,omitempty"` - MediaType string `json:"media_type,omitempty"` - Data []byte `json:"data,omitempty"` - FileID uuid.NullUUID `json:"file_id,omitempty" format:"uuid"` - // The following fields are only set when Type is - // ChatInputPartTypeFileReference. - FileName string `json:"file_name,omitempty"` - StartLine int `json:"start_line,omitempty"` - EndLine int `json:"end_line,omitempty"` + IsError bool `json:"is_error,omitempty" variants:"tool-result?"` + SourceID string `json:"source_id,omitempty" variants:"source?"` + URL string `json:"url,omitempty" variants:"source"` + Title string `json:"title,omitempty" variants:"source?"` + MediaType string `json:"media_type,omitempty" variants:"file"` + Data []byte `json:"data,omitempty" variants:"file?"` + FileID uuid.NullUUID `json:"file_id,omitempty" format:"uuid" variants:"file?"` + FileName string `json:"file_name,omitempty" variants:"file-reference"` + StartLine int `json:"start_line,omitempty" variants:"file-reference"` + EndLine int `json:"end_line,omitempty" variants:"file-reference"` // The code content from the diff that was commented on. - Content string `json:"content,omitempty"` + Content string `json:"content,omitempty" variants:"file-reference"` // ProviderMetadata holds provider-specific response metadata // (e.g. Anthropic cache control hints) as raw JSON. Internal // only: stripped by db2sdk before API responses. ProviderMetadata json.RawMessage `json:"provider_metadata,omitempty" typescript:"-"` // ProviderExecuted indicates the tool call was executed by // the provider (e.g. Anthropic computer use). - ProviderExecuted bool `json:"provider_executed,omitempty"` + ProviderExecuted bool `json:"provider_executed,omitempty" variants:"tool-call?,tool-result?"` } // StripInternal removes internal-only fields that must not be diff --git a/codersdk/chats_test.go b/codersdk/chats_test.go index b2494562ce..b2c3246bdf 100644 --- a/codersdk/chats_test.go +++ b/codersdk/chats_test.go @@ -6,6 +6,8 @@ import ( "net/http" "net/http/httptest" "net/url" + "reflect" + "strings" "testing" "time" @@ -191,6 +193,85 @@ func TestChatMessagePart_StripInternal(t *testing.T) { }) } +// TestChatMessagePartVariantTags validates the `variants` struct tags +// on ChatMessagePart fields. Every field must either declare variant +// membership or be explicitly excluded, and every known part type +// must appear in at least one tag. +// +// If this test fails, edit the variants struct tags on ChatMessagePart +// in codersdk/chats.go. +func TestChatMessagePartVariantTags(t *testing.T) { + t.Parallel() + + const editHint = "edit the variants struct tags on ChatMessagePart in codersdk/chats.go" + + // Fields intentionally excluded from all generated variants. + // If you add a new field to ChatMessagePart, either add a + // variants tag or add it here with a comment explaining why. + excludedFields := map[string]string{ + "type": "discriminant, added automatically by codegen", + "signature": "added in #22290, never populated by any code path", + "result_delta": "added in #22290, never populated by any code path", + "provider_metadata": "internal only, stripped by db2sdk before API responses", + } + + knownTypes := make(map[codersdk.ChatMessagePartType]bool) + for _, pt := range codersdk.AllChatMessagePartTypes() { + knownTypes[pt] = true + } + + // Parse all variants tags from the struct and validate them. + typ := reflect.TypeOf(codersdk.ChatMessagePart{}) + coveredTypes := make(map[codersdk.ChatMessagePartType]bool) + hasRequired := make(map[codersdk.ChatMessagePartType]bool) + + for i := range typ.NumField() { + f := typ.Field(i) + jsonTag := f.Tag.Get("json") + if jsonTag == "" || jsonTag == "-" { + continue + } + jsonName, _, _ := strings.Cut(jsonTag, ",") + + varTag := f.Tag.Get("variants") + if varTag == "" { + assert.Contains(t, excludedFields, jsonName, + "field %s (json:%q) has no variants tag and is not in excludedFields; %s", + f.Name, jsonName, editHint) + continue + } + + assert.NotEqual(t, "type", jsonName, + "the discriminant field must not have a variants tag; %s", editHint) + + for _, entry := range strings.Split(varTag, ",") { + isOptional := strings.HasSuffix(entry, "?") + typeLit := codersdk.ChatMessagePartType(strings.TrimSuffix(entry, "?")) + + assert.True(t, knownTypes[typeLit], + "field %s variants tag references unknown type %q; %s", + f.Name, typeLit, editHint) + + coveredTypes[typeLit] = true + if !isOptional { + hasRequired[typeLit] = true + } + } + } + + // Every known type must appear in at least one variants tag. + for pt := range knownTypes { + assert.True(t, coveredTypes[pt], + "ChatMessagePartType %q is not referenced by any variants tag; %s", pt, editHint) + } + + // Every variant must have at least one required field. + for pt := range coveredTypes { + assert.True(t, hasRequired[pt], + "variant %q has no required fields (all have ? suffix); %s", pt, editHint) + } +} + func TestModelCostConfig_LegacyNumericJSON(t *testing.T) { t.Parallel() diff --git a/scripts/apitypings/main.go b/scripts/apitypings/main.go index 599f2b00bf..77c648a050 100644 --- a/scripts/apitypings/main.go +++ b/scripts/apitypings/main.go @@ -3,9 +3,12 @@ package main import ( "fmt" "log" + "reflect" + "strings" "golang.org/x/xerrors" + "github.com/coder/coder/v2/codersdk" "github.com/coder/guts" "github.com/coder/guts/bindings" "github.com/coder/guts/config" @@ -74,6 +77,7 @@ func TSMutations(ts *guts.Typescript) { // of referencing maps that are actually null. config.NotNullMaps, FixSerpentStruct, + DiscriminatedChatMessagePart, // Prefer enums as types config.EnumAsTypes, // Enum list generator @@ -142,6 +146,169 @@ func TypeMappings(gen *guts.GoParser) error { return nil } +// DiscriminatedChatMessagePart splits the flat ChatMessagePart +// interface into a discriminated union of per-type sub-interfaces. +// Each sub-interface narrows the `type` field to a string literal +// and includes only the fields relevant to that part type. +// +// Variant membership is declared via `variants` struct tags on +// ChatMessagePart fields in codersdk/chats.go. This function +// reads those tags via reflect and builds the union from them. +func DiscriminatedChatMessagePart(ts *guts.Typescript) { + node, ok := ts.Node("ChatMessagePart") + if !ok { + return + } + iface, ok := node.(*bindings.Interface) + if !ok { + return + } + + // Build a lookup from field name to its PropertySignature so + // we can copy type information from the original interface. + fieldMap := make(map[string]*bindings.PropertySignature, len(iface.Fields)) + for _, f := range iface.Fields { + fieldMap[f.Name] = f + } + + // copyField copies a field from the original interface into a + // sub-interface, setting QuestionToken based on whether the + // field is required for that variant. + copyField := func(name string, required bool) *bindings.PropertySignature { + orig, exists := fieldMap[name] + if !exists { + return nil + } + return &bindings.PropertySignature{ + Name: orig.Name, + Modifiers: orig.Modifiers, + QuestionToken: !required, + Type: orig.Type, + SupportComments: orig.SupportComments, + } + } + + variants := parseVariantTags() + unionMembers := make([]bindings.ExpressionType, 0, len(variants)) + + for _, v := range variants { + fields := make([]*bindings.PropertySignature, 0, 1+len(v.required)+len(v.optional)) + + // Discriminant field: type narrowed to a string literal. + fields = append(fields, &bindings.PropertySignature{ + Name: "type", + Type: &bindings.LiteralType{Value: string(v.typeLiteral)}, + }) + + for _, name := range v.required { + if f := copyField(name, true); f != nil { + fields = append(fields, f) + } + } + for _, name := range v.optional { + if f := copyField(name, false); f != nil { + fields = append(fields, f) + } + } + + tsName := chatMessagePartTSName(v.typeLiteral) + subIface := &bindings.Interface{ + Name: bindings.Identifier{ + Name: tsName, + Package: iface.Name.Package, + Prefix: iface.Name.Prefix, + }, + Fields: fields, + Source: iface.Source, + } + + // Inject the sub-interface as a new top-level type. + if err := ts.SetNode(tsName, subIface); err != nil { + panic(fmt.Sprintf("ChatMessagePart variant %q: %v", v.typeLiteral, err)) + } + + unionMembers = append(unionMembers, bindings.Reference(bindings.Identifier{ + Name: tsName, + Package: iface.Name.Package, + Prefix: iface.Name.Prefix, + })) + } + + // Replace the original flat interface with a union alias. + ts.ReplaceNode("ChatMessagePart", &bindings.Alias{ + Name: iface.Name, + Modifiers: iface.Modifiers, + Type: bindings.Union(unionMembers...), + SupportComments: iface.SupportComments, + Source: iface.Source, + }) +} + +// chatPartVariant holds the parsed variant info for one part type. +type chatPartVariant struct { + typeLiteral codersdk.ChatMessagePartType + required []string // JSON field names + optional []string // JSON field names +} + +// parseVariantTags reads `variants` struct tags from ChatMessagePart +// and returns the per-type field sets using JSON tag names. Variants +// are returned in AllChatMessagePartTypes order for stable codegen. +func parseVariantTags() []chatPartVariant { + t := reflect.TypeFor[codersdk.ChatMessagePart]() + + type fieldSets struct { + required []string + optional []string + } + byType := make(map[codersdk.ChatMessagePartType]*fieldSets) + + for i := range t.NumField() { + f := t.Field(i) + varTag := f.Tag.Get("variants") + if varTag == "" { + continue + } + jsonName, _, _ := strings.Cut(f.Tag.Get("json"), ",") + for entry := range strings.SplitSeq(varTag, ",") { + isOptional := strings.HasSuffix(entry, "?") + typeLit := codersdk.ChatMessagePartType(strings.TrimSuffix(entry, "?")) + if byType[typeLit] == nil { + byType[typeLit] = &fieldSets{} + } + if isOptional { + byType[typeLit].optional = append(byType[typeLit].optional, jsonName) + } else { + byType[typeLit].required = append(byType[typeLit].required, jsonName) + } + } + } + + result := make([]chatPartVariant, 0, len(byType)) + for _, pt := range codersdk.AllChatMessagePartTypes() { + if fs, ok := byType[pt]; ok { + result = append(result, chatPartVariant{ + typeLiteral: pt, + required: fs.required, + optional: fs.optional, + }) + } + } + return result +} + +// chatMessagePartTSName derives a TypeScript interface name from +// a ChatMessagePartType literal. "tool-call" → "ChatToolCallPart". +func chatMessagePartTSName(t codersdk.ChatMessagePartType) string { + words := strings.Split(string(t), "-") + for i, w := range words { + if len(w) > 0 { + words[i] = strings.ToUpper(w[:1]) + w[1:] + } + } + return "Chat" + strings.Join(words, "") + "Part" +} + // FixSerpentStruct fixes 'serpent.Struct'. // 'serpent.Struct' overrides the json.Marshal to use the underlying type, // so the typescript type should be the underlying type. diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 03f729cacf..bdc2e128b6 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1219,6 +1219,26 @@ export interface ChatDiffStatus { readonly stale_at?: string; } +// From codersdk/chats.go +export interface ChatFilePart { + readonly type: "file"; + readonly media_type: string; + readonly data?: string; + readonly file_id?: string; +} + +// From codersdk/chats.go +export interface ChatFileReferencePart { + readonly type: "file-reference"; + readonly file_name: string; + readonly start_line: number; + readonly end_line: number; + /** + * The code content from the diff that was commented on. + */ + readonly content: string; +} + // From codersdk/chats.go /** * ChatGitChange represents a git file change detected during a chat session. @@ -1288,41 +1308,21 @@ export interface ChatMessage { * changes, and omitempty behavior all affect backward-compatible * deserialization of stored rows. Treat changes to this struct * with the same care as a database migration. + * + * The variants struct tag declares which discriminated-union + * variants include each field in the generated TypeScript. Bare + * name = required, ? suffix = optional. Fields without a variants + * tag are excluded from the generated union. See + * scripts/apitypings/main.go for the codegen that reads these. */ -export interface ChatMessagePart { - readonly type: ChatMessagePartType; - readonly text?: string; - readonly signature?: string; - readonly tool_call_id?: string; - readonly tool_name?: string; - readonly args?: Record; - readonly args_delta?: string; - readonly result?: Record; - readonly result_delta?: string; - readonly is_error?: boolean; - readonly source_id?: string; - readonly url?: string; - readonly title?: string; - readonly media_type?: string; - readonly data?: string; - readonly file_id?: string; - /** - * The following fields are only set when Type is - * ChatInputPartTypeFileReference. - */ - readonly file_name?: string; - readonly start_line?: number; - readonly end_line?: number; - /** - * The code content from the diff that was commented on. - */ - readonly content?: string; - /** - * ProviderExecuted indicates the tool call was executed by - * the provider (e.g. Anthropic computer use). - */ - readonly provider_executed?: boolean; -} +export type ChatMessagePart = + | ChatTextPart + | ChatReasoningPart + | ChatToolCallPart + | ChatToolResultPart + | ChatSourcePart + | ChatFilePart + | ChatFileReferencePart; // From codersdk/chats.go export type ChatMessagePartType = @@ -1673,6 +1673,20 @@ export interface ChatQueuedMessage { readonly created_at: string; } +// From codersdk/chats.go +export interface ChatReasoningPart { + readonly type: "reasoning"; + readonly text: string; +} + +// From codersdk/chats.go +export interface ChatSourcePart { + readonly type: "source"; + readonly url: string; + readonly source_id?: string; + readonly title?: string; +} + // From codersdk/chats.go export type ChatStatus = | "completed" @@ -1782,6 +1796,40 @@ export interface ChatSystemPrompt { readonly system_prompt: string; } +// From codersdk/chats.go +export interface ChatTextPart { + readonly type: "text"; + readonly text: string; +} + +// From codersdk/chats.go +export interface ChatToolCallPart { + readonly type: "tool-call"; + readonly tool_call_id: string; + readonly tool_name: string; + readonly args?: Record; + readonly args_delta?: string; + /** + * ProviderExecuted indicates the tool call was executed by + * the provider (e.g. Anthropic computer use). + */ + readonly provider_executed?: boolean; +} + +// From codersdk/chats.go +export interface ChatToolResultPart { + readonly type: "tool-result"; + readonly tool_call_id: string; + readonly tool_name: string; + readonly result?: Record; + readonly is_error?: boolean; + /** + * ProviderExecuted indicates the tool call was executed by + * the provider (e.g. Anthropic computer use). + */ + readonly provider_executed?: boolean; +} + // From codersdk/chats.go /** * ChatUsageLimitConfig is the deployment-wide default usage limit config. diff --git a/site/src/pages/AgentsPage/AgentDetail.stories.tsx b/site/src/pages/AgentsPage/AgentDetail.stories.tsx index d4d77f641f..d9589fb36e 100644 --- a/site/src/pages/AgentsPage/AgentDetail.stories.tsx +++ b/site/src/pages/AgentsPage/AgentDetail.stories.tsx @@ -653,8 +653,8 @@ export const WithSubagentCards: Story = { }, }; -/** Reasoning part renders collapsed and can be expanded on click. */ -export const WithReasoningCollapsed: Story = { +/** Reasoning part without title renders inline (no disclosure). */ +export const WithReasoningInline: Story = { parameters: { queries: buildQueries( { @@ -673,7 +673,6 @@ export const WithReasoningCollapsed: Story = { content: [ { type: "reasoning", - title: "Plan migration", text: "Reasoning body", }, ], @@ -687,17 +686,10 @@ export const WithReasoningCollapsed: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - const user = userEvent.setup(); - const reasoningToggle = await canvas.findByRole("button", { - name: "Plan migration", - }); - expect(reasoningToggle).toHaveAttribute("aria-expanded", "false"); - - await user.click(reasoningToggle); - - expect(reasoningToggle).toHaveAttribute("aria-expanded", "true"); + // Reasoning text renders inline, not behind a disclosure. expect(canvas.getByText("Reasoning body")).toBeInTheDocument(); + expect(canvas.queryByRole("button", { name: "Thinking" })).toBeNull(); }, }; diff --git a/site/src/pages/AgentsPage/AgentDetail/ConversationTimeline.stories.tsx b/site/src/pages/AgentsPage/AgentDetail/ConversationTimeline.stories.tsx index 0aa4a2bcb9..bfdb29f588 100644 --- a/site/src/pages/AgentsPage/AgentDetail/ConversationTimeline.stories.tsx +++ b/site/src/pages/AgentsPage/AgentDetail/ConversationTimeline.stories.tsx @@ -225,7 +225,7 @@ export const UserMessageWithImagesAndFileRefs: Story = { file_name: "src/main.go", start_line: 10, end_line: 25, - text: "main function", + content: 'func main() {\n\tfmt.Println("hello")\n}', }, ], }, diff --git a/site/src/pages/AgentsPage/AgentDetail/messageParsing.test.ts b/site/src/pages/AgentsPage/AgentDetail/messageParsing.test.ts index 0a5023bb07..cfdf46483c 100644 --- a/site/src/pages/AgentsPage/AgentDetail/messageParsing.test.ts +++ b/site/src/pages/AgentsPage/AgentDetail/messageParsing.test.ts @@ -1,23 +1,10 @@ import { describe, expect, it } from "vitest"; import { mergeTools, - normalizeBlockType, parseMessageContent, parseToolResultIsError, } from "./messageParsing"; -describe("normalizeBlockType", () => { - it("lowercases and replaces underscores with hyphens", () => { - expect(normalizeBlockType("Tool_Call")).toBe("tool-call"); - expect(normalizeBlockType("TOOL_RESULT")).toBe("tool-result"); - }); - - it("returns empty string for non-string input", () => { - expect(normalizeBlockType(undefined)).toBe(""); - expect(normalizeBlockType(null)).toBe(""); - }); -}); - describe("parseToolResultIsError", () => { it("returns the boolean is_error when present", () => { expect(parseToolResultIsError("tool", { is_error: true }, null)).toBe(true); @@ -126,9 +113,9 @@ describe("parseMessageContent", () => { }); }); - it("parses a thinking block", () => { + it("parses a reasoning block", () => { const result = parseMessageContent([ - { type: "thinking", text: "Let me think...", title: "Reasoning" }, + { type: "reasoning", text: "Let me think...", title: "Reasoning" }, ]); expect(result.reasoning).toBe("Let me think..."); expect(result.blocks).toEqual([ @@ -234,19 +221,6 @@ describe("parseMessageContent", () => { expect(result.markdown).toBe("fallback text"); }); - it("normalizes underscore block types like tool_call", () => { - const result = parseMessageContent([ - { - type: "tool_call", - tool_name: "test", - tool_call_id: "tc-1", - args: {}, - }, - ]); - expect(result.toolCalls).toHaveLength(1); - expect(result.toolCalls[0].name).toBe("test"); - }); - it("extracts fileId from a file block with file_id", () => { const result = parseMessageContent([ { @@ -303,39 +277,6 @@ describe("parseMessageContent", () => { }); }); - it("falls back to line_number when start_line and end_line are missing", () => { - const result = parseMessageContent([ - { - type: "file-reference", - file_name: "index.ts", - line_number: 42, - content: "fallback content", - text: "Fallback line.", - }, - ]); - const ref = result.blocks[0] as { startLine: number; endLine: number }; - expect(ref.startLine).toBe(42); - expect(ref.endLine).toBe(42); - }); - - it("uses line_number for end_line when only start_line is provided", () => { - // When start_line is present it is used directly. end_line is - // missing so the fallback chain tries line_number next. - const result = parseMessageContent([ - { - type: "file-reference", - file_name: "foo.ts", - start_line: 5, - line_number: 7, - content: "partial content", - text: "Partial fallback.", - }, - ]); - const ref = result.blocks[0] as { startLine: number; endLine: number }; - expect(ref.startLine).toBe(5); - expect(ref.endLine).toBe(7); - }); - it("defaults lines to 0 when no line fields are provided", () => { const result = parseMessageContent([ { @@ -374,6 +315,63 @@ describe("parseMessageContent", () => { text: "Nit.", }); }); + + it("skips provider_executed tool-call parts", () => { + const result = parseMessageContent([ + { + type: "tool-call", + tool_name: "web_search", + tool_call_id: "tc-1", + provider_executed: true, + }, + ]); + expect(result.toolCalls).toEqual([]); + expect(result.blocks.some((b) => b.type === "tool")).toBe(false); + }); + + it("skips provider_executed tool-result parts", () => { + const result = parseMessageContent([ + { + type: "tool-result", + tool_name: "web_search", + tool_call_id: "tc-1", + provider_executed: true, + result: { output: "results" }, + }, + ]); + expect(result.toolResults).toEqual([]); + expect(result.blocks.some((b) => b.type === "tool")).toBe(false); + }); + + it("parses a source part into a sources block", () => { + const result = parseMessageContent([ + { type: "source", url: "https://example.com", title: "Example" }, + ]); + expect(result.blocks).toHaveLength(1); + expect(result.blocks[0]).toEqual({ + type: "sources", + sources: [{ url: "https://example.com", title: "Example" }], + }); + expect(result.sources).toEqual([ + { url: "https://example.com", title: "Example" }, + ]); + }); + + it("groups multiple consecutive sources into a single sources block", () => { + const result = parseMessageContent([ + { type: "source", url: "https://example.com", title: "Example" }, + { type: "source", url: "https://other.com", title: "Other" }, + ]); + expect(result.blocks).toHaveLength(1); + expect(result.blocks[0]).toEqual({ + type: "sources", + sources: [ + { url: "https://example.com", title: "Example" }, + { url: "https://other.com", title: "Other" }, + ], + }); + expect(result.sources).toHaveLength(2); + }); }); describe("mergeTools", () => { diff --git a/site/src/pages/AgentsPage/AgentDetail/messageParsing.ts b/site/src/pages/AgentsPage/AgentDetail/messageParsing.ts index 9da048652c..0ea8e00df9 100644 --- a/site/src/pages/AgentsPage/AgentDetail/messageParsing.ts +++ b/site/src/pages/AgentsPage/AgentDetail/messageParsing.ts @@ -21,9 +21,6 @@ const appendText = (current: string, next: string): string => { export const asOptionalTitle = (value: unknown): string | undefined => asNonEmptyString(value); -export const normalizeBlockType = (value: unknown): string => - asString(value).toLowerCase().replace(/_/g, "-"); - const isSubagentToolName = (name: string): boolean => name === "spawn_agent" || name === "wait_agent" || name === "message_agent"; @@ -152,7 +149,7 @@ export const parseMessageContent = (content: unknown): ParsedMessageContent => { continue; } - switch (normalizeBlockType(typedBlock.type)) { + switch (asString(typedBlock.type)) { case "text": { const text = asString(typedBlock.text); parsed.markdown = appendText(parsed.markdown, text); @@ -163,8 +160,7 @@ export const parseMessageContent = (content: unknown): ParsedMessageContent => { ); break; } - case "reasoning": - case "thinking": { + case "reasoning": { const text = asString(typedBlock.text); const title = asOptionalTitle(typedBlock.title); parsed.reasoning = appendText(parsed.reasoning, text); @@ -176,8 +172,7 @@ export const parseMessageContent = (content: unknown): ParsedMessageContent => { ); break; } - case "tool-call": - case "toolcall": { + case "tool-call": { // Provider-executed tool calls (e.g. web_search) are // handled by the provider itself — hide them from the // tool card UI and let the sources component render @@ -185,16 +180,12 @@ export const parseMessageContent = (content: unknown): ParsedMessageContent => { if (typedBlock.provider_executed) { break; } - const name = - asString(typedBlock.tool_name) || asString(typedBlock.name); - const id = - asString(typedBlock.tool_call_id) || - asString(typedBlock.id) || - `tool-call-${index}`; + const name = asString(typedBlock.tool_name); + const id = asString(typedBlock.tool_call_id) || `tool-call-${index}`; parsed.toolCalls.push({ id, name: name || "Tool", - args: typedBlock.args ?? typedBlock.input ?? typedBlock.arguments, + args: typedBlock.args, }); parsed.blocks = ensureToolBlock(parsed.blocks, id); break; @@ -202,10 +193,8 @@ export const parseMessageContent = (content: unknown): ParsedMessageContent => { case "file-reference": { const text = asString(typedBlock.text); const fileName = asString(typedBlock.file_name); - const startLine = - Number(typedBlock.start_line ?? typedBlock.line_number) || 0; - const endLine = - Number(typedBlock.end_line ?? typedBlock.line_number) || startLine; + const startLine = Number(typedBlock.start_line) || 0; + const endLine = Number(typedBlock.end_line) || startLine; const contentStr = asString(typedBlock.content); parsed.blocks.push({ type: "file-reference", @@ -217,23 +206,15 @@ export const parseMessageContent = (content: unknown): ParsedMessageContent => { }); break; } - case "tool-result": - case "toolresult": { + case "tool-result": { // Skip synthetic results for provider-executed tools. if (typedBlock.provider_executed) { break; } - const name = - asString(typedBlock.tool_name) || asString(typedBlock.name); + const name = asString(typedBlock.tool_name); const id = - asString(typedBlock.tool_call_id) || - asString(typedBlock.id) || - `tool-result-${index}`; - const result = - typedBlock.result ?? - typedBlock.output ?? - typedBlock.content ?? - typedBlock.data; + asString(typedBlock.tool_call_id) || `tool-result-${index}`; + const result = typedBlock.result; parsed.toolResults.push({ id, name: name || "Tool", diff --git a/site/src/pages/AgentsPage/AgentDetail/streamState.test.ts b/site/src/pages/AgentsPage/AgentDetail/streamState.test.ts index fd3bf59268..387948d324 100644 --- a/site/src/pages/AgentsPage/AgentDetail/streamState.test.ts +++ b/site/src/pages/AgentsPage/AgentDetail/streamState.test.ts @@ -52,9 +52,9 @@ describe("applyMessagePartToStreamState", () => { expect(result).toBe(prev); }); - it("creates thinking block from thinking part", () => { + it("creates thinking block from reasoning part", () => { const result = applyMessagePartToStreamState(null, { - type: "thinking", + type: "reasoning", text: "Let me reason...", title: "Analysis", }); @@ -64,28 +64,19 @@ describe("applyMessagePartToStreamState", () => { ]); }); - it("handles reasoning type alias the same as thinking", () => { - const result = applyMessagePartToStreamState(null, { - type: "reasoning", - text: "hmm", - }); - expect(result).not.toBeNull(); - expect(result!.blocks[0].type).toBe("thinking"); - }); - - it("returns prev for thinking part with no text and no title", () => { + it("returns prev for reasoning part with no text and no title", () => { const prev = createEmptyStreamState(); const result = applyMessagePartToStreamState(prev, { - type: "thinking", + type: "reasoning", text: "", }); expect(result).toBe(prev); }); - it("returns prev for thinking part with only title and no text", () => { + it("returns prev for reasoning part with only title and no text", () => { const prev = createEmptyStreamState(); const result = applyMessagePartToStreamState(prev, { - type: "thinking", + type: "reasoning", text: "", title: "Some Title", }); @@ -199,16 +190,6 @@ describe("applyMessagePartToStreamState", () => { }); }); - it("handles tool_call underscore type alias", () => { - const result = applyMessagePartToStreamState(null, { - type: "tool_call", - tool_name: "test", - tool_call_id: "t1", - }); - expect(result).not.toBeNull(); - expect(result!.toolCalls.t1).toBeDefined(); - }); - it("returns prev for unknown part type", () => { const prev = createEmptyStreamState(); const result = applyMessagePartToStreamState(prev, { @@ -241,6 +222,114 @@ describe("applyMessagePartToStreamState", () => { expect(Object.keys(state!.toolCalls)).toHaveLength(2); expect(state!.blocks).toHaveLength(2); }); + + it("accumulates args via args_delta across multiple tool-call parts", () => { + let state: StreamState | null = null; + state = applyMessagePartToStreamState(state, { + type: "tool-call", + tool_call_id: "tc-1", + tool_name: "bash", + args_delta: '{"com', + }); + state = applyMessagePartToStreamState(state, { + type: "tool-call", + tool_call_id: "tc-1", + tool_name: "bash", + args_delta: 'mand":"ls"}', + }); + expect(state).not.toBeNull(); + expect(state!.toolCalls["tc-1"].args).toEqual({ command: "ls" }); + expect(state!.toolCalls["tc-1"].argsRaw).toBe('{"command":"ls"}'); + }); + + it("accepts complete args without args_delta", () => { + let state: StreamState | null = null; + state = applyMessagePartToStreamState(state, { + type: "tool-call", + tool_call_id: "tc-1", + tool_name: "bash", + args: { command: "ls" }, + }); + expect(state).not.toBeNull(); + expect(state!.toolCalls["tc-1"].args).toEqual({ command: "ls" }); + }); + + it("skips provider_executed tool-call parts", () => { + const prev = createEmptyStreamState(); + const result = applyMessagePartToStreamState(prev, { + type: "tool-call", + tool_name: "web_search", + tool_call_id: "tc-1", + provider_executed: true, + }); + expect(result).toBe(prev); + expect(prev.toolCalls).toEqual({}); + }); + + it("skips provider_executed tool-result parts", () => { + const prev = createEmptyStreamState(); + const result = applyMessagePartToStreamState(prev, { + type: "tool-result", + tool_name: "web_search", + tool_call_id: "tc-1", + provider_executed: true, + result: { output: "search results" }, + }); + expect(result).toBe(prev); + expect(prev.toolResults).toEqual({}); + }); + + it("adds a sources block from a source part", () => { + let state: StreamState | null = null; + state = applyMessagePartToStreamState(state, { + type: "source", + url: "https://example.com", + title: "Example", + }); + expect(state).not.toBeNull(); + expect(state!.sources).toEqual([ + { url: "https://example.com", title: "Example" }, + ]); + expect(state!.blocks).toHaveLength(1); + expect(state!.blocks[0]).toEqual({ + type: "sources", + sources: [{ url: "https://example.com", title: "Example" }], + }); + + // A second source with a different URL groups into the same block. + state = applyMessagePartToStreamState(state, { + type: "source", + url: "https://other.com", + title: "Other", + }); + expect(state!.sources).toHaveLength(2); + expect(state!.blocks).toHaveLength(1); + expect(state!.blocks[0]).toEqual({ + type: "sources", + sources: [ + { url: "https://example.com", title: "Example" }, + { url: "https://other.com", title: "Other" }, + ], + }); + }); + + it("deduplicates sources with the same URL", () => { + let state: StreamState | null = null; + state = applyMessagePartToStreamState(state, { + type: "source", + url: "https://example.com", + title: "Example", + }); + const afterFirst = state; + state = applyMessagePartToStreamState(state, { + type: "source", + url: "https://example.com", + title: "Example", + }); + // Second application returns prev unchanged. + expect(state).toBe(afterFirst); + expect(state!.sources).toHaveLength(1); + }); }); describe("buildStreamTools", () => { diff --git a/site/src/pages/AgentsPage/AgentDetail/streamState.ts b/site/src/pages/AgentsPage/AgentDetail/streamState.ts index 8ddba6477d..98f822d6b4 100644 --- a/site/src/pages/AgentsPage/AgentDetail/streamState.ts +++ b/site/src/pages/AgentsPage/AgentDetail/streamState.ts @@ -3,7 +3,6 @@ import { appendTextBlock } from "./blockUtils"; import { asOptionalTitle, ensureToolBlock, - normalizeBlockType, parseToolResultIsError, } from "./messageParsing"; import { mergeStreamPayload } from "./streamingJson"; @@ -25,7 +24,7 @@ export const applyMessagePartToStreamState = ( prev: StreamState | null, part: Record, ): StreamState | null => { - const partType = normalizeBlockType(part.type); + const partType = asString(part.type); const nextState: StreamState = prev ?? createEmptyStreamState(); switch (partType) { @@ -39,8 +38,7 @@ export const applyMessagePartToStreamState = ( blocks: appendStreamTextBlock(nextState.blocks, "response", text), }; } - case "reasoning": - case "thinking": { + case "reasoning": { const text = asString(part.text); if (!text) { return prev; @@ -56,8 +54,7 @@ export const applyMessagePartToStreamState = ( ), }; } - case "tool-call": - case "toolcall": { + case "tool-call": { // Provider-executed tool calls (e.g. web_search) are // handled natively by the provider — skip rendering them // as tool cards. @@ -94,8 +91,7 @@ export const applyMessagePartToStreamState = ( }, }; } - case "tool-result": - case "toolresult": { + case "tool-result": { // Skip synthetic results for provider-executed tools. if (part.provider_executed) { return prev; @@ -119,7 +115,7 @@ export const applyMessagePartToStreamState = ( existing?.result, existing?.resultRaw, part.result, - part.result_delta, + undefined, // no delta: tool results arrive complete, not streamed incrementally ); const nextToolName = toolName || existing?.name || "Tool"; const nextIsError = diff --git a/site/src/pages/AgentsPage/AgentDetailContent.tsx b/site/src/pages/AgentsPage/AgentDetailContent.tsx index 8fa8c35336..21914fa7e6 100644 --- a/site/src/pages/AgentsPage/AgentDetailContent.tsx +++ b/site/src/pages/AgentsPage/AgentDetailContent.tsx @@ -234,7 +234,10 @@ export const AgentDetailInput: FC = ({ setPreviewUrls(new Map()); return; } - const files = editingFileBlocks.map((block, i) => { + const fileBlocks = editingFileBlocks.filter( + (b): b is TypesGen.ChatFilePart => b.type === "file", + ); + const files = fileBlocks.map((block, i) => { const mt = block.media_type ?? "application/octet-stream"; const ext = mt.split("/")[1] ?? "png"; // Empty File used as a Map key only, its content is never @@ -246,13 +249,13 @@ export const AgentDetailInput: FC = ({ new Map( files.map((f, i) => [ f, - `/api/experimental/chats/files/${editingFileBlocks[i].file_id}`, + `/api/experimental/chats/files/${fileBlocks[i].file_id}`, ]), ), ); const newUploadStates = new Map(); for (const [i, file] of files.entries()) { - const block = editingFileBlocks[i]; + const block = fileBlocks[i]; if (block.file_id) { newUploadStates.set(file, { status: "uploaded",