Two Opus reviews of commit 8217a349e (Slice B: DAT icons in plugin
markup) found one BLOCKING defect and 14 SHOULD-FIX findings. All 15
fixed here in one commit per the review contract.
BLOCKING (finding 1): an unresolvable did painted a magenta square.
TextureCache.GetOrUploadRenderSurface's 1x1 magenta placeholder for a
missing RenderSurface is load-bearing for authored chrome, but
RetailMarkupIconResolver.ResolveDid only short-circuited did==0, so any
other unresolvable id fell through to that placeholder and got scaled
up by UiMarkupIcon/UiMarkupList/UiSimpleButton -- the classic
resolve(0)-style footgun (claude-memory/feedback_ui_resolve_zero_magenta.md),
just triggered by a missing id instead of a literal 0. Fixed by probing
Portal/HighRes existence via IDatReaderWriter.TryGet<RenderSurface>
BEFORE ever calling GetOrUploadRenderSurface -- that TryGet already
serializes concurrent DAT access internally (DatDatabaseWrapper's own
_databaseLock), the same synchronization IconComposer.TryDecode relies
on, so no additional lock was needed. RetailMarkupIconResolver now
takes IDatReaderWriter + TextureCache directly (RetailUiAssets gained a
TextureCache field, its one construction site in
InteractionRetainedUiComposition.cs updated) instead of the old
resolveSprite delegate, since it also needs the nearest-sampled upload
path for finding 6 below.
Finding 2 -- Smoke panel wiring bugs: its list fed iconkind="spell"
raw art DIDs (PluginSpellInfo.IconId) instead of spell ids, so
IMarkupIconResolver.ResolveSpell composited the wrong (or no) badge
every row. SmokeIconPanel.Binding.SpellIds now yields SpellId (the
printed text still shows IconId alongside). The bare-index demo and the
descriptor both moved from the unverified literal 7735 to 0x165 --
retail's real Melee Defense skill icon (SampleData.cs:64,
0x06000165) -- so the owner's visual gate proves real art, not a guess.
StartVisible flipped true, and a character with no self-buffs known
falls back to spell 1's real catalog entry (or an honest "no spells
known" row with icon 0 if even that fails) rather than fabricating art.
Finding 3 -- PluginIcons.Normalize's threshold was silently rewritten
from the contract's 0x01000000 to 0x06000000 during Slice B. Restored
to 0x01000000; the class/method XML docs now state the number directly
(no cref to the private const); the test table adds 0x02000000 (a value
that only distinguishes the two thresholds) and 0x01000000 itself
(passes through unchanged).
Finding 4 -- an unknown iconkind (e.g. "spel") only threw when a
resolver happened to be wired, because BuildIconSource/
BuildRowIconResolve validated inside their own null-icons early return.
A new ValidateIconKind helper runs UNCONDITIONALLY before that branch,
so a malformed iconkind is a Build-time author error on every host.
Finding 5 -- BindUintLiteralOrBinding required an exact uint property
type, rejecting the int-typed bindings Decal-facing code commonly uses
(MosswartMassacre's HudPictureBox.Image is int end to end). It now
matches BindUint's existing leniency: any property, converted via
Convert.ToUInt32 at read time. BindUintList likewise now accepts
IEnumerable<int> alongside IEnumerable<uint> (unchecked per-element
reinterpret -- icon ids never go negative in practice).
Finding 6 -- TextureCache._renderSurfaceGpuTextures was keyed by id
alone, so whichever caller asked for a given RenderSurface id FIRST won
the sampler for every later caller of the same id -- UiDatFont's glyph
atlases already request nearest:true while ResolveChrome's background
art requests nearest:false, so this was a real, reachable collision,
not hypothetical. Rekeyed to (id, nearest); RetailMarkupIconResolver.
ResolveDid now requests nearest:true (pixel-exact 32x32 icon art);
ResolveChrome is untouched (still nearest:false/linear). Audited every
other _renderSurfaceGpuTextures use site (TryGetValue/set/Dispose
iteration+Clear) plus the separate _nearestUiTextureSources/
_linearUiTwinHandles/_uploadMetadata dictionaries (all keyed by handle
or accounting name, unaffected) -- no other eviction/accounting path
assumed id-only keying.
Finding 7 -- column-reservation semantics, per the DECIDED shape:
MarkupDocument now sets button.IconSource / list.IconIdsSource +
IconResolve ONLY when a resolver (icons parameter) is actually wired --
previously button.IconSource was always assigned (even to an
always-empty func on an icons:null host); combined with this finding's
other half -- UiSimpleButton.OnDraw now reserves its icon column
whenever IconSource is non-null, regardless of a per-frame resolve miss,
so a bound id that goes briefly to 0 no longer slides the caption back
and forth -- would have permanently reserved a blank column on such a
host. UiMarkupList already reserved its column whenever IconIdsSource
was set; no draw-side change needed there.
Finding 8 -- added a with/without-icons comparison test for
UiMarkupList (mirroring the existing UiSimpleButton one): asserts the
row text quad's x is strictly greater with an icon column present, and
the icon quad itself has non-zero width.
Finding 9 -- <icon tooltip=""> (empty string) was still treated as
"has a tooltip" by a bare attribute-presence check, making the icon
swallow clicks with no visible tooltip ever appearing. Now uses
!string.IsNullOrWhiteSpace, matching ApplyCommon's own predicate for
every other element's tooltip.
Finding 10 -- PluginShelfButton.OnDraw drew nothing when a non-zero
descriptor icon id resolved to no texture (a bad Decal index, a DAT id
from a different install), rather than falling back to Initials the
way a zero id already did. Now decides once, on the first draw
(memoized, so Initials' string work never repeats every frame): a
failed resolve permanently switches Text to the initials fallback,
computed and assigned BEFORE base.OnDraw actually paints the caption.
Finding 11 -- MarkupDocument.AddElement's switch had no default arm, so
an unknown or miscased element name (<Icon>, <butotn>) silently
vanished from the built tree instead of failing loudly like every
other malformed-markup case. Added a default arm that throws
FormatException. Ran AcDream.Plugins.MossTank.Tests (337/337,
unchanged) and the full App markup suite to confirm no existing markup
relies on an unknown element.
Finding 12 -- PluginPanelDescriptor.IconSurfaceId's XML doc now states
that a bare Decal index is accepted and normalized, citing
PluginIcons.Normalize.
Finding 13 -- docs/plugin-ui-markup.md: replaced the blanket "wrong
type/missing property throws at Build" sentence with the per-attribute
truth table the review produced (which attributes are silent at
runtime vs. throw at Build, and each one's bound CLR/delegate type);
restated the icon-id boundary as 0x01000000; added the "do NOT add
0x06000000 to the four already-full IconId records" warning (citing
SkillBase._iconID / UIRegion::SetImageByDID @0x004f150e); documented
that 0x-prefixed hex is required (an unprefixed all-digit literal
parses as decimal); noted unknown element names now throw; called out
list colors (0xRRGGBB) vs. color=/background=/border= (#AARRGGBB) as
non-interchangeable grammars; documented the root <panel visible>
binding-only exception; corrected the shelf's collapse toggle glyphs
(</>, not the old doc's arrows) and the 28px collapsed-tab size; added
the IconId record-equality API-v1 note; and called out iconkind as
per-<list> (mixed id spaces need pre-normalized DIDs; the composited
spell badge has no did-space escape hatch) plus the existing
one-text-column LIMITATION being deferred to MossTank.
Coverage added for finding 14: a PluginSidePanelTests case proving the
shelf button normalizes a bare descriptor index before resolving, and a
reflection-based unit on AppAutomationSurface.ProjectWorldObject (its
public callers gate on IsAvailable, which needs a fully connected
session heavier than this mapping needs -- the plan's own documented
fallback) proving PluginWorldObject.IconId carries ClientObject.IconId
through unchanged; PluginInventoryItem.IconId uses the identical
one-line pattern inline in CaptureOwnedItems, reviewed by inspection.
Finding 15: recorded a "Review ledger" section in the plan doc with
both slices' commits, both review verdicts, and the two items
explicitly deferred to the MossTank plugin work (multi-column list,
root literal visible).
Verification: full solution builds green. Targeted filter
(Markup|PluginSidePanel|PluginIcons|AppAutomation|TextureCache|
UiDatFont) passes 131/131, including the two InstalledDat-lane tests
(RetailMarkupIconResolverInstalledDatTests,
AppAutomationSurfaceIconInstalledDatTests) actually resolving against
the real installed DAT, not skipping. AcDream.Plugins.MossTank.Tests
passes 337/337 unchanged. Full AcDream.App.Tests suite: 7351 passed /
97 skipped / 36 failed -- identical failure set/count to the
7334/97/36 baseline (the +17 passes are exactly the new/expanded
tests: 2 new PluginIconsTests.Normalize theory rows, 10 new
MarkupIconTests cases, 2 new PluginSidePanelTests cases, 1 new
AppAutomationSurfaceTests case, and the 2 new standalone test files).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Owner request: plugin panels (Decal/VirindiViewService-class, per the
MosswartMassacre reference usage) need to embed real DAT icons the way
FlagTrackerView.SafeSetListImage does — spell/skill art, raw portal
indices, and a window icon. This is Slice B of
docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md (Slice A, the
movable/collapsible shelf, landed in 01b98ca30/4fada238e/718005b21).
What shipped:
- AcDream.Plugin.Abstractions.PluginIcons.Normalize: the one Decal-style
bare-index -> 0x06xxxxxx RenderSurface DID grammar, applied at every
icon SINK (descriptor IconSurfaceId in PluginShelfButton, and markup
<icon did>/<button icon>/<list icons> did-kind ids) rather than on the
plugin-facing records, which already carry real DIDs read straight
from the client's tables.
- PluginSpellInfo.IconId / PluginSkillInfo.IconId /
PluginInventoryItem.IconId / PluginWorldObject.IconId: additive init
properties (default 0), filled in AppAutomationSurface from
SpellMetadata.IconId (already projected from SpellBase.Icon by
RetailSpellMetadataProjector — no gap there), a new BindSkillIcons
parallel to BindSkillNames (GameWindow reads
DatReaderWriter.Types.SkillBase.IconId — confirmed via reflection over
the installed Chorizite.DatReaderWriter package, since its XML docs
don't cover Pack/Unpack-generated public fields: Description, Name,
IconId (uint), TrainedCost, SpecializedCost, Category, ChargenUse,
MinLevel, Formula, UpperBound, LowerBound, LearnMod), and
ClientObject.IconId in CaptureOwnedItems/ProjectWorldObject.
- IMarkupIconResolver (AcDream.App.UI): ResolveDid/ResolveSpell/
ResolveItem. MarkupDocument.Build gains an optional parameter (null by
default -> every icon sink resolves to nothing rather than throwing,
so pre-Slice-B callers/tests are unaffected). RetailUiRuntime.
MountPlugins builds ONE RetailMarkupIconResolver per pass from
RetailUiAssets.ResolveSprite + RetailUiAssets.Icons (the shared
IconComposer) + Toolbar.Objects (the SAME ClientObjectTable
Magic/Toolbar bindings already borrow for their own icon resolution —
no second object lookup introduced).
- New UiMarkupIcon widget (<icon x y w h did|spell|item tooltip>):
exactly one source required (FormatException at Build otherwise,
matching every other malformed-attribute rule), aspect-preserved,
centered, click-through unless a tooltip makes it a real hit-test
target.
- UiSimpleButton.IconSource and UiMarkupList.IconIdsSource/IconResolve:
additive, default null/no-op, so every existing button/list caller
(including the plugin shelf's own toggle/minimize buttons) is
unaffected. Button icon draws flush left and shifts the caption's
centering region right; list icons reserve a leading RowHeight-2
column (Decal's IconColumn) and skip rows whose id is 0 or
unresolvable.
- MarkupDocument centralizes the did/spell/item dispatch (including
PluginIcons.Normalize for did) in two small helpers (BuildIconSource
for <icon>/<button>, BuildRowIconResolve for <list>) so all three
markup surfaces share one resolver call path.
- AcDream.Plugins.Smoke ships a RegisterPanelContent (in-memory KSML,
no plugin-side .xml file) proof panel exercising every new surface:
a bare-index <icon>, a literal-hex <icon>, a composited <icon
spell=...>, a <button icon=...>, and a <list icons=... iconkind=
spell> of the first five known self-buffs with their IconId printed
alongside. Descriptor IconSurfaceId reuses the same bare index to
prove the shelf button and the panel's own icon normalize identically.
- docs/plugin-ui-markup.md is the new SSOT for the full markup
vocabulary + icon grammar + the Slice A shelf; linked from
docs/README.md and docs/plans/2026-04-24-ui-framework.md.
Design decisions where the plan left room:
- Normalize runs inside the resolver dispatch (BuildIconSource/
BuildRowIconResolve), not scattered at each markup call site, so
every did-kind sink shares one choke point.
- did/spell/item all accept either a literal (decimal or 0x-hex) or a
{Binding}, via one BindUintLiteralOrBinding helper, for symmetry —
the plan only showed spell/item as bindings but didn't forbid a
literal.
- <icon> requires exactly one source INCLUDING zero (not just two);
an icon with no source is not a coherent element.
- The button/list icon draw math (icon column extent, padding) lives
in the widgets themselves (UiSimpleButton/UiMarkupList), not in
MarkupDocument, keeping the parser only responsible for wiring
Func<(tex,w,h)> sources.
Tests: PluginIconsTests (Normalize table), MarkupIconTests (icon/button/
list resolver dispatch via a fake IMarkupIconResolver, plus draw-level
pins via the RecordingGpuDevice/TextRenderer apparatus already used by
UiAncestorClipTests/UiRenderContextDrawStringDatOutlineTests — "draws
nothing when unresolvable" and "button/list icon shifts the text"),
and AppAutomationSurfaceIconInstalledDatTests (Lane=InstalledDat: a
known spell's IconId matches the real installed SpellTable's own Icon
field exactly). Verified every new test fails to COMPILE without this
change (git-stashed the src/ changes, rebuilt the test project: CS0246
on IMarkupIconResolver) before restoring. Full App suite: 7331 passed /
97 skipped / 36 failed (identical pre-existing failure set/count to the
7306/97/36 baseline; the +25 passes are exactly the new tests).
AcDream.Plugins.MossTank.Tests (the main consumer of the touched
Plugin.Abstractions records) passes 337/337 unchanged, confirming
API-v1 binary/source compatibility. Full solution builds green.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Opus re-review of 4fada238e (Slice A review-fix round) left seven residuals.
All seven addressed here, in one commit per the residual-round contract:
NEW-1 (test-coverage): no test pinned the Draggable:true->false change
itself. Added Drag_StartingOnShelfPadding_DoesNotMoveTheShelf, pressing the
shelf's own PADDING (left of the first button's Left=4, below the 12px grip)
and dragging 100px through the real UiRoot press/move/release path. Proven
to fail against the pre-4fada238e shelf: temporarily setting Draggable=true
in the ctor and re-running this test moved the shelf from Left=760 to 764
(clamped to parent.Width-Width) instead of leaving it in place — reverted
after confirming.
NEW-2 (correctness): RetailWindowLayoutPersistence.ClampAllToScreen clamps
generically to screen.Width-handle.Width, 4px (OuterPadding) off what the
shelf's own dock formula (parent.Width-Width-OuterPadding) would produce for
the same new parent size. A screen shrink that triggers that clamp used to
flip _userPositioned permanently even though nothing a user would call
"dragging" happened. PluginSidePanel.OnHandleMoved now re-derives "still
docked" at comparison time as a function of the CURRENT parent size: either
the live dock formula's own result, or what ClampAllToScreen's clamp would
produce from the PREVIOUS docked position — only a position matching
NEITHER flips _userPositioned. New test
ClampAllToScreen_AfterShrinkingTheRoot_DoesNotFlipAnchoring_ButARealDragStillDoes
shrinks the root, runs ClampAllToScreen, confirms the shelf still anchors
top-right through a later reflow and a collapse, then confirms a genuine
grip drag afterward still flips anchoring.
NEW-3 (decision, documented): KeepWindowReachable's per-tick clamp and
ClampAllToScreen's screen-resize sweep both touch plugin windows. Evidence
gathered and recorded as a doc comment on KeepWindowReachable:
RetailWindowManager.MoveTo (line 178) already short-circuits an unchanged
position before ever raising Moved; persistence's ScreenSize and Host.Root's
size derive from the same d.Window.Size and are reconciled every frame
(UiHost.Draw sets Root.Width/Height from screenSize); and because
RetainedGameplayUiFrame.Render ticks BEFORE it draws in the same frame,
ClampAllToScreen always resolves a screen-resize's clamp before
KeepWindowReachable ever observes the new size next tick, making
KeepWindowReachable's own pass a structural no-op for that case (never a
second write). KeepWindowReachable is NOT dropped, though: it is the only
reachability guarantee for (a) a plugin window whose geometry is mutated
directly rather than through MoveTo — pinned by the pre-existing
FullWidthPluginWindowStartsAndStaysReachableAtMinimumCanvas test, which
exercises exactly that with no screen resize at all — and (b) a host wired
with no RetailWindowLayoutPersistence at all. Decision: keep both; they do
not race.
NEW-4 (robustness): RetailWindowLayoutPersistence._attached is mutated
mid-session by WindowRegistered/WindowUnregistered (a callback invoked from
inside one of these loops — e.g. Apply -> Show()/Hide() -> a controller
unregistering another window — could otherwise mutate the list being
enumerated). All five bare `foreach (... in _attached)` loops
(RestoreAllCore, ClampAllToScreen, SaveAll, SaveNamed, RestoreNamed) now
snapshot with .ToArray(), matching Dispose's existing pattern.
NEW-5 (retail-faithfulness): the plugin-shelf hide message hard-coded
"Shift+Ctrl+F1" even though InputAction.TogglePluginManager is rebindable
through Configure Keyboard. RetailUiRuntime.PluginShelfHiddenMessage now
looks up the CURRENT binding via the live InputDispatcher and formats it
through Layout.RetailKeyNames.Describe — the exact formatter Configure
Keyboard's own row captions already use
(Layout/KeyboardConfigController.cs:284) — falling back to an honest
"bind it in Configure Keyboard" message when the action is unbound or no
dispatcher is wired.
NEW-6 (bookkeeping): added divergence-register row IA-27 for the plugin
shelf's repurposing of retail's plugin-manager chord (Shift+Ctrl+F1),
its ASCII </> toggle glyphs, its two acdream-authored system messages, and
its Draggable=false grip-only drag model — citing PluginSidePanel.cs and
the RetailUiRuntime.cs TogglePluginManager case. Header count bumped
23->24 active IA rows.
NEW-7 (correctness): the one-time dock wrote Left directly, so the
first-run docked position went unsaved until some later, unrelated event
happened to trigger a save. PluginSidePanel.OnTick now routes the one-time
dock through the retained-window handle's MoveTo when registered (falling
back to the direct field write when unregistered, as before) —
_dockLeft/_dockTop are set to the TARGET position BEFORE calling MoveTo so
the synchronous OnHandleMoved re-entry it triggers (NEW-2's logic) reads the
new dock position and does not flip _userPositioned. New test
FreshShelf_OneTick_SavesTheDockedPositionImmediately confirms the store has
the docked X/Y after exactly one tick and that a subsequent collapse still
preserves the right (not left) edge.
Verification: dotnet build src/AcDream.App (Release) green; targeted filter
(PluginSidePanel|RetailWindowLayout|Markup|UiRootInput|KeyboardConfig) 145
passed / 2 pre-existing Lane=Manual failures (unrelated, gated on
ACDREAM_PROBE_LIVE_MOUNT=1); full tests/AcDream.App.Tests suite: 7306
passed / 97 skipped / 36 failed — same 36 pre-existing failures by name
(installed-DAT/Cathedral/Linux/Lane=Manual probe tests), +3 passed vs the
7303 baseline (exactly the three new tests this round added).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Owner pivot 2026-09-06: before MossTank feature work resumes, the plugin
shelf must be movable and easy to hide, and plugin markup must embed DAT
icons the way Decal/VirindiViewService plugins (MosswartMassacre) do.
This plan is the contract for Slice A (shelf) and Slice B (icons): verified
current-state facts, behavior, files, tests, review lenses, and scope.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Prepare the original unpinned witness with an explicit cathedral-shell non-vacuity gate. Write-only while the separate production bridge owns test execution; preserve the scratch and its failed diagnostics.
Co-Authored-By: OpenAI Codex <noreply@openai.com>
Use the existing InvalidDataException content-integrity boundary for a loaded destination's out-of-range positive reciprocal index. Preserve negative/unavailable skips; record the managed guard with AP-159 at implementation landing.
Co-Authored-By: OpenAI Codex <noreply@openai.com>
Port the proven missing box admission and immediate destination transit, preserving sphere callers and separate registered source/equality residuals. Keep the failed authored-input diagnostic and original golden untouched.
Co-Authored-By: OpenAI Codex <noreply@openai.com>
Preserve the three explicit diagnostic outcomes and all18 outgoing edges. Native box containment rejects the extra room; five later-part sphere-input alarms remain red and nondecisive for these edge results. Correct AD-117's disproven widening guarantee. No production geometry or golden change, FPS remains deferred, G4 unpassed.
Co-Authored-By: OpenAI Codex <noreply@openai.com>
Project the original Run failure with loaded build, cached GPU and cell context before unwind. Preserve crash status and rethrow; contain report failures and omit arbitrary messages, source paths and session data.
Focused 19/19, narrow production/privacy review and default Release 17044/0/0 pass. Preserve the wrong-package smoke failure and real emitted report; corrected recipe10 smoke exits gracefully with two provisional PNG passes. AMD initiating cause and G4 remain open; FPS deferred. No new retail behavior deviation.
Native-boundary injection: old code 9 pass / 6 expected Assert.Throws failures. Separate acquire and present sabotage each fail their 3 fatal cases; restored focused gate 82/82. One independent API/production review PASS. Lead locked Release 0W0E; literal-hermetic 17051/0/0, manifest32/32. No retirement, recovery, normal result policy or retail behavior changes. This is first-failure evidence preservation, not an AMD reset fix; extended reproduction and G4 remain open.
Retain building and object collision for available neighboring landcells when the terrain polygon does not cover the sphere center. Verified against named and paired retail dispatch. Add prepared-flat regressions and installed cathedral repro with sabotage proof. Full Release gate 17036/0/0 and graphical wall blocking, overlap correction and escape pass provisionally. Owner accepts functional collision; exact retail settle coordinates are not claimed. Retire UN-10; keep AP159, AMD stability and final G4 open.
