Skip to content

feat: add typed sql parameter helpers#9

Merged
ditadi merged 9 commits intomainfrom
feat/sql-param-helpers
Dec 8, 2025
Merged

feat: add typed sql parameter helpers#9
ditadi merged 9 commits intomainfrom
feat/sql-param-helpers

Conversation

@ditadi
Copy link
Copy Markdown
Contributor

@ditadi ditadi commented Dec 8, 2025

Summary

Add sql.* helpers for type-safe SQL query parameters. Solves the problem of agents struggling to parameterize SQL and properly cast values.

Helpers

  • sql.date() - Date objects or ISO strings → YYYY-MM-DD
  • sql.timestamp() - Date, ISO string, or Unix timestamp
  • sql.string() - String, number, or boolean
  • sql.number() - Number or numeric string
  • sql.boolean() - Boolean, string ("true"/"false"), or number (0/1)
  • sql.binary() - Uint8Array, ArrayBuffer, or hex-encoded string

Usage

  import { sql } from "@databricks/app-kit-ui/js";

  const params = {
    startDate: sql.date(new Date()),
    endDate: sql.date("2024-12-31"),
    userId: sql.number(123),
    name: sql.string("John"),
    isActive: sql.boolean(true),
  };

Notes

  • All helpers validate inputs and throw descriptive errors
  • Exports SQLTypeMarker type and isSQLTypeMarker type guard for custom integrations
  • SQL Helpers are exported from the app-kit for server and from app-kit-ui/js for the client.

@ditadi ditadi requested review from a team, MarioCadenas and fjakobs December 8, 2025 10:43
Copy link
Copy Markdown
Collaborator

@fjakobs fjakobs left a comment

Choose a reason for hiding this comment

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

Looks good. Let's just make sure that the encoding for binary parameters is correct.

