[MCP] Server Features in MCP configuration: API Tools, Dynamic Tool Mode, Data Query Tools (Preview)

[onbuyuka]

Summary

Adds a Server Features surface to the MCP configuration card so admins can opt configurations into the Data Query Tools (Preview) MCP runtime, alongside the existing Dynamic Tool Mode and API Tools controls. Each feature is a self-contained handler behind an interface, so the card is data-driven and extensible. Activation persists on the real MCP Configuration fields shipped in BC-Platform #44811; only the Data Query Tools server runtime lands separately (see Platform dependency).

Feature handlers (interface-based)

• New interface "MCP Server Features" — SetActive / IsActive / HasSettings / OpenSettings / Description / LoadSystemTools / TryGetParentFeature. enum "MCP Server Feature" implements it, binding each value to a handler codeunit; resolving ServerFeature := Rec.Feature replaces the old per-feature case blocks.
• Both the MCP Server Feature List and MCP System Tool List pages are fully generic: each walks ServerFeature.Ordinals() + FromInteger(...) (the same pattern as MCP Config Warning Type) and dispatches every action — activation, settings, descriptions, and system-tool loading — through the handler. Adding a feature is a new enum value + handler codeunit — no page edits.
• Each feature owns its description, activation, settings, system tools, and rules; SetActive delegates to codeunit "MCP Config Implementation", which reads/writes the real MCP Configuration fields (EnableDynamicToolMode, EnableApiTools, EnableAlQueryTools); IsActive reads them back.

Card UX

• Server Features list-part with per-row Activate / Deactivate / Configure, mirroring page 7775 "Copilot AI Capabilities" → Copilot Capabilities GA. Configure appears only for features that have settings — gated on a per-row Configurable flag set from the handler's HasSettings(). Dynamic Tool Mode renders indented as a sub-feature of API Tools (TryGetParentFeature + an Indentation column on the temp table).
  • API Tools — gates the Available APIs list-part (Visible = not IsDefault and APIToolsActive), where the admin curates which API pages / queries the client can reach. No settings.
  • Dynamic Tool Mode — its Configure dialog hosts Discover Read-Only Objects; activation flows to EnableDynamicToolMode, deactivation cascade-clears DiscoverReadOnlyObjects. Enabling it requires API Tools to be enabled first — impl.EnableDynamicToolMode enforces the gate (APIToolsRequiredForDynamicErr).
  • Data Query Tools (Preview) — contributes the Data Query Tools system tools when active. No settings.
• Available APIs (page 8352) — the curated API list. Its Select APIs action opens a single lookup listing both API pages and API queries (new temp table "MCP API Object Buffer" + page "MCP API Object Lookup", populated by impl.LookupAPIObjects); the per-row Object Id lookup shares the same path. The part was renamed 'Available Tools' → 'API Tools' → 'Available APIs' to disambiguate it from the API Tools feature row.
• The single settings StandardDialog (page 8369 "MCP Server Feature Settings") is used only by Dynamic Tool Mode; its handler runs it via OpenSettings, writing on OK.
• The System Tools list-part (right-rail FactBox) is rebuilt by iterating the active features and calling each handler's LoadSystemTools.

Configuration list (page 8350)

• The Default field is read-only on the card; Set as Default / Clear Default actions (promoted) live on the configuration list instead.
• Read-only API Tools / Data Query Tools status columns (bound to Rec.EnableApiTools / Rec.EnableAlQueryTools) replaced the previous Dynamic Tool Mode / Discover Read-Only Objects columns.

Public API

• codeunit 8350 "MCP Config" exposes EnableAPITools and EnableDataQueryTools alongside the existing EnableDynamicToolMode / EnableDiscoverReadOnlyObjects. The two feature procedures read/write the MCP Configuration.EnableApiTools / .EnableAlQueryTools fields.

New objects

• interface "MCP Server Features"
• enum 8351 "MCP Server Feature" (implements "MCP Server Features"; API Tools, Dynamic Tool Mode, Data Query Tools)
• table 8355 "MCP Server Feature" (temporary; carries Configurable + Indentation)
• table 8357 "MCP API Object Buffer" (temporary; unifies API pages + queries for the Select APIs lookup)
• page 8368 "MCP Server Feature List", page 8369 "MCP Server Feature Settings", page 8376 "MCP API Object Lookup"
• handler codeunits 8369 "MCP API Tools Feature", 8368 "MCP Data Query Tools Feature", 8370 "MCP Dyn. Tool Mode Feature"

Deprecated

• Pages 8353 "MCP API Config Tool Lookup" / 8367 "MCP Query Config Tool Lookup" are obsoleted (ObsoleteState = Pending, ObsoleteTag = '29.0', wrapped in #if not CLEAN29) — superseded by the unified MCP API Object Lookup (8376). They're public System Application objects, so they're deprecated rather than deleted (removing them breaks dependent extensions — AS0029). Their internal MCP Config Implementation lookup procedures (LookupAPIPageTools / LookupAPIQueryTools) and the test-library wrappers were removed (neither is public surface).

Permissions

• MCP - Objects lists the new app tables (MCP Server Feature, MCP API Object Buffer).

Platform dependency

BC-Platform PR #44811 has merged, and this PR is productionized against it — no mocks remain (grep -rn "MOCK:|PLATFORM-PENDING" "src/System Application/App/MCP/" is empty).

• Activation persists on MCP Configuration.EnableApiTools (field 8, InitValue = true) / .EnableAlQueryTools (field 9), read/written through four MCP Config Implementation procedures. An upgrade (MCP Upgrade.EnableApiToolsOnExistingConfigurations) turns API Tools on for existing configurations (InitValue only covers new ones).
• Data Query Tools system tools come from "MCP Utilities".GetSystemToolsInDataQuery().

The platform uses the "AL Query Tools" field name; our UX stays "Data Query Tools" (it maps onto EnableAlQueryTools).

The Data Query Tools server runtime (compile + execute of client-submitted AL queries) is a separate, larger platform dependency — this PR only configures and advertises the tools. See Appendix E in docs/features/al-query-mcp-config-ui/design.md.

Test plan

• MCP Config Test (codeunit 130130): activation tests asserting the real EnableApiTools / EnableAlQueryTools field state; a Server Features TestPage set on page 8351 "MCP Config Card" (the list shows all three features in order, Configure is enabled only on Dynamic Tool Mode, activating Dynamic Tool Mode flips its row Status and sets EnableDynamicToolMode, and the part is hidden on the default configuration; API Tools is enabled before Dynamic Tool Mode so the gate is satisfied); TestLookupAPIObjects exercising the unified API lookup; and Export/Import round-trip tests asserting the two new fields survive.
• Manual walkthrough of the Server Features list / Configure / Activate-Deactivate cycle and the Select APIs lookup against a local BC instance.

Fixes AB#631012

🤖 Generated with Claude Code

[github-actions]

AL Documentation Audit

Documentation gaps were detected in the following apps:

• MCP-Test: 0% documentation coverage
• MCP: 67% documentation coverage

To generate documentation, run /al-docs init or /al-docs update using GitHub Copilot CLI or Claude Code.
This review is for awareness to help keep documentation in sync with code changes. It is okay to dismiss this request.

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Clustered PK change on non-temporary table needs upgrade handling

The clustered primary key of MCP System Tool was changed from ("Tool Name") to ("Server Feature", "Tool Name"). Changing the clustered key of a persisted table (not TableType = Temporary) triggers a full table rebuild during schema sync. While BC handles this automatically, any existing rows with a null "Server Feature" value will land in the 0 (API Tools) bucket by default, which may not accurately reflect their actual origin if the table had pre-existing data.

Recommendation:

• Verify that MCP System Tool contains only ephemeral/derived data (reloaded at runtime) and therefore always starts empty; if so, document that assumption with a comment. If rows can be persisted across sessions, add an upgrade step to set "Server Feature" correctly for existing rows.

// In MCPUpgrade.EnableApiToolsOnExistingConfigurations or a separate step:
// MCPSystemTool.ModifyAll("Server Feature", "MCP Server Feature"::"API Tools");

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

No test for Dynamic Tool Mode guard when API Tools disabled

A new guard was added to EnableDynamicToolMode that raises APIToolsRequiredForDynamicErr when API Tools is disabled. There is no asserterror test that verifies this guard fires correctly; the only test that touches the path (TestEnableDynamicToolMode) now pre-enables API Tools, so the error branch is completely untested.

Recommendation:

• Add a test that calls MCPConfig.EnableDynamicToolMode(ConfigId, true) without first enabling API Tools and asserts the expected error.

[Test]
procedure TestEnableDynamicToolModeWithoutAPIToolsFails()
var
    ConfigId: Guid;
begin
    ConfigId := CreateMCPConfig(false, false, true, false);
    asserterror MCPConfig.EnableDynamicToolMode(ConfigId, true);
    Assert.ExpectedError('API Tools must be enabled');
end;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Import can create inconsistent feature state

The JSON import path sets EnableApiTools and EnableAlQueryTools directly on the MCPConfiguration record before Insert(), bypassing all business-rule guards. A crafted (or migrated) JSON file with enableDynamicToolMode: true and enableApiTools: false will create a persisted configuration where Dynamic Tool Mode is enabled but API Tools is off — a state that the API-layer guard in EnableDynamicToolMode explicitly prevents at runtime.

Recommendation:

• After deserialising the JSON fields, enforce the dependency by calling the guarded procedures (EnableAPITools, EnableDynamicToolMode) rather than writing directly to the record fields, or add an explicit post-insert validation step that checks consistency.

// After Insert(), enforce dependencies:
if MCPConfiguration.EnableDynamicToolMode and not MCPConfiguration.EnableApiTools then begin
    MCPConfiguration.EnableDynamicToolMode := false;
    MCPConfiguration.DiscoverReadOnlyObjects := false;
    MCPConfiguration.Modify();
end;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Upgrade ModifyAll bypasses audit telemetry

MCPConfiguration.ModifyAll(EnableApiTools, true) bulk-updates every configuration record without calling LogConfigurationModified, so the change is invisible to the telemetry pipeline and audit log. Every user-driven toggle of API Tools is individually audited, but this upgrade silently changes potentially hundreds of production configurations with no trace.

Recommendation:

• Iterate over the configurations and call EnableAPITools (or log via LogConfigurationModified) for each record, or at minimum emit a single telemetry event summarising how many records were updated.

Suggested change

	MCPConfiguration.ModifyAll(EnableApiTools, true);
	if MCPConfiguration.FindSet(true) then
	repeat
	if not MCPConfiguration.EnableApiTools then begin
	MCPConfiguration.EnableApiTools := true;
	MCPConfiguration.Modify();
	end;
	until MCPConfiguration.Next() = 0;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

GetSelectedObjects silently drops insert failures

if MCPAPIObjectBuffer.Insert() then; swallows all insert errors without feedback. If the caller passes a pre-populated buffer (or the same object is selected twice in multi-select), the silent discard makes it impossible to distinguish between 'nothing was selected' and 'all inserts failed due to duplicates', and the return value of LookupAPIObjects will be misleadingly true.

Recommendation:

• Either clear the output buffer before populating it (so duplicate keys cannot arise) or replace the silent Insert() with a plain Insert() call that will surface conflicts explicitly.

Suggested change

	internal procedure GetSelectedObjects(var MCPAPIObjectBuffer: Record "MCP API Object Buffer")
	internal procedure GetSelectedObjects(var MCPAPIObjectBuffer: Record "MCP API Object Buffer")
	begin
	MCPAPIObjectBuffer.Reset();
	MCPAPIObjectBuffer.DeleteAll();
	CurrPage.SetSelectionFilter(Rec);
	if Rec.FindSet() then
	repeat
	MCPAPIObjectBuffer := Rec;
	MCPAPIObjectBuffer.Insert();
	until Rec.Next() = 0;
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Default field read-only with no card-level actions

The Default field was changed to Editable = false and its tooltip directs users to 'Set as Default and Clear Default actions on the configuration list'. However, no such actions were added to the card page. Users who navigate directly to a configuration card — whether via a URL, a link from a related page, or the card's own navigation — have no way to change the default designation from that context.

Recommendation:

• Add SetAsDefault and ClearDefault actions to the card page (mirroring those added to the list page), promoting them to the Actions bar so the experience is consistent regardless of entry point.

Suggested change

	MCPConfigImplementation.SetAsDefaultConfiguration(Rec.SystemId)
	action(SetAsDefault)
	{
	Caption = 'Set as Default';
	ToolTip = 'Set this configuration as the default.';
	Image = Approve;
	Enabled = not Rec.Default and not IsDefault;
	trigger OnAction()
	begin
	MCPConfigImplementation.SetAsDefaultConfiguration(Rec.SystemId);
	IsDefault := false;
	CurrPage.Update(false);
	end;
	}

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

SaveChanges case silently no-ops for new features

SaveChanges contains only a case Feature of Feature::"Dynamic Tool Mode" branch. If any future feature sets HasSettings() = true and OpenSettings is invoked, the user sees a dialog, can make changes, clicks OK — and SaveChanges silently discards everything without error, because no branch matches the new feature.

Recommendation:

• Add an else branch to the case statement that raises an error so that missing implementations are caught during development rather than silently failing in production.

Suggested change

	internal procedure SaveChanges()
	case Feature of
	Feature::"Dynamic Tool Mode":
	MCPConfigImplementation.EnableDiscoverReadOnlyObjects(ConfigSystemId, DiscoverReadOnlyObjectsLocal);
	else
	Error('SaveChanges is not implemented for feature %1.', Feature);
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Removed OnOpenPage auto-load breaks embedders

The OnOpenPage trigger that automatically loaded system tools was deleted. The page now requires callers to invoke the new Reload(ConfigSystemId) procedure explicitly. Any existing or future code that embeds MCP System Tool List as a part without calling Reload will silently display an empty list, with no compile-time or runtime warning.

Recommendation:

• Document the mandatory Reload call contract prominently in an XML doc comment on the procedure, or restore a fallback in OnOpenPage that loads tools for the embedded-without-init case (e.g., if ParentSystemId is null, load all active features).

Suggested change

	begin
	/// <summary>
	/// Must be called by the host page before the part is displayed.
	/// Loads the system tools for all active features of the given configuration.
	/// </summary>
	internal procedure Reload(ConfigSystemId: Guid)

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

app.json trailing newline removed

The PR removes the trailing newline from app.json (\ No newline at end of file). This is a regression — the file previously ended correctly — and will generate unnecessary noise in future diffs. Some tooling (POSIX utilities, certain CI linters) treats files without a trailing newline as malformed.

Recommendation:

• Restore the trailing newline at the end of app.json.

Suggested change

	}
	"contextSensitiveHelpUrl": "https://learn.microsoft.com/dynamics365/business-central/"
	}

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

