oracle: fix dotted-test-dir filter + dispose-optional StringWriter/StringReader (+ field notes)#147
Merged
Merged
Conversation
The integration token can't workflow_dispatch (403), so use the existing push-sentinel fallback to kick the cross-tool oracle. Adds this dev branch to the push allowlist and points corpus/oracle-target.txt at JamesNK/Newtonsoft.Json for a blind precision run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011YMngnax1jsy378fSAZb9r
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011YMngnax1jsy378fSAZb9r
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011YMngnax1jsy378fSAZb9r
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011YMngnax1jsy378fSAZb9r
…oduct diff _is_test_path matched a path segment only as a whole word, so the .NET <Project>.Tests / Foo.Benchmarks convention slipped through: the dir 'Newtonsoft.Json.Tests' is one segment, never equal to 'tests', so ~485 test findings leaked into the product diff (own-only 489 vs the true 4). Match each dot-component of a segment instead — catching the 'tests' tail while leaving single-component product dirs (SnippetEngine, Documentation, Newtonsoft.Json) untouched. Adds selftest cases for the dotted dir and its product sibling. Verified on the recorded Newtonsoft oracle output: own-only 489 -> 4, oracle_other 903 -> 235. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011YMngnax1jsy378fSAZb9r
…wtonsoft run First entries where Own.NET (not the oracle) over-reports, triaged from the 4 product own-only findings on JamesNK/Newtonsoft.Json: - #7 event '+=' on a freshly-created, RETURNED publisher (JsonSerializer:717): the dual of ownership transfer; bounded by the returned object's lifetime, so no '-=' needed. Already tiered as an advisory warning ('injected'), not a hard error — surfaces only under --severity warning. Proper fix is interprocedural publisher provenance. - #8 owning field whose IDisposable holds no unmanaged resource (TraceJsonReader/ Writer StringWriter/JsonTextWriter): a StringWriter is StringBuilder-backed, not a handle. Fixable by adding System.IO.StringWriter/StringReader to the existing IsDisposeOptional allowlist (mirrors Task/DataTable); the JsonTextWriter wrapper stays a low-value residual. Both verdicts: not real leaks, criticality ~0. CodeQL/Infer# correctly silent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011YMngnax1jsy378fSAZb9r
A StringWriter is StringBuilder-backed and a StringReader reads a string — neither holds an OS handle or unmanaged resource, so Dispose() frees nothing real and an undisposed owning field of these is not a leak. Add System.IO StringWriter/StringReader to IsDisposeOptional (the same lever already used for Task/ValueTask/DataTable/DataSet/ DataView), so the owned-field detector stays silent on them. MemoryStream is deliberately NOT exempted — it can own a real buffer and still warns. Surfaced by the oracle on Newtonsoft.Json (field-notes #8): TraceJsonReader/TraceJsonWriter own undisposed StringWriter/JsonTextWriter fields; CodeQL/Infer# correctly stay silent on the StringWriter (not a real resource), Own.NET over-reported. Adds a HolderWithStringWriter control to ResolvedDisposableSample + a CI assertion it stays silent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011YMngnax1jsy378fSAZb9r
Removes this dev branch from the oracle workflow push allowlist and restores corpus/oracle-target.txt to the committed fixture. The wiring was a dev-loop mechanism (the automation token can't workflow_dispatch); the workflow comment flags it 'remove before merging to main'. Net diff of both files vs main is now zero. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011YMngnax1jsy378fSAZb9r
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ChangesDisposal heuristics and validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e661573746
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…he filename The dotted-component match in _is_test_path also examined the FILE name (the last path segment), so a product finding in a file literally named 'Test.cs' / 'Doc.cs' / 'Foo.Tests.cs' was dropped from the product diff (under-counting real findings). Iterate directory segments only (segs[:-1]); the file's own name no longer classifies it as non-product. Test-*directories* (Newtonsoft.Json.Tests, src/Test/) still drop. Adds selftest negatives for the marker-named product files. (Codex P2 on #147.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011YMngnax1jsy378fSAZb9r
…3k6d07 # Conflicts: # docs/notes/field-notes-patterns.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Что и зачем
Прогон кросс-инструментального оракула по 4 OSS-целям (Newtonsoft.Json, Serilog, StackExchange.Redis, RestSharp) вскрыл два дефекта точности, которые тут и чинятся: (1) компаратор не отфильтровывал .NET тест-дир с точками (
Newtonsoft.Json.Tests), из-за чего ~485 тестовых находок попадали в product-дифф (own-only 489 вместо реальных 4); (2) экстрактор флагал owning-поляStringWriter/StringReaderкак утечку, хотя у них нет unmanaged-ресурса. Плюс зафиксированы два own-only идиома в field-notes (требованиеoracle.md).Тип изменения
Как проверено
python scripts/oracle_compare.py --selftest→ 25/25 (новые кейсы на дотовый тест-дирNewtonsoft.Json.Testsи его продуктового соседаNewtonsoft.Json)python tests/test_ownir.py→ 194/194 (не затронуто, sanity)ruff check scripts/oracle_compare.py→ cleanIsDisposeOptional+ sampleHolderWithStringWriter+ CI-ассерт) — C#/dotnet, валидируется в CI (локально dotnet недоступен): новый контроль должен остаться SILENT, controlMemoryStream/CTS — по-прежнему WARN.Что в диффе
scripts/oracle_compare.py—_is_test_pathматчит каждый дот-компонент сегмента (Newtonsoft.Json.Testsловится поtests), продуктовыеSnippetEngine/Documentation/Newtonsoft.Jsonне трогаются. +selftest-кейсы. Ловит проблему на любом репо с*.Tests/*.Benchmarks-неймингом.frontend/roslyn/OwnSharp.Extractor/Program.cs—System.IO.StringWriter/StringReader→IsDisposeOptional(тот же рычаг, что Task/DataTable).MemoryStreamнамеренно НЕ исключён.frontend/roslyn/samples/ResolvedDisposableSample.cs+.github/workflows/ci.yml— контрольHolderWithStringWriter+ ассерт молчания.docs/notes/field-notes-patterns.md— записи CI SHA-pinning #7 (подписка на returned publisher) и Quality gate, lifetimes module (WPF leaks), spec + proposals, and a real C# leak pipeline #8 (owning-field без unmanaged-ресурса); первые own-only-идиомы блокнота (где слишком строги мы, а оракул молчит).Связанные issue
Refs #146 — подписочный фикс (#7) требует межпроцедурного escape паблишера (дуал D5), не безопасный микрофикс; заведён отдельным трекером.
Чеклист
fix:/docs:/oracle:)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
StringReader/StringWriter) as dispose-optional, reducing false positives.*.Tests,*.Benchmarks).StringWriter/StringReadersample and updated oracle self-checks for the dotted-directory matching behavior.