[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\ —\ 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}

[github-actions]

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

IsAPIToolsEnabled returns table default for missing config

When GetBySystemId returns false (config not yet persisted or invalid GUID), the procedure calls MCPConfiguration.Init() and returns EnableApiTools from an uninitialised in-memory record. If the field's InitValue is false, passing any arbitrary or empty GUID silently returns false rather than signalling that the record does not exist, masking bugs in callers.

Recommendation:

• Return false explicitly when the config is not found, or add a guard that raises an error for non-null GUIDs that don't resolve. Reserve the Init() path only for the documented new-record case and document that behaviour.

Suggested change

	internal procedure IsAPIToolsEnabled(ConfigId: Guid): Boolean
	internal procedure IsAPIToolsEnabled(ConfigId: Guid): Boolean
	var
	MCPConfiguration: Record "MCP Configuration";
	begin
	if IsNullGuid(ConfigId) then
	exit(false); // new record not yet persisted
	if not MCPConfiguration.GetBySystemId(ConfigId) then
	exit(false);
	exit(MCPConfiguration.EnableApiTools);
	end;

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

[github-actions]

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

Reload called with empty GUID on new card record

When MCPConfigCard.OnAfterGetCurrRecord fires during creation of a new configuration record, Rec.SystemId is an empty GUID. RefreshSubPages passes this to ServerFeatureList.Page.Reload(emptyGuid, ...), which then calls each feature's IsActive(emptyGuid). IsAPIToolsEnabled calls GetBySystemId(emptyGuid), fails, and falls through to Init()—returning whatever the field's table default is. This could misrepresent API Tools as active or inactive for unsaved records.

Recommendation:

• Guard Reload against an empty GUID; skip the reload and show a neutral default state when the config has not yet been saved.

Suggested change

	ActionsEnabled := CanModify;
	internal procedure Reload(ConfigSystemId: Guid; CanModify: Boolean)
	begin
	if IsNullGuid(ConfigSystemId) then begin
	Rec.Reset();
	Rec.DeleteAll();
	exit;
	end;
	// existing reload logic ...
	end;

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

[github-actions]

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

Export action missing AccessByPermission guard

The new ExportConfiguration action on MCPConfigCard has no AccessByPermission property, meaning any user who can open the card page can export the full MCP configuration JSON—including tool definitions, API versions, and all settings—to a local file. The sibling ImportConfiguration action on MCPConfigList correctly uses AccessByPermission = tabledata "MCP Configuration" = IM.

Recommendation:

• Add AccessByPermission to the ExportConfiguration action on both MCPConfigCard and MCPConfigList consistent with the read intent.

action(ExportConfiguration)
{
    Caption = 'Export';
    ToolTip = 'Export the selected MCP configuration and its tools to a JSON file.';
    Image = Export;
    AccessByPermission = tabledata "MCP Configuration" = R;

    trigger OnAction()
    begin
        MCPConfigImplementation.ExportConfigurationToFile(Rec.SystemId, Rec.Name);
    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}}$

EnableAPITools allows modifying active configurations

EnableAPITools and EnableDataQueryTools do not check whether the target configuration is currently active before applying changes. The UI guards this with 'not IsDefault and not Rec.Active', but since these are internal procedures callable from other codeunits or test libraries, a programmatic caller can silently change feature flags on a live, active configuration mid-session.

Recommendation:

• Add an active-configuration guard analogous to the one already present in EnableDynamicToolMode: if the configuration is active, raise an error or return early.

internal procedure EnableAPITools(ConfigId: Guid; Enable: Boolean)
var
    MCPConfiguration: Record "MCP Configuration";
    xMCPConfiguration: Record "MCP Configuration";
begin
    if not MCPConfiguration.GetBySystemId(ConfigId) then
        Error(ConfigurationNotFoundErr);
    if MCPConfiguration.Active then
        Error(CannotModifyActiveConfigurationErr);
    xMCPConfiguration := MCPConfiguration;
    MCPConfiguration.EnableApiTools := Enable;
    if not Enable then begin
        MCPConfiguration.EnableDynamicToolMode := false;
        MCPConfiguration.DiscoverReadOnlyObjects := false;
    end;
    MCPConfiguration.Modify();
    LogConfigurationModified(MCPConfiguration, xMCPConfiguration);
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}}$

LookupAPIObjects test handler asserts no specific result

The replacement test handler LookupAPIObjectsOKHandler calls First() and OK() without verifying which object type or ID was actually selected. The test comment says '[THEN] A selection is returned', but the only assertion is on the Boolean return value of LookupAPIObjects(). The old tests (LookupAPIPageToolsOKHandler, LookupAPIQueryToolsOKHandler) explicitly navigated to known records and verified their IDs, providing much stronger coverage.

Recommendation:

• Make the test handler navigate to a specific known API page or query record (e.g. GoToKey for a mock API object) and add an assertion that the returned buffer contains the expected Object Type and Object ID.

Suggested change

	procedure LookupAPIObjectsOKHandler(var MCPAPIObjectLookup: TestPage "MCP API Object Lookup")
	[ModalPageHandler]
	procedure LookupAPIObjectsOKHandler(var MCPAPIObjectLookup: TestPage "MCP API Object Lookup")
	begin
	MCPAPIObjectLookup.GoToKey(MCPAPIObjectLookup."Object Type"::Page, Page::"Mock API");
	MCPAPIObjectLookup.OK().Invoke();
	end;
	[Test]
	[HandlerFunctions('LookupAPIObjectsOKHandler')]
	procedure TestLookupAPIObjects()
	var
	TempSelectedObjects: Record "MCP API Object Buffer";
	begin
	Assert.IsTrue(MCPConfigTestLibrary.LookupAPIObjects(TempSelectedObjects), 'Lookup returned no selection');
	TempSelectedObjects.FindFirst();
	Assert.AreEqual(TempSelectedObjects."Object Type"::Page, TempSelectedObjects."Object Type", 'Wrong object type');
	Assert.AreEqual(Page::"Mock API", TempSelectedObjects."Object ID", 'Wrong object ID');
	end;

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

[github-actions]

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

Upgrade enables API Tools on active configs

EnableApiToolsOnExistingConfigurations calls MCPConfiguration.ModifyAll(EnableApiTools, true), which bypasses all record-level validation and modifies every configuration record—including currently active ones. Enabling a feature flag on an active configuration could immediately change the behaviour of live MCP sessions without the administrator's explicit consent.

Recommendation:

• Exclude active configurations from the bulk update, or mark the upgrade so it only applies to inactive configurations. If enabling for active configs is intentional, add a comment explaining the decision and confirm there are no runtime side-effects.

Suggested change

	if UpgradeTag.HasDatabaseUpgradeTag(GetMCPEnableApiToolsUpgradeTag()) then
	internal procedure EnableApiToolsOnExistingConfigurations()
	var
	MCPConfiguration: Record "MCP Configuration";
	UpgradeTag: Codeunit "Upgrade Tag";
	begin
	if UpgradeTag.HasDatabaseUpgradeTag(GetMCPEnableApiToolsUpgradeTag()) then
	exit;
	MCPConfiguration.SetRange(Active, false);
	MCPConfiguration.ModifyAll(EnableApiTools, true);
	UpgradeTag.SetUpgradeTag(GetMCPEnableApiToolsUpgradeTag());
	end;

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

[github-actions]

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

app.json missing newline at end of file

The new version of app.json ends without a trailing newline ('No newline at end of file'), which can cause issues with diff tools, version control, and some CI checks that enforce POSIX compliance. This is a regression introduced by this PR.

Recommendation:

• Add a trailing newline to 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\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Upgrade procedure is internal instead of local

EnableApiToolsOnExistingConfigurations is declared internal, making it callable from other codeunits in the same app (e.g. test libraries). If called outside the upgrade pipeline—without the UpgradeTag guard being respected or with the tag already set—the ModifyAll would either silently no-op or unexpectedly re-enable API Tools. Making it internal also exposes the upgrade step to potential misuse in production.

Recommendation:

• Declare the procedure local unless it genuinely needs to be called from outside this codeunit. If tests need to invoke it directly, create a thin internal wrapper in a test library instead.

local procedure EnableApiToolsOnExistingConfigurations()
var
    MCPConfiguration: Record "MCP Configuration";
    UpgradeTag: Codeunit "Upgrade Tag";
begin
    if UpgradeTag.HasDatabaseUpgradeTag(GetMCPEnableApiToolsUpgradeTag()) then
        exit;
    MCPConfiguration.ModifyAll(EnableApiTools, true);
    UpgradeTag.SetUpgradeTag(GetMCPEnableApiToolsUpgradeTag());
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\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

RefreshSubPages fires DB reads on every record navigation

RefreshSubPages() is called from OnAfterGetCurrRecord, which fires every time the user navigates to a different row on the configuration card. Each call invokes IsActive(ConfigId) for every feature enum value (currently 3 features), issuing at least 3 database reads per navigation. Users rapidly scrolling through configurations will trigger a burst of reads.

Recommendation:

• Move the full RefreshSubPages reload out of OnAfterGetCurrRecord and keep it only in OnAfterGetRecord (triggered once per record load) plus the specific OnValidate handlers that actually change feature state, rather than on every cursor movement.

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

trigger OnAfterGetCurrRecord()
begin
    // do not reload sub-pages here; already handled in OnAfterGetRecord and OnValidate triggers
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}}$

API Tools cascade bypasses default-config guard

EnableAPITools(ConfigId, false) directly sets EnableDynamicToolMode := false without calling EnableDynamicToolMode, bypassing the DynamicToolModeCannotBeDisabledErr guard that prevents disabling Dynamic Tool Mode on the default configuration. A caller using the public MCPConfig.EnableAPITools facade can therefore silently disable Dynamic Tool Mode on the default config, breaking the invariant that EnableDynamicToolMode enforces.

Recommendation:

• Add an IsDefaultConfiguration guard to EnableAPITools before the cascade—similar to the one in EnableDynamicToolMode—or call EnableDynamicToolMode(ConfigId, false) through the existing validated procedure rather than setting the field directly.

Suggested change

	Error(ConfigurationNotFoundErr);
	if not Enable then begin
	if IsDefaultConfiguration(MCPConfiguration) then
	Error(DesignatedDefaultCannotBeDeactivatedErr);
	MCPConfiguration.EnableDynamicToolMode := false;
	MCPConfiguration.DiscoverReadOnlyObjects := false;
	end;

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

[github-actions]

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

File missing trailing newline

Both MCPUpgrade.Codeunit.al and app.json are saved without a trailing newline (git diff shows \ No newline at end of file). This is inconsistent with the rest of the codebase, can cause diff noise in future patches, and may trigger linting warnings.

Recommendation:

• Ensure each file ends with exactly one trailing newline (\n).

Suggested change

	end;
	// Add a blank line after the final `}` in MCPUpgrade.Codeunit.al

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

[github-actions]

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

Import allows inconsistent enableApiTools=false + enableDynamicToolMode=true

During ImportConfiguration, both EnableApiTools and EnableDynamicToolMode are written directly to the record before Insert(), with no cross-field validation. A JSON payload containing enableApiTools: false and enableDynamicToolMode: true will produce a configuration that violates the new invariant—Dynamic Tool Mode requires API Tools—without raising any error.

Recommendation:

• After reading all boolean flags from JSON and before MCPConfiguration.Insert(), add a consistency check: if EnableDynamicToolMode is true but EnableApiTools is false, either force EnableApiTools := true or throw an import error.

if MCPConfiguration.EnableDynamicToolMode and not MCPConfiguration.EnableApiTools then
    MCPConfiguration.EnableApiTools := true; // enforce prerequisite on import

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}}$

EnableAPITools logs modified event even when no fields changed

EnableAPITools calls LogConfigurationModified(MCPConfiguration, xMCPConfiguration) unconditionally after MCPConfiguration.Modify(). If the caller passes the same value as already stored (e.g., enabling API Tools when it is already enabled), the audit log and telemetry will record a spurious modification event with no changed dimensions, polluting the audit trail. The same pattern exists in EnableDataQueryTools.

Recommendation:

• Guard the Modify() and LogConfigurationModified() calls with a dirty-check: only proceed if the new value differs from the existing one, consistent with how other procedures in this codeunit behave.

Suggested change

	if MCPConfiguration.EnableApiTools = Enable then
	exit;
	MCPConfiguration.EnableApiTools := Enable;
	...

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

[github-actions]

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

Reload always iterates all features even when none changed

Reload is called after every SetActive call (to reflect cascaded changes). However, it also calls InsertRow for every enum ordinal, which calls ServerFeature.IsActive(ConfigId) (a DB read) for each feature on every activation/deactivation. If the feature list grows, this becomes O(n) DB calls on every user interaction, with no caching.

Recommendation:

• Consider reading all required field values from the MCP Configuration record in a single GetBySystemId call inside Reload, then populating the temp table from in-memory values rather than dispatching an IsActive call per feature.

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

[github-actions]

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

Missing negative test for Dynamic Tool Mode + API Tools gate

The test TestEnableDynamicToolMode was updated to first enable API Tools before enabling Dynamic Tool Mode, ensuring the positive path succeeds. However, there is no test that verifies calling EnableDynamicToolMode(ConfigId, true) when API Tools is disabled raises APIToolsRequiredForDynamicErr. Without a negative test, a future regression removing or relocating the guard would go undetected.

Recommendation:

• Add a test procedure (e.g., TestEnableDynamicToolModeRequiresAPITools) that calls MCPConfig.EnableDynamicToolMode(ConfigId, true) on a config where API Tools is disabled and asserts that the expected error is thrown.

[Test]
procedure TestEnableDynamicToolModeRequiresAPITools()
var
    ConfigId: Guid;
begin
    ConfigId := CreateMCPConfig(false, false, true, false);
    // API Tools is off by default on new config
    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}

[darjoo]

Should the rest of the tooltips etc also say API instead of tool?