fix(npc): address role review feedback

Author: ifBars

S1API.Tests/Entities/CustomNpcReadinessPolicyTests.cs

@@ -1,6 +1,7 @@
 using S1API.Entities;
 using S1API.Internal.Entities;
 using S1API.Internal.Patches;
+using S1API.Internal.Utils;
 
 namespace S1API.Tests.Entities;
 
@@ -14,6 +15,16 @@ public void ClientHydrationSignalsReadyOnlyAfterEveryCustomNpcTypeCompletes()
 
         try
         {
+            foreach (Type npcType in ReflectionUtils.GetDerivedClasses<NPC>())
+            {
+                if (npcType.Assembly != typeof(NPC).Assembly
+                    && npcType != typeof(DealerNpc)
+                    && npcType != typeof(CustomerNpc))
+                {
+                    NPC.FinalizedCustomNpcTypes.Add(npcType);
+                }
+            }
+
             var dealer = TestObjectFactory.CreateUninitialized<DealerNpc>();
             var customer = TestObjectFactory.CreateUninitialized<CustomerNpc>();
 

S1API.Tests/Entities/NPCRoleDeclarationTests.cs

@@ -11,11 +11,8 @@ public void IsCustomerIsVirtualReadOnlyBooleanDefaultingToFalse()
     {
         PropertyInfo? property = typeof(NPC).GetProperty(nameof(NPC.IsCustomer));
         MethodInfo? getter = property?.GetMethod;
-        Type npcType = typeof(CustomNpcReadinessPolicyTests).GetNestedType(
-            "DealerNpc",
-            BindingFlags.NonPublic)!;
         var npc = (NPC)System.Runtime.CompilerServices.RuntimeHelpers
-            .GetUninitializedObject(npcType);
+            .GetUninitializedObject(typeof(RoleTestNpc));
 
         Assert.NotNull(property);
         Assert.Equal(typeof(bool), property!.PropertyType);
@@ -25,6 +22,25 @@ public void IsCustomerIsVirtualReadOnlyBooleanDefaultingToFalse()
         Assert.False(npc.IsCustomer);
     }
 
+    [Fact]
+    public void DeclaredPropertiesRejectsNullType()
+    {
+        ArgumentNullException exception = Assert.Throws<ArgumentNullException>(
+            () => NpcRoleDeclarationResolver.GetDeclaredProperties(null!));
+
+        Assert.Equal("npcType", exception.ParamName);
+    }
+
+    [Fact]
+    public void DeclaredPropertiesRejectsNonNpcType()
+    {
+        ArgumentException exception = Assert.Throws<ArgumentException>(
+            () => NpcRoleDeclarationResolver.GetDeclaredProperties(typeof(string)));
+
+        Assert.Equal("npcType", exception.ParamName);
+        Assert.Contains("does not derive from", exception.Message);
+    }
+
     [Theory]
     [InlineData(false, false, false, false)]
     [InlineData(false, true, false, false)]
@@ -132,4 +148,11 @@ public void LegacyEnsureMethodsRemainFluentNonErrorObsoleteShims(
     private static NPCPrefabBuilder CompileLegacyFluentCalls(NPCPrefabBuilder builder) =>
         builder.EnsureCustomer().EnsureDealer().EnsureSupplier();
 #pragma warning restore CS0618
+
+    private sealed class RoleTestNpc : NPC
+    {
+        internal override void CreateInternal()
+        {
+        }
+    }
 }

S1API/Entities/Schedule/NPCScheduleBuilder.cs

@@ -374,8 +374,8 @@ public LocationBasedActionSpecBuilder LocationBased(Vector3 destination, int sta
         /// <returns>This builder instance for method chaining.</returns>
         /// <remarks>
         /// As of v0.4.2f4, deal handling is now automatic through the DealerAttendDealBehaviour system.
-        /// This method is kept for backwards compatibility but is a no-op. Dealer NPCs set up with
-        /// Dealer NPCs automatically handle deals when contracts are assigned.
+        /// This method is kept for backwards compatibility but is a no-op. Dealer NPCs
+        /// automatically handle deals when contracts are assigned.
         /// </remarks>
         [System.Obsolete("HandleDeal is no longer needed as of game version 0.4.2f4. Deal handling is now automatic through DealerAttendDealBehaviour.")]
         public PrefabScheduleBuilder HandleDeal(int startTime, string? name = null)

skills/schedule-one-custom-npcs/SKILL.md

@@ -21,7 +21,7 @@ Follow this order:
 
 Keep these responsibilities separate:
 
-- `ConfigurePrefab(...)`: identity, icon, spawn position, relationship defaults, customer defaults, dealer defaults, inventory defaults, schedule, and required `Ensure*` components.
+- `ConfigurePrefab(...)`: identity, icon, spawn position, relationship defaults, customer defaults, dealer defaults, inventory defaults, schedule, and action-specific `Ensure*` calls such as `plan.EnsureDealSignal()`. Role infrastructure comes automatically from `IsCustomer`, `IsDealer`, and `IsSupplier`; do not add `EnsureCustomer()`, `EnsureDealer()`, or `EnsureSupplier()`.
 - `OnCreated()`: `base.OnCreated()`, `Appearance.Build()`, `Schedule.Enable()`, `Schedule.InitializeActions()` when needed, dialogue wiring, event subscriptions, text messages, and runtime state.
 
 Do not move persistent customer, dealer, relationship, or schedule defaults into runtime code.

skills/schedule-one-custom-npcs/references/example-project-patterns.md

@@ -11,6 +11,7 @@ Use when the NPC is visible in the world, directly interactable, and participate
 Recommended structure:
 
 ```csharp
+public override bool IsPhysical => true;
 public override bool IsCustomer => true;
 
 builder.WithIdentity(...)