fix(core): SequencePosition's components are private (#2332, SR-AUD-069)

object_ and integer_ were public mutable data members, so a caller could rewrite
a position after the sequence handed it out -- to an unrelated segment, a
dangling pointer, or an offset the owning sequence never produced -- and every
downstream reader would then trust it. .NET's are private and readonly, and .NET
ENFORCES in the language the rule this port could only state in a doc-comment.

Landed under SA-8 with SA-2's five conditions.

NO FIRST-PARTY MIGRATION WAS NEEDED, and the review had already measured why:
ReadOnlySequence, SequenceReader, BuffersExtensions and their suites all went
through the constructor and GetObject()/GetInteger() already. The only direct
field access anywhere was inside the type itself.

THIS IS AN ACCESS CHANGE, NOT A LAYOUT CHANGE. sizeof, alignof and trivial
copyability are unchanged and a static_assert pins that, so it is not an SA-3
case and consumers need no rebuild for layout reasons. Brace initialisation with
two arguments still compiles -- through the constructor rather than as an
aggregate -- so construction sites need no edit either.

Three spellings break loudly (direct read, direct write, structured bindings)
and a fourth breaks SILENTLY: a std::is_aggregate_v trait query. That is the
fourth site of the negative fixture, for exactly that reason.

Fixture set: 21 fixtures / 147 sites. Downstream #2368: zero sites in either
consumer. Gate 17,294 run, 0 failed.

Author: robertvokac

CLAUDE.md

@@ -80,7 +80,7 @@ implementation ticket.
 | [SR-AUD-066](modules/core/include/System/Lazy.hpp.audit.md#sr-aud-066--medium--publicationonly-wrongly-rejects-recursive-value-access) | medium | remediated | `Lazy.hpp` | An unconditional reentrancy guard throws for PublicationOnly even though .NET reserves this recursive-`Value` exception for None and ExecutionAndPublication. **Remediated by this finding's own second stated repair (#2237, family #2235): the divergence is RETAINED BY DESIGN and is now documented and pinned, not matched.** The finding authorises either implementing the PublicationOnly rule without deadlock **or** documenting and deliberately exposing the restriction; the first is not available without a user decision, because `checkNotReentrant()` is load-bearing for memory safety — a recursive `getValueProperty()` would re-`lock()` the non-recursive `publicationOnlyMutex_`, which `[thread.mutex.requirements.mutex]` makes undefined behaviour — and both structural routes to .NET's behaviour (publish-only locking, or same-thread reentrancy with a first-publication-wins discard rule) either reverse the header's already-documented PublicationOnly serialisation deviation or let an unconditionally recursive factory recurse without bound, trading a catchable `InvalidOperationException` for stack exhaustion. So the class doc-comment now carries an explicit second deviation paragraph naming the .NET rule not implemented, the `std::mutex` reason and the reopening ticket; `getValueProperty()`'s `@throws` clause records that .NET raises this for None and ExecutionAndPublication only; and +5 tests pin the behaviour in **all three** modes plus the non-recursive, fault-retry and different-instance controls a future change must not break. No executable statement changed. The behaviour change itself is ticket **#2238 (`needs_user`)**. `docs/CoreLazyThreadSafetyModeFamilyPlan.md` §4.2. **RESIDUAL NOW REMEDIATED TOO (#2238, 2026-08-18).** #2237 documented and pinned the deviation; this is the behaviour change it deliberately did not make. `PublicationOnly` serialized its factory behind a mutex and rejected a recursive `Value()`; .NET does neither. Both are gone, transcribed from `Lazy.cs:351-378` — the factory runs with **no lock held** and the publish is a first-writer-wins guard, so every loser discards the value it computed. **The user decided this knowing the price, which was stated first**: a `PublicationOnly` factory may now run **concurrently and more than once**, and a recursive one **exhausts the stack** instead of raising a catchable `InvalidOperationException`. The guard stays in force for `None` and `ExecutionAndPublication`, where it is not a policy choice but what prevents undefined behaviour. The class doc-comment was **rewritten, not amended**, in the past tense. The old pins were **replaced**: `RecursiveValueAccess_RejectedInAllThreeModes` asserted the old contract and **hung** against the new code, which is the correct signal; its replacement bounds the recursion and pins that the **innermost** publication wins. Four mutations: two caught, one caught as a hang (inherent — it reintroduces the deadlock), and **one not caught, with the code saying so at the site** — restoring the guard changes nothing, because `creatingThreadId_` is no longer written on that path. +6 net tests. `docs/Migration-LazyPublicationOnly.md`. |
 | [SR-AUD-067](modules/core/include/System/Buffer.hpp.audit.md#sr-aud-067--high--raw-bufferblockcopy-converts-negative-metadata-into-an-unbounded-memmove) | high | remediated | `Buffer.hpp` | Raw-pointer BlockCopy accepts negative offsets/count; negative count casts to `size_t` and reaches ASan-confirmed unbounded `memmove` instead of deterministic argument validation. |
 | [SR-AUD-068](modules/core/include/System/ValueType.hpp.audit.md#sr-aud-068--medium--valuetype-is-publicly-constructible-and-defaults-to-identity-rather-than-net-value-semantics) | medium | confirmed | `ValueType.hpp`, `ValueTypeTests.cpp` | Public C++ `ValueType` is constructible and uses identity/address defaults, where .NET has an abstract fieldwise-value base; direct tests lock in the divergent fallback. **Reviewed 2026-08-11 (#2322); STILL CONFIRMED, nothing implemented — `needs_user`.** Live: `Equals` is `this == &other`, `GetHashCode` is the object address narrowed to `intcs`, `ToString` is the literal `"System.ValueType"`, and the implicit public default constructor makes `System::ValueType v;` compile. **Measured consumer surface: nothing derives from it in production** — the only derived types anywhere are `SimpleValueType` and `ConcreteValueType` in `ValueTypeTests.cpp`, and `System::Void` documents that it deliberately does *not* derive from it, so the finding's harm has no in-repository instance and the exposure is entirely downstream. **Not autonomous, on two different grounds.** Field-by-field `Equals`/`GetHashCode` and a runtime-type-name `ToString` are reflection, which `CLAUDE.md` lists as a **permanent deviation, out of scope** — not a TODO. What remains is making the incompatible default unreachable, and every route is a **public source break** in a shipped header (a `protected` constructor, matching .NET's own `protected ValueType()`, or an abstract class) — the same class as SR-AUD-063. Options priced in #2322. `docs/CoreOwnedFindingsReviews2317.md`. |
-| [SR-AUD-069](modules/core/include/System/SequencePosition.hpp.audit.md#sr-aud-069--medium--sequenceposition-exposes-mutable-public-representation-instead-of-an-opaque-readonly-position) | medium | confirmed | `SequencePosition.hpp`, `Batch6BuffersTests.cpp` | Public mutable `void*`/integer components let callers rewrite a returned position, unlike .NET's private readonly opaque representation; tests never protect that boundary. **Reviewed 2026-08-12 (#2331); STILL CONFIRMED — it is a CONJUNCTION, and its value-contract clause is REMEDIATED while its representation clause is `needs_user` (#2332).** **Measured consumer surface:** seven files mention the type, and **direct field access exists in exactly one place — inside `SequencePosition` itself**; every other use already goes through the constructor and `GetObject()`/`GetInteger()`, so the encapsulation repair's first-party cost is zero. The two closest siblings settle the convention: `System::Index` and `System::Range` are classes with private fields and public `Equals`/`GetHashCode`. **#2331 (`done`), purely additive:** `Equals(const SequencePosition&)` and `GetHashCode()` are added and `operator==`/`!=` delegate to `Equals`, so the named and operator forms cannot drift. The hash is `((h1 << 5) + h1) ^ h2` evaluated in `uintcs` — signed overflow here would be UB, the CCF-004 class already recorded for `detail::tupleHashCombine` — with the pointer fold guarded by `if constexpr (sizeof(std::uintptr_t) > sizeof(uintcs))` because `bits >> 32` on a 32-bit target is a shift at the operand width. **It is deliberately not .NET's hash value** and the header says so; that is *not* the unverifiable-reference-text class of #2321/#2323, because a hash value is documented unstable in .NET too and this port already ships two hashes that differ from .NET's (`Range::GetHashCode`'s `397`, and `System::HashCode`'s per-process `std::random_device` seed). **+7 tests**, closing the finding's own list — equal and unequal **non-null** segments (every pre-existing direct test used `nullptr`), default equality, hash agreement, hash stability, and the caveat that component equality is not sequence-location identity — all obeying `docs/HashAssertionContractRule.md` (R1 asserted, no R2 pair, segment dependence stated over a family of eight, one R3 pin naming its property: a null segment folds to zero so the hash reduces to the integer's own bits). Three mutations, three caught. No layout, vtable, signature or `noexcept` change. **#2332 (`needs_user`):** making `object_`/`integer_` private is a public source break (structured bindings, designated initialisers, direct assignment) with zero first-party cost and unmeasurable downstream cost; layout is unaffected because all members keep the same access. The header now documents the divergence without taking the decision. `docs/CoreOwnedFindingsReviews2317.md`. |
+| [SR-AUD-069](modules/core/include/System/SequencePosition.hpp.audit.md#sr-aud-069--medium--sequenceposition-exposes-mutable-public-representation-instead-of-an-opaque-readonly-position) | medium | remediated | `SequencePosition.hpp`, `Batch6BuffersTests.cpp` | Public mutable `void*`/integer components let callers rewrite a returned position, unlike .NET's private readonly opaque representation; tests never protect that boundary. **Reviewed 2026-08-12 (#2331); STILL CONFIRMED — it is a CONJUNCTION, and its value-contract clause is REMEDIATED while its representation clause is `needs_user` (#2332).** **Measured consumer surface:** seven files mention the type, and **direct field access exists in exactly one place — inside `SequencePosition` itself**; every other use already goes through the constructor and `GetObject()`/`GetInteger()`, so the encapsulation repair's first-party cost is zero. The two closest siblings settle the convention: `System::Index` and `System::Range` are classes with private fields and public `Equals`/`GetHashCode`. **#2331 (`done`), purely additive:** `Equals(const SequencePosition&)` and `GetHashCode()` are added and `operator==`/`!=` delegate to `Equals`, so the named and operator forms cannot drift. The hash is `((h1 << 5) + h1) ^ h2` evaluated in `uintcs` — signed overflow here would be UB, the CCF-004 class already recorded for `detail::tupleHashCombine` — with the pointer fold guarded by `if constexpr (sizeof(std::uintptr_t) > sizeof(uintcs))` because `bits >> 32` on a 32-bit target is a shift at the operand width. **It is deliberately not .NET's hash value** and the header says so; that is *not* the unverifiable-reference-text class of #2321/#2323, because a hash value is documented unstable in .NET too and this port already ships two hashes that differ from .NET's (`Range::GetHashCode`'s `397`, and `System::HashCode`'s per-process `std::random_device` seed). **+7 tests**, closing the finding's own list — equal and unequal **non-null** segments (every pre-existing direct test used `nullptr`), default equality, hash agreement, hash stability, and the caveat that component equality is not sequence-location identity — all obeying `docs/HashAssertionContractRule.md` (R1 asserted, no R2 pair, segment dependence stated over a family of eight, one R3 pin naming its property: a null segment folds to zero so the hash reduces to the integer's own bits). Three mutations, three caught. No layout, vtable, signature or `noexcept` change. **#2332 (`needs_user`):** making `object_`/`integer_` private is a public source break (structured bindings, designated initialisers, direct assignment) with zero first-party cost and unmeasurable downstream cost; layout is unaffected because all members keep the same access. The header now documents the divergence without taking the decision. `docs/CoreOwnedFindingsReviews2317.md`. **APPROVAL-GATED CLAUSE REMEDIATED (#2332, 2026-08-18) under SA-8.** `object_` and `integer_` were public mutable data members, so a caller could rewrite a position after the sequence handed it out — to an unrelated segment, a dangling pointer, or an offset the owning sequence never produced — and every downstream reader would then trust it. .NET's are private and readonly, and .NET *enforces* in the language the rule this port could only state in a doc-comment. **No first-party migration was needed**, which the review had already measured: every other use went through the constructor and `GetObject()`/`GetInteger()`. **This is an access change, not a layout change** — `sizeof`, `alignof` and trivial copyability are unchanged and a `static_assert` pins that, so it is not an SA-3 case. Three spellings break (direct read, direct write, structured bindings) and **a fourth breaks silently** — a `std::is_aggregate_v` trait query — which is why it is the fourth site of the negative fixture. Brace initialisation with two arguments **still compiles**, through the constructor. Downstream measured: zero sites in either consumer (#2368). `docs/Migration-SequencePositionPrivateComponents.md`. |
 | [SR-AUD-070](modules/buffers/include/System/Buffers/ArrayBufferWriter.hpp.audit.md#sr-aud-070--medium--arraybufferwritert-silently-requires-a-default-constructible-t) | medium | remediated | `ArrayBufferWriter.hpp`, `MemoryPool.hpp` | Vector resize and `Clear` impose an undocumented default-constructor requirement on `T`; a valid non-default-constructible type fails to compile at `GetSpan`, and MemoryPool repeats the constraint.  **Remediated (#2054, 2026-08-04, family B-C, `docs/BuffersNamespaceReviewPlan.md` §4.4/§23.4):** each requirement is now stated in the owning type's Doxygen block and `static_assert`ed **at the point where it was already enforced**, so *exactly the same set of programs compiles* and only the diagnostic changes — measured site by site against the pre-change headers materialised from `b294738`. **Two premise corrections.** (a) The site count is **six**, not two: the review already promoted `ArrayBufferWriter::checkAndResizeBuffer`, `::Clear`, `MemoryPoolHeapOwner_`'s constructor and `SharedArrayPool<T>::Rent` + `ArrayPool<T>::Return(clearArray=true)` to first-class sites, and implementation found the sixth, `ArrayBufferWriter(intcs initialCapacity)`, which resizes on its own. `SequenceReader<T>::TryRead`/`TryPeek` are two further `T{}` sites whose requirement was **already documented** (the CCF-014 contract); they gain the assert but were never silent. (b) "fails to compile at `GetSpan`" understates where it bites: `GetSpan`/`GetMemory` are `virtual` overrides and `ArrayPool`'s `Rent`/`Return` are `virtual`, so their bodies are instantiated **for the vtable** — measured, `ArrayBufferWriter<NoDefault> w;` and `ArrayPool<NoDefault>::Shared().Return(v, false)` were already rejected before this ticket. Naming the type and `sizeof` stay legal for any `T`, which is why no assert is at class scope. Copy-assignability, which `Clear` and `Return(…, true)` also require, was undocumented too and is now asserted. 13 negative consumer sites (`test/consumer/buffers_generic_requirements_negative.cpp`) prove the rejected half; 9 tests in `BuffersGenericRequirementsTests.cpp` pin the accepted half. No runtime code, signature, layout, vtable or `noexcept` specification changed. |
 | [SR-AUD-071](modules/buffers/include/System/Buffers/MemoryPool.hpp.audit.md#sr-aud-071--high--memorypool-owner-permits-post-dispose-access-and-invalidates-retained-memory-into-a-native-fault) | high | remediated | `MemoryPool.hpp`, `Batch6BuffersTests.cpp` | A disposed owner returns empty Memory rather than throwing; retained pre-dispose Memory retains length over freed vector storage and ASan confirms a native null-read fault. **HALF (a) REMEDIATED (#2056, 2026-08-17); half (b) is still open and stays pinned as such.** `MemoryPoolHeapOwner_::getMemoryProperty` had no disposed check and returned a **zero-length** `Memory` after `Dispose()`, so a caller could not tell a disposed owner from a live `Rent(0)`. It now throws `ObjectDisposedException`, which is what .NET's `ArrayMemoryPoolBuffer.Memory` does (`ArrayMemoryPool.ArrayMemoryPoolBuffer.cs:18-25`). `sizeof` 32 → 40 under SA-3. **Why a flag and not .NET's own discriminator, recorded as a decision rather than a comment**: .NET needs no flag — it nulls `_array` and tests `array is null` — and the port's natural equivalent, a `unique_ptr<vector<T>>`, would even make the object *smaller*. It was rejected because `reset()` frees the storage **deterministically** where `clear() + shrink_to_fit()` is non-binding, and half (b) is still open: that would turn a latent use-after-free from "usually survives" into "always broken" while nothing yet fixes it. The storage lifetime is left byte-identical and a test asserts it, so a future change adopting the null discriminator must confront half (b) at the same time. **Half (b)** — a `Memory<T>` obtained before `Dispose()` keeping a pointer and length over released storage — is a `Memory<T>` ownership change in `Core.Base`, cannot be defended against from this type, and remains pinned as unfixed rather than letting half a repair look whole. +1 net test, two mutations both caught. Downstream measured: zero `System::Buffers` sites in either consumer. `docs/Migration-BuffersDisposedOwnerAndDefaultSequence.md`. |
 | [SR-AUD-072](modules/buffers/include/System/Buffers/ReadOnlySequence.hpp.audit.md#sr-aud-072--high--raw-pointer-readonlysequence-construction-dereferences-invalid-pointerlength-metadata) | high | remediated | `ReadOnlySequence.hpp`, `Batch6BuffersTests.cpp` | The raw pointer constructor forms vector iterators without null or signed-length validation; `nullptr, 1` reaches UBSan/ASan-confirmed null dereference. |

audit/AUDIT_FINDINGS_INDEX.md

@@ -0,0 +1,63 @@
+<!-- SPDX-License-Identifier: MIT -->
+<!-- Copyright (c) Robert Vokac and contributors -->
+
+# Migration — `SequencePosition`'s components are private (ticket #2332)
+
+*2026-08-18.* `SequencePosition::object_` and `::integer_` were public mutable data members, so a
+caller could rewrite a position after the sequence handed it out. .NET's are private and
+readonly.
+
+Landed under `docs/StandingApprovals.md` **SA-8** with SA-2's five conditions. A public **source**
+break in three spellings. **No layout change** — making members private changes access, not
+storage — and no signature, vtable or `noexcept` change.
+
+---
+
+## 1. What changed
+
+| Spelling | Was | Is |
+|---|---|---|
+| `pos.integer_ = 99;` | compiled | **rejected** |
+| `void* raw = pos.object_;` | compiled | **rejected** |
+| `auto [o, i] = pos;` | compiled | **rejected** — the type is no longer an aggregate |
+| `std::is_aggregate_v<SequencePosition>` | `true` | **`false`** |
+| `SequencePosition{obj, 5}` | aggregate init | **still compiles**, through the constructor |
+| `SequencePosition{}`, copy, `==`, `Equals` | — | **unchanged** |
+| `GetObject()`, `GetInteger()` | — | **unchanged** |
+| `sizeof`, `alignof`, trivial copyability | — | **unchanged** |
+
+## 2. Why
+
+.NET documents that the parts of a position **must not be interpreted by anything except the
+sequence that created it**, and enforces that in the language. This port stated the same rule in
+a doc-comment and could not enforce it: a caller could point a position at an unrelated segment,
+a dangling pointer, or an offset the owning sequence never produced, and every downstream reader
+would then trust it.
+
+## 3. To migrate
+
+Build positions with the two-argument constructor; read them with `GetObject()` and
+`GetInteger()`:
+
+```cpp
+// before
+auto [object, integer] = position;
+position.integer_ = 99;
+
+// after
+void* object = position.GetObject();
+auto  integer = position.GetInteger();
+position = SequencePosition(object, 99);
+```
+
+Those two accessors cover every legitimate use — which is why **every other type in this
+repository already used them**, and why this change needed no first-party migration at all.
+
+## 4. Downstream, measured
+
+Neither `cna` nor `mobile-eggbert` mentions `SequencePosition` — **zero sites in both**. Neither
+repository was modified. The downstream ticket is **#2368**.
+
+The negative consumer fixture `test/consumer/core_sequenceposition_private_negative.cpp` pins all
+three broken spellings plus a fourth that breaks a consumer *silently* rather than loudly — a
+`std::is_aggregate_v` trait query — and asserts the five surviving ones.