fix(text-json): JsonArray/JsonObject enumeration is fail-fast (#1889)

Closes two measured defects. An iterator held across a reallocating Add was an
ASan-confirmed heap-use-after-free -- a SIGSEGV in a build without a sanitizer
(J11). One held across Clear() silently returned a value from DESTROYED STORAGE
with no diagnostic in any build (J12), which is the worse of the two precisely
because nothing traps.

The repository's standard idiom, the one List<T> and BitArray already use, and
CLAUDE.md's counter invariant followed exactly: detail::MutationCounter, never a
bare integer, because ++ on a signed counter is undefined at INTCS_MAX and an
implicit assignment would transplant the source's counter into the destination.
NarrowMutationCounter gains no user.

JsonArray and JsonObject go 48 -> 56. JsonNode (24) and JsonValue (40) are
unchanged, which is what shows the counter went where the enumerators are and
nowhere else. Zero consumer sites.

THE MODULE EDGE WAS MADE EXPLICIT BY THE VALIDATOR, NOT BY ME. MutationCounter
lives in modules/collections and the two headers are PUBLIC, so Text.Json needed
Collections.Core as a PUBLIC dependency where it had been private -- and the
boundary validator rejected the private declaration outright. A local copy was
never an option; CLAUDE.md forbids it in terms. The graph is UNCHANGED at 41/93:
the edge existed and only its kind moved, a smaller change than #1814's.
Catalogue regenerated; --check passes.

Two of my own measurements were wrong and the compiler corrected both. "Zero
first-party begin()/end() sites" missed JsonNodeParseDepthTests iterating with
`it->second`, so the enumerator needs operator-> (guarded there too). And a
shipped #1886 layout static_assert fired at compile time with a runtime one
failing beside it; both updated, the two unmoved figures left as #1886 wrote
them, and the growth additionally asserted as a relationship.

Six mutations, all caught -- but M6 only after the test was strengthened, and the
first result is recorded rather than quietly fixed: a begin() that bumps the
counter leaves a single range-for working perfectly, because the one enumerator
snapshots the version begin() just produced. It is observable only with TWO
enumerators over the same unmutated container. SetItem earns its own row for the
mirror reason -- it changes no element count, so only the counter can notice it.

Gate: 17,589 run, 17,589 passed, 0 failed, 0 skipped across 38 executables (+4).
Build directory: build/ only, --parallel 2 throughout.

Author: robertvokac

CLAUDE.md

@@ -43,7 +43,7 @@ for maintainers and are not part of the consumer include surface.
 | `Numerics` | `modules/numerics` | static | `Buffers`, `Collections.Core`, `Core.Base` | — | — | — | `System/Numerics/BigInteger.hpp` |
 | `Runtime` | `modules/runtime` | static | `Collections.Core`, `Core.Base` | — | — | — | `System/Runtime/AmbiguousImplementationException.hpp` |
 | `Text` | `modules/text` | static | `Buffers`, `Core.Base` | — | — | — | `System/Text/ASCIIEncoding.hpp` |
-| `Text.Json` | `modules/text-json` | static | `Core.Base`, `Text` | `Collections.Core` | `Collections.Core` | — | `System/Text/Json/JsonCommentHandling.hpp` |
+| `Text.Json` | `modules/text-json` | static | `Collections.Core`, `Core.Base`, `Text` | — | `Collections.Core` | — | `System/Text/Json/JsonCommentHandling.hpp` |
 | `Text.RegularExpressions` | `modules/text-regular-expressions` | interface | `Core.Base` | — | — | — | `System/Text/RegularExpressions/Capture.hpp` |
 | `Threading` | `modules/threading` | static | `Core.Base`, `TimeZone` | — | — | — | `System/AsyncCallback.hpp` |
 | `Threading.Tasks` | `modules/threading-tasks` | static | `Core.Base`, `Threading` | — | — | — | `System/IAsyncDisposable.hpp` |

docs/ComponentCatalog.md

@@ -0,0 +1,84 @@
+<!-- SPDX-License-Identifier: MIT -->
+# Migration — `JsonArray`/`JsonObject` enumerators are fail-fast (#1889)
+
+Ticket **#1889** (SR-AUD-327, CCF-019), landed 2026-08-19 on an explicit approval after being
+declined since July.
+
+## The two measured defects
+
+`begin()`/`end()` handed out **raw `std::vector` iterators with no version guard**:
+
+| probe | what it did |
+|---|---|
+| **J11** | an iterator held across a **reallocating `Add`** was an **ASan-confirmed heap-use-after-free** — a SIGSEGV in a build without a sanitizer |
+| **J12** | an iterator held across **`Clear()`** silently returned a value from **destroyed storage, with no diagnostic in any build** |
+
+J12 is the worse of the two: `Clear()` does not reallocate, so nothing traps — the caller simply
+gets a plausible wrong answer.
+
+## What changed
+
+The repository's standard fail-fast idiom, the same one `List<T>` and `BitArray` use: each
+container holds a `System::Collections::detail::MutationCounter`, the enumerator snapshots a
+`detail::MutationVersion`, and a stale **dereference or advance** raises
+`InvalidOperationException` through `detail::requireUnmodified`.
+
+**CLAUDE.md's collection invariant is binding and was followed exactly**: the counter must be
+`detail::MutationCounter` and never a bare integer — `++` on a signed counter is undefined at
+`INTCS_MAX`, and an implicitly declared assignment operator would transplant the *source's* counter
+into the destination, leaving an enumerator apparently valid over storage the assignment destroyed.
+`detail::NarrowMutationCounter` gains no user.
+
+| type | before | after |
+|---|---|---|
+| `JsonArray` | 48 | **56** |
+| `JsonObject` | 48 | **56** |
+| `JsonNode`, `JsonValue` | 24, 40 | **unchanged** |
+
+Exactly one counter per container, and the two non-containers are untouched — which is what shows
+it went where the enumerators are and nowhere else. Silent binary break; consumers rebuild.
+Measured: **zero** `JsonArray`/`JsonObject` sites in `cna` and `mobile-eggbert`.
+
+## The module edge, made explicit by the validator rather than by me
+
+`MutationCounter` lives in `modules/collections`, and `JsonArray.hpp`/`JsonObject.hpp` are **public**
+headers — so `Text.Json` needed `Collections.Core` as a **public** dependency, where it had been
+**private**. The module-boundary validator rejected the private declaration outright, which is how
+the edge became explicit instead of accidental. A local copy of the counter was not an option:
+CLAUDE.md forbids it in terms.
+
+**The graph is unchanged at 41 modules / 93 edges** — the edge already existed; only its *kind*
+moved from private to public. That is a smaller change than #1814's, which added one. The generated
+catalogue was regenerated and `--check` passes.
+
+## Two corrections to my own measurements, both found by the compiler
+
+1. **"Zero first-party `begin()`/`end()` sites" was wrong.** `JsonNodeParseDepthTests.cpp` iterates
+   with `it->second`, which the first grep pattern missed. The enumerator therefore needs
+   `operator->`, which it now has — with the guard running there too, not only on `operator*`.
+2. **A shipped `#1886` layout pin fired at compile time**, exactly as it was written to, and a
+   second runtime one failed. Both are updated; the two figures that did **not** move are left as
+   #1886 wrote them, and the growth is additionally asserted as a **relationship**
+   (`48 + sizeof(MutationVersion)`) so a later member cannot hide behind a hand-updated literal.
+
+## Mutation testing
+
+Six mutations, all caught — **but M6 only after the test was strengthened, and the first result is
+recorded rather than quietly fixed**:
+
+| # | Mutation | Caught by |
+|---|---|---|
+| M1 | `SetItem` does not bump | the every-door case |
+| M2 | `Clear` does not bump | the J12 case |
+| M3 | dereference unguarded | the J11 case |
+| M4 | advance unguarded | the J11 case |
+| M5 | `JsonObject::Remove` does not bump | the every-door case |
+| M6 | **`begin()` itself bumps** | the every-door case, **after repair** |
+
+M6 was **NOT CAUGHT** at first. A `begin()` that bumps the counter leaves a single range-for
+working perfectly — the one enumerator snapshots the version `begin()` just produced — so nothing
+noticed. It is observable only with **two enumerators over the same unmutated container**, which is
+a legitimate thing to hold, and that is what the case now asserts.
+
+`SetItem` is worth its own row for the mirror-image reason: it changes no element **count**, so only
+the counter can notice it at all.

docs/Migration-JsonEnumeratorsAreFailFast.md

@@ -5,7 +5,11 @@ sharp_runtime_register_module(
     NAME Text.Json
     TARGET sharp_runtime_text_json
     TYPE STATIC
-    PUBLIC_DEPENDENCIES Core.Base Text
-    PRIVATE_DEPENDENCIES Collections.Core
+    # Collections.Core became a PUBLIC edge with ticket #1889: JsonArray and JsonObject hold a
+    # System::Collections::detail::MutationCounter in their PUBLIC headers, because CLAUDE.md's
+    # collection invariant requires exactly that type for a fail-fast enumerator and forbids a
+    # local copy. The module-boundary validator rejected the private declaration, which is what
+    # made the edge explicit rather than accidental. Same shape as #1814's Net.Http.Json edge.
+    PUBLIC_DEPENDENCIES Core.Base Text Collections.Core
     TEST_DEPENDENCIES Collections.Core
 )

modules/text-json/CMakeLists.txt

@@ -3,6 +3,8 @@
 // Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
 #pragma once
 #include <memory>
+#include "System/Collections/detail/MutationCounter.hpp"
+#include "System/Collections/IEnumerator.hpp"
 #include <vector>
 #include "SharpRuntime/SharpRuntimeHelper.hpp"
 #include "System/ArgumentOutOfRangeException.hpp"
@@ -20,6 +22,13 @@ namespace System::Text::Json::Nodes {
     class JsonArray : public JsonNode {
         std::vector<std::shared_ptr<JsonNode>> items_;
 
+        /// Fail-fast enumeration counter (#1889). CLAUDE.md's collection invariant is binding here:
+        /// it must be `detail::MutationCounter` and never a bare integer -- `++` on a signed counter
+        /// is undefined at INTCS_MAX, and the implicitly declared assignment operator would
+        /// transplant the SOURCE's counter into the destination, leaving an enumerator apparently
+        /// valid over storage the assignment destroyed. `MutationCounter` solves both.
+        System::Collections::detail::MutationCounter version_;
+
     public:
         explicit JsonArray(JsonNodeOptions options = {}) : JsonNode(options) {}
 
@@ -64,12 +73,14 @@ namespace System::Text::Json::Nodes {
             if (value) value->AssignParent(this);
             if (auto& old = items_[static_cast<size_t>(index)]) old->DetachParent();
             items_[static_cast<size_t>(index)] = std::move(value);
+            ++version_;   // #1889
         }
 
         /** @brief Appends @p item to the end of the array. */
         void Add(std::shared_ptr<JsonNode> item) {
             if (item) item->AssignParent(this);
             items_.push_back(std::move(item));
+            ++version_;   // #1889
         }
 
         /** @brief Inserts @p item at @p index. @throws System::ArgumentOutOfRangeException if out of range. */
@@ -78,6 +89,7 @@ namespace System::Text::Json::Nodes {
                 throw System::ArgumentOutOfRangeException("index");
             if (item) item->AssignParent(this);
             items_.insert(items_.begin() + index, std::move(item));
+            ++version_;   // #1889
         }
 
         /** @brief Removes the item at @p index. @throws System::ArgumentOutOfRangeException if out of range. */
@@ -86,6 +98,7 @@ namespace System::Text::Json::Nodes {
                 throw System::ArgumentOutOfRangeException("index");
             if (auto& item = items_[static_cast<size_t>(index)]) item->DetachParent();
             items_.erase(items_.begin() + index);
+            ++version_;   // #1889
         }
 
         /**
@@ -104,6 +117,7 @@ namespace System::Text::Json::Nodes {
         void Clear() {
             for (auto& item : items_) if (item) item->DetachParent();
             items_.clear();
+            ++version_;   // #1889
         }
 
         /** @return The index of @p item within the array, or -1 if not found (pointer identity). */
@@ -113,8 +127,55 @@ namespace System::Text::Json::Nodes {
             return -1;
         }
 
-        [[nodiscard]] auto begin() const { return items_.begin(); }
-        [[nodiscard]] auto end() const { return items_.end(); }
+        /**
+         * @brief Fail-fast enumerator over the array's elements.
+         *
+         * Ticket **#1889**. `begin()`/`end()` used to hand out **raw `std::vector` iterators with
+         * no version guard**, and two measured defects followed:
+         *   * an iterator held across a reallocating `Add` was an **ASan-confirmed
+         *     heap-use-after-free** -- a SIGSEGV in a build without a sanitizer (probe case J11);
+         *   * an iterator held across `Clear()` **silently returned a value from destroyed
+         *     storage, with no diagnostic in any build** (J12).
+         *
+         * This is the repository's standard fail-fast idiom, the same one `List<T>` and `BitArray`
+         * use: the container holds a `detail::MutationCounter`, the enumerator snapshots a
+         * `detail::MutationVersion`, and a stale dereference or advance raises
+         * `InvalidOperationException` rather than reading freed memory.
+         */
+        class Enumerator {
+            const JsonArray* owner_;
+            std::size_t index_;
+            System::Collections::detail::MutationVersion version_;
+
+            void requireCurrent() const {
+                System::Collections::detail::requireUnmodified(version_ == owner_->version_);
+            }
+
+        public:
+            Enumerator(const JsonArray* owner, std::size_t index)
+                : owner_(owner), index_(index), version_(owner->version_) {}
+
+            [[nodiscard]] const std::shared_ptr<JsonNode>& operator*() const {
+                requireCurrent();
+                return owner_->items_[index_];
+            }
+            /// Provided because callers iterate with `it->`; a forward iterator must offer it,
+            /// and the guard runs here too rather than only on `operator*`.
+            [[nodiscard]] auto operator->() const { return &**this; }
+
+            Enumerator& operator++() {
+                requireCurrent();
+                ++index_;
+                return *this;
+            }
+            [[nodiscard]] bool operator==(const Enumerator& other) const {
+                return owner_ == other.owner_ && index_ == other.index_;
+            }
+            [[nodiscard]] bool operator!=(const Enumerator& other) const { return !(*this == other); }
+        };
+
+        [[nodiscard]] Enumerator begin() const { return Enumerator(this, 0); }
+        [[nodiscard]] Enumerator end() const { return Enumerator(this, items_.size()); }
 
         [[nodiscard]] JsonValueKind GetValueKind() const override { return JsonValueKind::Array; }
 

modules/text-json/include/System/Text/Json/Nodes/JsonArray.hpp

@@ -3,6 +3,8 @@
 // Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
 #pragma once
 #include <memory>
+#include "System/Collections/detail/MutationCounter.hpp"
+#include "System/Collections/IEnumerator.hpp"
 #include <string>
 #include <utility>
 #include <vector>
@@ -29,6 +31,10 @@ namespace System::Text::Json::Nodes {
     class JsonObject : public JsonNode {
         std::vector<std::pair<std::string, std::shared_ptr<JsonNode>>> properties_;
 
+        /// Fail-fast enumeration counter (#1889); see JsonArray for why it must be
+        /// `detail::MutationCounter` rather than a bare integer.
+        System::Collections::detail::MutationCounter version_;
+
         // Verified against JsonObject.IDictionary.cs's CreateDictionary(): real .NET's backing
         // dictionary uses StringComparer.OrdinalIgnoreCase (when PropertyNameCaseInsensitive is
         // set) as the comparer for every operation -- lookup, ContainsKey, the duplicate-key
@@ -94,6 +100,7 @@ namespace System::Text::Json::Nodes {
                 throw System::ArgumentException("An item with the same key has already been added.", "propertyName");
             if (value) value->AssignParent(this);
             properties_.emplace_back(propertyName, std::move(value));
+            ++version_;   // #1889
         }
 
         /** @brief Removes the property named @p propertyName. @return true if it was present. */
@@ -102,6 +109,7 @@ namespace System::Text::Json::Nodes {
             if (idx < 0) return false;
             if (auto& value = properties_[static_cast<size_t>(idx)].second) value->DetachParent();
             properties_.erase(properties_.begin() + idx);
+            ++version_;   // #1889
             return true;
         }
 
@@ -139,17 +147,59 @@ namespace System::Text::Json::Nodes {
             } else {
                 if (value) value->AssignParent(this);
                 properties_.emplace_back(propertyName, std::move(value));
+            ++version_;   // #1889
             }
         }
 
         /** @brief Removes all properties from the object. */
         void Clear() {
             for (auto& [name, value] : properties_) if (value) value->DetachParent();
             properties_.clear();
+            ++version_;   // #1889
         }
 
-        [[nodiscard]] auto begin() const { return properties_.begin(); }
-        [[nodiscard]] auto end() const { return properties_.end(); }
+        /**
+         * @brief Fail-fast enumerator over the object's properties (#1889).
+         *
+         * The same repository idiom as `JsonArray::Enumerator`, for the same two measured
+         * defects (J11, J12): raw `std::vector` iterators had no version guard, so one held
+         * across a reallocating `Add` was an ASan-confirmed heap-use-after-free and one held
+         * across `Clear()` silently read destroyed storage with no diagnostic in any build.
+         */
+        class Enumerator {
+            const JsonObject* owner_;
+            std::size_t index_;
+            System::Collections::detail::MutationVersion version_;
+
+            void requireCurrent() const {
+                System::Collections::detail::requireUnmodified(version_ == owner_->version_);
+            }
+
+        public:
+            Enumerator(const JsonObject* owner, std::size_t index)
+                : owner_(owner), index_(index), version_(owner->version_) {}
+
+            [[nodiscard]] const std::pair<std::string, std::shared_ptr<JsonNode>>& operator*() const {
+                requireCurrent();
+                return owner_->properties_[index_];
+            }
+            /// Provided because callers iterate with `it->`; a forward iterator must offer it,
+            /// and the guard runs here too rather than only on `operator*`.
+            [[nodiscard]] auto operator->() const { return &**this; }
+
+            Enumerator& operator++() {
+                requireCurrent();
+                ++index_;
+                return *this;
+            }
+            [[nodiscard]] bool operator==(const Enumerator& other) const {
+                return owner_ == other.owner_ && index_ == other.index_;
+            }
+            [[nodiscard]] bool operator!=(const Enumerator& other) const { return !(*this == other); }
+        };
+
+        [[nodiscard]] Enumerator begin() const { return Enumerator(this, 0); }
+        [[nodiscard]] Enumerator end() const { return Enumerator(this, properties_.size()); }
 
         [[nodiscard]] JsonValueKind GetValueKind() const override { return JsonValueKind::Object; }