feat(appkit): supervisor API adapter for agents plugin#345
feat(appkit): supervisor API adapter for agents plugin#345hubertzub-db wants to merge 2 commits into
Conversation
888f8ba to
bdcc529
Compare
29c1702 to
2db7108
Compare
2db7108 to
19d7450
Compare
Structural feedback on the supervisor API adapterFirst off — the substance of the adapter (recovery paths, terminal-event handling, structured tests) is solid. The feedback below is mostly about the public API shape and a few correctness / security items. The implementation can largely stay as-is; the changes are concentrated at the boundary between the adapter, This is 1. Discoverability — make
|
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>
…s#345 review Address structural feedback from Mario Cadenas' review of PR databricks#345 (sections 1-8). Stacked on top of the §9 defensive fixes commit. API changes (BETA surface only): - Add `DatabricksAdapter.fromSupervisorApi` static factory for discoverability alongside `.fromChatCompletions`. - Shrink `SupervisorApiAdapterOptions` to `{ model, workspaceClient? }`; tools no longer live on the adapter. - Hosted tools (`supervisorTools.*`) now return tagged `HostedSupervisorTool` records and accept named options instead of positional args. - Declare hosted tools on the agent's `tools` map (same place as function tools / sub-agents); the agents plugin and `runAgent` route them to the adapter via the new `AgentInput.extensions[SUPERVISOR_EXTENSION_KEY]`. - Add capability-negotiation fields to `AgentAdapter`: `acceptsExtensions?` + `consumesInputTools?`. The agents plugin and `runAgent` warn at registration when adapter capabilities don't match declared tools. Internals: - Extend `ResolvedToolEntry` / `StandaloneEntry` with a `hosted-supervisor` branch; `classifyTool` matches it before MCP hosted-tool rejection so standalone `runAgent` supports supervisor tools. - Defense-in-depth: both indexers throw if a `hosted-supervisor` entry is ever dispatched as a callable function. - `DatabricksAdapter.fromSupervisorApi` uses a dynamic import to avoid load-time cycles. Docs: - Rewrite supervisor-API section in docs/plugins/agents.md for the new shape. - Add cross-adapter sub-agent composition note (one-directional: chat parents can call supervisor children, not vice-versa, until SA's function-call events are routed back through `context.executeTool`). Playground: - Update dev-playground supervisor agent to the new shape (`DatabricksAdapter.fromSupervisorApi(...)` + tools on `createAgent`). Tests: - Rewrite supervisor-api.test.ts factory + adapter tests for the new shape. - Add `isSupervisorTool` and `DatabricksAdapter.fromSupervisorApi` tests. - New regression tests in `run-agent.test.ts` covering the hosted-supervisor extension-routing path and both capability-mismatch warnings. - New agents-plugin tests covering the same warning paths and the new `hosted-supervisor` tool-index branch. Signed-off-by: Hubert Zub <hubert.zub@databricks.com>
pkosiec
left a comment
There was a problem hiding this comment.
Overall LGTM but before we merge this please address those minor fixes:
PR #345 (agent/v2/sa/1-adapter) — Fixes
P1 — Must fix
| # | File:Line | Issue | Fix |
|---|---|---|---|
| 1 | supervisor-api.ts:356 |
readSseEvents throws on oversized blocks but for await loop has no try/catch — unhandled rejection tears down the request |
Wrap the for await loop in try/catch. On signal?.aborted return silently, otherwise yield emitError("transport", err). Add test. |
P2 — Should fix
| # | File:Line | Issue | Fix |
|---|---|---|---|
| 2 | supervisor-api.ts:397 |
Empty stream (zero SSE events) returns without terminal status — consumer stuck in "running" | When eventCounts.size === 0, yield emitError("transport", "stream closed without events") before returning. Add test. |
| 3 | supervisor-api.ts:428 |
incomplete_details != null treated as fatal even for benign max_output_tokens truncation |
Only treat as fatal when co-occurring with status === "failed" or error != null. Yield recovered text + complete when it's just truncation. Add test. |
| 4 | supervisor-api.ts:33 |
emitError uses %O at warn level, dumping full objects to logs (CWE-532) |
Replace logger.warn("... detail=%O", ...) with logger.warn("... detail=%s", code, summariseErrorPayload(detail)). Add logger.debug for full dump. |
| 5 | supervisor-api.ts:371 |
parsed.type as string — missing field produces "undefined" string |
Replace with typeof parsed.type === "string" ? parsed.type : "" |
| 6 | supervisor-api.ts:544 |
(data.delta as string) ?? "" — fragile cast |
Replace with typeof data.delta === "string" ? data.delta : "" |
| 7 | supervisor-api.ts:542 |
`data.item_id as string | undefined` — non-strings pass through |
P3 — Nice to have
| # | File:Line | Issue | Fix |
|---|---|---|---|
| 8 | sse-reader.ts:30 |
maxLineChars name says "line" but guards a "block" |
Rename to maxBlockChars (not yet public, safe to rename) |
| 9 | supervisor-api.ts:48 |
summariseErrorPayload — British spelling, codebase uses American |
Rename to summarizeErrorPayload |
Tests to add
- Empty stream path (zero SSE events)
readSseEventsthrowing mid-stream (maxLineChars/maxBufferChars propagation)response.completedwith onlyincomplete_detailssetresponse.output_item.donewith empty content array- Multiple system messages joined with
\n\n summariseErrorPayloadbranch coverage
Mario's Review Status
- Section 9 (15 items): All fully addressed on this branch
- Sections 1-8 (structural): Deferred to PR #400
Adds a fourth
AgentAdapterthat targets the Databricks AI Gateway Responses API, so AppKit apps can host server-side-orchestrated agents (Genie, Knowledge Assistants, UC functions, custom apps, UC connections) without managing tool execution locally.What's new
fromSupervisorApi({...})— factory that returns anAgentAdapter. Uses the SDK's default credential chain (env, profiles, OAuth, OIDC) just likeDatabricksAdapter.fromModelServing.supervisorTools.*— concise factories for the five hosted tool types (genieSpace,ucFunction,knowledgeAssistant,app,ucConnection).descriptionis required on every one — it's both validation and the routing hint the LLM uses to choose between tools.@databricks/appkit/agents/supervisor-api— new subpath export so consumers pick only the adapter they want; mirrors the existing./agents/{databricks,vercel-ai,langchain}pattern.Reference app
Demo
supervisoragent inapps/dev-playground/server/index.ts. Tools are commented out by default — uncomment anysupervisorTools.*entry to give the model real powers.Test plan