PopulateAPIObjects scans all pages and queries on every lookup

PopulateAPIObjects issues two full-table scans of Page Metadata and Query Metadata with filters on every invocation of the API object lookup. In a large environment with hundreds of API pages and queries, this can result in noticeable latency each time the admin opens the 'Select APIs' dialog with no caching.

Recommendation:

• Consider caching the buffer within the session or using a session-scoped temporary table populated only once (checking if not TempBuffer.IsEmpty() before re-populating). Alternatively, document the expected scale and confirm performance is acceptable.

Suggested change

	local procedure PopulateAPIObjects(var MCPAPIObjectBuffer: Record "MCP API Object Buffer")
	// Check if already populated before scanning:
	internal procedure LookupAPIObjects(var SelectedObjects: Record "MCP API Object Buffer"): Boolean
	var
	TempMCPAPIObjectBuffer: Record "MCP API Object Buffer";
	MCPAPIObjectLookup: Page "MCP API Object Lookup";
	begin
	if CachedAPIObjectBuffer.IsEmpty() then // re-use session cache
	PopulateAPIObjects(CachedAPIObjectBuffer);
	...

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

IsEnabled helpers silently return defaults for unknown GUIDs

IsAPIToolsEnabled and IsDataQueryToolsEnabled call MCPConfiguration.Init() and return the field's InitValue when the given ConfigId is not found, without any log or signal to the caller. A stale or wrong GUID passed by a bug would silently report the wrong active state, causing feature checks to make incorrect decisions.

