ClaudeDo/tests/ClaudeDo.Worker.Tests/External/ExternalMcpToolSchemaTests.cs

using System.ComponentModel;
using System.Reflection;
using ClaudeDo.Worker.External;
using Microsoft.Extensions.AI;
using ModelContextProtocol.Server;

namespace ClaudeDo.Worker.Tests.External;

/// <summary>
/// MCP clients routinely omit optional arguments. The generated tool schema only marks a
/// parameter optional when the C# method declares a default value — nullability alone is not
/// enough (verified against ModelContextProtocol/Microsoft.Extensions.AI 1.2.0 / 10.4.1's
/// AIJsonUtilities.CreateFunctionJsonSchema, which checks ParameterInfo.IsOptional). This sweeps
/// every [McpServerToolType] class in the External/ namespace so a future tool can't reintroduce
/// a nullable-but-required filter parameter.
/// </summary>
public sealed class ExternalMcpToolSchemaTests
{
    private static IEnumerable<MethodInfo> ExternalToolMethods()
    {
        var toolTypes = typeof(ExternalMcpService).Assembly.GetTypes()
            .Where(t => t.Namespace == typeof(ExternalMcpService).Namespace
                        && t.GetCustomAttribute<McpServerToolTypeAttribute>() is not null);

        foreach (var type in toolTypes)
        foreach (var method in type.GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly))
        {
            if (method.GetCustomAttribute<McpServerToolAttribute>() is not null)
                yield return method;
        }
    }

    [Fact]
    public void NoExternalTool_HasARequiredNullableParameter()
    {
        var nullabilityContext = new NullabilityInfoContext();
        var violations = new List<string>();

        foreach (var method in ExternalToolMethods())
        {
            var schema = AIJsonUtilities.CreateFunctionJsonSchema(method);
            var required = schema.TryGetProperty("required", out var requiredElement)
                ? requiredElement.EnumerateArray().Select(e => e.GetString()).ToHashSet()
                : new HashSet<string?>();

            foreach (var parameter in method.GetParameters())
            {
                if (parameter.ParameterType == typeof(CancellationToken)) continue;
                if (!required.Contains(parameter.Name)) continue;

                var isNullableValueType = Nullable.GetUnderlyingType(parameter.ParameterType) is not null;
                var isNullableRefType = !parameter.ParameterType.IsValueType
                    && nullabilityContext.Create(parameter).WriteState == NullabilityState.Nullable;

                if (isNullableValueType || isNullableRefType)
                {
                    violations.Add(
                        $"{method.DeclaringType!.Name}.{method.Name}({parameter.Name}) is a nullable " +
                        "type but has no default value, so MCP clients omitting it will fail. " +
                        "Give it a default value (e.g. '= null').");
                }
            }
        }

        Assert.True(violations.Count == 0, string.Join(Environment.NewLine, violations));
    }

    [Fact]
    public void ExternalToolMethods_AreDiscovered()
    {
        // Guards the sweep itself: if this drops to 0, ExternalToolMethods() broke silently
        // (e.g. namespace/attribute mismatch) and the schema test above would pass vacuously.
        Assert.True(ExternalToolMethods().Count() > 20);
    }

    [Fact]
    public void AtLeastOneTaskIdTool_DescriptionCarriesTaskNumberHint()
    {
        // The whole point of task numbers: without this clause the agent never learns to speak
        // #<number> to the user, even though every DTO already carries it.
        var hasHint = ExternalToolMethods()
            .Select(m => m.GetCustomAttribute<DescriptionAttribute>()?.Description ?? "")
            .Any(d => d.Contains(McpToolDocs.TaskNumberHint.Trim(), StringComparison.Ordinal));

        Assert.True(hasHint, "No external tool description carries McpToolDocs.TaskNumberHint.");
    }

    // Full inventory of registered external tool names, computed via the SDK's own
    // McpServerTool.Create (the same call Program.cs's WithTools<T>() makes internally) rather
    // than reimplementing the PascalCase-to-snake_case conversion — so this only breaks on an
    // actual rename/add/remove, not on a divergent reimplementation of the naming rule. Used to
    // prove that splitting ExternalMcpService.cs into topic files (MergeMcpTools, WorktreeMcpTools,
    // ...) is a byte-identical rename-preserving move: the set below must not change across that split.
    private static readonly string[] ExpectedToolNames =
    [
        "list_task_lists", "list_tasks", "get_task", "add_task", "update_task", "add_subtask",
        "update_task_status", "review_task", "run_task_now", "cancel_task", "delete_task",
        "get_task_worktree", "get_task_diff", "merge_task", "continue_merge", "verify_merges",
        "abort_merge", "get_merge_conflicts", "resolve_conflict_hunk", "preview_merge",
        "preview_merge_set", "revert_merge", "list_worktrees", "cleanup_task_worktree",
        "continue_task", "get_daily_prep_candidates", "set_my_day",
        "batch_get_tasks", "batch_add_tasks", "batch_update_task_status", "batch_cancel_tasks",
        "batch_delete_tasks", "batch_set_my_day", "batch_cleanup_task_worktrees",
        "create_list", "update_list", "delete_list",
        "get_list_config", "set_list_config", "set_task_config", "get_task_config", "get_effective_run_config",
        "list_runs", "get_run", "get_task_log",
        "list_agents",
        "reset_failed_task", "submit_task_for_review",
        "handoff_list_handler", "get_handoff_status",
        "get_app_settings",
        "wait_for_task_change",
        "get_queue_state",
        "add_task_attachment", "list_task_attachments", "remove_task_attachment",
        "save_finding",
        "add_note",
    ];

    [Fact]
    public void ExternalToolNames_MatchExpectedInventory()
    {
        // A throwaway, never-constructed instance per declaring type — McpServerTool.Create needs a
        // non-null target reference to bind the method to, but naming/schema derivation never
        // touches instance state, so skipping the real constructor (and its DI dependencies) is safe.
        var targets = new Dictionary<Type, object>();
        var actual = ExternalToolMethods()
            .Select(m =>
            {
                var target = targets.TryGetValue(m.DeclaringType!, out var t)
                    ? t
                    : targets[m.DeclaringType!] = System.Runtime.CompilerServices.RuntimeHelpers.GetUninitializedObject(m.DeclaringType!);
                return McpServerTool.Create(m, target).ProtocolTool.Name;
            })
            .OrderBy(n => n, StringComparer.Ordinal)
            .ToList();

        var expected = ExpectedToolNames.OrderBy(n => n, StringComparer.Ordinal).ToList();

        Assert.Equal(expected, actual);
    }
}