From df5dc870e2ce4fe933d7415bbff1469ad1107ef5 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Thu, 10 Sep 2026 12:15:28 +0200 Subject: [PATCH 1/6] fix(frontend): show the secret picker above the drawer and anchor the attach on the head revision The Attach a secret drawer sits at z-index 1000 above the Advanced dialog, and the shared Select portals its option list to body at z-50, so the list painted under the drawer and the picker looked empty. The drawer and the secret form now pass the layer above the drawer to every SelectContent they render. The drawer also refetches the vault list when it opens, because the vault query keeps a live subscriber for the whole page and nothing else ever refetched it. The bindings commit sent base_revision_id equal to the revision the panel displayed, and the server rejects any base that is not the head, so every attach from a panel on an older revision failed with a 409 and Retry resent the same stale base. The commit now reads the variant head first and builds the bindings-only revision on the head's data; on a real race it re-reads once and retries, then raises a plain message. Closes #6733 Closes #6734 Claude-Session: https://claude.ai/code/session_01AY886ajXX65KAa1Vc9JCXU --- docs/design/agent-custom-secrets/plan.md | 41 +++++ docs/design/agent-custom-secrets/status.md | 2 + .../agenta-entities/src/workflow/index.ts | 5 +- .../src/workflow/state/agentCredentials.ts | 147 ++++++++++++------ .../unit/agent-credentials-commit.test.ts | 89 +++++++++-- .../secret/AgentSecretAttachmentDrawer.tsx | 17 +- .../src/secret/CreateSecretDrawer.tsx | 2 +- .../src/secret/SecretForm/SecretForm.tsx | 12 +- 8 files changed, 247 insertions(+), 68 deletions(-) diff --git a/docs/design/agent-custom-secrets/plan.md b/docs/design/agent-custom-secrets/plan.md index b98097f0d9e..b3415d58c70 100644 --- a/docs/design/agent-custom-secrets/plan.md +++ b/docs/design/agent-custom-secrets/plan.md @@ -131,3 +131,44 @@ 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 only supplies the variant id and the dirty +check. If the head moves between the read and the commit, the atom re-reads once and +retries. A second conflict raises a plain message that names the fix. 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/status.md b/docs/design/agent-custom-secrets/status.md index dc9fae0fc9f..b41aa8ddc29 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 in progress on `fix/custom-secret-attach-drawer`. 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/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..b985c2eae34 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,50 @@ import { invalidateWorkflowRevisionsByVariantCache, } from "./store" +/** The message the drawer shows when the head moved twice in a row. Names the fix. */ +export const AGENT_CREDENTIALS_CONFLICT_MESSAGE = + "This agent changed while the secret was being attached. Reload the configuration and attach it 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 || detail.code === "revision_conflict" +} + +/** 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 only names the variant and gates on unsaved edits. export const commitAgentCredentialsAtom = atom( null, async ( @@ -36,59 +79,63 @@ 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 commitOn = async (base: Pick) => { + const data = withCredentials(base, bindings) + try { + const response = await getWorkflowsClient().commitWorkflowRevision( + { + workflow_revision: { + 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} + } catch (error) { + if (isRevisionConflict(error)) return {revision: null, conflict: true as const} + // 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} + } } + + // 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) + invalidateWorkflowRevisionsByVariantCache(variantId) const concurrentDraft = get(workflowDraftAtomFamily(revisionId)) if (concurrentDraft) { // Preserve edits typed while the binding request was in flight on the adopted revision. 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..c65d129e040 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"}) @@ -75,6 +95,55 @@ describe("secret attachment transaction", () => { 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("does not commit or consume unrelated unsaved edits", async () => { store.set(workflowIsDirtyAtomFamily("rev-1") as ReturnType>, true) await expect( @@ -84,7 +153,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 +170,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 +195,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-entity-ui/src/secret/AgentSecretAttachmentDrawer.tsx b/web/packages/agenta-entity-ui/src/secret/AgentSecretAttachmentDrawer.tsx index 9c32592aa3b..c10aa6d8e65 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], @@ -139,6 +143,9 @@ export function AgentSecretAttachmentDrawer({ 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() const original = editingBinding?.value const initialSlug = original?.secret.slug ?? "" const initialSecret = textSecrets.find((secret) => secret.slug === initialSlug) @@ -303,7 +310,7 @@ export function AgentSecretAttachmentDrawer({ } /> - + {allSecrets.map((secret) => ( {secret.name} @@ -346,7 +353,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} From 4e98fc3b10b8af11afca75e893ae160ddcb80835 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Thu, 10 Sep 2026 12:28:27 +0200 Subject: [PATCH 2/6] fix(frontend): refuse the attach when the head's attachments changed, and keep drafts off a newer head Review of the head-anchored commit found two data-loss paths. The callers send the full attachment list from the revision the panel displays, so a commit on a head whose attachments differ would silently drop the other change. The atom now compares the head's attachments with the displayed ones before every attempt and asks for a reload when they differ, which keeps the review rule from the design plan. Edits typed during the request are carried to the adopted revision only when it was built on the displayed revision, because copying them onto a newer head would revert the head's other fields locally. The 409 check now requires the revision_conflict code, so a proxy 409 stays a plain error. The vault refetch moved to its own effect keyed on open, because the refetch callback changes identity on every fetch. Claude-Session: https://claude.ai/code/session_01AY886ajXX65KAa1Vc9JCXU --- docs/design/agent-custom-secrets/plan.md | 12 ++-- docs/design/agent-custom-secrets/qa.md | 6 +- .../src/workflow/state/agentCredentials.ts | 41 +++++++++--- .../unit/agent-credentials-commit.test.ts | 64 +++++++++++++++++++ .../secret/AgentSecretAttachmentDrawer.tsx | 6 ++ 5 files changed, 113 insertions(+), 16 deletions(-) diff --git a/docs/design/agent-custom-secrets/plan.md b/docs/design/agent-custom-secrets/plan.md index b3415d58c70..72278a6eea2 100644 --- a/docs/design/agent-custom-secrets/plan.md +++ b/docs/design/agent-custom-secrets/plan.md @@ -163,10 +163,14 @@ displays. The server rejects any base that is not the head. When the panel shows 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 only supplies the variant id and the dirty -check. If the head moves between the read and the commit, the atom re-reads once and -retries. A second conflict raises a plain message that names the fix. The lost-response -recovery stays. +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 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/web/packages/agenta-entities/src/workflow/state/agentCredentials.ts b/web/packages/agenta-entities/src/workflow/state/agentCredentials.ts index b985c2eae34..abb0dab1bb9 100644 --- a/web/packages/agenta-entities/src/workflow/state/agentCredentials.ts +++ b/web/packages/agenta-entities/src/workflow/state/agentCredentials.ts @@ -20,15 +20,22 @@ import { invalidateWorkflowRevisionsByVariantCache, } from "./store" -/** The message the drawer shows when the head moved twice in a row. Names the fix. */ +/** The message the drawer shows when the attachments changed under it. Names the fix. */ export const AGENT_CREDENTIALS_CONFLICT_MESSAGE = - "This agent changed while the secret was being attached. Reload the configuration and attach it again." + "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 || detail.code === "revision_conflict" + 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. */ @@ -63,7 +70,9 @@ const withCredentials = ( // 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 only names the variant and gates on unsaved edits. +// 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 ( @@ -85,13 +94,19 @@ export const commitAgentCredentialsAtom = atom( () => null, ) - const commitOn = async (base: Pick) => { + 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: entity.workflow_id, + workflow_id: base.workflow_id ?? entity.workflow_id, workflow_variant_id: variantId, base_revision_id: base.id, data, @@ -108,14 +123,16 @@ export const commitAgentCredentialsAtom = atom( if (!revision) { throw new Error("The server did not return the saved agent revision.") } - return {revision, conflict: false as const} + return {revision, conflict: false as const, base: base.id} } catch (error) { - if (isRevisionConflict(error)) return {revision: null, conflict: true as const} + 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} + return {revision: latest, conflict: false as const, base: base.id} } } @@ -136,7 +153,11 @@ export const commitAgentCredentialsAtom = atom( primeWorkflowRevisionDetailCacheImperative(revision) primeCommittedRevisionRefLists(revision) invalidateWorkflowRevisionsByVariantCache(variantId) - const concurrentDraft = get(workflowDraftAtomFamily(revisionId)) + // 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 c65d129e040..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 @@ -91,6 +91,10 @@ 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", }) @@ -144,6 +148,66 @@ describe("secret attachment transaction", () => { 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( diff --git a/web/packages/agenta-entity-ui/src/secret/AgentSecretAttachmentDrawer.tsx b/web/packages/agenta-entity-ui/src/secret/AgentSecretAttachmentDrawer.tsx index c10aa6d8e65..d8576f029a7 100644 --- a/web/packages/agenta-entity-ui/src/secret/AgentSecretAttachmentDrawer.tsx +++ b/web/packages/agenta-entity-ui/src/secret/AgentSecretAttachmentDrawer.tsx @@ -145,7 +145,13 @@ export function AgentSecretAttachmentDrawer({ 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 const initialSlug = original?.secret.slug ?? "" const initialSecret = textSecrets.find((secret) => secret.slug === initialSlug) From 40f2263e112c7f805e6b36e4f65c062f0376cf87 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Thu, 10 Sep 2026 12:38:40 +0200 Subject: [PATCH 3/6] docs(agents): record the live verification of the attach fixes Claude-Session: https://claude.ai/code/session_01AY886ajXX65KAa1Vc9JCXU --- docs/design/agent-custom-secrets/status.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/agent-custom-secrets/status.md b/docs/design/agent-custom-secrets/status.md index b41aa8ddc29..289423451c9 100644 --- a/docs/design/agent-custom-secrets/status.md +++ b/docs/design/agent-custom-secrets/status.md @@ -2,7 +2,7 @@ ## Current phase -2026-09-10: follow-up fixes for #6733 (picker renders behind the drawer) and #6734 (attach anchored on a stale revision) are in progress on `fix/custom-secret-attach-drawer`. See the plan's follow-up section. +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. From c7f38de8e1ff991f0bf581fba4c5c5443f83c730 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Thu, 10 Sep 2026 12:45:00 +0200 Subject: [PATCH 4/6] fix(frontend): never hold the secret pickers on Loading because of the legacy key migration The vault hook reported loading while the one-time localStorage migration had not finished. A legacy payload that an older build wrote once instead of twice made the migration throw, it rolled back to not-migrated, and every secret picker and the Advanced section showed Loading secrets forever. The migration now accepts both encodings, backs up and clears a payload it cannot read, and always ends migrated. The loading flag no longer includes the migration state. Claude-Session: https://claude.ai/code/session_01AY886ajXX65KAa1Vc9JCXU --- .../agenta-entities/src/secret/state/atoms.ts | 44 +++++++++++++------ .../src/secret/state/useVaultSecret.ts | 7 ++- .../tests/unit/vault-migration.test.ts | 43 ++++++++++++++++++ 3 files changed, 79 insertions(+), 15 deletions(-) create mode 100644 web/packages/agenta-entities/tests/unit/vault-migration.test.ts diff --git a/web/packages/agenta-entities/src/secret/state/atoms.ts b/web/packages/agenta-entities/src/secret/state/atoms.ts index ccfc14b5363..8e1e3b8a8bb 100644 --- a/web/packages/agenta-entities/src/secret/state/atoms.ts +++ b/web/packages/agenta-entities/src/secret/state/atoms.ts @@ -349,9 +349,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 +366,42 @@ 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) + if (providers) { + for (const provider of providers) { + if (provider.key) { + await set(createStandardSecretAtom, provider as LlmProvider) + } } + } else { + console.error( + "[vault] Legacy provider keys could not be parsed; leaving them backed up.", + ) } // Create backup and cleanup localStorage.setItem(`${llmAvailableProvidersToken}Backup`, localStorageProviders) 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}) } }) + +/** + * The legacy localStorage payload was double-encoded JSON (a JSON string holding JSON), but + * older builds wrote it once. Accept both; return null for anything that is not a list. + */ +function parseLegacyProviders(raw: string): LlmProvider[] | null { + try { + let parsed: unknown = JSON.parse(raw) + if (typeof parsed === "string") parsed = JSON.parse(parsed) + return Array.isArray(parsed) ? (parsed as LlmProvider[]) : null + } catch { + return null + } +} 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/tests/unit/vault-migration.test.ts b/web/packages/agenta-entities/tests/unit/vault-migration.test.ts new file mode 100644 index 00000000000..a311f918558 --- /dev/null +++ b/web/packages/agenta-entities/tests/unit/vault-migration.test.ts @@ -0,0 +1,43 @@ +// @vitest-environment jsdom +import {createStore} from "jotai" +import {beforeEach, describe, expect, it, vi} from "vitest" +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() +}) + +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("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() + }) +}) From aa4448bfaa758b163ebb446fddb78c51dc51ac8e Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Thu, 10 Sep 2026 13:33:31 +0200 Subject: [PATCH 5/6] fix(frontend): validate legacy vault entries and keep migrating past a bad one CodeRabbit: a null entry in the legacy list threw inside the loop, which marked the migration done without the backup and cleanup. The list now goes through a zod schema via safeParseWithLogging, null entries drop out, a keyless entry is skipped, and one entry that fails to save no longer stops the others. Backup and cleanup always run. Claude-Session: https://claude.ai/code/session_01AY886ajXX65KAa1Vc9JCXU --- .../agenta-entities/src/secret/state/atoms.ts | 24 +++++++++++--- .../tests/unit/vault-migration.test.ts | 32 +++++++++++++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/web/packages/agenta-entities/src/secret/state/atoms.ts b/web/packages/agenta-entities/src/secret/state/atoms.ts index 8e1e3b8a8bb..68816a90ffa 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, @@ -369,8 +371,12 @@ export const migrateVaultKeysAtom = atom(null, async (get, set) => { const providers = parseLegacyProviders(localStorageProviders) if (providers) { for (const provider of providers) { - if (provider.key) { + if (!provider.key) continue + try { await set(createStandardSecretAtom, provider as LlmProvider) + } catch (error) { + // One bad entry must not stop the others; the backup below keeps it. + console.error("[vault] Legacy provider key was not migrated:", error) } } } else { @@ -392,16 +398,26 @@ export const migrateVaultKeysAtom = atom(null, async (get, set) => { } }) +/** One legacy entry: whatever else it carried, only a named provider with a key can migrate. */ +const legacyProviderSchema = z.object({key: z.string().optional()}).passthrough() +const legacyProvidersSchema = z.array(legacyProviderSchema.nullable()) + /** * The legacy localStorage payload was double-encoded JSON (a JSON string holding JSON), but - * older builds wrote it once. Accept both; return null for anything that is not a list. + * older builds wrote it once. Accept both; return null for anything that is not a list of + * objects. Null entries are dropped rather than failing the whole list. */ function parseLegacyProviders(raw: string): LlmProvider[] | null { + let parsed: unknown try { - let parsed: unknown = JSON.parse(raw) + parsed = JSON.parse(raw) if (typeof parsed === "string") parsed = JSON.parse(parsed) - return Array.isArray(parsed) ? (parsed as LlmProvider[]) : null } catch { return null } + const entries = safeParseWithLogging(legacyProvidersSchema, parsed, "[vault] legacy providers") + if (!entries) return null + return entries.filter( + (entry): entry is NonNullable => entry != null, + ) as LlmProvider[] } diff --git a/web/packages/agenta-entities/tests/unit/vault-migration.test.ts b/web/packages/agenta-entities/tests/unit/vault-migration.test.ts index a311f918558..bbc57c09209 100644 --- a/web/packages/agenta-entities/tests/unit/vault-migration.test.ts +++ b/web/packages/agenta-entities/tests/unit/vault-migration.test.ts @@ -1,6 +1,7 @@ // @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()})) @@ -18,6 +19,7 @@ beforeEach(() => { vi.resetAllMocks() localStorage.clear() store = createStore() + store.set(projectIdAtom, "project-1") }) describe("legacy vault key migration", () => { @@ -34,6 +36,36 @@ describe("legacy vault key migration", () => { 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}) + expect(localStorage.getItem(llmAvailableProvidersToken)).toBeNull() + }) + 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) From 0e246df3db7724cc2ca6fec686fe9696d0784946 Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Thu, 10 Sep 2026 13:41:49 +0200 Subject: [PATCH 6/6] fix(frontend): keep unsaved legacy vault keys for a retry and validate entries one by one Codex gate review: a legacy key whose save failed was cleared from storage with the migration marked done, so a temporary API error lost it for good. The entries that fail to save now stay in the active storage key in the canonical form, and the next page load retries them. The list is also validated entry by entry, so one malformed entry no longer rejects the valid ones beside it. Claude-Session: https://claude.ai/code/session_01AY886ajXX65KAa1Vc9JCXU --- .../agenta-entities/src/secret/state/atoms.ts | 39 ++++++++++++------- .../tests/unit/vault-migration.test.ts | 7 +++- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/web/packages/agenta-entities/src/secret/state/atoms.ts b/web/packages/agenta-entities/src/secret/state/atoms.ts index 68816a90ffa..1023bab1511 100644 --- a/web/packages/agenta-entities/src/secret/state/atoms.ts +++ b/web/packages/agenta-entities/src/secret/state/atoms.ts @@ -369,14 +369,16 @@ export const migrateVaultKeysAtom = atom(null, async (get, set) => { if (localStorageProviders) { const providers = parseLegacyProviders(localStorageProviders) + const failed: LlmProvider[] = [] if (providers) { for (const provider of providers) { if (!provider.key) continue try { - await set(createStandardSecretAtom, provider as LlmProvider) + await set(createStandardSecretAtom, provider) } catch (error) { - // One bad entry must not stop the others; the backup below keeps it. + // 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 { @@ -385,9 +387,17 @@ export const migrateVaultKeysAtom = atom(null, async (get, set) => { ) } - // 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) + } } } catch (error) { // A failed migration must not hold the vault UI in its loading state: the keys stay @@ -398,14 +408,13 @@ export const migrateVaultKeysAtom = atom(null, async (get, set) => { } }) -/** One legacy entry: whatever else it carried, only a named provider with a key can migrate. */ -const legacyProviderSchema = z.object({key: z.string().optional()}).passthrough() -const legacyProvidersSchema = z.array(legacyProviderSchema.nullable()) +/** 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 for anything that is not a list of - * objects. Null entries are dropped rather than failing the whole list. + * 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 @@ -415,9 +424,11 @@ function parseLegacyProviders(raw: string): LlmProvider[] | null { } catch { return null } - const entries = safeParseWithLogging(legacyProvidersSchema, parsed, "[vault] legacy providers") - if (!entries) return null - return entries.filter( - (entry): entry is NonNullable => entry != null, - ) as LlmProvider[] + 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/tests/unit/vault-migration.test.ts b/web/packages/agenta-entities/tests/unit/vault-migration.test.ts index bbc57c09209..6aa58998a3a 100644 --- a/web/packages/agenta-entities/tests/unit/vault-migration.test.ts +++ b/web/packages/agenta-entities/tests/unit/vault-migration.test.ts @@ -63,7 +63,12 @@ describe("legacy vault key migration", () => { await store.set(migrateVaultKeysAtom) expect(api.create).toHaveBeenCalledTimes(2) expect(store.get(vaultMigrationAtom)).toEqual({migrating: false, migrated: true}) - expect(localStorage.getItem(llmAvailableProvidersToken)).toBeNull() + // 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 () => {