diff --git a/docs/design/agent-custom-secrets/plan.md b/docs/design/agent-custom-secrets/plan.md index b98097f0d9e..72278a6eea2 100644 --- a/docs/design/agent-custom-secrets/plan.md +++ b/docs/design/agent-custom-secrets/plan.md @@ -131,3 +131,48 @@ closed at the next run boundary. Milestone one is implemented and validated in this PR. See [README](README.md) for the shipped behavior and [qa.md](qa.md) for the validation record. Milestone two, the vault policy for readable secrets, is not implemented, so this PR must not close #5703. + +## Follow-up fixes, 2026-09-10 + +Two bugs came out of live use on the OSS team stack after the feature shipped in v0.115.4. +Both are frontend only. They land together on one branch because they touch the same +drawer and the same commit path. + +### Slice A: the secret picker shows nothing (#6733) + +The **Attach a secret** drawer opens at `zIndex` 1000 so it sits above the Advanced +dialog. The shared `SelectContent` portals to `body` at `z-50`, so the option list paints +under the drawer. The list is in the DOM with every secret in it, and nothing is visible. +The same applies to the two selects inside `SecretForm` when it renders in +`CreateSecretDrawer` at 1100. + +Change: the drawer passes `zIndex + 1` down to every `SelectContent` it renders, and +`SecretForm` takes an optional `popupZIndex` for its own selects. The drawer also refetches +the vault list when it opens, because the vault query never refetches on its own once the +page holds a subscriber. + +Acceptance: with two text secrets in the vault, open Advanced, Custom secrets, Attach, +click the Secret select. Both names are visible on screen in a screenshot, not only in the +accessibility tree. Create a secret in another tab, reopen the drawer, and the new name is +listed without a reload. + +### Slice B: attach fails with a 409 and Retry can never succeed (#6734) + +`commitAgentCredentialsAtom` sends `base_revision_id` equal to the revision the panel +displays. The server rejects any base that is not the head. When the panel shows an older +revision, every attach fails, and the drawer prints the raw wire error. + +Change: the commit reads the variant head first and builds the bindings-only revision on +top of the head's data. The displayed revision supplies the variant id, the dirty check, +and the attachments the user was looking at. When the head's attachments differ from +those, the atom refuses with a plain message and the user reloads, because the callers +send the full list and a commit would undo the other change. This keeps the review rule +from step 4 of the request flow above. If the head moves between the read and the commit, +the atom re-reads once and retries under the same rule. Edits typed during the request are +carried to the adopted revision only when it was built on the displayed revision. The +lost-response recovery stays. + +Acceptance: with the panel on an older revision and a newer head, attach succeeds and the +panel adopts the new head. The unit suite in +`web/packages/agenta-entities/tests/unit/agent-credentials-commit.test.ts` covers the head +anchor, the single retry, and the plain conflict message. diff --git a/docs/design/agent-custom-secrets/qa.md b/docs/design/agent-custom-secrets/qa.md index da44a5f47cc..f0f9dcf8351 100644 --- a/docs/design/agent-custom-secrets/qa.md +++ b/docs/design/agent-custom-secrets/qa.md @@ -26,8 +26,10 @@ printing secret values. environment owners retain their values and policies. - Missing, deleted, wrong-project, wrong-kind, empty, and unreadable secrets fail before harness execution. API writes enforce the same permission rule as the editor. -- A moved `base_revision_id` produces a conflict without overwriting another edit. Retry - confirms the selected binding against the current revision. +- An attachment commits on the variant head, whatever revision the panel shows, and keeps + the head's other fields. If the head's attachments differ from the ones the panel showed, + or the head moves twice during the commit, the drawer asks for a reload instead of + overwriting them. ## Card completion and recovery diff --git a/docs/design/agent-custom-secrets/status.md b/docs/design/agent-custom-secrets/status.md index dc9fae0fc9f..289423451c9 100644 --- a/docs/design/agent-custom-secrets/status.md +++ b/docs/design/agent-custom-secrets/status.md @@ -2,6 +2,8 @@ ## Current phase +2026-09-10: follow-up fixes for #6733 (picker renders behind the drawer) and #6734 (attach anchored on a stale revision) are on PR #6743 (`fix/custom-secret-attach-drawer`, base `release/v0.116.0`). Both were verified live on an isolated EE dev stack built from the branch: the picker draws above the drawer on desktop and `/m`, an attach from a tab on an older revision lands on the head and keeps the head's edits, a head whose attachments changed refuses with a reload message, and the chat `request_secret` flow attaches, settles, and resumes. See the plan's follow-up section. + Implementation and independent review are complete. Runtime, SDK, runner, shared entity, shared UI, desktop, and mobile paths are present in the isolated feature worktree. The real-application request, resume, and targeted recovery checks passed. The remaining runtime matrix is listed below. ## Shipped decisions diff --git a/web/packages/agenta-entities/src/secret/state/atoms.ts b/web/packages/agenta-entities/src/secret/state/atoms.ts index ccfc14b5363..1023bab1511 100644 --- a/web/packages/agenta-entities/src/secret/state/atoms.ts +++ b/web/packages/agenta-entities/src/secret/state/atoms.ts @@ -41,7 +41,9 @@ import type {QueryKey} from "@tanstack/react-query" import {atom} from "jotai" import {atomWithStorage} from "jotai/utils" import {atomWithMutation, atomWithQuery} from "jotai-tanstack-query" +import {z} from "zod" +import {safeParseWithLogging} from "../../shared/utils/zodSchema" import {createVaultSecret, deleteVaultSecret, fetchVaultSecret, updateVaultSecret} from "../api/api" import { getEnvNameMap, @@ -349,9 +351,9 @@ export const deleteSecretAtom = atom(null, async (get, set, provider: LlmProvide * migrated. The hook's `useEffect` is responsible for the user-presence * trigger and the logout reset (re-arm). * - * On success, sets `{migrating: false, migrated: true}`. - * On failure, rolls back to `{migrating: false, migrated: false}` so the - * next mount can retry. + * Always ends in `{migrating: false, migrated: true}`, on failure too: a legacy payload + * that cannot be parsed stays backed up in localStorage, and the vault UI must never wait + * on it. The server list is the source of truth. */ export const migrateVaultKeysAtom = atom(null, async (get, set) => { const migrationStatus = get(vaultMigrationAtom) @@ -366,24 +368,67 @@ export const migrateVaultKeysAtom = atom(null, async (get, set) => { const localStorageProviders = localStorage.getItem(llmAvailableProvidersToken) if (localStorageProviders) { - const _providers = JSON.parse(localStorageProviders) - const providers = JSON.parse(_providers) - - for (const provider of providers) { - if (provider.key) { - await set(createStandardSecretAtom, provider as LlmProvider) + const providers = parseLegacyProviders(localStorageProviders) + const failed: LlmProvider[] = [] + if (providers) { + for (const provider of providers) { + if (!provider.key) continue + try { + await set(createStandardSecretAtom, provider) + } catch (error) { + // One bad entry must not stop the others; it stays for the next load. + console.error("[vault] Legacy provider key was not migrated:", error) + failed.push(provider) + } } + } else { + console.error( + "[vault] Legacy provider keys could not be parsed; leaving them backed up.", + ) } - // Create backup and cleanup + // Keep a backup, then leave only the entries that still need a retry in place, in + // the canonical double-encoded form. A later page load picks them up again. localStorage.setItem(`${llmAvailableProvidersToken}Backup`, localStorageProviders) - localStorage.removeItem(llmAvailableProvidersToken) + if (failed.length > 0) { + localStorage.setItem( + llmAvailableProvidersToken, + JSON.stringify(JSON.stringify(failed)), + ) + } else { + localStorage.removeItem(llmAvailableProvidersToken) + } } - - set(vaultMigrationAtom, {migrating: false, migrated: true}) } catch (error) { + // A failed migration must not hold the vault UI in its loading state: the keys stay + // in localStorage for a later attempt, and the app keeps working with the server list. console.error("Migration failed:", error) - set(vaultMigrationAtom, {migrating: false, migrated: false}) - throw error + } finally { + set(vaultMigrationAtom, {migrating: false, migrated: true}) } }) + +/** One legacy entry: whatever else it carried, only an object with a string key can migrate. */ +const legacyProviderSchema = z.object({key: z.string().nullish()}).passthrough() + +/** + * The legacy localStorage payload was double-encoded JSON (a JSON string holding JSON), but + * older builds wrote it once. Accept both; return null when the payload is not a list. Each + * entry is validated on its own, so one malformed entry does not block the valid ones. + */ +function parseLegacyProviders(raw: string): LlmProvider[] | null { + let parsed: unknown + try { + parsed = JSON.parse(raw) + if (typeof parsed === "string") parsed = JSON.parse(parsed) + } catch { + return null + } + if (!Array.isArray(parsed)) return null + const providers: LlmProvider[] = [] + for (const entry of parsed) { + const valid = safeParseWithLogging(legacyProviderSchema, entry, "[vault] legacy provider") + if (valid) providers.push(valid as unknown as LlmProvider) + } + return providers +} diff --git a/web/packages/agenta-entities/src/secret/state/useVaultSecret.ts b/web/packages/agenta-entities/src/secret/state/useVaultSecret.ts index 88ba92fbc42..e78872dd901 100644 --- a/web/packages/agenta-entities/src/secret/state/useVaultSecret.ts +++ b/web/packages/agenta-entities/src/secret/state/useVaultSecret.ts @@ -116,9 +116,12 @@ export const useVaultSecret = () => { vaultQuery.refetch() }, [vaultQuery]) + // "Not migrated yet" is not a loading state: the migration only moves legacy localStorage + // keys into the vault, and a failed or never-run migration must not hold every secret + // picker on "Loading" (#6733 follow-up). The server list is the source of truth. const loading = useMemo(() => { - return vaultQuery.isPending || migrationStatus.migrating || !migrationStatus.migrated - }, [vaultQuery.isPending, migrationStatus.migrating, migrationStatus.migrated]) + return vaultQuery.isPending || migrationStatus.migrating + }, [vaultQuery.isPending, migrationStatus.migrating]) return { loading, diff --git a/web/packages/agenta-entities/src/workflow/index.ts b/web/packages/agenta-entities/src/workflow/index.ts index e20b3b82580..64cdfb5d150 100644 --- a/web/packages/agenta-entities/src/workflow/index.ts +++ b/web/packages/agenta-entities/src/workflow/index.ts @@ -499,4 +499,7 @@ export type { export {agentRosterSearchAtom, matchesAgentQuery} from "./state/agentRoster" -export {commitAgentCredentialsAtom} from "./state/agentCredentials" +export { + commitAgentCredentialsAtom, + AGENT_CREDENTIALS_CONFLICT_MESSAGE, +} from "./state/agentCredentials" diff --git a/web/packages/agenta-entities/src/workflow/state/agentCredentials.ts b/web/packages/agenta-entities/src/workflow/state/agentCredentials.ts index a1a834b27e4..abb0dab1bb9 100644 --- a/web/packages/agenta-entities/src/workflow/state/agentCredentials.ts +++ b/web/packages/agenta-entities/src/workflow/state/agentCredentials.ts @@ -7,7 +7,7 @@ import {atom} from "jotai" import type {AgentSecretBinding} from "../../secret/core/types" import {safeParseWithLogging} from "../../shared/utils/zodSchema" import {retrieveWorkflowRevision} from "../api" -import {workflowRevisionResponseSchema} from "../core/schema" +import {workflowRevisionResponseSchema, type Workflow} from "../core/schema" import {invokeWorkflowCommitCallbacks} from "./commit" import { @@ -20,7 +20,59 @@ import { invalidateWorkflowRevisionsByVariantCache, } from "./store" +/** The message the drawer shows when the attachments changed under it. Names the fix. */ +export const AGENT_CREDENTIALS_CONFLICT_MESSAGE = + "This agent's secret attachments changed while you were editing them. Reload the configuration and attach again." + +const isRevisionConflict = (error: unknown): boolean => { + const candidate = error as {statusCode?: number; body?: unknown} | null + if (candidate?.statusCode !== 409) return false + const detail = (candidate.body as {detail?: {code?: string}} | undefined)?.detail + return detail?.code === "revision_conflict" +} + +const credentialsOf = (revision: Pick): unknown => { + const parameters = revision.data?.parameters as Record | undefined + const agent = parameters?.agent as Record | undefined + const sandbox = agent?.sandbox as Record | undefined + return sandbox?.credentials ?? [] +} + +/** The revision data with only the credentials replaced. Every other field rides along. */ +const withCredentials = ( + base: Pick, + bindings: AgentSecretBinding[], +): AgentaApi.WorkflowRevisionDataInput => { + const parameters = base.data?.parameters as Record | undefined + const agent = parameters?.agent as Record | undefined + if (!agent) throw new Error("This revision has no agent configuration.") + const sandbox = (agent.sandbox ?? {}) as Record + return { + ...base.data, + parameters: { + ...parameters, + agent: { + ...agent, + sandbox: { + ...sandbox, + credentials: bindings.map(({secret, binding}) => ({ + secret: {slug: secret.slug}, + binding: {type: binding.type, name: binding.name}, + })), + }, + }, + }, + } as AgentaApi.WorkflowRevisionDataInput +} + // A binding commit snapshots server configuration and never consumes unrelated editor changes. +// +// The commit anchors on the variant HEAD, not on the revision the panel displays. A secret +// attachment touches only `agent.sandbox.credentials`, so building it on the head keeps every +// newer edit and never fails because the panel sat on an older revision (#6734). The displayed +// revision names the variant, gates on unsaved edits, and supplies the attachments the user +// was looking at: the callers send the full list, so a head whose attachments differ from +// the displayed ones would be overwritten. That case asks for a reload instead. export const commitAgentCredentialsAtom = atom( null, async ( @@ -36,60 +88,76 @@ export const commitAgentCredentialsAtom = atom( if (get(workflowIsDirtyAtomFamily(revisionId))) { throw new Error("Save or discard your configuration changes before attaching a secret.") } - const parameters = entity.data.parameters as Record | undefined - const agent = parameters?.agent as Record | undefined - if (!agent) throw new Error("This revision has no agent configuration.") - const sandbox = (agent.sandbox ?? {}) as Record - const data = { - ...entity.data, - parameters: { - ...parameters, - agent: { - ...agent, - sandbox: { - ...sandbox, - credentials: bindings.map(({secret, binding}) => ({ - secret: {slug: secret.slug}, - binding: {type: binding.type, name: binding.name}, - })), - }, - }, - }, - } as AgentaApi.WorkflowRevisionDataInput - let revision - try { - const response = await getWorkflowsClient().commitWorkflowRevision( - { - workflow_revision: { - workflow_id: entity.workflow_id, - workflow_variant_id: entity.workflow_variant_id, - base_revision_id: revisionId, - data, - message: "Update agent secret attachments", - }, - }, - {queryParams: {project_id: projectId}}, + const variantId = entity.workflow_variant_id + const readHead = () => + retrieveWorkflowRevision({projectId, workflowVariantRef: {id: variantId}}).catch( + () => null, ) - revision = safeParseWithLogging( - workflowRevisionResponseSchema, - response, - "[commitAgentCredentials]", - )?.workflow_revision - if (!revision) throw new Error("The server did not return the saved agent revision.") - } catch (error) { - // A lost response may hide a successful commit. Recover only the exact intended - // configuration; a different head must be reviewed, never silently overwritten. - const latest = await retrieveWorkflowRevision({ - projectId, - workflowVariantRef: {id: entity.workflow_variant_id}, - }).catch(() => null) - if (!latest || latest.id === revisionId || !isEqual(latest.data, data)) throw error - revision = latest + + const displayedCredentials = credentialsOf(entity) + const commitOn = async (base: Pick) => { + if (base.id !== revisionId && !isEqual(credentialsOf(base), displayedCredentials)) { + // Someone attached, edited, or removed a secret since the panel loaded. The + // caller's list would silently undo that, so the user reviews it first. + throw new Error(AGENT_CREDENTIALS_CONFLICT_MESSAGE) + } + const data = withCredentials(base, bindings) + try { + const response = await getWorkflowsClient().commitWorkflowRevision( + { + workflow_revision: { + workflow_id: base.workflow_id ?? entity.workflow_id, + workflow_variant_id: variantId, + base_revision_id: base.id, + data, + message: "Update agent secret attachments", + }, + }, + {queryParams: {project_id: projectId}}, + ) + const revision = safeParseWithLogging( + workflowRevisionResponseSchema, + response, + "[commitAgentCredentials]", + )?.workflow_revision + if (!revision) { + throw new Error("The server did not return the saved agent revision.") + } + return {revision, conflict: false as const, base: base.id} + } catch (error) { + if (isRevisionConflict(error)) { + return {revision: null, conflict: true as const, base: base.id} + } + // A lost response may hide a successful commit. Recover only the exact intended + // configuration; a different head must be reviewed, never silently overwritten. + const latest = await readHead() + if (!latest || latest.id === base.id || !isEqual(latest.data, data)) throw error + return {revision: latest, conflict: false as const, base: base.id} + } } + + // The head read can fail (offline, permission). Then the displayed revision is the best + // base we have, and the server's conflict check still protects newer configuration. + const head = (await readHead()) ?? entity + let outcome = await commitOn(head) + if (outcome.conflict) { + // The head moved between the read and the commit: one re-read, one retry. + const moved = await readHead() + outcome = moved ? await commitOn(moved) : outcome + } + if (outcome.conflict || !outcome.revision) { + throw new Error(AGENT_CREDENTIALS_CONFLICT_MESSAGE) + } + const revision = outcome.revision + primeWorkflowRevisionDetailCacheImperative(revision) primeCommittedRevisionRefLists(revision) - invalidateWorkflowRevisionsByVariantCache(entity.workflow_variant_id) - const concurrentDraft = get(workflowDraftAtomFamily(revisionId)) + invalidateWorkflowRevisionsByVariantCache(variantId) + // Edits typed during the request live on the displayed revision's draft. Carry them to + // the adopted revision only when it was built on that same revision: copied onto a + // newer head they would revert the head's other fields locally. + const concurrentDraft = + outcome.base === revisionId ? get(workflowDraftAtomFamily(revisionId)) : null if (concurrentDraft) { // Preserve edits typed while the binding request was in flight on the adopted revision. const draftParameters = concurrentDraft.data?.parameters as diff --git a/web/packages/agenta-entities/tests/unit/agent-credentials-commit.test.ts b/web/packages/agenta-entities/tests/unit/agent-credentials-commit.test.ts index 08661e274f4..add3474a8c4 100644 --- a/web/packages/agenta-entities/tests/unit/agent-credentials-commit.test.ts +++ b/web/packages/agenta-entities/tests/unit/agent-credentials-commit.test.ts @@ -24,7 +24,10 @@ vi.mock("../../src/workflow/state/store", async () => { invalidateWorkflowRevisionsByVariantCache: vi.fn(), } }) -import {commitAgentCredentialsAtom} from "../../src/workflow/state/agentCredentials" +import { + AGENT_CREDENTIALS_CONFLICT_MESSAGE, + commitAgentCredentialsAtom, +} from "../../src/workflow/state/agentCredentials" import { workflowEntityAtomFamily, workflowIsDirtyAtomFamily, @@ -40,12 +43,29 @@ const base = { workflow_variant_id: "variant-1", data: {parameters: {agent: {instructions: "Keep this prompt", sandbox: {kind: "daytona"}}}}, } +/** A newer head the panel has not adopted: same variant, one more edit. */ +const head = { + ...base, + id: "rev-head", + data: { + parameters: { + agent: {instructions: "Edited after the panel loaded", sandbox: {kind: "daytona"}}, + }, + }, +} +const conflict = () => + Object.assign(new Error("Status code: 409"), { + statusCode: 409, + body: {detail: {code: "revision_conflict"}}, + }) + let store: ReturnType beforeEach(() => { vi.resetAllMocks() store = createStore() store.set(projectIdAtom, "project-1") store.set(workflowEntityAtomFamily("rev-1") as ReturnType>, base) + api.retrieve.mockResolvedValue(base) api.commit.mockImplementation(async ({workflow_revision}) => ({ workflow_revision: {...base, ...workflow_revision, id: "rev-2"}, })) @@ -53,7 +73,7 @@ beforeEach(() => { }) describe("secret attachment transaction", () => { - it("commits reference-only bindings against the pinned base and preserves the agent configuration", async () => { + it("commits reference-only bindings against the head and preserves the agent configuration", async () => { await expect( store.set(commitAgentCredentialsAtom, {revisionId: "rev-1", bindings}), ).resolves.toEqual({revisionId: "rev-2"}) @@ -71,10 +91,123 @@ describe("secret attachment transaction", () => { }, }) expect(options.queryParams.project_id).toBe("project-1") + expect(api.retrieve).toHaveBeenCalledTimes(1) + expect(api.retrieve).toHaveBeenCalledWith( + expect.objectContaining({workflowVariantRef: {id: "variant-1"}}), + ) + expect(api.adopt).toHaveBeenCalledWith(expect.objectContaining({newRevisionId: "rev-2"}), { + revisionId: "rev-1", + }) + }) + it("anchors on a newer head when the panel displays an older revision (#6734)", async () => { + api.retrieve.mockResolvedValue(head) + await expect( + store.set(commitAgentCredentialsAtom, {revisionId: "rev-1", bindings}), + ).resolves.toEqual({revisionId: "rev-2"}) + const [payload] = api.commit.mock.calls[0] + expect(payload.workflow_revision).toMatchObject({ + base_revision_id: "rev-head", + data: { + parameters: { + agent: { + instructions: "Edited after the panel loaded", + sandbox: {kind: "daytona", credentials: bindings}, + }, + }, + }, + }) expect(api.adopt).toHaveBeenCalledWith(expect.objectContaining({newRevisionId: "rev-2"}), { revisionId: "rev-1", }) }) + it("falls back to the displayed revision when the head cannot be read", async () => { + api.retrieve.mockRejectedValue(new Error("offline")) + await store.set(commitAgentCredentialsAtom, {revisionId: "rev-1", bindings}) + expect(api.commit.mock.calls[0][0].workflow_revision.base_revision_id).toBe("rev-1") + }) + it("re-reads once and retries when the head moves during the commit", async () => { + const moved = {...head, id: "rev-moved"} + api.retrieve.mockResolvedValueOnce(base).mockResolvedValueOnce(moved) + api.commit + .mockRejectedValueOnce(conflict()) + .mockImplementationOnce(async ({workflow_revision}) => ({ + workflow_revision: {...moved, ...workflow_revision, id: "rev-3"}, + })) + await expect( + store.set(commitAgentCredentialsAtom, {revisionId: "rev-1", bindings}), + ).resolves.toEqual({revisionId: "rev-3"}) + expect(api.commit).toHaveBeenCalledTimes(2) + expect(api.commit.mock.calls[1][0].workflow_revision.base_revision_id).toBe("rev-moved") + }) + it("gives up with a plain message after a second conflict", async () => { + api.retrieve.mockResolvedValue(head) + api.commit.mockRejectedValue(conflict()) + await expect( + store.set(commitAgentCredentialsAtom, {revisionId: "rev-1", bindings}), + ).rejects.toThrow(AGENT_CREDENTIALS_CONFLICT_MESSAGE) + expect(api.commit).toHaveBeenCalledTimes(2) + expect(api.adopt).not.toHaveBeenCalled() + }) + it("refuses when the head's attachments differ from the displayed ones", async () => { + const other = {secret: {slug: "other"}, binding: {type: "env" as const, name: "OTHER"}} + api.retrieve.mockResolvedValue({ + ...head, + data: { + parameters: { + agent: { + ...head.data.parameters.agent, + sandbox: {kind: "daytona", credentials: [other]}, + }, + }, + }, + }) + await expect( + store.set(commitAgentCredentialsAtom, {revisionId: "rev-1", bindings}), + ).rejects.toThrow(AGENT_CREDENTIALS_CONFLICT_MESSAGE) + expect(api.commit).not.toHaveBeenCalled() + expect(api.adopt).not.toHaveBeenCalled() + }) + it("refuses when the head that moved during the commit carries other attachments", async () => { + const other = {secret: {slug: "other"}, binding: {type: "env" as const, name: "OTHER"}} + const moved = { + ...head, + id: "rev-moved", + data: { + parameters: { + agent: { + ...head.data.parameters.agent, + sandbox: {kind: "daytona", credentials: [other]}, + }, + }, + }, + } + api.retrieve.mockResolvedValueOnce(base).mockResolvedValueOnce(moved) + api.commit.mockRejectedValueOnce(conflict()) + await expect( + store.set(commitAgentCredentialsAtom, {revisionId: "rev-1", bindings}), + ).rejects.toThrow(AGENT_CREDENTIALS_CONFLICT_MESSAGE) + expect(api.commit).toHaveBeenCalledTimes(1) + }) + it("treats a 409 without the conflict code as a plain failure", async () => { + api.commit.mockRejectedValue( + Object.assign(new Error("Status code: 409"), {statusCode: 409, body: ""}), + ) + await expect( + store.set(commitAgentCredentialsAtom, {revisionId: "rev-1", bindings}), + ).rejects.toThrow("Status code: 409") + expect(api.commit).toHaveBeenCalledTimes(1) + }) + it("does not carry an in-flight draft onto a revision built on a newer head", async () => { + api.retrieve.mockResolvedValue(head) + api.commit.mockImplementation(async ({workflow_revision}) => { + store.set(workflowDraftAtomFamily("rev-1"), { + data: {parameters: {agent: {instructions: "Typed while saving"}}}, + } as never) + return {workflow_revision: {...head, ...workflow_revision, id: "rev-2"}} + }) + await store.set(commitAgentCredentialsAtom, {revisionId: "rev-1", bindings}) + expect(store.get(workflowDraftAtomFamily("rev-2"))).toBeNull() + }) it("does not commit or consume unrelated unsaved edits", async () => { store.set(workflowIsDirtyAtomFamily("rev-1") as ReturnType>, true) await expect( @@ -84,7 +217,7 @@ describe("secret attachment transaction", () => { }) it("recovers a lost success response without creating another revision", async () => { api.commit.mockRejectedValue(new Error("Network disconnected")) - api.retrieve.mockResolvedValue({ + api.retrieve.mockResolvedValueOnce(base).mockResolvedValueOnce({ ...base, id: "rev-2", data: { @@ -101,17 +234,17 @@ describe("secret attachment transaction", () => { ).resolves.toEqual({revisionId: "rev-2"}) expect(api.commit).toHaveBeenCalledTimes(1) }) - it("does not adopt or overwrite a concurrent configuration change", async () => { - api.commit.mockRejectedValue(new Error("Revision conflict")) - api.retrieve.mockResolvedValue({...base, id: "rev-other"}) + it("does not adopt or overwrite a concurrent configuration change on a plain failure", async () => { + api.commit.mockRejectedValue(new Error("Network disconnected")) + api.retrieve.mockResolvedValueOnce(base).mockResolvedValueOnce({...base, id: "rev-other"}) await expect( store.set(commitAgentCredentialsAtom, {revisionId: "rev-1", bindings}), - ).rejects.toThrow("Revision conflict") + ).rejects.toThrow("Network disconnected") expect(api.adopt).not.toHaveBeenCalled() }) it("does not mistake a changed schema for the lost attachment response", async () => { - api.commit.mockRejectedValue(new Error("Revision conflict")) - api.retrieve.mockResolvedValue({ + api.commit.mockRejectedValue(new Error("Network disconnected")) + api.retrieve.mockResolvedValueOnce(base).mockResolvedValueOnce({ ...base, id: "rev-other", data: { @@ -126,7 +259,7 @@ describe("secret attachment transaction", () => { }) await expect( store.set(commitAgentCredentialsAtom, {revisionId: "rev-1", bindings}), - ).rejects.toThrow("Revision conflict") + ).rejects.toThrow("Network disconnected") expect(api.adopt).not.toHaveBeenCalled() }) diff --git a/web/packages/agenta-entities/tests/unit/vault-migration.test.ts b/web/packages/agenta-entities/tests/unit/vault-migration.test.ts new file mode 100644 index 00000000000..6aa58998a3a --- /dev/null +++ b/web/packages/agenta-entities/tests/unit/vault-migration.test.ts @@ -0,0 +1,80 @@ +// @vitest-environment jsdom +import {createStore} from "jotai" +import {beforeEach, describe, expect, it, vi} from "vitest" +import {projectIdAtom} from "@agenta/shared/state" +import {llmAvailableProvidersToken} from "@agenta/shared/utils" + +const api = vi.hoisted(() => ({create: vi.fn()})) +vi.mock("../../src/secret/api/api", () => ({ + fetchVaultSecret: vi.fn(async () => []), + createVaultSecret: api.create, + updateVaultSecret: vi.fn(), + deleteVaultSecret: vi.fn(), +})) + +import {migrateVaultKeysAtom, vaultMigrationAtom} from "../../src/secret/state/atoms" + +let store: ReturnType +beforeEach(() => { + vi.resetAllMocks() + localStorage.clear() + store = createStore() + store.set(projectIdAtom, "project-1") +}) + +describe("legacy vault key migration", () => { + it("marks the migration done when there is nothing to migrate", async () => { + await store.set(migrateVaultKeysAtom) + expect(store.get(vaultMigrationAtom)).toEqual({migrating: false, migrated: true}) + }) + + it("does not stay pending when the legacy payload cannot be parsed", async () => { + localStorage.setItem(llmAvailableProvidersToken, "{not json") + await store.set(migrateVaultKeysAtom) + expect(store.get(vaultMigrationAtom)).toEqual({migrating: false, migrated: true}) + expect(localStorage.getItem(llmAvailableProvidersToken)).toBeNull() + expect(localStorage.getItem(`${llmAvailableProvidersToken}Backup`)).toBe("{not json") + }) + + it("skips null and keyless entries, migrates the rest, and still backs up and clears", async () => { + localStorage.setItem( + llmAvailableProvidersToken, + JSON.stringify( + JSON.stringify([null, {title: "openai"}, {name: "OPENAI_API_KEY", key: "k"}]), + ), + ) + api.create.mockResolvedValue({}) + await store.set(migrateVaultKeysAtom) + expect(store.get(vaultMigrationAtom)).toEqual({migrating: false, migrated: true}) + expect(api.create).toHaveBeenCalledTimes(1) + expect(localStorage.getItem(llmAvailableProvidersToken)).toBeNull() + expect(localStorage.getItem(`${llmAvailableProvidersToken}Backup`)).toContain("openai") + }) + + it("keeps going when one legacy entry fails to save", async () => { + localStorage.setItem( + llmAvailableProvidersToken, + JSON.stringify([ + {name: "OPENAI_API_KEY", key: "k1"}, + {name: "COHERE_API_KEY", key: "k2"}, + ]), + ) + api.create.mockRejectedValueOnce(new Error("boom")).mockResolvedValueOnce({}) + await store.set(migrateVaultKeysAtom) + expect(api.create).toHaveBeenCalledTimes(2) + expect(store.get(vaultMigrationAtom)).toEqual({migrating: false, migrated: true}) + // The entry that failed stays behind for the next page load; the saved one is gone. + const left = JSON.parse( + JSON.parse(localStorage.getItem(llmAvailableProvidersToken) ?? '""'), + ) + expect(left).toEqual([{name: "OPENAI_API_KEY", key: "k1"}]) + expect(localStorage.getItem(`${llmAvailableProvidersToken}Backup`)).toContain("COHERE") + }) + + it("accepts a payload that an older build wrote once instead of twice", async () => { + localStorage.setItem(llmAvailableProvidersToken, JSON.stringify([{title: "openai"}])) + await store.set(migrateVaultKeysAtom) + expect(store.get(vaultMigrationAtom)).toEqual({migrating: false, migrated: true}) + expect(localStorage.getItem(llmAvailableProvidersToken)).toBeNull() + }) +}) diff --git a/web/packages/agenta-entity-ui/src/secret/AgentSecretAttachmentDrawer.tsx b/web/packages/agenta-entity-ui/src/secret/AgentSecretAttachmentDrawer.tsx index 9c32592aa3b..d8576f029a7 100644 --- a/web/packages/agenta-entity-ui/src/secret/AgentSecretAttachmentDrawer.tsx +++ b/web/packages/agenta-entity-ui/src/secret/AgentSecretAttachmentDrawer.tsx @@ -110,7 +110,11 @@ export function AgentSecretAttachmentDrawer({ onAttached, zIndex = 1000, }: AgentSecretAttachmentDrawerProps) { - const {namedSecrets, loading} = useVaultSecret() + const {namedSecrets, loading, mutate: refetchVault} = useVaultSecret() + // The drawer stacks above the Advanced dialog, and the shared Select portals its list to + // at z-50. Without this the options paint UNDER the drawer and the picker looks + // empty (#6733). One layer above the drawer is enough; nothing else sits between them. + const popupZIndex = zIndex + 1 const textSecrets = useMemo( () => namedSecrets.filter((secret) => secret.format === CustomSecretFormat.Text), [namedSecrets], @@ -137,6 +141,15 @@ export function AgentSecretAttachmentDrawer({ name: secret?.name || request?.name || "", }) + useEffect(() => { + if (!open) return + // The vault query keeps a live subscriber for the whole page, so nothing refetches it on + // its own; a secret created in Settings or another tab stays invisible until a reload. + // `refetchVault` is keyed on the query result and changes identity on every fetch, so + // it must stay out of the deps or this effect refetches forever. + refetchVault() + }, [open]) + useEffect(() => { if (!open) return const original = editingBinding?.value @@ -303,7 +316,7 @@ export function AgentSecretAttachmentDrawer({ } /> - + {allSecrets.map((secret) => ( {secret.name} @@ -346,7 +359,11 @@ export function AgentSecretAttachmentDrawer({ - + diff --git a/web/packages/agenta-entity-ui/src/secret/CreateSecretDrawer.tsx b/web/packages/agenta-entity-ui/src/secret/CreateSecretDrawer.tsx index c61f255af73..5621da20bf3 100644 --- a/web/packages/agenta-entity-ui/src/secret/CreateSecretDrawer.tsx +++ b/web/packages/agenta-entity-ui/src/secret/CreateSecretDrawer.tsx @@ -98,7 +98,7 @@ export function CreateSecretDrawer({ } styles={{body: {padding: 16}, footer: FOOTER_STYLE}} > - + ) } diff --git a/web/packages/agenta-entity-ui/src/secret/SecretForm/SecretForm.tsx b/web/packages/agenta-entity-ui/src/secret/SecretForm/SecretForm.tsx index 03b15535b2b..cfa389e266b 100644 --- a/web/packages/agenta-entity-ui/src/secret/SecretForm/SecretForm.tsx +++ b/web/packages/agenta-entity-ui/src/secret/SecretForm/SecretForm.tsx @@ -39,6 +39,11 @@ export interface SecretFormProps { controller: SecretFormController /** Attachment flows accept readable text secrets only. */ textOnly?: boolean + /** + * Layer for the select popups. They portal to at z-50, so a host drawer above that + * (the attach and create drawers sit at 1000 and 1100) hides them unless told the layer. + */ + popupZIndex?: number } const formatOptions = [ @@ -60,7 +65,8 @@ const HintText = ({children}: {children: React.ReactNode}) => ( {children} ) -export function SecretForm({controller, textOnly = false}: SecretFormProps) { +export function SecretForm({controller, textOnly = false, popupZIndex}: SecretFormProps) { + const popupStyle = popupZIndex != null ? {zIndex: popupZIndex} : undefined const { isEditing, name, @@ -251,7 +257,7 @@ export function SecretForm({controller, textOnly = false}: SecretFormProps) { - + true false @@ -283,7 +289,7 @@ export function SecretForm({controller, textOnly = false}: SecretFormProps) { - + {PRIMITIVE_TYPES.map((t) => ( {t}