Fix /kitedit registration error, add snapshot scope, and document kit commands - #294
Merged
Merged
Conversation
/kitedit was registered without RequiresPrivilege, so the command API rejected every invocation with "Programming error: Incomplete command - no name or required privilege has been set" before Stratum's own handler ever ran. Register it with Privilege.chat like every other Stratum command (/kit included); the configurable Commands.KitEdit check in CheckAccess stays the real gate, unchanged. Also reject arguments an action does not take (/kitedit create starter_kit 1 from the report silently ignored the trailing "1" instead of erroring), add a per-action usage line and an unknown-action message that lists valid actions, escape angle brackets in usage text so it renders in chat instead of being dropped as an unknown VTML tag, and improve both commands' in-game descriptions. Cover the fix with a console command probe in smoke-test.sh: a console caller always passes Stratum's access check and has no player entity, so piping /kitedit and /kit commands into the running server's stdin exercises registration, parsing, and argument handling end to end without a game client, including the exact command from the report. Fails red on the old registration, green after the fix. Also fixes a pre-existing bug in smoke-test.sh where launching the server with cwd=repo root broke Mono.Cecil's assembly resolution during mod loading (it searches the process's current directory), silently dropping every mod including the base "game" content and making every run fail before reaching RunGame. Fixes #293
Adds the public command reference the issue asked for: syntax, permissions, item snapshot behavior, assignment, cooldowns, flags, a typical workflow, troubleshooting table, and a symbol-to-behavior map so future implementation changes are easy to spot as needing a doc update. Addresses #293
create's snapshot has always walked hotbar and character inventory together, capturing whatever the creator was wearing at the time. Add a per-kit scope so an operator can opt out: /kitedit setscope <name> all|hotbar. "all" keeps the current behavior; "hotbar" limits create to the hotbar (offhand included, since it lives in the same inventory), never touching worn armor or clothing. Scope is a property of the kit, applied on the next create rather than retroactively, matching setrole/setcooldown/the flags. A brand new kit defaults to "all". preview now shows the current scope. Proven against a real connected player's real inventory (a worn armor piece included under all, excluded under hotbar) by a new case in the private Atlas regression suite; a console caller has no player entity to snapshot, so the public smoke test cannot exercise this path. Addresses #295
Zaldaryon
marked this pull request as ready for review
September 4, 2026 18:42
trevorftp
added a commit
that referenced
this pull request
Sep 6, 2026
* Fixing allocations inside ScheduleReadyTasks * Corrections for #273 * Improvements to AVX instructions in ChunkDataLayer * GenTerra.stratumGenerate optimizations * Fix 279 * Return to Floor rounding for identity * Closing issues with parity testing and preventing concurrent execution of the vanilla path * Fixes 1 and 2 for 279 v2 * Caves first edit * Caves delayed writes * Fix 279 v3 * Fix 285 * Fix 285 v2 * Fix 285 v3 * Fixed: Boats and rafts not responding to controls on servers * Fix 285 v4 * Fix 285 v5 * Fix 286 * Fix 285 v6 * Fix 288 * Fix 285 v7 * Skip server-side pose matrices until something reads them (#291) * Skip server-side pose matrices until something reads them Every player has requirePosesOnServer set, so ServerAnimator recomputed the whole seraph skeleton every tick for every player, and for every dead entity, while the server only reads poses through GetAttachmentPointPose and GetPosebyName in a handful of places. Mark the poses stale in calculateMatrices and recompute on the first read. Config: Performance.EntityTicking.LazyServerPoses (default true). Also replace the LINQ Any in AnimationManager.OnServerTick with a loop; it allocated an enumerator and a closure per entity per tick. Micro-benchmark on seraph.json, VintagestoryAPI 1.22.7: 5.5 to 7.4 us per player per tick with matrices, 0.01 to 0.14 us without. At 650 players that is 3.6 to 4.8 ms per tick, 30 to 40 percent of the entity.tick.players time in the 650-bot timings report. Lazy and eager poses compared over 6000 frames with random animation changes: 1842 attachment point reads, zero difference. * Lock the frame update with the lazy recompute, clear stale on eager path Review fixes for #291. OnFrame now holds the same lock as the lazy recompute so a physics-thread reader cannot rebuild poses from animation state the main thread is advancing. The eager path clears the stale flag so switching lazy poses off at runtime does not trigger one extra recompute per entity. The AnimationManager comment now says what the LINQ call actually cost: a boxed enumerator, not a closure. * Make the stale pose flag volatile The first check in StratumEnsurePosesFresh runs outside the lock as a fast path. Without volatile a physics-thread reader could miss the main thread setting the flag and hand out stale poses. Review fix for #291. * Fix /kitedit registration error, add snapshot scope, and document kit commands (#294) Fix /kitedit registration error, add snapshot scope, and document kit commands (#293, #295) * Snapshots for GenTerraPostProcess (#289) * Snapshots for GenTerraPostProcess * Fix 289 * Fixed: Prevent view distance griefing and invalid block break modes * Fixed: Close block interaction and mount movement exploits * Vectorizing SetLightBulkUnsafe (#296) * Vectorizing SetLightBulkUnsafe * Some improvements for 296 --------- Co-authored-by: tehtelev <tehtelev@gmail.com> Co-authored-by: tehtelev <50070668+tehtelev@users.noreply.github.com> Co-authored-by: Zaldaryon <273555259+Zaldaryon@users.noreply.github.com> Co-authored-by: Michael Andrzejewski <55041358+Michael-Andrzejewski@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related fixes to the kit commands:
/kiteditwas registered without a base command privilege, so Vintage Story's command API rejected every invocation before Stratum's own handler ever ran. Fixes Fix /kitedit registration error and document kit commands #293.create's snapshot always included worn armor/clothing with no way to opt out. Adds a per-kit/kitedit setscope <name> all|hotbarso an operator can limitcreateto the hotbar only. Addresses Add /kitedit scope option to exclude worn armor and clothing from create #295.Also adds a public command reference for
/kitand/kitedit, and a console-driven regression probe inscripts/smoke-test.sh.Part 1 — #293: registration error
Problem
Reported repro:
Server response:
Baseline reproduction
Confirmed directly against the pre-fix source by piping the reported command (and several others) into a running server's console:
Every
/kiteditinvocation failed the same way, regardless of action or arguments — the command never reachedHandleKitEditat all.Root cause
Vintage Story's command builder throws "Incomplete command - no name or required privilege has been set" when a registered command has no base privilege set.
/kitin the same file ends its registration with.RequiresPrivilege(Privilege.chat);/kiteditdid not have that call. Checked every other Stratum command registration inCmdStratumKits.cs,CmdStratumStaffCommands.cs, andCmdStratumRoles.cs:/kiteditwas the only one missing it.Secondary issues found while fixing this:
name,value), so a third word on a one-argument action (e.g. the reported1aftercreate starter_kit) was silently accepted and ignored instead of rejected.<name>insideTextCommandResult.Error(...); the client's chat renderer treats that as an unknown VTML tag and drops it silently, so the usage hint was invisible.Fix
sources/VintagestoryLib/Vintagestory.Server/CmdStratumKits.cs:.RequiresPrivilege(Privilege.chat)to the/kiteditregistration. This is the actual fix. The real, configurable gate (Commands.KitEdit/stratum.kitedit, enforced inCheckAccess) is unchanged.CheckActionArgs/ActionShape, a per-action argument-count table with a usage line, so an action rejects too few or too many arguments with a clear message instead of silently ignoring extras.UnknownAction, listing valid actions.</>as</>in every usage string so it renders in chat..WithDescriptionand add.WithAdditionalInformation(shown by/help kit//help kitedit) with real usage text.Regression proof
scripts/smoke-test.shnow pipes 8 console commands into the running server after it reachesRunGameand asserts each was dispatched and produced the expected message — including the literal reported command. A console caller always passes Stratum's own access check and has no player entity, so this exercises registration, parsing, and argument handling end to end without a game client.After the fix:
Also fixed en route:
smoke-test.shlaunched the server withcwd=repo root. Mono.Cecil's default assembly resolver searches the process's current directory when resolving mod references; from repo root it failed to resolveVintagestoryAPI, which silently dropped every mod (including the basegamecontent mod) and made the server crash beforeRunGameon every run, independent of this fix. Now launches from the server's own directory.Manual verification (done)
Ran a real Stratum server locally, connected with the stock client using the maintainer's normal account, and exercised the full command surface by hand:
/kitedit create,preview,additem,removeitem,/kit,/kit <name>, and/help kitedit. Create, preview, item add/remove, redeem, and the give-back all worked end to end with a real inventory.This manual pass is what surfaced the #295 follow-up below: the maintainer noticed
createpicked up worn armor/clothing along with the hotbar contents. That is correct per the original #210/#269 design (StratumKitGiver's own comments confirm armor placement was always intentional), just not something an operator could opt out of before this PR.Part 2 — #295: snapshot scope
Problem
createalways walks hotbar + character inventory together, so worn armor/clothing is captured with no way to opt out. Confirmed by manual testing above.Fix
sources/VintagestoryLib/Vintagestory.Server/CmdStratumKits.cs,StratumKitStore.cs:StratumKitDefinition.Scope("all"default, or"hotbar")./kitedit setscope <name> all|hotbar, added to the action table and argument validation the same way every other action is.SnapshotInventorytakes the kit's scope:allwalks hotbar + character (current behavior, unchanged),hotbarwalks only the hotbar (offhand included, since it's the same inventory).createreadsexisting?.Scope ?? "all"so a brand-new kit keeps today's default and a re-createrespects whateversetscopeset.previewshows the current scope.Scope is a kit property applied on the next
create, not retroactive to an already-stored item list — same model assetrole/setcooldown/the on/off flags.Regression proof
A console caller has no player entity, so this path cannot be exercised by the public smoke test (which is console-driven). Instead, added
Create_scope_all_includes_worn_armor_hotbar_excludes_itto the private Atlas regression suite (research/atlas-tests/stratum-pr-validation/KitScenarios.cs, not part of this public repo): a real test player is given a hotbar item and has armor equipped into a real character inventory slot, then:/kitedit createwith default scopeall→ resulting kit's items include both the hotbar item and the armor./kitedit setscope <name> hotbar→Find(...).Scope == "hotbar"./kitedit createagain → resulting kit's items include the hotbar item, not the armor./kitedit previewoutput contains"hotbar"./kitedit setscope <name> everything(invalid value) → rejected with the usage line.Ran the full
KitScenariosclass alongside it: 4 passed (including this new one), 1 pre-existing failure (Respawn_gives_assigned_kits_skips_unassigned_and_marks_one_per_life) — a real-time polling scenario on a code path this PR does not touch (OnPlayerRespawn/GiveAssignedRespawnKits), timing out at 600 ticks in this sandboxed environment. Not a regression from either change here.Documentation
docs/commands/kits.md: full reference for/kitand/kitedit, includingsetscope, the scope's effect oncreate, and a symbol-to-behavior sync table so a future implementation change is easy to spot as needing a doc update.Gates run
dotnet build VintageStory.slnx -c Release -p:EmbedPatchedFiles=true— 0 errors, 0 new warnings on touched files.bash scripts/smoke-test.sh—PASS(log above), covers Fix /kitedit registration error and document kit commands #293.KitScenarios(private suite) — covers Add /kitedit scope option to exclude worn armor and clothing from create #295's scope behavior against a real player/inventory.git diff --checkclean.Limitations
scripts/smoke-test.ps1was not extended with the console command probe and not re-verified on Windows; it now has a note pointing to the bash probe. The coverage for Fix /kitedit registration error and document kit commands #293 is bash-only for now.smoke-test.ps1prefersVintagestoryServer.exeoverStratumServer.exe, the opposite ofsmoke-test.sh's (correct) preference — likely boot-tests unpatched vanilla on Windows. Worth its own issue.CmdStratumKits.csalso exists inCmdStratumStaffCommands.csandCmdStratumRoles.cs. Left out to keep this PR scoped.dotnet build VintageStory.slnx -p:EmbedPatchedFiles=trueneeds to be run twice, andStratumServerneeds to actually be launched once, for its embedded/overlaidVintagestoryLib.dllto pick up a same-session code change. Not fixed here (build/overlay pipeline issue, outside this file's scope) but worth knowing when verifying any patch throughStratumServerlocally.setscopedoes not retroactively filter an existing kit's stored items; documented, not a bug.Risk
Low.
sources/-only changes, no vanilla patch touched..RequiresPrivilege(Privilege.chat)is the loosest possible base gate and matches every sibling command; no player loses access they had. The new scope defaults toall, so no existing kit's behavior changes unless an operator explicitly opts it intohotbar.Type
Checklist
scripts/extract-patches.shran clean (unrelated pre-existing working-tree drift in this environment was discarded before committing; only the intended files are in this diff).dotnet build VintageStory.slnx -c Releaseis green.// Stratummarker. (N/A — no vanilla file touched;CmdStratumKits.cs/StratumKitStore.csare Stratum-only undersources/.)Related issues
Fixes #293
Fixes #295
Related: #210 (original feature request), #269 (original implementation).