fix(opl): refuse property list invalidation raised from inside GetProperties

Any property getter reached from GetProperties that calls InvalidateProperties
takes the tooltip build down with it:

  System.ArgumentNullException: Value cannot be null. (Parameter 'array')
    at Server.ObjectPropertyList.AppendStringDirect(String value)
    at Server.Mobiles.PlayerMobile.GetProperties(IPropertyList list)

InvalidateProperties rebuilds in place -- Reset() then GetProperties() again on
the same instance -- and Reset() does two destructive things to a build already
in flight. It returns the pooled interpolation scratch buffer, which the compiler
rents in the interpolated-string handler ctor and returns in the closing Add, so
every hole is evaluated while that buffer is live; the next Append* then spans a
null array. It also rewinds the packet cursor, so properties already written are
overwritten by the nested pass.

There is no correct recovery, and retrying the build would only hide the defect,
so the engine refuses: the nested call logs an error with a stack trace, throws
in DEBUG so it gets found and fixed, and in RELEASE returns without touching the
list -- a possibly stale tooltip, but no crash, no corrupted packet, and nothing
leaked back to the pool. Getters that genuinely must invalidate should defer with
Timer.DelayCall(InvalidateProperties).

The guard flag lives on the ObjectPropertyList rather than on the entity: it is
that list's own lifecycle, it costs nothing (both Item and ObjectPropertyList
absorb it in existing padding, and the list is allocated lazily), and it stays
correct when builds for different entities nest.

Factions PlayerState was the getter that surfaced this, and it is now maintained
rather than lazily computed:

- Rank is a plain field read. The lazy `if (m_InvalidateRank)` recompute is gone
  along with the flag itself; UpdateRank() recomputes at each point an input
  actually changes (the RankIndex setter, the end of the KillPoints setter once
  the swap bookkeeping and ZeroRankOffset have settled, Faction.AddMember after
  the member is inserted, and FactionState after a load once the ordering is
  final). All six readers of Rank were checked; none relied on the old side
  effect.
- Both constructors seed the lowest rank. Nothing recomputes on read any more, so
  Rank must be usable immediately -- including for members that never get a
  RankIndex assigned, which is every member with no kill points.
- Rank always resolves. Ranks are ordered by Required descending ending at 0, so
  a negative percent (RankIndex out of sync with ZeroRankOffset) matched nothing
  and left m_Rank null -- an NRE on Rank.Title. It no longer divides by a zero
  ZeroRankOffset either.
- Fixes a pre-existing staleness bug: the KillPoints setter writes m_RankIndex
  directly in two places, bypassing the property setter, so the cached rank was
  never refreshed when a player crossed zero kill points.

PropertyList also publishes the list into m_PropertyList before building it
rather than assigning through `??=` afterwards, so a nested InvalidateProperties
sees the build in progress instead of recursing into a second throwaway list.

ObjectPropertyList re-rents its scratch buffer instead of spanning a null array,
so a stray Reset() from any other caller degrades rather than aborting
GetProperties.

Documents the rule as audit rule 19 in CLAUDE.md, a new section in
dev-docs/property-lists.md, and the property-lists and code-audit skills.

CLAUDE.md

@@ -28,6 +28,7 @@ Apply these when writing or reviewing `.cs` files under `Projects/`.
 16. **Prefer switch expressions and switch-when** — use switch expressions for value mapping and switch-when for pattern matching where they improve readability. Exception: skip if unreadable or cold path → `dev-docs/code-standards.md`
 17. **No `System.Text.StringBuilder`** — use `ValueStringBuilder` with `stackalloc` (bounded output) or `ValueStringBuilder.Create()` (unbounded). Supports `$"..."` interpolation directly. Always use `using var` for disposal. Use `Reset()` instead of reassigning → `dev-docs/string-handling.md`
 18. **Interpolation anti-patterns on handler-aware APIs** — `Send*`/`Say`/`Emote`/`PublicOverhead*`/`IPropertyList.Add`/gump `AddLabel`/`AddHtml`/`Html.Center`/`SpanWriter.Write*` all have `ref RawInterpolatedStringHandler` overloads that allocate zero strings, but only when the call-site argument is a `$"..."` literal directly. Avoid: ternaries with interpolated branches (`Send(c ? $"a" : $"b")`), switch expressions with interpolated arms, pre-built `var s = $"..."` locals (single-use), `.ToString()` / `.String()` / `string.Format` inside holes, string concat (`{a + b}`), LINQ string ops in holes. Use `:L` format spec for lowercase (`{rank:L}` not `rank.ToString().ToLowerInvariant()`) → `dev-docs/string-handling.md` § Interpolation Anti-Patterns
+19. **No `InvalidateProperties()` from inside `GetProperties`** — every property a `GetProperties` override reads must be a pure read. `InvalidateProperties()` rebuilds the list in place (`Reset()` + rebuild), and `Reset()` returns the pooled interpolation buffer — which the compiler rents for the whole `$"..."` expression, so every hole is evaluated while it is live — and rewinds the packet cursor. A getter that invalidates therefore throws `ArgumentNullException` (parameter `"array"`) out of `GetProperties` from an unrelated-looking line, or silently corrupts the tooltip. The engine refuses and logs an error; `DEBUG` throws. Lazy recomputation in a getter is fine — the *notification* is not. Invalidate in the setter that changes the value, or defer with `Timer.DelayCall(InvalidateProperties)` → `dev-docs/property-lists.md` § Never Invalidate From Inside `GetProperties`
 
 ## Dev-Docs Reference
 

Projects/Server.Tests/Tests/PropertyList/ObjectPropertyListReentrancyTests.cs

@@ -0,0 +1,142 @@
+using System;
+using Xunit;
+
+namespace Server.Tests;
+
+/// <summary>
+/// The interpolation buffer is rented by the handler ctor and returned by the closing Add, so every
+/// hole is evaluated while it is live. A Reset()/Dispose() landing in that window used to leave the
+/// next Append* spanning a null array: ArgumentNullException, parameter "array".
+/// </summary>
+public class ObjectPropertyListReentrancyTests
+{
+    // Stands in for a property getter that invalidates while its own tooltip is being built.
+    private static string ResettingHole(ObjectPropertyList list, string value)
+    {
+        list.Reset();
+        return value;
+    }
+
+    private static string DisposingHole(ObjectPropertyList list, string value)
+    {
+        list.Dispose();
+        return value;
+    }
+
+    [Fact]
+    public void InterpolatedAdd_ResetMidHole_DoesNotThrow()
+    {
+        var opl = new ObjectPropertyList(null);
+
+        var ex = Record.Exception(
+            () => opl.Add(1060776, $"{ResettingHole(opl, "Knight")}\t{"Council of Mages"}")
+        );
+
+        Assert.Null(ex);
+    }
+
+    [Fact]
+    public void InterpolatedAdd_DisposeMidHole_DoesNotThrow()
+    {
+        var opl = new ObjectPropertyList(null);
+
+        var ex = Record.Exception(
+            () => opl.Add(1060776, $"{DisposingHole(opl, "Knight")}\t{"Council of Mages"}")
+        );
+
+        Assert.Null(ex);
+    }
+}
+
+/// <summary>
+/// The guard is per-list, so nested builds (a GetProperties override that reads another entity's
+/// PropertyList) cannot unguard the outer one the way a single shared slot would.
+/// </summary>
+public class ObjectPropertyListNestedBuildTests
+{
+    [Fact]
+    public void NestedBuild_DoesNotUnguardTheOuterList()
+    {
+        var outer = new ObjectPropertyList(null);
+        var inner = new ObjectPropertyList(null);
+
+        outer.IsBuilding = true;
+        inner.IsBuilding = true;   // another entity starts building, and finishes
+        inner.IsBuilding = false;
+
+        Assert.True(outer.IsBuilding);
+    }
+
+    [Fact]
+    public void Reset_MidInterpolation_LeavesTheListUsable()
+    {
+        var opl = new ObjectPropertyList(null);
+
+        opl.Add(1060776, $"{Reset(opl, "Knight")}\t{"Council of Mages"}");
+        opl.Add(1042971, "still working");
+        opl.Terminate();
+
+        Assert.NotNull(opl.Buffer);
+    }
+
+    private static string Reset(ObjectPropertyList list, string value)
+    {
+        list.Reset();
+        return value;
+    }
+}
+
+
+/// <summary>
+/// Invalidating from inside GetProperties is a defect in the getter, not a case to recover from:
+/// DEBUG throws, RELEASE keeps a possibly stale tooltip without crashing or leaking.
+/// </summary>
+[Collection("Sequential Server Tests")]
+public class PropertyListInvalidationDuringBuildTests
+{
+    private class SelfInvalidatingMobile : Mobile
+    {
+        public int Builds;
+
+        public override void GetProperties(IPropertyList list)
+        {
+            Builds++;
+            base.GetProperties(list);
+            InvalidateProperties();
+            list.Add(1060776, $"{"Knight"}\t{"Council of Mages"}");
+        }
+    }
+
+    private static SelfInvalidatingMobile Place(int x)
+    {
+        var m = new SelfInvalidatingMobile();
+        m.MoveToWorld(new Point3D(x, 1000, 0), Map.Felucca);
+        return m;
+    }
+
+    [Fact]
+    public void InvalidatingFromGetProperties_FailsLoudlyWithoutTearingDownTheBuild()
+    {
+        var wasEnabled = ObjectPropertyList.Enabled;
+        ObjectPropertyList.Enabled = true;
+
+        try
+        {
+            var m = Place(1000);
+
+#if DEBUG
+            Assert.Throws<InvalidOperationException>(() => _ = m.PropertyList);
+#else
+            Assert.Null(Record.Exception(() => _ = m.PropertyList));
+#endif
+
+            // Refused, not retried.
+            Assert.Equal(1, m.Builds);
+            m.Delete();
+        }
+        finally
+        {
+            ObjectPropertyList.Enabled = wasEnabled;
+        }
+    }
+}

Projects/Server/Items/Item.cs

@@ -14,6 +14,7 @@
  *************************************************************************/
 
 using System;
+using System.Diagnostics;
 using System.Collections.Generic;
 using System.Reflection;
 using System.Runtime.CompilerServices;
@@ -798,7 +799,22 @@ public partial class Item : IHued, IComparable<Item>, ISpawnable, IObjectPropert
 
     public virtual int HuedItemID => m_ItemID;
 
-    public ObjectPropertyList PropertyList => m_PropertyList ??= InitializePropertyList(new ObjectPropertyList(this));
+    public ObjectPropertyList PropertyList
+    {
+        get
+        {
+            if (m_PropertyList == null)
+            {
+                // Publish the list before building it so a nested InvalidateProperties can see the
+                // build in progress and defer instead of recursing into a second throwaway list.
+                var list = new ObjectPropertyList(this);
+                m_PropertyList = list;
+                InitializePropertyList(list);
+            }
+
+            return m_PropertyList;
+        }
+    }
 
     /// <summary>
     ///     Overridable. Fills an <see cref="ObjectPropertyList" /> with everything applicable. By default, this invokes
@@ -2429,9 +2445,19 @@ public partial class Item : IHued, IComparable<Item>, ISpawnable, IObjectPropert
 
     private ObjectPropertyList InitializePropertyList(ObjectPropertyList list)
     {
-        GetProperties(list);
-        AppendChildProperties(list);
-        list.Terminate();
+        list.IsBuilding = true;
+
+        try
+        {
+            GetProperties(list);
+            AppendChildProperties(list);
+            list.Terminate();
+        }
+        finally
+        {
+            list.IsBuilding = false;
+        }
+
         return list;
     }
 
@@ -2448,6 +2474,26 @@ public partial class Item : IHued, IComparable<Item>, ISpawnable, IObjectPropert
             return;
         }
 
+        // Always a bug in the property getter, and there is no correct recovery: refuse rather than
+        // hide it. RELEASE keeps a possibly stale tooltip, DEBUG throws.
+        // See dev-docs/property-lists.md "Never Invalidate From Inside GetProperties".
+        if (m_PropertyList?.IsBuilding == true)
+        {
+            logger.Error(
+                "{Entity} called InvalidateProperties() while its property list was being built. Remove the side effect from the property getter, or defer it with Timer.DelayCall.\n{StackTrace}",
+                this,
+                new StackTrace()
+            );
+
+#if DEBUG
+            throw new InvalidOperationException(
+                $"{this} invalidated its property list from inside GetProperties. Remove the side effect from the property getter."
+            );
+#else
+            return;
+#endif
+        }
+
         if (m_Map != null && m_Map != Map.Internal && !World.Loading)
         {
             int? oldHash;

Projects/Server/Mobiles/Mobile.cs

@@ -26,6 +26,7 @@ using Server.Network;
 using Server.Prompts;
 using Server.Targeting;
 using System;
+using System.Diagnostics;
 using System.Collections.Generic;
 using System.Runtime.CompilerServices;
 using Server.Buffers;
@@ -2291,7 +2292,22 @@ public partial class Mobile : IHued, IComparable<Mobile>, ISpawnable, IObjectPro
     public int CompareTo(Mobile other) => other == null ? -1 : Serial.CompareTo(other.Serial);
 
     public virtual int HuedItemID => m_Female ? 0x2107 : 0x2106;
-    public ObjectPropertyList PropertyList => m_PropertyList ??= InitializePropertyList(new ObjectPropertyList(this));
+    public ObjectPropertyList PropertyList
+    {
+        get
+        {
+            if (m_PropertyList == null)
+            {
+                // Publish the list before building it so a nested InvalidateProperties can see the
+                // build in progress and defer instead of recursing into a second throwaway list.
+                var list = new ObjectPropertyList(this);
+                m_PropertyList = list;
+                InitializePropertyList(list);
+            }
+
+            return m_PropertyList;
+        }
+    }
 
     public virtual void GetProperties(IPropertyList list)
     {
@@ -7225,8 +7241,18 @@ public partial class Mobile : IHued, IComparable<Mobile>, ISpawnable, IObjectPro
 
     private ObjectPropertyList InitializePropertyList(ObjectPropertyList list)
     {
-        GetProperties(list);
-        list.Terminate();
+        list.IsBuilding = true;
+
+        try
+        {
+            GetProperties(list);
+            list.Terminate();
+        }
+        finally
+        {
+            list.IsBuilding = false;
+        }
+
         return list;
     }
 
@@ -7243,6 +7269,26 @@ public partial class Mobile : IHued, IComparable<Mobile>, ISpawnable, IObjectPro
             return;
         }
 
+        // Always a bug in the property getter, and there is no correct recovery: refuse rather than
+        // hide it. RELEASE keeps a possibly stale tooltip, DEBUG throws.
+        // See dev-docs/property-lists.md "Never Invalidate From Inside GetProperties".
+        if (m_PropertyList?.IsBuilding == true)
+        {
+            logger.Error(
+                "{Entity} called InvalidateProperties() while its property list was being built. Remove the side effect from the property getter, or defer it with Timer.DelayCall.\n{StackTrace}",
+                this,
+                new StackTrace()
+            );
+
+#if DEBUG
+            throw new InvalidOperationException(
+                $"{this} invalidated its property list from inside GetProperties. Remove the side effect from the property getter."
+            );
+#else
+            return;
+#endif
+        }
+
         if (m_Map != null && m_Map != Map.Internal && !World.Loading)
         {
             int? oldHash;

Projects/Server/PropertyList/ObjectPropertyList.cs

@@ -55,6 +55,13 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable
     private int _pos;
     private char[]? _arrayToReturnToPool;
 
+    /// <summary>
+    /// True while GetProperties is populating this list. Set by the owning entity so a nested
+    /// InvalidateProperties can be refused instead of Reset()ing a build already in flight.
+    /// </summary>
+    internal bool IsBuilding { get; set; }
+
+
     public ObjectPropertyList(IEntity? e)
     {
         Entity = e;
@@ -319,8 +326,23 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable
     private static int GetDefaultLength(int literalLength, int formattedCount) =>
         Math.Max(256, literalLength + formattedCount * 11);
 
+    // Reset()/Dispose() return the scratch buffer to the pool. If either lands while a `$"..."`
+    // handler is still appending, re-rent rather than spanning a null array and throwing out of
+    // GetProperties. Mobile/Item hold the primary guard; this covers any other caller.
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    private void EnsureInterpolationBuffer()
+    {
+        if (_arrayToReturnToPool == null)
+        {
+            _arrayToReturnToPool = STArrayPool<char>.Shared.Rent(256);
+            _pos = 0;
+        }
+    }
+
     public void AppendLiteral(string value)
     {
+        EnsureInterpolationBuffer();
+
         if (value.Length == 1)
         {
             var chars = _arrayToReturnToPool.AsSpan();
@@ -354,6 +376,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable
 
     public void AppendFormatted<T>(T value)
     {
+        EnsureInterpolationBuffer();
+
         string? s;
         if (value is IFormattable)
         {
@@ -384,6 +408,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable
 
     public void AppendFormatted<T>(T value, string? format)
     {
+        EnsureInterpolationBuffer();
+
         // '#' marks an integer argument as a cliloc ("#<value>"). Integers only -- a float/double/decimal
         // '#' is the standard numeric format, not a cliloc marker.
         if (format == "#" && value is int or uint or long or ulong or short or ushort or byte or sbyte)
@@ -442,6 +468,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable
 
     public void AppendFormatted(ReadOnlySpan<char> value)
     {
+        EnsureInterpolationBuffer();
+
         if (value.TryCopyTo(_arrayToReturnToPool.AsSpan(_pos..)))
         {
             _pos += value.Length;
@@ -454,6 +482,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable
 
     public void AppendFormatted(ReadOnlySpan<char> value, int alignment = 0, string? format = null)
     {
+        EnsureInterpolationBuffer();
+
         var leftAlign = false;
         if (alignment < 0)
         {
@@ -488,6 +518,8 @@ public sealed class ObjectPropertyList : IPropertyList, IDisposable
 
     public void AppendFormatted(string? value)
     {
+        EnsureInterpolationBuffer();
+
         if (value?.TryCopyTo(_arrayToReturnToPool.AsSpan(_pos..)) == true)
         {
             _pos += value.Length;

Projects/UOContent.Tests/Tests/Engines/Factions/FactionRankTests.cs

@@ -0,0 +1,100 @@
+using System.Collections.Generic;
+using Server;
+using Server.Factions;
+using Xunit;
+
+namespace UOContent.Tests;
+
+/// <summary>
+/// PlayerState.Rank is read from GetProperties, so it must stay a plain field read. These pin what
+/// that requires: the rank is never null, and it is correct without anyone having read it first.
+/// </summary>
+[Collection("Sequential UOContent Tests")]
+public class FactionRankTests
+{
+    // The faction ctor builds its own Definition, so no world state is needed.
+    private static Faction NewFaction() => new CouncilOfMages();
+
+    private static PlayerState AddMember(Faction faction, List<PlayerState> owner)
+    {
+        var state = new PlayerState(new Mobile(), faction, owner);
+        owner.Add(state);
+        return state;
+    }
+
+    [Fact]
+    public void Rank_IsPopulatedBeforeAnythingReadsIt()
+    {
+        var faction = NewFaction();
+        var state = new PlayerState(new Mobile(), faction, []);
+
+        // Nothing recomputes on read, so the ctor must leave a usable value or Rank.Title NREs.
+        Assert.NotNull(state.Rank);
+        Assert.NotNull(state.Rank.Title);
+    }
+
+    [Fact]
+    public void Rank_IsTheLowestRank_ForAnUnrankedMember()
+    {
+        var faction = NewFaction();
+        var owner = new List<PlayerState>();
+        var a = AddMember(faction, owner);
+        var b = AddMember(faction, owner);
+
+        a.UpdateRank();
+        b.UpdateRank();
+
+        var lowest = faction.Definition.Ranks[^1];
+
+        Assert.Equal(lowest.Rank, a.Rank.Rank);
+        Assert.Equal(lowest.Rank, b.Rank.Rank);
+    }
+
+    [Fact]
+    public void SettingRankIndex_UpdatesRankWithoutAnyoneReadingIt()
+    {
+        var faction = NewFaction();
+        var owner = new List<PlayerState>();
+        var top = AddMember(faction, owner);
+        var bottom = AddMember(faction, owner);
+
+        faction.ZeroRankOffset = 2;
+
+        top.RankIndex = 0;
+        bottom.RankIndex = 1;
+
+        // No read triggered these, yet the ordering is reflected.
+        Assert.True(top.Rank.Rank > bottom.Rank.Rank);
+    }
+
+    [Fact]
+    public void ReadingRank_IsStableAndSideEffectFree()
+    {
+        var faction = NewFaction();
+        var owner = new List<PlayerState>();
+        var state = AddMember(faction, owner);
+
+        faction.ZeroRankOffset = 1;
+        state.RankIndex = 0;
+
+        var first = state.Rank;
+        var second = state.Rank;
+
+        Assert.Same(first, second);
+    }
+
+    [Fact]
+    public void RankIndexOutOfSyncWithZeroRankOffset_StillResolvesARank()
+    {
+        var faction = NewFaction();
+        var owner = new List<PlayerState>();
+        var a = AddMember(faction, owner);
+        AddMember(faction, owner);
+
+        // A negative percent used to match no rank at all, leaving Rank null.
+        faction.ZeroRankOffset = 1;
+        a.RankIndex = 5;
+
+        Assert.NotNull(a.Rank);
+    }
+}

Projects/UOContent/Engines/Factions/Core/Faction.cs

@@ -242,7 +242,11 @@ public abstract class Faction : IComparable<Faction>, ISpanParsable<Faction>
 
     public virtual void AddMember(Mobile mob)
     {
-        Members.Insert(ZeroRankOffset, new PlayerState(mob, this, Members));
+        var state = new PlayerState(mob, this, Members);
+        Members.Insert(ZeroRankOffset, state);
+
+        // Ranked after the insert: the ctor ran while Owner was still short a member.
+        state.UpdateRank();
 
         mob.AddToBackpack(FactionItem.Imbue(new Robe(), this, false, Definition.HuePrimary));
         mob.SendLocalizedMessage(1010374); // You have been granted a robe which signifies your faction

Projects/UOContent/Engines/Factions/Core/FactionState.cs

@@ -114,6 +114,13 @@ public class FactionState
                         }
                     }
 
+                    // The loop above only assigns RankIndex to members with kill points, and nothing
+                    // computes rank on read, so rank everyone now that the ordering has settled.
+                    foreach (var player in Members)
+                    {
+                        player.UpdateRank();
+                    }
+
                     FactionItems = [];
 
                     if (version >= 2)

Projects/UOContent/Engines/Factions/Core/PlayerState.cs

@@ -8,7 +8,6 @@ public class PlayerState : IComparable<PlayerState>
 {
     private Town m_Finance;
 
-    private bool m_InvalidateRank = true;
     private int m_KillPoints;
     private MerchantTitle m_MerchantTitle;
     private RankDefinition m_Rank;
@@ -22,6 +21,10 @@ public class PlayerState : IComparable<PlayerState>
         Faction = faction;
         Owner = owner;
 
+        // Owner does not contain this state yet, so the count is short by one; the caller ranks it
+        // after inserting.
+        SeedLowestRank();
+
         Attach();
         Invalidate();
     }
@@ -54,6 +57,9 @@ public class PlayerState : IComparable<PlayerState>
                 }
         }
 
+        // Members are still being read; FactionState ranks everyone once the ordering settles.
+        SeedLowestRank();
+
         Attach();
     }
 
@@ -116,6 +122,8 @@ public class PlayerState : IComparable<PlayerState>
                         Owner.Remove(this);
                         Owner.Insert(Faction.ZeroRankOffset, this);
 
+                        // Direct, not through RankIndex: ZeroRankOffset is mid-update. The
+                        // UpdateRank() at the end of this setter covers it.
                         m_RankIndex = Faction.ZeroRankOffset;
                         Faction.ZeroRankOffset++;
                     }
@@ -180,6 +188,7 @@ public class PlayerState : IComparable<PlayerState>
                 }
 
                 m_KillPoints = value;
+                UpdateRank();
                 Invalidate();
             }
         }
@@ -193,49 +202,72 @@ public class PlayerState : IComparable<PlayerState>
             if (m_RankIndex != value)
             {
                 m_RankIndex = value;
-                m_InvalidateRank = true;
+
+                UpdateRank();
+                Invalidate();
             }
         }
     }
 
-    public RankDefinition Rank
+    /// <summary>
+    /// Read from PlayerMobile.GetProperties, so it must stay a plain field read -- recomputing or
+    /// invalidating here re-enters the property list build. Maintained by <see cref="UpdateRank"/>.
+    /// </summary>
+    public RankDefinition Rank => m_Rank;
+
+    // Lowest rank (Required 0): correct for an unranked member, and never null, so Rank.Title
+    // cannot NRE before the first UpdateRank().
+    private void SeedLowestRank()
     {
-        get
+        var ranks = Faction.Definition.Ranks;
+
+        if (ranks.Length > 0)
         {
-            if (m_InvalidateRank)
+            m_Rank = ranks[^1];
+        }
+    }
+
+    /// <summary>
+    /// Recomputes the cached rank. Call whenever <see cref="RankIndex"/>, the faction's
+    /// ZeroRankOffset, or the member count changes -- and only once they have settled.
+    /// </summary>
+    public void UpdateRank()
+    {
+        var ranks = Faction.Definition.Ranks;
+
+        if (ranks.Length == 0)
+        {
+            return;
+        }
+
+        int percent;
+
+        if (Owner.Count == 1)
+        {
+            percent = 1000;
+        }
+        else if (m_RankIndex == -1 || Faction.ZeroRankOffset <= 0)
+        {
+            percent = 0;
+        }
+        else
+        {
+            percent = (Faction.ZeroRankOffset - m_RankIndex) * 1000 / Faction.ZeroRankOffset;
+        }
+
+        // Ranks run Required-descending ending at 0, so anything >= 0 matches below. A negative
+        // percent (RankIndex out of sync with ZeroRankOffset) would otherwise leave it null.
+        m_Rank = ranks[^1];
+
+        for (var i = 0; i < ranks.Length; i++)
+        {
+            var check = ranks[i];
+
+            if (percent >= check.Required)
             {
-                var ranks = Faction.Definition.Ranks;
-                int percent;
-
-                if (Owner.Count == 1)
-                {
-                    percent = 1000;
-                }
-                else if (m_RankIndex == -1)
-                {
-                    percent = 0;
-                }
-                else
-                {
-                    percent = (Faction.ZeroRankOffset - m_RankIndex) * 1000 / Faction.ZeroRankOffset;
-                }
-
-                for (var i = 0; i < ranks.Length; i++)
-                {
-                    var check = ranks[i];
-
-                    if (percent >= check.Required)
-                    {
-                        m_Rank = check;
-                        m_InvalidateRank = false;
-                        break;
-                    }
-                }
-
-                Invalidate();
+                m_Rank = check;
+                break;
             }
-
-            return m_Rank;
         }
     }
 

dev-docs/claude-skills/modernuo-code-audit.md

@@ -191,8 +191,17 @@ mob.SendMessage($"You earned a {rank:L} trophy!");          // "gold" not "Gold"
 
 **See**: `dev-docs/string-handling.md` § "Interpolation Anti-Patterns" for the full reference with detailed before/after examples.
 
+### 19. No InvalidateProperties From Inside GetProperties
+**Check**: Any property read by a `GetProperties` override — including through helpers — must be a pure read. Flag getters that call `InvalidateProperties()` (or a wrapper like `Invalidate()`) as a side effect.
+**Bad**: a `Rank` getter that lazily recomputes and then calls `Invalidate()`; reading it from `GetProperties` re-enters the build.
+**Good**: invalidate in the setter that actually changes the value, or defer with `Timer.DelayCall(InvalidateProperties)`.
+**Why**: `InvalidateProperties()` rebuilds the list in place (`Reset()` + rebuild). `Reset()` returns the pooled interpolation buffer — which the compiler rents for the whole `$"..."` expression, so every hole is evaluated while it is live — and rewinds the packet cursor. Re-entering mid-build throws `ArgumentNullException` (parameter `"array"`) out of `GetProperties` from a line unrelated to the offending getter, or silently corrupts the tooltip. The engine refuses and logs an error, and `DEBUG` throws, so this shows up as a crash in development.
+**Note**: Lazy recomputation inside a getter is fine. It is the notification that must not happen there.
+
+**See**: `dev-docs/property-lists.md` § "Never Invalidate From Inside `GetProperties`".
+
 ## Severity Levels
-- **ERROR**: Rules 3, 9, 10, 13 (will cause bugs, build failures, or client-side leaks)
+- **ERROR**: Rules 3, 9, 10, 13, 19 (will cause bugs, build failures, or client-side leaks)
 - **WARNING**: Rules 1 (Tier 3 LINQ), 2, 4, 5, 6, 7, 8, 12, 14, 15, 17 (performance/convention issues)
 - **INFO**: Rules 1 (Tier 2 LINQ on warm paths — note it but don't flag as violation), 16 (switch patterns — suggest but don't flag)
 - **ASK**: Rule 11 (need user input)

dev-docs/claude-skills/modernuo-property-lists.md

@@ -20,6 +20,12 @@ description: >
 3. **String interpolation** works with `IPropertyList` -- use `$"..."` syntax
 4. **`[InvalidateProperties]`** on `[SerializableField]` auto-refreshes tooltip on change
 5. **Call `InvalidateProperties()`** manually when non-serialized state changes tooltip
+6. **Never invalidate from inside `GetProperties`** -- every property a `GetProperties` override
+   reads must be a pure read. `InvalidateProperties()` rebuilds in place (`Reset()` + rebuild), so a
+   getter with that side effect tears down the list mid-build: it returns the pooled interpolation
+   buffer under an in-flight `$"..."` handler (`ArgumentNullException`, parameter `"array"`) and
+   rewinds the packet cursor. The engine refuses and logs an error; `DEBUG` throws. Defer instead:
+   `Timer.DelayCall(InvalidateProperties)`
 
 ## IPropertyList Interface
 
@@ -235,6 +241,7 @@ block.Add("Cannot be repaired".AsSpan());   // plain span, no string alloc
 - **Excessive rebuilds**: Don't call `InvalidateProperties()` in tight loops
 - **Assuming tooltip support**: Check `ObjectPropertyList.Enabled` if needed
 - **One giant `Add()` for multi-line text**: A property over ~512 chars crashes the legacy 2D client. Use `AddChunked`/`OplTextBlock` for variable-length free text
+- **Side-effecting property getters**: A getter reached from `GetProperties` that calls `InvalidateProperties()` (directly or via a helper like `Invalidate()`) re-enters the build and is refused — error logged, `DEBUG` throws. Lazy recomputation in a getter is fine; the *notification* is not. Invalidate where the value changes, or `Timer.DelayCall(InvalidateProperties)`
 
 ## Real Examples
 - Item properties: `Projects/Server/Items/Item.cs` (AddNameProperties, GetProperties)

dev-docs/property-lists.md

@@ -301,6 +301,66 @@ public void UseCharge()
 }
 ```
 
+### Never Invalidate From Inside `GetProperties` (CRITICAL)
+
+`InvalidateProperties()` rebuilds the list **in place** — `Reset()`, then `GetProperties()` again on
+the same instance. Calling it from a property getter that the build itself reaches is therefore
+re-entrant, and `Reset()` does two destructive things to the build in flight:
+
+1. It returns the pooled interpolation scratch buffer. The compiler rents that buffer in the
+   interpolated-string handler's constructor and returns it in the closing `Add`, so **every hole is
+   evaluated while the buffer is live**. Pulling it out mid-append makes the next `Append*` span a
+   null array — `ArgumentNullException: Value cannot be null. (Parameter 'array')` thrown out of
+   `GetProperties`, from a line that looks unrelated to the getter that caused it.
+2. It rewinds the packet cursor, so properties already written are overwritten by the nested pass.
+
+This is always a defect in the property getter, so the engine refuses rather than trying to recover:
+a nested call logs an error with a stack trace, throws in `DEBUG`, and in `RELEASE` returns without
+touching the list — leaving a possibly stale tooltip, but never a crash, a corrupted packet, or a
+leaked pool buffer. Retrying the build would only hide the bug.
+
+```csharp
+// BAD -- a getter with a side effect. Reading it from GetProperties re-enters the build.
+public RankDefinition Rank
+{
+    get
+    {
+        if (_invalidateRank)
+        {
+            _rank = Recompute();
+            _invalidateRank = false;
+            Invalidate();       // -> InvalidateProperties() -> Reset() on the list being built
+        }
+
+        return _rank;
+    }
+}
+
+// GOOD -- getters stay side-effect free; invalidate where the value actually changes.
+public int RankIndex
+{
+    get => _rankIndex;
+    set
+    {
+        if (_rankIndex != value)
+        {
+            _rankIndex = value;
+            _invalidateRank = true;
+            Invalidate();
+        }
+    }
+}
+```
+
+Lazy recomputation inside a getter is fine — it is the *notification* that must not happen there. If
+something genuinely must invalidate in response to a read, defer it off the build:
+
+```csharp
+Timer.DelayCall(InvalidateProperties);
+```
+
+**Check when writing a `GetProperties` override**: every property it reads must be a pure read.
+
 ## ObjectPropertyList Internals
 
 Defined in `Projects/Server/PropertyList/ObjectPropertyList.cs`: