feat(connections): validate API keys before storing

[mikemikimike]

Closes #80\n\n## Background\n\nPUT /v1/connections/{prefix}/key previously stored any non-blank key, so invalid provider credentials were only reported on the first tool call.\n\n## Changes\n\n- Add an optional validate request to ProviderManifest.\n- Add reusable provider-level key validation before saving or enabling a connection.\n- Configure Discord validation with GET /users/@me.\n- Support both header and query API key validation.\n- Distinguish invalid credentials (401/403) from upstream failures (5xx/network errors).\n- Preserve existing behavior for manifests without validate.\n- Add regression tests for successful validation, rejected keys, query auth, upstream failures, and atomic refusal.\n\n## Compatibility\n\nThe new manifest field is optional. Existing providers and callers without it keep the previous behavior.\n\n## Verification\n\n- npm test -- test/providers/manifest.test.ts — 45 passed.\n- TDD red runs were observed before both implementation steps.\n- npm test -- test/connections.test.ts — blocked by unavailable Docker PostgreSQL: ECONNREFUSED ::1:5432 / 127.0.0.1:5432.\n- npm run typecheck and npm run build — blocked by incomplete local @electric-sql/pglite installation; its declared dist/index.d.ts was absent.\n- docker compose up -d db — PostgreSQL image pull did not complete in the available environment.

[fajarhide]

Thanks for this, and the direction is right: #80 asks for exactly this and validating before the vault write is the correct place to do it. Three things need changing before it can go in.

I approved the workflow run so CI could report. It is red, and not on your new tests:

FAIL test/admin-connections.test.ts:210 > stores the key encrypted and turns the provider on
FAIL test/connections-http.test.ts:124  > stores the key encrypted and turns the provider on
AssertionError: expected 401 to be 200
Tests  2 failed | 433 passed (435)

deps.fetch is declared but never wired at any call site, so connections.ts:141 falls through to the global fetch and the suite now makes a real request to discord.com. CI has network, Discord answers 401 for the fake key, and the PUT fails. These are the two files you could not run locally, so this is the part your environment was hiding rather than anything you did wrong. Injecting the fetch double through ConnectionDeps in those two fixtures should settle it, and it also keeps the suite offline, which is worth having on its own.

validateKey calls ctx.fetch directly, and it is the only outbound call in manifest.ts that does. Everything else goes through followRedirects (manifest.ts:195), which exists so a redirect to another origin cannot carry our credential to it. As written, a 302 from /users/@me sends authorization: Bot <token> to whatever host the upstream names. That was fixed once in #57 and this reopens it. followRedirects(ctx.fetch, fail, manifest.auth, url, init) is a drop-in and brings the hop cap with it.

The error code needs to change too. invalid_credential maps to HTTP 401 (errors.ts:16) and everywhere else in the codebase it means the caller's own gateway bearer is bad (auth.ts:22, service.ts:17, metering.ts:62). Meanwhile the executor maps an upstream 401 to reauth_required, 409 (manifest.ts:617). So a wrong Discord token comes back indistinguishable from an expired Selat token, and a client with the usual refresh-on-401 interceptor will re-auth against us and retry forever. reauth_required matches the executor. Same line hardcodes 403 and ignores manifest.errors.forbidden (manifest.ts:620), which exists because a bare 403 does not mean bad credential for every vendor.

Three smaller ones, none blocking on their own:

• No timeout budget. Every tool call is wrapped in withTimeout (call-tool.ts:160); this path is bare, so a vendor that accepts the connection and never answers holds the handler for undici's default.
• The response body is never read, so under undici the socket stays pinned until GC.
• A 429 during validation surfaces as upstream_error with no retry_after, skipping the rate-limit handling the executor already has.

What is right: validate before grants.save and enablement.enable, so the atomic refusal is genuine and the test for it proves something. The conditional spread keeps validateKey properly optional, and a manifest without validate is untouched. The query-auth path handles a validate request that already carries a query string.

[fajarhide]

Suggestions for the two blocking findings. I applied both on top of 24e6c84 locally: npx tsc --noEmit clean, and test/providers/manifest.test.ts 45 passed once the one invalid_credential expectation was updated.

[fajarhide]

deps.fetch is never wired at any call site, so this falls through to the global fetch and the suite reaches discord.com for real. That is what turns admin-connections.test.ts:210 and connections-http.test.ts:124 red. Both fixtures build their deps without a fetch, so passing the existing double through ConnectionDeps in those two files fixes it and keeps the suite offline. No suggestion here because those files are not in this diff.

Separately, this call has no timeout budget. Every tool call goes through withTimeout (call-tool.ts:160, module-private), so a vendor that accepts the connection and never answers holds the handler for undici's default. Either export that helper or hand the validate request an AbortSignal.timeout(...).

[fajarhide]

Both suggestions landed cleanly, thanks. CI is still red, three failures now:

FAIL test/providers/manifest.test.ts  > rejects an api key when its validation request fails
FAIL test/admin-connections.test.ts   > stores the key encrypted, turns the provider on and never echoes it back
FAIL test/connections-http.test.ts    > stores the key encrypted and turns the provider on

The first is the expectation this review comments on. The other two are the network problem, and the fix is smaller than my earlier comment implied: connectionOverrides already exists on ServerDeps (server.ts:35, spread at server.ts:76), so the test server can default a fetch and no production code moves. Doing it in the helper rather than in the two fixtures also covers any future provider that declares validate, since the cause is the missing offline default rather than those two tests.

test/helpers/server.ts is not in this diff so I cannot suggest it inline. Above startTestServer:

/** Answers a key validation probe 200 so nothing leaves the machine. A test
 *  that wants the refusal path passes its own fetch through
 *  connectionOverrides. */
const offlineFetch = (async () =>
  new Response('{}', {
    status: 200,
    headers: { 'content-type': 'application/json' },
  })) as typeof fetch

Then in the createServer call, after the ...opts.overrides spread:

    connectionOverrides: {
      // A provider that declares `validate` sends the fixture's fake api key to
      // the real vendor otherwise, which makes the suite need the network and
      // fail on whatever that vendor answers.
      fetch: offlineFetch,
      ...opts.overrides?.connectionOverrides,
    },

The spread order matters: the default goes first so a test can still pass its own fetch.

I ran this on top of 7bdf6ab locally. npx tsc --noEmit clean, and the full suite 435 passed across 42 files. That is with a local Postgres, which is the part your environment could not give you, so it should be the last round.

[fajarhide]

Follows from the error code change you took above.

Suggested change

	code: 'invalid_credential',
	code: 'invalid_arguments',

[fajarhide]

Green, and both suggestions plus the offline test server landed as written. Thanks for working through three rounds of this.

The timeout budget and the 429 handling are now #90. They belong to a change that covers every outbound call rather than this one, so they are not yours to carry here.