fix: construct AsyncEntry inside the try so keychain degradation engages (#1848) (#1945)

* fix: construct AsyncEntry inside the try so keychain degradation engages

`KeyringSecretStore` built its `AsyncEntry` outside the `try` in `get`,
`set`, and `delete`. `AsyncEntry::new` performs the platform-store setup
(on Linux, the Secret Service connect with a keyutils fallback) and
throws when no backend is reachable, so the documented degradation
contract never engaged for a construction-time failure: the raw keyring
error escaped, 500ing every `GET /api/servers` on a box without a Secret
Service (the published container, which has no D-Bus session).

`expectedSecretFields` always includes the OAuth slot, so `rehydrateConfig`
constructs an entry for every server — the default seeded catalog was
enough to 500 the first list load, before any server was added. And
because the escaping error wasn't a `KeychainUnavailableError`, it
bypassed both the routes' 503 translation and the `migratePlaintextSecrets`
skip branch, yielding a generic 500 instead of the actionable message.

Moving construction inside the existing `try` restores the contract for
that failure mode: `get` returns null, `delete` no-ops, and `set` is the
only operation that throws — as `KeychainUnavailableError`, which is what
the 503 translation and the migration skip both match on.

The test stub's constructor could not fail, which is why the coverage
gate never saw the gap. Adds a `constructorThrows` hook alongside the
existing method-level `failures` flags and covers all three methods plus
`deleteAllForServer` under it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F

* test: pin why the OAuth secret field is unconditional

`expectedSecretFields` always includes the OAuth slot, and #1848's report
frames that as why the 500 fired so broadly — which invites a follow-up
that skips the keychain read for servers with "no OAuth config". That
change would lose data.

`extractSecretsFromStored` deletes the `oauth` block outright when
`clientSecret` was its only property, so such a server carries NO marker
on disk that a secret exists — the keychain is the sole record, and the
unconditional slot is what finds it again. Gating the read on a
disk-visible `oauth` block would silently stop rehydrating exactly that
shape.

The existing tests do fail if the slot is made conditional (verified by
applying the change: 4 red), but only under names that explain nothing
about the consequence — `always lists the OAuth slot first` reads like a
tautology worth updating rather than a trap. Add a round-trip case that
states the consequence, so the next reader sees the field is load-bearing
rather than defensive.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

clients/web/src/test/core/mcp/serverList.test.ts

@@ -1076,6 +1076,36 @@ describe("expectedSecretFields", () => {
       }),
     ).toEqual([SECRET_FIELD_OAUTH_CLIENT_SECRET]);
   });
+
+  it("keeps a secret-only OAuth config reachable across the disk round trip", () => {
+    // The OAuth slot is unconditional, and that is load-bearing rather
+    // than merely defensive — skipping the keychain read for servers with
+    // "no OAuth config" is a tempting optimization that would silently
+    // stop rehydrating exactly this shape.
+    //
+    // `extractSecretsFromStored` deletes the `oauth` block outright when
+    // `clientSecret` was its only property, so the on-disk entry carries
+    // NO marker that a secret exists — the keychain is the only record.
+    // The unconditional slot is what finds it again.
+    const { stripped, secrets } = extractSecretsFromStored({
+      type: "streamable-http",
+      url: "https://x.test",
+      oauth: { clientSecret: "shh" },
+    });
+    expect(stripped).not.toHaveProperty("oauth");
+
+    // What a GET does: enumerate fields from the *stripped* on-disk shape,
+    // read those from the keychain, merge back.
+    const fields = expectedSecretFields(stripped);
+    expect(fields).toContain(SECRET_FIELD_OAUTH_CLIENT_SECRET);
+
+    const fromKeychain = Object.fromEntries(
+      fields.filter((f) => f in secrets).map((f) => [f, secrets[f]!]),
+    );
+    expect(mergeSecretsIntoStored(stripped, fromKeychain).oauth).toEqual({
+      clientSecret: "shh",
+    });
+  });
 });
 
 describe("enterpriseManaged oauth settings", () => {

clients/web/src/test/integration/auth/node/secret-store.test.ts

@@ -24,6 +24,12 @@ const keyringMocks = vi.hoisted(() => {
     deleteThrows: false,
     findThrows: false,
     deleteThrowsNoEntry: false,
+    // `AsyncEntry::new` itself does the platform-store setup and throws
+    // when no backend is reachable (a container with no D-Bus session,
+    // Linux without libsecret). Modelling it is what catches #1848: a
+    // stub that can only fail per-method leaves the construction path
+    // untested, so an escaping constructor error looks green here.
+    constructorThrows: false,
   };
   const credentials = (): Array<{ account: string; password: string }> => {
     const out: Array<{ account: string; password: string }> = [];
@@ -35,6 +41,11 @@ const keyringMocks = vi.hoisted(() => {
   class AsyncEntry {
     private readonly key: string;
     constructor(_service: string, username: string) {
+      if (failures.constructorThrows) {
+        throw new Error(
+          "Couldn't access platform storage: PermissionDenied (constructor)",
+        );
+      }
       this.key = username;
     }
     async getPassword(): Promise<string | undefined> {
@@ -215,6 +226,7 @@ describe("KeyringSecretStore (mocked native bindings)", () => {
     keyringMocks.failures.deleteThrows = false;
     keyringMocks.failures.findThrows = false;
     keyringMocks.failures.deleteThrowsNoEntry = false;
+    keyringMocks.failures.constructorThrows = false;
     store = new KeyringSecretStore();
   });
 
@@ -329,4 +341,48 @@ describe("KeyringSecretStore (mocked native bindings)", () => {
       expect((err as Error).message).toMatch(/libsecret/);
     }
   });
+
+  describe("keychain unreachable at AsyncEntry construction (#1848)", () => {
+    // `AsyncEntry::new` — not just its methods — throws when no platform
+    // store is reachable. The degradation contract must hold identically
+    // for that failure mode; constructing outside the `try` let the raw
+    // keyring error escape and 500 `GET /api/servers` before any secret
+    // was touched.
+    beforeEach(() => {
+      keyringMocks.failures.constructorThrows = true;
+    });
+
+    it("get returns null", async () => {
+      expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe(
+        null,
+      );
+    });
+
+    it("set throws KeychainUnavailableError, not the raw keyring error", async () => {
+      // The typed error is what the routes translate to a 503 and what
+      // `migratePlaintextSecrets` matches on to skip migration.
+      await expect(
+        store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v"),
+      ).rejects.toBeInstanceOf(KeychainUnavailableError);
+      await expect(
+        store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v"),
+      ).rejects.toThrow(/Couldn't access platform storage/);
+    });
+
+    it("delete silently no-ops", async () => {
+      await expect(
+        store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET),
+      ).resolves.toBeUndefined();
+    });
+
+    it("deleteAllForServer no-ops even when the credential sweep finds entries", async () => {
+      // findCredentialsAsync can succeed while per-entry construction
+      // fails; the sweep must still resolve rather than escape.
+      keyringMocks.failures.constructorThrows = false;
+      await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a");
+      keyringMocks.failures.constructorThrows = true;
+
+      await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined();
+    });
+  });
 });