diff --git a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp index 6b86ca406e..8b0cce9846 100644 --- a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp +++ b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp @@ -9,6 +9,7 @@ #include "EnumEntry.h" #include "../inc/AgentRegistry.h" #include "../inc/AgentHooksStatus.h" +#include "../inc/CustomAgentId.h" #include "../inc/WtaProcess.h" #include @@ -67,21 +68,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation winrt::hstring AIAgentsViewModel::_DeriveId(const winrt::hstring& command) { - const auto str = winrt::to_string(command); - const auto pos = str.find(' '); - auto token = (pos != std::string::npos) ? str.substr(0, pos) : str; - auto slash = token.rfind('\\'); - if (slash == std::string::npos) slash = token.rfind('/'); - if (slash != std::string::npos) token = token.substr(slash + 1); - for (const auto* ext : { ".exe", ".cmd", ".bat" }) - { - if (token.size() > strlen(ext) && token.substr(token.size() - strlen(ext)) == ext) - { - token = token.substr(0, token.size() - strlen(ext)); - break; - } - } - return winrt::to_hstring(token); + // Delegate to the header-only helper shared with the unit tests. + return ::Microsoft::Terminal::Settings::Model::DeriveCustomAgentId( + std::wstring_view{ command }); } void AIAgentsViewModel::_AppendAddNewEntry(IObservableVector& list) @@ -100,9 +89,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation const auto bareId = _DeriveId(customCommand); const bool isBuiltIn = _IsKnownAgent(bareId); - const auto settingsId = isBuiltIn - ? winrt::hstring{ L"custom:" + std::wstring_view{ bareId } } - : bareId; + // Mirror SaveCustom*: the saved id always carries "custom:". + const auto settingsId = winrt::hstring{ L"custom:" + std::wstring_view{ bareId } }; const auto displayName = isBuiltIn ? winrt::hstring{ std::wstring_view{ bareId } + L" (custom)" } : bareId; @@ -564,12 +552,19 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation if (_GlobalSettings.IsCustomAgentPolicyLocked()) return; if (_customAcpCommand.empty()) return; const auto bareId = _DeriveId(_customAcpCommand); + // Whitespace-only / quote-only commands derive to an empty id and + // would otherwise be saved as a bare "custom:" entry, leaving the + // UI with a blank, unusable custom agent. Reject before persisting. + if (bareId.empty()) return; _GlobalSettings.AcpCustomCommand(_customAcpCommand); + // Custom agents always carry the "custom:" discriminator — every + // downstream consumer (EffectiveAcpAgent policy gate, command-line + // resolver, custom-edit/delete UI gates) keys on this prefix. + // Storing a bare id silently breaks all of them and makes the page + // revert to the default agent on next load. const bool isBuiltIn = _IsKnownAgent(bareId); - const auto settingsId = isBuiltIn - ? winrt::hstring{ L"custom:" + std::wstring_view{ bareId } } - : bareId; + const auto settingsId = winrt::hstring{ L"custom:" + std::wstring_view{ bareId } }; const auto displayName = isBuiltIn ? winrt::hstring{ std::wstring_view{ bareId } + L" (custom)" } : bareId; @@ -601,12 +596,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation if (_GlobalSettings.IsCustomAgentPolicyLocked()) return; if (_customDelegateCommand.empty()) return; const auto bareId = _DeriveId(_customDelegateCommand); + // See SaveCustomAcpAgent — reject empty derivations before persisting. + if (bareId.empty()) return; _GlobalSettings.DelegateCustomCommand(_customDelegateCommand); + // See SaveCustomAcpAgent — always carry the "custom:" prefix. const bool isBuiltIn = _IsKnownAgent(bareId); - const auto settingsId = isBuiltIn - ? winrt::hstring{ L"custom:" + std::wstring_view{ bareId } } - : bareId; + const auto settingsId = winrt::hstring{ L"custom:" + std::wstring_view{ bareId } }; const auto displayName = isBuiltIn ? winrt::hstring{ std::wstring_view{ bareId } + L" (custom)" } : bareId; diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index d67a907830..556c8f6b54 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -644,3 +644,18 @@ bool GlobalAppSettings::IsAgentSessionHooksPolicyLocked() const { return AgentPolicy::GetAgentSessionHooksPolicy() == AgentPolicy::PolicyState::Blocked; } + +// ── Test-only hooks ───────────────────────────────────────────────── +// Defined here so the body executes inside SettingsModel.dll, which +// guarantees we patch the same AgentPolicy::s_snapshot that +// EffectiveAcpAgent / EffectiveDelegateAgent read. + +void GlobalAppSettings::_TestHookSetAgentPolicy(std::shared_ptr snap) +{ + AgentPolicy::SetSnapshotForTest(std::move(snap)); +} + +void GlobalAppSettings::_TestHookResetAgentPolicy() +{ + AgentPolicy::ResetForTest(); +} diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h index 0a41d9489d..e68e4eb608 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h @@ -33,6 +33,13 @@ namespace SettingsModelUnitTests class ColorSchemeTests; }; +// Forward declaration so we can declare the test-hook signature below +// without pulling in AgentPolicy.h from this widely-included header. +namespace Microsoft::Terminal::Settings::Model::AgentPolicy +{ + struct PolicySnapshot; +}; + namespace winrt::Microsoft::Terminal::Settings::Model::implementation { struct GlobalAppSettings : GlobalAppSettingsT, IInheritable @@ -95,6 +102,16 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool IsAutoFixPolicyLocked() const; bool IsAgentSessionHooksPolicyLocked() const; + // ── Test-only seam ────────────────────────────────────────────── + // Replace the SettingsModel DLL's cached AgentPolicy snapshot + // with a test-controlled value. Compiled into SettingsModel.dll + // so writes target the same `s_snapshot` that + // EffectiveAcpAgent / EffectiveDelegateAgent read. Pair every + // call with _TestHookResetAgentPolicy() in test cleanup. + // NOT for production use. + static void _TestHookSetAgentPolicy(std::shared_ptr snap); + static void _TestHookResetAgentPolicy(); + INHERITABLE_SETTING(Model::GlobalAppSettings, hstring, UnparsedDefaultProfile, L""); #define GLOBAL_SETTINGS_INITIALIZE(type, name, jsonKey, ...) \ diff --git a/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp b/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp new file mode 100644 index 0000000000..3ade8f9ae9 --- /dev/null +++ b/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp @@ -0,0 +1,317 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// CustomAgentAndPolicyTests.cpp +// +// Covers two areas that were previously untested at the settings-model +// layer (see PR #123): +// +// 1. JSON round-trip of the custom-agent settings. A regression that +// drops the "custom:" prefix from AcpAgent / DelegateAgent breaks +// every downstream consumer that keys on the prefix (the +// EffectiveAcpAgent policy gate, the launcher's command-line +// resolver, and the custom-edit/delete UI gates). The first half +// of this file asserts that these settings survive an unmodified +// load. +// +// 2. The GPO policy matrix on EffectiveAcpAgent / EffectiveDelegateAgent. +// AllowedAgents (registry REG_MULTI_SZ) only filters built-in agent +// IDs; the custom: scheme is gated separately by AllowCustomAgents +// (registry REG_DWORD). This file pins that behavior so a future +// refactor of the policy gate doesn't silently change it. +// Policy state is injected via GlobalAppSettings::_TestHookSetAgentPolicy +// so the tests do not touch the user's registry. + +#include "pch.h" + +#include "../TerminalSettingsModel/GlobalAppSettings.h" +#include "../TerminalSettingsModel/CascadiaSettings.h" +#include "../inc/AgentPolicy.h" +#include "JsonTestClass.h" + +using namespace Microsoft::Console; +using namespace WEX::Logging; +using namespace WEX::TestExecution; +using namespace WEX::Common; +using namespace winrt::Microsoft::Terminal::Settings::Model; +namespace AgentPolicy = ::Microsoft::Terminal::Settings::Model::AgentPolicy; + +namespace SettingsModelUnitTests +{ + class CustomAgentAndPolicyTests : public JsonTestClass + { + TEST_CLASS(CustomAgentAndPolicyTests); + + // Round-trip tests + TEST_METHOD(CustomAcpAgentRoundtrips); + TEST_METHOD(CustomDelegateAgentRoundtrips); + TEST_METHOD(QuotedPathCustomCommandRoundtrips); + + // Policy: AcpAgent + TEST_METHOD(EffectiveAcpAgentEmptyStaysEmpty); + TEST_METHOD(EffectiveAcpAgentBuiltInPassesWhenNoAllowlist); + TEST_METHOD(EffectiveAcpAgentBuiltInPassesWhenInAllowlist); + TEST_METHOD(EffectiveAcpAgentBuiltInBlockedWhenMissingFromAllowlist); + TEST_METHOD(EffectiveAcpAgentBuiltInMatchIsCaseInsensitive); + TEST_METHOD(EffectiveAcpAgentBuiltInBlockedByEmptyAllowlist); + TEST_METHOD(EffectiveAcpAgentCustomPassesWhenNoCustomPolicy); + TEST_METHOD(EffectiveAcpAgentCustomBlockedByCustomPolicy); + TEST_METHOD(EffectiveAcpAgentCustomIgnoresAllowedAgentsAllowlist); + + // Policy: DelegateAgent (parallel matrix) + TEST_METHOD(EffectiveDelegateAgentEmptyStaysEmpty); + TEST_METHOD(EffectiveDelegateAgentBuiltInPassesWhenNoAllowlist); + TEST_METHOD(EffectiveDelegateAgentBuiltInBlockedWhenMissingFromAllowlist); + TEST_METHOD(EffectiveDelegateAgentCustomBlockedByCustomPolicy); + TEST_METHOD(EffectiveDelegateAgentCustomIgnoresAllowedAgentsAllowlist); + + // Lock-state mirroring + TEST_METHOD(IsAgentPolicyLockedTracksAllowedAgents); + TEST_METHOD(IsCustomAgentPolicyLockedTracksBlocked); + + TEST_CLASS_CLEANUP(ClassCleanup) + { + // Defense in depth: never leave a test snapshot lying around + // for the next test class to inherit. + implementation::GlobalAppSettings::_TestHookResetAgentPolicy(); + return true; + } + + TEST_METHOD_CLEANUP(MethodCleanup) + { + // Every test that calls const auto settings = MakeSettings({}); SetPolicy() should be followed by a + // reset so the next test isn't poisoned by stale state. + implementation::GlobalAppSettings::_TestHookResetAgentPolicy(); + return true; + } + + private: + // Build a minimal CascadiaSettings JSON with the supplied global + // overrides spliced in. Profiles are required, so we provide one. + static winrt::com_ptr MakeSettings(std::string_view globalsExtra) + { + const auto userJson = std::string{ R"({ + "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "profiles": [ + { + "name": "p0", + "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}" + } + ])" } + + (globalsExtra.empty() ? "" : ("," + std::string{ globalsExtra })) + + "}"; + return winrt::make_self(userJson, std::string_view{}); + } + + static std::shared_ptr MakePolicy( + std::optional> allowedAgents = std::nullopt, + AgentPolicy::PolicyState customAgents = AgentPolicy::PolicyState::NotConfigured) + { + auto snap = std::make_shared(); + snap->allowedAgents = std::move(allowedAgents); + snap->customAgents = customAgents; + return snap; + } + + // Install a policy snapshot in the SettingsModel DLL for the + // remainder of the test. + // + // IMPORTANT: Must be called AFTER MakeSettings(). CascadiaSettings' + // load path calls AgentPolicy::Reload() which reads the real + // registry and clobbers any test snapshot installed beforehand. + static void SetPolicy(std::shared_ptr snap) + { + implementation::GlobalAppSettings::_TestHookSetAgentPolicy(std::move(snap)); + } + }; + + // ── Round-trip ────────────────────────────────────────────────────── + + void CustomAgentAndPolicyTests::CustomAcpAgentRoundtrips() + { + // The whole point of PR #123: a custom agent must survive load + // with its "custom:" prefix intact. If this regresses, the + // settings page reverts to the default agent on next load. + const auto settings = MakeSettings(R"("acpAgent": "custom:helper", "acpCustomCommand": "helper.cmd --acp")"); + const auto& globals = settings->GlobalSettings(); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:helper" }, globals.AcpAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"helper.cmd --acp" }, globals.AcpCustomCommand()); + } + + void CustomAgentAndPolicyTests::CustomDelegateAgentRoundtrips() + { + const auto settings = MakeSettings(R"("delegateAgent": "custom:helper", "delegateCustomCommand": "helper.cmd --acp")"); + const auto& globals = settings->GlobalSettings(); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:helper" }, globals.DelegateAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"helper.cmd --acp" }, globals.DelegateCustomCommand()); + } + + void CustomAgentAndPolicyTests::QuotedPathCustomCommandRoundtrips() + { + // Commands containing spaces (so containing JSON-escaped quotes) + // are common for users on the Windows installer paths. Make sure + // the parser preserves them verbatim. + const auto settings = MakeSettings( + R"("acpAgent": "custom:helper", "acpCustomCommand": "\"C:\\Program Files\\helper\\helper.cmd\" --acp")"); + const auto& globals = settings->GlobalSettings(); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:helper" }, globals.AcpAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ LR"("C:\Program Files\helper\helper.cmd" --acp)" }, + globals.AcpCustomCommand()); + } + + // ── EffectiveAcpAgent ─────────────────────────────────────────────── + + void CustomAgentAndPolicyTests::EffectiveAcpAgentEmptyStaysEmpty() + { + // User explicitly cleared the agent (vs. relying on the "copilot" + // default). EffectiveAcpAgent must short-circuit before policy + // checks and return empty unchanged. + const auto settings = MakeSettings(R"("acpAgent": "")"); + SetPolicy(MakePolicy()); + VERIFY_ARE_EQUAL(winrt::hstring{}, settings->GlobalSettings().EffectiveAcpAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveAcpAgentBuiltInPassesWhenNoAllowlist() + { + // No AllowedAgents policy → all built-in agents pass through. + const auto settings = MakeSettings(R"("acpAgent": "copilot")"); + SetPolicy(MakePolicy()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"copilot" }, settings->GlobalSettings().EffectiveAcpAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveAcpAgentBuiltInPassesWhenInAllowlist() + { + const auto settings = MakeSettings(R"("acpAgent": "copilot")"); + SetPolicy(MakePolicy(std::set{ L"copilot", L"gemini" })); + VERIFY_ARE_EQUAL(winrt::hstring{ L"copilot" }, settings->GlobalSettings().EffectiveAcpAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveAcpAgentBuiltInBlockedWhenMissingFromAllowlist() + { + // IT admin published an allowlist that does NOT contain "copilot". + // EffectiveAcpAgent must collapse the user's choice to empty. + const auto settings = MakeSettings(R"("acpAgent": "copilot")"); + SetPolicy(MakePolicy(std::set{ L"gemini" })); + VERIFY_ARE_EQUAL(winrt::hstring{}, settings->GlobalSettings().EffectiveAcpAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveAcpAgentBuiltInMatchIsCaseInsensitive() + { + // AgentPolicy::CaseInsensitiveLess is used so admin can spell + // "Copilot" / "COPILOT" / "copilot" and they all match. + const auto settings = MakeSettings(R"("acpAgent": "copilot")"); + SetPolicy(MakePolicy(std::set{ L"Copilot" })); + VERIFY_ARE_EQUAL(winrt::hstring{ L"copilot" }, settings->GlobalSettings().EffectiveAcpAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveAcpAgentBuiltInBlockedByEmptyAllowlist() + { + // Empty allowlist (configured but empty) means *nothing* is + // allowed. Distinct from "not configured" (nullopt) which means + // everything is allowed. + const auto settings = MakeSettings(R"("acpAgent": "copilot")"); + SetPolicy(MakePolicy(std::set{})); + VERIFY_ARE_EQUAL(winrt::hstring{}, settings->GlobalSettings().EffectiveAcpAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveAcpAgentCustomPassesWhenNoCustomPolicy() + { + const auto settings = MakeSettings(R"("acpAgent": "custom:helper", "acpCustomCommand": "helper.cmd")"); + SetPolicy(MakePolicy(/*allowedAgents*/ std::nullopt, AgentPolicy::PolicyState::NotConfigured)); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:helper" }, settings->GlobalSettings().EffectiveAcpAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveAcpAgentCustomBlockedByCustomPolicy() + { + const auto settings = MakeSettings(R"("acpAgent": "custom:helper", "acpCustomCommand": "helper.cmd")"); + SetPolicy(MakePolicy(/*allowedAgents*/ std::nullopt, AgentPolicy::PolicyState::Blocked)); + VERIFY_ARE_EQUAL(winrt::hstring{}, settings->GlobalSettings().EffectiveAcpAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveAcpAgentCustomIgnoresAllowedAgentsAllowlist() + { + // Documented behavior: AllowedAgents only filters built-in IDs. + // A custom: agent is gated solely by AllowCustomAgents. + // + // Admin allowlist with only "gemini" — would block built-in + // copilot. But a custom: agent passes through unchanged because + // customAgents policy is NotConfigured / Allowed. + const auto settings = MakeSettings(R"("acpAgent": "custom:helper", "acpCustomCommand": "helper.cmd")"); + SetPolicy(MakePolicy(std::set{ L"gemini" }, + AgentPolicy::PolicyState::NotConfigured)); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:helper" }, settings->GlobalSettings().EffectiveAcpAgent()); + } + + // ── EffectiveDelegateAgent ────────────────────────────────────────── + + void CustomAgentAndPolicyTests::EffectiveDelegateAgentEmptyStaysEmpty() + { + const auto settings = MakeSettings(R"("delegateAgent": "")"); + SetPolicy(MakePolicy()); + VERIFY_ARE_EQUAL(winrt::hstring{}, settings->GlobalSettings().EffectiveDelegateAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveDelegateAgentBuiltInPassesWhenNoAllowlist() + { + const auto settings = MakeSettings(R"("delegateAgent": "copilot")"); + SetPolicy(MakePolicy()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"copilot" }, settings->GlobalSettings().EffectiveDelegateAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveDelegateAgentBuiltInBlockedWhenMissingFromAllowlist() + { + const auto settings = MakeSettings(R"("delegateAgent": "copilot")"); + SetPolicy(MakePolicy(std::set{ L"gemini" })); + VERIFY_ARE_EQUAL(winrt::hstring{}, settings->GlobalSettings().EffectiveDelegateAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveDelegateAgentCustomBlockedByCustomPolicy() + { + const auto settings = MakeSettings(R"("delegateAgent": "custom:helper", "delegateCustomCommand": "helper.cmd")"); + SetPolicy(MakePolicy(/*allowedAgents*/ std::nullopt, AgentPolicy::PolicyState::Blocked)); + VERIFY_ARE_EQUAL(winrt::hstring{}, settings->GlobalSettings().EffectiveDelegateAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveDelegateAgentCustomIgnoresAllowedAgentsAllowlist() + { + const auto settings = MakeSettings(R"("delegateAgent": "custom:helper", "delegateCustomCommand": "helper.cmd")"); + SetPolicy(MakePolicy(std::set{ L"gemini" }, + AgentPolicy::PolicyState::NotConfigured)); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:helper" }, settings->GlobalSettings().EffectiveDelegateAgent()); + } + + // ── Lock-state ────────────────────────────────────────────────────── + + void CustomAgentAndPolicyTests::IsAgentPolicyLockedTracksAllowedAgents() + { + // No allowlist → not locked. + auto settings = MakeSettings({}); + SetPolicy(MakePolicy()); + VERIFY_IS_FALSE(settings->GlobalSettings().IsAgentPolicyLocked()); + + // Allowlist present → locked. + settings = MakeSettings({}); + SetPolicy(MakePolicy(std::set{ L"copilot" })); + VERIFY_IS_TRUE(settings->GlobalSettings().IsAgentPolicyLocked()); + + // Empty allowlist also counts as configured → locked. + settings = MakeSettings({}); + SetPolicy(MakePolicy(std::set{})); + VERIFY_IS_TRUE(settings->GlobalSettings().IsAgentPolicyLocked()); + } + + void CustomAgentAndPolicyTests::IsCustomAgentPolicyLockedTracksBlocked() + { + auto settings = MakeSettings({}); + SetPolicy(MakePolicy(std::nullopt, AgentPolicy::PolicyState::NotConfigured)); + VERIFY_IS_FALSE(settings->GlobalSettings().IsCustomAgentPolicyLocked()); + + settings = MakeSettings({}); + SetPolicy(MakePolicy(std::nullopt, AgentPolicy::PolicyState::Allowed)); + VERIFY_IS_FALSE(settings->GlobalSettings().IsCustomAgentPolicyLocked()); + + settings = MakeSettings({}); + SetPolicy(MakePolicy(std::nullopt, AgentPolicy::PolicyState::Blocked)); + VERIFY_IS_TRUE(settings->GlobalSettings().IsCustomAgentPolicyLocked()); + } +} diff --git a/src/cascadia/UnitTests_SettingsModel/SettingsModel.UnitTests.vcxproj b/src/cascadia/UnitTests_SettingsModel/SettingsModel.UnitTests.vcxproj index 4b488c2dca..e1e9dbe181 100644 --- a/src/cascadia/UnitTests_SettingsModel/SettingsModel.UnitTests.vcxproj +++ b/src/cascadia/UnitTests_SettingsModel/SettingsModel.UnitTests.vcxproj @@ -45,6 +45,7 @@ + Create diff --git a/src/cascadia/inc/AgentPolicy.h b/src/cascadia/inc/AgentPolicy.h index 1fdaede00b..1ae45a20cd 100644 --- a/src/cascadia/inc/AgentPolicy.h +++ b/src/cascadia/inc/AgentPolicy.h @@ -199,6 +199,33 @@ namespace Microsoft::Terminal::Settings::Model::AgentPolicy return _GetSnapshot()->autoFix; } + // ── Test-only seam ────────────────────────────────────────────────── + // + // Replace this DLL's cached snapshot with a caller-supplied one and + // mark the cache as loaded so production code paths see it. Used by + // unit tests to exercise the policy-aware getters without touching + // the user's registry. NOT for production use. + // + // Because each consuming DLL has its own inline-static cache, this + // helper only patches the DLL that compiled the call. To exercise + // EffectiveAcpAgent / EffectiveDelegateAgent (which live in the + // SettingsModel DLL), call through GlobalAppSettings::_TestHookSetAgentPolicy. + inline void SetSnapshotForTest(std::shared_ptr snap) + { + std::lock_guard lock{ s_policyMutex }; + s_snapshot = std::move(snap); + s_loaded.store(true, std::memory_order_release); + } + + // Drop the test snapshot so the next call lazy-loads from the real + // registry again. Tests should call this in cleanup. + inline void ResetForTest() + { + std::lock_guard lock{ s_policyMutex }; + s_snapshot.reset(); + s_loaded.store(false, std::memory_order_release); + } + inline PolicyState GetAgentSessionHooksPolicy() { return _GetSnapshot()->agentSessionHooks; diff --git a/src/cascadia/inc/CustomAgentId.h b/src/cascadia/inc/CustomAgentId.h new file mode 100644 index 0000000000..bddf84de5e --- /dev/null +++ b/src/cascadia/inc/CustomAgentId.h @@ -0,0 +1,90 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +// CustomAgentId.h — derive a short, stable identifier from a user-supplied +// command line for a "custom" AI agent (the ACP / delegate agent slot). +// +// The settings UI lets the user paste an arbitrary command (e.g. `helper.cmd +// --acp`, `"C:\Program Files\helper\helper.cmd" --acp`, or just `helper`). +// The settings model stores this command verbatim in AcpCustomCommand / +// DelegateCustomCommand. But the agent *id* itself (AcpAgent / +// DelegateAgent) needs to be a single short token so that the rest of the +// pipeline (policy allowlist, display name) has something stable to key +// on. +// +// `DeriveCustomAgentId` performs that extraction: +// 1. Trim leading whitespace. +// 2. Take the first whitespace-separated token, or if the command begins +// with a double-quote take the contents of the quoted region (so +// paths containing spaces work). +// 3. Strip any directory portion (last `/` or `\`). +// 4. Strip a trailing `.exe`, `.cmd`, or `.bat` extension +// (case-insensitive). +// +// Header-only and pure: no settings-model or registry access. Callers must +// always prefix the returned id with "custom:" before storing it in the +// AcpAgent / DelegateAgent setting — that prefix is the system-wide +// discriminator used by EffectiveAcpAgent, the command-line resolver, and +// the custom-edit/delete UI gates. (Telemetry collapses every non-built-in +// id to literal `custom` via `sanitizeProviderId` and does not key on the +// prefix.) Storing a bare id silently breaks the consumers above; see +// PR #123. + +#pragma once + +#include +#include + +namespace Microsoft::Terminal::Settings::Model +{ + inline winrt::hstring DeriveCustomAgentId(std::wstring_view command) + { + // Trim leading whitespace. + const auto firstNonSpace = command.find_first_not_of(L" \t"); + if (firstNonSpace == std::wstring_view::npos) + { + return winrt::hstring{}; + } + command.remove_prefix(firstNonSpace); + + // Pull out the executable token. + std::wstring_view token; + if (!command.empty() && command.front() == L'"') + { + command.remove_prefix(1); + const auto closing = command.find(L'"'); + token = (closing != std::wstring_view::npos) ? command.substr(0, closing) : command; + } + else + { + const auto pos = command.find_first_of(L" \t"); + token = (pos != std::wstring_view::npos) ? command.substr(0, pos) : command; + } + + // Strip directory portion. + const auto slash = token.find_last_of(L"\\/"); + if (slash != std::wstring_view::npos) + { + token = token.substr(slash + 1); + } + + // Case-insensitive trailing-extension strip. CompareStringOrdinal + // works on non-null-terminated buffers and matches the style of + // AgentPolicy.h. + for (const auto* ext : { L".exe", L".cmd", L".bat" }) + { + const auto extLen = static_cast(4); // all three are 4 wide chars + if (token.size() > extLen && + CompareStringOrdinal( + token.data() + token.size() - extLen, static_cast(extLen), + ext, static_cast(extLen), + TRUE) == CSTR_EQUAL) + { + token = token.substr(0, token.size() - extLen); + break; + } + } + + return winrt::hstring{ token }; + } +} diff --git a/src/cascadia/ut_app/CustomAgentIdTests.cpp b/src/cascadia/ut_app/CustomAgentIdTests.cpp new file mode 100644 index 0000000000..57dc065005 --- /dev/null +++ b/src/cascadia/ut_app/CustomAgentIdTests.cpp @@ -0,0 +1,215 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +// CustomAgentIdTests.cpp +// +// Tests for `DeriveCustomAgentId` (src/cascadia/inc/CustomAgentId.h). +// +// This is the function used by the AI Agents settings page to turn a +// user-supplied command line (e.g. `helper.cmd --acp`, `"C:\Program +// Files\helper\helper.cmd" --acp`) into the short token that becomes the +// suffix of the stored agent id (e.g. `custom:helper`). Every downstream +// consumer that keys on the prefixed id (EffectiveAcpAgent policy gate, +// command-line resolver, custom-edit/delete UI gates) depends on this +// derivation; regressing this function silently breaks the save/reload +// round-trip (PR #123) or the launcher. + +#include "precomp.h" + +#include "../inc/CustomAgentId.h" + +using namespace WEX::Logging; +using namespace WEX::TestExecution; +using namespace WEX::Common; +using namespace Microsoft::Terminal::Settings::Model; + +namespace TerminalAppUnitTests +{ + class CustomAgentIdTests + { + TEST_CLASS(CustomAgentIdTests); + + TEST_METHOD(BareName); + TEST_METHOD(NameWithExe); + TEST_METHOD(NameWithCmd); + TEST_METHOD(NameWithBat); + TEST_METHOD(ExtensionStripIsCaseInsensitive); + TEST_METHOD(NameWithArgs); + TEST_METHOD(UnquotedPath); + TEST_METHOD(QuotedPathWithSpaces); + TEST_METHOD(QuotedPathWithSpacesAndArgs); + TEST_METHOD(ForwardSlashPath); + TEST_METHOD(LeadingWhitespace); + TEST_METHOD(TabSeparator); + TEST_METHOD(Empty); + TEST_METHOD(WhitespaceOnly); + TEST_METHOD(UnclosedQuote); + TEST_METHOD(QuoteOnlyIsEmpty); + TEST_METHOD(EmptyQuotedIsEmpty); + TEST_METHOD(BuiltInAgentNameStillExtracts); + TEST_METHOD(PathWithSpacesAndExtensionStrip); + TEST_METHOD(MixedSlashesUsesLastSeparator); + TEST_METHOD(NoExtensionStripWhenTokenEqualsExtension); + TEST_METHOD(DoesNotStripUnknownExtension); + + // Helper: assert DeriveCustomAgentId(input) == expected. + static void Check(std::wstring_view input, std::wstring_view expected) + { + const auto actual = DeriveCustomAgentId(input); + VERIFY_ARE_EQUAL(winrt::hstring{ expected }, actual, + NoThrowString{}.Format(L"input=[%.*s] expected=[%.*s] actual=[%s]", + static_cast(input.size()), input.data(), + static_cast(expected.size()), expected.data(), + actual.c_str())); + } + }; + + void CustomAgentIdTests::BareName() + { + Check(L"helper", L"helper"); + } + + void CustomAgentIdTests::NameWithExe() + { + Check(L"helper.exe", L"helper"); + } + + void CustomAgentIdTests::NameWithCmd() + { + Check(L"helper.cmd", L"helper"); + } + + void CustomAgentIdTests::NameWithBat() + { + Check(L"helper.bat", L"helper"); + } + + void CustomAgentIdTests::ExtensionStripIsCaseInsensitive() + { + Check(L"helper.EXE", L"helper"); + Check(L"helper.Cmd", L"helper"); + Check(L"helper.BAT", L"helper"); + Check(L"helper.cMd --acp", L"helper"); + } + + void CustomAgentIdTests::NameWithArgs() + { + Check(L"helper.cmd --acp", L"helper"); + Check(L"helper --acp --stdio", L"helper"); + } + + void CustomAgentIdTests::UnquotedPath() + { + Check(L"C:\\tools\\helper.cmd", L"helper"); + Check(L"C:\\tools\\helper.cmd --acp", L"helper"); + Check(L"D:\\local-bin\\my-agent.exe", L"my-agent"); + } + + void CustomAgentIdTests::QuotedPathWithSpaces() + { + // Full path containing spaces, properly quoted — the whole quoted + // region is the executable. + Check(L"\"C:\\Program Files\\helper\\helper.cmd\"", L"helper"); + } + + void CustomAgentIdTests::QuotedPathWithSpacesAndArgs() + { + Check(L"\"C:\\Program Files\\helper\\helper.cmd\" --acp", L"helper"); + Check(L"\"C:\\Program Files (x86)\\my agent\\my-agent.exe\" --stdio --acp", + L"my-agent"); + } + + void CustomAgentIdTests::ForwardSlashPath() + { + // POSIX-style forward slashes (some users paste paths like this). + Check(L"/usr/bin/helper", L"helper"); + Check(L"C:/tools/helper.cmd --acp", L"helper"); + } + + void CustomAgentIdTests::LeadingWhitespace() + { + Check(L" helper", L"helper"); + Check(L" helper.cmd --acp", L"helper"); + Check(L"\thelper.cmd", L"helper"); + } + + void CustomAgentIdTests::TabSeparator() + { + // Tab between exe and args. + Check(L"helper.cmd\t--acp", L"helper"); + } + + void CustomAgentIdTests::Empty() + { + Check(L"", L""); + } + + void CustomAgentIdTests::WhitespaceOnly() + { + Check(L" ", L""); + Check(L"\t\t", L""); + Check(L" \t ", L""); + } + + void CustomAgentIdTests::UnclosedQuote() + { + // Missing closing quote — take everything after the opening quote. + // Whatever the user typed is at least a recognizable token, not a crash. + Check(L"\"C:\\Program Files\\helper\\helper.cmd", L"helper"); + } + + void CustomAgentIdTests::QuoteOnlyIsEmpty() + { + // Just a single quote: the token after it is empty. + Check(L"\"", L""); + } + + void CustomAgentIdTests::EmptyQuotedIsEmpty() + { + // "" : empty quoted region. + Check(L"\"\"", L""); + Check(L"\"\" --acp", L""); + } + + void CustomAgentIdTests::BuiltInAgentNameStillExtracts() + { + // If a user types a built-in name (`copilot`, `gemini`, ...), the + // function still extracts it. The caller's responsibility is to + // notice the collision and append " (custom)" to the display name + // — DeriveCustomAgentId itself does not enforce uniqueness. + Check(L"copilot", L"copilot"); + Check(L"gemini.cmd --acp", L"gemini"); + } + + void CustomAgentIdTests::PathWithSpacesAndExtensionStrip() + { + // Exercise both the quoted-path branch AND the extension strip. + Check(L"\"C:\\Program Files\\Tools\\foo.EXE\" arg1", L"foo"); + Check(L"\"C:\\foo bar\\baz.CMD\"", L"baz"); + } + + void CustomAgentIdTests::MixedSlashesUsesLastSeparator() + { + // The function strips at the *last* `\` or `/` (find_last_of), so + // mixed paths are handled. + Check(L"C:/foo\\bar/helper.cmd", L"helper"); + Check(L"C:\\foo/bar\\helper.exe", L"helper"); + } + + void CustomAgentIdTests::NoExtensionStripWhenTokenEqualsExtension() + { + // The strip guard is `token.size() > extLen`, so an extension-only + // filename (".exe", ".cmd") is returned verbatim, not collapsed to "". + Check(L".exe", L".exe"); + Check(L".cmd", L".cmd"); + } + + void CustomAgentIdTests::DoesNotStripUnknownExtension() + { + // We only strip .exe / .cmd / .bat. Other extensions are part of + // the id (e.g. PowerShell scripts). + Check(L"helper.ps1", L"helper.ps1"); + Check(L"helper.py", L"helper.py"); + Check(L"helper.sh", L"helper.sh"); + } +} diff --git a/src/cascadia/ut_app/TerminalApp.UnitTests.vcxproj b/src/cascadia/ut_app/TerminalApp.UnitTests.vcxproj index f89d8c02b7..fa8f1e0b5a 100644 --- a/src/cascadia/ut_app/TerminalApp.UnitTests.vcxproj +++ b/src/cascadia/ut_app/TerminalApp.UnitTests.vcxproj @@ -24,6 +24,7 @@ + Create