docs: rewrite README for the TextCommandCallingArgs API

FireBefore continued invoking every registered listener even after one had set Cancel, running callbacks against a command that was already dead. Break out of the loop as soon as Cancel is set.

CommandHook/ChatCommandApiPatch.cs

@@ -1,14 +1,56 @@
 using System;
 using HarmonyLib;
 using Vintagestory.API.Common;
-using Vintagestory.API.Server;
 using Vintagestory.Common;
 
 namespace CommandHook;
 
+/// <summary>
+/// Harmony prefix patch on <c>ChatCommandApi.Execute</c>. This is the only place
+/// CommandHook hooks into the game, everything else in the mod feeds into this.
+/// </summary>
 [HarmonyPatch(typeof(ChatCommandApi))]
 public static class ChatCommandApiPatch
 {
+    /// <summary>
+    /// Runs before the game's own command execution. Fires Before listeners for
+    /// <paramref name="commandName"/> and, if none of them cancel, lets the
+    /// original <c>Execute</c> run and wraps <paramref name="onCommandComplete"/>
+    /// so After listeners fire once a result exists.
+    /// </summary>
+    /// <remarks>
+    /// Runs for both invocation paths, server console and a connected player
+    /// typing in chat, since both go through <c>Execute</c> the same way
+    /// internally.
+    /// <para>
+    /// If a Before listener returns a non-null <see cref="TextCommandResult"/>,
+    /// this returns false to stop the original <c>Execute</c> from running at
+    /// all, and invokes <paramref name="onCommandComplete"/> directly with that
+    /// result so the caller sees whatever the listener intended (a real error
+    /// message, a silent <see cref="TextCommandResult.Deferred"/>, etc). After
+    /// listeners are not fired in this case.
+    /// </para>
+    /// <para>
+    /// If nothing cancels, this returns true and lets the original <c>Execute</c>
+    /// run, but first replaces <paramref name="onCommandComplete"/> with a wrapper
+    /// that calls the original callback, then fires After. The original callback
+    /// always runs first, so nothing about the vanilla command flow changes from
+    /// the caller's point of view.
+    /// </para>
+    /// </remarks>
+    /// <param name="commandName">The command name being executed, without the leading slash.</param>
+    /// <param name="args">
+    /// The calling args supplied by the game. Passed straight through to listeners,
+    /// live and unmodified, the engine's own object for this invocation.
+    /// </param>
+    /// <param name="onCommandComplete">
+    /// The completion callback supplied by the game. Replaced with a wrapper that
+    /// fires After listeners after invoking the original.
+    /// </param>
+    /// <returns>
+    /// False to skip the original <c>Execute</c> (a Before listener cancelled it),
+    /// true to let it run normally.
+    /// </returns>
     [HarmonyPatch(
         "Execute",
         new[] { typeof(string), typeof(TextCommandCallingArgs), typeof(Action<TextCommandResult>) }
@@ -21,26 +63,27 @@ public static class ChatCommandApiPatch
     )
     {
         var system = CommandHookModSystem.Instance;
+        // Instance is null before StartServerSide runs or after Dispose. Harmony
+        // patches stay applied for the process lifetime, so this null check is
+        // what actually guards against firing into a torn-down mod.
         if (system == null)
             return true;
 
-        var sender = args.Caller.Player as IServerPlayer;
+        var cancelResult = system.FireBefore(commandName, args);
-        var data = new CommandData(sender, "/" + commandName);
+        if (cancelResult != null)
-
-        if (system.FireBefore(commandName, ref data))
         {
-            onCommandComplete?.Invoke(
+            onCommandComplete?.Invoke(cancelResult);
-                new TextCommandResult { Status = EnumCommandStatus.Deferred }
-            );
             return false;
         }
 
+        // Not cancelled. Capture the game's own callback before replacing it,
+        // since we still need to call it, this patch only adds behavior around
+        // the vanilla flow, it never removes the vanilla completion handling.
         var original = onCommandComplete;
         onCommandComplete = result =>
         {
             original?.Invoke(result);
-            var afterData = data;
+            system.FireAfter(commandName, args, result);
-            system.FireAfter(commandName, ref afterData, result);
         };
 
         return true;

CommandHook/CommandData.cs

@@ -1,29 +0,0 @@
-using Vintagestory.API.Server;
-
-namespace CommandHook;
-
-public struct CommandData
-{
-    public readonly IServerPlayer? Sender;
-    public readonly string FullCommand;
-    public readonly string CommandName;
-    private byte flags;
-
-    private const byte FlagIsPlayerCommand = 1 << 0;
-    private const byte FlagCancel = 1 << 1;
-
-    public bool IsPlayerCommand => (flags & FlagIsPlayerCommand) != 0;
-    public bool Cancel
-    {
-        get => (flags & FlagCancel) != 0;
-        set => flags = value ? (byte)(flags | FlagCancel) : (byte)(flags & ~FlagCancel);
-    }
-
-    public CommandData(IServerPlayer? sender, string fullCommand)
-    {
-        Sender = sender;
-        FullCommand = fullCommand.Length > 0 ? fullCommand.Trim() : string.Empty;
-        CommandName = FullCommand.Length > 1 ? FullCommand[1..].Split(' ')[0] : string.Empty;
-        flags = sender != null ? FlagIsPlayerCommand : (byte)0;
-    }
-}

CommandHook/CommandDelegate.cs

@@ -2,5 +2,29 @@ using Vintagestory.API.Common;
 
 namespace CommandHook;
 
-public delegate void CommandBeforeDelegate(ref CommandData data);
+/// <summary>
-public delegate void CommandAfterDelegate(ref CommandData data, TextCommandResult result);
+/// Invoked before a watched command runs. Return a non-null
+/// <see cref="TextCommandResult"/> to cancel the command and report that
+/// result to the caller (e.g. <see cref="TextCommandResult.Error(string, string)"/>
+/// for a visible reason, or <see cref="TextCommandResult.Deferred"/> to cancel
+/// silently). Return null to let the command run normally.
+/// </summary>
+/// <param name="callingArgs">
+/// The live calling args for this invocation, supplied directly by the engine.
+/// </param>
+public delegate TextCommandResult? CommandBeforeDelegate(TextCommandCallingArgs callingArgs);
+
+/// <summary>
+/// Invoked after a watched command has run. Not invoked if a Before listener
+/// (for this command, any mod) cancelled it.
+/// </summary>
+/// <param name="callingArgs">
+/// The same live calling args object passed to Before. By now the real
+/// command handler has had full access to it, and what's safe to read
+/// depends on how that command is implemented internally.
+/// </param>
+/// <param name="result">The result the command produced.</param>
+public delegate void CommandAfterDelegate(
+    TextCommandCallingArgs callingArgs,
+    TextCommandResult result
+);

CommandHook/CommandHookModSystem.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Collections.Frozen;
 using System.Collections.Generic;
 using HarmonyLib;
@@ -6,12 +7,25 @@ using Vintagestory.API.Server;
 
 namespace CommandHook;
 
+/// <summary>
+/// Server-side event bus for chat commands. Other mods register an
+/// <see cref="ICommandHookListener"/> here to get Before/After callbacks on
+/// specific command names, namespaced by <see cref="ICommandHookListener.ModId"/>
+/// so multiple mods can watch the same command without stepping on each other.
+/// </summary>
+/// <remarks>
+/// Patches <c>ChatCommandApi.Execute</c> via Harmony. Server only, see
+/// <see cref="ShouldLoad"/>. A listener's <see cref="ICommandHookListener.Commands"/>
+/// and <see cref="ICommandHookListener.Registration"/> are read at registration
+/// time and cached in a <see cref="System.Collections.Frozen.FrozenDictionary{TKey,TValue}"/>
+/// for fast lookup on the command dispatch path. If you change which commands a
+/// listener watches, call <see cref="Register"/> again with the updated list.
+/// </remarks>
 public class CommandHookModSystem : ModSystem
 {
     internal static CommandHookModSystem? Instance;
 
     private readonly List<ICommandHookListener> listeners = new();
-    private readonly List<ICommandHookListener> wildcards = new();
 
     private FrozenDictionary<string, FrozenDictionary<string, CommandRegistration>> registrations =
         FrozenDictionary<string, FrozenDictionary<string, CommandRegistration>>.Empty;
@@ -28,8 +42,6 @@ public class CommandHookModSystem : ModSystem
         harmony = new Harmony(Mod.Info.ModID);
         harmony.PatchAll();
 
-        api.Event.ServerRunPhase(EnumServerRunPhase.RunGame, Rebuild);
-
         Mod.Logger.Notification("Loaded");
     }
 
@@ -37,10 +49,20 @@ public class CommandHookModSystem : ModSystem
     {
         harmony?.UnpatchAll(Mod.Info.ModID);
         listeners.Clear();
-        wildcards.Clear();
         Instance = null;
     }
 
+    /// <summary>
+    /// Registers a listener for the commands it returns from <see cref="ICommandHookListener.Commands"/>.
+    /// </summary>
+    /// <remarks>
+    /// Calling this again for a <see cref="ICommandHookListener.ModId"/> that is already
+    /// registered replaces its previous registration. If the new <see cref="ICommandHookListener.Commands"/>
+    /// list is identical to the existing one this is a no-op, no rebuild happens.
+    /// If <paramref name="listener"/> has a null or empty <see cref="ICommandHookListener.Commands"/>
+    /// list, this just calls <see cref="Unregister"/> instead.
+    /// </remarks>
+    /// <param name="listener">The listener to register or update.</param>
     public void Register(ICommandHookListener listener)
     {
         var commands = listener.Commands;
@@ -51,38 +73,48 @@ public class CommandHookModSystem : ModSystem
             return;
         }
 
-        for (int i = 0; i < listeners.Count; i++)
+        int index = listeners.FindIndex(l => l.ModId == listener.ModId);
+
+        bool changed = false;
+        if (index < 0)
         {
-            if (listeners[i].ModId != listener.ModId)
+            listeners.Add(listener);
-                continue;
+            changed = true;
-
+        }
-            if (CommandListEquals(listeners[i].Commands, commands))
+        else if (!CommandListEquals(listeners[index].Commands, commands))
-                return;
+        {
-
+            listeners[index] = listener;
-            listeners[i] = listener;
+            changed = true;
-            SyncWildcard(listener, commands);
-            Rebuild();
-            return;
         }
 
-        listeners.Add(listener);
+        if (changed)
-        SyncWildcard(listener, commands);
+            Rebuild();
     }
 
+    /// <summary>
+    /// Removes a listener's registration entirely. Safe to call on a listener
+    /// that was never registered, this is a no-op in that case.
+    /// </summary>
+    /// <param name="listener">The listener to remove, matched by <see cref="ICommandHookListener.ModId"/>.</param>
     public void Unregister(ICommandHookListener listener)
     {
-        for (int i = 0; i < listeners.Count; i++)
+        int index = listeners.FindIndex(l => l.ModId == listener.ModId);
-        {
-            if (listeners[i].ModId != listener.ModId)
-                continue;
 
-            listeners.RemoveAt(i);
+        if (index < 0)
-            wildcards.RemoveAll(w => w.ModId == listener.ModId);
-            Rebuild();
             return;
-        }
+
+        listeners.RemoveAt(index);
+        Rebuild();
     }
 
+    // Rebuilds the dispatch table from listeners. Runs unconditionally and
+    // immediately whenever Register/Unregister actually changes something,
+    // no batching or startup-phase awareness. Command list sizes here are
+    // small (a handful of mods, a handful of commands each), so the realloc
+    // on every change is cheap and not worth the complexity of an incremental
+    // update. The two-level structure (command -> ModId -> registration) is
+    // what FireBefore/FireAfter walk on the hot path, so it's built once here
+    // instead of being derived per-dispatch.
     private void Rebuild()
     {
         var builder = new Dictionary<string, Dictionary<string, CommandRegistration>>();
@@ -93,10 +125,6 @@ public class CommandHookModSystem : ModSystem
             if (commands == null || commands.Count == 0)
                 continue;
 
-            bool isWildcard = commands.Count == 1 && commands[0] == "*";
-            if (isWildcard)
-                continue;
-
             foreach (var cmd in commands)
             {
                 if (!builder.TryGetValue(cmd, out var mods))
@@ -108,10 +136,6 @@ public class CommandHookModSystem : ModSystem
             }
         }
 
-        foreach (var wildcard in wildcards)
-        foreach (var mods in builder.Values)
-            mods[wildcard.ModId] = wildcard.Registration;
-
         var pruned = new Dictionary<string, FrozenDictionary<string, CommandRegistration>>(
             builder.Count
         );
@@ -122,30 +146,66 @@ public class CommandHookModSystem : ModSystem
         registrations = pruned.ToFrozenDictionary();
     }
 
-    internal bool FireBefore(string commandName, ref CommandData data)
+    // Called from ChatCommandApiPatch.Prefix before the real command executes.
+    // callingArgs is the engine's own live object, passed straight through,
+    // no allocation here. Each listener's Before is isolated in its own
+    // try/catch so one mod throwing doesn't stop the rest from running or
+    // break command dispatch for the server. The first listener to return a
+    // non-null result wins, we stop walking immediately rather than letting
+    // later listeners run against an already-cancelled command.
+    internal TextCommandResult? FireBefore(string commandName, TextCommandCallingArgs callingArgs)
     {
         if (registrations.TryGetValue(commandName, out var mods))
-            foreach (var (_, reg) in mods)
+        {
-                reg.Before?.Invoke(ref data);
+            foreach (var (modId, reg) in mods)
+            {
+                TextCommandResult? result = null;
+                try
+                {
+                    result = reg.Before?.Invoke(callingArgs);
+                }
+                catch (Exception ex)
+                {
+                    Mod.Logger.Error("[{0}] Before /{1} threw: {2}", modId, commandName, ex);
+                }
 
-        return data.Cancel;
+                if (result != null)
+                    return result;
+            }
+        }
+
+        return null;
     }
 
-    internal void FireAfter(string commandName, ref CommandData data, TextCommandResult result)
+    // Called from the wrapped onCommandComplete in ChatCommandApiPatch.Prefix,
+    // only on the path where the command actually ran (no Before listener
+    // cancelled it). Same per-listener try/catch as FireBefore.
+    internal void FireAfter(
+        string commandName,
+        TextCommandCallingArgs callingArgs,
+        TextCommandResult result
+    )
     {
         if (registrations.TryGetValue(commandName, out var mods))
-            foreach (var (_, reg) in mods)
+        {
-                reg.After?.Invoke(ref data, result);
+            foreach (var (modId, reg) in mods)
-    }
+            {
-
+                try
-    private void SyncWildcard(ICommandHookListener listener, IReadOnlyList<string> commands)
+                {
-    {
+                    reg.After?.Invoke(callingArgs, result);
-        bool isWildcard = commands.Count == 1 && commands[0] == "*";
+                }
-        wildcards.RemoveAll(w => w.ModId == listener.ModId);
+                catch (Exception ex)
-        if (isWildcard)
+                {
-            wildcards.Add(listener);
+                    Mod.Logger.Error("[{0}] After /{1} threw: {2}", modId, commandName, ex);
+                }
+            }
+        }
     }
 
+    // Order-sensitive on purpose. Register only treats an update as a no-op
+    // if the list is identical in order and content, a reordered list still
+    // triggers a rebuild. Cheap to check (just an index walk, no allocation)
+    // and correctness here matters more than being lenient about ordering.
     private static bool CommandListEquals(IReadOnlyList<string> a, IReadOnlyList<string> b)
     {
         if (a.Count != b.Count)

CommandHook/CommandRegistration.cs

@@ -1,10 +1,26 @@
 namespace CommandHook;
 
+/// <summary>
+/// The pair of callbacks a listener provides for a command. Either one can be null.
+/// </summary>
 public struct CommandRegistration
 {
+    /// <summary>
+    /// Called before the command runs. Return a non-null <see cref="TextCommandResult"/>
+    /// to cancel the command, report that result to the caller, and skip any
+    /// remaining Before listeners. Return null to let the command run.
+    /// </summary>
     public readonly CommandBeforeDelegate? Before;
+
+    /// <summary>
+    /// Called after the command runs, with its result. Not called if a Before
+    /// listener cancelled the command.
+    /// </summary>
     public readonly CommandAfterDelegate? After;
 
+    /// <summary>
+    /// Creates a registration. Pass null for either delegate to only hook one side.
+    /// </summary>
     public CommandRegistration(CommandBeforeDelegate? before, CommandAfterDelegate? after)
     {
         Before = before;

CommandHook/ICommandHookListener.cs

@@ -2,9 +2,27 @@ using System.Collections.Generic;
 
 namespace CommandHook;
 
+/// <summary>
+/// Implement this to watch chat commands through CommandHook. Pass your
+/// implementation to <see cref="CommandHookModSystem.Register"/>.
+/// </summary>
 public interface ICommandHookListener
 {
+    /// <summary>
+    /// Your mod's id. Used as the namespace key so two mods can register
+    /// for the same command without colliding, and to find your existing
+    /// registration when you call <see cref="CommandHookModSystem.Register"/> again.
+    /// </summary>
     string ModId { get; }
+
+    /// <summary>
+    /// The command names to watch, without the leading slash (e.g. "tp", not "/tp").
+    /// Return null or an empty list to stop watching anything.
+    /// </summary>
     IReadOnlyList<string> Commands { get; }
+
+    /// <summary>
+    /// The Before/After delegates to invoke for the commands in <see cref="Commands"/>.
+    /// </summary>
     CommandRegistration Registration { get; }
 }

CommandHook/assets/commandhook/lang/en.json

@@ -1,3 +1 @@
-{
+{}
-    "hello": "hello world!"
-}

CommandHook/modinfo.json

@@ -2,9 +2,11 @@
     "type": "code",
     "modid": "commandhook",
     "name": "CommandHook",
-    "description": "Exposes server command events for other mods to hook into.",
+    "description": "Adds before/after hooks for chat commands, for other mods to use.",
-    "authors": ["anth64"],
+    "authors": [
-    "version": "0.1.0",
+        "anth64"
+    ],
+    "version": "2.0.0",
     "side": "Server",
     "dependencies": {
         "game": "1.22.3"

README.md

@@ -0,0 +1,110 @@
+# CommandHook
+
+Server-side mod for Vintage Story. Lets other mods hook into chat commands
+before and after they run, namespaced per mod so multiple mods can watch
+the same command without colliding.
+
+## Usage
+
+Install it like any other mod. It has no effect unless something else
+depends on it.
+
+## For mod development
+
+Add it as a dependency in your `modinfo.json`:
+
+```json
+"dependencies": {
+    "game": "1.22.3",
+    "commandhook": "2.0.0"
+}
+```
+
+CommandHook patches `ChatCommandApi.Execute` and dispatches to whoever
+has registered. Your mod doesn't patch anything itself, it just implements
+`ICommandHookListener` and calls `CommandHookModSystem.Register`.
+
+A listener provides:
+
+- `ModId`, your mod's id. Used to namespace your registration so it doesn't
+  collide with another mod watching the same command
+- `Commands`, the command names you want to watch, without the leading slash
+- `Registration`, a `Before` delegate, an `After` delegate, or both
+
+`Before` receives the engine's own `TextCommandCallingArgs` for the
+invocation, live and unmodified. Return a non-null `TextCommandResult` to
+cancel the command and report that result to the caller, use the engine's
+own factories, e.g. `TextCommandResult.Error(...)` for a visible reason, or
+`TextCommandResult.Deferred` to cancel silently (per its own doc comment,
+this prints no output). Return `null` to let the command run normally. If
+you cancel, any remaining `Before` listeners for that command are skipped,
+and `After` never fires for that invocation.
+
+`After` receives the same `TextCommandCallingArgs` object plus the
+`TextCommandResult` the command actually produced. It only fires on the
+path where the command ran.
+
+### A note on `TextCommandCallingArgs` at Before vs After
+
+This is the engine's own live object, not a copy, what's safe to read
+depends on timing and on how the specific command you're watching is
+implemented internally.
+
+### Example
+
+```csharp
+public class MyListener : ICommandHookListener
+{
+    // Your mod's id, used to namespace this registration.
+    public string ModId => "mymod";
+
+    // Commands you want to watch, no leading slash.
+    public IReadOnlyList<string> Commands => new[] { "tp" };
+
+    // Wire up Before, After, or both.
+    public CommandRegistration Registration => new(Before, After);
+
+    private TextCommandResult? Before(TextCommandCallingArgs args)
+    {
+        // args.Caller.Player is null for console invocations.
+        if (args.Caller.Player is IServerPlayer player && !IsAllowed(player))
+        {
+            // Cancels the command, skips any remaining Before listeners,
+            // and After never fires for this invocation. The caller sees
+            // this exact result.
+            return TextCommandResult.Error("You're not allowed to do that", "notallowed");
+        }
+
+        return null;
+    }
+
+    private void After(TextCommandCallingArgs args, TextCommandResult result)
+    {
+        // Only runs if nothing cancelled. result is whatever the command
+        // actually produced, check result.Status for success/error/deferred.
+        if (result.Status != EnumCommandStatus.Success)
+            Logger.Warn($"A watched command failed: {result.StatusMessage}");
+    }
+}
+```
+
+Register it once your mod starts, unregister it on dispose:
+
+```csharp
+public override void StartServerSide(ICoreServerAPI api)
+{
+    // CommandHookModSystem.Instance is null if CommandHook isn't loaded,
+    // hence the ?. Register your listener once, here.
+    CommandHookModSystem.Instance?.Register(myListener);
+}
+
+public override void Dispose()
+{
+    // Always unregister on dispose, otherwise a stale listener stays in
+    // the dispatch table after your mod is gone.
+    CommandHookModSystem.Instance?.Unregister(myListener);
+}
+```
+
+Calling `Register` again with the same `ModId` replaces your existing
+registration. If your `Commands` list hasn't changed, it's a no-op.