From 379a54ff1ce7017e8f81d0edcd4f165d188aaebd Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 22:15:51 +0000 Subject: [PATCH 1/8] feat: finish the Tier 1 gaps a downstream host had to patch locally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four contracts this repo owns had holes that every consumer worked around in its own tree, and one interaction kind it never had a name for. Chat history hygiene (@workglow/ai). normalizeHistoryForModel repairs the `user, user` and `tool, user` sequences that HuggingFace's apply_chat_template rejects with "Conversation roles must alternate" — a template this repo ships and, until now, shipped none of the defence for. A host reaches that state honestly: a turn stopped mid-thought leaves a trailing user message, and the next thing typed makes two. trimHistoryForModel caps a message list by dropping whole turns from the front, cutting only at user boundaries so no tool_result outlives the tool_use it answers, and keeping the newest turn even when it alone busts the budget — the alternative is a conversation erased for being too long. Tool-call id durability (@workglow/ai). The Gemini adapter already documents that ids restart at call_0 every run and compensates inside its own message conversion. That fixes what reaches the provider, not what a caller keeps: a host holding several rounds in one list collides on round two, and the symptom is not a crash but patches landing on the wrong entry and answers resolving the wrong call. uniquifyToolCallIds and repairDuplicateToolCallIds make the ids durable caller-side; ids are opaque to providers, so renaming both halves of a pair is invisible downstream. Bounds on a model-authored JSON Schema (@workglow/util/schema). Tool-call arguments already go through sanitizeToolArgs before validation. The schema those arguments are validated against went through nothing, and in this codebase a `format` annotation does not style a field, it selects a runtime editor and resolves a live resource. An unbounded format is the model choosing one, so the allowlist is the rule and the default set is frozen. TaskGraphJson shape validation (@workglow/task-graph). createGraphFromGraphJSON throws from inside its own construction, in words written for whoever wrote the deserializer. That is the wrong audience for graph JSON the process did not author — a file, a request body, a model's output — where the caller's next move is handing a reason back. It names the offending id and stays structural: whether a type is runnable is a question about the host's registry, asked separately. And IHumanRequest gains kind: "confirm". A confirm is not an elicit with two options: an elicit asks what a value should be and its schema describes fields to fill in; a confirm asks whether something should happen and its schema describes the action, for a person to read. HumanInputTask expects a response for it, McpElicitationConnector maps it onto elicitation explicitly — the closest honest transport MCP has — and the conformance contract gains a confirm capability and fixture, so a connector cannot claim the kind and silently auto-accept. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JEFYAGb9D3mWfyhYkmeAvN --- packages/ai/src/task/ChatHistory.ts | 105 ++++++++++++++ packages/ai/src/task/ToolCallIds.ts | 111 +++++++++++++++ .../ai/src/task/__tests__/ChatHistory.test.ts | 102 +++++++++++++ .../ai/src/task/__tests__/ToolCallIds.test.ts | 103 ++++++++++++++ packages/ai/src/task/index.ts | 2 + .../mcp/src/tasks/McpElicitationConnector.ts | 8 ++ packages/task-graph/src/common.ts | 1 + .../src/task-graph/TaskGraphJsonShape.ts | 75 ++++++++++ .../__tests__/TaskGraphJsonShape.test.ts | 105 ++++++++++++++ packages/tasks/src/task/HumanInputTask.ts | 9 +- .../assertions/capabilityHonesty.ts | 8 +- .../src/contract/human-connector/fixtures.ts | 14 ++ .../src/contract/human-connector/types.ts | 9 ++ ...cpElicitationConnector.conformance.test.ts | 1 + .../MockHumanConnector.conformance.test.ts | 1 + ...anConnector_NoFollowUp.conformance.test.ts | 1 + packages/util/src/human/HumanConnector.ts | 17 ++- .../src/json-schema/ModelAuthoredSchema.ts | 134 ++++++++++++++++++ .../__tests__/ModelAuthoredSchema.test.ts | 125 ++++++++++++++++ packages/util/src/schema-entry.ts | 1 + 20 files changed, 924 insertions(+), 8 deletions(-) create mode 100644 packages/ai/src/task/ChatHistory.ts create mode 100644 packages/ai/src/task/ToolCallIds.ts create mode 100644 packages/ai/src/task/__tests__/ChatHistory.test.ts create mode 100644 packages/ai/src/task/__tests__/ToolCallIds.test.ts create mode 100644 packages/task-graph/src/task-graph/TaskGraphJsonShape.ts create mode 100644 packages/task-graph/src/task-graph/__tests__/TaskGraphJsonShape.test.ts create mode 100644 packages/util/src/json-schema/ModelAuthoredSchema.ts create mode 100644 packages/util/src/json-schema/__tests__/ModelAuthoredSchema.test.ts diff --git a/packages/ai/src/task/ChatHistory.ts b/packages/ai/src/task/ChatHistory.ts new file mode 100644 index 000000000..8d19728e7 --- /dev/null +++ b/packages/ai/src/task/ChatHistory.ts @@ -0,0 +1,105 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { ChatMessage } from "./ChatMessage"; + +/** Concatenates the text blocks of a message, ignoring every other block kind. */ +function messageText(message: ChatMessage): string { + return message.content + .filter((block): block is Extract => block.type === "text") + .map((block) => block.text) + .join(""); +} + +/** + * Returns a copy of `history` that strict chat templates will accept. + * + * HuggingFace's `apply_chat_template` throws + * `Conversation roles must alternate user/assistant/...` on consecutive user + * messages, and this library ships the providers that hit it. A host driving a + * multi-turn conversation reaches that state honestly: a turn stopped while the + * model was still thinking leaves `[..., user]`, and the next thing the person + * types makes it `user, user`. + * + * Consecutive user messages are merged into one so both texts still reach the + * model. A `tool` result left without the assistant reply that would normally + * follow it gets a neutral assistant bridge, since the same templates reject + * `tool, user` for the same reason. + * + * Non-text blocks on a merged message are dropped: the merge exists to keep a + * template from throwing, and an image cannot be concatenated into a string. + * Callers who must preserve them should avoid producing the sequence instead. + */ +export function normalizeHistoryForModel(history: readonly ChatMessage[]): ChatMessage[] { + const out: ChatMessage[] = []; + for (const message of history) { + const last = out[out.length - 1]; + if (message.role === "user" && last?.role === "user") { + const previous = messageText(last); + const next = messageText(message); + const merged = + previous.length > 0 && next.length > 0 ? `${previous}\n\n${next}` : previous + next; + out[out.length - 1] = { role: "user", content: [{ type: "text", text: merged }] }; + continue; + } + if (message.role === "user" && last?.role === "tool") { + out.push({ role: "assistant", content: [{ type: "text", text: "Acknowledged." }] }); + } + out.push(message); + } + return out; +} + +/** + * Default character budget for a message list handed to a model. + * + * Characters rather than tokens on purpose: this module has no tokenizer and a + * wrong tokenizer is worse than an honest approximation. Size it well under the + * context window it is protecting. + */ +export const DEFAULT_MAX_HISTORY_CHARS = 120_000; + +function messageChars(message: ChatMessage): number { + try { + return JSON.stringify(message).length; + } catch { + return 0; + } +} + +/** + * Caps `history` at `max` characters by dropping whole turns from the front. + * + * A turn starts at a `user` message and owns every assistant and tool message + * after it, so cutting only at those boundaries keeps each `tool_result` with + * the `tool_use` it answers — providers reject a `tool_result` whose `tool_use` + * is missing, which is a harder failure than being over budget. + * + * The newest turn is kept even when it alone exceeds the budget: there would + * otherwise be nothing for the model to answer, and silently returning an empty + * list turns "this turn is too long" into "the conversation is gone". + */ +export function trimHistoryForModel( + history: readonly ChatMessage[], + max: number = DEFAULT_MAX_HISTORY_CHARS +): ChatMessage[] { + const sizes = history.map(messageChars); + let total = sizes.reduce((sum, n) => sum + n, 0); + if (total <= max) return [...history]; + + const turnStarts: number[] = []; + for (let i = 0; i < history.length; i++) { + if (history[i]?.role === "user") turnStarts.push(i); + } + + let cut = 0; + for (const start of turnStarts.slice(1)) { + for (let i = cut; i < start; i++) total -= sizes[i] ?? 0; + cut = start; + if (total <= max) break; + } + return history.slice(cut); +} diff --git a/packages/ai/src/task/ToolCallIds.ts b/packages/ai/src/task/ToolCallIds.ts new file mode 100644 index 000000000..73efb12d8 --- /dev/null +++ b/packages/ai/src/task/ToolCallIds.ts @@ -0,0 +1,111 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { ChatMessage, ContentBlockToolResult, ContentBlockToolUse } from "./ChatMessage"; +import type { ToolCall } from "./ToolCallingUtils"; + +/** + * Ids a model returns for tool calls are unique only within one model run. + * Gemini synthesizes `call_0`, `call_1`, … from zero on every run, and the + * provider's own message conversion already compensates by resolving each + * `tool_result` against the most recent preceding `tool_use` with that id. + * + * That fixes what reaches the provider. It does not fix what a caller keeps: + * a host running several rounds in one conversation holds every round's calls + * in one list, and the second round collides with the first. The failure is + * not a crash — patches land on the wrong entry, UI keys duplicate, and a + * pending answer resolves the wrong call. + * + * These helpers make the ids durable on the caller's side. Ids are opaque to + * providers, which rebuild their own id→name map from the messages each run, + * so renaming both halves of a pair is invisible downstream. + */ + +/** Every `tool_use` id already present in `history`. */ +export function collectToolUseIds(history: readonly ChatMessage[]): Set { + const seen = new Set(); + for (const message of history) { + if (message.role !== "assistant") continue; + for (const block of message.content) { + if (block.type === "tool_use") seen.add(block.id); + } + } + return seen; +} + +/** + * Renames any call whose id is already in `seen`, so a round's ids stay unique + * across the whole conversation. `seen` is not mutated. + */ +export function uniquifyToolCallIds( + calls: readonly ToolCall[], + seen: Iterable = [] +): ToolCall[] { + const used = new Set(seen); + return calls.map((call) => { + if (!used.has(call.id)) { + used.add(call.id); + return call; + } + let suffix = 2; + while (used.has(`${call.id}_${suffix}`)) suffix++; + const renamed = { ...call, id: `${call.id}_${suffix}` }; + used.add(renamed.id); + return renamed; + }); +} + +function renameForOccurrence(seen: Map, id: string): string { + const occurrence = (seen.get(id) ?? 0) + 1; + seen.set(id, occurrence); + return occurrence === 1 ? id : `${id}_${occurrence}`; +} + +/** + * Repairs a stored conversation whose tool-call ids are not unique — one + * written before the caller started uniquifying them, say. + * + * The k-th occurrence of an id becomes `_` consistently across + * `tool_use` and `tool_result` blocks. The two are paired by occurrence order + * rather than by matching ids, which is what keeps a pair together: within one + * conversation the n-th `tool_result` for an id answers the n-th `tool_use` of + * it, because a round's results are appended after its calls. + * + * An already-unique history passes through with only shallow copies. + */ +export function repairDuplicateToolCallIds(history: readonly ChatMessage[]): ChatMessage[] { + const useSeen = new Map(); + const resultSeen = new Map(); + return history.map((message) => { + if (message.role === "assistant") { + return { + ...message, + content: message.content.map((block) => + block.type === "tool_use" + ? ({ + ...block, + id: renameForOccurrence(useSeen, block.id), + } satisfies ContentBlockToolUse) + : block + ), + }; + } + if (message.role === "tool") { + return { + ...message, + content: message.content.map((block) => + block.type === "tool_result" + ? ({ + ...block, + tool_use_id: renameForOccurrence(resultSeen, block.tool_use_id), + } satisfies ContentBlockToolResult) + : block + ), + }; + } + return message; + }); +} diff --git a/packages/ai/src/task/__tests__/ChatHistory.test.ts b/packages/ai/src/task/__tests__/ChatHistory.test.ts new file mode 100644 index 000000000..a80231fc2 --- /dev/null +++ b/packages/ai/src/task/__tests__/ChatHistory.test.ts @@ -0,0 +1,102 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { ChatMessage } from "@workglow/ai"; +import { + DEFAULT_MAX_HISTORY_CHARS, + normalizeHistoryForModel, + trimHistoryForModel, +} from "@workglow/ai"; +import { describe, expect, it } from "vitest"; + +const user = (text: string): ChatMessage => ({ role: "user", content: [{ type: "text", text }] }); +const assistant = (text: string): ChatMessage => ({ + role: "assistant", + content: [{ type: "text", text }], +}); +const toolResult = (id: string): ChatMessage => ({ + role: "tool", + content: [{ type: "tool_result", tool_use_id: id, content: [{ type: "text", text: "ok" }] }], +}); + +describe("normalizeHistoryForModel", () => { + it("merges consecutive user messages, keeping both texts", () => { + // A turn stopped while the model was thinking leaves a trailing `user`; + // the next thing typed makes `user, user`, which strict templates reject. + const out = normalizeHistoryForModel([user("first"), user("second")]); + expect(out).toHaveLength(1); + expect(out[0]).toEqual({ role: "user", content: [{ type: "text", text: "first\n\nsecond" }] }); + }); + + it("does not introduce a blank line when one side is empty", () => { + const out = normalizeHistoryForModel([user(""), user("only")]); + expect(out[0]!.content).toEqual([{ type: "text", text: "only" }]); + }); + + it("bridges a tool result followed by a user message", () => { + const out = normalizeHistoryForModel([user("q"), toolResult("t1"), user("next")]); + expect(out.map((m) => m.role)).toEqual(["user", "tool", "assistant", "user"]); + }); + + it("leaves an already-alternating history untouched", () => { + const history = [user("a"), assistant("b"), user("c")]; + expect(normalizeHistoryForModel(history)).toEqual(history); + }); + + it("returns a copy rather than mutating its input", () => { + const history = [user("a"), user("b")]; + const before = JSON.stringify(history); + normalizeHistoryForModel(history); + expect(JSON.stringify(history)).toBe(before); + }); +}); + +describe("trimHistoryForModel", () => { + it("returns everything when the history fits", () => { + const history = [user("a"), assistant("b")]; + expect(trimHistoryForModel(history)).toEqual(history); + }); + + it("cuts only at a user message, so no tool_result outlives its tool_use", () => { + const history = [ + user("turn one"), + assistant("thinking"), + toolResult("t1"), + assistant("answer one"), + user("turn two"), + assistant("answer two"), + ]; + const trimmed = trimHistoryForModel(history, 120); + expect(trimmed[0]!.role).toBe("user"); + // Whatever survived, a tool message never leads the result. + expect(trimmed.some((m, i) => m.role === "tool" && i === 0)).toBe(false); + }); + + it("keeps the newest turn even when it alone exceeds the budget", () => { + // Discarding here is how a conversation gets erased for being too long. + const history = [user("old"), assistant("old reply"), user("x".repeat(5000))]; + const trimmed = trimHistoryForModel(history, 10); + expect(trimmed).toHaveLength(1); + expect(trimmed[0]!.role).toBe("user"); + }); + + it("drops whole leading turns until the budget is met", () => { + const history = [ + user("a".repeat(400)), + assistant("x"), + user("b".repeat(400)), + assistant("y"), + user("c"), + ]; + const trimmed = trimHistoryForModel(history, 500); + expect(trimmed.length).toBeLessThan(history.length); + expect(trimmed[0]!.role).toBe("user"); + }); + + it("exposes a default budget callers can reason about", () => { + expect(DEFAULT_MAX_HISTORY_CHARS).toBeGreaterThan(0); + }); +}); diff --git a/packages/ai/src/task/__tests__/ToolCallIds.test.ts b/packages/ai/src/task/__tests__/ToolCallIds.test.ts new file mode 100644 index 000000000..b062568d6 --- /dev/null +++ b/packages/ai/src/task/__tests__/ToolCallIds.test.ts @@ -0,0 +1,103 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { ChatMessage, ToolCall } from "@workglow/ai"; +import { collectToolUseIds, repairDuplicateToolCallIds, uniquifyToolCallIds } from "@workglow/ai"; +import { describe, expect, it } from "vitest"; + +const call = (id: string, name = "search"): ToolCall => ({ id, name, input: {} }); + +const assistantCalls = (...ids: string[]): ChatMessage => ({ + role: "assistant", + content: ids.map((id) => ({ type: "tool_use", id, name: "search", input: {} })), +}); +const toolResults = (...ids: string[]): ChatMessage => ({ + role: "tool", + content: ids.map((id) => ({ + type: "tool_result", + tool_use_id: id, + content: [{ type: "text", text: "ok" }], + })), +}); + +describe("collectToolUseIds", () => { + it("gathers ids from assistant turns only", () => { + const history = [assistantCalls("call_0", "call_1"), toolResults("call_0", "call_1")]; + expect([...collectToolUseIds(history)].sort()).toEqual(["call_0", "call_1"]); + }); + + it("is empty for a history with no tool calls", () => { + expect( + collectToolUseIds([{ role: "user", content: [{ type: "text", text: "hi" }] }]).size + ).toBe(0); + }); +}); + +describe("uniquifyToolCallIds", () => { + it("renames a call whose id the conversation already used", () => { + // Gemini restarts at call_0 every run, so round two collides with round one. + const out = uniquifyToolCallIds([call("call_0")], ["call_0"]); + expect(out[0]!.id).toBe("call_0_2"); + }); + + it("keeps going past an already-taken rename", () => { + const out = uniquifyToolCallIds([call("call_0")], ["call_0", "call_0_2"]); + expect(out[0]!.id).toBe("call_0_3"); + }); + + it("deduplicates within a single batch", () => { + const out = uniquifyToolCallIds([call("x"), call("x")]); + expect(out.map((c) => c.id)).toEqual(["x", "x_2"]); + }); + + it("leaves non-colliding ids alone and preserves the rest of the call", () => { + const original = { id: "a", name: "fetch", input: { url: "u" }, providerSignature: "sig" }; + const out = uniquifyToolCallIds([original], ["b"]); + expect(out[0]).toEqual(original); + }); + + it("does not mutate the caller's seen set", () => { + const seen = new Set(["call_0"]); + uniquifyToolCallIds([call("call_0")], seen); + expect([...seen]).toEqual(["call_0"]); + }); +}); + +describe("repairDuplicateToolCallIds", () => { + it("renames the k-th occurrence on both sides so pairs stay together", () => { + const history = [ + assistantCalls("call_0"), + toolResults("call_0"), + assistantCalls("call_0"), + toolResults("call_0"), + ]; + const out = repairDuplicateToolCallIds(history); + const uses = out.flatMap((m) => + m.role === "assistant" + ? m.content.filter((b) => b.type === "tool_use").map((b: any) => b.id) + : [] + ); + const results = out.flatMap((m) => + m.role === "tool" + ? m.content.filter((b) => b.type === "tool_result").map((b: any) => b.tool_use_id) + : [] + ); + expect(uses).toEqual(["call_0", "call_0_2"]); + expect(results).toEqual(["call_0", "call_0_2"]); + }); + + it("leaves an already-unique history unchanged", () => { + const history = [assistantCalls("a", "b"), toolResults("a", "b")]; + expect(repairDuplicateToolCallIds(history)).toEqual(history); + }); + + it("does not mutate its input", () => { + const history = [assistantCalls("call_0"), assistantCalls("call_0")]; + const before = JSON.stringify(history); + repairDuplicateToolCallIds(history); + expect(JSON.stringify(history)).toBe(before); + }); +}); diff --git a/packages/ai/src/task/index.ts b/packages/ai/src/task/index.ts index 94fbe7a23..b6397fbcc 100644 --- a/packages/ai/src/task/index.ts +++ b/packages/ai/src/task/index.ts @@ -20,6 +20,7 @@ export * from "./base/responseFormat"; export * from "./base/runWithIterable"; export * from "./base/StreamingAiTask"; export * from "./CacheCheckpointTask"; +export * from "./ChatHistory"; export * from "./ChatMessage"; export * from "./ChunkRetrievalTask"; export * from "./ChunkVectorUpsertTask"; @@ -68,6 +69,7 @@ export * from "./TextRewriterTask"; export * from "./TextSummaryTask"; export * from "./TextTranslationTask"; export * from "./ToolCallingTask"; +export * from "./ToolCallIds"; export * from "./ToolCallingUtils"; export * from "./TopicSegmenterTask"; export * from "./VectorQuantizeTask"; diff --git a/packages/mcp/src/tasks/McpElicitationConnector.ts b/packages/mcp/src/tasks/McpElicitationConnector.ts index 695fc23e8..3520b7f6f 100644 --- a/packages/mcp/src/tasks/McpElicitationConnector.ts +++ b/packages/mcp/src/tasks/McpElicitationConnector.ts @@ -124,6 +124,14 @@ export class McpElicitationConnector implements IHumanConnector { case "elicit": return this.handleElicit(request, signal); + // MCP has no approval primitive, and elicitation is the closest honest + // mapping: it is the one round-trip that asks a person to decide and + // carries accept/decline back. The schema describes the action rather + // than fields to fill in, which a client renders as a form — coarser + // than a native approval, but never silently auto-approved. + case "confirm": + return this.handleElicit(request, signal); + default: return this.handleElicit(request, signal); } diff --git a/packages/task-graph/src/common.ts b/packages/task-graph/src/common.ts index 7da25da6c..d2cedb647 100644 --- a/packages/task-graph/src/common.ts +++ b/packages/task-graph/src/common.ts @@ -23,6 +23,7 @@ export * from "./task-graph/RunScheduler"; export * from "./task-graph/StreamPump"; export * from "./task-graph/SubGraphEventBridge"; export * from "./task-graph/TaskGraph"; +export * from "./task-graph/TaskGraphJsonShape"; export * from "./task-graph/TaskGraphEvents"; export * from "./task-graph/TaskGraphRunner"; diff --git a/packages/task-graph/src/task-graph/TaskGraphJsonShape.ts b/packages/task-graph/src/task-graph/TaskGraphJsonShape.ts new file mode 100644 index 000000000..f7e130efb --- /dev/null +++ b/packages/task-graph/src/task-graph/TaskGraphJsonShape.ts @@ -0,0 +1,75 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { TaskGraphJson } from "../task/TaskJSON"; + +/** + * Checks the shape of a `TaskGraphJson` before deserialization touches it. + * + * `createGraphFromGraphJSON` throws from deep inside its own construction, and + * the message it throws is written for whoever wrote the deserializer. That is + * the wrong audience for graph JSON this process did not author — a CLI given a + * file, an HTTP endpoint given a body, a model asked to produce a graph. Those + * callers need to hand a reason back to whoever supplied the graph, and a + * stack from inside a constructor is not one. + * + * Returns the first problem as a sentence naming the offending id, or + * `undefined` when the shape is sound. It is deliberately structural only: + * whether a `type` is a task this host will *run* is a question about the + * host's registry, not about the JSON, and it is asked separately. + */ +export function taskGraphJsonShapeError(graph: unknown): string | undefined { + if (!graph || typeof graph !== "object" || Array.isArray(graph)) { + return "graph must be an object with tasks and dataflows"; + } + const candidate = graph as Record; + if (!Array.isArray(candidate.tasks)) return "graph.tasks must be an array"; + if (!Array.isArray(candidate.dataflows)) return "graph.dataflows must be an array"; + + const ids = new Set(); + for (const entry of candidate.tasks) { + if (!entry || typeof entry !== "object" || Array.isArray(entry)) + return "each task must be an object"; + const task = entry as Record; + if (typeof task.id !== "string" || task.id.length === 0) return "each task needs a string id"; + if (typeof task.type !== "string") return `task "${task.id}" needs a string type`; + if (ids.has(task.id)) return `duplicate task id "${task.id}"`; + ids.add(task.id); + const defaults = task.defaults; + if ( + defaults !== undefined && + (typeof defaults !== "object" || defaults === null || Array.isArray(defaults)) + ) { + return `task "${task.id}" defaults must be an object`; + } + } + + for (const entry of candidate.dataflows) { + if (!entry || typeof entry !== "object" || Array.isArray(entry)) + return "each dataflow must be an object"; + const dataflow = entry as Record; + for (const key of ["sourceTaskId", "sourceTaskPortId", "targetTaskId", "targetTaskPortId"]) { + if (typeof dataflow[key] !== "string") return `dataflow is missing ${key}`; + } + if (!ids.has(dataflow.sourceTaskId as string)) { + return `dataflow source "${dataflow.sourceTaskId}" is not a task id`; + } + if (!ids.has(dataflow.targetTaskId as string)) { + return `dataflow target "${dataflow.targetTaskId}" is not a task id`; + } + } + return undefined; +} + +export type TaskGraphJsonShapeResult = + | { readonly ok: true; readonly graph: TaskGraphJson } + | { readonly ok: false; readonly reason: string }; + +/** {@link taskGraphJsonShapeError} as a narrowing result. */ +export function validateTaskGraphJsonShape(graph: unknown): TaskGraphJsonShapeResult { + const reason = taskGraphJsonShapeError(graph); + return reason === undefined ? { ok: true, graph: graph as TaskGraphJson } : { ok: false, reason }; +} diff --git a/packages/task-graph/src/task-graph/__tests__/TaskGraphJsonShape.test.ts b/packages/task-graph/src/task-graph/__tests__/TaskGraphJsonShape.test.ts new file mode 100644 index 000000000..3227003de --- /dev/null +++ b/packages/task-graph/src/task-graph/__tests__/TaskGraphJsonShape.test.ts @@ -0,0 +1,105 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import { taskGraphJsonShapeError, validateTaskGraphJsonShape } from "@workglow/task-graph"; +import { describe, expect, it } from "vitest"; + +const flow = { + sourceTaskId: "a", + sourceTaskPortId: "out", + targetTaskId: "b", + targetTaskPortId: "in", +}; + +describe("taskGraphJsonShapeError", () => { + it("accepts a well-formed graph", () => { + expect( + taskGraphJsonShapeError({ + tasks: [ + { id: "a", type: "InputTask" }, + { id: "b", type: "OutputTask", defaults: { x: 1 } }, + ], + dataflows: [flow], + }) + ).toBeUndefined(); + }); + + it("accepts an empty graph", () => { + expect(taskGraphJsonShapeError({ tasks: [], dataflows: [] })).toBeUndefined(); + }); + + it("rejects a non-object graph", () => { + for (const bad of [null, undefined, 1, "g", []]) { + expect(taskGraphJsonShapeError(bad)).toBeDefined(); + } + }); + + it("requires both arrays", () => { + expect(taskGraphJsonShapeError({ dataflows: [] })).toContain("tasks must be an array"); + expect(taskGraphJsonShapeError({ tasks: [] })).toContain("dataflows must be an array"); + }); + + it("names the duplicate id rather than reporting that something is wrong", () => { + const reason = taskGraphJsonShapeError({ + tasks: [ + { id: "a", type: "InputTask" }, + { id: "a", type: "OutputTask" }, + ], + dataflows: [], + }); + expect(reason).toContain('duplicate task id "a"'); + }); + + it("requires a string id and type on every task", () => { + expect(taskGraphJsonShapeError({ tasks: [{ type: "InputTask" }], dataflows: [] })).toContain( + "string id" + ); + expect(taskGraphJsonShapeError({ tasks: [{ id: "a" }], dataflows: [] })).toContain( + "string type" + ); + expect(taskGraphJsonShapeError({ tasks: [{ id: "", type: "T" }], dataflows: [] })).toContain( + "string id" + ); + }); + + it("rejects defaults that are not a plain object", () => { + for (const defaults of [[], "x", 1]) { + expect( + taskGraphJsonShapeError({ tasks: [{ id: "a", type: "T", defaults }], dataflows: [] }) + ).toContain("defaults must be an object"); + } + }); + + it("catches a dataflow pointing at a task that does not exist", () => { + const reason = taskGraphJsonShapeError({ + tasks: [{ id: "a", type: "InputTask" }], + dataflows: [flow], + }); + expect(reason).toContain('dataflow target "b" is not a task id'); + }); + + it("catches a dataflow missing an endpoint field", () => { + const reason = taskGraphJsonShapeError({ + tasks: [{ id: "a", type: "InputTask" }], + dataflows: [{ sourceTaskId: "a", targetTaskId: "a", targetTaskPortId: "in" }], + }); + expect(reason).toContain("sourceTaskPortId"); + }); +}); + +describe("validateTaskGraphJsonShape", () => { + it("narrows on success", () => { + const result = validateTaskGraphJsonShape({ tasks: [], dataflows: [] }); + expect(result.ok).toBe(true); + if (result.ok) expect(result.graph.tasks).toEqual([]); + }); + + it("carries the reason on failure", () => { + const result = validateTaskGraphJsonShape({ tasks: "no", dataflows: [] }); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toContain("tasks must be an array"); + }); +}); diff --git a/packages/tasks/src/task/HumanInputTask.ts b/packages/tasks/src/task/HumanInputTask.ts index ee84d9778..fde03f4b4 100644 --- a/packages/tasks/src/task/HumanInputTask.ts +++ b/packages/tasks/src/task/HumanInputTask.ts @@ -36,8 +36,8 @@ const humanInputTaskConfigSchema = { type: "string", title: "Kind", description: - "Interaction kind: notify (one-way), display (show content), elicit (request input)", - enum: ["notify", "display", "elicit"], + "Interaction kind: notify (one-way), display (show content), elicit (request input), confirm (approve an action)", + enum: ["notify", "display", "elicit", "confirm"], default: "elicit", }, contentSchema: { @@ -144,6 +144,7 @@ export type HumanInputTaskOutput = { * - "notify": Send a notification (fire-and-forget, task completes immediately) * - "display": Present content to the human (charts, data, markdown) * - "elicit": Request structured input via a form (MCP elicitation model) + * - "confirm": Ask a human to approve or refuse one described action * * The contentSchema describes WHAT to render. The kind determines HOW. * For "elicit", the output includes the human's submitted data. @@ -224,7 +225,9 @@ export class HumanInputTask extends Task< message, contentSchema: this.config.contentSchema ?? emptySchema, contentData: input.contentData, - expectsResponse: kind === "elicit", + // A confirm expects an answer for the same reason an elicit does; it is + // single-turn because a decision has no follow-up round to negotiate. + expectsResponse: kind === "elicit" || kind === "confirm", mode: kind === "elicit" ? mode : "single", metadata: input.context ? { ...this.config.metadata, ...input.context } diff --git a/packages/test/src/contract/human-connector/assertions/capabilityHonesty.ts b/packages/test/src/contract/human-connector/assertions/capabilityHonesty.ts index fe0059189..55c80e3c7 100644 --- a/packages/test/src/contract/human-connector/assertions/capabilityHonesty.ts +++ b/packages/test/src/contract/human-connector/assertions/capabilityHonesty.ts @@ -24,7 +24,9 @@ function buildReq( ? { message: "elicit", contentSchema: fixture.elicitContentSchema, contentData: undefined } : kind === "notify" ? fixture.notifyRequest - : fixture.displayRequest; + : kind === "confirm" + ? fixture.confirmRequest + : fixture.displayRequest; return { requestId, targetHumanId: "default", @@ -32,7 +34,7 @@ function buildReq( message: base.message, contentSchema: base.contentSchema, contentData: base.contentData, - expectsResponse: kind === "elicit", + expectsResponse: kind === "elicit" || kind === "confirm", mode: "single", metadata: undefined, }; @@ -57,7 +59,7 @@ export function capabilityHonestyBlock( opts.timeout ); - for (const kind of ["notify", "display", "elicit"] as const) { + for (const kind of ["notify", "display", "elicit", "confirm"] as const) { itFn( `${kind}:false implies the connector either throws or surfaces a non-accept action (no silent accept)`, async () => { diff --git a/packages/test/src/contract/human-connector/fixtures.ts b/packages/test/src/contract/human-connector/fixtures.ts index 558eccdda..eea995f77 100644 --- a/packages/test/src/contract/human-connector/fixtures.ts +++ b/packages/test/src/contract/human-connector/fixtures.ts @@ -41,6 +41,20 @@ export const DEFAULT_HUMAN_CONFORMANCE_FIXTURE: ConformanceFixture = { contentSchema: emptySchema, contentData: { result: 42 }, }, + // A confirm's schema describes the action awaiting approval rather than + // fields to fill in, so the data is what a person reads before deciding. + confirmRequest: { + message: 'Run workflow "nightly-export"?', + contentSchema: { + type: "object", + properties: { + action: { type: "string", title: "Action" }, + reaches: { type: "string", title: "Reaches" }, + }, + additionalProperties: true, + }, + contentData: { action: "Run workflow", reaches: "network:http → https://example.test" }, + }, abortGraceMs: 1000, }; diff --git a/packages/test/src/contract/human-connector/types.ts b/packages/test/src/contract/human-connector/types.ts index 0c9f9940a..c1002e75d 100644 --- a/packages/test/src/contract/human-connector/types.ts +++ b/packages/test/src/contract/human-connector/types.ts @@ -26,6 +26,13 @@ export type HumanConnectorAssertionId = export interface HumanConnectorCapabilities { /** Connector handles `kind: "elicit"` requests with a real human-driven response. */ readonly elicit: boolean; + /** + * Connector handles `kind: "confirm"` requests with a real human decision. + * A connector without a native approval primitive may map it onto its + * elicitation path and still declare true — what it must not do is answer + * "accept" without asking anyone. + */ + readonly confirm: boolean; /** Connector handles `kind: "notify"` requests (fast-resolve, no script consumption). */ readonly notify: boolean; /** Connector handles `kind: "display"` requests (fast-resolve, no script consumption). */ @@ -81,6 +88,8 @@ export interface ConformanceFixture { readonly notifyRequest: Pick; /** Default display request payload. */ readonly displayRequest: Pick; + /** Default confirm request payload — the action awaiting approval. */ + readonly confirmRequest: Pick; /** Bound for abort propagation (ms). */ readonly abortGraceMs: number; } diff --git a/packages/test/src/test/human/McpElicitationConnector.conformance.test.ts b/packages/test/src/test/human/McpElicitationConnector.conformance.test.ts index b9cbaba2e..f63ad951e 100644 --- a/packages/test/src/test/human/McpElicitationConnector.conformance.test.ts +++ b/packages/test/src/test/human/McpElicitationConnector.conformance.test.ts @@ -22,6 +22,7 @@ runHumanConnectorConformance({ }, capabilities: { elicit: true, + confirm: true, notify: true, display: true, multiTurn: false, diff --git a/packages/test/src/test/human/MockHumanConnector.conformance.test.ts b/packages/test/src/test/human/MockHumanConnector.conformance.test.ts index 16ec400fe..c987269e9 100644 --- a/packages/test/src/test/human/MockHumanConnector.conformance.test.ts +++ b/packages/test/src/test/human/MockHumanConnector.conformance.test.ts @@ -24,6 +24,7 @@ runHumanConnectorConformance({ }, capabilities: { elicit: true, + confirm: true, notify: true, display: true, multiTurn: true, diff --git a/packages/test/src/test/human/MockHumanConnector_NoFollowUp.conformance.test.ts b/packages/test/src/test/human/MockHumanConnector_NoFollowUp.conformance.test.ts index 270c4f792..27dcc9395 100644 --- a/packages/test/src/test/human/MockHumanConnector_NoFollowUp.conformance.test.ts +++ b/packages/test/src/test/human/MockHumanConnector_NoFollowUp.conformance.test.ts @@ -24,6 +24,7 @@ runHumanConnectorConformance({ }, capabilities: { elicit: true, + confirm: true, notify: true, display: true, multiTurn: false, diff --git a/packages/util/src/human/HumanConnector.ts b/packages/util/src/human/HumanConnector.ts index 500036f18..d716f409e 100644 --- a/packages/util/src/human/HumanConnector.ts +++ b/packages/util/src/human/HumanConnector.ts @@ -15,8 +15,19 @@ import type { DataPortSchema } from "../json-schema/DataPortSchema"; * Response optional (acknowledgment). * - "elicit": Request structured input via a form schema (MCP elicitation). * Response expected with user-submitted data. + * - "confirm": Ask a human to approve or refuse one described action. Response + * expected, and it is a decision rather than data: "accept" means + * go ahead, "decline" means do not. + * + * "confirm" is not an "elicit" with two options. An elicit asks what a value + * should be and its schema describes fields to fill in; a confirm asks whether + * something should happen at all, and what its schema describes is the action + * about to be taken, for the person to read. Collapsing the two loses the + * distinction a renderer needs to draw an approval rather than a form, and + * loses the caller's ability to tell "the user chose nothing" from "the user + * said no". */ -export type HumanInteractionKind = "notify" | "display" | "elicit"; +export type HumanInteractionKind = "notify" | "display" | "elicit" | "confirm"; /** User action in response to an interaction (MCP-aligned for "elicit" kind) */ export type HumanResponseAction = "accept" | "decline" | "cancel"; @@ -43,6 +54,8 @@ export interface IHumanRequest { * For "display": Describes the data/visualization to present. Properties contain * the actual data to render. Use x-ui-viewer annotations for hints. * For "elicit": Describes the form fields for user input (MCP requestedSchema). + * For "confirm": Describes the action awaiting approval — what it is and what + * it will reach — with `contentData` carrying those values. */ readonly contentSchema: DataPortSchema; /** @@ -50,7 +63,7 @@ export interface IHumanRequest { * For "elicit", this is typically empty — the human provides the data. */ readonly contentData: Record | undefined; - /** Whether a response is expected. Default: true for "elicit", false for "notify"/"display". */ + /** Whether a response is expected. Default: true for "elicit" and "confirm", false for "notify"/"display". */ readonly expectsResponse: boolean; /** Interaction mode: single request-response or multi-turn conversation */ readonly mode: "single" | "multi-turn"; diff --git a/packages/util/src/json-schema/ModelAuthoredSchema.ts b/packages/util/src/json-schema/ModelAuthoredSchema.ts new file mode 100644 index 000000000..d30aeb105 --- /dev/null +++ b/packages/util/src/json-schema/ModelAuthoredSchema.ts @@ -0,0 +1,134 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { DataPortSchemaObject } from "./DataPortSchema"; + +/** + * Bounds on a JSON Schema that a language model wrote. + * + * A schema a model authored is model-controlled input, and the sibling guard + * for the other half of that surface already exists: tool-call *arguments* are + * run through `sanitizeToolArgs` before validation. Arguments are bounded by + * the schema; the schema itself is bounded by nothing, and it drives a + * renderer. + */ +export interface ModelAuthoredSchemaLimits { + /** Maximum properties on any one object. */ + readonly maxProperties: number; + /** Maximum nesting depth below the root. */ + readonly maxDepth: number; + /** + * `format` values a model may ask for. + * + * This is the load-bearing one, and it is an allowlist for the same reason + * the entitlement gate is: in this codebase a `format` annotation does not + * merely style a field, it selects a runtime editor and resolves a live + * resource — `"storage:tabular"`, `"knowledge-base"`, `"credential"`. An + * unbounded `format` is therefore the model choosing a resource, and a + * denylist would hand every format added later to whoever asks for it first. + */ + readonly allowedFormats: ReadonlySet; + /** + * Prefixes a `format` may carry, for families whose tail is a free + * parameter — `"model:EmbeddingTask"` names a task, not a resource to open. + */ + readonly allowedFormatPrefixes: readonly string[]; +} + +/** + * Formats safe to hand a model-authored form: presentation and picker hints + * that resolve nothing on their own. Anything naming a stored resource is + * absent, and absent means refused. + */ +export const DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS: ModelAuthoredSchemaLimits = Object.freeze({ + maxProperties: 20, + maxDepth: 3, + allowedFormats: new Set([ + "date", + "date-time", + "time", + "uri", + "email", + "textarea", + "color", + "model", + ]), + allowedFormatPrefixes: ["model:"], +}); + +export type ModelAuthoredSchemaResult = + | { readonly ok: true; readonly schema: DataPortSchemaObject } + | { readonly ok: false; readonly reason: string }; + +function formatAllowed(format: string, limits: ModelAuthoredSchemaLimits): boolean { + if (limits.allowedFormats.has(format)) return true; + return limits.allowedFormatPrefixes.some((prefix) => format.startsWith(prefix)); +} + +function check( + node: unknown, + path: string, + depth: number, + limits: ModelAuthoredSchemaLimits +): string | undefined { + if (!node || typeof node !== "object" || Array.isArray(node)) { + return `${path || "schema"} must be an object schema`; + } + const schema = node as Record; + if (depth > limits.maxDepth) return `nesting depth at ${path} exceeds ${limits.maxDepth}`; + if (typeof schema.format === "string" && !formatAllowed(schema.format, limits)) { + return `format "${schema.format}" at ${path || "root"} is not allowed in a model-authored schema`; + } + if (schema.type === "object" || schema.properties) { + const properties = schema.properties; + if (properties !== undefined) { + if (!properties || typeof properties !== "object" || Array.isArray(properties)) { + return `properties at ${path || "root"} must be an object`; + } + const keys = Object.keys(properties as Record); + if (keys.length > limits.maxProperties) { + return `too many properties at ${path || "root"} (max ${limits.maxProperties})`; + } + for (const key of keys) { + const error = check( + (properties as Record)[key], + path ? `${path}.${key}` : key, + depth + 1, + limits + ); + if (error) return error; + } + } + } + if (schema.items !== undefined) { + const error = check(schema.items, `${path}[]`, depth + 1, limits); + if (error) return error; + } + return undefined; +} + +/** + * Validates a model-authored JSON Schema before it reaches a form renderer. + * + * Returns the reason rather than throwing, because the caller's next move is + * usually to hand that reason back to the model so it can fix the schema — + * a rejection it can act on beats an exception it cannot see. + */ +export function validateModelAuthoredSchema( + schema: unknown, + limits: ModelAuthoredSchemaLimits = DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS +): ModelAuthoredSchemaResult { + if (!schema || typeof schema !== "object" || Array.isArray(schema)) { + return { ok: false, reason: "schema must be a JSON Schema object" }; + } + const root = schema as Record; + if (root.type !== "object" || !root.properties) { + return { ok: false, reason: 'schema root must have type "object" and "properties"' }; + } + const error = check(schema, "", 0, limits); + if (error) return { ok: false, reason: error }; + return { ok: true, schema: schema as DataPortSchemaObject }; +} diff --git a/packages/util/src/json-schema/__tests__/ModelAuthoredSchema.test.ts b/packages/util/src/json-schema/__tests__/ModelAuthoredSchema.test.ts new file mode 100644 index 000000000..398c8e8c8 --- /dev/null +++ b/packages/util/src/json-schema/__tests__/ModelAuthoredSchema.test.ts @@ -0,0 +1,125 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS, + validateModelAuthoredSchema, +} from "@workglow/util/schema"; +import { describe, expect, it } from "vitest"; + +const objectSchema = (properties: Record) => ({ type: "object", properties }); + +describe("validateModelAuthoredSchema", () => { + it("accepts an ordinary object schema", () => { + const result = validateModelAuthoredSchema( + objectSchema({ name: { type: "string", title: "Name" } }) + ); + expect(result.ok).toBe(true); + }); + + it("rejects anything that is not an object schema at the root", () => { + for (const bad of [null, undefined, 42, "s", [], { type: "string" }, { type: "object" }]) { + expect(validateModelAuthoredSchema(bad).ok).toBe(false); + } + }); + + it("rejects a format that resolves a live resource", () => { + // In this codebase `format` selects a runtime editor and resolves a real + // resource, so an unbounded format is the model picking one. + for (const format of ["storage:tabular", "knowledge-base", "credential"]) { + const result = validateModelAuthoredSchema(objectSchema({ f: { type: "string", format } })); + expect(result.ok, format).toBe(false); + if (!result.ok) expect(result.reason).toContain(format); + } + }); + + it("allows a presentation format and a prefixed model format", () => { + expect( + validateModelAuthoredSchema(objectSchema({ d: { type: "string", format: "date" } })).ok + ).toBe(true); + expect( + validateModelAuthoredSchema( + objectSchema({ m: { type: "string", format: "model:EmbeddingTask" } }) + ).ok + ).toBe(true); + }); + + it("refuses a format nobody has allowed, rather than passing it through", () => { + // The allowlist direction: a format added to the taxonomy later is refused + // until someone names it, instead of being served to whoever asks first. + const result = validateModelAuthoredSchema( + objectSchema({ f: { type: "string", format: "future:thing" } }) + ); + expect(result.ok).toBe(false); + }); + + it("bounds property count", () => { + const many = Object.fromEntries( + Array.from({ length: DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS.maxProperties + 1 }, (_, i) => [ + `p${i}`, + { type: "string" }, + ]) + ); + const result = validateModelAuthoredSchema(objectSchema(many)); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toContain("too many properties"); + }); + + it("bounds nesting depth, including through arrays", () => { + const deep = objectSchema({ + a: { + type: "object", + properties: { + b: { + type: "object", + properties: { c: { type: "object", properties: { d: { type: "string" } } } }, + }, + }, + }, + }); + const result = validateModelAuthoredSchema(deep); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toContain("nesting depth"); + + const deepArray = objectSchema({ + a: { + type: "array", + items: { + type: "array", + items: { type: "array", items: { type: "object", properties: {} } }, + }, + }, + }); + expect(validateModelAuthoredSchema(deepArray).ok).toBe(false); + }); + + it("reports the path of the offending node so a model can fix it", () => { + const result = validateModelAuthoredSchema( + objectSchema({ + outer: { type: "object", properties: { inner: { type: "string", format: "credential" } } }, + }) + ); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toContain("outer.inner"); + }); + + it("honours caller-supplied limits", () => { + const strict = { ...DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS, maxProperties: 1 }; + expect(validateModelAuthoredSchema(objectSchema({ a: { type: "string" } }), strict).ok).toBe( + true + ); + expect( + validateModelAuthoredSchema( + objectSchema({ a: { type: "string" }, b: { type: "string" } }), + strict + ).ok + ).toBe(false); + }); + + it("freezes the default limits so the guard cannot be widened at run time", () => { + expect(Object.isFrozen(DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS)).toBe(true); + }); +}); diff --git a/packages/util/src/schema-entry.ts b/packages/util/src/schema-entry.ts index b6de5d183..32a950571 100644 --- a/packages/util/src/schema-entry.ts +++ b/packages/util/src/schema-entry.ts @@ -9,6 +9,7 @@ export * from "./json-schema/DataPortSchema"; export * from "./json-schema/FromSchema"; export * from "./json-schema/JsonSchema"; +export * from "./json-schema/ModelAuthoredSchema"; export * from "./json-schema/SchemaUtils"; export * from "./json-schema/SchemaValidation"; export * from "./json-schema/PartialJsonStream"; From 0b2a2c1aec9f5e3505ec3a6070ffffdcbebcc803 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 23:56:46 +0000 Subject: [PATCH 2/8] test(task-graph): cover the gate's two remaining branches, and document the new modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverage now resolves @workglow/* to src rather than dist, so the approval gate could be measured for the first time: 92.85% branches, with the `?? []` on a malformed declaration and the `instanceof Set` fast path both untested. The Set path is the one worth having — Iterable is the parameter type, but a Set is what a caller reaches for first, and my own tests had switched to arrays. Both are covered now and the module is at 100% on all four metrics. CLAUDE.md gains the three modules added this week, in the per-package sections that already explain why each contract is shaped the way it is: the conversation helpers under @workglow/ai (and why the budget counts characters rather than tokens), the model-authored schema guard under @workglow/util (and why its format allowlist is the load-bearing part), and the TaskGraphJson shape check under @workglow/task-graph (and why it stays structural rather than asking whether a type is runnable). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JEFYAGb9D3mWfyhYkmeAvN --- .claude/CLAUDE.md | 38 +++++++++++++++++++ .../__tests__/EntitlementApproval.test.ts | 19 ++++++++++ 2 files changed, 57 insertions(+) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 9b41ce9d2..73a43071d 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -169,6 +169,13 @@ See `packages/task-graph/README.md` and `src/EXECUTION_MODEL.md`. - `runPreview()` → `executePreview()` — UI previews only, stays PENDING, must be fast - Lifecycle: `PENDING → PROCESSING → COMPLETED | FAILED | ABORTED` +`taskGraphJsonShapeError` / `validateTaskGraphJsonShape` check a `TaskGraphJson` **before** +`createGraphFromGraphJSON` touches it. That function throws from inside its own +construction, in words written for whoever wrote the deserializer — the wrong audience for +graph JSON this process did not author (a file, a request body, a model's output), where the +caller's next move is handing a reason back to whoever supplied it. Structural only: whether +a `type` is runnable is a question about the host's registry, asked separately. + Schemas are JSON Schema. `format` annotations drive runtime type resolution (`"model"`, `"model:EmbeddingTask"`, `"storage:tabular"`, `"knowledge-base"`); `x-ui-manual: true` marks user-added ports. Register classes with `TaskRegistry.registerTask`. @@ -203,6 +210,28 @@ RAG tasks: `ChunkVectorUpsertTask` (`knowledgeBase` + `chunks` + `vector`, optio `method: "similarity" | "hybrid"`), `HierarchyJoinTask`, `RerankerTask`, `QueryExpanderTask`, `TextChunkerTask`, `HierarchicalChunkerTask`. +**Conversation helpers for a host driving multiple turns.** Neither runs inside a task; +both exist because a caller keeping its own message list hits problems the providers cannot +fix for it. + +`normalizeHistoryForModel` / `trimHistoryForModel` (`ChatHistory.ts`). The first repairs +`user, user` and `tool, user`, which strict chat templates reject outright — +HuggingFace's `apply_chat_template` throws `Conversation roles must alternate`, and a host +reaches that state honestly when a stopped turn leaves a trailing user message. The second +caps a list by **characters**, not tokens (this package has no tokenizer where it runs, and +a wrong one is worse than an honest approximation), dropping whole turns from the front and +cutting only at `user` boundaries so no `tool_result` outlives its `tool_use`. It keeps the +newest turn even over budget: the alternative is a conversation erased for being too long. + +`uniquifyToolCallIds` / `repairDuplicateToolCallIds` / `collectToolUseIds` +(`ToolCallIds.ts`). Tool-call ids are unique only within one model run — Gemini restarts at +`call_0` every run, and `Gemini_ToolCalling` already compensates inside its own message +conversion. That fixes what reaches the provider, not what a caller keeps: a host holding +several rounds in one list collides on round two, and the symptom is not a crash but patches +landing on the wrong entry and answers resolving the wrong call. Ids are opaque to providers, +which rebuild their id→name map per run, so renaming both halves of a pair is invisible +downstream. + **Cache checkpoints** — `CacheCheckpointTask` (requires `["cache.checkpoint"]`) warms a prompt prefix and emits a `checkpoint` handle (`format: "cache-checkpoint"`) that `ToolCallingTask` / `TextGenerationTask` / `AiChatTask` accept to send only the tail; @@ -433,6 +462,15 @@ that cannot reach the CDN. `SchemaUtils`/`SchemaValidation`, `uuid4`, `sleep`, `WorkerManager`/`WorkerServer`, vector math, tensor types. +`validateModelAuthoredSchema` (`/schema`) bounds a JSON Schema a **model wrote** before it +reaches a form renderer: property count, nesting depth, and an allowlist of `format` values. +The allowlist is the load-bearing part and is frozen, because in this codebase `format` does +not style a field — it selects a runtime editor and resolves a live resource +(`"storage:tabular"`, `"knowledge-base"`, `"credential"`), so an unbounded `format` is the +model choosing a resource. It is the schema-side counterpart to `sanitizeToolArgs`, which +already hardens the *arguments* half of the same surface. It returns the reason rather than +throwing, since the caller's next move is usually handing that reason back to the model. + `WorkerManager` is written against the web `Worker` interface, and `Worker.node.ts` presents `node:worker_threads` through it. Two mismatches there are silent rather than loud, so leave the adaptation in place: `worker_threads` rejects a **stringified** `file://` URL diff --git a/packages/task-graph/src/task/__tests__/EntitlementApproval.test.ts b/packages/task-graph/src/task/__tests__/EntitlementApproval.test.ts index f3d0e45fe..f4661eb4e 100644 --- a/packages/task-graph/src/task/__tests__/EntitlementApproval.test.ts +++ b/packages/task-graph/src/task/__tests__/EntitlementApproval.test.ts @@ -59,6 +59,16 @@ describe("entitlementsBeyond", () => { expect(beyond.map((e) => e.id)).toEqual(["ai:autonomous-egress"]); }); + it("accepts a Set as well as an array", () => { + // The parameter is an Iterable, and a Set is the shape a caller reaches + // for first; passing one must not re-wrap it into a different answer. + const beyond = entitlementsBeyond( + [{ id: Entitlements.NETWORK_HTTP }, { id: Entitlements.STORAGE_READ }], + new Set([Entitlements.NETWORK_HTTP]) + ); + expect(beyond.map((e) => e.id)).toEqual(["storage:read"]); + }); + it("honours a caller-supplied ambient set", () => { const beyond = entitlementsBeyond( [{ id: Entitlements.NETWORK_HTTP }, { id: Entitlements.STORAGE_READ }], @@ -126,6 +136,15 @@ describe("taskClassNeedsApproval", () => { expect(taskClassNeedsApproval({} as EntitlementDeclaringTaskClass)).toBe(true); }); + it("treats a declaration missing its entitlements array as declaring nothing", () => { + // `entitlements()` is typed to return { entitlements }, but a hand-rolled + // or cast class can return a bare object; reading undefined.length there + // would throw inside the gate. + const malformed = { entitlements: () => ({}) } as unknown as EntitlementDeclaringTaskClass; + expect(taskClassReach(malformed)).toEqual([]); + expect(taskClassNeedsApproval(malformed)).toBe(false); + }); + it("gates a class whose declaration throws rather than letting the gate crash", () => { const throws: EntitlementDeclaringTaskClass = { entitlements: () => { From 452675f90c44c2e5d5d2b136007093a79e60a16e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 00:24:58 +0000 Subject: [PATCH 3/8] fix(ai): state is_error on the tool_result literals in the new tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ContentBlockToolResult` declares `is_error: boolean | undefined` — a required property under the repo's "T | undefined over T?" convention — so an object literal has to name it. Both helpers built one from scratch and omitted it. Vitest transpiles without typechecking, which is why the suite passed and `typecheck:tests` did not. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JEFYAGb9D3mWfyhYkmeAvN --- packages/ai/src/task/__tests__/ChatHistory.test.ts | 9 ++++++++- packages/ai/src/task/__tests__/ToolCallIds.test.ts | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/ai/src/task/__tests__/ChatHistory.test.ts b/packages/ai/src/task/__tests__/ChatHistory.test.ts index a80231fc2..d1abf9e87 100644 --- a/packages/ai/src/task/__tests__/ChatHistory.test.ts +++ b/packages/ai/src/task/__tests__/ChatHistory.test.ts @@ -19,7 +19,14 @@ const assistant = (text: string): ChatMessage => ({ }); const toolResult = (id: string): ChatMessage => ({ role: "tool", - content: [{ type: "tool_result", tool_use_id: id, content: [{ type: "text", text: "ok" }] }], + content: [ + { + type: "tool_result", + tool_use_id: id, + content: [{ type: "text", text: "ok" }], + is_error: undefined, + }, + ], }); describe("normalizeHistoryForModel", () => { diff --git a/packages/ai/src/task/__tests__/ToolCallIds.test.ts b/packages/ai/src/task/__tests__/ToolCallIds.test.ts index b062568d6..465e7bcb3 100644 --- a/packages/ai/src/task/__tests__/ToolCallIds.test.ts +++ b/packages/ai/src/task/__tests__/ToolCallIds.test.ts @@ -20,6 +20,7 @@ const toolResults = (...ids: string[]): ChatMessage => ({ type: "tool_result", tool_use_id: id, content: [{ type: "text", text: "ok" }], + is_error: undefined, })), }); From c1f193ebe90e779b83fb32e60137791014f6621d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 02:11:42 +0000 Subject: [PATCH 4/8] docs: name the new modules in CLAUDE.md without restating their rationale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three additions repeated, in prose, what each module's own JSDoc already says about why it is shaped the way it is. CLAUDE.md is a map — a branch adds where a thing lives, not the argument for it, or the file grows by a section per merge. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JEFYAGb9D3mWfyhYkmeAvN --- .claude/CLAUDE.md | 45 +++++++-------------------------------------- 1 file changed, 7 insertions(+), 38 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 73a43071d..860d30490 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -169,12 +169,8 @@ See `packages/task-graph/README.md` and `src/EXECUTION_MODEL.md`. - `runPreview()` → `executePreview()` — UI previews only, stays PENDING, must be fast - Lifecycle: `PENDING → PROCESSING → COMPLETED | FAILED | ABORTED` -`taskGraphJsonShapeError` / `validateTaskGraphJsonShape` check a `TaskGraphJson` **before** -`createGraphFromGraphJSON` touches it. That function throws from inside its own -construction, in words written for whoever wrote the deserializer — the wrong audience for -graph JSON this process did not author (a file, a request body, a model's output), where the -caller's next move is handing a reason back to whoever supplied it. Structural only: whether -a `type` is runnable is a question about the host's registry, asked separately. +`taskGraphJsonShapeError` / `validateTaskGraphJsonShape` (`TaskGraphJsonShape.ts`) check a +`TaskGraphJson` structurally before `createGraphFromGraphJSON` touches it. Schemas are JSON Schema. `format` annotations drive runtime type resolution (`"model"`, `"model:EmbeddingTask"`, `"storage:tabular"`, `"knowledge-base"`); @@ -210,27 +206,9 @@ RAG tasks: `ChunkVectorUpsertTask` (`knowledgeBase` + `chunks` + `vector`, optio `method: "similarity" | "hybrid"`), `HierarchyJoinTask`, `RerankerTask`, `QueryExpanderTask`, `TextChunkerTask`, `HierarchicalChunkerTask`. -**Conversation helpers for a host driving multiple turns.** Neither runs inside a task; -both exist because a caller keeping its own message list hits problems the providers cannot -fix for it. - -`normalizeHistoryForModel` / `trimHistoryForModel` (`ChatHistory.ts`). The first repairs -`user, user` and `tool, user`, which strict chat templates reject outright — -HuggingFace's `apply_chat_template` throws `Conversation roles must alternate`, and a host -reaches that state honestly when a stopped turn leaves a trailing user message. The second -caps a list by **characters**, not tokens (this package has no tokenizer where it runs, and -a wrong one is worse than an honest approximation), dropping whole turns from the front and -cutting only at `user` boundaries so no `tool_result` outlives its `tool_use`. It keeps the -newest turn even over budget: the alternative is a conversation erased for being too long. - -`uniquifyToolCallIds` / `repairDuplicateToolCallIds` / `collectToolUseIds` -(`ToolCallIds.ts`). Tool-call ids are unique only within one model run — Gemini restarts at -`call_0` every run, and `Gemini_ToolCalling` already compensates inside its own message -conversion. That fixes what reaches the provider, not what a caller keeps: a host holding -several rounds in one list collides on round two, and the symptom is not a crash but patches -landing on the wrong entry and answers resolving the wrong call. Ids are opaque to providers, -which rebuild their id→name map per run, so renaming both halves of a pair is invisible -downstream. +Conversation helpers for a host keeping its own message list, none of which run inside a +task: `normalizeHistoryForModel` / `trimHistoryForModel` (`ChatHistory.ts`), and +`collectToolUseIds` / `uniquifyToolCallIds` / `repairDuplicateToolCallIds` (`ToolCallIds.ts`). **Cache checkpoints** — `CacheCheckpointTask` (requires `["cache.checkpoint"]`) warms a prompt prefix and emits a `checkpoint` handle (`format: "cache-checkpoint"`) that @@ -459,17 +437,8 @@ that cannot reach the CDN. ### `@workglow/util` `EventEmitter`, `ServiceRegistry` (DI), `DirectedAcyclicGraph`, `DataPortSchema`/`JsonSchema`, -`SchemaUtils`/`SchemaValidation`, `uuid4`, `sleep`, `WorkerManager`/`WorkerServer`, vector -math, tensor types. - -`validateModelAuthoredSchema` (`/schema`) bounds a JSON Schema a **model wrote** before it -reaches a form renderer: property count, nesting depth, and an allowlist of `format` values. -The allowlist is the load-bearing part and is frozen, because in this codebase `format` does -not style a field — it selects a runtime editor and resolves a live resource -(`"storage:tabular"`, `"knowledge-base"`, `"credential"`), so an unbounded `format` is the -model choosing a resource. It is the schema-side counterpart to `sanitizeToolArgs`, which -already hardens the *arguments* half of the same surface. It returns the reason rather than -throwing, since the caller's next move is usually handing that reason back to the model. +`SchemaUtils`/`SchemaValidation`, `validateModelAuthoredSchema` (`/schema`), `uuid4`, +`sleep`, `WorkerManager`/`WorkerServer`, vector math, tensor types. `WorkerManager` is written against the web `Worker` interface, and `Worker.node.ts` presents `node:worker_threads` through it. Two mismatches there are silent rather than loud, so leave From 84b31cb42da492059b5f2c461c7413bf7f3e326e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 02:31:59 +0000 Subject: [PATCH 5/8] refactor(ai): move validateModelAuthoredSchema out of util, beside sanitizeToolArgs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two halves of one surface now sit in one directory: sanitizeToolArgs bounds a model's tool-call arguments, validateModelAuthoredSchema bounds the schema those arguments are validated against and a form is rendered from. util was the wrong home for a reason placement alone did not fix. The guard belongs at a trust boundary, and libs owns none that receives a model-authored schema — every such boundary is in a host, which is why it has no caller here either way. Given that, it goes where its sibling is rather than in the foundation package. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JEFYAGb9D3mWfyhYkmeAvN --- .claude/CLAUDE.md | 7 +++++-- .../src/json-schema => ai/src/task}/ModelAuthoredSchema.ts | 6 +++--- .../src/task}/__tests__/ModelAuthoredSchema.test.ts | 5 +---- packages/ai/src/task/index.ts | 1 + packages/util/src/schema-entry.ts | 1 - 5 files changed, 10 insertions(+), 10 deletions(-) rename packages/{util/src/json-schema => ai/src/task}/ModelAuthoredSchema.ts (95%) rename packages/{util/src/json-schema => ai/src/task}/__tests__/ModelAuthoredSchema.test.ts (97%) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 860d30490..64b2944cc 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -210,6 +210,9 @@ Conversation helpers for a host keeping its own message list, none of which run task: `normalizeHistoryForModel` / `trimHistoryForModel` (`ChatHistory.ts`), and `collectToolUseIds` / `uniquifyToolCallIds` / `repairDuplicateToolCallIds` (`ToolCallIds.ts`). +`validateModelAuthoredSchema` (`ModelAuthoredSchema.ts`) bounds a JSON Schema a model wrote +before a host renders a form from it, beside `sanitizeToolArgs`, which bounds the arguments. + **Cache checkpoints** — `CacheCheckpointTask` (requires `["cache.checkpoint"]`) warms a prompt prefix and emits a `checkpoint` handle (`format: "cache-checkpoint"`) that `ToolCallingTask` / `TextGenerationTask` / `AiChatTask` accept to send only the tail; @@ -437,8 +440,8 @@ that cannot reach the CDN. ### `@workglow/util` `EventEmitter`, `ServiceRegistry` (DI), `DirectedAcyclicGraph`, `DataPortSchema`/`JsonSchema`, -`SchemaUtils`/`SchemaValidation`, `validateModelAuthoredSchema` (`/schema`), `uuid4`, -`sleep`, `WorkerManager`/`WorkerServer`, vector math, tensor types. +`SchemaUtils`/`SchemaValidation`, `uuid4`, `sleep`, `WorkerManager`/`WorkerServer`, vector +math, tensor types. `WorkerManager` is written against the web `Worker` interface, and `Worker.node.ts` presents `node:worker_threads` through it. Two mismatches there are silent rather than loud, so leave diff --git a/packages/util/src/json-schema/ModelAuthoredSchema.ts b/packages/ai/src/task/ModelAuthoredSchema.ts similarity index 95% rename from packages/util/src/json-schema/ModelAuthoredSchema.ts rename to packages/ai/src/task/ModelAuthoredSchema.ts index d30aeb105..d29bedfc6 100644 --- a/packages/util/src/json-schema/ModelAuthoredSchema.ts +++ b/packages/ai/src/task/ModelAuthoredSchema.ts @@ -4,14 +4,14 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { DataPortSchemaObject } from "./DataPortSchema"; +import type { DataPortSchemaObject } from "@workglow/util/schema"; /** * Bounds on a JSON Schema that a language model wrote. * * A schema a model authored is model-controlled input, and the sibling guard - * for the other half of that surface already exists: tool-call *arguments* are - * run through `sanitizeToolArgs` before validation. Arguments are bounded by + * for the other half of that surface sits beside it: tool-call *arguments* go + * through {@link sanitizeToolArgs} before validation. Arguments are bounded by * the schema; the schema itself is bounded by nothing, and it drives a * renderer. */ diff --git a/packages/util/src/json-schema/__tests__/ModelAuthoredSchema.test.ts b/packages/ai/src/task/__tests__/ModelAuthoredSchema.test.ts similarity index 97% rename from packages/util/src/json-schema/__tests__/ModelAuthoredSchema.test.ts rename to packages/ai/src/task/__tests__/ModelAuthoredSchema.test.ts index 398c8e8c8..a8e9ce916 100644 --- a/packages/util/src/json-schema/__tests__/ModelAuthoredSchema.test.ts +++ b/packages/ai/src/task/__tests__/ModelAuthoredSchema.test.ts @@ -4,10 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { - DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS, - validateModelAuthoredSchema, -} from "@workglow/util/schema"; +import { DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS, validateModelAuthoredSchema } from "@workglow/ai"; import { describe, expect, it } from "vitest"; const objectSchema = (properties: Record) => ({ type: "object", properties }); diff --git a/packages/ai/src/task/index.ts b/packages/ai/src/task/index.ts index b6397fbcc..86e49279f 100644 --- a/packages/ai/src/task/index.ts +++ b/packages/ai/src/task/index.ts @@ -46,6 +46,7 @@ export * from "./KbReindexTask"; export * from "./KbSearchTask"; export * from "./KbToDocumentsTask"; export * from "./MessageConversion"; +export * from "./ModelAuthoredSchema"; export * from "./ModelDownloadRemoveTask"; export * from "./ModelDownloadTask"; export * from "./ModelInfoTask"; diff --git a/packages/util/src/schema-entry.ts b/packages/util/src/schema-entry.ts index 32a950571..b6de5d183 100644 --- a/packages/util/src/schema-entry.ts +++ b/packages/util/src/schema-entry.ts @@ -9,7 +9,6 @@ export * from "./json-schema/DataPortSchema"; export * from "./json-schema/FromSchema"; export * from "./json-schema/JsonSchema"; -export * from "./json-schema/ModelAuthoredSchema"; export * from "./json-schema/SchemaUtils"; export * from "./json-schema/SchemaValidation"; export * from "./json-schema/PartialJsonStream"; From 291e9474f99e56b43a86b9c11016a7ef58305f17 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 02:36:25 +0000 Subject: [PATCH 6/8] refactor(ai): move ModelAuthoredSchema under task/base Its test moves with it, since base modules keep theirs in base/__tests__. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JEFYAGb9D3mWfyhYkmeAvN --- packages/ai/src/task/{ => base}/ModelAuthoredSchema.ts | 0 .../src/task/{ => base}/__tests__/ModelAuthoredSchema.test.ts | 0 packages/ai/src/task/index.ts | 2 +- 3 files changed, 1 insertion(+), 1 deletion(-) rename packages/ai/src/task/{ => base}/ModelAuthoredSchema.ts (100%) rename packages/ai/src/task/{ => base}/__tests__/ModelAuthoredSchema.test.ts (100%) diff --git a/packages/ai/src/task/ModelAuthoredSchema.ts b/packages/ai/src/task/base/ModelAuthoredSchema.ts similarity index 100% rename from packages/ai/src/task/ModelAuthoredSchema.ts rename to packages/ai/src/task/base/ModelAuthoredSchema.ts diff --git a/packages/ai/src/task/__tests__/ModelAuthoredSchema.test.ts b/packages/ai/src/task/base/__tests__/ModelAuthoredSchema.test.ts similarity index 100% rename from packages/ai/src/task/__tests__/ModelAuthoredSchema.test.ts rename to packages/ai/src/task/base/__tests__/ModelAuthoredSchema.test.ts diff --git a/packages/ai/src/task/index.ts b/packages/ai/src/task/index.ts index 86e49279f..0dda1f80e 100644 --- a/packages/ai/src/task/index.ts +++ b/packages/ai/src/task/index.ts @@ -16,6 +16,7 @@ export * from "./base/AiTask"; export * from "./base/AiTaskSchemas"; export * from "./base/chatTurn"; export * from "./base/CheckpointPorts"; +export * from "./base/ModelAuthoredSchema"; export * from "./base/responseFormat"; export * from "./base/runWithIterable"; export * from "./base/StreamingAiTask"; @@ -46,7 +47,6 @@ export * from "./KbReindexTask"; export * from "./KbSearchTask"; export * from "./KbToDocumentsTask"; export * from "./MessageConversion"; -export * from "./ModelAuthoredSchema"; export * from "./ModelDownloadRemoveTask"; export * from "./ModelDownloadTask"; export * from "./ModelInfoTask"; From 61392c5b6db7c2d43c662bee42c3eab6e1556b06 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 05:01:50 +0000 Subject: [PATCH 7/8] fix: close the holes review found in the five new contracts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The format allowlist was the weakest of the five, and it was bypassable three ways. `check()` walked only `properties` and `items`, so nesting a field under any composition or applicator keyword skipped it — verified by execution for `oneOf`, `prefixItems`, `patternProperties` and `contains`, each of which a renderer resolves on the branch it picks. The walk now visits every keyword that can carry a subschema, distinguishing those describing the same value from those describing a child, and refuses `$ref`/`$defs` outright: a reference points where the walk cannot follow, and a model describing a form has no use for one. Branch keywords sit at their parent's depth because they describe the same value, which left `maxDepth` bounding nothing along a chain of them — a deep `oneOf` chain overflowed the stack, so the function documented to hand back a reason instead threw a RangeError the caller has no catch for. The walk now carries its own ceiling. `Object.freeze` does not reach a Set's members, so the default limits were widenable at run time by anything holding the export while `Object.isFrozen` stayed true — the test asserting otherwise passed. The allowlist is a frozen array now. `repairDuplicateToolCallIds` counted tool_use and tool_result occurrences in two independent maps, which only pairs correctly when every call has exactly one result. An interrupted round left the surviving result answering the abandoned call. Results now match against the calls of the round they follow. MCP has no approval primitive, and forwarding a confirm's contentSchema as the elicitation's requestedSchema turned the action's description into empty inputs for the person to type — with `required`, accept was unreachable. A confirm now sends the details in the message, the only part a client must display, and an empty form; its answer is the decision, so nothing comes back as content. The subgraph recursion added for nested graphs was itself unbounded, in the one function whose purpose is to return a sentence rather than throw from depth. Also: the conformance suite declared the confirm capability and asserted nothing about it, so a connector could claim the kind and silently auto-accept — the failure the capability exists to catch. Three assertions added, with ids so an adapter can mark one known-failing instead of dropping the capability. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JEFYAGb9D3mWfyhYkmeAvN --- packages/ai/src/task/ToolCallIds.ts | 65 ++++++- .../ai/src/task/__tests__/ChatHistory.test.ts | 39 ++-- .../ai/src/task/__tests__/ToolCallIds.test.ts | 43 +++++ .../ai/src/task/base/ModelAuthoredSchema.ts | 170 ++++++++++++++++-- .../__tests__/ModelAuthoredSchema.test.ts | 77 ++++++++ .../mcp/src/tasks/McpElicitationConnector.ts | 80 ++++++++- .../src/task-graph/TaskGraphJsonShape.ts | 22 +++ .../__tests__/TaskGraphJsonShape.test.ts | 44 +++++ packages/tasks/src/task/HumanInputTask.ts | 4 +- .../human-connector/assertions/roundtrip.ts | 70 ++++++++ .../src/contract/human-connector/types.ts | 3 + 11 files changed, 580 insertions(+), 37 deletions(-) diff --git a/packages/ai/src/task/ToolCallIds.ts b/packages/ai/src/task/ToolCallIds.ts index 73efb12d8..73ec917ad 100644 --- a/packages/ai/src/task/ToolCallIds.ts +++ b/packages/ai/src/task/ToolCallIds.ts @@ -64,30 +64,79 @@ function renameForOccurrence(seen: Map, id: string): string { return occurrence === 1 ? id : `${id}_${occurrence}`; } +/** + * Tracks which renamed `tool_use` id each `tool_result` should answer. + * + * Counting the two sides independently only pairs them while every call has + * exactly one result. A history holding a `tool_use` whose result never + * arrived — an interrupted round, a turn stored before the dangling call was + * dropped — puts the counters one apart, and from there every later result for + * that id is renamed onto the wrong call. + * + * Results are matched against the calls of the most recent assistant message + * that made any, because that is what a round is: an assistant turn asking for + * tools, then the results answering it. A second assistant turn asking again + * supersedes anything the first left unanswered. + */ +class ToolCallRenames { + private readonly occurrences = new Map(); + /** The newest rename minted for an id, whichever round made it. */ + private readonly latest = new Map(); + private round = new Map(); + + /** A new assistant turn asked for tools; its calls are what results answer now. */ + beginRound(): void { + this.round = new Map(); + } + + forToolUse(id: string): string { + const renamed = renameForOccurrence(this.occurrences, id); + this.latest.set(id, renamed); + const queue = this.round.get(id); + if (queue) queue.push(renamed); + else this.round.set(id, [renamed]); + return renamed; + } + + /** + * The rename of the oldest call in this round still unanswered. + * + * With none left — a second result for one call, in a history nothing else + * would have produced — the newest rename for that id is the fallback. The + * raw id looks like the safe answer and is the one wrong one: occurrence 1 + * keeps the id unchanged, so returning it silently re-answers the FIRST call + * ever made with that id, several rounds back. An id never renamed at all is + * its own latest, so the ordinary case is unaffected. + */ + forToolResult(id: string): string { + return this.round.get(id)?.shift() ?? this.latest.get(id) ?? id; + } +} + /** * Repairs a stored conversation whose tool-call ids are not unique — one * written before the caller started uniquifying them, say. * * The k-th occurrence of an id becomes `_` consistently across - * `tool_use` and `tool_result` blocks. The two are paired by occurrence order - * rather than by matching ids, which is what keeps a pair together: within one - * conversation the n-th `tool_result` for an id answers the n-th `tool_use` of - * it, because a round's results are appended after its calls. + * `tool_use` and `tool_result` blocks. A result takes the rename of the oldest + * call with that id it has not answered yet, which is what keeps a pair + * together even where a call has no result at all — within one conversation a + * round's results are appended after its calls. * * An already-unique history passes through with only shallow copies. */ export function repairDuplicateToolCallIds(history: readonly ChatMessage[]): ChatMessage[] { - const useSeen = new Map(); - const resultSeen = new Map(); + const renames = new ToolCallRenames(); return history.map((message) => { if (message.role === "assistant") { + if (message.content.some((block) => block.type === "tool_use")) renames.beginRound(); return { ...message, content: message.content.map((block) => block.type === "tool_use" ? ({ ...block, - id: renameForOccurrence(useSeen, block.id), + id: renames.forToolUse(block.id), } satisfies ContentBlockToolUse) : block ), @@ -100,7 +149,7 @@ export function repairDuplicateToolCallIds(history: readonly ChatMessage[]): Cha block.type === "tool_result" ? ({ ...block, - tool_use_id: renameForOccurrence(resultSeen, block.tool_use_id), + tool_use_id: renames.forToolResult(block.tool_use_id), } satisfies ContentBlockToolResult) : block ), diff --git a/packages/ai/src/task/__tests__/ChatHistory.test.ts b/packages/ai/src/task/__tests__/ChatHistory.test.ts index d1abf9e87..40da04087 100644 --- a/packages/ai/src/task/__tests__/ChatHistory.test.ts +++ b/packages/ai/src/task/__tests__/ChatHistory.test.ts @@ -68,18 +68,35 @@ describe("trimHistoryForModel", () => { }); it("cuts only at a user message, so no tool_result outlives its tool_use", () => { - const history = [ - user("turn one"), - assistant("thinking"), - toolResult("t1"), - assistant("answer one"), - user("turn two"), - assistant("answer two"), - ]; - const trimmed = trimHistoryForModel(history, 120); + // The invariant is not "the first message is a user message" — it is that + // every surviving `tool_result` still has the `tool_use` it answers, which + // both Anthropic and OpenAI reject outright when it is missing. + const filler = "z".repeat(400); + const history: ChatMessage[] = []; + for (let i = 0; i < 6; i++) { + history.push(user(`ask ${i}`)); + history.push({ + role: "assistant", + content: [{ type: "tool_use", id: `t${i}`, name: "echo", input: { v: filler } }], + }); + history.push(toolResult(`t${i}`)); + history.push(assistant(`done ${i}`)); + } + const trimmed = trimHistoryForModel(history, 3000); + expect(trimmed.length).toBeLessThan(history.length); expect(trimmed[0]!.role).toBe("user"); - // Whatever survived, a tool message never leads the result. - expect(trimmed.some((m, i) => m.role === "tool" && i === 0)).toBe(false); + const uses = new Set( + trimmed.flatMap((m) => + m.content.filter((b) => b.type === "tool_use").map((b) => (b as { id: string }).id) + ) + ); + const results = trimmed.flatMap((m) => + m.content + .filter((b) => b.type === "tool_result") + .map((b) => (b as { tool_use_id: string }).tool_use_id) + ); + expect(results.length).toBeGreaterThan(0); + for (const id of results) expect(uses.has(id)).toBe(true); }); it("keeps the newest turn even when it alone exceeds the budget", () => { diff --git a/packages/ai/src/task/__tests__/ToolCallIds.test.ts b/packages/ai/src/task/__tests__/ToolCallIds.test.ts index 465e7bcb3..3382c48bd 100644 --- a/packages/ai/src/task/__tests__/ToolCallIds.test.ts +++ b/packages/ai/src/task/__tests__/ToolCallIds.test.ts @@ -90,6 +90,49 @@ describe("repairDuplicateToolCallIds", () => { expect(results).toEqual(["call_0", "call_0_2"]); }); + it("keeps a result on its own call when an earlier call was never answered", () => { + // An interrupted round leaves a `tool_use` with no `tool_result`. Counting + // the two sides independently would rename the surviving result onto the + // abandoned call, silently re-answering the wrong one. + const history = [ + assistantCalls("call_0"), + assistantCalls("call_0"), + toolResults("call_0"), + assistantCalls("call_0"), + toolResults("call_0"), + ]; + const out = repairDuplicateToolCallIds(history); + const results = out.flatMap((m) => + m.role === "tool" + ? m.content.filter((b) => b.type === "tool_result").map((b: any) => b.tool_use_id) + : [] + ); + // The first call is the unanswered one, so the two results belong to the + // second and third. + expect(results).toEqual(["call_0_2", "call_0_3"]); + }); + + it("keeps a stray extra result off the first round's call", () => { + // A second result for one call is a history nothing normal produces, but + // falling back to the raw id would attach it to the FIRST `call_0` — the + // one occurrence 1 left unrenamed — silently re-answering a round that was + // already answered rounds ago. + const history = [ + assistantCalls("call_0"), + toolResults("call_0"), + assistantCalls("call_0"), + toolResults("call_0"), + toolResults("call_0"), + ]; + const out = repairDuplicateToolCallIds(history); + const results = out.flatMap((m) => + m.role === "tool" + ? m.content.filter((b) => b.type === "tool_result").map((b: any) => b.tool_use_id) + : [] + ); + expect(results).toEqual(["call_0", "call_0_2", "call_0_2"]); + }); + it("leaves an already-unique history unchanged", () => { const history = [assistantCalls("a", "b"), toolResults("a", "b")]; expect(repairDuplicateToolCallIds(history)).toEqual(history); diff --git a/packages/ai/src/task/base/ModelAuthoredSchema.ts b/packages/ai/src/task/base/ModelAuthoredSchema.ts index d29bedfc6..765762677 100644 --- a/packages/ai/src/task/base/ModelAuthoredSchema.ts +++ b/packages/ai/src/task/base/ModelAuthoredSchema.ts @@ -29,8 +29,13 @@ export interface ModelAuthoredSchemaLimits { * resource — `"storage:tabular"`, `"knowledge-base"`, `"credential"`. An * unbounded `format` is therefore the model choosing a resource, and a * denylist would hand every format added later to whoever asks for it first. + * + * A frozen array rather than a `ReadonlySet`: a Set keeps its members in + * internal slots, so `Object.freeze` does not reach them and the default + * below could be widened at run time by anything holding a reference to it. + * The list is short enough that a scan costs nothing. */ - readonly allowedFormats: ReadonlySet; + readonly allowedFormats: readonly string[]; /** * Prefixes a `format` may carry, for families whose tail is a free * parameter — `"model:EmbeddingTask"` names a task, not a resource to open. @@ -46,7 +51,7 @@ export interface ModelAuthoredSchemaLimits { export const DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS: ModelAuthoredSchemaLimits = Object.freeze({ maxProperties: 20, maxDepth: 3, - allowedFormats: new Set([ + allowedFormats: Object.freeze([ "date", "date-time", "time", @@ -56,7 +61,7 @@ export const DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS: ModelAuthoredSchemaLimits = O "color", "model", ]), - allowedFormatPrefixes: ["model:"], + allowedFormatPrefixes: Object.freeze(["model:"]), }); export type ModelAuthoredSchemaResult = @@ -64,21 +69,74 @@ export type ModelAuthoredSchemaResult = | { readonly ok: false; readonly reason: string }; function formatAllowed(format: string, limits: ModelAuthoredSchemaLimits): boolean { - if (limits.allowedFormats.has(format)) return true; + if (limits.allowedFormats.includes(format)) return true; return limits.allowedFormatPrefixes.some((prefix) => format.startsWith(prefix)); } -function check( - node: unknown, - path: string, - depth: number, - limits: ModelAuthoredSchemaLimits -): string | undefined { +/** + * Hard ceiling on how deep the walk itself may recurse. + * + * `maxDepth` bounds the value tree, and branch keywords deliberately sit at + * their parent's depth because they describe the same value — so a chain of + * them (`{oneOf:[{oneOf:[…]}]}`) is bounded by nothing and overflows the stack. + * This function is documented to hand a reason back rather than throw, and a + * `RangeError` is neither a reason nor catchable by the model, so the walk + * carries its own ceiling. It is far above anything a real form nests. + */ +const MAX_WALK_DEPTH = 64; + +/** Keywords whose value is a list of subschemas describing a CHILD value. */ +const CHILD_LIST_KEYWORDS = ["prefixItems"] as const; +/** Keywords whose value is a list of subschemas describing the SAME value. */ +const SIBLING_LIST_KEYWORDS = ["allOf", "anyOf", "oneOf"] as const; +/** Keywords whose value is one subschema describing the SAME value. */ +const SIBLING_KEYWORDS = ["not", "if", "then", "else"] as const; +/** Keywords whose value is one subschema describing a CHILD value. */ +const CHILD_KEYWORDS = [ + "additionalProperties", + "unevaluatedProperties", + "unevaluatedItems", + "contains", + "propertyNames", +] as const; +/** Keywords whose value maps a name to a subschema of the SAME value. */ +const SIBLING_MAP_KEYWORDS = ["dependentSchemas"] as const; +/** Keywords whose value maps a name to a subschema of a CHILD value. */ +const CHILD_MAP_KEYWORDS = ["patternProperties"] as const; + +/** + * Keywords that move the schema somewhere this walk cannot follow. + * + * A `$ref` is resolved by the renderer, not here, so a definition the walk + * never visits still selects a runtime editor — the same hole the branch + * keywords opened, one indirection further. Refusing is the allowlist + * direction: a model asked to describe a form has no use for references. + */ +const UNFOLLOWABLE_KEYWORDS = ["$ref", "$dynamicRef", "$defs", "definitions"] as const; + +interface Walk { + readonly path: string; + /** Depth in the VALUE tree, bounded by `limits.maxDepth`. */ + readonly depth: number; + /** Depth of this walk's own recursion, bounded by {@link MAX_WALK_DEPTH}. */ + readonly walkDepth: number; +} + +function check(node: unknown, walk: Walk, limits: ModelAuthoredSchemaLimits): string | undefined { + const { path, depth } = walk; + if (walk.walkDepth > MAX_WALK_DEPTH) { + return `schema at ${path || "root"} nests too many keywords to check`; + } if (!node || typeof node !== "object" || Array.isArray(node)) { return `${path || "schema"} must be an object schema`; } const schema = node as Record; if (depth > limits.maxDepth) return `nesting depth at ${path} exceeds ${limits.maxDepth}`; + for (const keyword of UNFOLLOWABLE_KEYWORDS) { + if (schema[keyword] !== undefined) { + return `"${keyword}" at ${path || "root"} is not allowed in a model-authored schema`; + } + } if (typeof schema.format === "string" && !formatAllowed(schema.format, limits)) { return `format "${schema.format}" at ${path || "root"} is not allowed in a model-authored schema`; } @@ -95,8 +153,7 @@ function check( for (const key of keys) { const error = check( (properties as Record)[key], - path ? `${path}.${key}` : key, - depth + 1, + child(walk, path ? `${path}.${key}` : key), limits ); if (error) return error; @@ -104,9 +161,94 @@ function check( } } if (schema.items !== undefined) { - const error = check(schema.items, `${path}[]`, depth + 1, limits); + const error = check(schema.items, child(walk, `${path}[]`), limits); if (error) return error; } + return checkComposition(schema, walk, limits); +} + +/** A subschema describing a child value: one step down the value tree. */ +function child(walk: Walk, path: string): Walk { + return { path, depth: walk.depth + 1, walkDepth: walk.walkDepth + 1 }; +} + +/** A subschema describing the same value: deeper in the walk, not in the tree. */ +function sibling(walk: Walk, path: string): Walk { + return { path, depth: walk.depth, walkDepth: walk.walkDepth + 1 }; +} + +/** + * Walks the keywords a subschema can hide under. + * + * Checking only `properties` and `items` leaves the format allowlist — the + * load-bearing half of this guard — trivially bypassable: a form renderer that + * understands `oneOf`, `prefixItems` or `patternProperties` resolves the + * `format` on the branch it picks, so a field the guard never looked at still + * selects a runtime editor. Every keyword that can carry a subschema is walked + * for that reason, and the ones this walk cannot follow at all are refused in + * {@link check}. Sibling keywords sit at the node's own depth because they + * describe the same value; the rest describe a child and count as one. + */ +function checkComposition( + schema: Record, + walk: Walk, + limits: ModelAuthoredSchemaLimits +): string | undefined { + const here = walk.path || "root"; + const lists = [ + { keywords: SIBLING_LIST_KEYWORDS, step: sibling }, + { keywords: CHILD_LIST_KEYWORDS, step: child }, + ] as const; + for (const { keywords, step } of lists) { + for (const keyword of keywords) { + const branches = schema[keyword]; + if (branches === undefined) continue; + if (!Array.isArray(branches)) return `${keyword} at ${here} must be an array`; + for (let index = 0; index < branches.length; index++) { + // A boolean subschema carries no `format` and no children. + if (typeof branches[index] === "boolean") continue; + const error = check( + branches[index], + step(walk, `${walk.path}.${keyword}[${index}]`), + limits + ); + if (error) return error; + } + } + } + + const singles = [ + { keywords: SIBLING_KEYWORDS, step: sibling }, + { keywords: CHILD_KEYWORDS, step: child }, + ] as const; + for (const { keywords, step } of singles) { + for (const keyword of keywords) { + const branch = schema[keyword]; + // `additionalProperties: true | false` says nothing about a subschema. + if (branch === undefined || typeof branch === "boolean") continue; + const error = check(branch, step(walk, `${walk.path}.${keyword}`), limits); + if (error) return error; + } + } + + const maps = [ + { keywords: SIBLING_MAP_KEYWORDS, step: sibling }, + { keywords: CHILD_MAP_KEYWORDS, step: child }, + ] as const; + for (const { keywords, step } of maps) { + for (const keyword of keywords) { + const entries = schema[keyword]; + if (entries === undefined) continue; + if (!entries || typeof entries !== "object" || Array.isArray(entries)) { + return `${keyword} at ${here} must be an object`; + } + for (const [name, value] of Object.entries(entries as Record)) { + if (typeof value === "boolean") continue; + const error = check(value, step(walk, `${walk.path}.${keyword}.${name}`), limits); + if (error) return error; + } + } + } return undefined; } @@ -128,7 +270,7 @@ export function validateModelAuthoredSchema( if (root.type !== "object" || !root.properties) { return { ok: false, reason: 'schema root must have type "object" and "properties"' }; } - const error = check(schema, "", 0, limits); + const error = check(schema, { path: "", depth: 0, walkDepth: 0 }, limits); if (error) return { ok: false, reason: error }; return { ok: true, schema: schema as DataPortSchemaObject }; } diff --git a/packages/ai/src/task/base/__tests__/ModelAuthoredSchema.test.ts b/packages/ai/src/task/base/__tests__/ModelAuthoredSchema.test.ts index a8e9ce916..27558d6ce 100644 --- a/packages/ai/src/task/base/__tests__/ModelAuthoredSchema.test.ts +++ b/packages/ai/src/task/base/__tests__/ModelAuthoredSchema.test.ts @@ -116,7 +116,84 @@ describe("validateModelAuthoredSchema", () => { ).toBe(false); }); + it("refuses a resource format hidden under a branch keyword", () => { + // A renderer that understands oneOf/allOf resolves the format on the branch + // it picks, so checking only `properties` and `items` leaves the allowlist + // bypassable by anything that can nest one level sideways. + const branches = [ + { oneOf: [{ type: "string" }, { type: "string", format: "credential" }] }, + { anyOf: [{ type: "string", format: "knowledge-base" }] }, + { allOf: [{ type: "string", format: "storage:tabular" }] }, + { type: "string", if: { format: "credential" } }, + { type: "object", additionalProperties: { type: "string", format: "credential" } }, + ]; + for (const field of branches) { + const result = validateModelAuthoredSchema(objectSchema({ f: field })); + expect(result.ok, JSON.stringify(field)).toBe(false); + } + }); + + it("refuses a resource format hidden under a child keyword the walk once skipped", () => { + // Same hole as the branch keywords, one keyword over: a renderer that + // understands tuples or pattern-keyed maps resolves the format on the + // subschema it picks, so every keyword that can carry one is walked. + const fields = [ + { type: "array", prefixItems: [{ type: "string", format: "credential" }] }, + { type: "object", patternProperties: { "^x": { type: "string", format: "credential" } } }, + { type: "object", propertyNames: { type: "string", format: "credential" } }, + { type: "array", contains: { type: "string", format: "knowledge-base" } }, + { type: "object", dependentSchemas: { a: { type: "string", format: "storage:tabular" } } }, + { type: "object", unevaluatedProperties: { type: "string", format: "credential" } }, + ]; + for (const field of fields) { + expect( + validateModelAuthoredSchema(objectSchema({ f: field })).ok, + JSON.stringify(field) + ).toBe(false); + } + }); + + it("refuses a reference, which points somewhere this guard cannot follow", () => { + const withRef = { + type: "object", + $defs: { secret: { type: "string", format: "credential" } }, + properties: { f: { $ref: "#/$defs/secret" } }, + }; + const result = validateModelAuthoredSchema(withRef); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toContain("$defs"); + }); + + it("returns a reason rather than overflowing on a chain of branch keywords", () => { + // Branch keywords sit at their parent's depth on purpose, so `maxDepth` + // does not bound them; without a ceiling of its own the walk throws a + // RangeError, which is neither a reason nor something a model can act on. + let node: unknown = { type: "string" }; + for (let i = 0; i < 5_000; i++) node = { oneOf: [node] }; + const result = validateModelAuthoredSchema(objectSchema({ f: node })); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toContain("too many keywords"); + }); + + it("still accepts an allowed format inside a branch", () => { + expect( + validateModelAuthoredSchema( + objectSchema({ f: { oneOf: [{ type: "null" }, { type: "string", format: "date" }] } }) + ).ok + ).toBe(true); + }); + it("freezes the default limits so the guard cannot be widened at run time", () => { expect(Object.isFrozen(DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS)).toBe(true); + // Shallow freezing is not enough on its own: whatever holds the allowlist + // has to be frozen too, or anything with a reference can add to it. + expect(Object.isFrozen(DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS.allowedFormats)).toBe(true); + expect(Object.isFrozen(DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS.allowedFormatPrefixes)).toBe(true); + expect(() => + (DEFAULT_MODEL_AUTHORED_SCHEMA_LIMITS.allowedFormats as string[]).push("credential") + ).toThrow(); + expect( + validateModelAuthoredSchema(objectSchema({ f: { type: "string", format: "credential" } })).ok + ).toBe(false); }); }); diff --git a/packages/mcp/src/tasks/McpElicitationConnector.ts b/packages/mcp/src/tasks/McpElicitationConnector.ts index 3520b7f6f..00316f0b5 100644 --- a/packages/mcp/src/tasks/McpElicitationConnector.ts +++ b/packages/mcp/src/tasks/McpElicitationConnector.ts @@ -37,6 +37,46 @@ function toMcpRequestedSchema( }; } +/** The label a person should read for one confirm field: its title, else its key. */ +function confirmLabel(contentSchema: unknown, key: string): string { + const properties = (contentSchema as { properties?: Record } | undefined) + ?.properties; + const property = properties?.[key] as { title?: unknown } | undefined; + return typeof property?.title === "string" && property.title ? property.title : key; +} + +/** `Action: Run workflow` — one line per value a person needs before deciding. */ +function withConfirmDetails( + message: string, + contentData: Record | undefined, + contentSchema: unknown +): string { + const entries = Object.entries(contentData ?? {}); + if (entries.length === 0) return message; + const lines = entries.map(([key, value]) => { + // `JSON.stringify` returns the VALUE undefined for undefined and functions, + // which templates as the string "undefined" — say so deliberately instead. + const rendered = typeof value === "string" ? value : (JSON.stringify(value) ?? String(value)); + return `${confirmLabel(contentSchema, key)}: ${rendered}`; + }); + return message ? `${message}\n\n${lines.join("\n")}` : lines.join("\n"); +} + +/** + * What a confirm asks the client to collect: nothing. + * + * A confirm's `contentSchema` describes the action awaiting approval, not + * fields to fill in. Forwarding it as `requestedSchema` renders those + * descriptions as empty inputs — the person is asked to type "Action" and + * "Reaches" rather than read them, a `required` entry makes accept + * unreachable, and whatever they type comes back as the confirm's `content`. + * The details go in the message; the form itself is empty. + */ +const CONFIRM_REQUESTED_SCHEMA = { + type: "object" as const, + properties: {}, +}; + export interface McpElicitationConnectorOptions { /** * The client request whose handling this elicitation belongs to — a tool @@ -58,10 +98,12 @@ export interface McpElicitationConnectorOptions { /** * IHumanConnector implementation that delegates to MCP Server.elicitInput(). * - * Handles all three interaction kinds: + * Handles every interaction kind: * - "notify": Sends a notification via MCP logging, resolves immediately. * - "display": Sends content for display, resolves immediately. * - "elicit": Delegates to Server.elicitInput() for structured form input. + * - "confirm": The same elicitation, with the action's details folded into the + * message — MCP has no approval primitive. * * The two one-way kinds go out as logging notifications, which the server must * have declared the `logging` capability to send at all — without it the @@ -130,7 +172,7 @@ export class McpElicitationConnector implements IHumanConnector { // than fields to fill in, which a client renders as a form — coarser // than a native approval, but never silently auto-approved. case "confirm": - return this.handleElicit(request, signal); + return this.handleConfirm(request, signal); default: return this.handleElicit(request, signal); @@ -193,6 +235,40 @@ export class McpElicitationConnector implements IHumanConnector { }; } + /** + * Handle "confirm" kind — ask a person to approve one described action. + * + * A confirm carries what it is approving in `contentData`, and + * `elicitInput` has nowhere to put it: form params are a message and a + * requested schema, with no field for values the client should show. So the + * values go into the message, which is the only part of an elicitation a + * client is obliged to display, and the requested schema is left empty — + * see {@link CONFIRM_REQUESTED_SCHEMA} for why forwarding it is worse than + * coarse. + */ + private async handleConfirm( + request: IHumanRequest, + signal: AbortSignal + ): Promise { + const mcpResult: ElicitResult = await this.server.elicitInput( + { + mode: "form", + message: withConfirmDetails(request.message, request.contentData, request.contentSchema), + requestedSchema: CONFIRM_REQUESTED_SCHEMA, + }, + { signal, relatedRequestId: this.options.relatedRequestId } + ); + + // A confirm's answer is the decision. Anything a client sent alongside it + // answers a form this never asked for, so it is not the caller's `content`. + return { + requestId: request.requestId, + action: mcpResult.action, + content: undefined, + done: true, + }; + } + /** * Handle "elicit" kind — request structured input via MCP elicitation. */ diff --git a/packages/task-graph/src/task-graph/TaskGraphJsonShape.ts b/packages/task-graph/src/task-graph/TaskGraphJsonShape.ts index f7e130efb..f84d9fed3 100644 --- a/packages/task-graph/src/task-graph/TaskGraphJsonShape.ts +++ b/packages/task-graph/src/task-graph/TaskGraphJsonShape.ts @@ -21,7 +21,22 @@ import type { TaskGraphJson } from "../task/TaskJSON"; * whether a `type` is a task this host will *run* is a question about the * host's registry, not about the JSON, and it is asked separately. */ +/** + * Ceiling on nested-subgraph recursion. + * + * This function exists so bad graph JSON comes back as a sentence rather than + * a throw from deep inside a constructor, and a chain of nested subgraphs is + * bad graph JSON — deep enough, it overflows the stack and this returns the + * one thing it promised never to. Far above anything a real graph nests. + */ +const MAX_SUBGRAPH_DEPTH = 32; + export function taskGraphJsonShapeError(graph: unknown): string | undefined { + return shapeError(graph, 0); +} + +function shapeError(graph: unknown, depth: number): string | undefined { + if (depth > MAX_SUBGRAPH_DEPTH) return `subgraphs nest deeper than ${MAX_SUBGRAPH_DEPTH}`; if (!graph || typeof graph !== "object" || Array.isArray(graph)) { return "graph must be an object with tasks and dataflows"; } @@ -45,6 +60,13 @@ export function taskGraphJsonShapeError(graph: unknown): string | undefined { ) { return `task "${task.id}" defaults must be an object`; } + // A nested graph is deserialized by the same constructor, so a duplicate id + // or a dangling dataflow inside one throws from exactly as deep as the + // top-level version this function exists to catch. + if (task.subgraph !== undefined) { + const nested = shapeError(task.subgraph, depth + 1); + if (nested) return `task "${task.id}" subgraph: ${nested}`; + } } for (const entry of candidate.dataflows) { diff --git a/packages/task-graph/src/task-graph/__tests__/TaskGraphJsonShape.test.ts b/packages/task-graph/src/task-graph/__tests__/TaskGraphJsonShape.test.ts index 3227003de..519c12da8 100644 --- a/packages/task-graph/src/task-graph/__tests__/TaskGraphJsonShape.test.ts +++ b/packages/task-graph/src/task-graph/__tests__/TaskGraphJsonShape.test.ts @@ -81,6 +81,50 @@ describe("taskGraphJsonShapeError", () => { expect(reason).toContain('dataflow target "b" is not a task id'); }); + it("checks a nested subgraph, naming the task that holds it", () => { + const reason = taskGraphJsonShapeError({ + tasks: [ + { + id: "outer", + type: "GraphAsTask", + subgraph: { + tasks: [ + { id: "a", type: "InputTask" }, + { id: "a", type: "OutputTask" }, + ], + dataflows: [], + }, + }, + ], + dataflows: [], + }); + expect(reason).toContain('task "outer" subgraph'); + expect(reason).toContain('duplicate task id "a"'); + }); + + it("accepts a well-formed subgraph", () => { + expect( + taskGraphJsonShapeError({ + tasks: [ + { + id: "outer", + type: "GraphAsTask", + subgraph: { tasks: [{ id: "a", type: "InputTask" }], dataflows: [] }, + }, + ], + dataflows: [], + }) + ).toBeUndefined(); + }); + + it("reports a reason rather than overflowing on nested subgraphs", () => { + let graph: unknown = { tasks: [], dataflows: [] }; + for (let i = 0; i < 5_000; i++) { + graph = { tasks: [{ id: "g", type: "GraphAsTask", subgraph: graph }], dataflows: [] }; + } + expect(taskGraphJsonShapeError(graph)).toContain("nest deeper than"); + }); + it("catches a dataflow missing an endpoint field", () => { const reason = taskGraphJsonShapeError({ tasks: [{ id: "a", type: "InputTask" }], diff --git a/packages/tasks/src/task/HumanInputTask.ts b/packages/tasks/src/task/HumanInputTask.ts index fde03f4b4..ae2a0b369 100644 --- a/packages/tasks/src/task/HumanInputTask.ts +++ b/packages/tasks/src/task/HumanInputTask.ts @@ -140,7 +140,7 @@ export type HumanInputTaskOutput = { /** * A task that sends an interaction to a human via an IHumanConnector. * - * Supports three interaction kinds: + * Supports four interaction kinds: * - "notify": Send a notification (fire-and-forget, task completes immediately) * - "display": Present content to the human (charts, data, markdown) * - "elicit": Request structured input via a form (MCP elicitation model) @@ -159,7 +159,7 @@ export class HumanInputTask extends Task< static override readonly category = "Human"; public static override title = "Human Input"; public static override description = - "Sends an interaction (notification, display, or input request) to a human"; + "Sends an interaction (notification, display, input request or approval) to a human"; public static override cachePolicy: CachePolicy = { kind: "none" }; public static override hasDynamicSchemas = true; diff --git a/packages/test/src/contract/human-connector/assertions/roundtrip.ts b/packages/test/src/contract/human-connector/assertions/roundtrip.ts index 559945742..e3a279d9f 100644 --- a/packages/test/src/contract/human-connector/assertions/roundtrip.ts +++ b/packages/test/src/contract/human-connector/assertions/roundtrip.ts @@ -28,6 +28,20 @@ function elicitReq(fixture: ConformanceFixture, requestId: string): IHumanReques }; } +function confirmReq(fixture: ConformanceFixture, requestId: string): IHumanRequest { + return { + requestId, + targetHumanId: "default", + kind: "confirm", + message: fixture.confirmRequest.message, + contentSchema: fixture.confirmRequest.contentSchema, + contentData: fixture.confirmRequest.contentData, + expectsResponse: true, + mode: "single", + metadata: undefined, + }; +} + export function roundtripBlock( opts: HumanConnectorConformanceOpts, fixture: ConformanceFixture, @@ -37,6 +51,9 @@ export function roundtripBlock( const itAccept = expectFails.has("roundtrip.accept") ? itExpectFail : it; const itDecline = expectFails.has("roundtrip.decline") ? itExpectFail : it; const itCancel = expectFails.has("roundtrip.cancel") ? itExpectFail : it; + const itConfirmAccept = expectFails.has("roundtrip.confirm.accept") ? itExpectFail : it; + const itConfirmDecline = expectFails.has("roundtrip.confirm.decline") ? itExpectFail : it; + const itConfirmDetails = expectFails.has("roundtrip.confirm.details") ? itExpectFail : it; describe.skipIf(!opts.capabilities.elicit)("Roundtrip elicit", () => { itAccept( @@ -89,4 +106,57 @@ export function roundtripBlock( opts.timeout ); }); + + // Declaring `confirm: true` is a claim that a person decided. Without a + // positive case the honesty block above proves nothing — it returns early + // whenever the capability is true — so a connector that answered "accept" + // without asking anyone would pass the whole suite. + describe.skipIf(!opts.capabilities.confirm)("Roundtrip confirm", () => { + itConfirmAccept( + "a decision reaches the caller as the person's own answer", + async () => { + const { connector, script } = getHandle(); + script.push({ requestId: "x", action: "accept", content: undefined, done: true }); + const ac = new AbortController(); + const res = await connector.send(confirmReq(fixture, "rt-confirm-accept"), ac.signal); + expect(res.requestId).toBe("rt-confirm-accept"); + expect(res.action).toBe("accept"); + expect(res.done).toBe(true); + }, + opts.timeout + ); + + itConfirmDecline( + "a refusal is a refusal, not an unanswered accept", + async () => { + const { connector, script } = getHandle(); + script.push({ requestId: "x", action: "decline", content: undefined, done: true }); + const ac = new AbortController(); + const res = await connector.send(confirmReq(fixture, "rt-confirm-decline"), ac.signal); + expect(res.requestId).toBe("rt-confirm-decline"); + expect(res.action).toBe("decline"); + expect(res.done).toBe(true); + }, + opts.timeout + ); + + itConfirmDetails( + "the action's details reach the person deciding", + async () => { + const { connector, script } = getHandle(); + script.push({ requestId: "x", action: "decline", content: undefined, done: true }); + const ac = new AbortController(); + await connector.send(confirmReq(fixture, "rt-confirm-details"), ac.signal); + const received = script.received.at(-1); + const shown = JSON.stringify({ + message: received?.message, + contentData: received?.contentData, + }); + for (const value of Object.values(fixture.confirmRequest.contentData ?? {})) { + if (typeof value === "string") expect(shown).toContain(value); + } + }, + opts.timeout + ); + }); } diff --git a/packages/test/src/contract/human-connector/types.ts b/packages/test/src/contract/human-connector/types.ts index c1002e75d..3b1da7a69 100644 --- a/packages/test/src/contract/human-connector/types.ts +++ b/packages/test/src/contract/human-connector/types.ts @@ -15,6 +15,9 @@ export type HumanConnectorAssertionId = | "roundtrip.accept" | "roundtrip.decline" | "roundtrip.cancel" + | "roundtrip.confirm.accept" + | "roundtrip.confirm.decline" + | "roundtrip.confirm.details" | "abort.beforeSend" | "abort.midElicit" | "concurrent.isolation" From 9a55f70ee2a16f8712be6fc456d8b409b6463b81 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 05:09:47 +0000 Subject: [PATCH 8/8] fix(task-graph): bind the dataflow ids before interpolating them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `restrict-template-expressions` rejected two messages that interpolate `dataflow.sourceTaskId` / `.targetTaskId` straight off a `Record`. The loop above proves all four keys are strings, but it tests them through a computed key, which narrows nothing at the property accesses below — which is why the sibling `task.id` messages, narrowed by a literal key, were not flagged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JEFYAGb9D3mWfyhYkmeAvN --- .../task-graph/src/task-graph/TaskGraphJsonShape.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/task-graph/src/task-graph/TaskGraphJsonShape.ts b/packages/task-graph/src/task-graph/TaskGraphJsonShape.ts index f84d9fed3..8d05ae1c0 100644 --- a/packages/task-graph/src/task-graph/TaskGraphJsonShape.ts +++ b/packages/task-graph/src/task-graph/TaskGraphJsonShape.ts @@ -76,11 +76,16 @@ function shapeError(graph: unknown, depth: number): string | undefined { for (const key of ["sourceTaskId", "sourceTaskPortId", "targetTaskId", "targetTaskPortId"]) { if (typeof dataflow[key] !== "string") return `dataflow is missing ${key}`; } - if (!ids.has(dataflow.sourceTaskId as string)) { - return `dataflow source "${dataflow.sourceTaskId}" is not a task id`; + // Narrowed by the loop above, which returns for any of the four that is + // not a string. The locals carry that into the messages below, which would + // otherwise interpolate an `unknown`. + const sourceTaskId = dataflow.sourceTaskId as string; + const targetTaskId = dataflow.targetTaskId as string; + if (!ids.has(sourceTaskId)) { + return `dataflow source "${sourceTaskId}" is not a task id`; } - if (!ids.has(dataflow.targetTaskId as string)) { - return `dataflow target "${dataflow.targetTaskId}" is not a task id`; + if (!ids.has(targetTaskId)) { + return `dataflow target "${targetTaskId}" is not a task id`; } } return undefined;