Recommendation:

• Only fall back to Init() when the GUID is explicitly null (new-record scenario). Return false and log a telemetry warning for non-null GUIDs that are not found, making the unexpected path visible.

Suggested change

	internal procedure IsAPIToolsEnabled(ConfigId: Guid): Boolean
	internal procedure IsAPIToolsEnabled(ConfigId: Guid): Boolean
	var
	MCPConfiguration: Record "MCP Configuration";
	begin
	if IsNullGuid(ConfigId) then begin
	MCPConfiguration.Init();
	exit(MCPConfiguration.EnableApiTools);
	end;
	if not MCPConfiguration.GetBySystemId(ConfigId) then
	exit(false); // config not found - treat as inactive
	exit(MCPConfiguration.EnableApiTools);
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

New configs created with API Tools disabled by default

CreateConfiguration inserts the record without setting EnableApiTools, so new configurations created after the upgrade silently start with API Tools off. The upgrade codeunit fixes existing configs, but any new config requires the admin to manually enable API Tools in the Server Features list before tools are usable. This is a behavior break from before when API Tools was implicitly always-on.

Recommendation:

• Either set MCPConfiguration.EnableApiTools := true inside CreateConfiguration, or ensure the table field has InitValue = true. Add a comment explaining the intentional default.

internal procedure CreateConfiguration(Name: Text[100]; Description: Text[250]): Guid
var
    MCPConfiguration: Record "MCP Configuration";