Comment thread apps/dev-playground/client/src/lib/utils/filter-utils.ts
Comment thread apps/dev-playground/client/src/routeTree.gen.ts
.join("");
} else if (value instanceof ArrayBuffer) {
binaryValue = Array.from(new Uint8Array(value))
.map((b) => b.toString(16).padStart(2, "0").toUpperCase())
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did you test sending binary data. Would like to have confidence that we are encoding the data properly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Improved the tests and added a new interactive demo showing the features + doing a real SQL test on the dev-playground.

Screen.Recording.2025-12-08.at.12.42.13.mov

@ditadi ditadi merged commit 3cddd0e into main Dec 8, 2025
3 checks passed
@MarioCadenas MarioCadenas deleted the feat/sql-param-helpers branch December 19, 2025 17:44
@pkosiec pkosiec mentioned this pull request Apr 23, 2026
3 tasks
MarioCadenas added a commit that referenced this pull request May 7, 2026
PR #303 agentic review applied (P1 + cheap P2 + P3 cleanup):

- Snapshot lifecycle hooks before iteration so a callback that registers
  another hook for the same event does not re-enter the loop.
- Add attachContext to EXCLUDED_FROM_PROXY so asUser() never proxies
  internal binding lifecycle into user context.
- Use SpanStatusCode.OK / .ERROR instead of magic numbers; the previous
  code: 0 was UNSET (no-op for setStatus), so the success path was
  silently unreported in OTel traces.
- Return getPlugins() as ReadonlyMap to prevent external mutation of
  the live plugin registry.
- Strengthen isToolProvider to also require asUser, narrow to a
  ToolProviderPlugin shape, and drop the (entry.plugin as any).asUser
  cast in executeTool.
- Guard double registerAsRouteTarget with logger.warn + ignore.
- Guard duplicate registerToolProvider name with logger.warn.
- Drop the ToolProviderEntry indirection; store ToolProviderPlugin
  directly keyed by name.

Tests cover Set-mutation safety, double registerAsRouteTarget, duplicate
tool-provider, the asUser requirement on isToolProvider, and the
SpanStatusCode assertions on success and failure paths.

Also adds plugin/to-plugin.ts to the knip ignore list. NamedPluginFactory
is consumed only by downstream branches (fromPlugin) and was being flagged
as unused on this branch in isolation.

Findings #8 (configurable executeTool timeout), #9 (double context
injection), and #10 (BasePluginConfig context cast) are advisory and
deferred to a follow-up.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
MarioCadenas added a commit that referenced this pull request May 7, 2026
…ediator (#303)

* feat(appkit): tool primitives and ToolProvider surfaces on core plugins

Introduces the tool-authoring primitives that peer plugins use to expose
their capabilities as agent tools, and updates analytics, files, genie,
and lakebase to implement the ToolProvider interface.

Tool helpers land in core/agent/ (not plugins/agents/) from day one so
peer plugins can depend on them without reaching across the sibling
boundary:

  core/agent/types.ts          — ToolkitEntry, AgentDefinition shape
  core/agent/build-toolkit.ts  — converts ToolRegistry → ToolkitEntry map
  core/agent/tools/
    define-tool.ts             — defineTool() + ToolRegistry
    function-tool.ts           — FunctionTool interface + helpers
    hosted-tools.ts            — HostedTool / mcpServer() types
    sql-policy.ts              — assertReadOnlySql guard
    tool.ts                    — tool() Zod-schema factory
    json-schema.ts             — Zod → JSON Schema converter
    index.ts                   — public barrel

MCP client (AppKitMcpClient) and host-policy live in
plugins/agents/tools/ at this stage; a later commit promotes them to
connectors/mcp/ once the connector layer exists.

* docs(appkit): explain hand-rolled AppKitMcpClient vs official MCP SDK

Add a file-level rationale (policy/auth, narrow scope, zero extra deps) and
point the class JSDoc at it to avoid duplicating the same story in two places.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* refactor(appkit): merge FilesPlugin ctor volume loops

Single pass over volumes: connectors, toolkit tools, and policy warnings.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* fix(appkit): restore beta header, index GA/jobs exports; drop invalid ./plugins barrel

- Keep v2/1 beta.ts comment block; retain Databricks + tool-primitive exports
- Restore JobsConnectorConfig, ga-exports.generated, and jobs plugin types on index
- Remove broken export from ./plugins (no plugins/index.ts on this branch)

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* chore(appkit): satisfy Biome lint on tool-primitive wiring

- organizeImports in core tools barrel, analytics, files, genie, lakebase, mcp-client
- drop stale noExplicitAny biome-ignore (rule is off; suppressions flagged)
- remove unused DownloadResponse import; use vi.mocked + cast in lakebase test

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* fix(appkit): type lakebase agent-tool test pool mock as PoolClient

Vitest mockReturnValueOnce is checked against pg.Pool; connect must return
Promise<PoolClient>. Use a stub client cast to PoolClient for the failure case.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* fix(appkit): re-export tool barrel from beta for knip parity

Expose defineTool, MCP client, toolkit helpers alongside existing beta
tool exports so Knip recognizes core/agent/tools/index as used entry surface.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* feat(appkit): drop Lakebase iUnderstandRunsAsServicePrincipal flag

The acknowledgement field added no real defense beyond the implicit
opt-in (exposeAsAgentTool is itself an explicit, undefined-by-default
field), and created asymmetry with sibling SP-bound SQL surfaces
(analytics, genie). It would also drift once OBO lands.

Real protections - read-only SQL classifier, BEGIN READ ONLY/ROLLBACK
transaction wrapping, destructive-call HITL approval gate, and the
startup warn log - are unchanged.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* feat(appkit): plugin infrastructure — attachContext lifecycle + PluginContext mediator

Third layer: the substrate every downstream PR relies on. No user-
facing API changes here; the surface for this PR is the mediator
pattern, lifecycle semantics, and factory stamping.

`Plugin` constructors become pure — no `CacheManager.getInstanceSync()`,
no `TelemetryManager.getProvider()`, no `PluginContext` wiring inside
`constructor()`. That work moves to a new lifecycle method:

```ts
interface BasePlugin {
  attachContext?(deps: {
    context?: unknown;
    telemetryConfig?: TelemetryOptions;
  }): void;
}
```

`createApp` calls `attachContext()` on every plugin after all
constructors have run, before `setup()`. This lets factories return
`PluginData` tuples at module scope without pulling core services into
the import graph — a prerequisite for later PRs that construct agent
definitions before `createApp`.

`packages/appkit/src/core/plugin-context.ts` — new class that mediates
all inter-plugin communication:

- **Route buffering**: `addRoute()` / `addMiddleware()` buffer until
  the server plugin calls `registerAsRouteTarget()`, then flush via
  `addExtension()`. Eliminates plugin-ordering fragility.
- **ToolProvider registry**: `registerToolProvider(name, plugin)` +
  live `getToolProviders()`. Typed discovery of tool-exposing plugins.
- **User-scoped tool execution**: `executeTool(req, pluginName,
  localName, args, signal?)` resolves the provider, wraps in
  `asUser(req)` for OBO, opens a telemetry span, applies a 30s
  timeout, dispatches, returns.
- **Lifecycle hooks**: `onLifecycle('setup:complete' | 'server:ready'
  | 'shutdown', cb)` + `emitLifecycle(event)`. Callback errors don't
  block siblings.

`packages/appkit/src/plugin/to-plugin.ts` — the factory now attaches a
read-only `pluginName` property to the returned function. Later PRs'
`fromPlugin(factory)` reads it to identify which plugin a factory
refers to without needing to construct an instance. `NamedPluginFactory`
type exported for consumers who want to type-constrain factories.

`ServerPlugin.setup()` no longer calls `extendRoutes()` synchronously.
It subscribes to the `setup:complete` lifecycle event via
`PluginContext` and starts the HTTP server there. This ensures that
any deferred-phase plugin (agents plugin in a later PR) has had a
chance to register routes via `PluginContext.addRoute()` before the
server binds. Removes the `plugins` field from `ServerConfig` (routes
are now discovered via the context, not a config snapshot).

- 25 new PluginContext tests (route buffering, tool provider registry,
  executeTool paths, lifecycle hooks, plugin metadata)
- Updated AppKit lifecycle tests to inject `context` instead of
  `plugins`
- Full appkit vitest suite: 1237 tests passing
- Typecheck clean across all 8 workspace projects

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* fix(appkit): apply review feedback to PluginContext and plugin proxy

PR #303 agentic review applied (P1 + cheap P2 + P3 cleanup):

- Snapshot lifecycle hooks before iteration so a callback that registers
  another hook for the same event does not re-enter the loop.
- Add attachContext to EXCLUDED_FROM_PROXY so asUser() never proxies
  internal binding lifecycle into user context.
- Use SpanStatusCode.OK / .ERROR instead of magic numbers; the previous
  code: 0 was UNSET (no-op for setStatus), so the success path was
  silently unreported in OTel traces.
- Return getPlugins() as ReadonlyMap to prevent external mutation of
  the live plugin registry.
- Strengthen isToolProvider to also require asUser, narrow to a
  ToolProviderPlugin shape, and drop the (entry.plugin as any).asUser
  cast in executeTool.
- Guard double registerAsRouteTarget with logger.warn + ignore.
- Guard duplicate registerToolProvider name with logger.warn.
- Drop the ToolProviderEntry indirection; store ToolProviderPlugin
  directly keyed by name.

Tests cover Set-mutation safety, double registerAsRouteTarget, duplicate
tool-provider, the asUser requirement on isToolProvider, and the
SpanStatusCode assertions on success and failure paths.

Also adds plugin/to-plugin.ts to the knip ignore list. NamedPluginFactory
is consumed only by downstream branches (fromPlugin) and was being flagged
as unused on this branch in isolation.

Findings #8 (configurable executeTool timeout), #9 (double context
injection), and #10 (BasePluginConfig context cast) are advisory and
deferred to a follow-up.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* docs(appkit): regenerate Plugin API reference for attachContext + context

