From 21c127e7806490502b921534c7bb346bbdbaffe6 Mon Sep 17 00:00:00 2001 From: Jose Antonio Alatorre Date: Sun, 18 Oct 2026 10:18:17 +0200 Subject: [PATCH 1/6] fix(command-center-ai): address model provider settings issues Show failed or refused sign-off messages without changing the signed-in card. Keep provider details open when Escape belongs to a host dialog, and respect already-handled Escape events. Cover both fixes with regression tests. Align the model-provider skill, guide, and READMEs with per-record user, editor, and owner policies while preserving the credential disclosure. Fixes #18 Fixes #19 Fixes #20 Co-authored-by: Codex --- command-center-ai/CHANGELOG.md | 17 ++++ .../manage-model-providers/SKILL.md | 38 +++++++-- command-center-ai/docs/model-providers.md | 38 +++++++-- .../ModelProviderSettings.client.test.tsx | 67 +++++++++++++++ .../model-providers/ModelProviderSettings.tsx | 5 +- .../src/model-providers/README.md | 44 +++++++--- .../src/ui/Dialog.client.test.tsx | 83 +++++++++++++++++++ command-center-ai/src/ui/Dialog.tsx | 18 +++- command-center-ai/src/ui/README.md | 4 + 9 files changed, 280 insertions(+), 34 deletions(-) create mode 100644 command-center-ai/src/ui/Dialog.client.test.tsx diff --git a/command-center-ai/CHANGELOG.md b/command-center-ai/CHANGELOG.md index addda5f6..e60ef65b 100644 --- a/command-center-ai/CHANGELOG.md +++ b/command-center-ai/CHANGELOG.md @@ -1,5 +1,22 @@ # Changelog +## Unreleased + +Compatibility axes: sign-off error reporting, dialog Escape handling, and the content of the +packaged `manage-model-providers` skill and its guides. No npm public API or entrypoint, +stylesheet class, browser storage key or value, skill name or path, installer behavior, SDK peer +range, or platform route or payload changes. The access guidance describes the platform's +existing policies; enforcement remains with the platform. No backend deployment or storage +migration is required. The updated skill ships in the next package release. + +- Failed or refused provider sign-off keeps the signed-in card and shows the platform's message + or network error above the providers; starting Sign in or Sign off again clears it (#18). +- The model-provider skill, human guide, and module README describe per-record `user`, `editor`, + and `owner` access policies for credentials and custom providers, including `missing_action` + refusals, while preserving the credential-delivery disclosure (#19). +- Escape in a host dialog opened from `renderSharing` closes only that dialog. Package dialogs + accept Escape from their own panel or the page body and respect an already-handled event (#20). + ## 0.0.15 Compatibility axes: additive npm API on `AgentIcon` (`activity`), on `CommandCenterAgentIcon` diff --git a/command-center-ai/agent_scaffold/skills/model-providers/manage-model-providers/SKILL.md b/command-center-ai/agent_scaffold/skills/model-providers/manage-model-providers/SKILL.md index 001b0817..1f5328ca 100644 --- a/command-center-ai/agent_scaffold/skills/model-providers/manage-model-providers/SKILL.md +++ b/command-center-ai/agent_scaffold/skills/model-providers/manage-model-providers/SKILL.md @@ -39,6 +39,8 @@ import { ModelProviderSettings } from "@dev-mainsequence/command-center-ai"; fails, or is cancelled. The dialog shows only the platform's next step: a browser callback that completes at the platform, or a device code the person enters with the provider. Never collect callback URLs, authorization codes, or other secrets by hand. +- A failed or refused sign-off keeps the card signed in and shows the platform's message or + network error above the providers. Starting Sign in or Sign off again clears it. - Today a card's title is built from the provider id, not from the catalog's name. Leave it: it is reported to the package's owner. @@ -85,7 +87,7 @@ again. The settings never change a session's model; the session's picker does. 3. Against the platform, sign in to a provider and pick one of its models in the thread. -## Named configurations, Environment and sharing +## Named configurations, Environment and access Each configured credential has its own `custom_id`, owner and Environment. The provider integration remains one catalog row. Existing callers may omit a name; @@ -94,12 +96,31 @@ their existing identifier and authentication fields. Pass `organizationEnvironmentUid` to `ModelProviderSettings` for the current Environment. Catalog caches distinguish the person and Environment. Pass -`renderSharing` to embed the host's existing permissions editor; it receives the +`renderSharing` to embed the host's per-record access panel; it receives the record UID, name, owner, Environment and existing resource name. Command Center -uses its standard editor for user, team and workload grants. No credential bytes -are returned to this screen. - -The Details and sharing dialog always shows this before any sharing controls: +passes its Access panel there, where people give and remove policies on the +record. No credential bytes are returned to this screen. A dialog the host opens +in its own portal keeps Details and sharing open when Escape closes the host +dialog; Escape in the inline access panel still closes Details and sharing. + +Each record is decided by its own policies: + +- A credential: `user` selects it and receives it in sessions and workloads, + including local Tau, where it can be copied; `editor` also refreshes it, + signs in to the provider again, revokes it and sees who has access; `owner` + also decides who has access. The credential's person holds `owner`. +- A custom provider: `user` sends requests through it and its models, and its + stored authentication is delivered to the holder's runtime, including local + Tau, where it can be copied; `editor` also changes its endpoint, + authentication and models, deletes it and sees who has access; `owner` also + decides who has access. Its creator holds `owner`. +- Organization admins hold every action on their Organization's records. + +A refused change answers `403` with `code` `missing_action`, the +`required_action` and the `granting_policies` that include it: tell the person +which policy to ask for instead of retrying. + +The Details and sharing dialog always shows this before any access controls: > Sharing this configured provider lets recipients receive and copy its > credentials in their own runtime, including local Tau. Only share with people @@ -111,6 +132,5 @@ The Details and sharing dialog always shows this before any sharing controls: > Organization, and their usage counts against it too. An admin with none of > their own for a provider gets the first one added in that Environment. -Retain the disclosure in details after sharing. A view grant allows credential -use and delivery; it does not authorize editing or revoking the stored credential. -Organization admins hold both on every configured provider without a grant. +Retain the disclosure in details after a policy is given. Removing a policy +stops future delivery; it cannot recall credentials already delivered. diff --git a/command-center-ai/docs/model-providers.md b/command-center-ai/docs/model-providers.md index e990991d..c26bf406 100644 --- a/command-center-ai/docs/model-providers.md +++ b/command-center-ai/docs/model-providers.md @@ -41,6 +41,8 @@ The settings never change a session's model. The composer's picker does, on the - Signing in starts an attempt that the screen follows every 1.5 seconds until it completes, fails, or is cancelled. The dialog shows only the platform's next step: a browser page whose callback completes at the platform, or a device code. The person never pastes a callback URL or a code. +- A failed or refused sign-off keeps the card signed in and shows the platform's message or + network error above the providers. Starting Sign in or Sign off again clears it. - Known issue: a card's title is built from the provider id instead of the catalog's name. ## Organization custom providers @@ -73,7 +75,7 @@ platform token and never through the application's rewrite or a proxy. The [model providers README](../src/model-providers/README.md) records every rule the screens follow. -## Named configurations, Environment and sharing +## Named configurations, Environment and access Each configured credential has its own `custom_id`, owner and Environment. The provider integration remains one catalog row. Existing callers may omit a name; @@ -82,12 +84,31 @@ their existing identifier and authentication fields. Pass `organizationEnvironmentUid` to `ModelProviderSettings` for the current Environment. Catalog caches distinguish the person and Environment. Pass -`renderSharing` to embed the host's existing permissions editor; it receives the +`renderSharing` to embed the host's per-record access panel; it receives the record UID, name, owner, Environment and existing resource name. Command Center -uses its standard editor for user, team and workload grants. No credential bytes -are returned to this screen. - -The Details and sharing dialog always shows this before any sharing controls: +passes its Access panel there, where people give and remove policies on the +record. No credential bytes are returned to this screen. A dialog the host opens +in its own portal keeps Details and sharing open when Escape closes the host +dialog; Escape in the inline access panel still closes Details and sharing. + +Each record is decided by its own policies: + +- A credential: `user` selects it and receives it in sessions and workloads, + including local Tau, where it can be copied; `editor` also refreshes it, + signs in to the provider again, revokes it and sees who has access; `owner` + also decides who has access. The credential's person holds `owner`. +- A custom provider: `user` sends requests through it and its models, and its + stored authentication is delivered to the holder's runtime, including local + Tau, where it can be copied; `editor` also changes its endpoint, + authentication and models, deletes it and sees who has access; `owner` also + decides who has access. Its creator holds `owner`. +- Organization admins hold every action on their Organization's records. + +A refused change answers `403` with `code` `missing_action`, the +`required_action` and the `granting_policies` that include it: tell the person +which policy to ask for instead of retrying. + +The Details and sharing dialog always shows this before any access controls: > Sharing this configured provider lets recipients receive and copy its > credentials in their own runtime, including local Tau. Only share with people @@ -99,6 +120,5 @@ The Details and sharing dialog always shows this before any sharing controls: > Organization, and their usage counts against it too. An admin with none of > their own for a provider gets the first one added in that Environment. -Retain the disclosure in details after sharing. A view grant allows credential -use and delivery; it does not authorize editing or revoking the stored credential. -Organization admins hold both on every configured provider without a grant. +Retain the disclosure in details after a policy is given. Removing a policy +stops future delivery; it cannot recall credentials already delivered. diff --git a/command-center-ai/src/model-providers/ModelProviderSettings.client.test.tsx b/command-center-ai/src/model-providers/ModelProviderSettings.client.test.tsx index 23c5f641..05f2a307 100644 --- a/command-center-ai/src/model-providers/ModelProviderSettings.client.test.tsx +++ b/command-center-ai/src/model-providers/ModelProviderSettings.client.test.tsx @@ -87,6 +87,7 @@ describe("ModelProviderSettings sign-in", () => { (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; vi.useFakeTimers(); vi.clearAllMocks(); + mocks.signOff.mockReset(); delete catalog.providers[0].configuredCredentials; mocks.refetch.mockResolvedValue(undefined); container = document.createElement("div"); @@ -222,4 +223,70 @@ describe("ModelProviderSettings sign-in", () => { expect(mocks.start).toHaveBeenCalledWith(expect.objectContaining({ provider: "openai-codex", customId: "openai-work", organizationEnvironmentUid: "environment-one" })); }); + it.each(["refusal", "network"])("shows a sign-off %s without changing the signed-in card, and clears it on retry", async (failure) => { + catalog.providers[0].configuredCredentials = [{ + uid: "credential-one", customId: "openai-work", userUid: "owner-one", + organizationEnvironmentUid: "environment-one", status: "active", authenticated: true, + }]; + const message = failure === "refusal" + ? "This needs the model-provider-credentials.revoke action." + : "The network connection was lost."; + const sendPlatformRequest = vi.fn(); + if (failure === "refusal") { + sendPlatformRequest.mockResolvedValue(new Response(JSON.stringify({ + detail: message, + code: "missing_action", + required_action: "model-provider-credentials.revoke", + granting_policies: ["model-provider-credentials.editor", "model-provider-credentials.owner"], + }), { status: 403, statusText: "Forbidden", headers: { "Content-Type": "application/json" } })); + } else { + sendPlatformRequest.mockRejectedValue(new Error(message)); + } + const api = await vi.importActual("../backend/model-provider-auth-api.js"); + mocks.signOff.mockImplementationOnce(api.signOffModelProvider); + const testConnection = createChatBackendConnection({ apiBaseUrl: "https://platform.test", sendPlatformRequest }); + const signOffButton = () => Array.from(container.querySelectorAll("button")) + .find((button) => button.textContent === "Sign off openai-work")!; + + await act(async () => root.render()); + await act(async () => signOffButton().click()); + + const request = sendPlatformRequest.mock.calls[0][0] as Request; + expect(request.url).toBe("https://platform.test/api/v1/model-provider-credentials/revoke/"); + expect(request.method).toBe("POST"); + expect(await request.json()).toMatchObject({ provider: "openai-codex", custom_id: "openai-work" }); + expect(container.querySelector(".ms-chat-providers__error")?.textContent).toContain(message); + expect(signOffButton().disabled).toBe(false); + expect(signOffButton().parentElement?.textContent).toContain("Signed in"); + expect(mocks.invalidate).not.toHaveBeenCalled(); + expect(mocks.refetch).not.toHaveBeenCalled(); + + let finishSignOff!: () => void; + mocks.signOff.mockImplementationOnce(() => new Promise((resolve) => { finishSignOff = resolve; })); + await act(async () => signOffButton().click()); + expect(container.querySelector(".ms-chat-providers__error")).toBeNull(); + expect(signOffButton().disabled).toBe(true); + await act(async () => finishSignOff()); + expect(mocks.refetch).toHaveBeenCalledTimes(1); + }); + + it("shows a fallback for an unknown sign-off failure and clears it when starting sign-in", async () => { + catalog.providers[0].configuredCredentials = [{ + uid: "credential-one", customId: "openai-work", userUid: "owner-one", + organizationEnvironmentUid: "environment-one", status: "active", authenticated: true, + }]; + mocks.signOff.mockRejectedValueOnce(null); + mocks.start.mockResolvedValue({ ok: true, statusCode: 201, provider: "openai-codex", attempt: attempt("awaiting_browser") }); + mocks.fetchAttempt.mockResolvedValue(attempt("awaiting_browser")); + await act(async () => root.render()); + await act(async () => { + Array.from(container.querySelectorAll("button")).find((button) => button.textContent === "Sign off openai-work")!.click(); + }); + expect(container.querySelector(".ms-chat-providers__error")?.textContent).toBe("Unable to sign off the provider."); + await act(async () => signIn()!.click()); + expect(container.querySelector(".ms-chat-providers__error")).toBeNull(); + expect(dialog()).not.toBeNull(); + }); + }); diff --git a/command-center-ai/src/model-providers/ModelProviderSettings.tsx b/command-center-ai/src/model-providers/ModelProviderSettings.tsx index e57e7f61..bce036ab 100644 --- a/command-center-ai/src/model-providers/ModelProviderSettings.tsx +++ b/command-center-ai/src/model-providers/ModelProviderSettings.tsx @@ -463,8 +463,9 @@ export function ModelProviderSettings({ auth, connection, notify, organizationEn customId, }); await refresh(); - } catch { - // A failed sign-off leaves the provider as it was; the card still shows it signed in. + } catch (error) { + // The card keeps its state; say why the sign-off did not happen. + setAttemptError(error instanceof Error ? error.message : "Unable to sign off the provider."); } finally { setPendingProvider(null); } diff --git a/command-center-ai/src/model-providers/README.md b/command-center-ai/src/model-providers/README.md index 23002221..1b9cf4d1 100644 --- a/command-center-ai/src/model-providers/README.md +++ b/command-center-ai/src/model-providers/README.md @@ -53,6 +53,8 @@ The screens hold their own state; nothing here needs a query client. cancelled. A failed read is tried once more after a second. An attempt the platform no longer knows (`signin_attempt_not_found`, `signin_attempt_not_active`) closes the dialog and refreshes the catalog. Starting a sign-in while one is in progress opens that attempt. +- A failed or refused sign-off keeps the card as it is and shows the platform's message (or the + network error) above the providers. Starting Sign in or Sign off again clears that message. - A change to a custom provider or model keeps its editor open, with the error, until the platform accepts it; then the list reloads, the editor closes, and `notify` reports it. @@ -104,6 +106,9 @@ The screens hold their own state; nothing here needs a query client. - Keep the test chat dialog rendered outside the provider and model `
` elements. React events bubble through portals, so nesting it would let its events reach the form submit handler. While it is open the provider editor ignores Escape so the draft is not discarded. +- A dialog opened by host content in its own portal, such as Grant access from `renderSharing`, + keeps the underlying settings dialog open on Escape. Escape closes a package dialog only from + inside its panel or the page body, and already-handled Escape events are ignored. - In the test chat only real inputs may look typable. The conversation has no empty state: an empty bordered box with hint text reads as the message field and people click and type into it. The message box is the labelled, highlighted field, takes the keyboard on open (the first credential @@ -125,13 +130,14 @@ The screens hold their own state; nothing here needs a query client. ## Tests `ModelProviderSettings.client.test.tsx` covers the sign-in loop: the attempt is followed until it -completes, an unknown attempt closes the dialog, and a sign-in already in progress opens. +completes, an unknown attempt closes the dialog, and a sign-in already in progress opens. It also +covers refused and failed sign-off, preserving the signed-in card, and clearing errors on retry. `CustomModelProviderSettings.client.test.tsx` covers the atomic provider editor and a test conversation opened from a draft model; `CustomModelTestChatDialog.client.test.tsx` covers the test conversation; `model-provider-auth-state.test.ts` covers the credential labels. -## Named configurations, Environment and sharing +## Named configurations, Environment and access Each configured credential has its own `custom_id`, owner and Environment. The provider integration remains one catalog row. Existing callers may omit a name; @@ -140,12 +146,31 @@ their existing identifier and authentication fields. Pass `organizationEnvironmentUid` to `ModelProviderSettings` for the current Environment. Catalog caches distinguish the person and Environment. Pass -`renderSharing` to embed the host's existing permissions editor; it receives the +`renderSharing` to embed the host's per-record access panel; it receives the record UID, name, owner, Environment and existing resource name. Command Center -uses its standard editor for user, team and workload grants. No credential bytes -are returned to this screen. - -The Details and sharing dialog always shows this before any sharing controls: +passes its Access panel there, where people give and remove policies on the +record. No credential bytes are returned to this screen. A dialog the host opens +in its own portal keeps Details and sharing open when Escape closes the host +dialog; Escape in the inline access panel still closes Details and sharing. + +Each record is decided by its own policies: + +- A credential: `user` selects it and receives it in sessions and workloads, + including local Tau, where it can be copied; `editor` also refreshes it, + signs in to the provider again, revokes it and sees who has access; `owner` + also decides who has access. The credential's person holds `owner`. +- A custom provider: `user` sends requests through it and its models, and its + stored authentication is delivered to the holder's runtime, including local + Tau, where it can be copied; `editor` also changes its endpoint, + authentication and models, deletes it and sees who has access; `owner` also + decides who has access. Its creator holds `owner`. +- Organization admins hold every action on their Organization's records. + +A refused change answers `403` with `code` `missing_action`, the +`required_action` and the `granting_policies` that include it: tell the person +which policy to ask for instead of retrying. + +The Details and sharing dialog always shows this before any access controls: > Sharing this configured provider lets recipients receive and copy its > credentials in their own runtime, including local Tau. Only share with people @@ -157,6 +182,5 @@ The Details and sharing dialog always shows this before any sharing controls: > Organization, and their usage counts against it too. An admin with none of > their own for a provider gets the first one added in that Environment. -Retain the disclosure in details after sharing. A view grant allows credential -use and delivery; it does not authorize editing or revoking the stored credential. -Organization admins hold both on every configured provider without a grant. +Retain the disclosure in details after a policy is given. Removing a policy +stops future delivery; it cannot recall credentials already delivered. diff --git a/command-center-ai/src/ui/Dialog.client.test.tsx b/command-center-ai/src/ui/Dialog.client.test.tsx new file mode 100644 index 00000000..40e0b00c --- /dev/null +++ b/command-center-ai/src/ui/Dialog.client.test.tsx @@ -0,0 +1,83 @@ +// @vitest-environment jsdom + +import { act, useState } from "react"; +import { createPortal } from "react-dom"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { Dialog } from "./Dialog.js"; + +describe("Dialog Escape ownership", () => { + let container: HTMLDivElement; + let root: Root; + + beforeEach(() => { + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => root.unmount()); + container.remove(); + }); + + function escape(target: EventTarget) { + const event = new KeyboardEvent("keydown", { key: "Escape", bubbles: true, cancelable: true }); + act(() => { target.dispatchEvent(event); }); + return event; + } + + it.each(["input", "body"])("closes when Escape belongs to its %s", (target) => { + const onClose = vi.fn(); + act(() => root.render()); + const input = document.querySelector('[role="dialog"] input')!; + if (target === "input") input.focus(); + else input.blur(); + + escape(target === "input" ? input : document.body); + + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it("keeps provider details open when Escape closes a host dialog in a separate portal", () => { + const onClose = vi.fn(); + function HostAccessPanel() { + const [open, setOpen] = useState(true); + return <> + + {open ? createPortal(
{ + if (event.key === "Escape") setOpen(false); + }}>
, document.body) : null} + ; + } + act(() => root.render()); + const hostInput = document.querySelector('input[aria-label="Grant access"]')!; + hostInput.focus(); + + escape(hostInput); + + expect(document.querySelector('input[aria-label="Grant access"]')).toBeNull(); + expect(onClose).not.toHaveBeenCalled(); + expect(document.querySelector('[role="dialog"]')?.textContent).toContain("Provider details"); + + // The host's inline access controls still belong to the package dialog. + const accessInput = document.querySelector('input[aria-label="Access panel"]')!; + accessInput.focus(); + escape(accessInput); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it("respects Escape already handled by content inside the dialog", () => { + const onClose = vi.fn(); + act(() => root.render( + event.preventDefault()} /> + )); + const input = document.querySelector('[role="dialog"] input')!; + input.focus(); + + expect(escape(input).defaultPrevented).toBe(true); + expect(onClose).not.toHaveBeenCalled(); + }); +}); diff --git a/command-center-ai/src/ui/Dialog.tsx b/command-center-ai/src/ui/Dialog.tsx index 1264503b..d13a100b 100644 --- a/command-center-ai/src/ui/Dialog.tsx +++ b/command-center-ai/src/ui/Dialog.tsx @@ -1,4 +1,4 @@ -import { useEffect, type ReactNode } from "react"; +import { useEffect, useRef, type ReactNode } from "react"; import { X } from "lucide-react"; import { Button } from "@dev-mainsequence/command-center-sdk/controls"; @@ -21,7 +21,9 @@ interface DialogProps { /** * A modal dialog in a portal: a backdrop, a panel with a title, an optional description, a close - * button, and a scrolling body. Escape closes it and the page does not scroll while it is open. + * button, and a scrolling body. Escape closes it when focus is inside it or nowhere (the page + * body), and not when another dialog or an already-handled Escape has it. The page does not + * scroll while it is open. * `className` sizes the panel; `headerClassName` and `contentClassName` style the header and body. */ export function Dialog({ @@ -36,6 +38,8 @@ export function Dialog({ open, title, }: DialogProps) { + const panelRef = useRef(null); + useEffect(() => { if (!open) { return undefined; @@ -45,9 +49,14 @@ export function Dialog({ document.body.style.overflow = "hidden"; function handleKeyDown(event: KeyboardEvent) { - if (event.key === "Escape") { - onClose(); + if (event.key !== "Escape" || event.defaultPrevented) { + return; + } + const target = event.target; + if (target instanceof Node && target !== document.body && !panelRef.current?.contains(target)) { + return; } + onClose(); } window.addEventListener("keydown", handleKeyDown); @@ -75,6 +84,7 @@ export function Dialog({