feat(canvas): fix extractMessageText empty-string bug + add test coverage
Fixes the bug in extractMessageText (ConversationTraceModal.tsx) where
`if (p.text)` treated empty-string text fields as falsy, falling through
to root.text instead of returning "". Changed to `if ("text" in p)`.
Export extractReplyText (ChatTab.tsx) and deriveProvidersFromModels
(ConfigTab.tsx) for unit testing.
New test files:
- extractReplyText.test.ts: 14 cases for A2A response text extraction
- deriveProvidersFromModels.test.ts: 12 cases for vendor-slug derivation
Regression test added to ConversationTraceModal.test.tsx:
- empty-string text field is returned without falling through to root.text
Closes #874.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
parent
23f53ed361
commit
8e59d1ca2b
@ -35,7 +35,9 @@ export function extractMessageText(body: Record<string, unknown> | null): string
|
||||
const rParts = (result?.parts || []) as Array<Record<string, unknown>>;
|
||||
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<string, unknown> | undefined;
|
||||
return (root?.text as string) || "";
|
||||
})
|
||||
|
||||
@ -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", () => {
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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<string>();
|
||||
const out: string[] = [];
|
||||
for (const m of models) {
|
||||
|
||||
@ -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"]);
|
||||
});
|
||||
});
|
||||
149
canvas/src/components/tabs/__tests__/extractReplyText.test.ts
Normal file
149
canvas/src/components/tabs/__tests__/extractReplyText.test.ts
Normal file
@ -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");
|
||||
});
|
||||
});
|
||||
Loading…
Reference in New Issue
Block a user