begin
    MCPConfiguration.Name := Name;
    MCPConfiguration.Description := Description;
    MCPConfiguration.EnableApiTools := true; // API Tools on by default for new configs
    MCPConfiguration.Insert();
    LogConfigurationCreated(MCPConfiguration);
    exit(MCPConfiguration.SystemId);
end;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

EnableDynamicToolMode error message lacks actionability

The new error label APIToolsRequiredForDynamicErr reads 'API Tools must be enabled before Dynamic Tool Mode can be enabled.' but doesn't tell the user where or how to enable API Tools. For admins unfamiliar with the new Server Features list, this may be confusing.

Recommendation:

• Update the label to reference the Server Features list: e.g. 'API Tools must be enabled in the Server Features list before Dynamic Tool Mode can be activated.'

Suggested change

	if Enable and not IsAPIToolsEnabled(ConfigId) then
	APIToolsRequiredForDynamicErr: Label 'API Tools must be enabled in the Server Features list before Dynamic Tool Mode can be activated.';

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

LoadSystemTools tool description truncation silently drops data

CopyStr(SystemTools.Get(ToolName), 1, MaxStrLen(MCPSystemTool."Tool Description")) silently truncates tool descriptions longer than the field length (250 chars). If the platform later returns longer descriptions, the truncated text will be stored without any warning, potentially making descriptions unintelligible.

Recommendation:

• Assert or log a warning when the source string exceeds the target field length, or verify with the platform that descriptions will never exceed 250 characters and add a comment documenting that contract.

Suggested change

	InsertTool(MCPSystemTool, CopyStr(ToolName, 1, MaxStrLen(MCPSystemTool."Tool Name")), CopyStr(SystemTools.Get(ToolName), 1, MaxStrLen(MCPSystemTool."Tool Description")));
	local procedure InsertTool(var MCPSystemTool: Record "MCP System Tool"; ToolName: Text[100]; ToolDescription: Text[250])
	begin
	MCPSystemTool."Server Feature" := MCPSystemTool."Server Feature"::"Data Query Tools";
	MCPSystemTool."Tool Name" := ToolName;
	MCPSystemTool."Tool Description" := ToolDescription; // platform guarantees max 250 chars
	MCPSystemTool.Insert();
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Name OnValidate fires CurrPage.Update on every keystroke for new records

