oracle: fix dotted-test-dir filter + dispose-optional StringWriter/StringReader (+ field notes)

.github/workflows/ci.yml

@@ -257,6 +257,12 @@ jobs:
           if echo "$out" | grep -q "HolderWithDisposeOptional"; then
             echo "FAIL: a dispose-optional (Task/DataTable) field was wrongly flagged"; exit 1
           fi
+          # the same rule for string-backed reader/writer fields (field-notes #8, Newtonsoft
+          # TraceJsonReader/Writer): a new'd, undisposed StringWriter/StringReader holds no
+          # unmanaged resource -> must stay SILENT (IsDisposeOptional, System.IO).
+          if echo "$out" | grep -q "HolderWithStringWriter"; then
+            echo "FAIL: a dispose-optional (StringWriter/StringReader) field was wrongly flagged"; exit 1
+          fi
           # field release recognition (mined: ImageSharp). #2 null-conditional dispose
           # `field?.Dispose()` must be recognized -> silent; the undisposed control still warns.
           if echo "$out" | grep -q "DisposesViaConditional"; then

docs/notes/field-notes-patterns.md

@@ -214,17 +214,110 @@ not luck — and the next recall lever for this family is the deferred field-sto
 anything in step 2's scope. *(Confidence: high — dispositions verified against the pinned
 Polly source, not just the SARIF excerpts.)*
 
