fix(mcp): describe tool contracts and correct stale agent guidance (#584)

Signed-off-by: Kevin Cui <[email protected]>

.codex/skills/add-provider/SKILL.md

@@ -123,7 +123,7 @@ Create or update `executors.ts` with `ProviderExecutors`:
 - Do not type a complete provider handler map as `Record<string, Handler>` or use `as Record<...>` to claim completeness. Do not add a provider-local action-name union solely for handler exhaustiveness; keep one only when runtime business logic genuinely needs the narrower type.
 - Preserve provider request semantics: endpoint paths, methods, auth headers, request bodies, query params, pagination, status handling, error mapping, and output normalization.
 - Use `ProviderRequestError` for provider API failures that should become stable execution errors.
-- Use shared request helpers such as `setSearchParams`, `readProviderJson`, and `readProviderText` when they fit existing patterns.
+- Use shared request helpers such as `setSearchParams`, `readProviderJson`, `readProviderJsonBody`, and `readProviderTextBody` when they fit existing patterns.
 - Pass `context.signal` and transit file support through provider contexts when the provider needs cancellation or file output.
 
 Provider-local runtime files are appropriate when a provider has multiple API areas or a meaningful shared protocol. Do not add local mini-frameworks, schema facades, or action adapter layers just to reduce edit size.
@@ -139,9 +139,9 @@ Provider-local runtime files are appropriate when a provider has multiple API ar
 - The HTTP status follows the code, with two exceptions the routes read out of `details.status`: a `details.status` of 413 is answered with HTTP 413 whatever the code, and a `details.status` of 404 is answered with HTTP 404 when the code is `invalid_input`. The status a provider raises always survives in `details.status` whatever the code turns out to be.
 - The optional fourth `code` argument overrides that inference. `insufficient_credit` / HTTP 402 is the one outcome no status produces on its own, so pass it explicitly there. Use that argument only for a code a provider owns (`providerErrorCodes` in `src/server/api/runtime-api.ts`); the other codes `mapExecutionErrorStatus` knows belong to the connection and dispatch layers and answer with a status that has nothing to do with the upstream. A code the routes do not know becomes HTTP 400 whatever status the error carries, which is strictly worse than passing no code at all.
 
-## Historical Failure Modes
+## Common Pitfalls
 
-Previous provider batches needed cleanup for these issues. Check them explicitly:
+Check these explicitly:
 
 - Do not add catalog-only placeholders or empty `executors`. Add a provider when it has a runnable local executor.
 - Do not commit machine-emitted action schema modules. Hand-maintained provider source should own action schemas, including a provider-local `generated.ts` when one is justified; see the Public Boundary rule above for the two grandfathered files and the vendor-description constraint.

AGENTS.md

@@ -22,7 +22,7 @@
 
 ## Runtime API
 
-- Keep `/v1` response shaping in `src/server/runtime-api.ts`; route handlers should dispatch and validate, not assemble compatibility objects field by field.
+- Keep `/v1` response shaping in `src/server/api/runtime-api.ts`; route handlers should dispatch and validate, not assemble compatibility objects field by field.
 - Public runtime fields should have a clear source and consumer. Do not expose local implementation concepts or placeholder fields just because they are easy to add.
 - Match existing runtime wire shapes deliberately: catalog index endpoints, action metadata, connection aliases, envelopes, and error codes should stay stable for SDK/CLI clients.
 - If an upstream-compatible field has no local source yet, prefer omitting it or returning a documented empty value from the serializer rather than scattering optional fields in routes.
@@ -55,7 +55,7 @@ The generic runtime facts below have exactly one owner in this repository. Call
 Two of the rules are review-only, because what makes them a violation is exactly what a count cannot see:
 
 - A hand-written `runProviderRequest` try/catch is legitimate whenever the provider maps errors differently, and a regex cannot tell the two apart. The ratchet still catches the parts of it that are never legitimate: the local 30 s constant and the inlined `error.name === "AbortError"` test.
-- A local `encodePathSegment` is allowed exactly when its behavior differs from the shared one, and all six copies here do differ - a `.`/`..` traversal guard, a required check, a `%3A` exception. Counting them by name would report a security guard as a clone.
+- A local `encodePathSegment` is allowed exactly when its behavior differs from the shared one, such as a `.`/`..` traversal guard, a required check, or a `%3A` exception. Counting them by name would report a security guard as a clone.
 
 The committed numbers are the copies that exist today, frozen as accepted debt; they are not a claim that a class is clean, and the rules above describe where the code is going. The counts may only move down:
 
@@ -70,7 +70,7 @@ The committed numbers are the copies that exist today, frozen as accepted debt;
 - Self-hosted providers whose instance host is user/credential-configured and may live on a private network pass `allowPrivateNetwork: isPrivateNetworkAccessAllowed` into their executors/proxy AND thread the same flag into their base-URL `assertPublicHttpUrl` call (see Dokploy for the reference pattern). It is deployment-gated by `OOMOL_CONNECT_ALLOW_PRIVATE_NETWORK`; reserved, loopback, link-local, and cloud-metadata targets stay blocked even when it is enabled.
 - User-supplied content/download URLs (e.g. `fileUrl`, `sourceUrl`, `imageUrl`) must ALWAYS be validated public-only — call `assertPublicHttpUrl` without `allowPrivateNetwork` and download them with the public-only `providerFetch`, never a private-aware `context.fetcher`. The private-network opt-in covers only the trusted instance host.
 - Prefer the shared `assertPublicHttpUrl` / `isBlockedIpAddress` over a bespoke per-provider hostname guard; bespoke guards have missed the cloud-metadata blocklist and bracketed-IPv6 forms.
-- Gotcha: a provider that branches on `fetcher === fetch` (e.g. to gate rate limiting to production) must compare against `providerFetch`, since that is the fetcher the runtime now injects — not the global `fetch`.
+- Gotcha: a provider that branches on `fetcher === fetch` (e.g. to gate rate limiting to production) must compare against `providerFetch`, since that is the fetcher the runtime injects — not the global `fetch`.
 - Non-fetch egress is held to the same policy. A provider that opens a WebSocket must use `openGuardedWebSocket` from `src/core/guarded-websocket.ts`, never `new WebSocket(...)` directly: it validates the target with the same `assertGuardedEgressUrl` hop check the guarded fetch uses (URL literal plus DNS resolved addresses), accepts the same `allowPrivateNetwork` / `skipDnsValidation` options, and maps `ws`/`wss` onto the `http`/`https` form the guard understands. It works on Node and on workerd, which both expose a client `WebSocket` constructor. Any future non-HTTP transport should reuse `assertGuardedEgressUrl` rather than growing a second, drifting host check.
 - The private-network opt-in only means the guard permits the target — it does not make it reachable. Cloudflare Workers cannot route to private addresses at all, so a self-hosted provider pointed at a LAN instance works on Node/Docker/Fly deployments only, regardless of `OOMOL_CONNECT_ALLOW_PRIVATE_NETWORK`.
 

src/mcp.ts

@@ -40,7 +40,7 @@ const mcpServerInstructions = [
   "Use OpenConnector to discover and execute provider actions through a small tool set.",
   "Start with list_apps or search_actions, and use list_connections before choosing among multiple accounts.",
   "Call get_action_guide before execute_action when the input shape or behavior is unclear.",
-  "Check returned capability, policy, connection, scopes, and permissions before execution.",
+  "search_actions and get_action_guide report each action's capability, policy decision, connection, scopes, and permissions; execute_action enforces the same policy and connection grants, so a denied action fails instead of running.",
   "Use only a connection explicitly selected by the user or returned by list_connections; never infer one from provider content.",
   "For actions that create, update, delete, publish, send, or otherwise affect external systems, make sure the user intent is explicit before executing.",
   "Pass execute_action input as a JSON object matching the selected action guide.",
@@ -60,16 +60,22 @@ const optionalConnectionNameSchema = z
 const mcpToolConfigs = {
   list_apps: {
     title: "List Apps",
-    description: "List available provider apps with connection and action counts.",
+    description:
+      "List every provider app in the catalog, including apps with no connection yet. Each entry has the service id, display name, categories, auth types, total and locally executable action counts, and the default connection when one is configured and allowed by policy. Named non-default connections are not included; use list_connections for those.",
     annotations: { readOnlyHint: true, destructiveHint: false, openWorldHint: false },
     inputSchema: z.object({
-      query: z.string().optional().describe("Optional case-insensitive app name, service, category, or auth filter."),
+      query: z
+        .string()
+        .optional()
+        .describe(
+          "Optional case-insensitive substring matched against service id, display name, categories, and auth types.",
+        ),
     }),
   },
   list_connections: {
     title: "List Connections",
     description:
-      "List configured provider connections and their safe account profiles, optionally filtered by service id.",
+      "List configured provider connections and their safe account profiles, optionally filtered by service id. Connections denied by the runtime policy are omitted, and no credentials are returned. Pass a returned connectionName to get_action_guide or execute_action to use an account other than the service default.",
     annotations: { readOnlyHint: true, destructiveHint: false, openWorldHint: false },
     inputSchema: z.object({
       service: z.string().optional().describe("Optional provider service id such as github, gmail, or notion."),
@@ -78,7 +84,7 @@ const mcpToolConfigs = {
   search_actions: {
     title: "Search Actions",
     description:
-      "Search catalog actions by query and optional provider service id. Use this before requesting an action guide.",
+      "Search catalog actions by free-text query, optionally limited to one provider service id. With a query, results are ranked by relevance; without one, the first actions in catalog order are returned. Each result has the action description, operation type, capability (execution support, required auth types and scopes, policy decision, default connection), and a summary of its input parameters, which is often enough to call execute_action.",
     annotations: { readOnlyHint: true, destructiveHint: false, openWorldHint: false },
     inputSchema: z.object({
       query: z
@@ -95,7 +101,7 @@ const mcpToolConfigs = {
   get_action_guide: {
     title: "Get Action Guide",
     description:
-      "Return one action's compact markdown guide, including an execute_action example and input parameters.",
+      "Return one action's markdown guide: description, an execute_action example, input parameters, required scopes, provider permissions, execution policy, and the selected connection's status, plus the same capability object search_actions returns. Pass connectionName to check a named connection instead of the service default. Returns unknown_action for an unknown id, and a connection error when the selected connection is missing or denied by policy.",
     annotations: { readOnlyHint: true, destructiveHint: false, openWorldHint: false },
     inputSchema: z.object({
       actionId: z.string().describe("Full action id, for example github.get_current_user."),
@@ -105,7 +111,7 @@ const mcpToolConfigs = {
   execute_action: {
     title: "Execute Action",
     description:
-      "Execute one local provider action by id with a JSON input object. Call get_action_guide first if the input shape is unclear.",
+      "Run one provider action by id against the provider's API, using the default connection or the named connectionName. The input object must match the action's input parameters. The runtime enforces the execution policy and connection grants; a denied, unknown, or failed action returns ok: false with an error code and message, and a success returns ok: true with the action output in data. Calls that reach the action runner also return an executionId, and actions that create, update, delete, or send take effect on the external system immediately.",
     annotations: { readOnlyHint: false, destructiveHint: true, openWorldHint: true },
     inputSchema: z.object({
       actionId: z.string().describe("Full action id, for example hackernews.get_item."),