The OnValidate trigger on the Name field calls CurrPage.Update() whenever IsNullGuid(Rec.SystemId) is true (i.e., for any unsaved new record). Since OnValidate fires after each field exit (or with auto-save on every keystroke in some clients), this causes repeated full page refreshes including sub-page reloads while the user is still typing the config name.

Recommendation:

• Move the CurrPage.Update() call to OnNewRecord or use a flag to run it only once after the first meaningful field entry, rather than on every Name validation for new records.

trigger OnNewRecord(BelowxRec: Boolean)
begin
    // Trigger a refresh so sub-pages initialize correctly for the new record.
    CurrPage.Update();
end;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

SystemToolList FactBox always reloads on cursor move

RefreshSubPages() is called from OnAfterGetCurrRecord, which fires every time the record changes on the card. This triggers ServerFeature.IsActive(ConfigSystemId) DB calls for every feature plus a full LoadSystemTools iteration for the SystemToolList, on every page open and every field save. On slower connections this creates visible latency.

Recommendation:

• Defer Reload calls inside OnAfterGetRecord (once per record, not every cursor event) and call RefreshSubPages only on specific state changes (Active toggle, feature activation). Reserve OnAfterGetCurrRecord for lightweight UI-only refreshes.

trigger OnAfterGetRecord()
begin
    IsDefault := MCPConfigImplementation.IsDefaultConfiguration(Rec);
    RefreshSubPages();
end;

trigger OnAfterGetCurrRecord()
begin
    // Lightweight: only refresh action visibility, not full DB-reading sub-page reload
end;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

SetAsDefault / ClearDefault miss active-config guard

The new SetAsDefault and ClearDefault actions call SetAsDefaultConfiguration and ClearDefaultConfiguration directly without checking if the configuration is active first. SetAsDefaultConfiguration in the implementation presumably performs this check, but if a future refactor moves the guard, the list actions have no safety net.

Recommendation:

• Add Enabled = Rec.Active to SetAsDefault (only active configs should be set as default, consistent with the tooltip text) and document the active requirement explicitly.

