fix(appkit): address PR #345 review findings (section 9)

Apply Mario's defensive/correctness fixes from the supervisor API adapter
review without touching the public API shape (sections 1-8 will land in a
stacked branch). Highlights:

High
- Route the three SSE error-leak sites in supervisor-api.ts (streamBody
  catch, mapEvent "error", output_item.done with id="error") through a
  single emitError helper that returns a stable client-facing code
  (`Supervisor API error (transport|upstream_failed|upstream_tool|
  upstream_unknown)`) and logs the verbose detail server-side only.
  Addresses CWE-209 verbatim-upstream-error-text leak.

Medium
- Gate the terminal {status:"complete"} emission on lastCompleted.status
  / .error / .incomplete_details so a `response.completed` with a nested
  failed status no longer silently succeeds; surface as upstream_failed
  instead. Regression tests added.
- Skip the terminal error in the streamBody catch when signal.aborted —
  consumer-initiated aborts now end with a clean stop, not a contradictory
  terminal error event. Regression test added.
- Tighten the output_item.done error match: require item.type === "error"
  (or pair the reserved id="error" with a non-message type) so a stray
  assistant message with id="error" is not mis-classified.
- Add maxLineChars / maxBufferChars caps to readSseEvents with 1 MiB / 8
  MiB defaults; throw on overflow. Addresses CWE-770. Tests added.
- Docs: add a CWE-1427 callout warning that hosted-tool `description` is a
  prompt-injection sink — do not derive it from untrusted input.
- Redact the no-delta warning log: summariseErrorPayload extracts a short
  `type: message` line; full payload only via DEBUG. Addresses CWE-532.
- Gate the buffer-level CRLF normalize in sse-reader on `\r` presence to
  skip the regex on LF-only steady state.

Low
- mapEvent("error") fallback no longer wraps "Unknown error" with literal
  JSON quotes (uses string branch).
- Drop the misleading "we await the factory at module init" comment in
  dev-playground; the code never awaits.
- Fix @example imports in supervisor-api.ts JSDoc to use
  @databricks/appkit/beta (the actual public re-export).
- Replace trimStart() with single-U+0020 strip in sse-reader per the SSE
  spec; remove the now-dead per-line `\r$` strip after the buffer-level
  CRLF normalise.
- Flag streamPath as @internal in connectors/serving/client.ts noting the
  CWE-918 SSRF risk if it ever leaks to user-controlled input.
- Add JSDoc warning to workspaceClient on SupervisorApiAdapterOptions:
  passing a per-request OBO client would leak identity across requests
  (CWE-664).

Signed-off-by: Hubert Zub <hubert.zub@databricks.com>

Author: hubertzub-db

apps/dev-playground/server/index.ts

