Reject duplicate connection names on create

.changeset/connection-create-conflict.md

@@ -0,0 +1,11 @@
+---
+"@executor-js/sdk": patch
+---
+
+**Creating a connection over an existing one is rejected instead of silently overwriting it**
+
+`connections.create` used to upsert: a create with the same (owner, integration, name) replaced the saved connection and, for a pasted value, overwrote the stored secret itself. It now fails with the new `ConnectionAlreadyExistsError` and leaves the existing connection untouched. Remove the connection first, or pick a different name.
+
+This adds one error to the wire contract: the `POST /connections` endpoint can answer **HTTP 409** with tag `ConnectionAlreadyExistsError`, and the `connections.create` core tool resolves the same case as `{ ok: false, error: { code: "connection_already_exists" } }`. The core tool now also resolves the other expected input failures the same way instead of as opaque internal errors: `integration_not_found` for an unknown integration and `invalid_connection_input` for an invalid input. The change is additive — no existing status, field, or success shape moves.
+
+OAuth is unaffected. Fresh OAuth connects already resolve a taken name to the next free suffix through `newConnection`, and reconnect still re-mints the same connection on purpose.

e2e/cloud/connections-credentials.test.ts

@@ -3,9 +3,11 @@
 // identified by (owner, integration, name), with its value stored through the
 // real vault. The product promises under test: the secret goes in but NEVER
 // comes back out of any endpoint; metadata round-trips; re-creating the same
-// connection replaces it instead of duplicating; removal really removes; and
-// unknown connections fail with a typed not-found error.
+// connection is rejected as a conflict instead of silently replacing it;
+// removal really removes; and unknown connections fail with a typed
+// not-found error.
 import { randomBytes } from "node:crypto";
+import { createServer } from "node:http";
 
 import { expect } from "@effect/vitest";
 import { Effect } from "effect";
@@ -22,7 +24,8 @@ type Client = HttpApiClient.ForApi<typeof api>;
 
 const TEMPLATE_API_KEY = AuthTemplateSlug.make("apiKey");
 
