feat(triggers): skill triggers fire from ModernUO SkillEvents.SkillUsed; XmlSpawner outcome and value-window semantics

CLAUDE.md

@@ -61,7 +61,8 @@ ModernSpawner-specific:
   hooks (`OnStarted`, `OnSpawned`, `OnSpawnedDeath`, entry-aware `GetSpawnPosition`) carry the modern
   behaviour. Never add a parallel entry list or hide base members with `new`.
 - Triggers register through `TriggerSystem`; proximity uses `Item.HandlesOnMovement`/`OnMovement`, speech
-  uses `HandlesOnSpeech`. Extended (beyond 24-tile) proximity is stubbed pending a ModernUO area-movement API.
+  uses `HandlesOnSpeech`, skill uses `Server.Misc.SkillEvents.SkillUsed` (players only). Extended (beyond
+  24-tile) proximity is stubbed pending a ModernUO area-movement API.
 - Trigger list changes go through the generated helpers (`AddToTriggerDefinitions`,
   `RemoveFromTriggerDefinitionsAt`, `ClearTriggerDefinitions`), then call `EnsureTriggersActive()`; the
   `TriggerActivated` setter does this for you. Never call `TriggerSystem.ActivateTriggers` directly — it is

Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs

@@ -182,4 +182,50 @@ public void SpawnPointMigrator_RunningFalse_ProducesStoppedSpawner()
         Assert.True(running.Running);
         running.Delete();
     }
+
+    [Theory]
+    [InlineData("Mining", "skill:Mining:8:0:False:5")]
+    [InlineData("Mining+", "skill:Mining+:8:0:False:5")]
+    [InlineData("Magery-,50,90", "skill:Magery-:8:50-90:False:5")]
+    // An absent min is 0, the reading XmlSpawner itself gave "SkillName,,max".
+    [InlineData("Magery,,90", "skill:Magery:8:0-90:False:5")]
+    public void Migrator_MapsSkillTriggerAttribute(string xml, string expected)
+    {
+        var node = ParseNode($"<Point X=\"1500\" Y=\"1500\" Z=\"0\" Map=\"Felucca\" Running=\"false\" ProximityRange=\"8\" SkillTrigger=\"{xml}\" />");
+        var spawner = XmlSpawnerMigrator.ParseXmlSpawnerNode(node);
+        try
+        {
+            Assert.Contains(expected, spawner.TriggerDefinitions);
+        }
+        finally
+        {
+            spawner.Delete();
+        }
+    }
+
+    [Theory]
+    // An unknown skill name, and a window whose max is below its min: both are logged and dropped, so no
+    // skill definition reaches the spawner. TriggerActivated still comes from ProximityRange on the node,
+    // which proves the rejected attribute neither set it nor cleared it.
+    [InlineData("NotASkill")]
+    [InlineData("99")]
+    [InlineData("Magery,90,50")]
+    public void Migrator_RejectsMalformedSkillTriggerAttribute(string xml)
+    {
+        var node = ParseNode($"<Point X=\"1500\" Y=\"1500\" Z=\"0\" Map=\"Felucca\" Running=\"false\" ProximityRange=\"8\" SkillTrigger=\"{xml}\" />");
+        var spawner = XmlSpawnerMigrator.ParseXmlSpawnerNode(node);
+        try
+        {
+            Assert.DoesNotContain(spawner.TriggerDefinitions, d => d.StartsWith("skill:", StringComparison.Ordinal));
+
+            // The proximity definition from the same node is untouched, so this is a targeted rejection
+            // rather than the whole trigger block being lost.
+            Assert.Contains("proximity:8:true:false:5:0", spawner.TriggerDefinitions);
+            Assert.True(spawner.TriggerActivated);
+        }
+        finally
+        {
+            spawner.Delete();
+        }
+    }
 }

Projects/ModernSpawner.Tests/Core/SkillTriggerTests.cs