+## 8. Event subscription on a freshly-created, *returned* publisher
+
+**Seen in:** Newtonsoft.Json `Src/Newtonsoft.Json/JsonSerializer.cs:717`
+(`ApplySerializerSettings`, commit `4f73e74`).
+
+A factory configures a new object and wires an event on it before handing it back;
+the subscription is never `-=`'d, but it doesn't need to be.
+
+```csharp
+public static JsonSerializer Create(JsonSerializerSettings? settings)
+{
+    JsonSerializer serializer = new JsonSerializer();
+    if (settings != null) ApplySerializerSettings(serializer, settings);
+    return serializer;                              // publisher escapes to the caller
+}
+private static void ApplySerializerSettings(JsonSerializer serializer, JsonSerializerSettings settings)
+{
+    if (settings.Error != null)
+        serializer.Error += settings.Error;         // <- flagged: "+= but never -="
+}
+```
+
+**Why:** the *publisher* (`serializer`) is the returned object. The handler lives
+exactly as long as the serializer the caller now holds; when that is collected, the
+subscription dies with it. No `-=` is needed — there is no longer-lived source
+retaining a shorter-lived target (the dangerous direction our OWN001/OWN014
+subscription leak targets). It's the publisher itself that is short/caller-scoped.
+
+**Analyzer angle — this is an *own-only* over-report (the first in this notebook
+where Own.NET, not the oracle, is too strict).** CodeQL/Infer# have no
+event-subscription-leak query, so they stay silent (correctly). Own.NET fires
+because, *inside* `ApplySerializerSettings`, `serializer` is an opaque **parameter**
+— `SubscriptionSourceKind` can't see that the caller `new`s it and `return`s it, so
+it conservatively tiers it `injected` and emits a **warning** (not a hard error;
+P-004 severity tiering already hedges unknown-lifetime publishers). So the default
+`error` posture is unaffected — it only surfaces under `--severity warning` (the
+oracle's setting). **Subscription-leak analysis is the dual of ownership transfer:
+a `+=` on a publisher that *escapes by return* is as bounded as a returned
+`IDisposable` — the fix is the same "follow the reference" escape rule, but
+interprocedural (the construct-and-return is in the caller), which is the hard part.
+The honest interim posture — advisory warning, never a hard error — is already in
+place.**
+
+## 9. Owning field whose IDisposable holds no unmanaged resource
+
+**Seen in:** Newtonsoft.Json `Src/Newtonsoft.Json/Serialization/TraceJsonReader.cs:37,38`
+and `TraceJsonWriter.cs:39` (commit `4f73e74`).
+
+A type owns a disposable field but never disposes it — and that is fine, because the
+field's `Dispose()` frees nothing real.
+
+```csharp
+internal class TraceJsonReader : JsonReader   // JsonReader : IDisposable, no Dispose override
+{
+    private readonly StringWriter _sw;         // StringBuilder-backed: Dispose() is a no-op
+    private readonly JsonTextWriter _textWriter;  // wraps _sw; at most returns a pooled char buffer
+    public TraceJsonReader(JsonReader inner)
+    {
+        _sw = new StringWriter(CultureInfo.InvariantCulture);
+        _textWriter = new JsonTextWriter(_sw);
+    }
+}
+```
+
+**Why:** these are short-lived, per-call diagnostic helpers (created only when a
+`TraceWriter` is attached at `Verbose`) that capture the JSON text into an in-memory
+`StringWriter`. A `StringWriter` wraps a `StringBuilder` — no OS handle, no
+unmanaged state; `Dispose()` just flips a closed flag. Not disposing it leaks
+nothing the GC won't reclaim.
+
+**Analyzer angle — also an *own-only* over-report.** CodeQL/Infer# stay silent
+(they model real resource handles; an undisposed `StringWriter` isn't one). Own.NET's
+owning-field detector flags every `IDisposable` field equally. The lever already
+exists: `IsOwnedDisposableType` exempts `IsDisposeOptional` types (Task/ValueTask/
+DataTable/DataSet/DataView — "Dispose is a no-op / only a lazy wait handle"). The
+fix for the two `StringWriter` fields is to **add `System.IO.StringWriter`/
+`StringReader` to that exemption** — a one-liner mirroring the existing set. The
+third field (`JsonTextWriter`, a third-party writer over the StringWriter, which on
+Dispose returns a pooled char buffer) is not generically exemptable by name and
+stays a low-value residual. **Rule of thumb: "owns an `IDisposable` field" is only a
+leak when the field owns a *real* resource — a string/in-memory writer is not one,
+and the `IsDisposeOptional` allowlist is where that knowledge belongs.**
+
 ---
 
 ## The through-line
 
-Five of six entries are the *same lesson from different angles*: **disposal
+Entries 1–6 are the *same lesson from different angles*: **disposal
 responsibility travels with the reference** — out of a factory (1, 6), forward in
 time via a callback (2), into a pool (3), or down a `using` on a value type (4).
 Naive "every disposable needs a lexical `using`/`Dispose` on every path" checks
 misread all of them, which is why Infer#/CodeQL over-report here and a
-transfer/escape-aware checker (Own.NET) correctly stays quiet. Worth learning as
-C#; worth pinning as the precision frontier.
+transfer/escape-aware checker (Own.NET) correctly stays quiet. (Entry 7 is a run
+ledger — an audit record of a Polly re-run, not an idiom.)
+
+Entries 8–9 are the **mirror image — the first cases where _Own.NET_ is the one
+over-reporting and the oracle is correctly silent.** They map our own precision
+frontier: a subscription on a publisher that *escapes by return* (8, the dual of
+ownership transfer — bounded, but the construct-and-return is interprocedural), and
+an owning field whose `IDisposable` holds no real resource (9, a `StringWriter` is
+not a handle — belongs in the `IsDisposeOptional` allowlist). Same moral as 1–6,
+pointed back at us: **a leak is about the *resource* and the *reference's
+lifetime*, not the mere presence of an `IDisposable` and a missing `Dispose`/`-=`.**
+Worth learning as C#; worth pinning as the precision frontier.
 
 ## Maintaining this notebook (a repo requirement)
 

frontend/roslyn/OwnSharp.Extractor/Program.cs

@@ -727,7 +727,14 @@
 {
     var ns = t.ContainingNamespace?.ToString();
     return (ns == "System.Threading.Tasks" && t.Name is "Task" or "ValueTask")
-        || (ns == "System.Data" && t.Name is "DataTable" or "DataSet" or "DataView");
+        || (ns == "System.Data" && t.Name is "DataTable" or "DataSet" or "DataView")
+        // System.IO string-backed reader/writer: a StringWriter wraps a StringBuilder
+        // and a StringReader reads a string — neither holds an OS handle / unmanaged
+        // resource, so Dispose() frees nothing real and an undisposed one is not a leak.
+        // (field-notes #8, mined on Newtonsoft.Json's TraceJsonReader/TraceJsonWriter,
+        // where these are owning fields the trace helpers never dispose.) MemoryStream is
+        // deliberately NOT here — it can own a real buffer, so it still warns.
+        || (ns == "System.IO" && t.Name is "StringWriter" or "StringReader");
 }
 
 // A type that is System.Windows.Forms.Form or derives from it (semantic, walks the
@@ -2864,7 +2871,7 @@
     .Split(Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries)
     .Where(p => p.EndsWith(".dll", StringComparison.OrdinalIgnoreCase))
     .ToList();
 var refNames = new HashSet<string>(tpa.Select(Path.GetFileName), StringComparer.OrdinalIgnoreCase);
 var references = tpa.Select(p => (MetadataReference)MetadataReference.CreateFromFile(p)).ToList();
 // P-004 WPF profile: widen the reference set with assemblies named by the
 // OWN_EXTRA_REF_DIRS env var (colon-separated dirs) — e.g. the WindowsDesktop ref

frontend/roslyn/samples/ResolvedDisposableSample.cs

@@ -52,3 +52,16 @@ public sealed class HolderWithDisposeOptional
 
     public int Use() => this.task.Id + this.table.Columns.Count;
 }
+
+// String-backed reader/writer dispose-optional control (field-notes #8, mined on
+// Newtonsoft.Json's TraceJsonReader/TraceJsonWriter): a StringWriter wraps a
+// StringBuilder and a StringReader reads a string — no unmanaged resource, so Dispose()
+// frees nothing real. A new'd, undisposed field of these must stay SILENT (IsDisposeOptional).
+// Contrast HolderWithRealDisposable's MemoryStream above, which still warns.
+public sealed class HolderWithStringWriter
+{
+    private readonly StringWriter writer = new();
+    private readonly StringReader reader = new("x");
+
+    public string Use() => this.writer.ToString() + this.reader.ReadToEnd();
+}

scripts/oracle_compare.py

@@ -378,14 +378,24 @@ def _is_test_path(path: str) -> bool:
     (e.g. Polly's `src/Snippets/Docs/*`, where ~20 `HttpResponseMessage`/`HttpClient`
     examples are never disposed by design) — counting them as product leaks inflates
     the oracle-only recall gap with example code that was never meant to dispose."""
-    for seg in path.lower().split("/"):
-        # Exact match for short, collision-prone names — a `snippet` *prefix* would
-        # wrongly drop product dirs like `SnippetEngine`/`SnippetService` (Polly's
-        # `src/Snippets/Docs/*` is caught by the exact `snippets`/`docs` segments
-        # anyway). Prefix match only for the long, unambiguous plural-able ones.
-        if (seg in ("test", "tests", "doc", "docs", "snippet", "snippets")
-                or seg.startswith(("benchmark", "sample", "example"))):
-            return True
+    # Only DIRECTORY segments classify a finding as non-product — NEVER the file's own
+    # name (the last segment). A product file literally named `Test.cs` / `Doc.cs` /
+    # `Foo.Tests.cs` is real product code and must NOT be dropped from the diff, or we
+    # under-count findings (Codex). So iterate the directory segments only (`segs[:-1]`).
+    segs = path.lower().split("/")
+    for seg in segs[:-1]:
+        # Split each directory segment on '.' before matching, for the ubiquitous .NET
+        # `<Project>.Tests` / `Foo.Benchmarks` directory convention: the dir
+        # `Newtonsoft.Json.Tests` is ONE path segment, so a bare-segment check
+        # misses it (and silently drags ~485 test findings into the diff). Matching
+        # each dot-component catches the `tests` tail while keeping the original
+        # guards intact — exact match for short, collision-prone names (so a single
+        # component `SnippetEngine`/`Documentation` is NOT dropped, only an exact
+        # `snippet`/`doc`), prefix only for the long, unambiguous plural-able ones.
+        for part in seg.split("."):
+            if (part in ("test", "tests", "doc", "docs", "snippet", "snippets")
+                    or part.startswith(("benchmark", "sample", "example"))):
+                return True
     return False
 
 
@@ -560,10 +570,24 @@ def _selftest() -> int:
                ("tests/Foo/Bar.cs", "benchmarks/X/Y.cs", "src/Test/Z.cs",
                 "src/Snippets/Docs/Fallback.cs", "src/MyLib/docs/Example.cs")):
         fails.append("_is_test_path should match test/benchmark/doc/snippet trees")
+    # The .NET `<Project>.Tests` / `Foo.Benchmarks` convention: the dotted dir is a
+    # single path segment, so it must be matched on its dot-components — else a repo
+    # like Newtonsoft.Json (test tree `Newtonsoft.Json.Tests`) leaks ~485 test
+    # findings into the product diff (own-only 489 vs the true 4). The sibling
+    # product dir `Newtonsoft.Json` must stay IN scope.
+    if not all(_is_test_path(p) for p in
+               ("Src/Newtonsoft.Json.Tests/Bson/BsonReaderTests.cs",
+                "src/Foo.Benchmarks/Bench.cs")):
+        fails.append("_is_test_path should match dotted .NET test/benchmark project dirs")
     # ...but a product dir whose name merely *starts with* a marker word is NOT
-    # excluded — exact match for snippet/doc guards against dropping real code.
+    # excluded — exact match for snippet/doc guards against dropping real code, and
+    # the dot-split must leave a non-test dotted product namespace untouched. Critically,
+    # the dot-split must NOT look at the FILE name: a product file literally named
+    # `Test.cs` / `Doc.cs` / `Foo.Tests.cs` is real code, not a test tree (Codex).
     if any(_is_test_path(p) for p in ("Dapper/SqlMapper.cs", "src/Lib/A.cs",
-                                      "src/SnippetEngine/Foo.cs", "src/Documentation/Api.cs")):
+                                      "src/SnippetEngine/Foo.cs", "src/Documentation/Api.cs",
+                                      "Src/Newtonsoft.Json/JsonSerializer.cs",
+                                      "src/Test.cs", "src/Doc.cs", "src/Foo.Tests.cs")):
         fails.append("_is_test_path should not match product paths")
     # the scope note is gated on mode, not count: a product-only run that excluded
     # nothing must still say so (else it reads like a full-scope run).
@@ -607,7 +631,7 @@ def _selftest() -> int:
         fails.append(f"non-SARIF JSON masked as clean: {len(not_sarif)} findings, "
                      f"{ns_drift} unparsed")
 
-    total = 24
+    total = 25
     for f in fails:
         print(f"ORACLE SELFTEST FAIL: {f}")
     print(f"oracle_compare selftest: {total - len(fails)}/{total} checks passed")