From 3faac7a96d95a424e022bc41244803ba267b9bcc Mon Sep 17 00:00:00 2001 From: ajnart Date: Sun, 2 Aug 2026 14:20:40 +0200 Subject: [PATCH] fix: harden assistant interactive tools --- apps/docs/docs/management/assistant.mdx | 3 +- .../assistant/assistant-auto-approval.spec.ts | 79 ++++++++++++++++++- .../assistant/assistant-auto-approval.tsx | 15 ++-- .../assistant-board-settings-tool.tsx | 9 ++- .../assistant/assistant-human-tool-status.ts | 5 +- .../assistant/assistant-human-tools.spec.ts | 10 ++- .../assistant/assistant-human-tools.tsx | 23 +++++- .../assistant/assistant-provider.tsx | 3 +- packages/translation/src/lang/en.json | 4 + 9 files changed, 134 insertions(+), 17 deletions(-) diff --git a/apps/docs/docs/management/assistant.mdx b/apps/docs/docs/management/assistant.mdx index 2cb9f69c6..f54886bb3 100644 --- a/apps/docs/docs/management/assistant.mdx +++ b/apps/docs/docs/management/assistant.mdx @@ -145,7 +145,8 @@ question. Users can select one of the proposed answers or choose **Other** and e Submitting the answer resumes the same agent run automatically. The minimized activity bar shows the question or approval that needs attention and opens directly back to the request. While an answer, app form, or mutation approval is pending, the composer preserves its draft but does not replace the pending -tool call with a new message. +tool call with a new message. If the provider stops before an interactive tool supplies complete arguments, +Homarr replaces the loading state with an explicit error and confirms that no change was made. App creation uses Homarr's native app form inside the conversation. The form reuses the local icon picker, including automatic matching and searches through the `icon.findIcons` procedure, before the final diff --git a/apps/nextjs/src/components/assistant/assistant-auto-approval.spec.ts b/apps/nextjs/src/components/assistant/assistant-auto-approval.spec.ts index b2656ff7d..0de56119b 100644 --- a/apps/nextjs/src/components/assistant/assistant-auto-approval.spec.ts +++ b/apps/nextjs/src/components/assistant/assistant-auto-approval.spec.ts @@ -1,6 +1,30 @@ -import { describe, expect, test } from "vitest"; +// @vitest-environment jsdom -import { createAssistantAutoApprovalTracker } from "./assistant-auto-approval"; +import { act, createElement } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, describe, expect, test } from "vitest"; + +import { + AssistantAutoApprovalProvider, + createAssistantAutoApprovalTracker, + useAssistantAutoApproval, +} from "./assistant-auto-approval"; + +(globalThis as typeof globalThis & { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +type AutoApproval = ReturnType; +let currentAutoApproval: AutoApproval | null = null; +const containers: HTMLDivElement[] = []; + +const AutoApprovalProbe = () => { + currentAutoApproval = useAssistantAutoApproval(); + return null; +}; + +afterEach(() => { + currentAutoApproval = null; + for (const container of containers.splice(0)) container.remove(); +}); describe("assistant automatic approval tracker", () => { test("claims a tool call only once across duplicate conversation renderers", () => { @@ -21,4 +45,55 @@ describe("assistant automatic approval tracker", () => { tracker.clear(); expect(tracker.claim("app-create-1")).toBe(true); }); + + test("keeps the setting for the same local conversation and resets it on a real switch", async () => { + const container = document.createElement("div"); + containers.push(container); + document.body.append(container); + const root = createRoot(container); + const renderConversation = async (conversationId: string) => { + await act(async () => { + root.render(createElement(AssistantAutoApprovalProvider, { conversationId }, createElement(AutoApprovalProbe))); + }); + }; + + await renderConversation("local-thread-1"); + act(() => currentAutoApproval?.setEnabled(true)); + expect(currentAutoApproval?.enabled).toBe(true); + + await renderConversation("local-thread-1"); + expect(currentAutoApproval?.enabled).toBe(true); + + await renderConversation("local-thread-2"); + expect(currentAutoApproval?.enabled).toBe(false); + + await act(async () => root.unmount()); + }); + + test("clears claimed calls when automatic approvals are turned off", async () => { + const container = document.createElement("div"); + containers.push(container); + document.body.append(container); + const root = createRoot(container); + + await act(async () => { + root.render( + createElement( + AssistantAutoApprovalProvider, + { conversationId: "local-thread-1" }, + createElement(AutoApprovalProbe), + ), + ); + }); + act(() => currentAutoApproval?.setEnabled(true)); + + expect(currentAutoApproval?.requestApproval("app-create-1", () => undefined)).toBe(true); + expect(currentAutoApproval?.requestApproval("app-create-1", () => undefined)).toBe(false); + + act(() => currentAutoApproval?.setEnabled(false)); + act(() => currentAutoApproval?.setEnabled(true)); + expect(currentAutoApproval?.requestApproval("app-create-1", () => undefined)).toBe(true); + + await act(async () => root.unmount()); + }); }); diff --git a/apps/nextjs/src/components/assistant/assistant-auto-approval.tsx b/apps/nextjs/src/components/assistant/assistant-auto-approval.tsx index 962f48eef..af9bd6cbe 100644 --- a/apps/nextjs/src/components/assistant/assistant-auto-approval.tsx +++ b/apps/nextjs/src/components/assistant/assistant-auto-approval.tsx @@ -32,18 +32,23 @@ const AssistantAutoApprovalContext = createContext) => { - const [enabled, setEnabled] = useState(false); +}: PropsWithChildren<{ conversationId: string }>) => { + const [enabled, setEnabledState] = useState(false); const trackerRef = useRef(createAssistantAutoApprovalTracker()); const previousConversationIdRef = useRef(conversationId); + const setEnabled = useCallback((nextEnabled: boolean) => { + if (!nextEnabled) trackerRef.current.clear(); + setEnabledState(nextEnabled); + }, []); + useEffect(() => { const previousConversationId = previousConversationIdRef.current; previousConversationIdRef.current = conversationId; - if (previousConversationId === undefined || previousConversationId === conversationId) return; + if (previousConversationId === conversationId) return; trackerRef.current.clear(); - setEnabled(false); + setEnabledState(false); }, [conversationId]); const requestApproval = useCallback( @@ -61,7 +66,7 @@ export const AssistantAutoApprovalProvider = ({ [enabled], ); - const value = useMemo(() => ({ enabled, setEnabled, requestApproval }), [enabled, requestApproval]); + const value = useMemo(() => ({ enabled, setEnabled, requestApproval }), [enabled, requestApproval, setEnabled]); return {children}; }; diff --git a/apps/nextjs/src/components/assistant/assistant-board-settings-tool.tsx b/apps/nextjs/src/components/assistant/assistant-board-settings-tool.tsx index b02708691..c8d198673 100644 --- a/apps/nextjs/src/components/assistant/assistant-board-settings-tool.tsx +++ b/apps/nextjs/src/components/assistant/assistant-board-settings-tool.tsx @@ -44,7 +44,8 @@ import { useI18n, useScopedI18n } from "@homarr/translation/client"; import { boardSavePartialSettingsSchema } from "@homarr/validation/board"; import { getChangedBoardSettings, getCustomCssWarnings } from "./assistant-board-settings"; -import { hasCompleteAssistantToolArguments } from "./assistant-human-tool-status"; +import { hasCompleteAssistantToolArguments, hasFailedAssistantToolArguments } from "./assistant-human-tool-status"; +import { AssistantHumanToolError } from "./assistant-human-tools"; import classes from "./assistant-panel.module.css"; import type { AssistantBoardSettingsChanges, @@ -116,10 +117,14 @@ export const AssistantConfigureBoardSettingsTool = ({ ); } - if (!hasCompleteArguments || !args?.boardId || !args.boardName) { + if (hasFailedAssistantToolArguments(status)) return ; + + if (!hasCompleteArguments) { return ; } + if (!args?.boardId || !args.boardName) return ; + if (settings.isError) { return ( }> diff --git a/apps/nextjs/src/components/assistant/assistant-human-tool-status.ts b/apps/nextjs/src/components/assistant/assistant-human-tool-status.ts index 9141e62a3..f6b90e8f6 100644 --- a/apps/nextjs/src/components/assistant/assistant-human-tool-status.ts +++ b/apps/nextjs/src/components/assistant/assistant-human-tool-status.ts @@ -1,4 +1,7 @@ import type { ToolCallMessagePartStatus } from "@assistant-ui/react"; export const hasCompleteAssistantToolArguments = (status: ToolCallMessagePartStatus | undefined) => - status?.type !== "running"; + status?.type !== "running" && status?.type !== "incomplete"; + +export const hasFailedAssistantToolArguments = (status: ToolCallMessagePartStatus | undefined) => + status?.type === "incomplete"; diff --git a/apps/nextjs/src/components/assistant/assistant-human-tools.spec.ts b/apps/nextjs/src/components/assistant/assistant-human-tools.spec.ts index 24766e415..3f2fcd0d2 100644 --- a/apps/nextjs/src/components/assistant/assistant-human-tools.spec.ts +++ b/apps/nextjs/src/components/assistant/assistant-human-tools.spec.ts @@ -2,7 +2,7 @@ import { describe, expect, test } from "vitest"; import { appManageSchema } from "@homarr/validation/app"; -import { hasCompleteAssistantToolArguments } from "./assistant-human-tool-status"; +import { hasCompleteAssistantToolArguments, hasFailedAssistantToolArguments } from "./assistant-human-tool-status"; import { getAssistantAppFormValues } from "./assistant-human-tools"; describe("assistant human tool forms", () => { @@ -10,6 +10,14 @@ describe("assistant human tool forms", () => { expect(hasCompleteAssistantToolArguments({ type: "running" })).toBe(false); expect(hasCompleteAssistantToolArguments({ type: "complete" })).toBe(true); expect(hasCompleteAssistantToolArguments({ type: "requires-action", reason: "interrupt" })).toBe(true); + expect(hasCompleteAssistantToolArguments({ type: "incomplete", reason: "error" })).toBe(false); + expect(hasFailedAssistantToolArguments({ type: "incomplete", reason: "error" })).toBe(true); + }); + + test("treats cancelled and truncated tool arguments as failed instead of loading forever", () => { + expect(hasFailedAssistantToolArguments({ type: "incomplete", reason: "cancelled" })).toBe(true); + expect(hasFailedAssistantToolArguments({ type: "incomplete", reason: "length" })).toBe(true); + expect(hasFailedAssistantToolArguments({ type: "running" })).toBe(false); }); test("waits for streamed app arguments before initializing the form", () => { diff --git a/apps/nextjs/src/components/assistant/assistant-human-tools.tsx b/apps/nextjs/src/components/assistant/assistant-human-tools.tsx index ecb006718..5b3b8bf92 100644 --- a/apps/nextjs/src/components/assistant/assistant-human-tools.tsx +++ b/apps/nextjs/src/components/assistant/assistant-human-tools.tsx @@ -2,8 +2,8 @@ import { useEffect, useRef, useState } from "react"; import type { ToolCallMessagePartProps, ToolCallMessagePartStatus } from "@assistant-ui/react"; -import { Box, Button, Group, Skeleton, Stack, Text, TextInput, ThemeIcon } from "@mantine/core"; -import { IconCheck, IconMessageQuestion, IconPencil, IconX } from "@tabler/icons-react"; +import { Alert, Box, Button, Group, Skeleton, Stack, Text, TextInput, ThemeIcon } from "@mantine/core"; +import { IconAlertTriangle, IconCheck, IconMessageQuestion, IconPencil, IconX } from "@tabler/icons-react"; import type { z } from "zod/v4"; import { AppForm } from "@homarr/forms-collection"; @@ -11,7 +11,7 @@ import { useScopedI18n } from "@homarr/translation/client"; import type { appManageSchema } from "@homarr/validation/app"; import classes from "./assistant-panel.module.css"; -import { hasCompleteAssistantToolArguments } from "./assistant-human-tool-status"; +import { hasCompleteAssistantToolArguments, hasFailedAssistantToolArguments } from "./assistant-human-tool-status"; import { normalizeAssistantAppIconUrl } from "./assistant-tool-contracts"; import type { AskUserArgs, AskUserResult, ConfigureAppArgs } from "./assistant-tool-contracts"; @@ -30,6 +30,15 @@ export const getAssistantAppFormValues = ( status: ToolCallMessagePartStatus | undefined, ) => (hasCompleteAssistantToolArguments(status) ? toAssistantAppValues(args) : null); +export const AssistantHumanToolError = () => { + const t = useScopedI18n("common.assistant.toolPreparationError"); + return ( + }> + {t("description")} + + ); +}; + export const AssistantAskUserTool = ({ args, result, @@ -65,7 +74,9 @@ export const AssistantAskUserTool = ({ ); } - if (!hasCompleteAssistantToolArguments(status) || !args?.question || options.length < 2) { + if (hasFailedAssistantToolArguments(status)) return ; + + if (!hasCompleteAssistantToolArguments(status)) { return ( @@ -77,6 +88,8 @@ export const AssistantAskUserTool = ({ ); } + if (!args?.question || options.length < 2) return ; + const submitResult = (answer: AskUserResult) => { if (submitting) return; setSubmitting(true); @@ -212,6 +225,8 @@ export const AssistantConfigureAppTool = ({ ); } + if (hasFailedAssistantToolArguments(status)) return ; + if (initialValues === null) { return ( diff --git a/apps/nextjs/src/components/assistant/assistant-provider.tsx b/apps/nextjs/src/components/assistant/assistant-provider.tsx index 7070f6299..d77362e6f 100644 --- a/apps/nextjs/src/components/assistant/assistant-provider.tsx +++ b/apps/nextjs/src/components/assistant/assistant-provider.tsx @@ -535,7 +535,8 @@ const EnabledAssistantProvider = ({ children }: PropsWithChildren) => { const aui = useAui(); const preferences = useAssistantPreferences(); const messages = useAuiState((state) => state.thread.messages); - const conversationId = useAuiState((state) => state.threadListItem.remoteId); + // The local id is stable while a new thread is initialized and changes for every real thread switch. + const conversationId = useAuiState((state) => state.threadListItem.id); const isRunning = useAuiState((state) => state.thread.isRunning); const isLoading = useAuiState((state) => state.thread.isLoading); const latestAssistantMessage = getLatestMessage(messages, "assistant"); diff --git a/packages/translation/src/lang/en.json b/packages/translation/src/lang/en.json index 31db48655..0ca29a2ec 100644 --- a/packages/translation/src/lang/en.json +++ b/packages/translation/src/lang/en.json @@ -2129,6 +2129,10 @@ "description": "Try again. If the problem continues, ask an administrator to verify the configured model endpoint.", "retry": "Try again" }, + "toolPreparationError": { + "title": "This request could not be prepared", + "description": "The model stopped before it supplied complete details. Retry the response or describe the request again. No change was made." + }, "busy": { "title": "The assistant is already working", "description": "Wait for the current response or open it to stop the run."