@@ -0,0 +1,131 @@
+using System;
+using Server.Engines.ModernSpawner.Tests.Fixtures;
+using Server.Engines.ModernSpawner.Triggers;
+using Server.Misc;
+using Server.Mobiles;
+using Xunit;
+
+namespace Server.Engines.ModernSpawner.Tests;
+
+/// <summary>
+/// End-to-end cover for skill triggers: a real <see cref="SkillCheck" /> handler raises
+/// <see cref="SkillEvents.SkillUsed" />, <see cref="ModernSpawnerEvents" /> forwards it, and the spawner
+/// fires only for players, only in range, only for the configured outcome, and only once per cooldown.
+/// </summary>
+[Collection("Sequential ModernSpawner Tests")]
+public class SkillTriggerTests
+{
+    private static ModernSpawner Place(string definition)
+    {
+        var spawner = new ModernSpawner(1, TimeSpan.FromMinutes(5), TimeSpan.FromMinutes(10), 0, default, "Rabbit");
+        spawner.MoveToWorld(new Point3D(1500, 1500, 0), Map.Felucca);
+        spawner.AddToTriggerDefinitions(definition);
+        spawner.TriggerActivated = true;
+        return spawner;
+    }
+
+    private static PlayerMobile PlacePlayer(Point3D at)
+    {
+        // Mobile.Player is not set by the PlayerMobile constructor (production sets it on login), and the
+        // bridge filters on it, so the test host sets it the way ModernUO's own mobile tests do.
+        var player = new PlayerMobile { Name = "Miner", Player = true };
+        player.MoveToWorld(at, Map.Felucca);
+        return player;
+    }
+
+    [Fact]
+    public void PlayerSkillUse_InRange_FiresTrigger()
+    {
+        var spawner = Place("skill:Mining:10:0:false:0");
+        var player = PlacePlayer(new Point3D(1503, 1500, 0));
+        try
+        {
+            Assert.False(spawner.Triggered);
+            SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, 1.0);
+            Assert.True(spawner.Triggered);
+        }
+        finally
+        {
+            player.Delete();
+            spawner.Delete();
+        }
+    }
+
+    [Fact]
+    public void PlayerSkillUse_OutOfRange_DoesNotFire()
+    {
+        var spawner = Place("skill:Mining:5:0:false:0");
+        var player = PlacePlayer(new Point3D(1520, 1500, 0));
+        try
+        {
+            SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, 1.0);
+            Assert.False(spawner.Triggered);
+        }
+        finally
+        {
+            player.Delete();
+            spawner.Delete();
+        }
+    }
+
+    [Fact]
+    public void FailureOnlyTrigger_IgnoresSuccess()
+    {
+        var spawner = Place("skill:Mining-:10:0:false:0");
+        var player = PlacePlayer(new Point3D(1503, 1500, 0));
+        try
+        {
+            SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, 1.0);
+            Assert.False(spawner.Triggered);
+            SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, -0.1);
+            Assert.True(spawner.Triggered);
+        }
+        finally
+        {
+            player.Delete();
+            spawner.Delete();
+        }
+    }
+
+    [Fact]
+    public void CreatureSkillUse_DoesNotFire()
+    {
+        var spawner = Place("skill:Any:10:0:false:0");
+        var rabbit = new Rabbit();
+        rabbit.MoveToWorld(new Point3D(1503, 1500, 0), Map.Felucca);
+        try
+        {
+            SkillCheck.Mobile_SkillCheckDirectTarget(rabbit, SkillName.Mining, null, 1.0);
+            Assert.False(spawner.Triggered);
+        }
+        finally
+        {
+            rabbit.Delete();
+            spawner.Delete();
+        }
+    }
+
+    [Fact]
+    public void Cooldown_SuppressesSecondFiringUntilElapsed()
+    {
+        var spawner = Place("skill:Mining:10:0:false:5");
+        var player = PlacePlayer(new Point3D(1503, 1500, 0));
+        try
+        {
+            SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, 1.0);
+            Assert.True(spawner.Triggered);
+            spawner.ResetTrigger();
+            SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, 1.0);
+            Assert.False(spawner.Triggered);
+
+            ModernSpawnerTestServer.AdvanceClock(TimeSpan.FromSeconds(6));
+            SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, 1.0);
+            Assert.True(spawner.Triggered);
+        }
+        finally
+        {
+            player.Delete();
+            spawner.Delete();
+        }
+    }
+}

Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Reflection;
 using System.Threading;
 using Server.Engines.ModernSpawner.Scripting;
@@ -23,6 +24,12 @@ namespace Server.Engines.ModernSpawner.Tests.Fixtures;
 /// </summary>
 public static class ModernSpawnerTestServer
 {
+    /// <summary>
+    /// The instant <see cref="Core.Now" /> is seeded to. Fixed so that wall-clock-sensitive triggers
+    /// take the same branch on every run; noon UTC is outside the night windows the trigger tests use.
+    /// </summary>
+    public static readonly DateTime FixedStartTime = new(2020, 1, 1, 12, 0, 0, DateTimeKind.Utc);
+
     private static readonly Lock _lock = new();
     private static bool _initialized;
 
@@ -46,13 +53,15 @@ public static void Initialize()
 
             SkillsInfo.Configure();
 
-            // NOTE: production (Main.cs) and ModernUO's own fixtures seed the loop clock here with
-            // `Core._now = DateTime.UtcNow`. That field is `internal` to Server.dll and its
-            // InternalsVisibleTo list only names Server.Tests and UOContent.Tests, so this assembly
-            // cannot set it and there is no public equivalent. Core.Now therefore stays
-            // DateTime.MinValue for this host. Nothing on the spawner lifecycle paths depends on an
-            // absolute wall clock (deadlines are relative, and the timer wheel runs on tick counts),
-            // but a future test that moves or reads the clock will need ModernUO to expose a seam.
+            // Production (Main.cs) and ModernUO's own fixtures seed the loop clock here; Server.dll
+            // grants this assembly InternalsVisibleTo, so the same seam is available. Without it
+            // Core.Now stays DateTime.MinValue and anything comparing against an absolute wall clock
+            // (cooldowns, time windows) reads as "never elapsed".
+            //
+            // A fixed instant rather than DateTime.UtcNow: a real clock makes every wall-time window
+            // test depend on when the suite happens to run, so a run at 19:00 and a run at 09:00 would
+            // exercise different branches. Noon UTC sits outside the night windows the trigger tests use.
+            Core._now = FixedStartTime;
 
             // The timer wheel must exist before NetState.Configure(), which schedules a recurring
             // sweep through Timer.DelayCall (production order in Main.cs: Timer.Init runs before
@@ -91,4 +100,25 @@ public static void Initialize()
             _initialized = true;
         }
     }
+
+    /// <summary>
+    /// Moves the engine clock forward. Only valid in this host, which never ticks the timer wheel, so
+    /// nothing schedules off the value being advanced.
+    /// <para>
+    /// The clock only ever moves forward: it is seeded once per process, never rewound, and shared by
+    /// every test in the collection. A test must therefore assert on elapsed intervals rather than on
+    /// an absolute <see cref="Core.Now" /> deadline, and must not assume the clock is where an earlier
+    /// test left it.
+    /// </para>
+    /// </summary>
+    /// <param name="by">How far forward to move <see cref="Core.Now" />. A negative span is ignored.</param>
+    public static void AdvanceClock(TimeSpan by)
+    {
+        if (by <= TimeSpan.Zero)
+        {
+            return;
+        }
+
+        Core._now += by;
+    }
 }

Projects/ModernSpawner.Tests/ModernSpawner.Tests.csproj

@@ -5,7 +5,7 @@
   </PropertyGroup>
 
   <ItemGroup>
-    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="18.9.0" />
+    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="18.10.0" />
     <PackageReference Include="xunit" Version="2.9.3" />
     <PackageReference Include="xunit.runner.visualstudio" Version="4.0.0">
       <PrivateAssets>all</PrivateAssets>

Projects/ModernSpawner.Tests/Triggers/TriggerParsingTests.cs

@@ -159,7 +159,7 @@ public void SkillTrigger_Parse_AnySkill()
         var trigger = SkillTrigger.Parse("skill:Any:8");
 
         Assert.NotNull(trigger);
-        Assert.Equal((SkillName)(-1), trigger.TargetSkill);
+        Assert.True(trigger.AnySkill);
         Assert.Equal(8, trigger.Range);
     }
 
@@ -234,5 +234,118 @@ public void SkillTrigger_Range_MinimumIsOne()
         Assert.Equal(1, trigger.Range);
     }
 
+    [Fact]
+    public void SkillTrigger_Parse_OutcomeSuffix()
+    {
+        var success = SkillTrigger.Parse("skill:Mining+:10");
+        Assert.NotNull(success);
+        Assert.Equal(SkillName.Mining, success.TargetSkill);
+        Assert.Equal(SkillOutcome.Success, success.Outcome);
+
+        var failure = SkillTrigger.Parse("skill:Mining-:10");
+        Assert.NotNull(failure);
+        Assert.Equal(SkillOutcome.Failure, failure.Outcome);
+
+        var any = SkillTrigger.Parse("skill:Mining:10");
+        Assert.NotNull(any);
+        Assert.Equal(SkillOutcome.Any, any.Outcome);
+    }
+
+    [Fact]
+    public void SkillTrigger_Parse_ValueWindow()
+    {
+        var window = SkillTrigger.Parse("skill:Magery:5:50-90");
+        Assert.NotNull(window);
+        Assert.Equal(50.0, window.MinSkillValue);
+        Assert.Equal(90.0, window.MaxSkillValue);
+
+        var minOnly = SkillTrigger.Parse("skill:Magery:5:50.0");
+        Assert.NotNull(minOnly);
+        Assert.Equal(50.0, minOnly.MinSkillValue);
+        Assert.Equal(-1.0, minOnly.MaxSkillValue);
+    }
+
+    [Fact]
+    public void SkillTrigger_Any_UsesFlagNotSentinel()
+    {
+        var any = SkillTrigger.Parse("skill:Any+:8");
+        Assert.NotNull(any);
+        Assert.True(any.AnySkill);
+        Assert.True(any.MatchesSkill(SkillName.Alchemy));
+        Assert.True(any.MatchesSkill(SkillName.Mining));
+        Assert.Equal(SkillOutcome.Success, any.Outcome);
+
+        var mining = SkillTrigger.Parse("skill:Mining:8");
+        Assert.False(mining.AnySkill);
+        Assert.False(mining.MatchesSkill(SkillName.Alchemy));
+    }
+
+    [Theory]
+    [InlineData("skill:Mining:10")]
+    [InlineData("skill:Mining+:10:50-90:true:15")]
+    [InlineData("skill:Any-:8:0:false:5")]
+    [InlineData("skill:Blacksmith:15:80.0:true:10")]
+    public void SkillTrigger_Serialize_RoundTrips(string definition)
+    {
+        var first = SkillTrigger.Parse(definition);
+        Assert.NotNull(first);
+        var second = SkillTrigger.Parse(first.Serialize());
+        Assert.NotNull(second);
+        Assert.Equal(first.AnySkill, second.AnySkill);
+        Assert.Equal(first.TargetSkill, second.TargetSkill);
+        Assert.Equal(first.Outcome, second.Outcome);
+        Assert.Equal(first.Range, second.Range);
+        Assert.Equal(first.MinSkillValue, second.MinSkillValue);
+        Assert.Equal(first.MaxSkillValue, second.MaxSkillValue);
+        Assert.Equal(first.RequireLOS, second.RequireLOS);
+        Assert.Equal(first.Cooldown, second.Cooldown);
+    }
+
+    [Fact]
+    public void SkillTrigger_Evaluate_HonoursOutcomeAndWindow()
+    {
+        // Evaluate needs a spawner only for Running/Map/range; build the context without one and
+        // exercise the pure checks through a helper on the trigger (see Step 3: MatchesContext).
+        var success = SkillTrigger.Parse("skill:Mining+:10:50-90");
+        Assert.True(success.MatchesContext(SkillName.Mining, 60.0, true));
+        Assert.False(success.MatchesContext(SkillName.Mining, 60.0, false));
+        Assert.False(success.MatchesContext(SkillName.Mining, 40.0, true));
+        Assert.False(success.MatchesContext(SkillName.Mining, 95.0, true));
+        Assert.False(success.MatchesContext(SkillName.Magery, 60.0, true));
+
+        var failure = SkillTrigger.Parse("skill:Any-:10");
+        Assert.True(failure.MatchesContext(SkillName.Magery, 0.0, false));
+        Assert.False(failure.MatchesContext(SkillName.Magery, 0.0, true));
+    }
+
+    [Theory]
+    // The empty window segment is the regression: IndexOf('-', 1) threw on "" because startIndex 1 is
+    // past the end of a zero-length string, so a hand-edited or round-tripped definition took down the
+    // whole ActivateTriggers pass rather than being skipped.
+    [InlineData("skill:Mining:10::false:5")]
+    [InlineData("skill:Mining:10:")]
+    // TryParse<SkillName> accepts any numeric string, so these must be rejected on the enum, not the parse.
+    [InlineData("skill:99:10")]
+    [InlineData("skill:-1:10")]
+    // An inverted window can never match.
+    [InlineData("skill:Magery:5:90-50")]
+    public void SkillTrigger_Parse_MalformedDefinition_ReturnsNullWithoutThrowing(string definition)
+    {
+        var trigger = SkillTrigger.Parse(definition);
+
+        Assert.Null(trigger);
+    }
+
+    [Fact]
+    public void SkillTrigger_Parse_UnparseableRange_KeepsTheDocumentedDefault()
+    {
+        // int.TryParse writes 0 on failure; the default is 10, and Math.Max(1, 0) would have silently
+        // made this a 1-tile trigger.
+        var trigger = SkillTrigger.Parse("skill:Mining:wide");
+
+        Assert.NotNull(trigger);
+        Assert.Equal(10, trigger.Range);
+    }
+
     #endregion
 }