diff --git a/agent/tests/test_write_confirmation.py b/agent/tests/test_write_confirmation.py index 0f84d702..aa131d67 100644 --- a/agent/tests/test_write_confirmation.py +++ b/agent/tests/test_write_confirmation.py @@ -5,6 +5,71 @@ import write_confirmation from langchain_core.tools import StructuredTool from langchain_mcp_adapters.interceptors import MCPToolCallRequest +from write_confirmation import summarize_args + + +def test_summarize_args_omits_empty_values(): + fields = summarize_args( + { + "name": "OpenTag", + "color": "", + "id": None, + "labels": [], + "metadata": {}, + } + ) + + assert fields == [{"label": "Name", "value": "OpenTag"}] + + +def test_summarize_args_keeps_zero_and_false(): + fields = summarize_args({"priority": 0, "archived": False}) + + assert fields == [ + {"label": "Priority", "value": "0"}, + {"label": "Archived", "value": "No"}, + ] + + +def test_summarize_args_humanizes_camel_case_and_snake_case_keys(): + fields = summarize_args({"addTeams": ["CopilotKit"], "due_date": "2026-08-03"}) + + assert [f["label"] for f in fields] == ["Add teams", "Due date"] + + +def test_summarize_args_joins_lists_and_compacts_nested_values(): + fields = summarize_args( + { + "teams": ["CopilotKit", "Growth"], + "lead": {"email": "jerel@copilotkit.ai"}, + } + ) + + assert fields[0] == {"label": "Teams", "value": "CopilotKit, Growth"} + assert fields[1] == { + "label": "Lead", + "value": '{"email": "jerel@copilotkit.ai"}', + } + + +def test_summarize_args_truncates_an_overlong_value(): + fields = summarize_args({"description": "x" * 400}) + + assert len(fields) == 1 + assert fields[0]["value"] == "x" * 300 + "…" + + +def test_summarize_args_caps_rows_and_notes_the_overflow(): + fields = summarize_args({f"field{i}": f"value{i}" for i in range(15)}) + + assert len(fields) == 13 + assert fields[12] == {"label": "…", "value": "3 more fields"} + + +def test_summarize_args_renders_booleans_as_yes_and_no(): + fields = summarize_args({"notify": True}) + + assert fields == [{"label": "Notify", "value": "Yes"}] def test_write_confirmation_emits_the_copilotkit_interrupt_envelope(monkeypatch): @@ -41,7 +106,7 @@ async def handler(request): "action": "confirm_write", "args": { "action": "Create issue", - "detail": '{"title": "Checkout 500s"}', + "fields": [{"label": "Title", "value": "Checkout 500s"}], }, } @@ -121,7 +186,7 @@ async def handler(request): "action": "confirm_write", "args": { "action": "Create issue", - "detail": '{"title": "Checkout 500s"}', + "fields": [{"label": "Title", "value": "Checkout 500s"}], }, } ] diff --git a/agent/write_confirmation.py b/agent/write_confirmation.py index 0dbba5e9..e7db6d11 100644 --- a/agent/write_confirmation.py +++ b/agent/write_confirmation.py @@ -1,6 +1,7 @@ """Approval enforcement for mutating MCP tools.""" import json +import re from copilotkit.langgraph import copilotkit_interrupt from langchain_core.tools import BaseTool @@ -11,6 +12,70 @@ from mcp.types import CallToolResult, TextContent +# Max rows rendered in the confirmation table; the rest are counted in a note. +_MAX_FIELDS = 12 + +# Longest value rendered in a cell before it is elided. +_MAX_VALUE = 300 + + +def _humanize(key: str) -> str: + """`addTeams` / `due_date` -> `Add teams` / `Due date`.""" + spaced = re.sub(r"(?<=[a-z0-9])(?=[A-Z])", " ", key) + spaced = spaced.replace("_", " ").replace("-", " ").strip() + words = spaced.split() + if not words: + return key + first, *rest = words + return " ".join([first[:1].upper() + first[1:], *(w.lower() for w in rest)]) + + +def _is_empty(value) -> bool: + """Carries no information for an approver. + + Deliberately not Python falsiness: `0` is a real Linear priority ("No + priority") and `False` a real flag value, so both must survive. + """ + if value is None: + return True + return isinstance(value, (str, list, tuple, dict, set)) and len(value) == 0 + + +def _stringify(value) -> str: + if isinstance(value, bool): + return "Yes" if value else "No" + if isinstance(value, (list, tuple)): + return ", ".join(str(v) for v in value) + if isinstance(value, dict): + return json.dumps(value, ensure_ascii=False, default=str) + return str(value) + + +def summarize_args(args: dict) -> list[dict]: + """Render mutating-tool args as approver-readable `{label, value}` rows. + + Empty values are dropped so the two or three fields that matter aren't + buried among defaults, and the row count is capped so Slack doesn't collapse + the card behind "Show more" — an approver who can't read the payload can't + meaningfully approve it. + """ + fields = [] + for key, value in args.items(): + if _is_empty(value): + continue + text = _stringify(value) + if len(text) > _MAX_VALUE: + text = text[:_MAX_VALUE] + "…" + fields.append({"label": _humanize(key), "value": text}) + + if len(fields) > _MAX_FIELDS: + hidden = len(fields) - _MAX_FIELDS + fields = fields[:_MAX_FIELDS] + fields.append({"label": "…", "value": f"{hidden} more fields"}) + + return fields + + class WriteConfirmationInterceptor: """Require approval for every MCP tool not marked read-only.""" @@ -39,15 +104,9 @@ async def __call__( action = request.name.replace("_", " ").replace("-", " ").strip() action = action[:1].upper() + action[1:] - detail = json.dumps( - request.args, - ensure_ascii=False, - sort_keys=True, - default=str, - ) _answer, response = copilotkit_interrupt( action="confirm_write", - args={"action": action, "detail": detail}, + args={"action": action, "fields": summarize_args(request.args)}, ) if isinstance(response, str): diff --git a/app/channel.test.ts b/app/channel.test.ts index fc2d492a..69ed33dc 100644 --- a/app/channel.test.ts +++ b/app/channel.test.ts @@ -334,6 +334,52 @@ describe("createOpenTagChannel", () => { expect(JSON.stringify(blocks)).toContain("CPK-9: Checkout 500s"); }); + it("renders the interrupt's fields as a table on the posted card", async () => { + const envelope = { + __copilotkit_interrupt_value__: { + action: "confirm_write", + args: { + action: "Save project", + fields: [ + { label: "Name", value: "OpenTag" }, + { label: "Lead", value: "jerel@copilotkit.ai" }, + ], + }, + }, + __copilotkit_messages__: [], + }; + const agent = new FakeAgent([ + (subscriber) => { + subscriber.onCustomEvent?.({ + event: { + type: EventType.CUSTOM, + name: "on_interrupt", + value: JSON.stringify(envelope), + }, + } as never); + }, + ]); + const { adapter, channel } = makeChannel({ agent }); + + await channel.ɵruntime.start(); + await adapter.getSink().onTurn({ + conversationKey: "c1", + replyTarget: {}, + userText: "save it", + platform: "slack", + }); + + expect(adapter.posted).toHaveLength(1); + const { blocks } = renderSlackMessage(adapter.posted[0]!); + const table = blocks.find((b) => b.type === "table") as + | { rows: { text: string }[][] } + | undefined; + expect(table?.rows.map((row) => row.map((cell) => cell.text))).toEqual([ + ["Name", "OpenTag"], + ["Lead", "jerel@copilotkit.ai"], + ]); + }); + it("rejects malformed confirm_write interrupt payloads", async () => { const consoleError = vi .spyOn(console, "error") diff --git a/app/channel.tsx b/app/channel.tsx index c7771e69..3b6168e6 100644 --- a/app/channel.tsx +++ b/app/channel.tsx @@ -64,6 +64,7 @@ export function createOpenTagChannel( await thread.post( , ); diff --git a/app/human-in-the-loop/__tests__/confirm-write.test.tsx b/app/human-in-the-loop/__tests__/confirm-write.test.tsx index 8be2f8f7..f43852f8 100644 --- a/app/human-in-the-loop/__tests__/confirm-write.test.tsx +++ b/app/human-in-the-loop/__tests__/confirm-write.test.tsx @@ -84,7 +84,7 @@ describe("ConfirmWrite", () => { | { elements: { text: string }[] } | undefined; expect(context?.elements[0]?.text).toContain( - "Nothing is written until you click", + "Nothing is changed until you click", ); // "Create" is authored as Markdown bold (`**Create**`) so the IR→mrkdwn // transform renders it as Slack bold (`*Create*`), matching the old card. @@ -106,6 +106,135 @@ describe("ConfirmWrite", () => { expect(blocks.some((b) => b.type === "section")).toBe(false); }); + it("labels the confirm button with the action's own verb", () => { + const ir = renderToIR(); + const { blocks } = renderSlackMessage(ir); + + const actions = blocks.find((b) => b.type === "actions") as + | { elements: { text: { text: string } }[] } + | undefined; + expect(actions?.elements.map((e) => e.text.text)).toEqual([ + "Delete", + "Cancel", + ]); + }); + + it("does not label both buttons the same when the verb collides with Cancel", () => { + const ir = renderToIR(); + const { blocks } = renderSlackMessage(ir); + + const actions = blocks.find((b) => b.type === "actions") as + | { elements: { text: { text: string } }[] } + | undefined; + const labels = actions?.elements.map((e) => e.text.text); + + // "Cancel subscription" would otherwise render Cancel/Cancel, where one of + // the two identical buttons destroys the subscription. + expect(labels).toEqual(["Confirm", "Cancel"]); + expect(new Set(labels).size).toBe(2); + + // Relabelling must not cost the destructive styling — the action is still + // a cancellation, whatever the button ends up reading. + const styled = blocks.find((b) => b.type === "actions") as + | { elements: { style?: string }[] } + | undefined; + expect(styled?.elements[0]?.style).toBe("danger"); + }); + + it("styles a destructive action's confirm button as dangerous", () => { + const ir = renderToIR(); + const { blocks } = renderSlackMessage(ir); + + const actions = blocks.find((b) => b.type === "actions") as + | { elements: { style?: string }[] } + | undefined; + // The destructive button carries the warning colour; Cancel becomes the + // neutral escape hatch rather than the red one. + expect(actions?.elements[0]?.style).toBe("danger"); + expect(actions?.elements[1]?.style).toBeUndefined(); + }); + + it("names the derived verb in the lock context", () => { + const ir = renderToIR(); + const { blocks } = renderSlackMessage(ir); + + const context = blocks.find((b) => b.type === "context") as + | { elements: { text: string }[] } + | undefined; + expect(context?.elements[0]?.text).toContain("*Delete*"); + expect(context?.elements[0]?.text).not.toContain("Create"); + }); + + it("renders fields as a headerless Slack table", () => { + const ir = renderToIR( + , + ); + const { blocks } = renderSlackMessage(ir); + + const table = blocks.find((b) => b.type === "table") as + | { rows: { text: string }[][]; column_settings?: unknown } + | undefined; + expect(table).toBeDefined(); + + // No `columns` prop, so no header row is emitted — the first row is data. + expect(table?.column_settings).toBeUndefined(); + expect(table?.rows.map((row) => row.map((cell) => cell.text))).toEqual([ + ["Name", "OpenTag"], + ["Description", "Project for OpenTag work."], + ]); + }); + + it("prefers the fields table over a legacy detail string", () => { + const ir = renderToIR( + , + ); + const { blocks } = renderSlackMessage(ir); + + expect(blocks.some((b) => b.type === "table")).toBe(true); + expect(JSON.stringify(blocks)).not.toContain('{\\"name\\"'); + }); + + it("falls back to the detail section when fields is empty", () => { + const ir = renderToIR( + , + ); + const { blocks } = renderSlackMessage(ir); + + expect(blocks.some((b) => b.type === "table")).toBe(false); + const section = blocks.find((b) => b.type === "section") as + | { text: { text: string } } + | undefined; + expect(section?.text.text).toContain("CPK-9"); + }); + + it("renders fields as a Teams Adaptive Card table without a header row", () => { + const card = renderAdaptiveCard( + renderToIR( + , + ), + ); + + const table = ( + card.body as { type: string; firstRowAsHeader?: boolean }[] + ).find((el) => el.type === "Table"); + expect(table).toBeDefined(); + expect(table?.firstRowAsHeader).toBe(false); + expect(JSON.stringify(card)).toContain("OpenTag"); + }); + it("renders Create and Cancel actions as a Teams Adaptive Card", () => { const card = renderAdaptiveCard( renderToIR( diff --git a/app/human-in-the-loop/confirm-write.tsx b/app/human-in-the-loop/confirm-write.tsx index 065b249e..f5f5cd5c 100644 --- a/app/human-in-the-loop/confirm-write.tsx +++ b/app/human-in-the-loop/confirm-write.tsx @@ -20,13 +20,31 @@ import { Context, Actions, Button, + Table, + Row, + Cell, } from "@copilotkit/channels"; import type { InteractionContext } from "@copilotkit/channels"; +/** One argument of the pending write, already labelled and stringified. */ +export interface ConfirmWriteField { + label: string; + value: string; +} + interface ConfirmWriteProps { /** Short imperative title of the write, e.g. 'Create Linear issue'. */ action: string; - /** The specifics being approved — issue title + one-line description, etc. */ + /** + * The write's arguments as approver-readable rows, rendered as a table. The + * agent decides which fields are worth showing (see `summarize_args`); this + * component only decides how they look. + */ + fields?: ConfirmWriteField[]; + /** + * Legacy single-string summary. Superseded by `fields`, but still rendered + * when an agent revision predating `fields` is what sent the interrupt. + */ detail?: string; } @@ -59,16 +77,80 @@ async function resumeOrShowFailure( } } -export function ConfirmWrite({ action, detail }: ConfirmWriteProps) { +/** + * The write's arguments, as a headerless two-column table. No `columns` prop is + * passed, so neither renderer emits a header row — "Field | Value" would spend a + * row restating what the layout already says. Slack renders cells as `raw_text`, + * so labels cannot be bolded here. + */ +function fieldTable(fields: ConfirmWriteField[]) { + return ( + + {fields.map((field) => ( + + {field.label} + {field.value} + + ))} +
+ ); +} + +/** + * Verbs whose confirmation must not look inviting. A destructive action gets the + * warning colour on its confirm button, and Cancel drops to neutral so the red + * on the card marks the irreversible choice rather than the safe one. + */ +const DESTRUCTIVE = new Set(["delete", "remove", "archive", "cancel", "revoke"]); + +/** The label of the button that declines the write. */ +const DECLINE_LABEL = "Cancel"; + +/** The action's own leading verb, e.g. `Delete customer` -> `Delete`. */ +function verbOf(action: string): string { + const word = action.trim().split(/\s+/)[0] ?? ""; + if (!word) return ""; + return word[0]!.toUpperCase() + word.slice(1); +} + +/** + * How the confirm button reads. Normally the action's own verb, so the button + * never claims a delete is a create. + * + * Falls back to `Confirm` when the verb would collide with the decline button: + * `Cancel subscription` otherwise renders two buttons both reading "Cancel", + * one cancelling the subscription and one cancelling the request. Derived + * separately from `destructive` below, so relabelling never costs the styling. + */ +function confirmLabel(verb: string): string { + if (!verb) return "Confirm"; + return verb.toLowerCase() === DECLINE_LABEL.toLowerCase() ? "Confirm" : verb; +} + +export function ConfirmWrite({ action, fields, detail }: ConfirmWriteProps) { + const body = fields?.length + ? fieldTable(fields) + : detail + ?
{detail}
+ : null; + + const verb = verbOf(action); + const label = confirmLabel(verb); + // Read from the action's real verb, never from `label` — a relabelled + // destructive action is still destructive. + const destructive = DESTRUCTIVE.has(verb.toLowerCase()); + return (
{`📝 ${action}?`}
- {detail ?
{detail}
: null} - {"🔒 Nothing is written until you click **Create**."} + {body} + + {`🔒 Nothing is changed until you click **${label}**.`} +