fix(text-json): JsonNode is not copyable and DetachParent is not public (#1888)

Deletes JsonNode's four copy/move members and moves DetachParent to protected
with JsonArray/JsonObject as friends, closing three measured defects: a copy gave
a second container sharing the SAME children, each still reporting the ORIGINAL
as its parent (J08); assignment SLICED, rewriting parent_ on a node still stored
in a container (J09); and public DetachParent let one node sit in TWO containers
(J13).

A .NET JsonNode is a REFERENCE type, so there was never an object copy to
translate, and XObject already deleted all four -- this ends an asymmetry inside
the port rather than inventing a restriction. Measured: zero first-party
copy/assign sites, zero in both consumers.

THE HEADER'S OWN NOTE WAS WRONG AND THE REFERENCE CORRECTS IT. It claimed to
mirror "JsonNode.cs's internal DetachParent"; there is no DetachParent on
JsonNode.cs at all. .NET puts it on the CONTAINERS, private on each
(JsonObject.cs:316, JsonArray.IList.cs:231), body `item?.Parent = null`, with
Parent's setter internal. Protected-plus-friends is that reachability in C++.

Four pins inverted, and the fourth was not where the measurement said to look:
JsonNodeTeardownTests built its second container with
make_shared<JsonArray>(realOwner), a copy commented "shares children". A grep for
`X = *y` missed it; the compiler found it. Its real subject is #1886's `== this`
guard, so it is rewritten to reach that guard without a copy.

Three mutations. M1 (restore copy) and M3 (DetachParent public) caught at compile
time. M2 (restore move) is NOT CAUGHT and is reported as such: a proven
equivalence, measured with a probe. JsonNode is ABSTRACT, so
is_move_constructible_v is false whatever the declaration says, and
JsonArray/JsonObject have user-declared destructors (#1895) that suppress their
implicit moves, so move-constructibility falls back to the already-deleted copy.
The deletion is kept for intent and becomes load-bearing if a container drops
that destructor -- the header and the fixture's own site both say so.

It also finally writes the fixture #1894 could not: test/consumer/
text_json_node_lifetime_negative.cpp, named in #1894's acceptance criteria, which
recorded correctly that it "cannot be started, not merely should not be" because
no CCF-019 repair had outlawed any spelling. Fixture set 45/231 -> 46/236.
Downstream ticket #2396: zero sites in both consumers.

Gate: 17,585 run, 17,585 passed, 0 failed, 0 skipped across 38 executables (+0,
deliberately -- three pins became two and one case was added).
Build directory: build/ only, --parallel 2 throughout.

Author: robertvokac

CLAUDE.md

@@ -0,0 +1,82 @@
+<!-- SPDX-License-Identifier: MIT -->
+# Migration — `JsonNode` is not copyable, and `DetachParent` is not public (#1888)
+
+Ticket **#1888** (SR-AUD-327, CCF-019), landed 2026-08-19 on an explicit approval after being
+declined since July.
+
+## What changed
+
+```cpp
+JsonNode(const JsonNode&) = delete;   JsonNode& operator=(const JsonNode&) = delete;
+JsonNode(JsonNode&&)      = delete;   JsonNode& operator=(JsonNode&&)      = delete;
+protected: void DetachParent();       // was public; JsonArray and JsonObject are friends
+```
+
+## The three measured defects it closes
+
+| probe | what it did |
+|---|---|
+| **J08** | `JsonArray copy = *orig` gave a second array sharing the **same children**, each still reporting the **original** as its parent |
+| **J09** | `nodeRefA = nodeRefB` **sliced**, rewriting `parent_` on a node still stored in a container |
+| **J13** | public `DetachParent()` let a caller sever the link a container believes it owns and put the same node into a **second** container |
+
+**A .NET `JsonNode` is a reference type**, so there was never an object copy to translate —
+assigning one C# variable to another copies a reference. All four members were a C++ artefact, and
+`System::Xml::Linq::XObject` already deleted all four, so this **ends an asymmetry inside the port**
+rather than inventing a restriction.
+
+## The header's own note about `DetachParent` was wrong, and the reference corrects it
+
+It said the member "mirrors `JsonNode.cs`'s internal `DetachParent`". **There is no `DetachParent`
+on `JsonNode.cs` at all.** .NET puts it on the *containers*, as a **private** helper on each
+(`JsonObject.cs:316`, `JsonArray.IList.cs:231`), whose whole body is `item?.Parent = null` — and
+`Parent`'s setter is `internal`. So in .NET a consumer can neither call it nor reach what it does.
+Protected-plus-friends is that reachability expressed in C++.
+
+## Migration
+
+* a copy → **`DeepClone()`**, which does what the implicit copy did not: the clone's children are
+  its own and name the **clone** as their parent;
+* moving a node between containers → **`Remove`/`Add`**, which keeps the links and the containers in
+  step.
+
+**Measured impact: zero.** No first-party copy/assign site existed, and `cna` and `mobile-eggbert`
+have zero `JsonNode`/`JsonArray`/`JsonObject` sites. (`cna`'s single `JsonObject` match is its own
+`ExtractJsonObjectFieldEXT` helper, an unrelated name.)
+
+## Four shipped pins were inverted, and one was not where the measurement said to look
+
+Three were the probe-case pins, each carrying a `NOLINT - deliberate: pins today's implicit copy`
+marker. The fourth was **not found by the initial measurement**: `JsonNodeTeardownTests.cpp` built
+its second container with `std::make_shared<JsonArray>(realOwner)` — a copy-construction commented
+*"shares children"*. A grep for `X = *y` patterns missed it; the **compiler** found it. Its real
+subject is #1886's `== this` guard, so it is rewritten to reach that guard through two containers
+that genuinely hold different children, and a second case covers the moved-child form.
+
+## SA-2 conditions
+
+1. This note. ✔
+2. **`test/consumer/text_json_node_lifetime_negative.cpp` — 5 sites.** This is the fixture #1894's
+   acceptance criteria named and could not write for seven weeks: its notes recorded, correctly,
+   that it *"cannot be started, not merely should not be"*, because no CCF-019 repair had outlawed
+   any spelling. #1888 is the one that finally did. Fixture set **45 / 231 → 46 / 236**. ✔
+3. Downstream ticket **#2396**. ✔
+4. Full gate. ✔
+5. Measured consumer impact: **zero sites** in both. ✔
+
+## Mutation testing
+
+Three mutations. **M1** (restore the copy members) and **M3** (make `DetachParent` public again) are
+caught at compile time. **M2 (restore the move members) is NOT caught, and that is reported rather
+than dressed up** — it is a **proven equivalence**, for two independent reasons measured with a
+probe:
+
+* `JsonNode` is **abstract** (three pure virtuals), so `is_move_constructible_v<JsonNode>` is false
+  whatever those declarations say;
+* `JsonArray` and `JsonObject` each have a **user-declared destructor** (#1895's iterative
+  teardown), which suppresses their implicit move constructors — so their move-constructibility
+  falls back to the copy constructor, already deleted.
+
+The deletion is kept because it states the intent and becomes load-bearing the day a container drops
+its destructor. Both the header and the fixture's site 3 say exactly this, so a later reader is not
+misled into thinking the deletion is what rejects the spelling today.

docs/Migration-JsonNodeOwnershipIsNotCopyable.md

@@ -36,6 +36,34 @@ namespace System::Text::Json::Nodes {
     public:
         virtual ~JsonNode() = default;
 
+        // Ticket #1888. A .NET `JsonNode` is a REFERENCE type, so there is no object copy to
+        // translate -- assigning one C# variable to another copies a reference. C++ generates the
+        // copy and move members implicitly here, and all four are wrong for a parented node:
+        //
+        //   * copy construction gave a second container sharing the SAME children, each of which
+        //     still reported the ORIGINAL as its parent (probe case J08);
+        //   * copy assignment SLICED, rewriting parent_ on a node that was still stored in a
+        //     container (J09).
+        //
+        // `System::Xml::Linq::XObject` already deletes all four for the same reason, so this ends an
+        // asymmetry inside the port rather than inventing a restriction. Use DeepClone() for a copy.
+        //
+        // HONEST RECORD: deleting the two MOVE members is currently an EQUIVALENCE, and it is kept
+        // deliberately rather than because a test can see it. Measured -- restoring them as
+        // `= default` changes no observable, for two independent reasons:
+        //   * `JsonNode` is ABSTRACT (three pure virtuals), so `is_move_constructible_v<JsonNode>`
+        //     is false whatever these declarations say;
+        //   * `JsonArray` and `JsonObject` each have a USER-DECLARED destructor (#1895's iterative
+        //     teardown), which suppresses their implicit move constructors, so their
+        //     move-constructibility falls back to the copy constructor -- already deleted.
+        // The deletion states the intent and becomes load-bearing the day a container drops its
+        // user-declared destructor. It is not load-bearing today, and the mutation that restores
+        // it is reported as uncaught rather than dressed up.
+        JsonNode(const JsonNode&) = delete;
+        JsonNode& operator=(const JsonNode&) = delete;
+        JsonNode(JsonNode&&) = delete;
+        JsonNode& operator=(JsonNode&&) = delete;
+
         /** @return The options this node was constructed with. */
         [[nodiscard]] JsonNodeOptions getOptionsProperty() const { return options_; }
 
@@ -64,6 +92,26 @@ namespace System::Text::Json::Nodes {
          */
         void AssignParent(JsonNode* parent);
 
+    protected:
+        /**
+         * @brief Clears the parent container pointer.
+         *
+         * @note **Protected since ticket #1888, and the header's previous note about it was wrong.**
+         * It said this "mirrors JsonNode.cs's internal DetachParent" -- there is **no
+         * `DetachParent` on `JsonNode.cs` at all**. .NET puts it on the *containers*, as a
+         * **private** helper on each (`JsonObject.cs:316`, `JsonArray.IList.cs:231`), whose whole
+         * body is `item?.Parent = null` -- and `Parent`'s setter is `internal`. So in .NET a
+         * consumer can neither call it nor reach what it does.
+         *
+         * Public here, it let a caller put one node into **two** containers (probe case J13). It is
+         * now protected, with the two containers as friends, which is the same reachability .NET
+         * has expressed in C++.
+         */
+        void DetachParent() { parent_ = nullptr; }
+
+        friend class JsonArray;
+        friend class JsonObject;
+
     private:
         /** @return true if this node currently contains other nodes. #1896's cycle-guard
          *  short-circuit; see AssignParent. Non-virtual by design -- a virtual would be a vtable
@@ -72,8 +120,6 @@ namespace System::Text::Json::Nodes {
 
     public:
 
-        /** @brief Internal: clears the parent container pointer. Not part of .NET's public surface (mirrors JsonNode.cs's internal DetachParent). */
-        void DetachParent() { parent_ = nullptr; }
 
         /** @return This node cast to JsonArray. @throws System::InvalidOperationException if this isn't a JsonArray. */
         [[nodiscard]] JsonArray& AsArray();

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

@@ -838,6 +838,11 @@ TEST(JsonEmbeddedNulTests, THEDELIBERATEEXCEPTIONJsonNodeParseStillHasNoDepthBou
 // names the ticket that owns it. None of them endorses the behaviour; each of them fails the
 // day it changes, which is exactly when the owning ticket needs re-reading.
 
+namespace detail1888Review {
+    /// Dependent parameter, per the #2299 gcc trap.
+    template <typename T> concept HasPublicDetachParent = requires(T& t) { t.DetachParent(); };
+}
+
 TEST(JsonGatedBehaviourPins, Decl2118_GetRawTextReRendersRatherThanReturningSourceText) {
     // RENAMED AND RE-ROLED BY #2118 on 2026-08-19: this was a GATED pin ("a defect knowingly still
     // present"); it is now a DECLARATION. The user decided against retaining source spans, so the
@@ -979,11 +984,19 @@ TEST(JsonGatedBehaviourPins, PINCCF019JsonNodeSParentIsABorrowedPointerAndDetach
     object->Add("a", child);
     EXPECT_EQ(child->getParentProperty(), object.get()) << "a raw, borrowed parent pointer";
     EXPECT_EQ((*object)["a"]->GetValueKind(), JsonValueKind::Number);
-    // DetachParent is public -- the source break #1888 is blocked on.
-    child->DetachParent();
-    EXPECT_EQ(child->getParentProperty(), nullptr)
-        << "#1888: a caller can sever the link the container believes it owns";
-    EXPECT_TRUE(object->ContainsKey("a")) << "and the container still lists it";
+    // INVERTED BY #1888 (2026-08-19). This used to call DetachParent() directly and assert that a
+    // caller "can sever the link the container believes it owns" while the container still lists
+    // the key -- the defect, pinned. DetachParent is now protected with JsonArray/JsonObject as
+    // friends, which is the reachability .NET has: its DetachParent is a PRIVATE helper on each
+    // container and there is none on JsonNode at all.
+    static_assert(!detail1888Review::HasPublicDetachParent<Nodes::JsonObject>,
+                  "#1888: DetachParent must not be publicly callable");
+
+    // The link can still be severed -- through the container, which keeps the two in step instead
+    // of letting them disagree. That is the property the old assertion was really about.
+    object->Remove("a");
+    EXPECT_EQ(child->getParentProperty(), nullptr) << "the container detaches its own child";
+    EXPECT_FALSE(object->ContainsKey("a")) << "#1888: and no longer lists it -- they cannot disagree";
 }
 
 TEST(JsonGatedBehaviourPins, PINConvertersAndReferenceHandlingAreDECLARATIONONLYNotConsulted) {

modules/text-json/tests/System/Text/Json/JsonNamespaceReviewTests.cpp

@@ -33,6 +33,12 @@ using System::Text::Json::Nodes::JsonNode;
 using System::Text::Json::Nodes::JsonObject;
 using System::Text::Json::Nodes::JsonValue;
 
+namespace detail1888 {
+    /// Dependent parameter, per the #2299 gcc trap: a non-dependent `requires` on a missing or
+    /// inaccessible name hard-errors instead of yielding false.
+    template <typename T> concept HasPublicDetachParent = requires(T& t) { t.DetachParent(); };
+}
+
 namespace {
 
     std::shared_ptr<JsonNode> str(const std::string& s) { return JsonValue::Create(s); }
@@ -171,58 +177,78 @@ TEST(JsonNodeLifetimeTests, RetainedLeafOnly_AfterWholeTreeDestroyed_HasNoParent
     EXPECT_EQ(leaf->getRootProperty(), leaf.get());
 }
 
-// --- Only the destroying owner's own links are cleared (J08, J13) -------------------------
-
-// Probe case J13: DetachParent() is public today, so a node can legitimately end up stored in
-// one container while its parent link names another. The destructor of the container that no
-// longer owns it must leave that link alone.
-TEST(JsonNodeLifetimeTests, ChildOwnedByAnotherContainer_KeepsItsParentWhenTheFormerOwnerDies) {
+// --- The three defects #1888 closed (J08, J09, J13) ---------------------------------------
+//
+// These three cases used to PIN the defects, each with a `NOLINT - deliberate` marker saying so.
+// #1888 landed on 2026-08-19 and they are INVERTED: what they assert now is that the spelling is
+// gone, and that the lifetime behaviour they were really protecting still holds by other means.
+
+TEST(JsonNodeLifetimeTests, Fix1888_ANodeCannotBePutIntoTwoContainers) {
+    // WAS: ChildOwnedByAnotherContainer_KeepsItsParentWhenTheFormerOwnerDies (probe case J13).
+    // DetachParent() was PUBLIC, so a caller could clear a node's parent link and hand the same
+    // node to a second container -- leaving one container holding a node whose parent named
+    // another. The old case pinned the destructor's behaviour in that state.
+    //
+    // The state is now unreachable: DetachParent is protected, with JsonArray and JsonObject as
+    // friends. That is the reachability .NET has -- its DetachParent is a PRIVATE helper on each
+    // container (JsonObject.cs:316, JsonArray.IList.cs:231) whose body is `item?.Parent = null`,
+    // and `Parent`'s setter is internal. There is no DetachParent on JsonNode.cs at all.
+    static_assert(!detail1888::HasPublicDetachParent<JsonArray>,
+                  "#1888: DetachParent must not be publicly callable");
+
+    // What the old case really protected -- a container releasing only its OWN children -- still
+    // holds, and is reached the supported way: Remove detaches, and the former owner's death
+    // leaves the surviving container's link alone.
     JsonArray live;
     std::shared_ptr<JsonNode> child;
     {
         JsonArray stale;
         stale.Add(str("v"));
         child = stale[0];
-        child->DetachParent();
+        stale.RemoveAt(0);                       // the supported detach
+        ASSERT_EQ(child->getParentProperty(), nullptr);
         live.Add(child);
         ASSERT_EQ(child->getParentProperty(), &live);
     }
     EXPECT_EQ(child->getParentProperty(), &live);
     EXPECT_EQ(child->getRootProperty(), &live);
 }
 
-// Probe case J08: JsonNode's copy operations are still implicitly generated (ticket #1888, not
-// approved), so a copy-constructed array shares the original's children, which keep reporting
-// the original as their parent. Destroying the copy must not steal that link.
-TEST(JsonNodeLifetimeTests, CopyConstructedArrayDestroyed_LeavesTheOriginalsParentLinkIntact) {
+TEST(JsonNodeLifetimeTests, Fix1888_CopyAndAssignmentAreGoneAndDeepCloneReplacesThem) {
+    // WAS: CopyConstructedArrayDestroyed_LeavesTheOriginalsParentLinkIntact (J08) and
+    // OriginalDestroyedBeforeItsCopy_DetachesOnceAndTheCopyIsHarmless (J09). Both carried a
+    // `NOLINT - deliberate: pins today's implicit copy` marker.
+    //
+    // A .NET JsonNode is a REFERENCE type, so there was never an object copy to translate --
+    // these four members were a C++ artefact, and both were wrong for a parented node: the copy
+    // shared the original's children, each still reporting the ORIGINAL as its parent, and
+    // assignment SLICED, rewriting parent_ on a node still stored in a container.
+    static_assert(!std::is_copy_constructible_v<JsonArray>);
+    static_assert(!std::is_copy_assignable_v<JsonArray>);
+    static_assert(!std::is_move_constructible_v<JsonArray>);
+    static_assert(!std::is_move_assignable_v<JsonArray>);
+    static_assert(!std::is_copy_constructible_v<JsonObject>);
+    static_assert(!std::is_copy_constructible_v<JsonNode>);
+
+    // XObject already deleted all four, so this ended an asymmetry inside the port. That is NOT
+    // asserted here: modules/text-json does not depend on modules/xml-linq, and adding the edge
+    // for a test convenience is what the module-boundary rule exists to stop -- the same rule that
+    // blocked #1997's A-2. XLinqLifetimeTests owns the XObject half.
+
+    // DeepClone is the replacement, and it does what the implicit copy did NOT: the clone's
+    // children are its own, and they name the CLONE as their parent.
     JsonArray original;
     original.Add(str("v"));
     auto child = original[0];
-    {
-        JsonArray aliasing = original;  // NOLINT - deliberate: pins today's implicit copy
-        ASSERT_EQ(aliasing.getCountProperty(), 1);
-        ASSERT_EQ(child->getParentProperty(), &original);
-    }
-    EXPECT_EQ(child->getParentProperty(), &original);
-    EXPECT_EQ(child->getRootProperty(), &original);
-}
-
-// The reverse order: the original dies first (detaching the child), then the aliasing copy
-// dies. The copy must find a link that no longer names it and do nothing - in particular it
-// must not re-clear, re-read, or otherwise touch an already detached child.
-TEST(JsonNodeLifetimeTests, OriginalDestroyedBeforeItsCopy_DetachesOnceAndTheCopyIsHarmless) {
-    std::shared_ptr<JsonNode> child;
-    {
-        JsonArray aliasing;
-        {
-            JsonArray original;
-            original.Add(str("v"));
-            child = original[0];
-            aliasing = original;  // NOLINT - deliberate: pins today's implicit copy-assign
-        }
-        EXPECT_EQ(child->getParentProperty(), nullptr);
-    }
-    EXPECT_EQ(child->getParentProperty(), nullptr);
+    const auto clone = original.DeepClone();
+    auto* asArray = dynamic_cast<JsonArray*>(clone.get());
+    ASSERT_NE(asArray, nullptr);
+    ASSERT_EQ(asArray->getCountProperty(), 1);
+    EXPECT_NE((*asArray)[0].get(), child.get()) << "a deep clone shares no child with the original";
+    EXPECT_EQ((*asArray)[0]->getParentProperty(), asArray)
+        << "and the clone's children name the CLONE -- which is exactly what the implicit copy "
+           "got wrong";
+    EXPECT_EQ(child->getParentProperty(), &original) << "the original is untouched";
 }
 
 // --- Structural mutation before destruction ----------------------------------------------