-/** Minimal OpenAPI spec with a single GET /ping — never contacted here. */
+/** Minimal OpenAPI spec with a single GET /ping — only the conflict scenario
+ *  ever contacts it, to prove which stored secret the connection resolves. */
 const pingSpec = JSON.stringify({
   openapi: "3.0.3",
   info: { title: "Ping API", version: "1.0.0" },
@@ -33,15 +36,58 @@ const pingSpec = JSON.stringify({
   },
 });
 
-/** Registers a fresh apiKey-authenticated integration for connections to bind to. */
-const registerIntegration = (client: Client) =>
+type CaptureUpstream = {
+  readonly url: string;
+  /** Every Authorization header `GET /ping` has received, in order. */
+  readonly authorizationHeaders: () => readonly string[];
+  readonly close: () => void;
+};
+
+/** Upstream on 127.0.0.1 that records the Authorization header of every
+ *  `GET /ping`. This is how a scenario proves WHICH stored secret a connection
+ *  resolves, since no endpoint ever echoes the value itself. */
+const serveCaptureUpstream = () =>
+  Effect.acquireRelease(
+    Effect.callback<CaptureUpstream>((resume) => {
+      const headers: string[] = [];
+      const server = createServer((request, response) => {
+        if (request.method === "GET" && (request.url ?? "").startsWith("/ping")) {
+          headers.push(request.headers.authorization ?? "");
+          response.writeHead(200, { "content-type": "application/json" });
+          response.end(JSON.stringify({ pong: true }));
+          return;
+        }
+        response.writeHead(404, { "content-type": "application/json" });
+        response.end(JSON.stringify({ error: "not_found" }));
+      });
+      server.listen(0, "127.0.0.1", () => {
+        const address = server.address();
+        const port = typeof address === "object" && address ? address.port : 0;
+        resume(
+          Effect.succeed({
+            url: `http://127.0.0.1:${port}`,
+            authorizationHeaders: () => [...headers],
+            close: () => {
+              server.close();
+              server.closeAllConnections();
+            },
+          }),
+        );
+      });
+    }),
+    (upstream) => Effect.sync(upstream.close),
+  );
+
+/** Registers a fresh apiKey-authenticated integration for connections to bind
+ *  to. Identifier-safe slug: it becomes a property path in sandbox code. */
+const registerIntegration = (client: Client, baseUrl = "http://127.0.0.1:59999") =>
   Effect.gen(function* () {
-    const slug = IntegrationSlug.make(`conn-scn-${randomBytes(4).toString("hex")}`);
+    const slug = IntegrationSlug.make(`connscn${randomBytes(4).toString("hex")}`);
     yield* client.openapi.addSpec({
       payload: {
         spec: { kind: "blob", value: pingSpec },
         slug,
-        baseUrl: "http://127.0.0.1:59999", // never contacted during registration
+        baseUrl, // the default is never contacted during registration
         authenticationTemplate: [
           {
             slug: "apiKey",
@@ -102,48 +148,82 @@ scenario(
 );
 
 scenario(
-  "Connections · re-creating the same connection replaces it instead of duplicating",
+  "Connections · re-creating the same connection is rejected and leaves the original intact",
   {},
-  Effect.gen(function* () {
-    const target = yield* Target;
-    const { client: apiClient } = yield* Api;
-    const identity = yield* target.newIdentity();
-    const client = yield* apiClient(api, identity);
-    const integration = yield* registerIntegration(client);
-    const name = freshConnectionName();
+  Effect.scoped(
+    Effect.gen(function* () {
+      const target = yield* Target;
+      const { client: apiClient } = yield* Api;
+      const identity = yield* target.newIdentity();
+      const client = yield* apiClient(api, identity);
+      const upstream = yield* serveCaptureUpstream();
+      const integration = yield* registerIntegration(client, upstream.url);
+      const name = freshConnectionName();
 
-    yield* client.connections.create({
-      payload: {
-        owner: "org",
-        name,
-        integration,
-        template: TEMPLATE_API_KEY,
-        identityLabel: "first key",
-        value: "first-value",
-      },
-    });
-    const first = yield* client.connections.list({ query: { integration } });
-    expect(
-      first.filter((connection) => connection.name === name).map((c) => c.identityLabel),
-      "the first create stores one row with its label",
-    ).toEqual(["first key"]);
+      yield* client.connections.create({
+        payload: {
+          owner: "org",
+          name,
+          integration,
+          template: TEMPLATE_API_KEY,
+          identityLabel: "first key",
+          value: "first-value",
+        },
+      });
+      const first = yield* client.connections.list({ query: { integration } });
+      expect(
+        first.filter((connection) => connection.name === name).map((c) => c.identityLabel),
+        "the first create stores one row with its label",
+      ).toEqual(["first key"]);
 
-    yield* client.connections.create({
-      payload: {
-        owner: "org",
-        name,
-        integration,
-        template: TEMPLATE_API_KEY,
-        identityLabel: "rotated key",
-        value: "second-value",
-      },
-    });
-    const second = yield* client.connections.list({ query: { integration } });
-    expect(
-      second.filter((connection) => connection.name === name).map((c) => c.identityLabel),
-      "re-creating the same (owner, integration, name) updates the row in place",
-    ).toEqual(["rotated key"]);
-  }),
+      const error = yield* client.connections
+        .create({
+          payload: {
+            owner: "org",
+            name,
+            integration,
+            template: TEMPLATE_API_KEY,
+            identityLabel: "clobber attempt",
+            value: "second-value",
+          },
+        })
+        .pipe(Effect.flip);
+      expect(
+        (error as { _tag?: string })._tag,
+        "re-creating the same (owner, integration, name) fails with the typed conflict",
+      ).toBe("ConnectionAlreadyExistsError");
+
+      const second = yield* client.connections.list({ query: { integration } });
+      expect(
+        second.filter((connection) => connection.name === name).map((c) => c.identityLabel),
+        "the rejected create left the original row untouched",
+      ).toEqual(["first key"]);
+
+      // The stored SECRET is intact too, not just the metadata: invoking
+      // through the original connection must still authenticate upstream with
+      // the first value. The pasted value's provider item id is derived from
+      // the connection name, so a rejected create that wrote before losing
+      // would have replaced the credential while every metadata read above
+      // still looked untouched.
+      const tools = yield* client.tools.list({ query: { integration } });
+      const address = tools
+        .map((tool) => String(tool.address))
+        .find((toolAddress) => toolAddress.includes(".ping."));
+      expect(address, "the ping tool is in the catalog").toBeDefined();
+      if (address === undefined) return;
+
+      const execution = yield* client.executions.execute({
+        payload: {
+          code: [`const result = await ${address}({});`, "return result;"].join("\n"),
+        },
+      });
+      expect(execution.status, "the invoke completes").toBe("completed");
+      expect(
+        upstream.authorizationHeaders(),
+        "the original connection still authenticates with the first value",
+      ).toEqual(["Bearer first-value"]);
+    }),
+  ),
 );
 
 scenario(

e2e/scenarios/health-checks.test.ts

@@ -141,14 +141,20 @@ const discriminatedUnionSpec = (baseUrl: string): string => {
 };
 
 /** A real node:http identity API on 127.0.0.1. `GET /me` returns the account
- *  JSON only when the bearer token matches `validToken`; any other token is a
- *  401 (the "the dev token got revoked" case the health check classifies as
- *  expired). Closed by the scope's finalizer. */
+ *  JSON only when the bearer token matches the CURRENT `validToken`; any other
+ *  token is a 401 (the "the dev token got revoked" case the health check
+ *  classifies as expired). `revoke()` rotates the server-side token so a saved
+ *  key stops working mid-scenario. Closed by the scope's finalizer. */
 const serveIdentityApi = (validToken: string) =>
   Effect.acquireRelease(
-    Effect.callback<{ readonly url: string; readonly close: () => void }>((resume) => {
+    Effect.callback<{
+      readonly url: string;
+      readonly revoke: () => void;
+      readonly close: () => void;
+    }>((resume) => {
+      let currentToken = validToken;
       const server = createServer((request, response) => {
-        const authorized = request.headers["authorization"] === `Bearer ${validToken}`;
+        const authorized = request.headers["authorization"] === `Bearer ${currentToken}`;
         if (request.method === "GET" && (request.url ?? "").startsWith("/me")) {
           if (!authorized) {
             response.writeHead(401, { "content-type": "application/json" });
@@ -168,6 +174,9 @@ const serveIdentityApi = (validToken: string) =>
         resume(
           Effect.succeed({
             url: `http://127.0.0.1:${port}`,
+            revoke: () => {
+              currentToken = `revoked_${validToken}`;
+            },
             close: () => {
               server.close();
               server.closeAllConnections();
@@ -323,17 +332,9 @@ scenario(
           expect(healthy.httpStatus, "the saved probe saw the 200").toBe(200);
           expect(healthy.identity, "the saved probe derives the account identity").toBe(IDENTITY);
 
-          // Re-creating the same (owner, integration, name) replaces the stored
-          // key in place: now the connection holds a key the server rejects.
-          yield* client.connections.create({
-            payload: {
-              owner: "org",
-              name,
-              integration: slug,
-              template: TEMPLATE,
-              value: "rotated-away",
-            },
-          });
+          // The server revokes the key: the connection's saved value now gets
+          // a 401 (the "dev token got revoked" case).
+          server.revoke();
           const expired = yield* client.connections.checkHealth({
             params: { owner: "org", integration: slug, name },
             query: {},
@@ -642,22 +643,14 @@ scenario(
             },
           });
 
-          // Seed a verdict, then re-create the connection with a DEAD key: the
-          // persisted verdict (healthy) and reality (expired) now disagree.
+          // Seed a verdict, then revoke the key server-side: the persisted
+          // verdict (healthy) and reality (expired) now disagree.
           const seeded = yield* client.connections.checkHealth({
             params: { owner: "org", integration: slug, name },
             query: {},
           });
           expect(seeded.status, "the seed probe is healthy").toBe("healthy");
-          yield* client.connections.create({
-            payload: {
-              owner: "org",
-              name,
-              integration: slug,
-              template: TEMPLATE,
-              value: "rotated-away",
-            },
-          });
+          server.revoke();
 
           // Within the freshness window the CACHED verdict comes back with no
           // probe, so the dead key still reads healthy (stale by design).

e2e/scenarios/no-auth-connection.test.ts

@@ -93,6 +93,19 @@ const created = await tools.executor.coreTools.connections.create({
 return created.ok ? { ok: true, connection: created.data } : { ok: false, error: created.error };
 `;
 
+// Create is never a replace: re-creating the SAME (owner, integration, name)
+// must fail with ConnectionAlreadyExistsError instead of silently upserting
+// over the existing connection.
+const createDuplicateConnectionCode = (slug: string) => `
+const created = await tools.executor.coreTools.connections.create({
+  owner: "org",
+  name: "public",
+  integration: ${JSON.stringify(slug)},
+  template: "none",
+});
+return created.ok ? { ok: true, connection: created.data } : { ok: false, error: created.error };
+`;
+
 // The relaxed filter must still reject an origin on a no-auth create — an
 // empty `inputs: {}` is a (degenerate) origin and a credential the connection
 // can't hold, so it stays a validation failure.
@@ -189,6 +202,19 @@ scenario(
         rejected.ok,
         `a no-auth create with an empty inputs origin is rejected: ${JSON.stringify(rejected)}`,
       ).toBe(false);
+
+      // 5. Create is never a replace: re-creating the same (owner,
+      //    integration, name) fails with a conflict instead of silently
+      //    editing over the existing connection.
+      const duplicate = yield* executeJson(session, createDuplicateConnectionCode(integration));
+      expect(
+        duplicate.ok,
+        `re-creating an existing connection name is rejected: ${JSON.stringify(duplicate)}`,
+      ).toBe(false);
+      expect(
+        JSON.stringify(duplicate.error),
+        "the failure names the conflict so callers can act on it",
+      ).toContain("already exists");
     }).pipe(
       // Selfhost shares one workspace identity — leaked connections fail other
       // scenarios' zero-state assertions, so drop everything this run made.