diff --git a/frontend/bindings/github.com/MaimoryLab/OneAgent/internal/binding/models.ts b/frontend/bindings/github.com/MaimoryLab/OneAgent/internal/binding/models.ts index 4ea67767..11e64760 100644 --- a/frontend/bindings/github.com/MaimoryLab/OneAgent/internal/binding/models.ts +++ b/frontend/bindings/github.com/MaimoryLab/OneAgent/internal/binding/models.ts @@ -163,6 +163,13 @@ export interface SaveProviderRequest { "base_url": string; "anthropic_base_url": string; "api_key": string; + + /** + * Create refuses an ID that already exists rather than overwriting it. It + * defaults to false so an edit, which legitimately writes over an existing + * entry, is the behaviour a caller gets without asking. + */ + "create": boolean; } export interface UpdateRequest { diff --git a/frontend/src/backend/wails.test.ts b/frontend/src/backend/wails.test.ts index ec3be91c..aa8225de 100644 --- a/frontend/src/backend/wails.test.ts +++ b/frontend/src/backend/wails.test.ts @@ -129,7 +129,7 @@ describe("Wails backend adapter", () => { await expect(wailsApi.probe({ provider: "custom", apiBaseUrl: "https://proxy.test/v1", apiKey: "secret", model: "m", agents: [] })).resolves.toBe(probe); await expect(wailsApi.models({ provider: "ppio", apiBaseUrl: "", apiKey: "secret" })).resolves.toBe(models); await expect(wailsApi.getProvider("acme")).resolves.toBe(provider); - await expect(wailsApi.saveProvider({ id: "acme", name: "Acme", home: "", base_url: "https://api.acme.test", anthropic_base_url: "", api_key: "secret" })).resolves.toBe(provider); + await expect(wailsApi.saveProvider({ id: "acme", name: "Acme", home: "", base_url: "https://api.acme.test", anthropic_base_url: "", api_key: "secret", create: true })).resolves.toBe(provider); await expect(wailsApi.deleteProvider("acme")).resolves.toBeUndefined(); await expect(wailsApi.install({ agents: ["codex"], provider: "ppio", api_key: "secret", model: "m", configure: true, install_agent: false, skip_test: true })).resolves.toBe(install); await wailsApi.openRegister("ppio", []); diff --git a/frontend/src/i18n.tsx b/frontend/src/i18n.tsx index fcc9fbdb..518671ba 100644 --- a/frontend/src/i18n.tsx +++ b/frontend/src/i18n.tsx @@ -45,6 +45,14 @@ const english = { "编辑配置": "Edit configuration", "详情": "Details", "删除": "Delete", + // Delete confirmations. Both name the record and say the deletion cannot be + // undone; the Provider wording also names the API Key, because that is the part + // the user cannot recreate from what is on screen. + // "删除 Profile" is already defined further down, as the edit-form heading. + "确定删除 Profile「{name}」吗？该操作无法撤销。": "Delete the Profile “{name}”? This cannot be undone.", + "删除 Provider": "Delete Provider", + "确定删除 Provider「{name}」吗？已保存的 API Key 会一并删除，该操作无法撤销。": "Delete the Provider “{name}”? Its saved API key is deleted with it, and this cannot be undone.", + "仅供本机识别，可保留默认值。小写字母、数字或连字符": "Identifies this Provider on your machine; the default is fine. Lowercase letters, digits, or hyphens", "名称": "Name", "模型": "Model", "API 类型": "API type", diff --git a/frontend/src/pages/AgentProfilePage.tsx b/frontend/src/pages/AgentProfilePage.tsx index 6ec8877e..cf04a1d2 100644 --- a/frontend/src/pages/AgentProfilePage.tsx +++ b/frontend/src/pages/AgentProfilePage.tsx @@ -194,7 +194,9 @@ export function AgentProfilePage() {
- setDraft({ ...draft, id: event.target.value })} disabled={Boolean(draft.originalId)} required /> + {/* Escaped hyphen: compiled with the `v` flag, a literal `-` inside + a character class throws and the attribute then accepts anything. */} + setDraft({ ...draft, id: event.target.value })} disabled={Boolean(draft.originalId)} required />
diff --git a/frontend/src/pages/ProfilesPage.test.tsx b/frontend/src/pages/ProfilesPage.test.tsx index a91d866d..b12618a3 100644 --- a/frontend/src/pages/ProfilesPage.test.tsx +++ b/frontend/src/pages/ProfilesPage.test.tsx @@ -9,6 +9,11 @@ import { ProfilesPage } from "./ProfilesPage"; const refreshStatus = vi.fn<() => Promise>(); const dispatch = vi.fn(); +// Hoisted so the module factory below can close over it: vi.mock is lifted above +// the imports, so a plain const declared here would not exist yet when it runs. +const { question } = vi.hoisted(() => ({ question: vi.fn<(options: { Message: string }) => Promise>() })); +vi.mock("@wailsio/runtime", () => ({ Dialogs: { Question: question } })); + vi.mock("../state/WizardContext", () => ({ useWizard: () => ({ state: mockState, dispatch, refreshStatus }), })); @@ -99,6 +104,7 @@ describe("ProfilesPage", () => { vi.restoreAllMocks(); refreshStatus.mockReset(); refreshStatus.mockResolvedValue(); + question.mockReset(); }); it("lists public Profile details and reports the Provider's key state", () => { @@ -128,14 +134,49 @@ describe("ProfilesPage", () => { expect(screen.getByTestId("profile-unused").textContent).toContain("暂无 Agent 使用"); }); - it("deletes a Profile after confirmation", async () => { - vi.spyOn(window, "confirm").mockReturnValue(true); - const remove = vi.spyOn(api, "deleteProfile").mockResolvedValue(); - renderPage([profile({ id: "unused", label: "未使用" })]); + // These two replace a test that stubbed window.confirm and asserted the delete + // went through. The page never called window.confirm, so the stub did nothing + // and the test passed against a delete button with no confirmation at all -- + // exactly the bug it was named for. Asserting the cancel path is what makes the + // confirmation load-bearing: without it, deleting unconditionally still passes. + it("does not delete a Profile when the confirmation is declined", async () => { + question.mockResolvedValue("取消"); + const remove = vi.spyOn(api, "deleteProfile").mockResolvedValue(); + renderPage([profile({ id: "unused", label: "未使用" })]); + + fireEvent.click(screen.getByRole("button", { name: "删除 未使用" })); + await waitFor(() => expect(question).toHaveBeenCalled()); + expect(remove).not.toHaveBeenCalled(); + expect(refreshStatus).not.toHaveBeenCalled(); + }); - fireEvent.click(screen.getByRole("button", { name: "删除 未使用" })); - await waitFor(() => expect(remove).toHaveBeenCalledWith("unused")); - expect(refreshStatus).toHaveBeenCalled(); + it("deletes a Profile once the confirmation is accepted", async () => { + question.mockResolvedValue("删除"); + const remove = vi.spyOn(api, "deleteProfile").mockResolvedValue(); + renderPage([profile({ id: "unused", label: "未使用" })]); + + fireEvent.click(screen.getByRole("button", { name: "删除 未使用" })); + await waitFor(() => expect(remove).toHaveBeenCalledWith("unused")); + expect(refreshStatus).toHaveBeenCalled(); + // The prompt names the Profile: "delete this?" with no subject is how a user + // confirms the wrong row. + expect(question.mock.calls[0][0].Message).toContain("未使用"); + }); + + it("blocks a second delete while the first is still running", async () => { + question.mockResolvedValue("删除"); + let release = () => {}; + const remove = vi.spyOn(api, "deleteProfile").mockReturnValue(new Promise((resolve) => { release = resolve; })); + renderPage([profile({ id: "unused", label: "未使用" })]); + + const button = screen.getByRole("button", { name: "删除 未使用" }); + fireEvent.click(button); + // Double-clicking sent two deletes, and the second one reported the Profile + // it had just removed as unknown. + await waitFor(() => expect(button).toBeDisabled()); + fireEvent.click(button); + expect(remove).toHaveBeenCalledTimes(1); + release(); }); it("explains why an in-use Profile cannot be deleted", () => { diff --git a/frontend/src/pages/ProfilesPage.tsx b/frontend/src/pages/ProfilesPage.tsx index e934cd82..ee7d2aec 100644 --- a/frontend/src/pages/ProfilesPage.tsx +++ b/frontend/src/pages/ProfilesPage.tsx @@ -7,6 +7,7 @@ import { PageScaffold } from "../components/PageScaffold"; import { ProviderSegment } from "../components/ProviderSegment"; import { SelectField } from "../components/SelectField"; import { useI18n } from "../i18n"; +import { confirmDelete } from "../state/confirmDelete"; import { byProviderCreatedAt } from "../state/ranking"; import { installTaskRoute, taskKey, useTaskCenter } from "../state/TaskCenterContext"; import { useWizard } from "../state/WizardContext"; @@ -184,6 +185,14 @@ export function ProfilesPage() { })); return; } + // Asked after the in-use check, so a Profile that cannot be deleted anyway + // explains itself rather than prompting first and refusing afterwards. + if (!await confirmDelete({ + title: t("删除 Profile"), + message: t("确定删除 Profile「{name}」吗？该操作无法撤销。", { name: profile.label }), + confirmLabel: t("删除"), + cancelLabel: t("取消"), + })) return; setBusy(true); setFailure(""); try { @@ -226,7 +235,10 @@ export function ProfilesPage() { id="profile-id" value={editor.id} onChange={(event) => setEditor({ ...editor, id: event.target.value })} - pattern="[a-z0-9][a-z0-9_-]{0,63}" + /* Escaped hyphen: compiled with the `v` flag, a literal `-` in a + character class throws, and that error left the attribute + accepting every value. */ + pattern="[a-z0-9][a-z0-9_\-]{0,63}" placeholder={t("例如 team-ppio")} disabled={Boolean(editor.originalId)} required @@ -329,7 +341,10 @@ export function ProfilesPage() { - diff --git a/frontend/src/pages/ProvidersPage.test.tsx b/frontend/src/pages/ProvidersPage.test.tsx index 5abcabdc..fe500496 100644 --- a/frontend/src/pages/ProvidersPage.test.tsx +++ b/frontend/src/pages/ProvidersPage.test.tsx @@ -6,6 +6,11 @@ import { api } from "../backend/api"; import type { StatusResponse } from "../types/api"; import { ProvidersPage } from "./ProvidersPage"; +// Hoisted so the module factory can close over it: vi.mock is lifted above the +// imports, so a plain const here would not exist yet when the factory runs. +const { question } = vi.hoisted(() => ({ question: vi.fn<(options: { Message: string }) => Promise>() })); +vi.mock("@wailsio/runtime", () => ({ Dialogs: { Question: question } })); + vi.mock("../state/WizardContext", () => ({ useWizard: () => ({ state: mockState, dispatch: vi.fn(), refreshStatus: vi.fn() }), })); @@ -79,7 +84,10 @@ function renderPage(agents: Record) { } describe("ProvidersPage", () => { - afterEach(() => vi.restoreAllMocks()); + afterEach(() => { + vi.restoreAllMocks(); + question.mockReset(); + }); it("lists each Provider with its endpoint", () => { renderPage({ codex: "ppio" }); @@ -211,21 +219,74 @@ describe("ProvidersPage", () => { fireEvent.change(screen.getByLabelText("OpenAI 兼容 Base URL"), { target: { value: "https://api.acme.test" } }); fireEvent.change(screen.getByLabelText("API Key"), { target: { value: "sk-acme" } }); fireEvent.click(screen.getByRole("button", { name: /^保存$/ })); - await waitFor(() => expect(save).toHaveBeenCalledWith(expect.objectContaining({ id: "acme", base_url: "https://api.acme.test", api_key: "sk-acme" }))); + await waitFor(() => expect(save).toHaveBeenCalledWith(expect.objectContaining({ id: "acme", base_url: "https://api.acme.test", api_key: "sk-acme", create: true }))); }); - it("deletes a user Provider", async () => { + // The ID is a storage key the user should not have to invent, but a collision is + // now refused rather than silently overwriting -- so the suggested value has to + // be one that is actually free. + it("prefills a free Provider ID and states the rule", () => { + renderPage({ codex: null }); + fireEvent.click(screen.getByRole("button", { name: "新增 Provider" })); + const id = screen.getByLabelText("Provider ID") as HTMLInputElement; + expect(id.value).toMatch(/^[a-z0-9][a-z0-9-]*$/); + expect(Object.keys(mockState.status?.providers ?? {})).not.toContain(id.value); + expect(screen.getByText(/小写字母、数字或连字符/)).toBeTruthy(); + }); + + // create separates the two intents. An edit must keep overwriting, or saving a + // Provider you opened from the list would be refused as a duplicate of itself. + it("saves an edited Provider without the create flag", async () => { + const save = vi.spyOn(api, "saveProvider").mockResolvedValue({ + entry: { + id: "ppio", name: "PPIO Cloud", home: "", base_url: "https://api.ppio.com/openai", + anthropic_base_url: "", api_key: "", built_in: true, + }, + reapplied: null, + failures: null, + }); + vi.spyOn(api, "getProvider").mockResolvedValue({ + id: "ppio", name: "PPIO", home: "", base_url: "https://api.ppio.com/openai", + anthropic_base_url: "", api_key: "", built_in: true, + }); + renderPage({ codex: "ppio" }); + fireEvent.click(screen.getByRole("button", { name: "编辑 PPIO" })); + await waitFor(() => expect(screen.getByLabelText("名称")).toBeTruthy()); + fireEvent.change(screen.getByLabelText("名称"), { target: { value: "PPIO Cloud" } }); + fireEvent.click(screen.getByRole("button", { name: /^保存$/ })); + await waitFor(() => expect(save).toHaveBeenCalledWith(expect.objectContaining({ id: "ppio", create: false }))); + }); + + // As on the Profiles page, this used to stub window.confirm, which the page + // never called -- so it passed against an unconfirmed delete. The declined case + // is what holds the confirmation in place. + const renderWithUserProvider = () => { renderPage({ codex: null }); if (!mockState.status) throw new Error("missing status"); mockState.status.providers.acme = { name: "Acme", home: "", base_url: "https://api.acme.test", custom: true }; - vi.spyOn(window, "confirm").mockReturnValue(true); - const remove = vi.spyOn(api, "deleteProvider").mockResolvedValue(); render( , ); + }; + + it("does not delete a Provider when the confirmation is declined", async () => { + question.mockResolvedValue("取消"); + const remove = vi.spyOn(api, "deleteProvider").mockResolvedValue(); + renderWithUserProvider(); + fireEvent.click(screen.getByRole("button", { name: "删除 Acme" })); + await waitFor(() => expect(question).toHaveBeenCalled()); + expect(remove).not.toHaveBeenCalled(); + }); + + it("deletes a user Provider once the confirmation is accepted", async () => { + question.mockResolvedValue("删除"); + const remove = vi.spyOn(api, "deleteProvider").mockResolvedValue(); + renderWithUserProvider(); fireEvent.click(screen.getByRole("button", { name: "删除 Acme" })); await waitFor(() => expect(remove).toHaveBeenCalledWith("acme")); + // The saved key going too is the part worth warning about. + expect(question.mock.calls[0][0].Message).toContain("API Key"); }); }); diff --git a/frontend/src/pages/ProvidersPage.tsx b/frontend/src/pages/ProvidersPage.tsx index 6056d721..15bf3c5f 100644 --- a/frontend/src/pages/ProvidersPage.tsx +++ b/frontend/src/pages/ProvidersPage.tsx @@ -6,6 +6,7 @@ import { api, describeError } from "../backend/api"; import { PageScaffold } from "../components/PageScaffold"; import { SecureKeyField } from "../components/SecureKeyField"; import { useI18n } from "../i18n"; +import { confirmDelete } from "../state/confirmDelete"; import { useWizard } from "../state/WizardContext"; import { byProviderCreatedAt } from "../state/ranking"; import type { ProviderEntry } from "../types/api"; @@ -20,6 +21,24 @@ const emptyProvider: ProviderEntry = { built_in: false, }; +/** + * A free ID for a new Provider. + * + * The ID is a storage key, not something a user should have to invent, but it + * still has to be unique and match the backend's pattern — and a collision is now + * refused rather than silently overwriting the existing Provider. Suggesting a + * valid unused value means the common path never has to think about it. The + * numeric suffix loop mirrors the one in ProfilesPage.openCreate. + */ +function suggestProviderID(taken: Iterable, base = "provider"): string { + const slug = base.toLowerCase().replace(/[^a-z0-9-]/g, "-").replace(/^-+/, "") || "provider"; + const used = new Set([...taken].map((id) => id.toLowerCase())); + let id = slug; + let suffix = 2; + while (used.has(id)) id = `${slug}-${suffix++}`; + return id; +} + export function ProvidersPage({ create = false }: { create?: boolean }) { const navigate = useNavigate(); const { locale, t } = useI18n(); @@ -27,6 +46,10 @@ export function ProvidersPage({ create = false }: { create?: boolean }) { const { state, refreshStatus } = useWizard(); const status = state.status; const [editor, setEditor] = useState(create ? { ...emptyProvider } : null); + // Tracks whether the open editor is creating rather than editing. Derived from + // the route on a /providers/new load, but set explicitly by the inline "add" + // button, which opens the same editor on the list route. + const [creating, setCreating] = useState(create); const [busy, setBusy] = useState(false); const [failure, setFailure] = useState(""); const [applied, setApplied] = useState(""); @@ -34,12 +57,16 @@ export function ProvidersPage({ create = false }: { create?: boolean }) { const requestedProvider = searchParams.get("provider"); const returnTo = requestedReturn?.startsWith("/") && !requestedReturn.startsWith("//") ? requestedReturn : "/providers"; const openedProvider = useRef(""); + const prefilled = useRef(false); const nameOf = (agentId: string) => status?.catalog.find((item) => item.id === agentId)?.name || agentId; const closeEditor = () => { if (create) navigate(returnTo); - else setEditor(null); + else { + setEditor(null); + setCreating(false); + } }; const edit = async (providerId: string) => { @@ -48,6 +75,7 @@ export function ProvidersPage({ create = false }: { create?: boolean }) { setApplied(""); try { setEditor(await api.getProvider(providerId)); + setCreating(false); } catch (error) { setFailure(describeError(error, t("无法读取 Provider")).message); } finally { @@ -55,6 +83,16 @@ export function ProvidersPage({ create = false }: { create?: boolean }) { } }; + // Suggests an ID once status has loaded. The /providers/new route mounts the + // editor before status arrives, so this cannot be done where the state is + // initialised. The ref keeps it a suggestion: it fills the field once and never + // overwrites what the user typed, even though status refreshes on every save. + useEffect(() => { + if (!create || prefilled.current || !status) return; + prefilled.current = true; + setEditor((current) => (current && !current.id ? { ...current, id: suggestProviderID(Object.keys(status.providers)) } : current)); + }, [create, status]); + useEffect(() => { if (!create && !editor && requestedProvider && requestedProvider !== openedProvider.current && status?.providers[requestedProvider]) { openedProvider.current = requestedProvider; @@ -71,7 +109,10 @@ export function ProvidersPage({ create = false }: { create?: boolean }) { try { // Changing an endpoint or key rewrites every Agent already using this // Provider, so the outcome has to be reported rather than silently applied. - const result = await api.saveProvider(editor); + // create tells Go to refuse an ID that is taken instead of overwriting that + // Provider. Only the caller knows which of the two this is: a complete + // entry whose ID is not on disk looks the same either way. + const result = await api.saveProvider({ ...editor, create: creating }); const reapplied = result.reapplied ?? []; const failures = Object.entries(result.failures ?? {}); if (failures.length) { @@ -101,6 +142,13 @@ export function ProvidersPage({ create = false }: { create?: boolean }) { })); return; } + // The saved API key goes with it, which is the part a user does not get back. + if (!await confirmDelete({ + title: t("删除 Provider"), + message: t("确定删除 Provider「{name}」吗？已保存的 API Key 会一并删除，该操作无法撤销。", { name }), + confirmLabel: t("删除"), + cancelLabel: t("取消"), + })) return; setBusy(true); setFailure(""); setApplied(""); @@ -129,7 +177,7 @@ export function ProvidersPage({ create = false }: { create?: boolean }) { description={t("管理模型服务、端点与本机保存的 API Key")} bodyClassName="management-page" secondaryAction={!create ? ( - @@ -151,11 +199,20 @@ export function ProvidersPage({ create = false }: { create?: boolean }) { id="provider-id" value={editor.id} onChange={(event) => setEditor({ ...editor, id: event.target.value })} - pattern="[a-z0-9][a-z0-9-]{0,63}" + /* Escaped hyphen: the browser compiles `pattern` with the `v` flag, + where a literal `-` in a character class is a syntax error. The + unescaped form threw during validation and the error was + swallowed, so this attribute accepted anything — "ACME!!" + included — leaving Go as the only check. */ + pattern="[a-z0-9][a-z0-9\-]{0,63}" placeholder={t("例如 siliconflow")} disabled={editor.built_in} required /> + {/* The rule was only enforced by `pattern`, so a user learned it by + being rejected. Stated here instead, next to the prefilled value + they are free to keep. */} + {t("仅供本机识别，可保留默认值。小写字母、数字或连字符")}
@@ -214,7 +271,7 @@ export function ProvidersPage({ create = false }: { create?: boolean }) { {meta.custom ? ( - ) : null} diff --git a/frontend/src/pages/TransferPage.tsx b/frontend/src/pages/TransferPage.tsx index 6d167aea..86da3848 100644 --- a/frontend/src/pages/TransferPage.tsx +++ b/frontend/src/pages/TransferPage.tsx @@ -131,7 +131,10 @@ export function TransferPage() { const password = encrypted ? await askPassword("import") : ""; if (encrypted && !password) return; const data = await parseTransfer(raw, password || ""); - for (const provider of data.providers ?? []) await api.saveProvider(provider); + // create: false — an import restores Providers, so an ID that already + // exists is the expected case and overwriting it is the point. Refusing + // duplicates here would make re-importing a backup fail. + for (const provider of data.providers ?? []) await api.saveProvider({ ...provider, create: false }); for (const profile of data.profiles ?? []) await api.saveProfile({ id: profile.id, label: profile.label, provider: profile.provider, apiBaseUrl: "", apiKey: "", model: profile.model || "", configMode: "provider", protocol: profile.protocol || "" }); await refreshStatus(); setSuccess(t("导入完成")); diff --git a/frontend/src/pages/id-pattern.test.ts b/frontend/src/pages/id-pattern.test.ts new file mode 100644 index 00000000..c2e65436 --- /dev/null +++ b/frontend/src/pages/id-pattern.test.ts @@ -0,0 +1,53 @@ +/** + * Guards the `pattern` attributes on the ID fields. + * + * Browsers compile `pattern` as a `v`-flag regex, and under `v` a literal `-` + * inside a character class is a syntax error rather than a hyphen. All three ID + * fields shipped with an unescaped `[a-z0-9-]`, so validation threw, the browser + * swallowed the error, and the attribute accepted every value including "ACME!!". + * Nothing failed visibly — the Go validator caught it at save time instead — which + * is why this went unnoticed and why it needs a test rather than just a fix. + * + * Reading the source is deliberate: the bug is in the attribute string, so a test + * that imported the component and inspected the DOM would pass on a value the + * browser had already rejected. + */ +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +import { describe, expect, it } from "vitest"; + +const FIELDS = [ + { file: "ProvidersPage.tsx", label: "Provider ID" }, + { file: "ProfilesPage.tsx", label: "Profile ID" }, + { file: "AgentProfilePage.tsx", label: "Agent Profile ID" }, +]; + +function patterns(file: string): string[] { + const source = readFileSync(fileURLToPath(new URL(file, import.meta.url)), "utf8"); + return [...source.matchAll(/pattern="([^"]+)"/g)].map((match) => match[1]); +} + +describe("ID field patterns", () => { + it("compiles under the v flag the browser actually uses", () => { + for (const { file, label } of FIELDS) { + const found = patterns(file); + expect(found.length, `${label} should declare a pattern`).toBeGreaterThan(0); + for (const pattern of found) { + // Anchored the way a browser anchors `pattern`, and with the same flag. + expect(() => new RegExp(`^(?:${pattern})$`, "v"), `${label}: ${pattern}`).not.toThrow(); + } + } + }); + + it("still rejects the values it exists to reject", () => { + for (const { file, label } of FIELDS) { + for (const pattern of patterns(file)) { + const valid = new RegExp(`^(?:${pattern})$`, "v"); + expect(valid.test("acme-x"), `${label} rejected a legal ID`).toBe(true); + expect(valid.test("ACME!!"), `${label} accepted an illegal ID`).toBe(false); + expect(valid.test("-leading"), `${label} accepted a leading hyphen`).toBe(false); + } + } + }); +}); diff --git a/frontend/src/state/confirmDelete.test.ts b/frontend/src/state/confirmDelete.test.ts new file mode 100644 index 00000000..a3098e3f --- /dev/null +++ b/frontend/src/state/confirmDelete.test.ts @@ -0,0 +1,61 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +// Hoisted so the module factory can close over it: vi.mock is lifted above the +// imports, so a plain const here would not exist yet when the factory runs. +const { question } = vi.hoisted(() => ({ question: vi.fn<() => Promise>() })); +vi.mock("@wailsio/runtime", () => ({ Dialogs: { Question: question } })); + +const { confirmDelete } = await import("./confirmDelete"); + +const options = { title: "Delete", message: "Delete “acme”?", confirmLabel: "删除", cancelLabel: "取消" }; + +describe("confirmDelete", () => { + afterEach(() => { + vi.restoreAllMocks(); + question.mockReset(); + }); + + it("approves only the confirm button", async () => { + question.mockResolvedValue("删除"); + expect(await confirmDelete(options)).toBe(true); + question.mockResolvedValue("取消"); + expect(await confirmDelete(options)).toBe(false); + }); + + // Dialogs.Question needs a WebView to host the dialog. Under `-tags server` + // there is none and the call never settles -- it does not reject, so a catch + // could not see it. This is what broke the E2E delete: the button did nothing + // and produced no error. + it("falls back to window.confirm when the native dialog never answers", async () => { + question.mockReturnValue(new Promise(() => {})); + const confirm = vi.spyOn(window, "confirm").mockReturnValue(true); + expect(await confirmDelete(options)).toBe(true); + expect(confirm).toHaveBeenCalledWith(options.message); + + confirm.mockReturnValue(false); + expect(await confirmDelete(options)).toBe(false); + }); + + it("falls back when the native dialog rejects outright", async () => { + question.mockRejectedValue(new Error("no webview")); + const confirm = vi.spyOn(window, "confirm").mockReturnValue(true); + expect(await confirmDelete(options)).toBe(true); + }); + + // A declined native dialog is an answer. Re-prompting through window.confirm + // would ask the user twice and turn a "no" into a second chance to say yes. + it("does not re-prompt when the native dialog was declined", async () => { + question.mockResolvedValue("取消"); + const confirm = vi.spyOn(window, "confirm").mockReturnValue(true); + expect(await confirmDelete(options)).toBe(false); + expect(confirm).not.toHaveBeenCalled(); + }); + + it("refuses when neither prompt can be shown", async () => { + question.mockRejectedValue(new Error("no webview")); + // A hardened webview can omit window.confirm; "cannot ask" must not delete. + vi.stubGlobal("confirm", undefined); + expect(await confirmDelete(options)).toBe(false); + vi.unstubAllGlobals(); + }); +}); diff --git a/frontend/src/state/confirmDelete.ts b/frontend/src/state/confirmDelete.ts new file mode 100644 index 00000000..4a5d9bc4 --- /dev/null +++ b/frontend/src/state/confirmDelete.ts @@ -0,0 +1,58 @@ +import { Dialogs } from "@wailsio/runtime"; + +/** + * Asks the user to confirm destroying something, returning false if they decline. + * + * Deleting a Profile or a Provider removes a record and its API key from disk + * with no undo, so both go through here. It is a plain function rather than a + * React component because the two callers need an answer in the middle of an + * async handler, not a second render pass with a pending-delete state to unwind. + * + * The native dialog is the same mechanism the update prompt and the export + * password flow already use, which is why there is no in-app modal to build: it + * is modal for free, it cannot be dismissed by a stray click on the page, and it + * needs no styling in either theme. + * + * Neither button is marked IsDefault. A default button is the one Enter + * activates, and for an irreversible delete the safe outcome of an accidental + * keypress is "nothing happened". + * + * Falls back to window.confirm when the native dialog does not answer, which is a + * real environment difference rather than a test affordance. Dialogs.Question + * needs a WebView to host the dialog, and under `-tags server` — the browser + * preview and the E2E build — there is none: the call neither resolves nor + * rejects, it simply never settles. A rejection could be caught, but a hang + * cannot, so the request is raced against a timer. Without this the delete button + * silently did nothing in server mode, with no error to explain why. + */ +const NATIVE_DIALOG_TIMEOUT_MS = 1500; + +export async function confirmDelete(options: { + title: string; + message: string; + confirmLabel: string; + cancelLabel: string; +}): Promise { + const pending = Dialogs.Question({ + Title: options.title, + Message: options.message, + Buttons: [ + { Label: options.confirmLabel }, + { Label: options.cancelLabel, IsCancel: true }, + ], + }).then( + (choice) => ({ answered: true, approved: choice === options.confirmLabel }), + () => ({ answered: false, approved: false }), + ); + // The sentinel is a distinct object rather than a boolean so a real answer of + // "declined" is not confused with "never answered": one must not fall through + // to a second prompt. + const unanswered = { answered: false, approved: false }; + const timer = new Promise((resolve) => setTimeout(() => resolve(unanswered), NATIVE_DIALOG_TIMEOUT_MS)); + const outcome = await Promise.race([pending, timer]); + if (outcome.answered) return outcome.approved; + // window.confirm is absent in some hardened webviews. Where neither prompt can + // be shown, "cannot ask" has to mean "do not delete". + if (typeof window.confirm !== "function") return false; + return window.confirm(options.message); +} diff --git a/internal/app/desktopapp_test.go b/internal/app/desktopapp_test.go index e82a5940..a37cf1d6 100644 --- a/internal/app/desktopapp_test.go +++ b/internal/app/desktopapp_test.go @@ -94,7 +94,7 @@ func TestConfigureWorkBuddyWritesModelsJSONFromProvider(t *testing.T) { } reapplied, err := core.SaveProvider(context.Background(), provider.Entry{ ID: "ppio", Name: "PPIO", BaseURL: "https://relay.example/openai", APIKey: "rotated-key", - }) + }, false) if err != nil || len(reapplied.Failures) != 0 || len(reapplied.Reapplied) != 1 || reapplied.Reapplied[0] != desktopapp.WorkBuddyID { t.Fatalf("WorkBuddy Provider reapply = %#v, err=%v", reapplied, err) } diff --git a/internal/app/provider.go b/internal/app/provider.go index 5cc5bc34..f2847da6 100644 --- a/internal/app/provider.go +++ b/internal/app/provider.go @@ -136,7 +136,11 @@ func (u *UseCases) GetProvider(ctx context.Context, providerID string) (provider // without the user re-applying each Profile by hand. Reapply failures are // returned per Agent instead of failing the save: the Provider record is already // correct on disk, and reverting it would lose the edit. -func (u *UseCases) SaveProvider(ctx context.Context, entry provider.Entry) (SaveProviderResult, error) { +// create distinguishes the "add Provider" form from editing an existing entry. +// The distinction has to come from the caller: both arrive here as a complete +// Entry, and an ID that is not on disk is equally consistent with creating a new +// Provider and with renaming one, so the store cannot infer the intent. +func (u *UseCases) SaveProvider(ctx context.Context, entry provider.Entry, create bool) (SaveProviderResult, error) { if u == nil { return SaveProviderResult{}, oneerrors.New(oneerrors.InternalError, "Provider service is not configured", oneerrors.WithStatus(501)) } @@ -146,7 +150,11 @@ func (u *UseCases) SaveProvider(ctx context.Context, entry provider.Entry) (Save u.writeMu.Lock() defer u.writeMu.Unlock() before, _ := u.providers.Get(entry.ID) - saved, err := u.providers.Save(ctx, entry) + write := u.providers.Save + if create { + write = u.providers.Create + } + saved, err := write(ctx, entry) if err != nil { return SaveProviderResult{}, err } diff --git a/internal/app/provider_test.go b/internal/app/provider_test.go index 76f99fb2..9eff56a7 100644 --- a/internal/app/provider_test.go +++ b/internal/app/provider_test.go @@ -108,7 +108,7 @@ func TestSavedProviderDrivesStatusAndProbeWithoutResendingKey(t *testing.T) { core := NewUseCasesWithProviderClient(options, client) if _, err := core.SaveProvider(context.Background(), provider.Entry{ ID: "acme", Name: "Acme", BaseURL: "https://api.acme.test/openai", APIKey: "saved-key", - }); err != nil { + }, true); err != nil { t.Fatal(err) } @@ -152,7 +152,7 @@ func TestSaveProviderReappliesEveryAgentBoundToIt(t *testing.T) { result, err := core.SaveProvider(context.Background(), provider.Entry{ ID: "ppio", Name: "PPIO", BaseURL: "https://relay.ppio.test/openai", APIKey: "rotated-key", - }) + }, false) if err != nil { t.Fatal(err) } @@ -201,7 +201,7 @@ func TestSaveProviderSkipsReapplyWhenOnlyMetadataChanges(t *testing.T) { t.Fatal(err) } entry.Name = "PPIO Cloud" - result, err := core.SaveProvider(context.Background(), entry) + result, err := core.SaveProvider(context.Background(), entry, false) if err != nil || len(result.Reapplied) != 0 || len(result.Failures) != 0 { t.Fatalf("metadata-only save reapplied: %#v, err=%v", result, err) } @@ -212,7 +212,7 @@ func TestDeleteProviderRejectsBoundAgents(t *testing.T) { core := activationCore(t, home, provider.NewClient(nil), "linux") if _, err := core.SaveProvider(context.Background(), provider.Entry{ ID: "acme", Name: "Acme", BaseURL: "https://api.acme.test", APIKey: "key", - }); err != nil { + }, true); err != nil { t.Fatal(err) } if _, err := core.ActivateAgent(context.Background(), ActivateAgentOptions{ @@ -284,7 +284,7 @@ func TestDeleteIgnoresStaleBindingWithoutAgentConfig(t *testing.T) { core := activationCore(t, home, provider.NewClient(nil), "linux") if _, err := core.SaveProvider(context.Background(), provider.Entry{ ID: "acme", Name: "Acme", BaseURL: "https://api.acme.test", APIKey: "key", - }); err != nil { + }, true); err != nil { t.Fatal(err) } if _, err := core.SaveProfile(context.Background(), SaveProfileOptions{ diff --git a/internal/app/status_test.go b/internal/app/status_test.go index 0180577f..f9e2c1d2 100644 --- a/internal/app/status_test.go +++ b/internal/app/status_test.go @@ -206,7 +206,7 @@ func TestSaveProfileCanSwitchAKeylessProfileProvider(t *testing.T) { func TestSaveProfileUsesProtocolEndpoint(t *testing.T) { core := NewUseCases(StatusOptions{Home: t.TempDir(), Platform: platform.For("linux", "amd64"), Lookup: func(string) (string, bool) { return "", false }}) - if _, err := core.SaveProvider(context.Background(), provider.Entry{ID: "anthropic-only", Name: "Anthropic", AnthropicBaseURL: "https://api.example.test/anthropic"}); err != nil { + if _, err := core.SaveProvider(context.Background(), provider.Entry{ID: "anthropic-only", Name: "Anthropic", AnthropicBaseURL: "https://api.example.test/anthropic"}, true); err != nil { t.Fatal(err) } if _, err := core.SaveProfile(context.Background(), SaveProfileOptions{ID: "anthropic", Provider: "anthropic-only", Model: "model", ConfigMode: "provider", Protocol: "anthropic"}); err != nil { @@ -429,3 +429,49 @@ func TestStatusReportsFirstRunUntilOneAgentDirExists(t *testing.T) { t.Fatal("firstRun must clear once ~/.oneagent exists") } } + +// DeleteProfile had no coverage at all, which is how the double-click bug in the +// UI reached a user: the second click hit a Profile whose file was already gone +// and got "Unknown Profile" back. +func TestDeleteProfileRemovesTheProfileAndItsSecret(t *testing.T) { + home := t.TempDir() + core := NewUseCases(StatusOptions{Home: home, Platform: platform.For("linux", "amd64"), Lookup: func(string) (string, bool) { return "", false }}) + if _, err := core.SaveProfile(context.Background(), SaveProfileOptions{ + ID: "team", Provider: "ppio", Model: "model-a", APIKey: "stored-secret", + }); err != nil { + t.Fatal(err) + } + if err := core.DeleteProfile(context.Background(), "team"); err != nil { + t.Fatalf("delete = %v", err) + } + profiles, err := core.ListProfiles(context.Background()) + if err != nil || len(profiles) != 0 { + t.Fatalf("profiles after delete = %#v, err=%v", profiles, err) + } + // The secret is a separate file, so a delete that only unlinked the Profile + // would leave a credential behind for an ID the user believes is gone. + if entries, _ := os.ReadDir(filepath.Join(home, ".oneagent", "profiles")); len(entries) != 0 { + t.Fatalf("profile directory still holds %d entries", len(entries)) + } +} + +// Deleting something that is already absent is what the user asked for, so it +// succeeds. This is the assertion that pins the double-click fix. +func TestDeleteProfileSucceedsWhenTheProfileIsAlreadyGone(t *testing.T) { + home := t.TempDir() + core := NewUseCases(StatusOptions{Home: home, Platform: platform.For("linux", "amd64"), Lookup: func(string) (string, bool) { return "", false }}) + if _, err := core.SaveProfile(context.Background(), SaveProfileOptions{ + ID: "team", Provider: "ppio", Model: "model-a", + }); err != nil { + t.Fatal(err) + } + if err := core.DeleteProfile(context.Background(), "team"); err != nil { + t.Fatalf("first delete = %v", err) + } + if err := core.DeleteProfile(context.Background(), "team"); err != nil { + t.Fatalf("second delete of the same Profile = %v, want nil", err) + } + if err := core.DeleteProfile(context.Background(), "never-existed"); err != nil { + t.Fatalf("delete of an unknown Profile = %v, want nil", err) + } +} diff --git a/internal/binding/services.go b/internal/binding/services.go index 2514f516..22319a3d 100644 --- a/internal/binding/services.go +++ b/internal/binding/services.go @@ -243,7 +243,7 @@ func (s *ProviderService) SaveProvider(ctx context.Context, request SaveProvider return s.core.SaveProvider(ctx, provider.Entry{ ID: request.ID, Name: request.Name, Home: request.Home, BaseURL: request.BaseURL, AnthropicBaseURL: request.AnthropicBaseURL, APIKey: request.APIKey, - }) + }, request.Create) } func (s *ProviderService) DeleteProvider(ctx context.Context, request ProviderIDRequest) (ProviderMutationResponse, error) { @@ -486,6 +486,10 @@ type SaveProviderRequest struct { BaseURL string `json:"base_url"` AnthropicBaseURL string `json:"anthropic_base_url"` APIKey string `json:"api_key"` + // Create refuses an ID that already exists rather than overwriting it. It + // defaults to false so an edit, which legitimately writes over an existing + // entry, is the behaviour a caller gets without asking. + Create bool `json:"create"` } type ProviderMutationResponse struct { diff --git a/internal/binding/services_test.go b/internal/binding/services_test.go index 54c7f9e3..14e41466 100644 --- a/internal/binding/services_test.go +++ b/internal/binding/services_test.go @@ -128,7 +128,7 @@ func TestOpenRegistrationFallsBackToHomeWithoutAKeyPage(t *testing.T) { }) if _, err := core.SaveProvider(context.Background(), provider.Entry{ ID: "acme", Name: "Acme", Home: "https://acme.example.com/", BaseURL: "https://api.acme.example.com/openai", - }); err != nil { + }, true); err != nil { t.Fatal(err) } var opened string diff --git a/internal/profile/write.go b/internal/profile/write.go index 61af2adf..1e2e1b48 100644 --- a/internal/profile/write.go +++ b/internal/profile/write.go @@ -90,6 +90,12 @@ func (s Store) Save(ctx context.Context, request SaveRequest) (Profile, error) { // Delete removes a Profile and its sibling secret. Deleting the active Profile // also clears the active pointer so status returns to an unconfigured state. +// +// A Profile that is already gone is not an error: the caller asked for it to not +// exist, and it does not. Reporting failure there made a double-clicked delete +// button surface "Unknown Profile" for the second click, and it would also have +// left a Profile whose record vanished out-of-band undeletable — the secret and +// the active pointer below are cleaned up either way. func (s Store) Delete(ctx context.Context, id string) error { if err := requestContext(ctx); err != nil { return err @@ -98,10 +104,7 @@ func (s Store) Delete(ctx context.Context, id string) error { return err } profilePath, _ := s.ProfilePath(id) - if err := os.Remove(profilePath); err != nil { - if os.IsNotExist(err) { - return oneerrors.New(oneerrors.InvalidRequest, "Unknown Profile: "+id) - } + if err := os.Remove(profilePath); err != nil && !os.IsNotExist(err) { return writeError("Cannot delete Profile %s: %v", id, err) } if secretPath, err := s.SecretPath(id); err == nil { diff --git a/internal/profile/write_test.go b/internal/profile/write_test.go index 22e4e29a..c3de8b8c 100644 --- a/internal/profile/write_test.go +++ b/internal/profile/write_test.go @@ -116,8 +116,13 @@ func TestDeleteProfileRemovesRecordSecretAndActivePointer(t *testing.T) { t.Errorf("%s still exists: %v", path, err) } } - if err := store.Delete(context.Background(), "team"); err == nil || oneerrors.As(err).Code != oneerrors.InvalidRequest { - t.Fatalf("deleting missing Profile returned %v", err) + // Deleting it again succeeds. This asserted an InvalidRequest until the + // double-clicked delete button in the UI showed what that costs: the second + // click reported "Unknown Profile" for a Profile the user had just deleted + // successfully. Absence is the requested end state, so reaching it twice is + // not a failure. + if err := store.Delete(context.Background(), "team"); err != nil { + t.Fatalf("deleting an already-deleted Profile returned %v, want nil", err) } } diff --git a/internal/provider/store.go b/internal/provider/store.go index 2594561b..9b448167 100644 --- a/internal/provider/store.go +++ b/internal/provider/store.go @@ -140,6 +140,44 @@ func (s Store) Public() (map[string]catalog.Provider, error) { return result, nil } +// Create saves a Provider that must not exist yet, and is what the "add +// Provider" form goes through. +// +// Save is an upsert, which is correct for editing but wrong for creating: an ID +// that collided with an existing entry silently replaced its endpoints and key, +// and because only "custom" was reserved, an ID matching a built-in Provider +// shadowed the catalog entry. SaveProvider then reapplies the Provider to every +// Agent bound to that ID, so a typo could repoint working Agents at another +// vendor's endpoint with no prompt. +func (s Store) Create(ctx context.Context, entry Entry) (Entry, error) { + entry.ID = strings.TrimSpace(entry.ID) + if err := s.checkIDAvailable(entry.ID); err != nil { + return Entry{}, err + } + return s.Save(ctx, entry) +} + +// checkIDAvailable reports whether an ID is free for a new Provider. Built-in IDs +// are refused as firmly as user ones: overriding a built-in endpoint is a real +// need, but it belongs behind editing that Provider, not behind reusing its name +// by accident. +func (s Store) checkIDAvailable(id string) error { + if id == "" { + return nil // validateEntry owns the shape of the ID; this only owns collisions. + } + if _, builtIn := catalog.ProviderByID(id); builtIn { + return oneerrors.New(oneerrors.InvalidRequest, fmt.Sprintf("Provider ID %q is already used by a built-in Provider. Pick a different ID, or edit that Provider instead.", id)) + } + file, err := s.load() + if err != nil { + return err + } + if _, exists := file.Providers[id]; exists { + return oneerrors.New(oneerrors.InvalidRequest, fmt.Sprintf("Provider ID %q is already in use. Pick a different ID, or edit that Provider instead.", id)) + } + return nil +} + func (s Store) Save(ctx context.Context, entry Entry) (Entry, error) { entry.ID = strings.TrimSpace(entry.ID) entry.Name = strings.TrimSpace(entry.Name) diff --git a/internal/provider/store_test.go b/internal/provider/store_test.go index d30a27a8..12f1f3ae 100644 --- a/internal/provider/store_test.go +++ b/internal/provider/store_test.go @@ -5,6 +5,7 @@ import ( "os" "testing" + oneerrors "github.com/MaimoryLab/OneAgent/internal/errors" "github.com/MaimoryLab/OneAgent/internal/securefs" ) @@ -58,3 +59,51 @@ func TestStoreAcceptsEitherAPIEndpoint(t *testing.T) { t.Fatal("empty Provider unexpectedly saved") } } + +// Create is the "add Provider" path and must refuse an ID that is taken. Save +// stays an upsert, because editing a Provider legitimately overwrites it, and +// SaveKey rewrites the whole entry to rotate a key. +func TestCreateRefusesAnIDThatAlreadyExists(t *testing.T) { + store := NewStore(t.TempDir(), securefs.New(securefs.Options{OS: "linux"})) + first := Entry{ID: "acme", Name: "Acme", BaseURL: "https://api.acme.test/openai", APIKey: "sk-first"} + if _, err := store.Create(context.Background(), first); err != nil { + t.Fatal(err) + } + second := Entry{ID: "acme", Name: "Impostor", BaseURL: "https://api.impostor.test/openai", APIKey: "sk-second"} + if _, err := store.Create(context.Background(), second); err == nil { + t.Fatal("creating a Provider with a taken ID succeeded") + } else if oneerrors.As(err).Code != oneerrors.InvalidRequest { + t.Fatalf("duplicate ID error code = %v", oneerrors.As(err).Code) + } + // The point of refusing is that the original survives untouched. + got, err := store.Get("acme") + if err != nil || got.Name != "Acme" || got.APIKey != "sk-first" { + t.Fatalf("refused create still modified the Provider: %#v, err=%v", got, err) + } + // Editing the same ID through Save is still allowed. + if _, err := store.Save(context.Background(), second); err != nil { + t.Fatalf("editing an existing Provider was refused: %v", err) + } +} + +// A built-in ID is reserved too. Shadowing one used to be silent, and because +// SaveProvider reapplies the entry to every Agent bound to that ID, it repointed +// working Agents at another vendor's endpoint. +func TestCreateRefusesABuiltInProviderID(t *testing.T) { + store := NewStore(t.TempDir(), securefs.New(securefs.Options{OS: "linux"})) + _, err := store.Create(context.Background(), Entry{ + ID: "ppio", Name: "Not PPIO", BaseURL: "https://api.impostor.test/openai", + }) + if err == nil { + t.Fatal("creating a Provider over a built-in ID succeeded") + } + if oneerrors.As(err).Code != oneerrors.InvalidRequest { + t.Fatalf("built-in collision error code = %v", oneerrors.As(err).Code) + } + // Overriding a built-in endpoint remains possible through the edit path. + if _, err := store.Save(context.Background(), Entry{ + ID: "ppio", Name: "PPIO", BaseURL: "https://relay.ppio.test/openai", + }); err != nil { + t.Fatalf("editing a built-in Provider was refused: %v", err) + } +}