Suggested change

	Caption = 'Set as Default';
	action(SetAsDefault)
	{
	Caption = 'Set as Default';
	ToolTip = 'Set this configuration as the default. Only active configurations can be set as the default.';
	Image = Approve;
	AccessByPermission = tabledata "MCP Configuration" = M;
	Scope = Repeater;
	Enabled = not Rec.Default and Rec.Active;
	...

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

AddAPIObjects deletes current rec unconditionally after lookup

At the end of AddAPIObjects(), if not IsNullGuid(Rec.SystemId) then Rec.Delete() deletes the current record when it has a SystemId. This is inherited from the old per-type lookup logic and assumes the 'lookup from an existing row' scenario deletes that row. If an admin clicks 'Select APIs' while positioned on a valid existing tool row (non-null SystemId), that row is silently deleted after the lookup completes.

Recommendation:

• Remove or clarify the Rec.Delete() call. The delete was originally intended only for removing a blank new-row placeholder created by the OnLookup trigger. Use a flag or only delete when called from the OnLookup trigger path.

Suggested change

	if not MCPConfigImplementation.CheckAPIToolExists(Rec.ID, TempSelectedObjects."Object ID", Rec."Object Type"::Query) then
	local procedure AddAPIObjects(DeleteCurrentRow: Boolean)
	var
	TempSelectedObjects: Record "MCP API Object Buffer";
	begin
	if not MCPConfigImplementation.LookupAPIObjects(TempSelectedObjects) then
	exit;
	...
	if DeleteCurrentRow and not IsNullGuid(Rec.SystemId) then
	Rec.Delete();
	CurrPage.Update();
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Activate visible for Dynamic Tool Mode when parent inactive

The Activate action is Visible = ActionsEnabled and (Rec.Status = Rec.Status::Inactive) with no check that the parent feature (API Tools) is active. A user clicking Activate on the Dynamic Tool Mode row when API Tools is inactive receives an error instead of a gracefully disabled button.

Recommendation:

• Add a computed boolean variable ParentFeatureActive in Reload that is set per-row, or extend the MCP Server Feature table with a ParentActive field, and include it in the Visible/Enabled expression for the Activate action.

Suggested change

	{
	action(Activate)
	{
	...
	Visible = ActionsEnabled and (Rec.Status = Rec.Status::Inactive);
	Enabled = ActionsEnabled and (Rec.Status = Rec.Status::Inactive) and (Rec.Indentation = 0 or ParentFeatureActive);
	...

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

SaveChanges silently no-ops for unhandled features

SaveChanges() uses a case Feature of with only "Dynamic Tool Mode" handled and no else branch. Any feature that gains HasSettings() = true in a future iteration will open the settings dialog but silently discard changes when the user clicks OK.

Recommendation:

• Add an else Error(...) branch, or an assertion, so an unhandled feature triggers a developer-visible error rather than a silent no-op.

Suggested change

	begin
	internal procedure SaveChanges()
	var
	MCPConfigImplementation: Codeunit "MCP Config Implementation";
	begin
	case Feature of
	Feature::"Dynamic Tool Mode":
	MCPConfigImplementation.EnableDiscoverReadOnlyObjects(ConfigSystemId, DiscoverReadOnlyObjectsLocal);
	else
	Error('Unhandled feature settings for: %1', Feature);
	end;
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

TestDisableAPITools missing cascade-off verification for DiscoverReadOnly

TestDisableAPITools only asserts EnableApiTools = false but doesn't verify that EnableDynamicToolMode and DiscoverReadOnlyObjects also cascade off when API Tools is disabled on a config that had them enabled. This is tested in TestDisableAPIToolsDisablesDynamicToolMode but only for Dynamic Tool Mode; a separate test (or the same test) should also cover DiscoverReadOnlyObjects.

Recommendation:

• Merge the cascade assertion into TestDisableAPITools, or add a dedicated test that starts with DynToolMode=true, DiscoverReadOnly=true, APITools=true and verifies all three cascade off.

Suggested change

	Assert.IsFalse(MCPConfiguration.EnableApiTools, 'API Tools should be disabled');
	[Test]
	procedure TestDisableAPIToolsCascadesDiscoverReadOnly()
	var
	MCPConfiguration: Record "MCP Configuration";
	ConfigId: Guid;
	begin
	ConfigId := CreateMCPConfig(false, true, true, true);
	MCPConfig.EnableAPITools(ConfigId, true);
	MCPConfig.EnableAPITools(ConfigId, false);
	MCPConfiguration.GetBySystemId(ConfigId);
	Assert.IsFalse(MCPConfiguration.EnableDynamicToolMode, 'DynToolMode should cascade off');
	Assert.IsFalse(MCPConfiguration.DiscoverReadOnlyObjects, 'DiscoverReadOnly should cascade off');
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Upgrade tag date mismatch with merge date

The upgrade tag value is 'MS-631012-MCPEnableApiTools-20260603' (June 3) but the PR description notes that the latest merge date is June 4 (2026-06-04). While functionally harmless, upgrade tags with dates earlier than the actual deployment can be confusing during incident triage.

Recommendation:

• Update the tag date to match the actual merge/deployment date, or use a date-independent suffix (e.g., the sprint or iteration number) to avoid this discrepancy.

Suggested change

	local procedure GetMCPEnableApiToolsUpgradeTag(): Text[250]
	local procedure GetMCPEnableApiToolsUpgradeTag(): Text[250]
	begin
	exit('MS-631012-MCPEnableApiTools-20260604');
	end;

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}