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() {