@@ -78,10 +78,10 @@ const helper = createAgent({
 // Uncomment a `supervisorTools.*` entry (and import 'supervisorTools' from
 // '@databricks/appkit/beta') to give the model real powers.
 //
-// We `await` the factory at module init so a misconfigured workspace
-// (missing host, bad credentials) fails fast with a clear error here
-// instead of as an unhandled rejection. Top-level await is fine in this
-// ESM module.
+// `createAgent({ model })` accepts an adapter promise, so the factory's
+// host/credential resolution is awaited lazily on first dispatch (via
+// `resolveAdapter` in the agents plugin). A misconfigured workspace will
+// surface at first chat request, not at module init.
 const supervisor = createAgent({
   instructions:
     "You are an assistant powered by the Databricks Supervisor API.",

docs/docs/plugins/agents.md

@@ -273,6 +273,12 @@ const model = fromSupervisorApi({
 
 `description` is **required and non-empty** — the LLM uses it to route between tools, so two Genie spaces both labelled "Genie space" will be indistinguishable.
 
+:::warning Hosted-tool descriptions are trusted application configuration (CWE-1427)
+A hosted tool's `description` is read by the LLM to decide when to route to that tool. **Do not derive it from untrusted input** — user messages, request bodies, freeform fields from external systems, or any value an attacker could influence. Treat `description` (and `id`/`name`) as application-controlled, alongside the agent's `instructions`. Allowing a user-controlled string here is a prompt-injection sink: a hostile description can convince the model to route to (or away from) a tool for any future request handled by the agent.
+
+The same caution applies to MCP `description`s and to any other field the model reads at routing time.
+:::
+
 | Factory | Tool kind | Identifier |
 |---|---|---|
 | `supervisorTools.genieSpace(id, description)` | Genie space | space id |

packages/appkit/src/agents/supervisor-api.ts

@@ -12,6 +12,59 @@ import { readSseEvents } from "../stream";
 
 const logger = createLogger("agents:supervisor-api");
 
+/**
+ * Stable client-facing error codes. We never surface raw upstream error
+ * strings to the client (CWE-209) — the helper logs the verbose detail
+ * server-side and returns one of these codes in the {@link AgentEvent}.
+ */
+type SupervisorErrorCode =
+  | "transport"
+  | "upstream_failed"
+  | "upstream_tool"
+  | "upstream_unknown";
+
+/**
+ * Single sink for all error events emitted by the adapter. Logs the verbose
+ * detail (stack, upstream payload, etc.) at `warn` level and returns a
+ * sanitised {@link AgentEvent} carrying only a stable code so the client
+ * never sees raw upstream text.
+ */
+function emitError(code: SupervisorErrorCode, detail: unknown): AgentEvent {
+  logger.warn("supervisor-api error code=%s detail=%O", code, detail);
+  return {
+    type: "status",
+    status: "error",
+    error: `Supervisor API error (${code})`,
+  };
+}
+
+/**
+ * Renders an upstream error / incomplete_details payload as a short
+ * single-line string for log lines. Avoids dumping the full JSON tree
+ * (CWE-532): we keep the discriminator (`type`/`code`) plus a trimmed
+ * message, and that's it. Full payloads are still available via
+ * `DEBUG=appkit:agents:supervisor-api`.
+ */
+function summariseErrorPayload(payload: unknown): string {
+  if (payload == null) return "<none>";
+  if (typeof payload === "string") {
+    return payload.length > 80 ? `${payload.slice(0, 80)}…` : payload;
+  }
+  if (typeof payload !== "object") return String(payload);
+  const obj = payload as Record<string, unknown>;
+  const kind =
+    (typeof obj.type === "string" && obj.type) ||
+    (typeof obj.code === "string" && obj.code) ||
+    (typeof obj.reason === "string" && obj.reason) ||
+    "object";
+  const message =
+    (typeof obj.message === "string" && obj.message) ||
+    (typeof obj.detail === "string" && obj.detail) ||
+    "";
+  const trimmed = message.length > 80 ? `${message.slice(0, 80)}…` : message;
+  return trimmed ? `${kind}: ${trimmed}` : kind;
+}
+
 /**
  * Transport shim: given a request body, returns the raw SSE byte stream from
  * the Supervisor API endpoint. Injected at construction time so callers can
@@ -135,6 +188,12 @@ export interface SupervisorApiAdapterOptions {
    * and per-request authentication. When omitted, a `WorkspaceClient({})`
    * is created internally using the default SDK credential chain
    * (`DATABRICKS_HOST`, OAuth, PAT, etc.).
+   *
+   * ⚠ The `workspaceClient` is captured at construction and reused across
+   * every request. Passing a per-request OBO (On-Behalf-Of) client here
+   * would silently leak the first request's identity into all subsequent
+   * requests served by this adapter instance. Use the default credential
+   * chain or pass a service-principal client. (CWE-664)
    */
   workspaceClient?: WorkspaceClientLike;
 }
@@ -168,11 +227,12 @@ export interface SupervisorApiAdapterCtorOptions {
  *
  * @example
  * ```ts
- * import { createApp, createAgent, agents } from "@databricks/appkit";
+ * import { createApp, createAgent } from "@databricks/appkit";
  * import {
+ *   agents,
  *   fromSupervisorApi,
  *   supervisorTools,
- * } from "@databricks/appkit/agents/supervisor-api";
+ * } from "@databricks/appkit/beta";
  *
  * const adapter = await fromSupervisorApi({
  *   model: "databricks-claude-sonnet-4",
@@ -254,13 +314,11 @@ export class SupervisorApiAdapter implements AgentAdapter {
     try {
       stream = await this.streamBody(body, signal);
     } catch (err) {
-      const message = err instanceof Error ? err.message : String(err);
-      logger.warn("Supervisor API request failed: %s", message);
-      yield {
-        type: "status",
-        status: "error",
-        error: `Supervisor API error: ${message}`,
-      };
+      // Aborts surface as exceptions thrown by `fetch`/SDK transports when
+      // the consumer cancels mid-request. Treat as a clean stop so consumers
+      // don't see a contradictory terminal `error` after their own abort.
+      if (signal?.aborted) return;
+      yield emitError("transport", err);
       return;
     }
 
@@ -360,26 +418,41 @@ export class SupervisorApiAdapter implements AgentAdapter {
     }
 
     if (eventCounts.has("response.completed")) {
+      // SA sometimes signals a failed turn via `response.completed` with a
+      // nested `status: "failed"` (or a populated `error`/`incomplete_details`)
+      // rather than emitting `response.failed`. Without this gate the
+      // adapter would silently yield `complete` on a server-side failure.
+      if (
+        lastCompleted?.status === "failed" ||
+        lastCompleted?.error != null ||
+        lastCompleted?.incomplete_details != null
+      ) {
+        yield emitError("upstream_failed", {
+          status: lastCompleted?.status,
+          error: lastCompleted?.error,
+          incomplete_details: lastCompleted?.incomplete_details,
+        });
+        return;
+      }
       yield { type: "status", status: "complete" };
     }
 
     if (!receivedAnyDelta) {
       const histogram = [...eventCounts.entries()]
         .map(([t, n]) => `${t}=${n}`)
         .join(", ");
-      const completedError = lastCompleted?.error
-        ? JSON.stringify(lastCompleted.error)
-        : undefined;
-      const completedIncomplete = lastCompleted?.incomplete_details
-        ? JSON.stringify(lastCompleted.incomplete_details)
-        : undefined;
       logger.warn(
         "Supervisor API stream completed without any output_text deltas. " +
           "events={%s} completed.status=%s completed.error=%s completed.incomplete=%s",
         histogram,
         lastCompleted?.status ?? "<none>",
-        completedError ?? "<none>",
-        completedIncomplete ?? "<none>",
+        summariseErrorPayload(lastCompleted?.error),
+        summariseErrorPayload(lastCompleted?.incomplete_details),
+      );
+      logger.debug(
+        "Supervisor API no-delta full payload: error=%O incomplete=%O",
+        lastCompleted?.error,
+        lastCompleted?.incomplete_details,
       );
     }
   }
@@ -476,14 +549,20 @@ function mapEvent(
     // the stream produced none, then emits `{status:"complete"}` itself.
 
     case "response.failed":
-      return { type: "status", status: "error", error: "Response failed" };
+      return emitError("upstream_failed", data);
 
     case "error": {
-      const errMsg =
+      // Branch detail extraction so a missing `error` field doesn't surface
+      // the JSON-stringified literal `'"Unknown error"'` (with quotes) in
+      // server logs. The client never sees this string — `emitError`
+      // sanitises it to a stable code.
+      const detail =
         typeof data.error === "string"
           ? data.error
-          : JSON.stringify(data.error ?? "Unknown error");
-      return { type: "status", status: "error", error: errMsg };
+          : data.error == null
+            ? "Unknown error"
+            : data.error;
+      return emitError("upstream_unknown", detail);
     }
 
     case "response.output_item.done": {
@@ -495,9 +574,15 @@ function mapEvent(
           }
         | undefined;
 
-      if (item?.id === "error") {
-        const errText = item.content?.[0]?.text ?? "Unknown tool error from SA";
-        return { type: "status", status: "error", error: errText };
+      // SA's contract reserves `item.id === "error"` for tool failures, but
+      // a 5-char identifier collision is too small a margin. Require either
+      // an explicit `type === "error"` or pair the reserved id with a
+      // non-message type (a normal assistant message uses `type: "message"`).
+      if (
+        item?.type === "error" ||
+        (item?.id === "error" && item?.type !== "message")
+      ) {
+        return emitError("upstream_tool", item);
       }
 
       // Fallback: when SA produces a tool-driven response (e.g. Genie space),
@@ -541,7 +626,7 @@ function mapEvent(
  * import {
  *   fromSupervisorApi,
  *   supervisorTools,
- * } from "@databricks/appkit/agents/supervisor-api";
+ * } from "@databricks/appkit/beta";
  *
  * const adapter = await fromSupervisorApi({
  *   model: "databricks-claude-sonnet-4",
@@ -553,6 +638,12 @@ function mapEvent(
  *   ],
  * });
  * ```
+ *
+ * @remarks
+ * ⚠ When passing your own `workspaceClient`, see the warning on
+ * {@link SupervisorApiAdapterOptions.workspaceClient} — the client is
+ * captured once and reused, so per-request OBO clients would leak
+ * identity across requests.
  */
 export async function fromSupervisorApi(
   options: SupervisorApiAdapterOptions,