fix(chat): consolidated-review fixes — retail /help Detail extraction, seam wiring test
SHOULD-FIX 1: RetailClientCommandCatalog's ~45 catalog leaf verbs were showing acdream-authored Summary text for /help <verb> instead of retail's own Detail_HelpType(2) text. Byte-swept every Help* handler against the PDB-paired acclient.exe (verified MATCH), confirmed each Detail/Summary branch by reading the actual decompiled if/else shape (address order and string length both proved unreliable alone), and fixed a sweep_weenie_strings.py 800-char truncation bug that silently dropped several longer Detail branches. Resolved every ambiguous CmdHashData-registered verb (hor/hr/hom/hoa/alh/ah/friends_add/ friends_remove/squelch/unsquelch) by reading for Binary Ninja's nullptr-4th-arg decompiler artifact instead of trusting it. Coverage: 42 of 47 distinct catalog Definitions verbatim-extracted, 4 confirmed-null (index/clist/on/off register with a genuinely null help pointer — DoHelp falls to UnknownCommand for these, now reproduced), 1 honest UNVERIFIED (messagetypes builds its text from a runtime enum table, not a static string). ChatCommandRouter now prefers retail Detail text over the catalog summary; RetailCommandHelpTable's class doc no longer overclaims its own scope. SHOULD-FIX 2: extracted the a5a7eb4f-class OnInterfaceText wiring into a testable CreateChatViewModel method and added ComposedChatViewModelWiresOnInterfaceTextToSpewBox, which the prior FakeFactory-based test suite could never exercise. SHOULD-FIX 3: retires register row AP-113. DoLifestone/DoMarketplace print their own 0x1A refusal text (byte-recovered, UTF-16LE) instead of falling through to the generic 0x26 fallback; ChatCommandRouter's comment corrected to state the fallback's real scope. SHOULD-FIX 4: corrected the divergence register's stale AP section header sentence about AP-190's opacity default (refuted bycc582899). NITs: (a) HeadlessStaticStateAudit routes through the injected HeadlessDiagnosticWriter instead of Console.WriteLine; (b) a bounded 300-pump liveness diagnostic on the IsQuiescent conductor gate (no retry, no behavior change); (c) fixed the #365 hydration test's doc comment contradiction against diagnosis §8; (d) the 0x26 fallback dispatches on WeenieErrorMessages' own Type instead of hardcoding ClientLocal. Full Release suite: 12,553 passed / 4 skipped / 0 failed (baseline03404b71: 12,542/4/0; net +11 tests, zero regressions). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
03404b7121
commit
f7a6f46ba0
15 changed files with 947 additions and 85 deletions
File diff suppressed because one or more lines are too long
|
|
@ -289,6 +289,7 @@ implementer per slice against a pinned contract (per
|
|||
| CH6b floating windows 1–4 | `22020ef2`, reworked `1aa77099` | 12,420 passed / 4 skipped / 0 failed | REJECT (docs/research/2026-08-10-ch6ab-review-findings.md) → reworked `1aa77099` — SHOULD-FIXES 2/3/4/5 + NITs 1-5 applied | pending — no client launches this session (hard constraint); needs the next connected round for keybind/mirror/filter visual confirmation, plus the new 0x2100005B fixture's resolved-type assumptions |
|
||||
| CH6c opacity | `a819687c` | 12,459 passed / 4 skipped / 0 failed | APPROVE-WITH-FIXES; fixed this commit — BLOCKER (out-of-box `DefaultOpacity` 0.5→1.0), AP-190 reworded + two new decomp-verified clauses (retail's per-tick ease, retail's entry-field-specific focus predicate), NITs (`UiElement.cs` stale comment, `WindowUnregistered` detach, post-Dispose `Set*` guards, `DrawString`/outline-pass alpha tests) | pending — needs the next connected round for visual confirmation (window fade on focus change, Settings slider live-apply) |
|
||||
| Goal-window #363/#367 interface-text seam | `09453eca` | 12,542 passed / 4 skipped / 0 failed | implementer-only, no subagent review this session (hard constraint) | pending — needs the next connected round to confirm the SpewBox now flashes for the reclassified refusals (see "Goal-window follow-up" below) |
|
||||
| Consolidated review — retail `/help` Detail extraction, seam wiring test | this commit | 12,553 passed / 4 skipped / 0 failed | APPROVE-WITH-FIXES; fixed this commit — SHOULD-FIX 1 (42 of 47 catalog leaf verbs given retail Detail_HelpType text, 4 confirmed-null, 1 honest UNVERIFIED, retiring the class doc's overclaim), SHOULD-FIX 2 (`ComposedChatViewModelWiresOnInterfaceTextToSpewBox` — the a5a7eb4f defect class had no test), SHOULD-FIX 3 (AP-113 RETIRED — Lifestone's and Marketplace's own bespoke bad-args refusal text recovered byte-exact), SHOULD-FIX 4 (register header's stale 0.5/1.0 sentence corrected), NITs (a)-(d) (`HeadlessDiagnosticWriter` instead of `Console.WriteLine`, bounded non-quiescent-pump liveness diagnostic, hydration test doc-comment contradiction, 0x26 fallback dispatches on its own `Type`) | pending — needs the next connected round to confirm `/help <verb>` now shows retail's exact wording (see "Consolidated review" below) |
|
||||
|
||||
### CH4 closeout (2026-08-09)
|
||||
|
||||
|
|
@ -940,3 +941,133 @@ red instead of doing nothing; `/hslist badtype` and a bad-args catalog
|
|||
command (e.g. `/ls now`) now flash red SpewBox text instead of showing a
|
||||
green "Usage:" line in chat; `/help nonsenseverb` and a bare `/`/`@` now
|
||||
flash on the SpewBox instead of appearing in the chat scroll.
|
||||
|
||||
## Consolidated review — goal-window round (2026-08-10)
|
||||
|
||||
Applied the consolidated-review findings against the `09453eca` goal
|
||||
window under the same hard constraints (no subagents, no client launches,
|
||||
one commit). Baseline `03404b71`, full Release suite 12,542/4/0.
|
||||
|
||||
**SHOULD-FIX 1 — retail `/help <verb>` Detail extraction.** Retail's
|
||||
`DoHelp @0x0057F9E0` calls a resolved verb's OWN registered help callback
|
||||
with `Detail_HelpType(2)`, never `Summary_HelpType(1)` —
|
||||
`RetailClientCommandCatalog`'s ~45 leaf Definitions were all showing their
|
||||
acdream-authored Summary-shaped `HelpText` for `/help <verb>` instead.
|
||||
Located every `Help*` handler by name in `acclient_2013_pseudo_c.txt`,
|
||||
swept its exact byte extent for `push imm32` string literals against the
|
||||
PDB-paired `C:\Users\erikn\Downloads\acclient.exe` (verified MATCH), and
|
||||
confirmed each Detail-vs-Summary branch assignment by reading the actual
|
||||
decompiled if/else shape — address order and string length both proved
|
||||
UNRELIABLE indicators on their own (HelpDie's Detail sits at the HIGHER
|
||||
address, HelpCorpse's at the LOWER; the "longest text" heuristic held for
|
||||
every simple 2/3-branch case but AFK/Emote/Squelch turned out to
|
||||
CONCATENATE multiple literals within one branch instead). Found and fixed
|
||||
a real tool gap along the way: `sweep_weenie_strings.py`'s stock 800-char
|
||||
cap silently dropped several longer Detail branches as a bare `None`
|
||||
(HelpConsent 978 chars, HelpFillComponents 1036, HelpEndurance 2459) — a
|
||||
custom unbounded pass recovered them.
|
||||
|
||||
`InitializeCommands @0x00581970`'s registration blocks always render the
|
||||
CmdHashData help-pointer 4th argument as a literal `nullptr` in Binary
|
||||
Ninja's pseudo-C — a confirmed, systematic decompiler artifact (the real
|
||||
pointer is stored to a throwaway local immediately before the constructor
|
||||
call, which BN fails to thread through). Reading for the PRESENCE of that
|
||||
typed local (vs. a plain `int32_t = 0`) resolved every ambiguous verb:
|
||||
hor/hr/hom/hoa and alh/ah share their Detail text with `HelpHouse`/
|
||||
`HelpAllegiance` (i.e. `HouseOverview`/`AllegianceOverview`, already
|
||||
extracted); friends_add/friends_remove share `HelpFriends`;
|
||||
squelch/unsquelch share `HelpSquelch`. Four verbs
|
||||
(index/clist/on/off) genuinely have no help pointer — retail's own
|
||||
`DoHelp` falls straight to `UnknownCommand` for these, a CONFIRMED
|
||||
behavior now reproduced (`RetailCommandHelpTable.CatalogVerbsWithNoRetailHelp`),
|
||||
not a guess. `messagetypes`/its 3 aliases stay an honest acdream summary —
|
||||
`HelpMessageTypes` builds its text from a live enum table at runtime, not
|
||||
a static string. Final coverage (measured by test, not hand-counted): 42
|
||||
of 47 distinct catalog leaf Definitions verbatim-extracted, 4
|
||||
confirmed-null, 1 unverified.
|
||||
|
||||
`ChatCommandRouter.EmitVerbHelp` now checks
|
||||
`CatalogVerbsWithNoRetailHelp` → `RetailCommandHelpTable.TryGetCatalogVerbDetailText`
|
||||
→ `RetailClientCommandCatalog.TryGetHelpText` (the summary fallback,
|
||||
unchanged) → the pre-existing `RetailCommandHelpTable.TryGetHelpText`
|
||||
(chat-alias/channel verbs, a disjoint key space). `RetailCommandHelpTable`'s
|
||||
class doc, which claimed to cover only verbs the catalog "doesn't dispatch
|
||||
directly," is corrected — it now overlaps the catalog's own leaf verbs by
|
||||
design. A fresh full-length byte-sweep of `HelpAllegiance @0x0057ae10` (the same
|
||||
function `AllegianceOverview` already cites) also turned up one
|
||||
previously-dropped line (" WARNING! Officers banning...") the earlier
|
||||
extraction missed — now split into `AllegianceWarningLine` and restored
|
||||
in place at its correct position (between "ban list" and "info").
|
||||
|
||||
**SHOULD-FIX 2 — `OnInterfaceText` production wiring had no test.** The
|
||||
a5a7eb4f defect class (a composed hook wired but never transferred) went
|
||||
undetected because `InteractionRetainedUiCompositionTests`' `FakeFactory`
|
||||
substitutes a stub `ChatVM` for every composition-order test and never
|
||||
exercises `RetailInteractionRetainedUiCompositionFactory`'s real wiring.
|
||||
Extracted the wiring (`InteractionRetainedUiComposition.cs`, previously
|
||||
inline at the line the review cited) into its own testable
|
||||
`CreateChatViewModel` method — no GPU/dat/UiHost dependency, so it is
|
||||
callable directly against the existing null-heavy test `Fixture` without
|
||||
a full `Compose()`. New test
|
||||
`ComposedChatViewModelWiresOnInterfaceTextToSpewBox` asserts the hook is
|
||||
non-null AND that a `ShowInterfaceText` call actually lands in
|
||||
`RuntimeCommunicationState.SpewBox` after a `Tick`.
|
||||
|
||||
**SHOULD-FIX 3 — AP-113 RETIRED.** `DoLifestone`/`DoMarketplace
|
||||
@0x0056FC70`/`@0x0056FCE0` print their OWN 0x1A string for bad args and
|
||||
return 1 — retail never reaches the generic `HandleFailureEvent(0x26)`
|
||||
fallback for either. Both literals were mis-attributed by Binary Ninja to
|
||||
unrelated vtable-slot symbols (the same pooled-string artifact class this
|
||||
campaign has hit before); read directly off the raw `push imm32` operand
|
||||
and decoded as UTF-16LE against the PDB-paired binary: `"Please see @help
|
||||
lifestone for more information on how to use this command."` and the
|
||||
`marketplace` analogue. `RetailClientCommandCatalog.Lifestone`/`Marketplace`
|
||||
now carry them as `InvalidArgumentsText`. AP-113 is RETIRED (its only
|
||||
named verb, lifestone, now has its exact wording); Marketplace was never a
|
||||
filed divergence, so its fix is a plain accuracy improvement alongside it.
|
||||
`ChatCommandRouter.cs`'s bad-args comment is corrected to name both
|
||||
verbs and state the 0x26 fallback's actual scope (verbs with NO bespoke
|
||||
retail refusal, not every bad-args case).
|
||||
|
||||
**SHOULD-FIX 4 — register header sentence fixed.** `retail-divergence-
|
||||
register.md`'s AP section header still said AP-190 "ships one shared
|
||||
0.5/1.0 default rather than gmMainChatUI's per-class 1.0/1.0 override" —
|
||||
refuted by `cc582899`, which fixed the shipped default TO 1.0/1.0 (the
|
||||
row's own REWORDED (2) already said so; only the header summary hadn't
|
||||
caught up). Corrected in place; the active-row count drops to 132 with
|
||||
AP-113's retirement.
|
||||
|
||||
**NITs (a)-(d).** (a) `HeadlessStaticStateAudit`'s single-session log line
|
||||
now routes through the injected `HeadlessDiagnosticWriter` (constructed
|
||||
before the audit call in `HeadlessProcessHost`, not after) instead of a
|
||||
bare `Console.WriteLine` that bypassed the structured stream every other
|
||||
headless diagnostic uses. (b) `HeadlessSessionWorldProjection.PumpFirstEntry`
|
||||
gained a bounded, diagnostic-only trip-wire: 300 consecutive non-quiescent
|
||||
pumps (generous — many seconds at the host's tick cadence) emit ONE
|
||||
message naming the stuck landblock; no retry, no behavior change, the gate
|
||||
itself is untouched. (c) The #365 hydration test's doc comment is
|
||||
corrected — it claimed reverting the `IsQuiescent` gates makes the
|
||||
conductor reach "`PublicationCommitted` — a non-null dormant controller,"
|
||||
which contradicts the diagnosis doc's own §8 finding (the controller is
|
||||
ALREADY built AND published, `CanExecuteLiveMovement = True`, when the
|
||||
assertion actually fails); the comment now also states plainly that this
|
||||
bounded xunit test proves the GATE, not the #365 stall's closure — that
|
||||
broader claim's evidence is the separate live-ACE run §8 records. (d)
|
||||
`ChatCommandRouter`'s 0x26 fallback now resolves
|
||||
`WeenieErrorMessages.Resolve(0x026u, null)` once and dispatches on its own
|
||||
`Type` field instead of assuming `ShowInterfaceText`'s hardcoded
|
||||
`ClientLocal` is correct for it (it is, today — this just stops relying on
|
||||
that being an unstated invariant).
|
||||
|
||||
Full Release suite: 12,553 passed / 4 skipped / 0 failed (net +11 tests
|
||||
from this round, zero regressions; one `LandblockPresentationPipelineTests`
|
||||
failure observed on one parallel full-solution run reproduced as PASS in
|
||||
isolation and on two subsequent full-solution reruns — pre-existing
|
||||
run-order flakiness in an unrelated streaming test, not caused by this
|
||||
round's changes). No subagent review this session (hard constraint); no
|
||||
connected user gate (hard constraint — no client launches). The next
|
||||
connected round should confirm `/help die`, `/help lifestone`,
|
||||
`/help endurance`, and a handful of the other 42 newly-extracted verbs
|
||||
show retail's exact wording instead of the old acdream summaries, and that
|
||||
`/help index`/`/clist`/`/on`/`/off` now show "Unknown command" via the
|
||||
SpewBox.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue