From 5a3f88d9cef6db4f529de7522eefffd33c3f1466 Mon Sep 17 00:00:00 2001 From: Yee Lam Lee Date: Fri, 29 May 2026 23:05:55 +0800 Subject: [PATCH 1/7] fix(settings): always prefix custom agent ids with `custom:` `SaveCustomAcpAgent` / `SaveCustomDelegateAgent` previously only added the `custom:` prefix when the derived bare id collided with a built-in agent name. For everything else (e.g. user types `qwen.cmd --acp`, bare id `qwen`) the bare id was stored verbatim in `AcpAgent` / `DelegateAgent`. That bare id breaks every downstream consumer that uses `custom:` as the discriminator: * `AIAgentsViewModel::_MaybeAppendCustomEntry` early-returns when the saved id does not start with `custom:`, so the rebuilt dropdown never surfaces the entry and `CurrentAcpAgent` falls back to the first built-in. This is why the AI Agents page reverts to Copilot right after clicking the page-level Save. * `GlobalAppSettings::Effective{Acp,Delegate}Agent` treats anything without the prefix as a built-in id and runs it through the `AllowedAgents` GPO allowlist, returning empty for unknown ids - downstream consumers then think no agent is selected. * `TerminalPage::_ResolveEffectiveAgentCliPath` only honours `AcpCustomCommand` when the id has the prefix; without it the launcher falls back to `_BuildAgentCommandLine`, which returns the bare id verbatim instead of the full custom command line. * `DeleteCustomAcpAgent` / `EditCustomAcpAgent` / `IsCustom*AgentSelected` / `CustomAcpCommandPreview` / `ShowAcpModel` all gate on the prefix - the user can neither edit nor delete the entry after reload. * `AllowCustomAgents` GPO is bypassed (policy code only inspects the prefix), and telemetry serializes the raw user-chosen binary name instead of the privacy-preserving `custom:` discriminator. Always prefix the saved id with `custom:` regardless of whether the bare id matches a built-in. The `displayName` branch is preserved so a custom override of a built-in still shows `copilot (custom)`. Also update `_MaybeAppendCustomEntry` to compute the same id so list rebuilds round-trip correctly. Note: users who already saved a bare id from a previous build keep the same broken UX until they re-add the agent (`EffectiveAcpAgent` was already returning empty for them); no auto-migration is needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AIAgentsViewModel.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp index 6b86ca406e..e5a71f14dc 100644 --- a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp +++ b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp @@ -100,9 +100,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; @@ -566,10 +565,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation const auto bareId = _DeriveId(_customAcpCommand); _GlobalSettings.AcpCustomCommand(_customAcpCommand); + // Custom agents always carry the "custom:" discriminator — every + // downstream consumer (EffectiveAcpAgent policy gate, command-line + // resolver, custom-edit/delete UI gates, telemetry) 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; @@ -603,10 +605,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation const auto bareId = _DeriveId(_customDelegateCommand); _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; From 01c2c0a4e921ef637703e5f0643e078c5b6395f7 Mon Sep 17 00:00:00 2001 From: Yee Lam Lee Date: Sat, 30 May 2026 15:50:45 +0800 Subject: [PATCH 2/7] Handle quoted full paths in _DeriveId for custom agents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user types a custom ACP command containing a quoted full path (e.g. "C:\Program Files\qwen\qwen.cmd" --acp), _DeriveId previously split on the first space — which fell inside the quotes — and produced a bogus id like "C:\Program. The derived id is what the settings page uses to build the saved custom: value and the dropdown label, so a broken id silently broke save/round-trip for any path containing a space (very common: Program Files, AppData\Local\..., user profile paths with spaces, etc.). Changes to _DeriveId: - Trim leading whitespace before parsing. - If the command begins with ", take everything up to the next " as the executable token (proper quoted-path handling). - Otherwise split on the first run of whitespace (space OR tab). - Make the trailing .exe/.cmd/.bat strip case-insensitive via _stricmp so paths like qwen.EXE work the same as qwen.exe. Verified by tracing 10 inputs (unquoted, quoted-with-spaces, mixed slashes, tab separator, mixed extension case, leading whitespace, empty, single-quote pathological, very long) — all produce the expected basename or return an empty hstring safely. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AIAgentsViewModel.cpp | 36 ++++++++++++++++--- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp index e5a71f14dc..8f2b9b3a2a 100644 --- a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp +++ b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp @@ -67,17 +67,43 @@ 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; + // Extract the executable token. If the command begins with a double-quote + // (e.g. "C:\Program Files\qwen\qwen.cmd" --acp), treat everything up to + // the next double-quote as the executable; otherwise split on the first + // whitespace. This ensures full paths containing spaces are handled. + auto str = winrt::to_string(command); + // Trim leading whitespace so a stray leading space doesn't confuse the parser. + const auto firstNonSpace = str.find_first_not_of(" \t"); + if (firstNonSpace == std::string::npos) + { + return winrt::hstring{}; + } + str.erase(0, firstNonSpace); + + std::string token; + if (!str.empty() && str.front() == '"') + { + const auto closing = str.find('"', 1); + token = (closing != std::string::npos) ? str.substr(1, closing - 1) : str.substr(1); + } + else + { + const auto pos = str.find_first_of(" \t"); + 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); + + // Case-insensitive trailing-extension strip. for (const auto* ext : { ".exe", ".cmd", ".bat" }) { - if (token.size() > strlen(ext) && token.substr(token.size() - strlen(ext)) == ext) + const auto extLen = strlen(ext); + if (token.size() > extLen && + _stricmp(token.c_str() + token.size() - extLen, ext) == 0) { - token = token.substr(0, token.size() - strlen(ext)); + token = token.substr(0, token.size() - extLen); break; } } From 06511fba912353a4b200dd2b97335f8b7202b695 Mon Sep 17 00:00:00 2001 From: Yee Lam Lee Date: Sat, 30 May 2026 16:29:35 +0800 Subject: [PATCH 3/7] test(settings): add unit tests for custom agent id + GPO matrix Extends PR #123 with coverage at two layers that were previously unguarded for the custom-agent save/load and GPO filter paths. Production refactor (no behavior change): - Extract AIAgentsViewModel::_DeriveId into header-only src/cascadia/inc/CustomAgentId.h so tests can call it without pulling in TerminalSettingsEditor.dll. - Add AgentPolicy::SetSnapshotForTest / ResetForTest seam + static GlobalAppSettings::_TestHookSetAgentPolicy forwarders so the injected snapshot lands in SettingsModel.dll (where EffectiveAcpAgent consults it). Tests: - ut_app/CustomAgentIdTests.cpp (22 cases): bare names, .exe/.cmd/.bat case-insensitive strip, quoted paths with spaces+args, forward/mixed slashes, leading whitespace, empty/quoted-empty, extension-only filename, unknown extensions (.ps1/.py/.sh) left intact, built-in collision. - UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp (19 cases): round-trip of acpAgent / delegateAgent custom: prefix (regression guard for #123), quoted Windows path round-trip, EffectiveAcpAgent and EffectiveDelegateAgent matrices across AllowedAgents (nullopt / allow / block / empty / case-insensitive) and AllowCustomAgents (NotConfigured / Allowed / Blocked), plus IsAgentPolicyLocked / IsCustomAgentPolicyLocked mirroring. Verified locally: 41/41 passing on x64 Debug via te.exe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AIAgentsViewModel.cpp | 45 +-- .../GlobalAppSettings.cpp | 15 + .../TerminalSettingsModel/GlobalAppSettings.h | 17 + .../CustomAgentAndPolicyTests.cpp | 316 ++++++++++++++++++ .../SettingsModel.UnitTests.vcxproj | 1 + src/cascadia/inc/AgentPolicy.h | 27 ++ src/cascadia/inc/CustomAgentId.h | 88 +++++ src/cascadia/ut_app/CustomAgentIdTests.cpp | 215 ++++++++++++ .../ut_app/TerminalApp.UnitTests.vcxproj | 1 + 9 files changed, 684 insertions(+), 41 deletions(-) create mode 100644 src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp create mode 100644 src/cascadia/inc/CustomAgentId.h create mode 100644 src/cascadia/ut_app/CustomAgentIdTests.cpp diff --git a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp index 8f2b9b3a2a..a7ba8635b1 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,47 +68,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation winrt::hstring AIAgentsViewModel::_DeriveId(const winrt::hstring& command) { - // Extract the executable token. If the command begins with a double-quote - // (e.g. "C:\Program Files\qwen\qwen.cmd" --acp), treat everything up to - // the next double-quote as the executable; otherwise split on the first - // whitespace. This ensures full paths containing spaces are handled. - auto str = winrt::to_string(command); - // Trim leading whitespace so a stray leading space doesn't confuse the parser. - const auto firstNonSpace = str.find_first_not_of(" \t"); - if (firstNonSpace == std::string::npos) - { - return winrt::hstring{}; - } - str.erase(0, firstNonSpace); - - std::string token; - if (!str.empty() && str.front() == '"') - { - const auto closing = str.find('"', 1); - token = (closing != std::string::npos) ? str.substr(1, closing - 1) : str.substr(1); - } - else - { - const auto pos = str.find_first_of(" \t"); - 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); - - // Case-insensitive trailing-extension strip. - for (const auto* ext : { ".exe", ".cmd", ".bat" }) - { - const auto extLen = strlen(ext); - if (token.size() > extLen && - _stricmp(token.c_str() + token.size() - extLen, ext) == 0) - { - token = token.substr(0, token.size() - extLen); - 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) 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..94974eaa74 --- /dev/null +++ b/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp @@ -0,0 +1,316 @@ +// 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 (the EffectiveAcpAgent policy gate, the +// launcher's command-line resolver, the custom-edit/delete UI +// gates, and telemetry). 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:qwen", "acpCustomCommand": "qwen.cmd --acp")"); + const auto& globals = settings->GlobalSettings(); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:qwen" }, globals.AcpAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"qwen.cmd --acp" }, globals.AcpCustomCommand()); + } + + void CustomAgentAndPolicyTests::CustomDelegateAgentRoundtrips() + { + const auto settings = MakeSettings(R"("delegateAgent": "custom:qwen", "delegateCustomCommand": "qwen.cmd --acp")"); + const auto& globals = settings->GlobalSettings(); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:qwen" }, globals.DelegateAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"qwen.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:qwen", "acpCustomCommand": "\"C:\\Program Files\\qwen\\qwen.cmd\" --acp")"); + const auto& globals = settings->GlobalSettings(); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:qwen" }, globals.AcpAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ LR"("C:\Program Files\qwen\qwen.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:qwen", "acpCustomCommand": "qwen.cmd")"); + SetPolicy(MakePolicy(/*allowedAgents*/ std::nullopt, AgentPolicy::PolicyState::NotConfigured)); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:qwen" }, settings->GlobalSettings().EffectiveAcpAgent()); + } + + void CustomAgentAndPolicyTests::EffectiveAcpAgentCustomBlockedByCustomPolicy() + { + const auto settings = MakeSettings(R"("acpAgent": "custom:qwen", "acpCustomCommand": "qwen.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:qwen", "acpCustomCommand": "qwen.cmd")"); + SetPolicy(MakePolicy(std::set{ L"gemini" }, + AgentPolicy::PolicyState::NotConfigured)); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:qwen" }, 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:qwen", "delegateCustomCommand": "qwen.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:qwen", "delegateCustomCommand": "qwen.cmd")"); + SetPolicy(MakePolicy(std::set{ L"gemini" }, + AgentPolicy::PolicyState::NotConfigured)); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:qwen" }, 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..9418a80f42 --- /dev/null +++ b/src/cascadia/inc/CustomAgentId.h @@ -0,0 +1,88 @@ +// 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. `qwen.cmd +// --acp`, `"C:\Program Files\qwen\qwen.cmd" --acp`, or just `qwen`). 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, telemetry, 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, the +// custom-edit/delete UI gates, and telemetry. Storing a bare id silently +// breaks all of them; 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..b5488573bb --- /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. `qwen.cmd --acp`, `"C:\Program +// Files\qwen\qwen.cmd" --acp`) into the short token that becomes the +// suffix of the stored agent id (e.g. `custom:qwen`). Every downstream +// consumer (EffectiveAcpAgent policy gate, command-line resolver, +// custom-edit/delete UI gates, telemetry) keys on the resulting id; +// 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"qwen", L"qwen"); + } + + void CustomAgentIdTests::NameWithExe() + { + Check(L"qwen.exe", L"qwen"); + } + + void CustomAgentIdTests::NameWithCmd() + { + Check(L"qwen.cmd", L"qwen"); + } + + void CustomAgentIdTests::NameWithBat() + { + Check(L"qwen.bat", L"qwen"); + } + + void CustomAgentIdTests::ExtensionStripIsCaseInsensitive() + { + Check(L"qwen.EXE", L"qwen"); + Check(L"qwen.Cmd", L"qwen"); + Check(L"qwen.BAT", L"qwen"); + Check(L"qwen.cMd --acp", L"qwen"); + } + + void CustomAgentIdTests::NameWithArgs() + { + Check(L"qwen.cmd --acp", L"qwen"); + Check(L"qwen --acp --stdio", L"qwen"); + } + + void CustomAgentIdTests::UnquotedPath() + { + Check(L"C:\\tools\\qwen.cmd", L"qwen"); + Check(L"C:\\tools\\qwen.cmd --acp", L"qwen"); + 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\\qwen\\qwen.cmd\"", L"qwen"); + } + + void CustomAgentIdTests::QuotedPathWithSpacesAndArgs() + { + Check(L"\"C:\\Program Files\\qwen\\qwen.cmd\" --acp", L"qwen"); + 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/qwen", L"qwen"); + Check(L"C:/tools/qwen.cmd --acp", L"qwen"); + } + + void CustomAgentIdTests::LeadingWhitespace() + { + Check(L" qwen", L"qwen"); + Check(L" qwen.cmd --acp", L"qwen"); + Check(L"\tqwen.cmd", L"qwen"); + } + + void CustomAgentIdTests::TabSeparator() + { + // Tab between exe and args. + Check(L"qwen.cmd\t--acp", L"qwen"); + } + + 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\\qwen\\qwen.cmd", L"qwen"); + } + + 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/qwen.cmd", L"qwen"); + Check(L"C:\\foo/bar\\qwen.exe", L"qwen"); + } + + 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"qwen.ps1", L"qwen.ps1"); + Check(L"qwen.py", L"qwen.py"); + Check(L"qwen.sh", L"qwen.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 From 43b1bab9e1fe01542d70fb11d1feaac48ee76ed1 Mon Sep 17 00:00:00 2001 From: Yee Lam Lee Date: Sat, 30 May 2026 17:08:27 +0800 Subject: [PATCH 4/7] fix(settings): reject empty custom agent ids; align comment with consumers Address Copilot review feedback on PR #123: - SaveCustomAcpAgent / SaveCustomDelegateAgent now bail out when DeriveCustomAgentId returns an empty id (whitespace-only or quote-only commands), so the UI cannot persist a bare `custom:` entry that would leave a blank, unusable custom agent selected. - Drop the stale `telemetry` mention from the consumer list comment in SaveCustomAcpAgent; telemetry sanitizes any non-built-in id to the literal `custom` and does not key on the prefix. - Rename `qwen` to `mybot` in CustomAgentAndPolicyTests.cpp to avoid check-spelling alerts without adding to the expect list. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AIAgentsViewModel.cpp | 12 +++++-- .../CustomAgentAndPolicyTests.cpp | 34 +++++++++---------- 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp index a7ba8635b1..8b0cce9846 100644 --- a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp +++ b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp @@ -552,13 +552,17 @@ 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, telemetry) 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. + // 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 = winrt::hstring{ L"custom:" + std::wstring_view{ bareId } }; const auto displayName = isBuiltIn @@ -592,6 +596,8 @@ 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. diff --git a/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp b/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp index 94974eaa74..e3bbad46ca 100644 --- a/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp @@ -131,18 +131,18 @@ namespace SettingsModelUnitTests // 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:qwen", "acpCustomCommand": "qwen.cmd --acp")"); + const auto settings = MakeSettings(R"("acpAgent": "custom:mybot", "acpCustomCommand": "mybot.cmd --acp")"); const auto& globals = settings->GlobalSettings(); - VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:qwen" }, globals.AcpAgent()); - VERIFY_ARE_EQUAL(winrt::hstring{ L"qwen.cmd --acp" }, globals.AcpCustomCommand()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:mybot" }, globals.AcpAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"mybot.cmd --acp" }, globals.AcpCustomCommand()); } void CustomAgentAndPolicyTests::CustomDelegateAgentRoundtrips() { - const auto settings = MakeSettings(R"("delegateAgent": "custom:qwen", "delegateCustomCommand": "qwen.cmd --acp")"); + const auto settings = MakeSettings(R"("delegateAgent": "custom:mybot", "delegateCustomCommand": "mybot.cmd --acp")"); const auto& globals = settings->GlobalSettings(); - VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:qwen" }, globals.DelegateAgent()); - VERIFY_ARE_EQUAL(winrt::hstring{ L"qwen.cmd --acp" }, globals.DelegateCustomCommand()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:mybot" }, globals.DelegateAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"mybot.cmd --acp" }, globals.DelegateCustomCommand()); } void CustomAgentAndPolicyTests::QuotedPathCustomCommandRoundtrips() @@ -151,10 +151,10 @@ namespace SettingsModelUnitTests // are common for users on the Windows installer paths. Make sure // the parser preserves them verbatim. const auto settings = MakeSettings( - R"("acpAgent": "custom:qwen", "acpCustomCommand": "\"C:\\Program Files\\qwen\\qwen.cmd\" --acp")"); + R"("acpAgent": "custom:mybot", "acpCustomCommand": "\"C:\\Program Files\\mybot\\mybot.cmd\" --acp")"); const auto& globals = settings->GlobalSettings(); - VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:qwen" }, globals.AcpAgent()); - VERIFY_ARE_EQUAL(winrt::hstring{ LR"("C:\Program Files\qwen\qwen.cmd" --acp)" }, + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:mybot" }, globals.AcpAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ LR"("C:\Program Files\mybot\mybot.cmd" --acp)" }, globals.AcpCustomCommand()); } @@ -215,14 +215,14 @@ namespace SettingsModelUnitTests void CustomAgentAndPolicyTests::EffectiveAcpAgentCustomPassesWhenNoCustomPolicy() { - const auto settings = MakeSettings(R"("acpAgent": "custom:qwen", "acpCustomCommand": "qwen.cmd")"); + const auto settings = MakeSettings(R"("acpAgent": "custom:mybot", "acpCustomCommand": "mybot.cmd")"); SetPolicy(MakePolicy(/*allowedAgents*/ std::nullopt, AgentPolicy::PolicyState::NotConfigured)); - VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:qwen" }, settings->GlobalSettings().EffectiveAcpAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:mybot" }, settings->GlobalSettings().EffectiveAcpAgent()); } void CustomAgentAndPolicyTests::EffectiveAcpAgentCustomBlockedByCustomPolicy() { - const auto settings = MakeSettings(R"("acpAgent": "custom:qwen", "acpCustomCommand": "qwen.cmd")"); + const auto settings = MakeSettings(R"("acpAgent": "custom:mybot", "acpCustomCommand": "mybot.cmd")"); SetPolicy(MakePolicy(/*allowedAgents*/ std::nullopt, AgentPolicy::PolicyState::Blocked)); VERIFY_ARE_EQUAL(winrt::hstring{}, settings->GlobalSettings().EffectiveAcpAgent()); } @@ -235,10 +235,10 @@ namespace SettingsModelUnitTests // 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:qwen", "acpCustomCommand": "qwen.cmd")"); + const auto settings = MakeSettings(R"("acpAgent": "custom:mybot", "acpCustomCommand": "mybot.cmd")"); SetPolicy(MakePolicy(std::set{ L"gemini" }, AgentPolicy::PolicyState::NotConfigured)); - VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:qwen" }, settings->GlobalSettings().EffectiveAcpAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:mybot" }, settings->GlobalSettings().EffectiveAcpAgent()); } // ── EffectiveDelegateAgent ────────────────────────────────────────── @@ -266,17 +266,17 @@ namespace SettingsModelUnitTests void CustomAgentAndPolicyTests::EffectiveDelegateAgentCustomBlockedByCustomPolicy() { - const auto settings = MakeSettings(R"("delegateAgent": "custom:qwen", "delegateCustomCommand": "qwen.cmd")"); + const auto settings = MakeSettings(R"("delegateAgent": "custom:mybot", "delegateCustomCommand": "mybot.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:qwen", "delegateCustomCommand": "qwen.cmd")"); + const auto settings = MakeSettings(R"("delegateAgent": "custom:mybot", "delegateCustomCommand": "mybot.cmd")"); SetPolicy(MakePolicy(std::set{ L"gemini" }, AgentPolicy::PolicyState::NotConfigured)); - VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:qwen" }, settings->GlobalSettings().EffectiveDelegateAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:mybot" }, settings->GlobalSettings().EffectiveDelegateAgent()); } // ── Lock-state ────────────────────────────────────────────────────── From 24cb0d8308bde5a282dba2a727a453be04ea8559 Mon Sep 17 00:00:00 2001 From: Yee Lam Lee Date: Sat, 30 May 2026 17:12:11 +0800 Subject: [PATCH 5/7] docs(custom-agent): drop telemetry from prefix consumer list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Telemetry's sanitizeProviderId collapses every non-built-in id to literal `custom` without checking the `custom:` prefix. Update CustomAgentId.h to call out that the prefix matters for EffectiveAcpAgent, the resolver, and custom-edit/delete UI gates only — and that telemetry deliberately does not depend on it. Also rename the `qwen` example to `mybot` for consistency with the test fixtures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/cascadia/inc/CustomAgentId.h | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/cascadia/inc/CustomAgentId.h b/src/cascadia/inc/CustomAgentId.h index 9418a80f42..0f8414f6ce 100644 --- a/src/cascadia/inc/CustomAgentId.h +++ b/src/cascadia/inc/CustomAgentId.h @@ -4,13 +4,13 @@ // 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. `qwen.cmd -// --acp`, `"C:\Program Files\qwen\qwen.cmd" --acp`, or just `qwen`). The -// settings model stores this command verbatim in AcpCustomCommand / +// The settings UI lets the user paste an arbitrary command (e.g. `mybot.cmd +// --acp`, `"C:\Program Files\mybot\mybot.cmd" --acp`, or just `mybot`). +// 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, telemetry, display name) has something -// stable to key on. +// pipeline (policy allowlist, display name) has something stable to key +// on. // // `DeriveCustomAgentId` performs that extraction: // 1. Trim leading whitespace. @@ -24,9 +24,11 @@ // 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, the -// custom-edit/delete UI gates, and telemetry. Storing a bare id silently -// breaks all of them; see PR #123. +// 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 From 32b44109b1af7586332873512cb3ed324f40267a Mon Sep 17 00:00:00 2001 From: Yee Lam Lee Date: Sat, 30 May 2026 17:18:35 +0800 Subject: [PATCH 6/7] docs(tests): drop telemetry from custom-agent test header comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Match the prior cleanup in CustomAgentId.h (24cb0d830) — the test file headers should not claim telemetry depends on the custom: prefix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../CustomAgentAndPolicyTests.cpp | 9 +++++---- src/cascadia/ut_app/CustomAgentIdTests.cpp | 14 +++++++------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp b/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp index e3bbad46ca..f70a6fcf1d 100644 --- a/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp @@ -8,10 +8,11 @@ // // 1. JSON round-trip of the custom-agent settings. A regression that // drops the "custom:" prefix from AcpAgent / DelegateAgent breaks -// every downstream consumer (the EffectiveAcpAgent policy gate, the -// launcher's command-line resolver, the custom-edit/delete UI -// gates, and telemetry). The first half of this file asserts that -// these settings survive an unmodified load. +// 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 diff --git a/src/cascadia/ut_app/CustomAgentIdTests.cpp b/src/cascadia/ut_app/CustomAgentIdTests.cpp index b5488573bb..0eca9be882 100644 --- a/src/cascadia/ut_app/CustomAgentIdTests.cpp +++ b/src/cascadia/ut_app/CustomAgentIdTests.cpp @@ -6,13 +6,13 @@ // 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. `qwen.cmd --acp`, `"C:\Program -// Files\qwen\qwen.cmd" --acp`) into the short token that becomes the -// suffix of the stored agent id (e.g. `custom:qwen`). Every downstream -// consumer (EffectiveAcpAgent policy gate, command-line resolver, -// custom-edit/delete UI gates, telemetry) keys on the resulting id; -// regressing this function silently breaks the save/reload round-trip -// (PR #123) or the launcher. +// user-supplied command line (e.g. `mybot.cmd --acp`, `"C:\Program +// Files\mybot\mybot.cmd" --acp`) into the short token that becomes the +// suffix of the stored agent id (e.g. `custom:mybot`). 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" From 447cfaa493abe3913b9be6694dc4b3dce9dd1e20 Mon Sep 17 00:00:00 2001 From: Yee Lam Lee Date: Sat, 30 May 2026 17:35:17 +0800 Subject: [PATCH 7/7] test(spelling): use real word 'helper' as custom-agent placeholder The earlier 'mybot' placeholder was still flagged by check-spelling because it is not a real English word. Replace remaining 'qwen' occurrences in CustomAgentIdTests.cpp (which were missed in 24cb0d830) and the 'mybot' occurrences in CustomAgentId.h, CustomAgentIdTests.cpp, and CustomAgentAndPolicyTests.cpp with the real word 'helper'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../CustomAgentAndPolicyTests.cpp | 34 +++++------ src/cascadia/inc/CustomAgentId.h | 4 +- src/cascadia/ut_app/CustomAgentIdTests.cpp | 58 +++++++++---------- 3 files changed, 48 insertions(+), 48 deletions(-) diff --git a/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp b/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp index f70a6fcf1d..3ade8f9ae9 100644 --- a/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp @@ -132,18 +132,18 @@ namespace SettingsModelUnitTests // 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:mybot", "acpCustomCommand": "mybot.cmd --acp")"); + const auto settings = MakeSettings(R"("acpAgent": "custom:helper", "acpCustomCommand": "helper.cmd --acp")"); const auto& globals = settings->GlobalSettings(); - VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:mybot" }, globals.AcpAgent()); - VERIFY_ARE_EQUAL(winrt::hstring{ L"mybot.cmd --acp" }, globals.AcpCustomCommand()); + 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:mybot", "delegateCustomCommand": "mybot.cmd --acp")"); + const auto settings = MakeSettings(R"("delegateAgent": "custom:helper", "delegateCustomCommand": "helper.cmd --acp")"); const auto& globals = settings->GlobalSettings(); - VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:mybot" }, globals.DelegateAgent()); - VERIFY_ARE_EQUAL(winrt::hstring{ L"mybot.cmd --acp" }, globals.DelegateCustomCommand()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:helper" }, globals.DelegateAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"helper.cmd --acp" }, globals.DelegateCustomCommand()); } void CustomAgentAndPolicyTests::QuotedPathCustomCommandRoundtrips() @@ -152,10 +152,10 @@ namespace SettingsModelUnitTests // are common for users on the Windows installer paths. Make sure // the parser preserves them verbatim. const auto settings = MakeSettings( - R"("acpAgent": "custom:mybot", "acpCustomCommand": "\"C:\\Program Files\\mybot\\mybot.cmd\" --acp")"); + R"("acpAgent": "custom:helper", "acpCustomCommand": "\"C:\\Program Files\\helper\\helper.cmd\" --acp")"); const auto& globals = settings->GlobalSettings(); - VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:mybot" }, globals.AcpAgent()); - VERIFY_ARE_EQUAL(winrt::hstring{ LR"("C:\Program Files\mybot\mybot.cmd" --acp)" }, + 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()); } @@ -216,14 +216,14 @@ namespace SettingsModelUnitTests void CustomAgentAndPolicyTests::EffectiveAcpAgentCustomPassesWhenNoCustomPolicy() { - const auto settings = MakeSettings(R"("acpAgent": "custom:mybot", "acpCustomCommand": "mybot.cmd")"); + 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:mybot" }, settings->GlobalSettings().EffectiveAcpAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:helper" }, settings->GlobalSettings().EffectiveAcpAgent()); } void CustomAgentAndPolicyTests::EffectiveAcpAgentCustomBlockedByCustomPolicy() { - const auto settings = MakeSettings(R"("acpAgent": "custom:mybot", "acpCustomCommand": "mybot.cmd")"); + 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()); } @@ -236,10 +236,10 @@ namespace SettingsModelUnitTests // 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:mybot", "acpCustomCommand": "mybot.cmd")"); + 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:mybot" }, settings->GlobalSettings().EffectiveAcpAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:helper" }, settings->GlobalSettings().EffectiveAcpAgent()); } // ── EffectiveDelegateAgent ────────────────────────────────────────── @@ -267,17 +267,17 @@ namespace SettingsModelUnitTests void CustomAgentAndPolicyTests::EffectiveDelegateAgentCustomBlockedByCustomPolicy() { - const auto settings = MakeSettings(R"("delegateAgent": "custom:mybot", "delegateCustomCommand": "mybot.cmd")"); + 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:mybot", "delegateCustomCommand": "mybot.cmd")"); + 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:mybot" }, settings->GlobalSettings().EffectiveDelegateAgent()); + VERIFY_ARE_EQUAL(winrt::hstring{ L"custom:helper" }, settings->GlobalSettings().EffectiveDelegateAgent()); } // ── Lock-state ────────────────────────────────────────────────────── diff --git a/src/cascadia/inc/CustomAgentId.h b/src/cascadia/inc/CustomAgentId.h index 0f8414f6ce..bddf84de5e 100644 --- a/src/cascadia/inc/CustomAgentId.h +++ b/src/cascadia/inc/CustomAgentId.h @@ -4,8 +4,8 @@ // 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. `mybot.cmd -// --acp`, `"C:\Program Files\mybot\mybot.cmd" --acp`, or just `mybot`). +// 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 diff --git a/src/cascadia/ut_app/CustomAgentIdTests.cpp b/src/cascadia/ut_app/CustomAgentIdTests.cpp index 0eca9be882..57dc065005 100644 --- a/src/cascadia/ut_app/CustomAgentIdTests.cpp +++ b/src/cascadia/ut_app/CustomAgentIdTests.cpp @@ -6,9 +6,9 @@ // 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. `mybot.cmd --acp`, `"C:\Program -// Files\mybot\mybot.cmd" --acp`) into the short token that becomes the -// suffix of the stored agent id (e.g. `custom:mybot`). Every downstream +// 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 @@ -66,42 +66,42 @@ namespace TerminalAppUnitTests void CustomAgentIdTests::BareName() { - Check(L"qwen", L"qwen"); + Check(L"helper", L"helper"); } void CustomAgentIdTests::NameWithExe() { - Check(L"qwen.exe", L"qwen"); + Check(L"helper.exe", L"helper"); } void CustomAgentIdTests::NameWithCmd() { - Check(L"qwen.cmd", L"qwen"); + Check(L"helper.cmd", L"helper"); } void CustomAgentIdTests::NameWithBat() { - Check(L"qwen.bat", L"qwen"); + Check(L"helper.bat", L"helper"); } void CustomAgentIdTests::ExtensionStripIsCaseInsensitive() { - Check(L"qwen.EXE", L"qwen"); - Check(L"qwen.Cmd", L"qwen"); - Check(L"qwen.BAT", L"qwen"); - Check(L"qwen.cMd --acp", L"qwen"); + 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"qwen.cmd --acp", L"qwen"); - Check(L"qwen --acp --stdio", L"qwen"); + Check(L"helper.cmd --acp", L"helper"); + Check(L"helper --acp --stdio", L"helper"); } void CustomAgentIdTests::UnquotedPath() { - Check(L"C:\\tools\\qwen.cmd", L"qwen"); - Check(L"C:\\tools\\qwen.cmd --acp", L"qwen"); + 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"); } @@ -109,12 +109,12 @@ namespace TerminalAppUnitTests { // Full path containing spaces, properly quoted — the whole quoted // region is the executable. - Check(L"\"C:\\Program Files\\qwen\\qwen.cmd\"", L"qwen"); + Check(L"\"C:\\Program Files\\helper\\helper.cmd\"", L"helper"); } void CustomAgentIdTests::QuotedPathWithSpacesAndArgs() { - Check(L"\"C:\\Program Files\\qwen\\qwen.cmd\" --acp", L"qwen"); + 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"); } @@ -122,21 +122,21 @@ namespace TerminalAppUnitTests void CustomAgentIdTests::ForwardSlashPath() { // POSIX-style forward slashes (some users paste paths like this). - Check(L"/usr/bin/qwen", L"qwen"); - Check(L"C:/tools/qwen.cmd --acp", L"qwen"); + Check(L"/usr/bin/helper", L"helper"); + Check(L"C:/tools/helper.cmd --acp", L"helper"); } void CustomAgentIdTests::LeadingWhitespace() { - Check(L" qwen", L"qwen"); - Check(L" qwen.cmd --acp", L"qwen"); - Check(L"\tqwen.cmd", L"qwen"); + 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"qwen.cmd\t--acp", L"qwen"); + Check(L"helper.cmd\t--acp", L"helper"); } void CustomAgentIdTests::Empty() @@ -155,7 +155,7 @@ namespace TerminalAppUnitTests { // 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\\qwen\\qwen.cmd", L"qwen"); + Check(L"\"C:\\Program Files\\helper\\helper.cmd", L"helper"); } void CustomAgentIdTests::QuoteOnlyIsEmpty() @@ -192,8 +192,8 @@ namespace TerminalAppUnitTests { // The function strips at the *last* `\` or `/` (find_last_of), so // mixed paths are handled. - Check(L"C:/foo\\bar/qwen.cmd", L"qwen"); - Check(L"C:\\foo/bar\\qwen.exe", L"qwen"); + Check(L"C:/foo\\bar/helper.cmd", L"helper"); + Check(L"C:\\foo/bar\\helper.exe", L"helper"); } void CustomAgentIdTests::NoExtensionStripWhenTokenEqualsExtension() @@ -208,8 +208,8 @@ namespace TerminalAppUnitTests { // We only strip .exe / .cmd / .bat. Other extensions are part of // the id (e.g. PowerShell scripts). - Check(L"qwen.ps1", L"qwen.ps1"); - Check(L"qwen.py", L"qwen.py"); - Check(L"qwen.sh", L"qwen.sh"); + Check(L"helper.ps1", L"helper.ps1"); + Check(L"helper.py", L"helper.py"); + Check(L"helper.sh", L"helper.sh"); } }