Skip to content

feat: add type-safe sql queries with query registry - #11

Merged
ditadi merged 5 commits into
mainfrom
feat/type-safety-build-time
Dec 9, 2025
Merged

feat: add type-safe sql queries with query registry#11
ditadi merged 5 commits into
mainfrom
feat/type-safety-build-time

Conversation

@ditadi

@ditadiditadi commented Dec 8, 2025

Copy link
Copy Markdown
Contributor

Type-Safe SQL Queries with QueryRegistry

Summary

This PR introduces a compile-time type system for SQL queries, providing full type safety from query definition to result consumption. The system generates TypeScript types by analyzing SQL files and using DESCRIBE QUERY against Databricks to infer result schemas without executing the actual queries.

Features

QueryRegistry Type Generation

Automatically generates TypeScript interfaces for all SQL queries in your project:

interfaceQueryRegistry{spend_data: {name: "spend_data";parameters: {/** DATE - use sql.date() */startDate: SQLDateMarker;/** STRING - use sql.string() */groupBy: SQLStringMarker;};result: Array<{/** @sqlType STRING */group_key: string;/** @sqlType DECIMAL */cost_usd: number;}>;};}

SQL Parameter Type Safety

  • Marker Types: Specific interfaces for each SQL type (SQLDateMarker, SQLStringMarker, SQLNumberMarker, SQLBooleanMarker, SQLTimestampMarker, SQLBinaryMarker)
  • Numeric Type Support: All numeric types (INT, BIGINT, DECIMAL, FLOAT, DOUBLE, etc.) map to SQLNumberMarker with sql.number() helper
  • @param Annotations: Declare parameter types in SQL files:
    -- @param startDate DATE-- @param endDate DATE-- @param groupBy STRINGSELECT*FROM spend WHEREdate BETWEEN :startDate AND :endDate
  • JSDoc Hints: Generated types include inline documentation showing which sql.*() helper to use
  • Graceful Fallback: Unannotated parameters use SQLTypeMarker union, still enforcing helper usage

Type-Safe Query Hook

useAnalyticsQuery now provides full IntelliSense:

// ✅ Type-safe: IDE shows required parameters and their typesconst{ data, loading, error }=useAnalyticsQuery("spend_data",{startDate: sql.date("2024-01-01"),endDate: sql.date("2024-12-31"),groupBy: sql.string("app_name"),});// data is typed as Array<{ group_key: string; cost_usd: number }>

Result Schema Inference

  • Uses DESCRIBE QUERY to extract column metadata without executing queries
  • Normalizes parameterized types (DECIMAL(38,6)DECIMAL, ARRAY<STRING>ARRAY)
  • Maps all Databricks types to TypeScript (STRING, DECIMAL, INT, BIGINT, ARRAY, MAP, STRUCT, etc.)
  • Adds @sqlType JSDoc comments with full type info (e.g., @sqlType DECIMAL(38,6))
  • Intelligent caching to avoid redundant queries (invalidates when SQL changes)

Developer Experience

BeforeAfter
Parameters typed as anySpecific marker types with hints
Runtime errors for wrong typesCompile-time validation
No autocomplete for resultsFull IntelliSense on data
Manual type assertionsAutomatic type inference

Architecture

SQL Files (with @param annotations)
↓
Type Generator (npx appkit-generate-types)
├─ Parse @param comments → Parameter types
└─ DESCRIBE QUERY → Result schema (no execution)
↓
Generated appKitTypes.d.ts
↓
Module Augmentation (QueryRegistry)
↓
Type-safe useAnalyticsQuery<K>
├─ InferParams<K> → Required parameters
└─ InferResult<K> → Typed results

Test Plan

  • Run npx appkit-generate-types and verify appKitTypes.d.ts is generated correctly
  • Confirm IDE autocomplete works for query names in useAnalyticsQuery
  • Verify parameter types match @param annotations
  • Test compile error when using wrong sql.*() helper
  • Validate result types match actual query output
  • Ensure cache invalidation works when SQL files change

@ditadi
ditadi requested review from a team and fjakobsDecember 8, 2025 15:42
- Generate TypeScript types from SQL files using LIMIT 0 queries
- Add specific marker types (SQLDateMarker, SQLStringMarker, etc.)
- Parse @PARAM annotations in SQL files for parameter types
- Generate JSDoc hints showing which sql.*() helper to use
- Add intelligent caching to avoid redundant Databricks queries
- Integrate with useAnalyticsQuery for full type inference
@ditadi
ditadiforce-pushed the feat/type-safety-build-time branch from 0a07b6b to 2e680feCompareDecember 8, 2025 15:48
Comment threadpackages/app-kit/src/type-generator/query-registry.ts Outdated
@ditadi
ditadi requested a review from fjakobsDecember 8, 2025 18:15
// numeric
NUMERIC: "sql.number()",
INT: "sql.number()",
BIGINT: "sql.number()",

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.

not blocking: we should map BigInt to JS BigInt numbers

@ditadi
ditadi merged commit 58f73ad into mainDec 9, 2025
3 checks passed
@MarioCadenas
MarioCadenas deleted the feat/type-safety-build-time branch December 19, 2025 17:44
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>
pkosiec pushed a commit that referenced this pull request May 21, 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>
IamGalymzhan added a commit that referenced this pull request Aug 11, 2026
Verified and fixed the findings from an independent code review:
- #1 (correctness) expectStream dropped the wire `event:` name when the JSON
payload carried its own `type` (spread ran after the assignment). Spread the
payload first, then set `type = name ?? parsed.type`, so a frame like
`event: error` + `data: {"type":"result"}` reports `error`. Regression test added.
- #2 (contract) `@databricks/appkit/testing` eagerly loads vitest via fixtures
even for `expectStream`, so vitest is a real requirement. Drop the "optional"
peerDependenciesMeta and correct the docs sentence.
- #6 (OBO fidelity) the fake `asUser` recorded `asUser: true` unconditionally.
Enforce the real `Plugin.asUser` token precondition: a request without
`x-forwarded-access-token` throws `missingToken` (missing user id throws too),
and the resolved `userId` is recorded on each tool call. Tests now assert both
directions (well-formed request vs token-less).
- #3 (fidelity) attach() now mirrors AppKit core: registerPlugin plus
registerToolProvider for real tool providers, without clobbering injected
fakes. getPlugins()/getPluginNames()/hasPlugin() behave as in production.
- #12 unknown-tool lookup used `tools[name] === undefined`, so a tool named
"constructor"/"toString" hit Object.prototype. Use Object.hasOwn.
- #5 drop data-less named SSE frames (real clients ignore them).
- #7 re-export the PluginContext type from the testing barrel so
MockPluginContext.ctx is nameable through the exports map.
- #13 correct the docs: mock.telemetry captures the context's executeTool spans,
not plugin-level spans (attachContext rebuilds the plugin's own telemetry).
- #4 parseSSEResponse now delegates to the same parseSSEBody as expectStream —
one parser, no divergence. All 3 analytics.integration call sites still pass.
- #8 reformat template/server/example.test.ts with the template's Prettier so a
scaffolded app's `npm run format` passes.
- #10 fix the package-doc @example (agentsPlugin._handleStream does not exist).
- #11 add kit tests that exercise attach() end-to-end (cache seed, isReady,
registration, fake-not-clobbered).
Build passes attw + publint; full appkit suite 3125 passed / 1 pre-existing skip.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
IamGalymzhan added a commit that referenced this pull request Aug 20, 2026
* feat(appkit): make PluginContext telemetry injectable
The testing kit needs to construct a real PluginContext without a live
OpenTelemetry pipeline. Add an optional constructor dependency for the
telemetry provider, defaulting to the shared "plugin-context" provider so
the production path is unchanged. This is the single production edit
required to wrap the real class in tests rather than reimplementing it.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* feat(appkit): ship @databricks/appkit/testing and migrate first stub
Wire the testing kit as a published subpath and prove it against the first
of the two hand-rolled context stubs (the design gate):
- Add ./testing to both exports maps (dev + publishConfig) following the
./type-generator shape, add src/testing/index.ts to the tsdown entry, and
declare vitest as an optional peerDependency. Build passes attw + publint;
dist/testing/{index,mock-plugin-context,expect-stream,fixtures}.{js,d.ts}
are emitted and vitest stays external to the main entry.
- Migrate dispatch-tool-call.test.ts: replace (plugin as any).context =
{ executeTool } with mockPluginContext. executeTool is now the REAL method,
so the forwarded toolCallTimeoutMs is asserted through actual signal
composition, the on-behalf-of (asUser) path is verified, and a new test
proves the forwarded timeout actually aborts a slow toolkit tool end-to-end.
This is the primary win from the plan: executeTool's OBO and timeout paths
gain real assertions instead of a stub that proved nothing.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* test(appkit): migrate route-handler-errors context stub to mockPluginContext
Replace the second and final hand-rolled stub — (plugin as any).context =
{ addRoute } — with the real PluginContext from mockPluginContext. The kit's
route recorder captures raw handlers, so the alias assertion (both
/invocations and /responses mount the same handler reference) holds against
the real class, where forwardAsyncErrors wrapping would otherwise break
reference identity.
Both context stubs the plan identified are now migrated.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* docs(appkit): document the testing kit and ship a template example test
- Add docs/docs/development/testing.md covering mockPluginContext(),
expectStream(), and the fixture helpers, with a full end-to-end example.
Cross-links to local-development, custom-plugins, and execution-context.
- Add template/server/example.test.ts: a self-contained, plugin-agnostic
example that scaffolded apps ship with — it defines a tiny custom plugin
and exercises both mockPluginContext (route recording) and expectStream
(ordered event assertions), running with no workspace or network.
Ships the kit to users, satisfying the plan's acceptance criteria that a
docs page exists and the template carries at least one example test.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* docs(appkit): fix testing-kit examples to instantiate the plugin class
Validation by scaffolding a real app with `databricks apps init` surfaced
that the examples called the `analytics()`/`toPlugin()` factory and then
treated the result as a plugin instance — but a factory returns a
{ plugin, config, name } descriptor for createApp to construct, so
`.attachContext`/handler methods are absent.
Rewrite both the template example test and the docs "Full example" to
instantiate the plugin class directly (`new GreeterPlugin({})`), matching how
the migrated agents suites use the kit. The scaffolded app's `npm test` and
`tsc` both pass against the published `@databricks/appkit/testing` subpath
with no workspace or network.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* refactor(appkit): tighten FakeToolResponse so a missing value is a type error
Drop `undefined` from the static FakeToolValue union. `resolve()` treats an
undefined map entry as "unregistered tool" and throws, so allowing undefined
as a declared response made `{ query: undefined }` a confusing runtime error
instead of a compile error. A function returning undefined still works for the
rare "returns nothing" case. Add a test pinning that a null response is
returned as a value, not misread as a missing tool.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* refactor(appkit): make tools/test-helpers a shim over the shipped testing kit
The plan's step 5 was to MOVE the fixtures into the package, not copy them.
The shipped kit (src/testing/fixtures.ts) duplicated all 15 exports of
tools/test-helpers.ts, which would drift over time. Collapse the original
into a thin re-export of @databricks/appkit/testing so src/testing is the
single source of truth while the 18 existing @tools/test-helpers importers
keep working unchanged.
The re-exported mockServiceContext is now synchronous; every call site either
awaits it (no-op on a non-promise) or reads it through
Awaited<ReturnType<...>>, so all suites pass unchanged (full appkit suite:
3117 passed, 1 pre-existing skip).
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* fix(appkit): normalize CRLF in expectStream SSE parsing; sharpen testing docs
Code review follow-ups:
- expectStream's parseSSEBody split frames on \n\n, so a spec-compliant SSE
stream delimited by \r\n\r\n (from a real server) collapsed into one event.
AppKit's own writer uses \n\n so existing tests were unaffected, but
expectStream is public API that accepts any Response. Normalize CRLF to LF
before splitting; add a CRLF regression test.
- Docs: instantiate the plugin CLASS in the attach() snippet (the factory
returns a descriptor, not an instance), and note that the cache attach()
seeds is a per-process singleton shared by tests within a file.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* fix(appkit): resolve repo-wide Biome error blocking CI
CI's "Lint & Type Check" job runs `pnpm run check` over the whole repo, so a
pre-existing lint error unrelated to this branch failed the build:
- remote-tunnel-controller.test.ts had two `afterEach` hooks in one describe
(lint/suspicious/noDuplicateTestHooks, error severity). Merge them into one —
behavior preserved (env reset + console-spy clear both still run after each
test). This file is byte-identical to main; the error predated the branch and
only surfaced because CI lints the entire tree.
Also drop two dead `biome-ignore lint/suspicious/noExplicitAny` suppressions in
the testing kit (fixtures.ts, expect-stream.test.ts): `noExplicitAny` is turned
off repo-wide in biome.json, so the comments had no effect (suppressions/unused
warnings). The invalid-source test now casts through `unknown as never`.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* fix(appkit): address cross-model review findings in the testing kit
Verified and fixed the findings from an independent code review:
- #1 (correctness) expectStream dropped the wire `event:` name when the JSON
payload carried its own `type` (spread ran after the assignment). Spread the
payload first, then set `type = name ?? parsed.type`, so a frame like
`event: error` + `data: {"type":"result"}` reports `error`. Regression test added.
- #2 (contract) `@databricks/appkit/testing` eagerly loads vitest via fixtures
even for `expectStream`, so vitest is a real requirement. Drop the "optional"
peerDependenciesMeta and correct the docs sentence.
- #6 (OBO fidelity) the fake `asUser` recorded `asUser: true` unconditionally.
Enforce the real `Plugin.asUser` token precondition: a request without
`x-forwarded-access-token` throws `missingToken` (missing user id throws too),
and the resolved `userId` is recorded on each tool call. Tests now assert both
directions (well-formed request vs token-less).
- #3 (fidelity) attach() now mirrors AppKit core: registerPlugin plus
registerToolProvider for real tool providers, without clobbering injected
fakes. getPlugins()/getPluginNames()/hasPlugin() behave as in production.
- #12 unknown-tool lookup used `tools[name] === undefined`, so a tool named
"constructor"/"toString" hit Object.prototype. Use Object.hasOwn.
- #5 drop data-less named SSE frames (real clients ignore them).
- #7 re-export the PluginContext type from the testing barrel so
MockPluginContext.ctx is nameable through the exports map.
- #13 correct the docs: mock.telemetry captures the context's executeTool spans,
not plugin-level spans (attachContext rebuilds the plugin's own telemetry).
- #4 parseSSEResponse now delegates to the same parseSSEBody as expectStream —
one parser, no divergence. All 3 analytics.integration call sites still pass.
- #8 reformat template/server/example.test.ts with the template's Prettier so a
scaffolded app's `npm run format` passes.
- #10 fix the package-doc @example (agentsPlugin._handleStream does not exist).
- #11 add kit tests that exercise attach() end-to-end (cache seed, isReady,
registration, fake-not-clobbered).
Build passes attw + publint; full appkit suite 3125 passed / 1 pre-existing skip.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* chore(appkit): drop knip vitest-ignore now that vitest is a real peer dep
With vitest declared as a (non-optional) peerDependency, knip recognizes it as
used, so the earlier ignoreDependencies entry is unnecessary. This reverts
knip.json to its original state.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* fix(appkit): make vitest a normal dependency, not a package-wide peer
A required peerDependency has no per-subpath scope: it applied to the whole
@databricks/appkit package, so every production consumer that never imports
the testing kit got an unsatisfied peer (npm 7+ auto-installs vitest into
their tree; pnpm warns) — a wider blast radius than the eager-import bug it
was meant to fix.
Follow appkit's own precedent instead: `vite` backs the ./type-generator
subpath as a normal `dependency`, installed for everyone but loaded only by
importers of that subpath. Do the same for `vitest` and ./testing. vitest is
referenced solely by dist/testing/fixtures.js, never by the main/plugin/core
entry, so a consumer importing createApp never loads it.
Verified end-to-end: scaffolded an app whose own vitest (4.1.9) differs in
major from appkit's dependency (3.2.4), forcing a nested second copy. The
testing kit's vi.fn()/vi.spyOn() mocks and expect(...).toHaveBeenCalled()
assertions work across the two instances (vi spies carry their own call
state), and npm install emits no peer-dep warning. Build passes attw + publint.
Also fold in the template example's Prettier formatting (template uses Prettier,
not Biome) so a scaffolded app's `npm run format` passes.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* refactor(appkit): rename mockPluginContext to createTestPluginContext
The helper builds the REAL PluginContext with faked edges — it does not mock
the context — so the name was misleading. Rename to createTestPluginContext
(and the MockPluginContext type to TestPluginContext), matching the
create*-for-tests convention, and rename the files to test-plugin-context.ts.
Pre-merge and unreleased, so no external consumers are affected.
Also finish the #13 doc-accuracy fix in the shipped JSDoc (not just the docs
page): the telemetry field comment now states it captures the context's spans
(executeTool), not plugin-internal spans — attachContext rebuilds the plugin's
this.telemetry from the real TelemetryManager. These comments ship in
dist/testing/*.d.ts, so IntelliSense previously showed the unqualified claim.
Build passes attw + publint; full appkit suite 3125 passed / 1 pre-existing skip.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* refactor(appkit): dedupe testing fixtures and tidy test-plugin-context
Behavior-preserving cleanups in the testing kit:
- createMockRequest reuses createMockWorkspaceClient() instead of an inline
copy of the same mock client (verified identical).
- createMockServiceContext / createMockUserContext / mockServiceContext inline
the createMockWorkspaceClient() call into the `||` fallback, so the mock
client is built only when the caller did not supply one.
- The fake asUser view spreads `...base` and overrides executeAgentTool rather
than re-declaring getAgentTools.
- expectStream's isSubsequence breaks once the expected sequence is fully
matched.
No semantic change; typecheck clean and all kit + migrated tests pass.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* fix(appkit): resolve third-review findings in the testing kit
- #1 (P1) The docs called vitest a peer dependency, but the manifest ships it
under `dependencies` (the decision we landed on, matching how appkit ships
`vite` for ./type-generator). Correct the docs to match: appkit installs
vitest for you, and it loads only when you import ./testing. Manifest and
docs now agree.
- #2 (P2) expectStream buffered the source eagerly with no bound, so a
non-terminating stream hung until the runner's own timeout. Add an optional
`{ timeout }` that fails fast with a clear, kit-specific error; document it
and cover both directions with tests.
- #3 (P2) The fake asUser replicates asUser's token precondition but not the
real dev-mode `DEV_OBO_FALLBACK_KEY` OTel marker (a module-private telemetry
detail). Narrow the docs and JSDoc to say so and point users at the recorded
asUser/userId fields instead of isDevOboFallback().
Build passes attw + publint; full appkit suite 3141 passed / 1 pre-existing skip.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* test(appkit): dogfood the testing kit on analytics and genie plugins
Exercise @databricks/appkit/testing against real core plugins to validate it
beyond the two agent proof sites and produce usage references:
- analytics.kit.test.ts: cross-plugin executeTool via createTestPluginContext —
OBO identity (asUser/userId), token-precondition rejection, and per-call
timeout abort. Needs only the kit (no workspace/ServiceContext).
- genie.kit.test.ts: drives the real _handleSendMessage SSE stream and asserts
event order with expectStream(...).toEmit(...).
Both add genuinely new coverage (streamed SSE order + OBO dispatch identity were
untested). Full appkit suite 3145 passed / 1 pre-existing skip.
Developer-experience notes (kit wins + friction, e.g. createMockResponse doesn't
compose with expectStream) captured in internal/ for the milestone review.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* refactor(appkit): address testing-kit review feedback
Resolve the eight review comments on the testing kit:
- createMockResponse now captures written SSE bytes and exposes
sseResponse(); expectStream reads a captured mock response directly, so
streaming-route tests no longer need a hand-rolled bridge.
- Ship vitest as an optional peer dependency (+ devDependency) instead of a
plain runtime dependency, keeping the test framework out of production
installs and deduping to the app's own copy. Ignore it in knip.
- Add an obo option to createMockRequest so on-behalf-of tests set the
forwarded identity headers with one flag.
- Add resetTestCache() to clear the shared cache singleton between tests.
- Use the documented attach() instead of an any-cast in the agents
dispatch tests.
- Drop the unused createMockServiceContext/createMockUserContext builders
from the public surface; keep the service-context builder internal.
- Pin the previously untested edges: the Object.hasOwn tool-lookup guard,
the dev-mode asUser branch, and parseSSEBody's non-object data values.
- Add useServiceContextMock() to register the mock lifecycle in one line,
returning a live accessor.
Dogfood the new helpers in the analytics, genie, and serving suites, and
document them in the testing guide.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* docs(appkit): move the testing guide under Plugins
The testing kit is entirely plugin-scoped (createTestPluginContext,
attach(plugin), plugin route/tool/SSE assertions), and the page's own
cross-links already pointed into plugins/. Move it next to custom-plugins
and fix the relative links. Keep the heading as 'Testing'; the Plugins
section supplies the context.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* test(appkit): fold dogfood tests into plugin suites
Address round-2 review: the kit should be the default way to test a
plugin, not a parallel '*.kit.test.ts' track.
- Fold the three cross-plugin executeTool OBO tests into analytics.test.ts
and delete analytics.kit.test.ts.
- Upgrade genie.test.ts's SSE test to assert event ORDER via
expectStream on genie's real event names (message_start, status,
message_result, query_result), replacing brittle write.mock.calls
substring checks, and delete genie.kit.test.ts.
- Trim the heavy comment narration from the folded-in tests.
- Re-export createTestPluginContext and expectStream from the test-helpers
shim.
- Finish the testing-guide move under plugins/ (sidebar position + links).
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* test(appkit): restore toHaveLength(1) on the analytics OBO dispatch test
The dogfood fold trimmed expect(mock.toolCalls).toHaveLength(1), so a
double-dispatch would no longer fail the happy-path test — and it was
inconsistent with the token-less sibling that kept toHaveLength(0).
Restore it.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* test(appkit): re-assert genie SSE payloads after the expectStream swap
The toEmit swap pinned event order but dropped the payload values the old
substring checks covered (conversationId=new-conv-id, status=ASKING_AI),
which aren't asserted elsewhere. Restore them structurally via collect() +
toMatchObject — keeping the ordering guarantee without brittle substrings.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* fix(appkit): drop fabricated workspace-client fields from createMockRequest
createMockRequest returned userWorkspaceClient, serviceWorkspaceClient,
getWarehouseId and getWorkspaceId — fields no production code reads
(plugins resolve those through getWorkspaceClient()/getWarehouseId() from
src/context, which mockServiceContext stands in for). Publishing them via
@databricks/appkit/testing would make four inert fields a permanent public
promise.
The two warehouse cold-start tests (analytics + metric) overrode
mockReq.serviceWorkspaceClient.warehouses.get, which the route never reads
— so they passed on the default RUNNING client without exercising the
warehouse path at all. Route the warehouse client through
mockServiceContext (the real seam) so the tests are live, and drop the
'mock WorkspaceClient' claim from the testing guide.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* chore: drop the .claude ignores and restore the client lockfile
Both were swept into the oxlint merge from a dirty working tree and corrected
later on the branch; folding those corrections in here keeps them out of the
follow-up PR.
The knip `.claude/**` entry and the `**/.claude` ignorePatterns in oxfmt/oxlint
were never needed — nothing in the repo lints or formats that directory. The
`packages/appkit` vitest ignoreDependencies entry stays: vitest is a real
dependency of the testing entry.
apps/dev-playground/client/package-lock.json is restored to origin/main
byte-for-byte; npm had run in that directory and pruned its `extraneous: true`
entries.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
* fix(appkit): lowercase mock request header keys, as Express does
`createMockRequest` stored header keys exactly as given while `header()`
lowercased the lookup, so a mixed-case override was unreachable:
createMockRequest({ obo: { userId: "alice" }, headers: { "X-Forwarded-User": "bob" } });
// header("x-forwarded-user") === "alice"
Both keys were kept — ["x-forwarded-access-token", "x-forwarded-user",
"X-Forwarded-User"] — and the lowercase one obo seeded still answered, which
contradicted the "an explicit override wins" contract documented right above it.
Keys are now lowercased on the way in, matching what Node's parser hands
Express. Thanks @pkosiec.
The existing override test passed because it used a lowercase key, so it is now
parametrised over both casings, and the case-insensitivity test additionally
pins that every stored key is lowercase. Reverting the fix fails both.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
---------
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Sign up for freeto 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

@ditadi@fjakobs