Typedoc reflects the new attachContext lifecycle method and the
PluginContext-typed context field added in 91e66e1.

Fixes the docs:build sync gate failing on agent/v2/3-plugin-infra CI.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

---------

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
Co-authored-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
MarioCadenas added a commit that referenced this pull request May 7, 2026
… string)

Address the two PR-304 P2 findings deferred from the first round:

#8 — Hardcoded 30s timeout in PluginContext.executeTool truncated
legitimate cold SQL Warehouse and long Genie tool calls. Add
agents({ limits: { toolCallTimeoutMs } }) (default 5 minutes), plumb
through RunState into PluginContext.executeTool which now accepts an
optional timeoutMs (also defaulting to 300_000 for non-agents callers).

#9 — normalizeToolResult returned the raw object for short non-string
results and a string for truncated results. Every downstream consumer
stringified at the wire boundary anyway, so the asymmetry just complicated
the type signature. Always return string and JSON-stringify null/objects;
the existing defensive typeof === "string" ? : JSON.stringify(...) at
adapter and translator sites still handles non-AppKit-mediated results.

#11 (printRegistry uses console.log) is left as-is — intentional
picocolors-styled startup banner; logger prefix would break the column
alignment and bypass log-level filtering.

Tests cover the new toolCallTimeoutMs default (300_000), the override
path, the runState → context plumbing, and the always-string contract on
normalizeToolResult including the null → "null" change.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
MarioCadenas added a commit that referenced this pull request May 8, 2026
PR #305 agentic-review fixes (P1 + cheap P2):

- _handleChat / _handleInvocations now wrap threadStore calls in
  try/catch and surface failures as a 500. Without this an unreachable
  store hung the SSE client until the upstream proxy timeout because
  the async Express handler propagated the rejection without a
  response.
- runAgent rejects hosted/MCP tools at index-build time with a clear
  pointer to createApp({ plugins: [..., agents()] }). Previously the
  adapter saw the tool list and the failure surfaced mid-conversation.
- Extract applyToolkitOptions: single source of truth for the
  prefix/only/except/rename filtering shared by build-toolkit (registry
  path) and toolkit-resolver (getAgentTools fallback). Bug fixes here
  apply to both paths instead of drifting between them.
- resolveStandaloneProvider now uses an isStandaloneToolProvider type
  guard instead of `instance as unknown as ToolProvider`. Distinct from
  core/plugin-context.isToolProvider which also requires asUser
  (request-scoped, only meaningful inside createApp).

Tests added:

- threadStore failure paths on /chat (get/create/addMessage rejections)
  and /invocations (create + addMessage mid-loop) — assert 500
  responses with the canonical error body.
- runAgent rejects hosted tools at standalone resolution.
- runAgent surfaces a clear error when fromPlugin references a plugin
  lacking ToolProvider methods.
- runAgent recursively executes sub-agents declared on def.agents.

#4 (countUserStreams O(n)) deferred — n bounded by 5 × users in the
default config; not a hot path. #11 (printRegistry console.log) and

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
#9-#15 advisory items left as-is per the PR-comment thread.
MarioCadenas added a commit that referenced this pull request May 8, 2026
…agents (#304)

* feat(appkit): agents() plugin, createAgent(def), and markdown-driven agents

The main product layer. Turns an AppKit app into an AI-agent host with
markdown-driven agent discovery, code-defined agents, sub-agents, and
a standalone run-without-HTTP executor.

Agent runtime files land in core/agent/ from day one:

  core/agent/create-agent.ts    — createAgent() definition factory
  core/agent/run-agent.ts       — standalone adapter loop (no HTTP)
  core/agent/load-agents.ts     — markdown agent discovery
  core/agent/system-prompt.ts   — base system prompt + composition
  core/agent/types.ts           — updated with AgentDefinition,
                                   AgentsPluginConfig, RegisteredAgent, etc.

HTTP-facing concerns stay in plugins/agents/:
  agents.ts, thread-store.ts, tool-approval-gate.ts,
  event-channel.ts, event-translator.ts, schemas.ts,
  defaults.ts, manifest.json

* refactor(appkit): generalize default base system prompt

Tool-agnostic guidelines instead of SQL/files-specific defaults; accept full
PromptContext in buildBaseSystemPrompt for parity with custom callbacks.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* feat(appkit): optional serving_endpoint on agents plugin manifest

Register DATABRICKS_SERVING_ENDPOINT_NAME as optional CAN_QUERY so apps using
Databricks-hosted agent models get resource wiring; optional when agents use
only external adapters. Sync template/appkit.plugins.json.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* fix(appkit): agents manifest uses DATABRICKS_AGENT_ENDPOINT

Align optional serving resource with `DatabricksAdapter.fromModelServing()`, which
reads `DATABRICKS_AGENT_ENDPOINT` — not `DATABRICKS_SERVING_ENDPOINT_NAME`
(serving plugin). Sync template.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* feat(agents): folder-based markdown discovery (<id>/agent.md)

Top-level config/agents/*.md is no longer loaded. Use
<agentId>/agent.md. The skills directory name is reserved and skipped.
Orphan top-level .md files error at load; subdirs without agent.md
error.

Export agentIdFromMarkdownPath for path-based id resolution.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* refactor(appkit): promote MCP client + host policy to connectors/mcp

The MCP transport client and host policy aren't agents-specific; they are
HTTP + JSON-RPC transport with URL/DNS allowlisting. Move them under
packages/appkit/src/connectors/mcp/ so they sit alongside the other
transport-layer modules (serving, genie, sql-warehouse, lakebase, …) and
stop being reachable only through the agents plugin.

- Move mcp-client.ts          -> connectors/mcp/client.ts
- Move mcp-host-policy.ts     -> connectors/mcp/host-policy.ts
- Move McpEndpointConfig type -> connectors/mcp/types.ts
- Add connectors/mcp/index.ts barrel; re-export from connectors/index.ts
- Move mcp-client / mcp-host-policy tests to connectors/mcp/tests/
- Agents plugin keeps hosted-tools.ts (HostedTool sugar + resolve) and
  imports connector types from ../../connectors/mcp.
- tools/ barrel no longer re-exports AppKitMcpClient (never was public).

No behaviour change. All existing tests pass against the new paths.

* refactor(appkit): extract normalizeToolResult, consumeAdapterStream, dispatchToolCall

Three small helpers pulled out of the AgentsPlugin streaming path to cut
duplication and shrink the two large methods.

- normalize-result.ts: void->"", JSON-stringify, 50K truncation with a
  human-readable marker. Unit-testable (previously covered only via the
  HTTP path).
- consume-adapter-stream.ts: the 'message_delta' + 'message' accumulation
  loop shared between _streamAgent and runSubAgent. Accepts an optional
  signal and per-event side-effect callback (for SSE translation).
- tool-dispatch.ts: one place that fans out toolkit/function/mcp/subagent
  entries. 'never'-typed default forces exhaustiveness: adding a fifth
  source is now a compile error at every call site.

_streamAgent: executeTool closure shrinks from ~60 lines of dispatch +
normalize to a single dispatchToolCall + normalizeToolResult call.
Stream consumption collapses to consumeAdapterStream.

runSubAgent: childExecute shrinks from ~30 lines of if/else dispatch to
one dispatchToolCall call. Adapter loop collapses to consumeAdapterStream.

Behaviour change (minor): childExecute previously silently fell through to
'Unsupported sub-agent tool source' when mcpClient or PluginContext was
missing; now it throws the same specific error as the main stream. Matches
the main-path behaviour.

Tests: 15 new unit tests for normalizeToolResult + consumeAdapterStream.
dispatchToolCall is exercised transitively through the full agent suite
(288 existing tests still pass, 303 total on this branch).

* fix(agents): propagate tool annotations through tool() → FunctionTool → def

The `annotations` field (notably `destructive: true`) was silently dropped
as tools flowed from `tool({...})` into the resolved `AgentToolDefinition`,
so user-defined destructive tools never triggered the approval gate.

- `ToolConfig` now accepts `annotations?: ToolAnnotations`.
- `tool()` forwards it to the returned `FunctionTool`.
- `FunctionTool` exposes `annotations` and `functionToolToDefinition`
  preserves it on the definition it builds.
- `AgentsPlugin` reads the flag via `isDestructiveToolEntry()` (falls back
  to `functionTool.annotations` so a future divergence between def and
  function cannot re-introduce the bug) and emits the merged annotations
  via `combinedToolAnnotations()` on the `approval_pending` SSE payload.

Covered by `tests/tool-approval-gate.test.ts` and
`tests/function-tool.test.ts`.

* feat(agents): semantic ToolEffect — write/update/destructive tiers

ToolAnnotations.destructive is binary and has started to mislead:
"save_view" captures a screenshot and creates a new file, which is
nothing like deleting a dashboard, yet both trip the same red
"destructive" approval card. This adds a semantic `effect` enum with
four tiers — `read`, `write`, `update`, `destructive` — so tool
authors can tell the UI what blast radius they actually have. The
approval gate fires for any mutating effect (`write`/`update`/
`destructive`) and continues to honour the legacy `destructive: true`
flag so existing tools keep their current red treatment without
migration. Callers consuming `annotations` over the wire (MCP clients,
approval UIs) can now differentiate; the playground will ship a
tiered approval card as a follow-up.

* chore(appkit): post-rebase formatting and lockfile sync

Biome import collapsing on agent loader, run-agent, and tests after
rebasing onto main. Lockfile and synced plugin manifest reflect the
current main state (including get-port from #349 already on main).

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* fix(appkit): apply review feedback to agents plugin

PR #304 agentic review applied (P1 + cheap P2):

- Approval gate now honours the modern `effect` field (write/update/
  destructive), matching the documented contract on ToolAnnotations.
  Previously a tool authored with `effect: "destructive"` and no legacy
  `destructive: true` boolean bypassed the gate.
- Sub-agent tool calls share the parent's RunState, so the per-run
  tool-call budget and the destructive-tool approval gate apply to
  nested sub-agent calls — not only the top-level adapter.
- /invocations enforces `maxConcurrentStreamsPerUser`. Without this a
  client could bypass the cap by switching from /chat to /invocations.
- /cancel uses a Zod schema instead of a raw `as` cast, matching the
  validation pattern of the sibling routes.
- agents.reload() builds the registry into a fresh Map and only swaps
  on success. A malformed markdown file no longer wipes the live
  registry and breaks in-flight requests.
- consumeAdapterStream and normalizeToolResult are wired into the four
  call sites that previously inlined the same accumulation /
  serialization logic (top-level _streamAgent, runSubAgent,
  dispatchToolCall, runAgent).
- load-agents.ts uses fs.promises so reload() does not block the event
  loop.
- Pin js-yaml to 4.1.1 (drop the caret), matching the rest of appkit's
  pinned deps.

Tests cover the approval-gate `effect` matrix (destructive/write/update
trigger; read/undefined skip), the deny path, the shared budget across
top-level + sub-agent dispatches, and the /invocations rate-limit gate.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* fix(appkit): forward sub-agent events into the parent SSE stream

After dispatchToolCall consolidated the sub-agent path, runSubAgent
called consumeAdapterStream without forwarding the child's adapter
events. The parent stream therefore only emitted the outer agent-<name>
function call; nested tool_call / tool_result events from the sub-agent
never reached the client.

The smart-dashboard query agent delegates to dashboard_pilot whose UI-
action tools (apply_filter, highlight_period, focus_chart, etc.) rely on
the SSE stream to apply React state mutations. Without the forwarding,
the user asks for a highlight and nothing visible happens.

Forward every sub-agent event except metadata. Sub-agents have their
own threadId; emitting it would overwrite the parent's thread state on
the client and break multi-turn continuity.

Test exercises the metadata-skipping rule and asserts tool_call,
tool_result, and message_delta all reach outboundEvents.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

* fix(appkit): apply remaining review feedback (timeout config + result string)

Address the two PR-304 P2 findings deferred from the first round:

#8 — Hardcoded 30s timeout in PluginContext.executeTool truncated
legitimate cold SQL Warehouse and long Genie tool calls. Add
agents({ limits: { toolCallTimeoutMs } }) (default 5 minutes), plumb
through RunState into PluginContext.executeTool which now accepts an
optional timeoutMs (also defaulting to 300_000 for non-agents callers).

#9 — normalizeToolResult returned the raw object for short non-string
results and a string for truncated results. Every downstream consumer
stringified at the wire boundary anyway, so the asymmetry just complicated
the type signature. Always return string and JSON-stringify null/objects;
the existing defensive typeof === "string" ? : JSON.stringify(...) at
adapter and translator sites still handles non-AppKit-mediated results.

#11 (printRegistry uses console.log) is left as-is — intentional
picocolors-styled startup banner; logger prefix would break the column
alignment and bypass log-level filtering.

Tests cover the new toolCallTimeoutMs default (300_000), the override
path, the runState → context plumbing, and the always-string contract on
normalizeToolResult including the null → "null" change.

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>

---------

Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
Co-authored-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
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