Cherry-pick MCP fixes (#3508, #3544, #3425) to release/2.0#3606
Merged
anushakolan merged 4 commits intoMay 20, 2026
Merged
Conversation
## Why make this change? Two bugs were discovered in the CLI logging behavior when running DAB in MCP stdio mode: 1. **Missing colors**: When `start --mcp-stdio --LogLevel <level>` was used, the CLI's own startup logs (e.g., "Setting minimum LogLevel: Information.") appeared on stderr **without any colors**, while engine logs were properly colored. This created an inconsistent visual experience. 2. **Missing CLI logs with config-only log level**: When `start --mcp-stdio` was used with the `runtime.telemetry.log-level` set in the config file (and no `--LogLevel` CLI flag), CLI startup logs were **completely suppressed**, even though the user had clearly expressed an intent to see logs at that level. ## What is this change? The CLI logger (`CustomLoggerProvider`) only knew about CLI overrides (`--LogLevel`). It never learned that the runtime config file can also explicitly request a log level. As a result: - It defaulted to `LogLevel.None` (suppress all) whenever `--LogLevel` was absent in MCP mode, ignoring the config. - The MCP stderr code path also skipped the foreground/background color formatting that the non-MCP path applied. This change makes the CLI logger aware of both override sources and applies colors consistently in both stdout (non-MCP) and stderr (MCP) paths. ### Modified files: - `src/Cli/Utils.cs` — Added `IsLogLevelOverriddenByConfig` (bool) and `ConfigLogLevel` (LogLevel) static properties. - `src/Cli/ConfigGenerator.cs` — In `TryStartEngineWithOptions()`, sets the new properties when `RuntimeConfig.HasExplicitLogLevel()` is true. - `src/Cli/CustomLoggerProvider.cs`: - Constructor: chooses minimum log level from CLI → Config → None. - `Log()`: suppresses only when **neither** CLI nor config set a level; applies foreground/background colors before writing the abbreviation to stderr in MCP mode. ## How was this tested? Manually tested all combinations of MCP / non-MCP × CLI override / config override / no override: | # | Scenario | Expected | Result | |---|----------|----------|--------| | 1 | MCP, no flags, no config log-level | No CLI logs (clean stdout) | ✅ | | 2 | MCP + `--LogLevel Information` | CLI logs on stderr with **green** `info:` | ✅ | | 3 | MCP + config `log-level` only | CLI logs on stderr with **green** `info:` | ✅ Bug 2 fixed | | 4 | MCP + `--LogLevel Warning` + config `log-level: Information` | CLI wins → no `info:` shown | ✅ | | 5 | Non-MCP mode | Unchanged behavior | ✅ | | 6 | MCP + `--LogLevel Warning` (StaticWebApps config) | **Yellow** `warn:` shown | ✅ Bug 1 fixed | ## Note This is a follow-up bug fix on top of PR #3419 (MCP Set Log Level) and PR #3484 (MCP notifications/message). It targets `main`. --------- Co-authored-by: Aniruddh Munde <anmunde@microsoft.com> (cherry picked from commit 6f986c7)
…Config) (#3544) ## Why make this change? Closes #3533— *Log-level precedence should be Agent > CLI > Config; MCP `logging/setLevel` is silently ignored when CLI/Config set a level.* The runtime today implements **CLI > Config > Agent**: once `--LogLevel` is passed or `runtime.telemetry.log-level` is set, the agent's request silently no-ops. There is no way for an agent to dial verbosity up or down at runtime, which defeats the purpose of MCP `logging/setLevel`. The team locked the corrected precedence on 5/1 : **Agent > CLI > Config > defaults**. The agent must always be able to override; CLI still beats Config when the agent doesn't speak up; defaults (Production = Error, Development = Debug, `--mcp-stdio` = None) are unchanged. ## What is this change? [`DynamicLogLevelProvider`](src/Service/Telemetry/DynamicLogLevelProvider.cs) is the single chokepoint for all log-level updates. The fix: - `UpdateFromMcp` no longer rejects when CLI or Config has set a level. It updates the level, flips a new `IsAgentOverriding` flag, and emits an `Information` audit log ("Log level updated to {Level} via MCP logging/setLevel (agent override)."). - `UpdateFromRuntimeConfig` is guarded so a config hot-reload won't overwrite an Agent- or CLI-set level. - [`ILogLevelController`](src/Core/Telemetry/ILogLevelController.cs) gains `IsAgentOverriding` and updated XML docs. - [`Startup.cs`](src/Service/Startup.cs) wires a logger on the provider once `ILoggerFactory` exists, so the audit log reaches the standard pipeline. - [`McpStdioServer.HandleSetLevel`](src/Azure.DataApiBuilder.Mcp/Core/McpStdioServer.cs) enables the MCP notification writer **before** calling `UpdateFromMcp` so the audit log surfaces to the agent. CLI > Config, default levels, and the `--mcp-stdio` default of `None` are unchanged. ## How was this tested? - [ ] Integration Tests - [x] Unit Tests - [x] Manual end-to-end test against live SQL Server with the MCP Inspector ### Unit tests Added 1 new test and updated existing ones in [`DynamicLogLevelProviderTests`](src/Service.Tests/UnitTests/DynamicLogLevelProviderTests.cs) to lock Agent > CLI > Config under both single-call and concurrent paths. All 9 tests pass. ### Manual end-to-end test Started the engine with `--mcp-stdio --LogLevel Error` and drove it from the MCP Inspector against a live SQL Server. | # | What | How | Result | | --- | --- | --- | --- | | 1 | Agent sets level to debug | Send `logging/setLevel { level: "debug" }` | Success response ✅ | | 2 | Audit log surfaces | Watch MCP notifications | Info notification with override message received ✅ | | 3 | Debug logs flow despite `--LogLevel Error` | Trigger activity (e.g. `tools/list`) | Debug notifications received ✅ | Also spot-checked HTTP mode: Config-only, CLI-over-Config, and Config-alone all behave as before. ## Sample Request(s) ### MCP `logging/setLevel` from the agent Request: ```json { "jsonrpc": "2.0", "id": 2, "method": "logging/setLevel", "params": { "level": "debug" } } ``` Response: ```json { "jsonrpc": "2.0", "id": 2, "result": {} } ``` Followed by the new visibility notification: ```json { "jsonrpc": "2.0", "method": "notifications/message", "params": { "level": "info", "logger": "Azure.DataApiBuilder.Service.Telemetry.DynamicLogLevelProvider", "data": "Log level updated to Debug via MCP logging/setLevel (agent override)." } } ``` ### CLI usage (precedence still works as expected) ```bash # Agent can override even when CLI pins the level: dab start --mcp-stdio --LogLevel Error # → agent sends logging/setLevel debug → debug logs flow # CLI still beats Config when no agent is involved: dab start --LogLevel Warning --config dab-config.json # config has log-level: Information # → only Warning and above appear # Config alone, no CLI, no agent — unchanged: dab start --config dab-config.json # config has log-level: Information # → info: lines visible, debug: filtered out ``` (cherry picked from commit fcb09b8)
…se. (#3425) ## Why make this change? Closes #3400. `describe_entities` could return stored-procedure entities with empty or partial `parameters` metadata when parameters were not fully listed in config. MCP clients/agents then missed required inputs and `execute_entity` calls failed. ## What is this change? `describe_entities` now uses the runtime database metadata as the source of truth for stored-procedure parameter discovery, with config acting as an overlay on top. Per the rules locked in #3400: - **Names** come from the database. Config never overrides parameter names. - **`required`** defaults to `true`. Config overrides per-parameter when the user explicitly supplies it. To make this round-trip cleanly through JSON deserialization, `ParameterMetadata.Required` is now `bool?` so "user omitted the key" is distinct from "user wrote `required: false`". The global `JsonSerializer` policy `DefaultIgnoreCondition.WhenWritingNull` keeps generated configs clean. - **`default`** is config-only. T-SQL doesn't expose parameter defaults via metadata for ordinary stored procedures, so the DB-side value is unreliable. - **`description`** is config-only. There is no DB column carrying per-parameter descriptions. - Parameters discovered from the database that are not listed in config are still surfaced (with `required: true`, no default, no description) so agents can see and call them. The merge itself is already performed upstream in `SqlMetadataProvider.FillSchemaForStoredProcedureAsync`, which populates `DatabaseStoredProcedure.StoredProcedureDefinition.Parameters` with config values laid over the introspected DB schema. `DescribeEntitiesTool.BuildParameterMetadataInfo` projects that already-merged structure directly, instead of carrying its own parallel merge. If the upstream merge invariant is ever violated (i.e. an SP entity reaches `describe_entities` without populated DB metadata), the tool throws inside the per-entity `try` and the surrounding `catch` drops just that entity from the response. Returning the SP with `parameters: []` would be indistinguishable from a real zero-parameter SP and would mislead the agent into calling the SP with no arguments. A config that declares an SP parameter the DB does not have continues to fail loudly at `InitializeAsync` (via `HandleOrRecordException`), so `describe_entities` never runs against a half-merged state in production. ## How was this tested? - [x] Unit tests (projection logic, with the merged `ParameterDefinition` dictionary supplied directly) - [x] Integration tests against a live SQL Server (real `MsSqlMetadataProvider`, real merge, real DB introspection) - [x] Manual stdio MCP testing ### Unit tests — `DescribeEntitiesStoredProcedureParametersTests` These tests pass a hand-built `Dictionary<string, ParameterDefinition>` representing the post-merge state and verify the projection without touching a database. | Test | What it locks in | | --- | --- | | `MixedConfiguredAndUnconfiguredParameters_ProjectsBoth` | A parameter with config overrides and one without are projected side-by-side: the overridden param reflects config; the non-overridden one defaults to `required=true`, `default=null`, `description=""`. | | `ExplicitRequiredFalse_IsHonored` (`WithDefault` / `WithoutDefault`) | An explicit `required=false` written onto the `ParameterDefinition` by the upstream merge is honored both with and without a `Default` value. | | `DropsEntity_WhenStoredProcedureMetadataIsMissing` | If the upstream invariant ever regresses and an SP entity has no `DatabaseStoredProcedure`, the tool drops that entity from the response rather than returning it with `parameters=[]`. When it's the only entity, the tool returns the `NoEntitiesConfigured` error result. | ### Integration tests — `DescribeEntitiesStoredProcedureParametersMsSqlIntegrationTests` (`TestCategory.MSSQL`) Extend `SqlTestBase`, so they bootstrap the real `MsSqlMetadataProvider` against the live test database via `dab-config.MsSql.json` and `DatabaseSchema-MsSql.sql`. These exercise the actual config-onto-DB merge that the unit tests cannot. | Test | SP / config | What it locks in | | --- | --- | --- | | `StoredProcedureWithNoDbOrConfigParameters_EmitsEmptyParametersArray` | `GetBooks` → `get_books` (0 DB params, no config overrides) | A truly zero-parameter SP emits `parameters: []`. | | `DbParameterWithoutConfigOverride_DefaultsToRequiredTrue` | `GetBook` → `get_book_by_id @id int` (no config override) | A DB-discovered parameter with no config entry comes out `required=true, default=null, description=""`. | | `DbParametersWithConfigOverrides_MergesConfigValuesOntoDbParameters` | `InsertBook` → `insert_book @title, @publisher_id` (config sets both `required=false` with defaults) | Config-side `required`/`default` flow onto DB-discovered params end-to-end. | | `PartialConfigOverride_MergesOverriddenParamAndDefaultsTheOther` | `update_book_title @id, @title` (config overrides `id` only) | Mixed merge against a real SP: the overridden param reflects config; the non-overridden one falls back to the merge defaults. | | `SqlMetadataProvider_FailsAtInit_WhenConfigDeclaresParameterNotInDb` | `get_book_by_id` with a bogus extra param in config | Locks in the upstream invariant `DescribeEntitiesTool` depends on: a config that declares a parameter the DB does not have throws `DataApiBuilderException` at `InitializeAsync` (mentioning both the bogus param name and the SP name), so the tool never reaches a half-merged state in production. | ### Manual end-to-end test Validated against a live SQL Server over the MCP stdio transport. Each test entity is backed by the same two-parameter stored-procedure shape — `(@id INT, @Locale NVARCHAR(10) = 'en-US')` — and each entity declares its parameters differently in `dab-config.json` so we can verify how config and DB metadata combine. The custom-tool entity is backed by a separate two-parameter SP that takes a search title and a max-row count. <details> <summary>Stored procedure DDL</summary> ```sql -- Each test entity gets its own SP with the same shape so config on one -- entity does not pollute the shared parameter metadata of another. CREATE PROCEDURE dbo.get_book_full @id INT, @Locale NVARCHAR(10) = 'en-US' AS ... CREATE PROCEDURE dbo.get_book_partial @id INT, @Locale NVARCHAR(10) = 'en-US' AS ... CREATE PROCEDURE dbo.get_book_empty @id INT, @Locale NVARCHAR(10) = 'en-US' AS ... CREATE PROCEDURE dbo.search_books @title NVARCHAR(200), @max_results INT AS ... ``` </details> <details> <summary>Entities in dab-config.json</summary> ```jsonc "BookFull": { "source": { "object": "dbo.get_book_full", "type": "stored-procedure", "parameters": [ { "name": "id", "required": true, "default": "42", "description": "Book id from config" }, { "name": "locale", "required": false, "default": "fr-FR", "description": "Locale from config" } ] }, "permissions": [{ "role": "anonymous", "actions": [{ "action": "execute" }] }] }, "BookPartial": { "source": { "object": "dbo.get_book_partial", "type": "stored-procedure", "parameters": { "id": 1 } // old object form; no `required`, locale omitted }, "permissions": [{ "role": "anonymous", "actions": [{ "action": "execute" }] }] }, "BookEmpty": { "source": { "object": "dbo.get_book_empty", "type": "stored-procedure" // no parameters block at all }, "permissions": [{ "role": "anonymous", "actions": [{ "action": "execute" }] }] }, "SearchBooks": { "source": { "object": "dbo.search_books", "type": "stored-procedure", "parameters": [ { "name": "title", "required": true, "description": "Substring to search for in book title" }, { "name": "max_results", "required": true, "description": "Maximum rows to return" } ] }, "mcp": { "custom-tool": true }, "permissions": [{ "role": "anonymous", "actions": [{ "action": "execute" }] }] } ``` </details> **T1 — `tools/call describe_entities`** (verifies the metadata returned to MCP clients) For each entity below, the columns show what the tool reported for each parameter. The "What this verifies" column explains why this row matters. | Entity (config shape) | Param | required | default | description | What this verifies | | --- | --- | --- | --- | --- | --- | | `BookFull` — config sets `required`, `default`, `description` for both params | `id` | true | `"42"` | `"Book id from config"` | Explicit per-param config flows through unchanged. | | `BookFull` | `locale` | false | `"fr-FR"` | `"Locale from config"` | Explicit `required: false` from the user is still honored (rule 2: config overrides default). | | `BookPartial` — config uses old object form `"parameters": { "id": 1 }` (no `required` key, `locale` not mentioned) | `id` | true | `"1"` | `""` | Old-format config supplies the default; absent `required` defaults to **true** (the fix). | | `BookPartial` | `locale` | true | `null` | `""` | Param is in the SP but not in config — still surfaced from DB metadata, defaults to **required: true** (the fix; rule 5). | | `BookEmpty` — no `parameters` block in config at all | `id` | true | `null` | `""` | Both params discovered purely from DB metadata, defaulting to required (the fix; rule 5). | | `BookEmpty` | `locale` | true | `null` | `""` | Same as above. | | `SearchBooks` — `mcp.custom-tool: true`; config marks both params `required: true` with descriptions | `title` | true | `null` | `"Substring to search for in book title"` | Entities marked as custom tools still appear in `describe_entities` with the same merge rules applied. | | `SearchBooks` | `max_results` | true | `null` | `"Maximum rows to return"` | Same as above. | **T2 — `tools/call execute_entity`** for `BookEmpty` (no params declared in config) with arguments `{ "entity": "BookEmpty", "parameters": { "id": 2, "locale": "fr-FR" } }` → `status: success`, returns the row for id=2 (`"Le Petit Prince"`). Confirms an agent that follows the `describe_entities` output can invoke the SP successfully even when the config declares zero parameters. **T3 — `tools/call execute_entity`** for `BookPartial` with no `parameters` argument → `status: success`, returns the row `{ id: 1, title: "The Pragmatic Programmer", locale: "en-US" }`. The returned `id=1` matches the config-supplied default `"1"`, and `locale="en-US"` matches the T-SQL `DEFAULT 'en-US'` on `@locale`. Confirms that omitted parameters fall back to whichever default is available. **T4 — `tools/call search_books`** (the custom-tool surface for `SearchBooks`) with `{ "title": "Pr", "max_results": 5 }` → `status: success`, returns 2 rows whose titles contain "Pr": `"The Pragmatic Programmer"` and `"Le Petit Prince"`. Confirms a stored procedure exposed as a top-level MCP custom tool can be invoked end-to-end with the new `Required` semantics. (cherry picked from commit 73e6b14)
anushakolan
requested review from
RubenCerna2079 and
souvikghosh04
as code owners
anushakolan
requested review from
Alekhya-Polavarapu,
rusamant,
sourabh1007,
stuartpa and
vadeveka
as code owners
anushakolan
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Backports three MCP-related fixes from main to release/2.0: improved CLI logging in MCP stdio mode (colors + config log-level), agent-over-CLI-over-config precedence for log levels, and corrected stored-procedure parameter projection in describe_entities.
Changes:
- Apply ANSI color formatting to CLI startup logs in MCP stdio mode and honor
runtime.telemetry.log-levelfrom config when--LogLevelis absent. - Establish Agent > CLI > Config log-level precedence with proper locking in
DynamicLogLevelProvider, renamingIsCliOverridden/IsConfigOverriddentoIsCliOverriding/IsConfigOverridingand addingIsAgentOverriding. describe_entitiesnow uses runtime DB metadata as source of truth for stored-procedure parameters, with config treated as an overlay.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Telemetry/DynamicLogLevelProvider.cs | New precedence logic with thread-safe state mutations and agent override flag. |
| src/Service/Startup.cs | Renames CLI override flag and wires audit logger on DynamicLogLevelProvider. |
| src/Service/Program.cs | Renames CLI/config override out-parameters consistently. |
| src/Service.Tests/UnitTests/DynamicLogLevelProviderTests.cs | Updated and added tests covering agent-always-wins semantics and hot-reload precedence. |
| src/Service.Tests/Mcp/DescribeEntitiesStoredProcedureParametersTests.cs | New unit tests for SP parameter projection from DB metadata. |
| src/Service.Tests/Mcp/DescribeEntitiesStoredProcedureParametersMsSqlIntegrationTests.cs | New MSSQL integration tests exercising the DB+config merge end-to-end. |
| src/Service.Tests/Mcp/DescribeEntitiesFilteringTests.cs | Adds stub metadata provider to satisfy the new SP init invariant. |
| src/Core/Telemetry/ILogLevelController.cs | Adds IsAgentOverriding and renames flags; updates precedence docs. |
| src/Config/ObjectModel/ParameterMetadata.cs | Makes Required nullable so "not set in config" is distinguishable. |
| src/Config/DatabasePrimitives/DatabaseObject.cs | Drops default false on ParameterDefinition.Required for tri-state semantics. |
| src/Cli/Utils.cs | Adds IsConfigOverriding/ConfigLogLevel statics; renames CLI override flag. |
| src/Cli/Program.cs | Renames CLI override flag. |
| src/Cli/CustomLoggerProvider.cs | Applies colors to MCP stderr output, honors config log-level, uses try/finally to restore colors. |
| src/Cli/ConfigGenerator.cs | Preserves matched Required on parameter updates; sets config override flags at startup. |
| src/Cli.Tests/Snapshots/*.verified.txt | Snapshot updates reflecting omitted Required: false. |
| src/Cli.Tests/EndToEndTests.cs | Updates references to renamed flag. |
| src/Cli.Tests/CustomLoggerTests.cs | Expands tests for MCP CLI/config override paths. |
| src/Azure.DataApiBuilder.Mcp/Utils/McpMetadataHelper.cs | Adds TryResolveDatabaseObject convenience wrapper and softens factory resolution. |
| src/Azure.DataApiBuilder.Mcp/Core/McpStdioServer.cs | Reorders setLevel side-effects so the agent-override log line is forwarded. |
| src/Azure.DataApiBuilder.Mcp/BuiltInTools/DescribeEntitiesTool.cs | Projects SP parameters from DatabaseStoredProcedure rather than config-only list. |
Aniruddh25
approved these changes
May 19, 2026
Aniruddh25
approved these changes
May 20, 2026
aaronburtle
approved these changes
May 20, 2026
anushakolan
deleted the
dev/anushakolan/cherry-pick-mcp-fixes-to-release-2-0
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why make this change?
Backport three MCP-related fixes from
maintorelease/2.0.6f986c7a13aba640fcb09b8f170940c573e6b14f1ad22062describe_entitiesresponseWhat is this change?
1. #3508 — Fix MCP CLI logging: apply colors and respect config log-level
Two CLI logging bugs in MCP stdio mode:
dab start --mcp-stdio --LogLevel <level>was logging without ANSI color codes that the engine logger uses, making CLI output visually inconsistent with engine output. Color formatting is now applied to CLI startup logs.--LogLevelwas omitted, the CLI ignored theruntime.telemetry.log-levelvalue fromdab-config.jsonand fell back to the hard-coded default. The CLI now reads the config'slog-leveland uses it as the effective minimum log level.2. #3544 — Agent > CLI > Config log-level precedence
Defines and enforces the three-tier log-level precedence for MCP stdio sessions:
mcp/setLevelrequest from an MCP agent at runtime.--LogLevelpassed todab start.runtime.telemetry.log-levelindab-config.json.3. #3425 —
describe_entitiesreturns full stored-procedure parametersCloses #3400.
describe_entitieswas returning stored-procedure entities with empty or partialparametersmetadata when parameters were not fully listed in config. MCP agents then missed required inputs andexecute_entitycalls failed.describe_entitiesnow uses the runtime database metadata as the source of truth and treats config as an overlay on top..
How was this tested?
All the commits were individually tested in their respective PRs.