Skip to content

refactor(appkit): restructure supervisor-api adapter per PR databricks#345 review#400

Open
hubertzub-db wants to merge 3 commits into
databricks:mainfrom
hubertzub-db:agent/v2/sa/1-adapter-fixes
Open

refactor(appkit): restructure supervisor-api adapter per PR databricks#345 review#400
hubertzub-db wants to merge 3 commits into
databricks:mainfrom
hubertzub-db:agent/v2/sa/1-adapter-fixes

Conversation

@hubertzub-db
Copy link
Copy Markdown

@hubertzub-db hubertzub-db commented May 22, 2026

Address structural feedback from Mario Cadenas' review of PR #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.

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>
@hubertzub-db hubertzub-db requested a review from a team as a code owner May 22, 2026 08:32
@hubertzub-db hubertzub-db requested a review from calvarjorge May 22, 2026 08:32
@hubertzub-db hubertzub-db changed the title Agent/v2/sa/1 adapter fixes refactor(appkit): restructure supervisor-api adapter per PR databricks#345 review May 22, 2026
…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>
Copy link
Copy Markdown
Member

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @hubertzub-db for your contribution! I'd like to merge that shortly after #345
to avoid breaking changes in the API. Could you please take a look at some final comments? Thanks!

PR #400 (agent/v2/sa/1-adapter-fixes) — Fixes

Inherited from PR #345 (fix on #345, picked up via rebase)

All 9 issues from PR #345 apply identically — the code is carried over. Fix on #345 first.

# 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.
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
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

PR #400-specific fixes

P1 — Must fix

# File:Line Issue Fix
10 supervisor-api.ts:650 fromSupervisorApi returns Promise<SupervisorApiAdapter> not Promise<AgentAdapter> — binds consumers to concrete class Narrow return type to Promise<AgentAdapter>
11 supervisor-api.ts:315 No timeout on streamBody or SSE stream — hangs indefinitely if upstream stalls Verify TimeoutInterceptor wraps run(). If not, compose AbortSignal.timeout(ms) with context signal.

P2 — Should fix

# File:Line Issue Fix
12 beta.ts:23 SupervisorApiAdapterCtorOptions exported — internal type leaks as stable contract Remove from beta.ts exports
13 supervisor-api.ts:85 WorkspaceClientLike duplicated across adapters, not exported but used in exported type Export it, or use ApiClientLike & { config: ... }
14 supervisor-api.ts:654 Double-cast as unknown as WorkspaceClientLike signals structural mismatch Verify SDK's WorkspaceClient satisfies WorkspaceClientLike structurally; widen the type or narrow the cast
15 supervisor-api.ts:75 StreamBody type duplicated in both adapters Extract to connectors/serving/client.ts

P3 — Nice to have

# File:Line Issue Fix
16 supervisor-api.ts:101 SupervisorTool closed union — new upstream tool type = breaking change Consider `

Gap — Mario's Section 6

  • Frontmatter-referenced supervisor tools: The mechanism works (tool-index routing composes via agents({ tools: { nyc: supervisorTools.genieSpace(...) } }) + frontmatter tools: [nyc]), but no doc example or test demonstrates this path. Add a doc example and test.

Tests to add

  • fromSupervisorApi dynamic-import fallback (no workspaceClient supplied)
  • Capability-mismatch warning paths (partially present already)
  • Frontmatter-referenced supervisor tool resolution

Mario's Review Status

  • Section 9 (15 items): All addressed on PR #345
  • Sections 1-8 (structural): 7/8 fully addressed on PR #400, 1 partially (section 6 — frontmatter doc/test gap)
  • Section 10 (follow-ups): Acknowledged as non-blocking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants