fix(vt): round 3 item 12 — cleanup pass (stale refs, silent swallows, comments)
Five small fixes bundled per the round's cleanup item:
- VtankNavRouteSerializer.cs's doc comment cited a "WriteBinaryNavBlob"
method that no longer exists anywhere in the codebase (MetaEngine's
embedded-navigation contract moved to the typed MetaAction.EmbeddedRoute
NavigationSettings, saved/loaded through MetafSerializer.SaveNav/
TryLoadNav, back at round 2 step B) — corrected to name the real
mechanism.
- MossTankCommands.cs:274's comment referenced an "exports/nav/" mirror
directory that stopped existing when route profiles cut over to writing
their real .af file directly (round 2 steps 2-3) — corrected.
- docs/research/vtank-kb/07-meta-and-expressions.md section 5.2 row 6
described the pre-cutover "MossTankMetaProfileStore.WriteLegacyExport
convenience mirror" design; .af is now the SOLE authoritative Meta
store, so a disabled rule's save refusal now blocks the profile itself
— the row now says a disabled rule makes the profile file genuinely
unsaveable, not that a mirror goes stale.
- The two bare `catch (FormatException) { }` blocks that silently dropped
a corrupt monster-rule expression (one in SideCarDocument.Apply, reached
from a corrupt side-car; one in LegacyCombatProfileDocument.Apply,
reached during legacy-JSON migration) now log a warning via the host's
IPluginLogger, threaded through as an optional parameter from every call
site.
- VtankDatabase.Render()'s table-sort doc comment now states explicitly
that StringComparer.Ordinal matching .NET Framework's SortedDictionary
default order is confirmed only for the plain-ASCII table names VTank
ships (AntiExtraBuffSpells, MyMonsters, Settings, …), not as a general
claim for any string — comment only, no behavior change.
Added CorruptSideCarMonsterRuleIsLoggedNotSilentlySwallowed (FakeLogger
now captures Warn() calls via a new FakeHost.Logger property) pinning the
swallow-to-log fix.
Mutation: reverted MossTankProfileStore.cs to HEAD (keeping only the new
test) and ran it — failed with an empty Warnings collection, confirming
the silent-swallow bug before the fix.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
55010f92c7
commit
f58e997b19
6 changed files with 95 additions and 24 deletions
|
|
@ -723,7 +723,7 @@ line counts). This is an unusually precise reverse-engineering result — the
|
|||
| 3 | **`MonsterNameCountWithinDistance`'s name pattern is case-insensitive in MossTank, case-sensitive in retail** | `d7.c()` compiles with `RegexOptions.Compiled` only (`d7.cs:26`) — case-sensitive | `Meta.cs`'s `MonsterCount` helper (`Meta.cs:536-552`) compiles with `RegexOptions.IgnoreCase \| RegexOptions.CultureInvariant` | **Medium-high.** A pattern authored against retail's case-sensitive matching (e.g. deliberately excluding a differently-cased variant name) will over-match in MossTank. Note `ChatMessage`/`ChatMessageCapture` do **not** have this divergence — MossTank's `ChatMatch` (`Meta.cs:463-513`) is correctly case-sensitive (`RegexOptions.CultureInvariant` only), matching `hl.cs`/`c5.cs`. |
|
||||
| 4 | **`;` sequence-operator value convention is inverted vs. retail (but matches UtilityBelt, and was chosen deliberately)** | Retail VTank: `a;b` evaluates to **`a`** (the left/first operand), discarding `b`'s value, while still executing `b` for side effects (`ExpressionEvaluator.cs:787-790`); `;` is an ordinary operator that can nest anywhere via normal precedence | `ExpressionProgram.Evaluate` (`ExpressionEngine.cs:20-27`) treats top-level `;`-separated statements as a `Node[]` program and returns the value of the **last** statement — this matches UtilityBelt's own audited grammar ("multiple `;`-separated statements, returning the final result", `2026-08-26-mosstank-vtank-utilitybelt-research.md:237`), which the campaign explicitly chose as MossTank's baseline dialect (§3.1 there) | **Medium, and by design, not an oversight.** Retail VTank and UtilityBelt already disagree with each other on `;`'s return value; MossTank correctly implements UtilityBelt's convention. The compat risk is narrower than a plain bug: only a `.met` authored *against retail's own* semantics (chaining `sideeffect[]; realcheck[]` and relying on the *first* value) evaluates to the opposite result once imported. |
|
||||
| 5 | **Watchdog ring pre-fill differs (far sentinel vs. arm-time position)** | All 10 position-history slots start as a sentinel far from any real coordinate (`(1000,1000,1000)`, `h7.cs:45-49`), guaranteeing expiry cannot even be *reachable* until one full `timeSpan` window has elapsed and every slot has been overwritten with a real sample | `SetWatchdog` (`Meta.cs:585-596`) pre-fills all 10 slots with the **current** position at arm time | **Lower-medium.** Both converge on "earliest possible expiry is ~one `TimeSpanSeconds` after arming" in the common case, but they diverge for a watchdog that is armed, then the player leaves and returns to very near the arm point before the window closes — retail's sentinel-seeded ring cannot spuriously read the arm-time position as one of its 10 samples, MossTank's can. |
|
||||
| 6 | **`MetaRule.Enabled` (a per-rule disable-without-delete toggle) has no metaf-compatible representation at all** | Real VTank/metaf has no such concept — `metaf_monolithic.py` has zero occurrences of "enabled"/"disabled" anywhere; a rule in a `.met`/`.af` is simply present or absent | `MetafSerializer.SaveMeta` (Campaign VT slice-1 item H) now **refuses to save** a profile containing any disabled rule by default (throws `InvalidOperationException` naming the count) rather than silently dropping it — the caller must opt in via `SaveMeta(profile, dropDisabledRules: true)` to accept the loss explicitly. `MossTankMetaProfileStore.WriteLegacyExport` (the `.af` convenience mirror written alongside MossTank's own fully-fidelity JSON storage) deliberately does NOT opt in: it leaves that mirror stale rather than losing the disabled rule, and logs a warning via the existing try/catch. | **Low** (by design, not a bug): this is a MossTank-only UI extension with no VTank equivalent to diverge from; the representational loss is confined to the legacy-export mirror, never the authoritative profile data, and is now impossible to hit silently. |
|
||||
| 6 | **`MetaRule.Enabled` (a per-rule disable-without-delete toggle) has no metaf-compatible representation at all** | Real VTank/metaf has no such concept — `metaf_monolithic.py` has zero occurrences of "enabled"/"disabled" anywhere; a rule in a `.met`/`.af` is simply present or absent | `MetafSerializer.SaveMeta` (Campaign VT slice-1 item H) **refuses to save** a profile containing any disabled rule by default (throws `InvalidOperationException` naming the count) rather than silently dropping it — the caller must opt in via `SaveMeta(profile, dropDisabledRules: true)` to accept the loss explicitly. Round 2 (Campaign VT slice 1 Part A) made `.af` the SOLE authoritative Meta store — `MossTankMetaProfileStore` has no separate JSON storage or `.af` "convenience mirror" any more, so this refusal now blocks the save of the profile itself: a disabled rule makes that profile file genuinely **unsaveable** until the user re-enables or deletes the rule (the file on disk keeps its last successfully-saved content in the meantime; round 3 item 12 corrected this row, which previously described the pre-cutover "legacy-export mirror" design). | **Low** (by design, not a bug): this is a MossTank-only UI extension with no VTank equivalent to diverge from, and the refusal is loud (a user-visible notice) rather than a silent data loss. |
|
||||
|
||||
### 5.3 Confirmed non-gaps (the format/engine is otherwise unusually faithful)
|
||||
|
||||
|
|
|
|||
|
|
@ -271,9 +271,13 @@ internal sealed partial class MossTankPanel
|
|||
// Item J (slice-1 fix round): .af is the only VTank-compatible
|
||||
// storage format now (Campaign VT slice 1 Part A); ".nav" is
|
||||
// stripped for legacy-typed names, ".af" for names copy-pasted
|
||||
// from the exports/nav/ directory or a real VTank profile folder —
|
||||
// without both, "/vt nav save Foo.af" would have produced a
|
||||
// doubled "Foo.af.af" export.
|
||||
// from a real .af file (the VtankProfiles directory, or an
|
||||
// imports/ drop-in for TryImportLegacy below) — without both,
|
||||
// "/vt nav save Foo.af" would have produced a doubled "Foo.af.af"
|
||||
// file. Round 3 item 12: this comment previously cited an
|
||||
// "exports/nav/" mirror directory that no longer exists — route
|
||||
// profiles have written directly to their real .af file since the
|
||||
// round 2 step 2-3 cutover, with no separate export copy.
|
||||
name = StripExtension(name, ".nav", ".af");
|
||||
if (operation == "save")
|
||||
{
|
||||
|
|
|
|||
|
|
@ -212,7 +212,7 @@ internal sealed class MossTankProfileStore
|
|||
database = VtankDefaultSettingsDatabase.Parse();
|
||||
ApplyFromDatabase(database, settings);
|
||||
sidecar = SideCarDocument.CreateDefaults();
|
||||
sidecar.Apply(settings, noBuffItemNames);
|
||||
sidecar.Apply(settings, noBuffItemNames, _host.Log);
|
||||
}
|
||||
WriteUsdText(fileName, database.Render());
|
||||
WriteJson(SideCarKey(fileName), sidecar);
|
||||
|
|
@ -268,7 +268,7 @@ internal sealed class MossTankProfileStore
|
|||
// template/defaults rather than leaving the live settings alone.
|
||||
VtankDatabase fresh = VtankDefaultSettingsDatabase.Parse();
|
||||
ApplyFromDatabase(fresh, settings);
|
||||
SideCarDocument.CreateDefaults().Apply(settings, noBuffItemNames);
|
||||
SideCarDocument.CreateDefaults().Apply(settings, noBuffItemNames, _host.Log);
|
||||
_currentDatabase = fresh;
|
||||
_currentDatabaseFileName = fileName;
|
||||
return;
|
||||
|
|
@ -286,7 +286,7 @@ internal sealed class MossTankProfileStore
|
|||
_host.Log.Warn(RecoveryNotice);
|
||||
VtankDatabase fresh = VtankDefaultSettingsDatabase.Parse();
|
||||
ApplyFromDatabase(fresh, settings);
|
||||
SideCarDocument.CreateDefaults().Apply(settings, noBuffItemNames);
|
||||
SideCarDocument.CreateDefaults().Apply(settings, noBuffItemNames, _host.Log);
|
||||
_currentDatabase = fresh;
|
||||
_currentDatabaseFileName = fileName;
|
||||
return;
|
||||
|
|
@ -294,7 +294,7 @@ internal sealed class MossTankProfileStore
|
|||
_currentDatabase = database;
|
||||
_currentDatabaseFileName = fileName;
|
||||
(ReadJson<SideCarDocument>(SideCarKey(fileName)) ?? SideCarDocument.CreateDefaults())
|
||||
.Apply(settings, noBuffItemNames);
|
||||
.Apply(settings, noBuffItemNames, _host.Log);
|
||||
}
|
||||
|
||||
public void SaveCurrent(
|
||||
|
|
@ -319,7 +319,7 @@ internal sealed class MossTankProfileStore
|
|||
{
|
||||
VtankDatabase database = VtankDefaultSettingsDatabase.Parse();
|
||||
ApplyFromDatabase(database, settings);
|
||||
SideCarDocument.CreateDefaults().Apply(settings, noBuffItemNames);
|
||||
SideCarDocument.CreateDefaults().Apply(settings, noBuffItemNames, _host.Log);
|
||||
string fileName = CurrentFileName();
|
||||
WriteUsdText(fileName, database.Render());
|
||||
WriteJson(SideCarKey(fileName), SideCarDocument.CreateDefaults());
|
||||
|
|
@ -499,7 +499,8 @@ internal sealed class MossTankProfileStore
|
|||
};
|
||||
var noBuffItemNames = new HashSet<string>(StringComparer.Ordinal);
|
||||
legacy.Apply(
|
||||
settings.Combat, settings.Buffs, settings.Vitals, settings.Inventory, noBuffItemNames);
|
||||
settings.Combat, settings.Buffs, settings.Vitals, settings.Inventory,
|
||||
noBuffItemNames, _host.Log);
|
||||
VtankDatabase database = VtankSettingsProfileSerializer.CreateNew(settings);
|
||||
WriteUsdText(fileName, database.Render());
|
||||
WriteJson(SideCarKey(fileName), SideCarDocument.Capture(settings, noBuffItemNames));
|
||||
|
|
@ -563,7 +564,7 @@ internal sealed class MossTankProfileStore
|
|||
return;
|
||||
}
|
||||
|
||||
legacy.Apply(settings.Combat, settings.Buffs, settings.Vitals, settings.Inventory, noBuffItemNames);
|
||||
legacy.Apply(settings.Combat, settings.Buffs, settings.Vitals, settings.Inventory, noBuffItemNames, _host.Log);
|
||||
VtankDatabase database = VtankSettingsProfileSerializer.CreateNew(settings);
|
||||
WriteUsdText(fileName, database.Render());
|
||||
WriteJson(SideCarKey(fileName), SideCarDocument.Capture(settings, noBuffItemNames));
|
||||
|
|
@ -823,7 +824,8 @@ internal sealed class MossTankProfileStore
|
|||
|
||||
public void Apply(
|
||||
VtankSettingsProfileSerializer.AllSettings settings,
|
||||
ISet<string> noBuffItemNames)
|
||||
ISet<string> noBuffItemNames,
|
||||
IPluginLogger? logger = null)
|
||||
{
|
||||
Replace(settings.Combat.CombatItemNames, ItemNames);
|
||||
settings.Combat.CombatItemObjectIds.Clear();
|
||||
|
|
@ -851,7 +853,12 @@ internal sealed class MossTankProfileStore
|
|||
foreach (MonsterRuleDocument rule in CombatRules ?? [])
|
||||
{
|
||||
try { settings.Combat.Rules.Add(rule.ToRule()); }
|
||||
catch (FormatException) { }
|
||||
catch (FormatException error)
|
||||
{
|
||||
// Round 3 item 12: log rather than silently drop a
|
||||
// corrupt side-car monster rule.
|
||||
logger?.Warn($"MossTank could not restore a monster rule: {error.Message}");
|
||||
}
|
||||
}
|
||||
if (!settings.Combat.Rules.Any(static rule => rule.IsDefault))
|
||||
settings.Combat.Rules.Add(new MonsterRule("DEFAULT", 0));
|
||||
|
|
@ -1014,9 +1021,10 @@ internal sealed class MossTankProfileStore
|
|||
BuffSettings buffs,
|
||||
VitalSettings vitals,
|
||||
InventorySettings inventory,
|
||||
ISet<string> noBuffItemNames)
|
||||
ISet<string> noBuffItemNames,
|
||||
IPluginLogger? logger = null)
|
||||
{
|
||||
(Combat ?? LegacyCombatProfileDocument.Capture(new CombatSettings())).Apply(combat);
|
||||
(Combat ?? LegacyCombatProfileDocument.Capture(new CombatSettings())).Apply(combat, logger);
|
||||
(Buffs ?? LegacyBuffProfileDocument.Capture(new BuffSettings())).Apply(buffs);
|
||||
(Vitals ?? LegacyVitalProfileDocument.Capture(new VitalSettings())).Apply(vitals);
|
||||
(Inventory ?? LegacyInventoryProfileDocument.Capture(new InventorySettings())).Apply(inventory);
|
||||
|
|
@ -1237,7 +1245,7 @@ internal sealed class MossTankProfileStore
|
|||
Rules = value.Rules.Select(MonsterRuleDocument.From).ToArray(),
|
||||
};
|
||||
|
||||
public void Apply(CombatSettings value)
|
||||
public void Apply(CombatSettings value, IPluginLogger? logger = null)
|
||||
{
|
||||
value.Enabled = Enabled;
|
||||
value.MaximumRange = Math.Clamp(MaximumRange, 2f, 100f);
|
||||
|
|
@ -1282,7 +1290,12 @@ internal sealed class MossTankProfileStore
|
|||
foreach (MonsterRuleDocument rule in Rules ?? [])
|
||||
{
|
||||
try { value.Rules.Add(rule.ToRule()); }
|
||||
catch (FormatException) { }
|
||||
catch (FormatException error)
|
||||
{
|
||||
// Round 3 item 12: log rather than silently drop a
|
||||
// corrupt legacy-JSON monster rule during migration.
|
||||
logger?.Warn($"MossTank could not restore a legacy monster rule: {error.Message}");
|
||||
}
|
||||
}
|
||||
if (!value.Rules.Any(static rule => rule.IsDefault))
|
||||
value.Rules.Add(new MonsterRule("DEFAULT", 0));
|
||||
|
|
|
|||
|
|
@ -8,11 +8,17 @@ namespace AcDream.Plugins.MossTank;
|
|||
/// import path only. Per the owner's 2026-09-06 "MossTank does not author
|
||||
/// <c>.nav</c>" direction (Campaign VT slice 1 Part A), the writer that
|
||||
/// used to live here was deleted: <c>.af</c> (<see cref="MetafSerializer"/>)
|
||||
/// is the only storage/authoring format for navigation routes now. The
|
||||
/// small binary-blob writer <see cref="MetaEngine"/>'s embedded-navigation
|
||||
/// contract still needs lives in <see cref="MetafSerializer"/> itself
|
||||
/// (<c>WriteBinaryNavBlob</c>) rather than here, since that shape is a
|
||||
/// MossTank runtime contract, not a VTank file on disk.
|
||||
/// is the only storage/authoring format for navigation routes now.
|
||||
/// <see cref="MetaEngine"/>'s embedded-navigation contract (a Meta rule's
|
||||
/// <see cref="MetaActionKind.LoadEmbeddedNavigationRoute"/> action,
|
||||
/// <c>.af</c> tag <c>EmbedNav</c>) is a typed
|
||||
/// <see cref="MetaAction.EmbeddedRoute"/> <see cref="NavigationSettings"/>
|
||||
/// now (round 2 step B, replacing an earlier binary-blob shape), saved and
|
||||
/// loaded through the SAME <see cref="MetafSerializer.SaveNav"/>/
|
||||
/// <see cref="MetafSerializer.TryLoadNav"/> grammar this class no longer
|
||||
/// owns any writer for — round 3 item 12 cleanup: this doc comment
|
||||
/// previously cited a "<c>WriteBinaryNavBlob</c>" method that does not
|
||||
/// exist anywhere in the codebase.
|
||||
/// </summary>
|
||||
internal static class VtankNavRouteSerializer
|
||||
{
|
||||
|
|
|
|||
|
|
@ -268,6 +268,17 @@ internal sealed class VtankDatabase
|
|||
/// accident of whatever order the source data happened to arrive in
|
||||
/// (no observable behavior change for any fixture today; it only
|
||||
/// matters the day a future code path adds or reorders a table).
|
||||
///
|
||||
/// Round 3 item 12: <see cref="StringComparer.Ordinal"/> is used here
|
||||
/// as a stand-in for .NET Framework's <c>SortedDictionary<string, bd></c>
|
||||
/// default key order — that equivalence is confirmed only for the
|
||||
/// plain-ASCII, single-case table names VTank itself ships
|
||||
/// (AntiExtraBuffSpells, MyMonsters, Settings, …), not as a general
|
||||
/// claim that ordinal and .NET's culture-aware default string
|
||||
/// comparer agree for every possible string; a hypothetical future
|
||||
/// table name using non-ASCII characters or mixed-width comparisons
|
||||
/// could sort differently under the two. No behavior change is
|
||||
/// implied or made by this note.
|
||||
/// </summary>
|
||||
public string Render()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -26,6 +26,35 @@ public sealed class MossTankPanelTests
|
|||
Assert.Contains("{ this is not json", backup.Value, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Round 3 item 12: a side-car monster rule whose Expression fails to
|
||||
/// compile used to be silently dropped (a bare
|
||||
/// <c>catch (FormatException) { }</c>); it must now log a warning
|
||||
/// instead, while still falling back gracefully (the malformed rule is
|
||||
/// skipped, everything else loads).
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void CorruptSideCarMonsterRuleIsLoggedNotSilentlySwallowed()
|
||||
{
|
||||
var storage = new MemoryStorage();
|
||||
string usdKey = VtankProfileDirectory.AutoCharacterFileName("Barris", string.Empty, "usd");
|
||||
storage.Text[usdKey] = VtankDefaultSettingsDatabase.Parse().Render();
|
||||
storage.Text["profiles/macro/sidecar/--Barris_.usd.json"] = """
|
||||
{
|
||||
"CombatRules": [
|
||||
{ "Expression": "(((" },
|
||||
{ "Expression": "DEFAULT" }
|
||||
]
|
||||
}
|
||||
""";
|
||||
|
||||
var host = new FakeHost(new FakeAutomation { Name = "Barris" }, storage);
|
||||
_ = new MossTankPanel(host);
|
||||
|
||||
Assert.Contains(host.Logger.Warnings, message =>
|
||||
message.Contains("monster rule", StringComparison.OrdinalIgnoreCase));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Reproduces the pre-cutover hashed JSON key a "By char" macro profile
|
||||
/// used to be stored under (<c>MossTankProfileStore.LegacyProfileKey</c>,
|
||||
|
|
@ -2158,7 +2187,10 @@ public sealed class MossTankPanelTests
|
|||
IPluginStorage? vtankProfiles = null) : IPluginHost
|
||||
{
|
||||
public bool HasUi => false;
|
||||
public IPluginLogger Log { get; } = new FakeLogger();
|
||||
// Round 3 item 12: exposed as the concrete FakeLogger (not just the
|
||||
// IPluginHost.Log interface view) so a test can inspect Warnings.
|
||||
public FakeLogger Logger { get; } = new();
|
||||
public IPluginLogger Log => Logger;
|
||||
public IGameState State { get; } = new FakeState();
|
||||
public IEvents Events { get; } = new FakeEvents();
|
||||
public ISelectionService Selection { get; } = new FakeSelection();
|
||||
|
|
@ -2482,8 +2514,13 @@ public sealed class MossTankPanelTests
|
|||
|
||||
private sealed class FakeLogger : IPluginLogger
|
||||
{
|
||||
// Round 3 item 12: capture Warn() calls so a test can prove a
|
||||
// corrupt monster-rule expression is logged rather than silently
|
||||
// swallowed by the two former bare "catch (FormatException) { }"
|
||||
// blocks in MossTankProfileStore.
|
||||
public List<string> Warnings { get; } = [];
|
||||
public void Info(string message) { }
|
||||
public void Warn(string message) { }
|
||||
public void Warn(string message) => Warnings.Add(message);
|
||||
public void Error(string message, Exception? exception = null) { }
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue