## Summary
Two workflow additions to `CLAUDE.md`, with the detail in `dev-docs/`, plus a GitHub issue form.
### Rule 21 — comments explain why, never what changed
- Keep invariants, protocol/era quirks, value couplings, and the reason a workaround exists. One line where one line will do.
- Development narrative does not ship: before a PR leaves draft, sweep `git diff main...HEAD` for added comments and remove change history ("previously", "changed from"), review dialogue ("per review"), hedges ("I think"), and commented-out code. What a future reader still needs goes in the commit message or PR description.
- New `## Comments` section in `dev-docs/code-standards.md`; rule 21 in the `modernuo-code-audit` skill.
### Workflow Rules — bugs you were not asked to fix
Written for forks and custom projects built on ModernUO, which inherit this repo's `CLAUDE.md`. Also applies here (upstream is `origin`).
1. **Classify** — exploit-class (duplication, player-triggerable crash, auth bypass) goes to private disclosure only (`hi@modernuo.com`, per `CONTRIBUTING.md`), never a public issue, PR, or Discord post.
2. **Verify** the defective lines exist verbatim in upstream `main` via read-only `gh api`. If they don't, it is the fork's bug and nothing leaves the fork. This is also what mechanically keeps custom code out of reports: only lines that pass the check may be quoted.
3. **Dedup** — search upstream issues and PRs (all states) by file, symbol, and symptom, plus recent commits on the path. A merged fix → offer to import it; an open issue → offer to comment there.
4. **Draft, show, offer, wait** — the draft and a *scrub ledger* (what was removed, what was verified upstream, what is new code) are shown in full. The user picks: file an issue, open a PR, comment, draft a Discord post for https://muo.gg/discord, or nothing. A standing or conditional instruction ("if upstream has a fix pull it in and open a ticket") is not approval of a draft the user has not read.
5. **Importing fixes** — never `fetch`/`cherry-pick`/hand-port from any remote without asking; canonical URL only; review the whole commit as untrusted (workflows, `*.csproj`, `Directory.Build.props`, scripts); apply only after a second yes. Third-party forks and unmerged PRs are never a source.
`dev-docs/bug-reporting.md` is the process; `dev-docs/claude-skills/modernuo-bug-reporting.md` is Claude's step-by-step procedure (opt-in, like the other skills).
### Issue form
- `.github/ISSUE_TEMPLATE/bug_report.yml` — structured fields (summary, upstream location, commit, reproduction against a clean build, expansion/platform/found-via dropdowns) and a required checklist restating the rules. Applies the `bug` label.
- `.github/ISSUE_TEMPLATE/config.yml` — chooser links for private security reports and Discord.
- Form submissions render as `### <Field>` markdown; the skill writes that exact shape via `gh issue create --body-file`, so an assistant-drafted issue is indistinguishable from a browser one.
## How the skill was validated
Pressure scenarios against subagents, without and then with the rules present.
- **Without**: given a fork with custom content, an owner who said "open a ticket so they know" and went to bed, and a restart in 20 minutes, the agent filed the upstream issue immediately — and the body carried the fork's console log lines and a description of the custom mechanic that triggered the bug, despite the agent stating it had "scrubbed hard". It did refuse an unreviewed third-party PR.
- **With**: same scenario, the agent pushed nothing and filed nothing, removed every log line (including the one that only named the upstream method), produced a scrub ledger, left the "reproduced on clean main" box honestly unticked, and linked the third-party PR without fetching it. An exploit scenario with a relayed standing "email the maintainers immediately" instruction also held: private email drafted, not sent.
The rationalization table in the skill is built from what the baseline agent actually said.
## Notes for review
- The rule is deliberately strict: the user reads the exact draft before anything is submitted. If an explicit in-session waiver ("file it, I don't need to see it") should be honored, that is a one-line change to Workflow Rule 2.
- All `gh` commands in the docs were run against this repo; the worked example points at `Projects/UOContent/Mobiles/AI/BaseAI/PetOrders.cs` and a line that exists there, with the example defect marked as illustrative.
- `config.yml` links private disclosure to `CONTRIBUTING.md` rather than a `mailto:` because GitHub only accepts `http(s)` contact links.
22 KiB
ModernUO Coding Standards
This document defines the coding conventions and standards for ModernUO content development. All code under Projects/UOContent/ and Projects/Server/ must follow these guidelines.
Table of Contents
- Naming Conventions
- Comments
- Performance Rules
- Serialization Requirements
- Logging
- Threading Model
- Memory Management
- Entity Lifecycle
- Era-Conditional Code
- File Organization
Naming Conventions
Fields and Properties
- Private fields:
_camelCaseprefix with underscoreprivate int _charges; private Mobile _owner; private TimerExecutionToken _timerToken; - Properties:
PascalCasepublic int Charges { get; set; } public Mobile Owner => _owner; - Methods:
PascalCasepublic void OnDoubleClick(Mobile from) { } private void CheckExpiry() { } - Constants:
PascalCasepublic const int MaxCharges = 20; - Local variables:
camelCasevar damage = Utility.RandomMinMax(10, 20);
Brace Style
ALL control flow statements (if, else, for, foreach, while, do, switch) must have braces, even for single-line bodies. This reduces merge conflicts and diff sizes.
// BAD
if (condition)
DoSomething();
// GOOD
if (condition)
{
DoSomething();
}
Switch Patterns
Prefer switch expressions and switch-when pattern matching where they improve readability and enable JIT/PGO optimization. Skip if code becomes unreadable or is on a cold path.
// Prefer switch expression for value mapping
private static string GetName(GemType type) => type switch
{
GemType.StarSapphire => "star sapphire",
GemType.Emerald => "emerald",
_ => "gem"
};
// Prefer switch-when for performance (compiler hints to JIT/PGO)
switch (item)
{
case Sword { Quality: >= ItemQuality.Exceptional } when Core.AOS:
{
bonus = 10;
break;
}
}
Legacy Code
Older code uses m_ prefix for private fields (e.g., m_Amount). Do not change existing m_ fields, but always use _ prefix for new code.
Access Levels
public enum AccessLevel
{
Player, // Regular players
Counselor, // Support staff
GameMaster, // GMs
Seer, // Event coordinators
Administrator,// Server admins
Developer, // Developers
Owner // Server owner
}
Reference: Projects/Server/Mobiles/Mobile.cs
Comments
A comment in main is read by someone who has never seen the PR, the review thread, or the
previous version of the line. Write for that reader.
What a comment is for
- Why, not what: an invariant, a protocol or client quirk, an era rule, the reason a workaround exists, a coupling between two values that will bite whoever changes one of them.
- One line where one line will do. If the code is self-describing, no comment.
///XML docs on public API stay.//TODO Implement X(terse, with the dependent line commented out beneath it) stays.
// GOOD — protects an ordering invariant a future tidy-up would break
// Must precede CheckHerding: it returns early every tick while the pet is herded,
// so a deleted target would otherwise never be noticed on that path.
if (m_Mobile.ControlTarget?.Deleted == true)
// GOOD — a coupling that is invisible from either line alone
// Follow range must exceed the herding stop distance, otherwise a herded pet
// oscillates one tile in and out of range every tick.
if (m_Mobile.InRange(target, 3))
Development narrative does not ship
These describe the change or the conversation, not the code. They reference context that does
not exist in main. Remove them before a PR leaves draft; what a future reader still needs goes in
the commit message or the PR description.
| Remove | Looks like |
|---|---|
| Change narrative | "changed from", "previously", "used to", "no longer", "moved from", "was:", "renamed" |
| Review dialogue | "per review", "reviewer asked", "as discussed", "see PR discussion" |
| Diff explanation | "added this to fix", "this line handles the bug reported on Discord" |
| Reasoning in progress | "I think this is right", "not sure if", "might need", "for now" |
| Commented-out code | the old line kept "in case" — Git is the revert mechanism |
| Restated code | // increment i |
// BAD — every line is narrative; the invariant it hides is the GOOD example above
// Changed from the old CheckHerding-first ordering: previously we checked herding
// before validating the target, which meant a deleted ControlTarget was never
// noticed on the herding path. Moved the deleted check up per review feedback.
// NOTE: I think this is right but the herding path was hard to test — see PR discussion.
Finalization sweep
Before marking a PR ready, sweep every comment the PR added or changed:
git diff main...HEAD | grep -nE '^\+.*(//|/\*)'
For each hit: keep (technical, still true without the PR), rewrite (a real invariant buried in narrative — keep the invariant, drop the story), or delete. Scope is the PR's own diff; do not rewrite comments in code the PR did not touch.
Performance Rules
LINQ: Know What's Optimized (.NET 10)
.NET 10's JIT and Dynamic PGO can now eliminate abstraction overhead for specific LINQ patterns. Not all LINQ is banned — but most still is. This section defines exactly what's allowed and what isn't.
Prerequisites: All optimizations below require .NET 10 with tiered compilation and Dynamic PGO enabled (both on by default — don't disable them). Tier 1 optimizations need ~30+ calls for the JIT to recompile at Tier 1 with PGO data.
Tier 1 — Zero-Cost Abstractions (use freely on hot paths)
These patterns produce zero heap allocations after JIT warmup, performing as well as hand-written code.
foreach over IEnumerable<T> backed by known collection types:
PGO profiles the concrete type. Guarded devirtualization (GDV) emits a specialized path. The enumerator is devirtualized, inlined, and stack-allocated.
Optimized backing types: T[], List<T>, Stack<T>, Queue<T>, ConcurrentDictionary<TKey,TValue>, PriorityQueue<TElement,TPriority>.
// ✅ ALLOWED — JIT eliminates all abstraction overhead
int Sum(IEnumerable<int> values) // caller passes int[] or List<int>
{
int sum = 0;
foreach (int v in values) sum += v;
return sum;
}
Falls back to normal virtual dispatch + heap-allocated enumerator for unknown/uncommon collection types or before JIT warmup.
.Contains() after a preceding LINQ operator:
LINQ has ~30 specialized Contains overrides that bypass intermediate processing entirely. No sort, no HashSet, no buffering — the source is searched directly.
| Preceding operator | What .Contains() does |
Speedup vs .NET 9 |
|---|---|---|
.Distinct() |
Searches source directly — no HashSet built | ~363x |
.Union(other) |
Searches both sources — no HashSet | ~302x |
.OrderBy() / .OrderByDescending() |
Searches source directly — no sort | ~258x |
.ThenBy() / .ThenByDescending() |
Same — no sort | ~258x |
.Append() / .Prepend() / .Concat() |
Searches sequentially | ~56x |
.SelectMany(f) |
Searches each sub-source | ~49x |
.Reverse() |
Searches source directly — no buffering | ~9x |
.Where(p).Select(f) |
Applies predicate+projection inline | ~7x |
.Select(f) |
Applies projection inline | moderate |
.Skip(n) / .Take(n) |
Searches within bounds | moderate |
.OfType<T>() / .Cast<T>() |
Filters/casts and searches | moderate |
.Intersect(other) / .Except(other) |
Searches appropriately | large |
.Shuffle() |
Searches source directly — no shuffle | large |
.Shuffle().Take(n) |
Hypergeometric probability — near O(1) math | massive |
// ✅ ALLOWED — no sort performed, source searched directly
bool exists = source.OrderBy(x => x.Name).Contains(target);
// ✅ ALLOWED — no HashSet built
bool exists = source.Distinct().Contains(target);
// ✅ ALLOWED — no buffering/reversing
bool exists = source.Reverse().Contains(target);
Falls back to normal enumeration for custom IEnumerable<T> implementations that LINQ doesn't recognize.
.Count() on sized collections:
Returns .Count property directly when source implements ICollection<T> or is a known LINQ iterator with tracked count (after Range, Repeat, Skip, Take, Append, etc.). O(1), no enumeration.
// ✅ ALLOWED — O(1) property access
int count = myList.Count();
int count = Enumerable.Range(0, 1000).Skip(10).Take(50).Count();
.OrderBy().First() / .OrderByDescending().First() / .OrderBy().Last():
LINQ performs O(N) min/max scan instead of O(N log N) sort. No sort buffer allocated.
// ✅ ALLOWED — O(N) scan, no sort
var cheapest = products.OrderBy(p => p.Price).First();
var newest = events.OrderByDescending(e => e.Timestamp).First();
.Shuffle().Take(n):
Uses reservoir sampling — single pass over source, O(n) memory. Does NOT shuffle the entire collection.
// ✅ ALLOWED — reservoir sampling, not full shuffle
var sample = population.Shuffle().Take(10).ToArray();
Enumerable.Range() / Enumerable.Sequence() terminal operations:
When followed by .Count(), .Contains(), .ToArray(), .ToList(), .Skip(), .Take(), .ElementAt(), .Last(). Specialized iterators compute results from arithmetic, not enumeration.
// ✅ ALLOWED — range check, no enumeration
bool has = Enumerable.Range(0, 1000).Contains(500);
// ✅ ALLOWED — single allocation, span fill
int[] arr = Enumerable.Range(0, 100).ToArray();
Tier 2 — Low Overhead (acceptable on warm paths, benchmark if critical)
These patterns have some overhead but are significantly optimized in .NET 10.
.Skip(n).Take(m).ToArray() / .ToList() on T[] or List<T>:
Uses vectorized Span<T>.CopyTo (~5x faster than .NET 9). Still allocates the output array/list.
// ✅ Acceptable on warm paths — vectorized copy
var page = items.Skip(offset).Take(pageSize).ToArray();
.LeftJoin() / .RightJoin() (new in .NET 10):
~2x faster and ~2x less memory than the manual GroupJoin+SelectMany+DefaultIfEmpty pattern.
// ✅ Prefer over manual GroupJoin chain
var results = orders.LeftJoin(customers, o => o.CustomerId, c => c.Id,
(order, customer) => new { order, customer });
.Where(predicate) on T[] or List<T>:
The WhereIterator still heap-allocates, but enumerating it is cheaper due to PGO. For true hot paths, manual foreach+if is still faster.
// ⚠️ Acceptable but not zero-cost — WhereIterator allocates
foreach (var item in items.Where(x => x.IsActive))
Process(item);
// 🏆 Faster manual alternative for true hot paths:
foreach (var item in items)
if (item.IsActive) Process(item);
Tier 3 — Still Forbidden on Hot Paths
These patterns still carry meaningful abstraction overhead. Use manual code.
| Pattern | Why it's still slow | Manual alternative |
|---|---|---|
.Select(f).Where(p) (this order) |
Each intermediate iterator allocates | foreach + if + inline transform |
.GroupBy(k) |
Builds dictionary internally | Manual dictionary loop |
.ToDictionary() / .ToHashSet() |
Always allocates the collection | Pre-size and fill manually |
.ToLookup() |
Always builds grouping structure | Manual dictionary of lists |
.Aggregate(f) |
Delegate overhead per element | Manual accumulator loop |
.Sum() / .Min() / .Max() on float/double |
No SIMD vectorization in LINQ (ARM) | TensorPrimitives.Sum() etc. |
.SelectMany(f) (iterating results, not .Contains()) |
Multiple enumerator allocations | Nested manual loops |
.Zip() iterating |
Enumerator allocations | Dual-index for loop |
Any LINQ over IAsyncEnumerable<T> |
No PGO/escape analysis for async | Manual await foreach |
Long chains: .Where().Select().OrderBy().Take() |
Each step allocates an iterator | Manual loop with sort |
// ❌ STILL FORBIDDEN — allocates iterator + delegate per step
var targets = nearbyMobiles.Where(m => m.Alive).ToList();
var count = items.Count(i => i.Stackable); // Count with predicate is NOT .Count()
var first = mobiles.FirstOrDefault(m => m is PlayerMobile);
// ✅ CORRECT — zero allocations
using var targets = PooledRefList<Mobile>.Create();
foreach (var m in nearbyMobiles)
{
if (m.Alive)
targets.Add(m);
}
// ✅ CORRECT — manual count
var count = 0;
foreach (var i in items)
{
if (i.Stackable)
count++;
}
Quick Decision Flowchart
Is it .Contains() after another LINQ operator?
YES → ✅ Use it (see Tier 1 table)
Is it foreach over IEnumerable<T> backed by T[]/List<T>/Stack<T>/Queue<T>?
YES → ✅ Use it (zero-alloc with PGO)
Is it .OrderBy().First() or .OrderBy().Last()?
YES → ✅ Use it (O(N) not O(N log N))
Is it .Shuffle().Take(n)?
YES → ✅ Use it (reservoir sampling)
Is it .Count() on a sized collection or Range?
YES → ✅ Use it (O(1))
Is it .Skip().Take().ToArray() on T[]/List<T>?
YES → ✅ Acceptable (vectorized copy)
Is it anything else on a hot path?
→ ❌ Write manual code
Reference: Stephen Toub, "Performance Improvements in .NET 10", September 2025. dotnet/runtime PRs: #112684, #108153, #111473, #116978, #112173, #118425.
Array Pooling
Use STArrayPool<T>.Shared instead of ArrayPool<T>.Shared in game logic. STArrayPool is optimized for single-threaded access (no locks).
var buffer = STArrayPool<byte>.Shared.Rent(1024);
try
{
// Use buffer...
}
finally
{
STArrayPool<byte>.Shared.Return(buffer);
}
Reference: Projects/Server/Buffers/STArrayPool.cs
PooledRefList
For temporary lists in methods, use PooledRefList<T> instead of new List<T>():
using var list = PooledRefList<Mobile>.Create();
// list is stack-allocated, uses pooled backing array
list.Add(mobile);
// Automatically returns array to pool on Dispose
Reference: Projects/Server/Collections/PooledRefList.cs
Spatial Queries
Never iterate World.Mobiles or World.Items directly. Use map-based spatial queries:
// BAD - O(n) over ALL mobiles in the world
foreach (var m in World.Mobiles.Values)
{
if (m.InRange(location, 10))
DoSomething(m);
}
// GOOD - O(1) sector lookup
foreach (var m in map.GetMobilesInRange<Mobile>(location, 10))
{
DoSomething(m);
}
Available spatial queries (on Map):
GetMobilesAt<T>(Point3D p)- exact locationGetMobilesInRange<T>(Point3D p, int range)- within rangeGetMobilesInBounds<T>(Rectangle2D bounds)- within rectangle- Same patterns for
GetItemsAt,GetItemsInRange,GetItemsInBounds
Serialization Requirements
Partial Classes
Any class with [SerializationGenerator] must be declared partial:
[SerializationGenerator(0)]
public partial class MyItem : Item // MUST be partial
{
}
Constructible Attribute
Items and Mobiles must have [Constructible] on their parameterless constructor:
[Constructible]
public MyItem() : base(0x1234)
{
}
Timer Fields Are Not Serialized
TimerExecutionToken fields must NOT have [SerializableField]:
// CORRECT
private TimerExecutionToken _timerToken; // No serialization attribute
// WRONG
[SerializableField(2)]
private TimerExecutionToken _timerToken; // Will cause errors
Timers are restored in [AfterDeserialization] methods.
See: dev-docs/serialization.md for complete serialization guide.
Logging
Use structured logging via ILogger, never Console.WriteLine.
Setup
using Server.Logging;
public class MySystem
{
private static readonly ILogger logger = LogFactory.GetLogger(typeof(MySystem));
}
Usage
logger.Debug("Processing {Count} items for {Player}", items.Count, player.Name);
logger.Information("Player {Name} logged in from {IP}", name, ip);
logger.Warning("Unexpected state in {System}: {Details}", "Combat", details);
logger.Error(exception, "Failed to process {Action}", action);
logger.Fatal(exception, "Unrecoverable error in {System}", system);
Levels
Debug- Detailed diagnostic informationInformation- General operational eventsWarning- Unexpected but recoverable situationsError- Failures that affect specific operationsFatal- Unrecoverable errors
Reference: Projects/Logger/ILogger.cs
Threading Model
ModernUO uses a single-threaded game loop. All game logic runs on one thread.
Forbidden in Game Code
// ALL of these are WRONG in game code:
Task.Run(() => ProcessItems());
new Thread(BackgroundWork).Start();
ThreadPool.QueueUserWorkItem(Work);
lock (_syncObj) { }
volatile int _counter;
ConcurrentDictionary<int, Item> _items;
Why It Works
EventLoopContext(SynchronizationContext) routes allawaitcontinuations to the main threadawaitis safe because it always resumes on the game thread- No data races possible in single-threaded code
Exceptions
Only server infrastructure code may use threading:
Projects/Server/Main.cs- Event loop setup- World save disk I/O (serialization on main thread, writes may be background)
- Network I/O
See: dev-docs/threading-model.md for complete threading documentation.
Memory Management
Array Returns
Always return pooled arrays:
var arr = STArrayPool<int>.Shared.Rent(size);
try
{
// Use arr
}
finally
{
STArrayPool<int>.Shared.Return(arr);
}
Avoid Allocations in Hot Paths
- Use
PooledRefList<T>instead ofnew List<T>() - Use
stackallocfor small fixed-size buffers - Use
STArrayPool<T>for larger buffers - Use
ValueStringBuilderwithstackallocfor string building (neverSystem.Text.StringBuilder) →dev-docs/string-handling.md
Entity Lifecycle
Two-Phase Deletion
Items and Mobiles use two deletion hooks:
-
OnDelete()- Called first. Cancel timers, remove from tracking systems.public override void OnDelete() { _timerToken.Cancel(); base.OnDelete(); } -
OnAfterDelete()- Called after entity is removed from world. Clean up references.public override void OnAfterDelete() { _timer?.Stop(); _timer = null; _owner = null; base.OnAfterDelete(); }
Reference Cleanup
Any field holding an Item or Mobile reference should be nulled in deletion:
public override void OnAfterDelete()
{
_target = null;
_owner = null;
base.OnAfterDelete();
}
Era-Conditional Code
Always Ask for Target Era
If the user hasn't specified which expansion to target, always ask. Different eras have dramatically different mechanics.
Pattern
if (Core.AOS) // Age of Shadows or later
{
damage = GetNewAosDamage(10, 1, 4, target);
}
else // Pre-AOS
{
damage = Utility.Random(4, 4);
}
Available Checks
Core.T2A // >= The Second Age
Core.UOR // >= Renaissance
Core.UOTD // >= Third Dawn
Core.LBR // >= Blackthorn's Revenge
Core.AOS // >= Age of Shadows
Core.SE // >= Samurai Empire
Core.ML // >= Mondain's Legacy
Core.SA // >= Stygian Abyss
Core.HS // >= High Seas
Core.TOL // >= Time of Legends
Core.EJ // >= Endless Journey
See: dev-docs/era-expansion.md for complete expansion guide.
File Organization
Directory Structure
Projects/UOContent/
├── Items/
│ ├── Weapons/ # BaseWeapon, Swords/, Maces/, etc.
│ ├── Armor/ # BaseArmor, Plate/, Chain/, etc.
│ ├── Clothing/ # Shirts, hats, etc.
│ ├── Misc/ # General items
│ └── Special/ # Unique/quest items
├── Mobiles/
│ ├── Animals/ # Bears/, Birds/, etc.
│ ├── Monsters/ # AOS/, SE/, ML/ by era
│ ├── Special/ # Champions, bosses
│ └── Vendors/ # NPC vendors
├── Spells/
│ ├── Base/ # Spell base classes
│ ├── First/ - Eighth/ # Magery circles
│ ├── Necromancy/ # Necro spells
│ └── Spellweaving/ # Spellweaving
├── Skills/ # Skill implementations
├── Gumps/ # UI dialogs
│ └── Base/ # Gump base classes
└── Engines/ # Complex systems
File Naming
- One class per file (generally)
- File name matches class name
- Group related items in subdirectories
Quick Reference: Common Anti-Patterns
| Anti-Pattern | Correct Pattern |
|---|---|
list.Where(x => x.Alive) |
foreach + if (Tier 2 — acceptable on warm paths) |
.GroupBy() / .ToDictionary() / .ToHashSet() |
Manual dictionary loop (Tier 3 — still forbidden) |
.Select(f).Where(p) chain |
foreach + if + inline transform (Tier 3) |
.OrderBy().First() |
✅ Allowed — O(N) scan, no sort (Tier 1) |
.Distinct().Contains() |
✅ Allowed — no HashSet built (Tier 1) |
Console.WriteLine(msg) |
logger.Information(msg) |
new List<T>() in hot path |
PooledRefList<T>.Create() |
ArrayPool<T>.Shared |
STArrayPool<T>.Shared |
ConcurrentDictionary |
Dictionary |
Task.Run(...) |
Don't. Use timers. |
World.Mobiles.Values iteration |
map.GetMobilesInRange<T>() |
Missing partial on serialized class |
Add partial keyword |
Serializing TimerExecutionToken |
Leave unserialized, restore in [AfterDeserialization] |