From 35454a9f5843e690a8a315532f77c1634b8020d3 Mon Sep 17 00:00:00 2001 From: Erik Date: Mon, 24 Aug 2026 12:53:11 +0200 Subject: [PATCH] fix #436: combat no-target refusal reaches the SpewBox with retail's exact text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attacking with no valid target has told the player nothing since Campaign V slice V11 orphaned the DebugVM toast the message was wired to (#434 found the drop; this closes it retail-faithfully). Ground truth from the Ghidra decompile of ClientCombatSystem::ExecuteAttack (0x0056bb70): retail writes "You must select a valid combat target before attacking" via ClientSystem::AddTextToScroll(..., 0x1A, true, 0) — the ClientLocal SpewBox channel this codebase already routes every other client-local refusal through. And retail has ONE message, not the two we carried: attacking outside melee/missile modes is silent (ExecuteAttack is unreachable there), so the invented "Enter melee or missile combat first" text is deleted rather than rerouted, and the invented "No monster target" is replaced by the retail string, which joins ClientTextRefusals with its decomp citation. Wiring: CombatFeedbackSlot gains the sibling BindOwned session-lifetime shape, and SessionPlayerComposition.CompleteSessionPlayer binds it to RuntimeCommunicationState.AddText(ClientLocal) with session-owned teardown — a torn-down session's slot returns to its silent unbound state. A binding-seam test (CompleteSessionPlayerBindsCombatFeedbackToTheClientLocalSpewBoxRoute) inspects the compiled composition for the BindOwned call and its AddText-routing lambda, so the slot can never again pass its unit tests while production leaves it unbound — the exact failure mode that hid this defect. The two tests that pinned the invented strings now pin the retail contract (exact string; silence for the unsupported-mode case). Full hermetic suite 15,325 passed / 0 failed. Co-Authored-By: Claude Fable 5 --- docs/ISSUES.md | 24 ++++++++++- .../Combat/LiveCombatAttackOperations.cs | 43 ++++++++++++++----- .../Composition/SessionPlayerComposition.cs | 10 +++++ src/AcDream.Core/Chat/ClientTextRefusals.cs | 15 +++++++ .../Combat/CombatFeedbackSlotTests.cs | 37 ++++++++++++++-- .../Combat/LiveCombatAttackOperationsTests.cs | 15 +++++-- .../SessionPlayerCompositionTests.cs | 32 ++++++++++++++ 7 files changed, 157 insertions(+), 19 deletions(-) diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 915cada9..aefc049b 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -138,9 +138,29 @@ id list), so deletion order matters. --- -## #436 — Combat refusal text ("No monster target") is silently dropped +## #436 — CLOSED: Combat refusal text ("No monster target") is silently dropped -**Status:** OPEN +**Status:** CLOSED 2026-08-24, retail-faithfully. Ghidra decompile of +`ClientCombatSystem::ExecuteAttack` (0x0056bb70) settled both open +questions: (1) retail's exact string is **"You must select a valid combat +target before attacking"**, routed via +`ClientSystem::AddTextToScroll(..., 0x1A, true, 0)` — the ClientLocal +SpewBox channel our chat pipeline already owns; (2) retail has ONE message, +not two — attacking while not in melee/missile mode is silent (that path is +unreachable in retail's dispatch), so the invented "Enter melee or missile +combat first" text is deleted rather than rerouted. Implementation: the +string joined `ClientTextRefusals` with its decomp citation; +`CombatFeedbackSlot` gained the sibling `BindOwned` session-lifetime shape; +`SessionPlayerComposition.CompleteSessionPlayer` binds it to +`RuntimeCommunicationState.AddText(ClientLocal)` with session-owned +teardown. Coverage includes a binding-seam test +(`CompleteSessionPlayerBindsCombatFeedbackToTheClientLocalSpewBoxRoute`) +so the slot can never again pass its unit tests while production leaves it +unbound — the exact failure mode that hid this for months. + +**Original report follows.** + +**Status (original):** OPEN **Severity:** MEDIUM (missing user feedback on a common action) **Filed:** 2026-08-24 (exposed by #434's dead-code removal) **Component:** combat / chat presentation diff --git a/src/AcDream.App/Combat/LiveCombatAttackOperations.cs b/src/AcDream.App/Combat/LiveCombatAttackOperations.cs index 96816196..79fff29c 100644 --- a/src/AcDream.App/Combat/LiveCombatAttackOperations.cs +++ b/src/AcDream.App/Combat/LiveCombatAttackOperations.cs @@ -67,16 +67,18 @@ internal interface ICombatFeedbackSink } /// -/// Routes combat refusal text ("No monster target") to whichever surface is -/// bound to show it. +/// Routes combat refusal text to whichever surface is bound to show it — +/// in production, the SpewBox as RetailLogTextType.ClientLocal +/// (retail: ClientCombatSystem::ExecuteAttack 0x0056bb70 → +/// ClientSystem::AddTextToScroll(..., 0x1A, ...)). /// /// -/// #434: the bound target used to be the developer DebugVM, which -/// Campaign V slice V11 left unreachable — nothing has constructed it since, -/// so both messages below have been going nowhere. The binding target is now a -/// plain delegate so this seam no longer depends on that dead class, but it -/// still has no production binder: wiring it to the chat window, where retail -/// puts this text, is #436. +/// #434/#436: the bound target used to be the developer DebugVM, +/// which Campaign V slice V11 left unreachable, so these messages were +/// silently dropped. The session composition now owns the binding via +/// (same lifetime shape as +/// ); before a session binds — +/// and after it tears down — is a deliberate no-op. /// internal sealed class CombatFeedbackSlot : ICombatFeedbackSink { @@ -91,6 +93,16 @@ internal sealed class CombatFeedbackSlot : ICombatFeedbackSink _target = target; } + public IDisposable BindOwned(Action target) + { + ArgumentNullException.ThrowIfNull(target); + if (_target is not null) + throw new InvalidOperationException( + "Combat feedback is already bound to a presentation target."); + _target = target; + return new Binding(this, target); + } + public void Unbind(Action target) { ArgumentNullException.ThrowIfNull(target); @@ -99,6 +111,12 @@ internal sealed class CombatFeedbackSlot : ICombatFeedbackSink } public void Show(string message) => _target?.Invoke(message); + + private sealed class Binding(CombatFeedbackSlot slot, Action target) + : IDisposable + { + public void Dispose() => slot.Unbind(target); + } } internal sealed class CombatAttackOperationsSlot @@ -221,7 +239,10 @@ internal sealed class LiveCombatAttackOperations if (!CombatInputPlanner.SupportsTargetedAttack(_combat.CurrentMode)) { - _feedback.Show("Enter melee or missile combat first"); + // Retail is SILENT here: ClientCombatSystem::ExecuteAttack + // (0x0056bb70) is unreachable outside melee/missile modes, so + // no user-facing text exists for this case — only the no-target + // branch below speaks (#436). Console.WriteLine( "combat: attack ignored; not in melee/missile combat mode"); return false; @@ -229,7 +250,9 @@ internal sealed class LiveCombatAttackOperations if (_targets.GetSelectedOrClosestCombatTarget(_settings.AutoTarget) is null) { - _feedback.Show("No monster target"); + // Retail: ExecuteAttack's edi==0 branch (0x0056bc05) → + // AddTextToScroll(0x1A) — the ClientLocal SpewBox channel. + _feedback.Show(AcDream.Core.Chat.ClientTextRefusals.MustSelectCombatTarget); Console.WriteLine("combat: attack ignored; no creature target found"); return false; } diff --git a/src/AcDream.App/Composition/SessionPlayerComposition.cs b/src/AcDream.App/Composition/SessionPlayerComposition.cs index bbb7bb9d..14fbe652 100644 --- a/src/AcDream.App/Composition/SessionPlayerComposition.cs +++ b/src/AcDream.App/Composition/SessionPlayerComposition.cs @@ -809,6 +809,16 @@ internal sealed class SessionPlayerCompositionPhase liveSessionSource, liveSessionSource, d.CombatFeedback))); + // #436: combat refusal text goes to the SpewBox as ClientLocal — + // retail's ClientCombatSystem::ExecuteAttack (0x0056bb70) routes + // "You must select a valid combat target before attacking" through + // AddTextToScroll(0x1A). Session-owned so a torn-down session's + // slot goes back to its silent unbound state. + bindings.Adopt( + "combat feedback", + d.CombatFeedback.BindOwned( + text => d.Communication.AddText( + text, RetailLogTextType.ClientLocal))); Fault(SessionPlayerCompositionPoint.CombatOperationsBound); MouseLookController? mouseLook = diff --git a/src/AcDream.Core/Chat/ClientTextRefusals.cs b/src/AcDream.Core/Chat/ClientTextRefusals.cs index a1851827..d7a65c8d 100644 --- a/src/AcDream.Core/Chat/ClientTextRefusals.cs +++ b/src/AcDream.Core/Chat/ClientTextRefusals.cs @@ -144,4 +144,19 @@ public static class ClientTextRefusals /// SpewBox-only channel every other refusal in this file uses. /// public const string CantLogOffMidAir = "Cannot log off while in mid-air."; + + /// + /// #436 — attack pressed with no valid combat target. Inline literal in + /// ClientCombatSystem::ExecuteAttack (0x0056bb70; the + /// refusal branch at 0x0056bc05), routed via + /// ClientSystem::AddTextToScroll(..., 0x1A, true, 0) — the same + /// RetailLogTextType.ClientLocal SpewBox channel as the rest of + /// this file, though unlike the 11 globals above it is not one of + /// ClientCommunicationSystem's static-ctor strings. Retail shows + /// ONLY this message here: attacking while not in melee/missile mode is + /// silent (that path is unreachable in retail's dispatch), so acdream + /// deliberately emits nothing for that case either. + /// + public const string MustSelectCombatTarget = + "You must select a valid combat target before attacking"; } diff --git a/tests/AcDream.App.Tests/Combat/CombatFeedbackSlotTests.cs b/tests/AcDream.App.Tests/Combat/CombatFeedbackSlotTests.cs index 2f2e6d9e..ec2627e0 100644 --- a/tests/AcDream.App.Tests/Combat/CombatFeedbackSlotTests.cs +++ b/tests/AcDream.App.Tests/Combat/CombatFeedbackSlotTests.cs @@ -34,13 +34,42 @@ public sealed class CombatFeedbackSlotTests Assert.Throws(() => slot.Bind(_ => { })); } + [Fact] + public void SessionOwnedBindingForwardsAndItsDisposalRestoresSilence() + { + // #436: the session composition owns the production binding + // (SpewBox/ClientLocal) through BindOwned, mirroring + // CombatAttackOperationsSlot's lifetime shape — disposal at session + // teardown returns the slot to its silent unbound state instead of + // leaving a dead session's chat sink reachable. + var slot = new CombatFeedbackSlot(); + List sink = []; + + IDisposable binding = slot.BindOwned(sink.Add); + slot.Show("routed"); + Assert.Equal(["routed"], sink); + + binding.Dispose(); + slot.Show("after-teardown"); + Assert.Equal(["routed"], sink); + } + + [Fact] + public void SessionOwnedBindingRefusesASecondOwner() + { + var slot = new CombatFeedbackSlot(); + using IDisposable binding = slot.BindOwned(_ => { }); + + Assert.Throws(() => slot.BindOwned(_ => { })); + } + [Fact] public void AnUnboundSlotDropsItsMessages() { - // #434/#436: this is the shipped behavior, not an aspiration — - // nothing binds the slot in production, so combat refusal text - // ("No monster target") is discarded. Pinned so the day it gets a - // real binder, this test is the one that has to change. + // Deliberate: before a session composes (and after one tears down) + // there is no chat surface, so Show is a no-op rather than a queue — + // matching retail, where the refusal text only exists inside a live + // session's ExecuteAttack path (0x0056bb70). var slot = new CombatFeedbackSlot(); slot.Show("dropped"); diff --git a/tests/AcDream.App.Tests/Combat/LiveCombatAttackOperationsTests.cs b/tests/AcDream.App.Tests/Combat/LiveCombatAttackOperationsTests.cs index 12ca50ae..72f2e963 100644 --- a/tests/AcDream.App.Tests/Combat/LiveCombatAttackOperationsTests.cs +++ b/tests/AcDream.App.Tests/Combat/LiveCombatAttackOperationsTests.cs @@ -54,13 +54,18 @@ public sealed class LiveCombatAttackOperationsTests } [Fact] - public void UnsupportedCombatModeUsesTypedFeedbackSink() + public void UnsupportedCombatModeRefusesSilently() { + // #436: retail emits NO text for this case — + // ClientCombatSystem::ExecuteAttack (0x0056bb70) is unreachable + // outside melee/missile modes, so there is no retail string to show. + // The pre-#436 "Enter melee or missile combat first" message was + // invented dev text and is deleted, not rerouted. Harness harness = CreateHarness(inWorld: true); Assert.False(harness.Owner.CanStartAttack()); - Assert.Equal(["Enter melee or missile combat first"], harness.Feedback.Messages); + Assert.Empty(harness.Feedback.Messages); Assert.Equal(0, harness.Targets.ResolveCount); } @@ -88,7 +93,11 @@ public sealed class LiveCombatAttackOperationsTests Assert.False(harness.Owner.CanStartAttack()); - Assert.Equal(["No monster target"], harness.Feedback.Messages); + // Retail's exact string — ClientCombatSystem::ExecuteAttack's + // no-valid-target branch (0x0056bc05), AddTextToScroll(0x1A). + Assert.Equal( + [AcDream.Core.Chat.ClientTextRefusals.MustSelectCombatTarget], + harness.Feedback.Messages); } private static Harness CreateHarness(bool inWorld = false) diff --git a/tests/AcDream.App.Tests/Composition/SessionPlayerCompositionTests.cs b/tests/AcDream.App.Tests/Composition/SessionPlayerCompositionTests.cs index 2167540c..288a21d7 100644 --- a/tests/AcDream.App.Tests/Composition/SessionPlayerCompositionTests.cs +++ b/tests/AcDream.App.Tests/Composition/SessionPlayerCompositionTests.cs @@ -106,6 +106,38 @@ public sealed class SessionPlayerCompositionTests field => field.FieldType == typeof(GameWindow)); } + [Fact] + public void CompleteSessionPlayerBindsCombatFeedbackToTheClientLocalSpewBoxRoute() + { + // #436 binding-seam check (a slot can pass its own unit tests while + // nothing in production binds it — that is exactly how the combat + // refusals were silently dropped for months): CompleteSessionPlayer + // must session-own a CombatFeedbackSlot.BindOwned whose target lambda + // routes to RuntimeCommunicationState.AddText (the ClientLocal + // SpewBox chokepoint; retail ClientCombatSystem::ExecuteAttack + // 0x0056bb70 -> AddTextToScroll(0x1A)). + MethodInfo complete = RequiredMethod( + typeof(SessionPlayerCompositionPhase), + "CompleteSessionPlayer"); + Assert.Contains( + CompiledCallGraph.Read(complete), + call => call.Target.DeclaringType + == typeof(AcDream.App.Combat.CombatFeedbackSlot) + && call.Target.Name + == nameof(AcDream.App.Combat.CombatFeedbackSlot.BindOwned)); + + IEnumerable lambdas = + CompiledCallGraph.ReadMethodReferences(complete) + .Select(call => call.Target) + .Where(method => method.GetMethodBody() is not null); + Assert.Contains( + lambdas, + method => CompiledCallGraph.Read(method).Any(call => + call.Target.DeclaringType + == typeof(AcDream.Runtime.Gameplay.RuntimeCommunicationState) + && call.Target.Name == "AddText")); + } + [Fact] public void ProductionPhaseStartsStreamerBeforeSessionAndTransfersPortalLast() {