fix: harden assistant interactive tools
This commit is contained in:
1 parent
38440a2e57
commit
3faac7a96d
9 files changed
+134
-17
No files matched your search
@@ -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
|
||||
|
||||
@@ -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<typeof useAssistantAutoApproval>;
|
||||
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());
|
||||
});
|
||||
});
|
||||
@@ -32,18 +32,23 @@ const AssistantAutoApprovalContext = createContext<AssistantAutoApprovalContextV
|
||||
export const AssistantAutoApprovalProvider = ({
|
||||
children,
|
||||
conversationId,
|
||||
}: PropsWithChildren<{ conversationId: string | undefined }>) => {
|
||||
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 <AssistantAutoApprovalContext.Provider value={value}>{children}</AssistantAutoApprovalContext.Provider>;
|
||||
};
|
||||
|
||||
@@ -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 <AssistantHumanToolError />;
|
||||
|
||||
if (!hasCompleteArguments) {
|
||||
return <BoardSettingsSkeleton label={t("loading")} />;
|
||||
}
|
||||
|
||||
if (!args?.boardId || !args.boardName) return <AssistantHumanToolError />;
|
||||
|
||||
if (settings.isError) {
|
||||
return (
|
||||
<Alert color="red" variant="light" title={t("loadErrorTitle")} icon={<IconAlertTriangle size={18} />}>
|
||||
|
||||
@@ -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";
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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 (
|
||||
<Alert color="red" variant="light" title={t("title")} icon={<IconAlertTriangle size={18} />}>
|
||||
{t("description")}
|
||||
</Alert>
|
||||
);
|
||||
};
|
||||
|
||||
export const AssistantAskUserTool = ({
|
||||
args,
|
||||
result,
|
||||
@@ -65,7 +74,9 @@ export const AssistantAskUserTool = ({
|
||||
);
|
||||
}
|
||||
|
||||
if (!hasCompleteAssistantToolArguments(status) || !args?.question || options.length < 2) {
|
||||
if (hasFailedAssistantToolArguments(status)) return <AssistantHumanToolError />;
|
||||
|
||||
if (!hasCompleteAssistantToolArguments(status)) {
|
||||
return (
|
||||
<Box className={classes.humanTool} aria-label={t("preparing")}>
|
||||
<Stack gap="xs">
|
||||
@@ -77,6 +88,8 @@ export const AssistantAskUserTool = ({
|
||||
);
|
||||
}
|
||||
|
||||
if (!args?.question || options.length < 2) return <AssistantHumanToolError />;
|
||||
|
||||
const submitResult = (answer: AskUserResult) => {
|
||||
if (submitting) return;
|
||||
setSubmitting(true);
|
||||
@@ -212,6 +225,8 @@ export const AssistantConfigureAppTool = ({
|
||||
);
|
||||
}
|
||||
|
||||
if (hasFailedAssistantToolArguments(status)) return <AssistantHumanToolError />;
|
||||
|
||||
if (initialValues === null) {
|
||||
return (
|
||||
<Box className={classes.appTool} aria-label={t("preparing")}>
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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."
|
||||
|
||||
Reference in new issue
Block a user