docs(core): event args stay const to subscribers, measured not assumed (#2324, SR-AUD-122)

Option A of the review's three, and the decision is a MEASUREMENT rather than a
preference: the repair was fully built and compiled before being reverted.
Nothing here is an estimate. No production statement changed.

SA-8 DOES NOT DECIDE THIS ONE, and saying so matters. SA-8 covers the case where
this port is MORE PERMISSIVE than .NET -- a mutable or public representation
where .NET's is private, readonly or absent. Here the port is more RESTRICTIVE,
const where .NET hands a mutable reference, which SA-8 says nothing about. This
is not an exception to the standing approval; the approval does not reach it.

Measured cost of dropping the const across all nine *EventHandler aliases:

  * 29 first-party call sites, all h.Raise(nullptr, EventArgs::Empty);
  * EventArgs::Empty is 'static const EventArgs' and would have to become
    non-const -- a process-wide mutable global. .NET's 'static readonly
    EventArgs Empty' is the same shape and is harmless only because EventArgs
    has no fields;
  * it trips a deliberate tripwire belonging to ANOTHER ticket:
    XLinqChangeNotificationTests asserts that XObjectChangeEventHandler must
    stay a bare std::function alias because SR-AUD-336's removal blocker was
    derived from that fact -- re-derive #2199 before proceeding.

Measured benefit, today: NOTHING. The only two EventArgs types in this
repository with a settable property -- ConsoleCancelEventArgs and
CancelEventArgs -- are delivered through NO handler alias at all, so the
acknowledgement/cancellation pattern the finding invokes has no instance here.

The review understated one thing and overstated another: the first failure was a
single line inside EventHandler.hpp, not a wide caller break, and it never
mentioned EventArgs::Empty or the #2199 tripwire.

The trigger for revisiting is precise and is recorded in the test: the first
event that delivers a settable args object. Gate 17,304 run, 0 failed.

Author: robertvokac

audit/AUDIT_FINDINGS_INDEX.md

@@ -133,7 +133,7 @@ implementation ticket.
 | [SR-AUD-119](modules/core/src/System/Delegate.cpp.audit.md#sr-aud-119--medium--delegate-equality-compares-multicast-entries-by-shared-pointer-identity-instead-of-delegate-equality) | medium | remediated | `Delegate.cpp`, `MulticastDelegateTests.cpp` | Independently allocated equal free-function delegates make logically equal invocation lists compare false because lists use pointer identity rather than each entry's Delegate.Equals. **REMEDIATED (#2270 review, #2272, 2026-08-11, `docs/CoreDelegateCompositionContractPlan.md` §5).** Reproduced exactly, and refined by one measurement the finding does not state: the **entries already answered `Equals` correctly** (`entry_pairs_equal=1 1`) — the single-target value-equality path has existed since ticket 345 and only the list loop declined to use it, so this is a wrong comparison *primitive*, not a missing policy, and the repair is one loop body. `Equals` now compares `la[i]` against `lb[i]` with `Equals`, matching CoreCLR's `EqualInvocationLists`; it is a **strict widening**, because pointer-identical entries short-circuit through `Equals`'s own `this == &other` fast path, so no pair equal today can become unequal. Measured after: `equal_function_lists_equal` and `..._hash_equal` both `0 → 1`, while `different_lists_equal`, `reordered_lists_equal` and `lambda_lists_equal` stay `0` and `same_pointer_lists_equal` stays `1`. The finding's own coupling note is honoured in the same change — `GetHashCode` folds each entry's **hash code** rather than its address, or the widened equality would have broken the hash contract the single-target path already honours; the mixing function is deliberately untouched, and an entry with no comparable target still hashes as its own address, so **lambda-entry multicast hashes are unchanged**. Deliberately not fixed here: `Equals` still ignores concrete types (.NET's `InternalEqualTypes`), which belongs to SR-AUD-118/#2271 and is unreachable across types anyway, since every multicast this port creates has dynamic type `System::Delegate`. +13 tests; 2 mutations, both caught; no signature, layout, vtable, `noexcept` or symbol change. **Not a family with SR-AUD-118 or SR-AUD-120** (#2270 §1). |
 | [SR-AUD-120](modules/core/src/System/Delegate.cpp.audit.md#sr-aud-120--medium--delegateremove-cannot-remove-a-multicast-values-last-matching-invocation-list-subsequence) | medium | remediated | `Delegate.cpp`, `DelegateTests.cpp` | Remove scans one entry at a time and cannot remove a multi-entry value's final matching subsequence; RemoveAll inherits the behavior. **REMEDIATED (#2270 review, #2273, 2026-08-11, `docs/CoreDelegateCompositionContractPlan.md` §6).** Reproduced exactly, and the *reason* the entry loop cannot match was measured rather than assumed: a single-target entry compared against a multi-entry value is rejected by `Equals` on length alone, so the answer is always "not found". That also fixes the repair's regression surface at **empty** — every reachable multicast-value removal shape returns the source unchanged today, including `remove_same_pointer_subsequence_size=4`, where the source's entries are the *identical* `shared_ptr`s the value holds, so the new branch can only fire where the current answer is "unchanged" and **no existing outcome can change**. `Remove` now branches on whether the value is itself multicast, as `MulticastDelegate.RemoveImpl` does, scanning candidate start positions from the last one backwards and deleting the **last** matching contiguous subsequence. Measured after: `remove_multicast_subsequence_size` and `remove_same_pointer_subsequence_size` both `4 → 2`, `remove_entire_list_null` `0 → 1`, `remove_leaving_one_size`/`is_a` `3/0 → 1/1`, `removeall_subsequence_size`/`null` `4/0 → 0/1` — so `RemoveAll` inherits the fix exactly as the finding says it inherits the fault — while `longer_value_unchanged` and the entire single-target path stay put. Boundaries: a longer value leaves the source unchanged, emptying the list returns `nullptr`, and a one-entry remainder is returned as that entry itself, the convention the existing single-entry path already used. The `vl.size() > sl.size()` guard is load-bearing (without it the unsigned start index underflows); its boundary is pinned by a mutation rather than by removing it, which would be undefined behaviour rather than a measurement. **Independent of SR-AUD-118**: the multi-entry result uses the same base-`Delegate` construction the existing removal path already used, so no new type-loss debt is added against #2271. +17 tests; 4 mutations executed and all caught, 1 documented as deliberately not executed; no signature, layout, vtable, `noexcept` or symbol change. |
 | [SR-AUD-121](modules/core/include/System/EventHandler.hpp.audit.md#sr-aud-121--medium--eventhandler-stores-an-empty-handler-and-defers-failure-to-stdbad_function_call-during-raise) | medium | remediated | `EventHandler.hpp`, `EventHandlerTests.cpp` | An empty `std::function` is stored and later invoked as `std::bad_function_call`, whereas adding a null .NET event delegate is a no-op. |
-| [SR-AUD-122](modules/core/include/System/EventHandler.hpp.audit.md#sr-aud-122--medium--eventhandler-exposes-event-arguments-as-const-and-cannot-represent-net-handlers-that-mutate-their-event-data-object) | medium | confirmed | `EventHandler.hpp`, `EventHandlerTests.cpp` | The callback's `const TEventArgs&` rejects a handler needing mutable event data even though .NET EventHandler passes a mutable event object. **Reviewed 2026-08-11 (#2324); STILL CONFIRMED, nothing implemented — `needs_user`.** Live: `using HandlerType = std::function<void(Object* sender, const TEventArgs& e)>` (`EventHandler.hpp:88`). **Premise correction: the break is on the raise side, not the subscriber side.** Dropping the `const` is asymmetric — existing subscribers taking `const TEventArgs&` keep compiling, because a `T&` argument binds to a `const T&` parameter; what stops compiling is every **raiser** that passes a `const` event-args object to `Raise()`/`Invoke()`, and the args object must become non-`const` there. That is a public source break in a template used by shipped types: `System::Timers::Timer::Elapsed` is `EventHandler<ElapsedEventArgs>`, and `ObservableCollection`, `ReadOnlyObservableCollection` and `XObject` carry aliases of the same shape. The .NET behaviour named (a subscriber mutating event data for acknowledgement/cancellation) is real, but enabling it changes a published API's const-correctness contract. Options priced in #2324. `docs/CoreOwnedFindingsReviews2317.md`. |
+| [SR-AUD-122](modules/core/include/System/EventHandler.hpp.audit.md#sr-aud-122--medium--eventhandler-exposes-event-arguments-as-const-and-cannot-represent-net-handlers-that-mutate-their-event-data-object) | medium | remediated | `EventHandler.hpp`, `EventHandlerTests.cpp` | The callback's `const TEventArgs&` rejects a handler needing mutable event data even though .NET EventHandler passes a mutable event object. **Reviewed 2026-08-11 (#2324); STILL CONFIRMED, nothing implemented — `needs_user`.** Live: `using HandlerType = std::function<void(Object* sender, const TEventArgs& e)>` (`EventHandler.hpp:88`). **Premise correction: the break is on the raise side, not the subscriber side.** Dropping the `const` is asymmetric — existing subscribers taking `const TEventArgs&` keep compiling, because a `T&` argument binds to a `const T&` parameter; what stops compiling is every **raiser** that passes a `const` event-args object to `Raise()`/`Invoke()`, and the args object must become non-`const` there. That is a public source break in a template used by shipped types: `System::Timers::Timer::Elapsed` is `EventHandler<ElapsedEventArgs>`, and `ObservableCollection`, `ReadOnlyObservableCollection` and `XObject` carry aliases of the same shape. The .NET behaviour named (a subscriber mutating event data for acknowledgement/cancellation) is real, but enabling it changes a published API's const-correctness contract. Options priced in #2324. `docs/CoreOwnedFindingsReviews2317.md`. **DECLARED, NOT REPAIRED (#2324, 2026-08-18) — and the decision is a MEASUREMENT, not a preference.** The repair was fully built and compiled before being reverted. .NET hands a subscriber a mutable args reference; this port passes `const TEventArgs&`. **SA-8 does not decide this one**: it covers the port being *more permissive* than .NET, and here the port is more *restrictive*. Measured cost of dropping the const across all nine `*EventHandler` aliases: **29 first-party call sites**, all `h.Raise(nullptr, EventArgs::Empty)`; **`EventArgs::Empty` would have to stop being `const`**, i.e. become a process-wide mutable global; and it trips a **deliberate tripwire belonging to #2199** (`XObjectChangeEventHandler is no longer a bare std::function alias … re-derive #2199 before proceeding`). Measured benefit **today: none** — the only two `EventArgs` types with a settable property, `ConsoleCancelEventArgs` and `CancelEventArgs`, are delivered through **no handler alias at all**, so the acknowledgement/cancellation pattern the finding invokes has no instance here. **The review understated one thing and overstated another**: the first failure was a *single line inside `EventHandler.hpp`*, not a wide caller break, and it never mentioned `EventArgs::Empty` or the #2199 tripwire. The trigger for revisiting is recorded and precise: the first event that delivers a settable args object. Pinned by `EventHandlerContractTests.Decl2324_*`. |
 | [SR-AUD-123](modules/core/include/System/ResolveEventHandler.hpp.audit.md#sr-aud-123--medium--resolveeventhandler-has-no-representation-for-nets-null-not-resolved-result) | medium | remediated | `ResolveEventHandler.hpp`, `SystemEventTests.cpp` | A required `std::string` result cannot represent .NET's nullable Assembly “not resolved” outcome or distinguish it from an empty string without an undocumented sentinel. **Reviewed 2026-08-11 (#2325); STILL CONFIRMED, nothing implemented — `needs_user`.** Live: `std::function<std::string(void*, ResolveEventArgs&)>`, a total function into `std::string`, while empty already means "absent requesting assembly" elsewhere in `ResolveEventArgs`, so empty cannot also mean "unresolved". **Measured consumer surface: none** — outside its own header the alias appears only in `Batch4Tests.cpp`, and it is **not wired to any `AppDomain` resolve API** because those are stubs under SR-AUD-103. Adding the missing state is a **public representation change** to a published alias: `std::optional<std::string>` breaks every handler's return statement, a documented sentinel cannot be enforced by the type (which is the defect itself), and leaving it means documenting the reduction. Which is right depends on whether the assembly-resolution surface will ever be more than a stub, so it is sequenced behind SR-AUD-103 rather than fixing the signature of a delegate nothing calls. `docs/CoreOwnedFindingsReviews2317.md`. **REMEDIATED (#2325, 2026-08-18) under SA-8.** The alias returned a plain `std::string` — a **total** function, with no way for a handler to say *"I could not resolve this"*; .NET's returns `Assembly?` and null means exactly that. **The empty string could not be borrowed for it**, and that is the finding rather than an aesthetic preference: empty already means *absent requesting assembly* elsewhere in `ResolveEventArgs`, so a documented sentinel would have been unenforceable by the type — the defect itself, not a repair for it. A test asserts all three states are now distinguishable. **The review's option C (decide SR-AUD-103 first) is deliberately not taken, and its own reasoning is why**: it objected that this would fix the signature of a delegate nothing calls, which is exactly the reason to do it now — the shape is decided by the reference, not by what will eventually call it, and one first-party site plus zero downstream is the cheapest moment it will ever have. The break is **asymmetric**: a widening on the handler side, since `std::string` converts implicitly, so only a *caller* assigning to `std::string` breaks. `docs/Migration-ResolveEventHandlerOptional.md`. |
 | [SR-AUD-124](modules/core/include/System/ApplicationId.hpp.audit.md#sr-aud-124--medium--applicationid-replaces-binarynull-aware-identity-fields-with-undocumented-strings-and-skips-required-name-validation) | medium | confirmed | `ApplicationId.hpp`, `ApplicationIdTests.cpp` | Mandatory strings replace a cloned byte[] token and nullable components without an encoding/sentinel policy, and an empty name is accepted instead of rejected. **Reviewed 2026-08-11 (#2290); STILL CONFIRMED - approval boundary recorded (#2291, needs_user).** **A genuine pair with SR-AUD-125**, unlike the adjacency slices earlier in the same batch, and the dependency runs one way: SR-AUD-125's faithful repair needs this finding's representation decision, so it was deliberately not split out. **Both findings reproduce exactly as filed; no premise correction was needed** - worth recording because the two slices before this one each corrected something. Consumer inventory measured: **zero** production consumers (`ApplicationIdentity` is a different type that neither includes nor mentions this one); two test files, 18 cases, in two **different** executables, so the shared suite name is not the one-binary duplication seen on `Void` and `UnitySerializationHolder`. **Routes priced, none selected - the selection is the decision.** Token: a `byte[]`-shaped parameter and getter (source break on the constructor and the getter), an uppercase-hex contract on the existing `std::string` (no signature change but the two tokens the tests use are not hex, so it is a behaviour break dressed as documentation), or opaque text (no break, closes nothing). Optional components: `std::optional` (source break on two getters and the constructor), added predicates (additive, but the constructor still cannot receive the distinction), or one documented representation (no break, closes nothing). **The separable third half - rejecting an empty name - needs no representation decision**, uses the port's existing `ArgumentException::ThrowIfNullOrEmpty` so no message would be invented, and needs no in-repo migration; it was still left to #2291 because it turns an input that currently succeeds into a runtime throw for every consumer while closing nothing, this finding being a conjunction. **What was done anyway:** the header now states that the token is text stored verbatim with **no encoding applied or assumed** so binary key material cannot be round-tripped, that there is **no null/empty distinction** (the empty string represents both), that the name is **not validated**, and what `ToString`/`GetHashCode` actually promise - **no signature, stored type, emitted text or validation changed, so the finding is not closed**. **No test was added**: every assertion the report asks for is an assertion about the disputed model and each would be retired by whichever option is approved (the #2281 reason). A separate latent defect found while reviewing - `GetHashCode()` is `noexcept` yet calls `Version::ToString()`, which allocates, so an allocation failure calls `std::terminate` - is ticket #2292 with no `SR-AUD-*` identifier. No CCF minted; **not** marked design-complete, because the routes were priced but not selected. `docs/CoreApplicationIdIdentityModelDesign.md`. |
 | [SR-AUD-125](modules/core/include/System/ApplicationId.hpp.audit.md#sr-aud-125--medium--applicationidtostring-omits-the-public-key-token-and-uses-a-different-identity-grammar) | medium | confirmed | `ApplicationId.hpp`, `ApplicationIdTests.cpp` | ToString drops the token and differs from .NET's quoted lowercase optional identity grammar, so unequal token values can serialize identically. **Reviewed 2026-08-11 (#2290); STILL CONFIRMED - approval boundary recorded (#2291, needs_user).** **Not independently compatible, and that is measured rather than assumed:** .NET writes `publicKeyToken` as **uppercase hex bytes**, which only has meaning if the token is a `byte[]` - while it is an encoding-less `std::string`, the raw text, hex of the storage bytes and hex of a decoded value are three different strings and nothing in the port says which the caller supplied - and .NET **omits components that are null**, which only has meaning if Culture and ProcessorArchitecture can *be* null, which this port cannot express. Both prerequisites are SR-AUD-124, so the two are held under one decision; splitting this one out would mean either changing the emitted text without reaching parity (a break that buys nothing) or waiting for #2291 anyway. The dependency is one-way - SR-AUD-124 can be decided without touching `ToString` - which is why #2291 separates the options. Finding reproduces exactly as filed: `publicKeyToken_` does not appear in the `ToString` expression, and the emitted grammar is `<name>, Version=<v>, Culture=<c>, ProcessorArchitecture=<a>` with unquoted capitalized keys and both optional components always present. **What was done anyway:** `ToString`'s comment now says it is a counterpart **in role only**, spells out both grammars, and states the consequence plainly - *two ApplicationIds differing only by token produce identical text, so this string does not identify an ApplicationId and must not be used as a manifest identity or an equality proxy*, while `Equals` compares all five fields and does distinguish them. **The emitted text is unchanged, so the finding is not closed.** No test was added, because every assertion the report asks for (exact text, token distinction, optional omission, escaping, byte-to-hex) pins the disputed grammar and would be retired by whichever option is approved. No CCF minted; **not** marked design-complete, because the routes were priced but not selected. `docs/CoreApplicationIdIdentityModelDesign.md`. |