Fix secret edits to preserve values (#1889)

Co-authored-by: openhands <openhands@all-hands.dev>

__tests__/api/secrets-service.test.ts

@@ -0,0 +1,100 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+
+import type { Backend } from "#/api/backend-registry/types";
+import {
+  __resetActiveStoreForTests,
+  setActiveSelection,
+  setRegisteredBackends,
+} from "#/api/backend-registry/active-store";
+import { SecretsService } from "#/api/secrets-service";
+
+const {
+  mockGetSecret,
+  mockUpsertSecret,
+  mockDeleteSecret,
+} = vi.hoisted(() => ({
+  mockGetSecret: vi.fn(),
+  mockUpsertSecret: vi.fn(),
+  mockDeleteSecret: vi.fn(),
+}));
+
+vi.mock("@openhands/typescript-client/clients", () => ({
+  SettingsClient: vi.fn(function SettingsClientMock() {
+    return {
+      getSecret: mockGetSecret,
+      upsertSecret: mockUpsertSecret,
+      deleteSecret: mockDeleteSecret,
+    };
+  }),
+}));
+
+const localBackend: Backend = {
+  id: "local",
+  name: "Local",
+  host: "http://127.0.0.1:8000",
+  apiKey: "",
+  kind: "local",
+};
+
+describe("SecretsService", () => {
+  beforeEach(() => {
+    window.localStorage.clear();
+    __resetActiveStoreForTests();
+    setRegisteredBackends([localBackend]);
+    setActiveSelection({ backendId: localBackend.id });
+    mockGetSecret.mockReset();
+    mockUpsertSecret.mockReset();
+    mockDeleteSecret.mockReset();
+  });
+
+  afterEach(() => {
+    __resetActiveStoreForTests();
+  });
+
+  it("preserves the existing value when updating description", async () => {
+    // Arrange
+    mockGetSecret.mockResolvedValue("keep-this");
+    mockUpsertSecret.mockResolvedValue({
+      name: "OpenAI_API_Key",
+      description: "Updated description",
+    });
+
+    // Act
+    await SecretsService.updateSecret(
+      "OpenAI_API_Key",
+      "OpenAI_API_Key",
+      "Updated description",
+    );
+
+    // Assert
+    expect(mockGetSecret).toHaveBeenCalledWith("OpenAI_API_Key");
+    expect(mockUpsertSecret).toHaveBeenCalledWith({
+      name: "OpenAI_API_Key",
+      value: "keep-this",
+      description: "Updated description",
+    });
+    expect(mockDeleteSecret).not.toHaveBeenCalled();
+  });
+
+  it("renames secrets by re-upserting and removing the old entry", async () => {
+    // Arrange
+    mockGetSecret.mockResolvedValue("original-value");
+    mockUpsertSecret.mockResolvedValue({
+      name: "New_Key",
+      description: "Renamed",
+    });
+    mockDeleteSecret.mockResolvedValue({ deleted: true });
+
+    // Act
+    await SecretsService.updateSecret("Old_Key", "New_Key", "Renamed");
+
+    // Assert
+    expect(mockGetSecret).toHaveBeenCalledWith("Old_Key");
+    expect(mockUpsertSecret).toHaveBeenCalledWith({
+      name: "New_Key",
+      value: "original-value",
+      description: "Renamed",
+    });
+    expect(mockDeleteSecret).toHaveBeenCalledWith("Old_Key");
+  });
+});

src/api/secrets-service.ts

@@ -91,29 +91,39 @@ export class SecretsService {
   }
 
   /**
-   * Update a secret's value and/or description.
-   * Uses the same upsert endpoint as createSecret since agent-server
-   * doesn't have a separate update endpoint.
+   * Update a secret's name and/or description while preserving its value.
+   * The agent-server only exposes an upsert endpoint, so we fetch the
+   * existing value and re-upsert it under the updated name/description.
    *
-   * @param name - Secret name (used as identifier)
-   * @param value - New secret value
+   * @param secretToEdit - Existing secret name
+   * @param name - New (or same) secret name
    * @param description - Optional new description
    * @throws Error if the API call fails after retries
    */
   static async updateSecret(
+    secretToEdit: string,
     name: string,
-    value: string,
     description?: string,
   ): Promise<void> {
     if (getActiveBackend().backend.kind === "cloud") {
-      // The cloud PUT endpoint renames + redescribes only (no value field),
-      // matching what `useUpdateSecret` actually sends:
-      // (secretToEdit=name, newName=value, description).
-      await withRetry(() => updateCloudSecret(name, value, description));
+      await withRetry(() => updateCloudSecret(secretToEdit, name, description));
       return;
     }
-    // Agent-server uses upsert, so update is the same as create
-    await this.createSecret(name, value, description);
+
+    const client = new SettingsClient(getAgentServerClientOptions());
+    const value = await withRetry(() => client.getSecret(secretToEdit));
+
+    await withRetry(() =>
+      client.upsertSecret({
+        name,
+        value,
+        description,
+      }),
+    );
+
+    if (name !== secretToEdit) {
+      await this.deleteSecret(secretToEdit);
+    }
   }
 
   /**