docs: record static-class subscriber exemption as a rejected approach#158
Merged
Conversation
A subscription to a process-lived static event (AppDomain.ProcessExit, Console.CancelKeyPress) from inside a `static class` is not an OWN014 region escape: a static class has NO instance, so the 'promotes the component instance to the source's lifetime' premise is vacuously false — its state is already process-lived by the language definition. Sound by guarantee, not heuristic. Extends the existing process-lived-subscriber exemption (the WPF `App` singleton, clsIsApp) with clsIsStatic, gated identically: only the source=="static" region escape, non-timers. Mined on CsvHelper's static ConsoleHost, whose ProcessExit/CancelKeyPress shutdown hooks were false OWN014s. New corpus fixture subscription-static-class-host: before.cs is an INSTANCE host subscribing this to Console.CancelKeyPress (real OWN014), after.cs is the same as a static class (silent). case.own models the token form (OWN001) like the sibling screentogif-systemevents-leak case. Newtonsoft TraceJsonReader._textWriter stays baselined: it is a JsonTextWriter over a StringWriter, no-op only because of what it wraps — recognising that needs wrapper-cascade analysis (unsound to blanket-exempt JsonTextWriter), so the honest call is to leave it. Re-pointing the oracle at CsvHelper to confirm the ConsoleHost FPs clear. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
…restore scaffolding Live CsvHelper oracle re-run confirms the clsIsStatic exemption clears both ConsoleHost hooks (AppDomain.ProcessExit / Console.CancelKeyPress) at the source: Own.NET leak total 2 -> 0, both findings absent from own-only and baselined. Delete the two CsvHelper baseline entries (no CsvHelper FP entries remain). Restore dev scaffolding: oracle-target.txt back to the systemevents-console fixture, remove the branch from the oracle.yml push filter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Both Codex and CodeRabbit independently flagged the clsIsStatic exemption as
unsound: it skipped EVERY static-source subscription in a static class without a
capture check, silently dropping real captured-local leaks — e.g.
static class Foo { void Attach(VM vm) {
SystemEvents.UserPreferenceChanged += (_, _) => vm.Refresh(); } }
where the process-lived event pins the captured vm forever (was OWN014).
A sound narrowing (suppress only when the handler captures nothing) would not
clear the CsvHelper ConsoleHost target either — its shutdown hooks capture
cts/resetEvent locals — so the static-class signal cannot soundly distinguish
that FP from a real captured-local leak. CsvHelper ConsoleHost therefore stays
baselined, same bucket as Newtonsoft serializer.Error.
Reverts the extractor change, the subscription-static-class-host fixture, the doc
update, and restores the two CsvHelper ConsoleHost baseline entries — branch
returns to main. PR #157 is closed (no net change). Caught pre-merge by review.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Document the unsound `|| clsIsStatic` exemption attempted in #157 (reverted in 488d505) so it is not retried. A static class only rules out an instance `this`; a lambda handler can still capture a local that a process-lived event pins for the whole process, so the exemption would silently swallow a real captured-local leak. A capture-gated narrowing would be sound but still would not clear CsvHelper ConsoleHost (it captures cts/resetEvent), which therefore stays baselined. - Program.cs: ANTI-PATTERN warning comment at the clsIsApp exemption site. - oracle-known-fps.md: new "Rejected approaches" section with the why. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
|
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 (2)
📝 WalkthroughWalkthroughAdds a multi-line anti-pattern comment in Static-class exemption rejection notes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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.
Что и зачем
Фиксируем в документации и в коде, почему расширение OWN014-исключения до
|| clsIsStatic(статик-класс как подписчик) — неверный путь, чтобы его не повторили. Подход пробовали в #157, отловили в ревью (Codex P2 + CodeRabbit Major) и откатили (488d505) ещё до мержа; чистый diff этой ветки = только документация + предупреждающий комментарий.Тип изменения
Как проверено
python scripts/oracle_compare.py --selftest→ 31/31;python tests/test_corpus.py→ 27/27)python tests/run_tests.pyruff check .иmypyИзменения только документационные (без изменения поведения экстрактора): чистый diff vs
main— 58 строк вdocs/notes/oracle-known-fps.mdи комментарий вProgram.cs. Селфтесты прогнаны для контроля, что ничего не задето.Связанные issue
Refs #157 (откачено в
488d505). Самостоятельной issue нет.Чеклист
feat:,fix:,docs:…)Что внутри
Program.cs—ANTI-PATTERNкомментарий на месте исключенияclsIsApp: не расширять до|| clsIsStatic. Статик-класс убирает только инстансныйthis, но лямбда-обработчик всё ещё захватывает локал, который process-lived событие пинит на весь процесс → реальная captured-local утечка, которую исключение молча проглотило бы.oracle-known-fps.md— новая секция «Rejected approaches» с полным разбором: почемуclsIsAppбезопасен, аclsIsStatic— нет; почему даже capture-gated версия не закрыла бы CsvHelperConsoleHost(он захватываетcts/resetEvent), поэтому те находки остаются в baseline.🤖 Generated with Claude Code
https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Generated by Claude Code
Summary by CodeRabbit