diff --git a/canvas/src/components/ConversationTraceModal.tsx b/canvas/src/components/ConversationTraceModal.tsx index 7789b4c1..9eed3966 100644 --- a/canvas/src/components/ConversationTraceModal.tsx +++ b/canvas/src/components/ConversationTraceModal.tsx @@ -35,7 +35,9 @@ export function extractMessageText(body: Record | null): string const rParts = (result?.parts || []) as Array>; const rText = rParts .map((p) => { - if (p.text) return p.text as string; + // Use "text" in p (not p.text) so empty-string text fields + // are returned without falling through to root.text. + if ("text" in p) return p.text as string; const root = p.root as Record | undefined; return (root?.text as string) || ""; }) diff --git a/canvas/src/components/__tests__/ConversationTraceModal.test.tsx b/canvas/src/components/__tests__/ConversationTraceModal.test.tsx index 5df302ca..8df428d6 100644 --- a/canvas/src/components/__tests__/ConversationTraceModal.test.tsx +++ b/canvas/src/components/__tests__/ConversationTraceModal.test.tsx @@ -100,6 +100,21 @@ describe("extractMessageText — response result format", () => { // The implementation: all non-empty strings joined with newline. expect(extractMessageText(body)).toBe("Direct text\nRoot text"); }); + + // Regression test for the bug where `if (p.text)` (truthy check) treated + // empty-string text fields as falsy, falling through to root.text. + // Fix: use "text" in p instead of p.text. + it("returns empty-string text field without falling through to root.text", () => { + const body = { + result: { + parts: [{ text: "", root: { text: "should-not-appear" } }], + }, + }; + // text="" is present in the part — it must be returned, NOT root.text. + expect(extractMessageText(body)).toBe(""); + // root.text must NOT appear when text is an empty string (bug behavior). + expect(extractMessageText(body)).not.toContain("should-not-appear"); + }); }); describe("extractMessageText — plain string result", () => { diff --git a/canvas/src/components/tabs/ChatTab.tsx b/canvas/src/components/tabs/ChatTab.tsx index 156f87e8..7b0ee0d2 100644 --- a/canvas/src/components/tabs/ChatTab.tsx +++ b/canvas/src/components/tabs/ChatTab.tsx @@ -67,7 +67,7 @@ interface A2AResponse { // Server-side counterpart in workspace-server/internal/channels/ // manager.go has the same single-part bug; fix that too if/when a // channel-delivered reply (Slack, Lark, etc.) gets truncated. -function extractReplyText(resp: A2AResponse): string { +export function extractReplyText(resp: A2AResponse): string { const collect = (parts: A2APart[] | undefined): string => { if (!parts) return ""; return parts diff --git a/canvas/src/components/tabs/ConfigTab.tsx b/canvas/src/components/tabs/ConfigTab.tsx index 0c8b5bc3..6563a621 100644 --- a/canvas/src/components/tabs/ConfigTab.tsx +++ b/canvas/src/components/tabs/ConfigTab.tsx @@ -144,7 +144,7 @@ interface RuntimeOption { // haven't migrated to the explicit `providers:` field yet, AND // continues to be a useful fallback for any future runtime whose // derive-provider semantics happen to match the slug prefix. -function deriveProvidersFromModels(models: ModelSpec[]): string[] { +export function deriveProvidersFromModels(models: ModelSpec[]): string[] { const seen = new Set(); const out: string[] = []; for (const m of models) { diff --git a/canvas/src/components/tabs/__tests__/deriveProvidersFromModels.test.ts b/canvas/src/components/tabs/__tests__/deriveProvidersFromModels.test.ts new file mode 100644 index 00000000..0c4db2b8 --- /dev/null +++ b/canvas/src/components/tabs/__tests__/deriveProvidersFromModels.test.ts @@ -0,0 +1,111 @@ +// @vitest-environment jsdom +/** + * Tests for ConfigTab's deriveProvidersFromModels helper. + * + * Covers: colon-slug derivation, slash-slug derivation, deduplication, + * missing/undefined id handling, empty input, and edge cases. + */ +import { describe, expect, it } from "vitest"; +import { deriveProvidersFromModels } from "../ConfigTab"; + +type ModelSpec = { id: string; name?: string; required_env?: string[] }; + +describe("deriveProvidersFromModels", () => { + it("returns empty array for empty input", () => { + expect(deriveProvidersFromModels([])).toEqual([]); + }); + + it("returns empty array when all models have no id", () => { + expect(deriveProvidersFromModels([{ id: "" }, { id: "" }])).toEqual([]); + }); + + it("derives provider from colon-slug (anthropic:claude-opus-4-7)", () => { + const models: ModelSpec[] = [{ id: "anthropic:claude-opus-4-7" }]; + expect(deriveProvidersFromModels(models)).toEqual(["anthropic"]); + }); + + it("derives provider from slash-slug (nousresearch/hermes-4-70b)", () => { + const models: ModelSpec[] = [{ id: "nousresearch/hermes-4-70b" }]; + expect(deriveProvidersFromModels(models)).toEqual(["nousresearch"]); + }); + + it("derives multiple unique providers", () => { + const models: ModelSpec[] = [ + { id: "anthropic:claude-opus-4-7" }, + { id: "openai:gpt-4o" }, + { id: "anthropic:claude-sonnet-4-5" }, + ]; + expect(deriveProvidersFromModels(models)).toEqual(["anthropic", "openai"]); + }); + + it("deduplicates repeated provider", () => { + const models: ModelSpec[] = [ + { id: "anthropic:claude-opus-4-7" }, + { id: "anthropic:claude-sonnet-4-5" }, + { id: "anthropic:haiku" }, + ]; + expect(deriveProvidersFromModels(models)).toEqual(["anthropic"]); + }); + + it("skips models with no vendor separator", () => { + const models: ModelSpec[] = [ + { id: "claude-opus-4-7" }, // no colon or slash + { id: "anthropic:claude-opus-4-7" }, + ]; + expect(deriveProvidersFromModels(models)).toEqual(["anthropic"]); + }); + + it("skips models with separator at position 0 (malformed slug)", () => { + const models: ModelSpec[] = [ + { id: ":claude-opus-4-7" }, // separator at start + { id: "/claude-opus-4-7" }, + { id: "anthropic:claude-opus-4-7" }, + ]; + expect(deriveProvidersFromModels(models)).toEqual(["anthropic"]); + }); + + it("prefers colon over slash when both present", () => { + // Real-world models don't have both, but the regex takes whichever comes first. + const models: ModelSpec[] = [{ id: "anthropic:foo/bar" }]; + expect(deriveProvidersFromModels(models)).toEqual(["anthropic"]); + }); + + it("skips models with undefined id", () => { + const models: ModelSpec[] = [ + { id: "anthropic:claude-opus-4-7" }, + { id: undefined as unknown as string }, + ]; + expect(deriveProvidersFromModels(models)).toEqual(["anthropic"]); + }); + + it("skips models with null id", () => { + const models: ModelSpec[] = [ + { id: "openai:gpt-4o" }, + { id: null as unknown as string }, + ]; + expect(deriveProvidersFromModels(models)).toEqual(["openai"]); + }); + + it("handles mixed colon and slash slugs together", () => { + const models: ModelSpec[] = [ + { id: "anthropic:claude-opus-4-7" }, + { id: "nousresearch/hermes-4-70b" }, + { id: "google:gemini-2.5" }, + ]; + expect(deriveProvidersFromModels(models)).toEqual([ + "anthropic", + "nousresearch", + "google", + ]); + }); + + it("preserves insertion order of first occurrence", () => { + const models: ModelSpec[] = [ + { id: "openai:gpt-4o" }, + { id: "anthropic:claude-opus-4-7" }, + { id: "anthropic:claude-sonnet-4-5" }, // duplicate + { id: "openai:gpt-4o-mini" }, // duplicate + ]; + expect(deriveProvidersFromModels(models)).toEqual(["openai", "anthropic"]); + }); +}); diff --git a/canvas/src/components/tabs/__tests__/extractReplyText.test.ts b/canvas/src/components/tabs/__tests__/extractReplyText.test.ts new file mode 100644 index 00000000..cebe6563 --- /dev/null +++ b/canvas/src/components/tabs/__tests__/extractReplyText.test.ts @@ -0,0 +1,149 @@ +// @vitest-environment jsdom +/** + * Tests for ChatTab's extractReplyText helper. + * + * Covers: empty/undefined response, single text part, multiple text parts + * joined with newlines, non-text part filtering, artifact collection, + * null/undefined safety. + */ +import { describe, expect, it } from "vitest"; +import { extractReplyText } from "../ChatTab"; + +type A2APart = { kind: string; text?: string }; +type A2AResponse = { + result?: { + parts?: A2APart[]; + artifacts?: Array<{ parts: A2APart[] }>; + }; +}; + +function resp(shape: A2AResponse): A2AResponse { + return shape; +} + +describe("extractReplyText", () => { + it("returns empty string for undefined response", () => { + expect(extractReplyText(undefined as unknown as A2AResponse)).toBe(""); + }); + + it("returns empty string when result is undefined", () => { + expect(extractReplyText(resp({}))).toBe(""); + }); + + it("returns empty string when result.parts is undefined", () => { + expect(extractReplyText(resp({ result: {} }))).toBe(""); + }); + + it("returns empty string when result.parts is empty", () => { + expect(extractReplyText(resp({ result: { parts: [] } }))).toBe(""); + }); + + it("extracts single text part", () => { + const r = resp({ result: { parts: [{ kind: "text", text: "Hello world" }] } }); + expect(extractReplyText(r)).toBe("Hello world"); + }); + + it("joins multiple text parts with newlines", () => { + const r = resp({ + result: { + parts: [ + { kind: "text", text: "First part" }, + { kind: "text", text: "Second part" }, + ], + }, + }); + expect(extractReplyText(r)).toBe("First part\nSecond part"); + }); + + it("filters out non-text parts", () => { + const r = resp({ + result: { + parts: [ + { kind: "text", text: "Visible" }, + { kind: "file", text: "should-be-ignored" }, + { kind: "other" }, + ], + }, + }); + expect(extractReplyText(r)).toBe("Visible"); + }); + + it("handles parts with undefined text", () => { + const r = resp({ + result: { + parts: [ + { kind: "text" }, // text is undefined + { kind: "text", text: "Valid" }, + ], + }, + }); + expect(extractReplyText(r)).toBe("Valid"); + }); + + it("returns empty string when all parts are non-text", () => { + const r = resp({ + result: { + parts: [{ kind: "file" }, { kind: "image" }], + }, + }); + expect(extractReplyText(r)).toBe(""); + }); + + it("collects text from artifacts", () => { + const r = resp({ + result: { + parts: [], + artifacts: [{ parts: [{ kind: "text", text: "Artifact text" }] }], + }, + }); + expect(extractReplyText(r)).toBe("Artifact text"); + }); + + it("combines parts and artifacts text with newlines", () => { + const r = resp({ + result: { + parts: [{ kind: "text", text: "From parts" }], + artifacts: [{ parts: [{ kind: "text", text: "From artifact" }] }], + }, + }); + expect(extractReplyText(r)).toBe("From parts\nFrom artifact"); + }); + + it("handles multiple artifacts", () => { + const r = resp({ + result: { + artifacts: [ + { parts: [{ kind: "text", text: "A1" }] }, + { parts: [{ kind: "text", text: "A2" }] }, + ], + }, + }); + expect(extractReplyText(r)).toBe("A1\nA2"); + }); + + it("handles artifacts with empty parts array", () => { + const r = resp({ + result: { + parts: [{ kind: "text", text: "Parts only" }], + artifacts: [{ parts: [] }], + }, + }); + expect(extractReplyText(r)).toBe("Parts only"); + }); + + it("filters non-text parts in artifacts", () => { + const r = resp({ + result: { + artifacts: [ + { + parts: [ + { kind: "text", text: "Visible" }, + { kind: "file" }, + ], + }, + ], + }, + }); + expect(extractReplyText(r)).toBe("Visible"); + }); +});