feat(diagnostics): structural identity for a diagnostic (#255 PR 1/3) - #319
Conversation
The step-4 parity key is `(line, code)`. It answered "which verdicts fire", but it CANNOT tell apart two findings that share an anchor and differ in subject, resource kind, severity or evidence — and that remainder is the entire subject of step 5a. This slice adds the key that separates them. `LocatedDiagnostic` wraps a ported verdict with the primary-location identity the dataclass does not carry; `DiagIdentity` is the canonical, non-collapsing comparison key over the whole contract. The load-bearing decision, fixed here so PR 2/3 cannot drift it: the location is an ENVELOPE, not two new fields on `Diagnostic`. The Python dataclass has neither `path` (it is the input identity — the caller knows which file it handed in) nor `column` (that lives on `ownir.Finding` per #317, is optional, and is never substituted; `Diagnostic._caret_col` is a renderer heuristic that re-reads the source line, not a source column). Adding both to the Rust type would make it stop mirroring the reference in order to suit a fixture — the one thing the migration rules forbid. Paths compare verbatim: `src\A.cs` and `src/A.cs` are two records. Python folds separators only at the SARIF seam (`evidence._phys`), so folding them at this layer would invent a behaviour the reference does not have. The projection owns it in #256. The differential is real, not decorative: each fixture case carries near-identical records plus the `distinct_records` count computed by Python's own frozen-dataclass equality. Rust must reach the same count from its key — collapsing a pair or splitting an identical pair both fail. The `identical_records_are_one` negative control exists so a key that hashed object identity could not pass the other ten cases by accident. Ordering is asserted only as a PROPERTY here (total, antisymmetric, reproducible from any input permutation). Which sequence a surface emits is PR 2's contract, pinned against Python — a total order is claimed now because a non-total key would make the identity itself unsound, not because this slice knows the emission order. `Severity` gains `Ord` purely so the key can sort; the doc says outright that declaration order carries no claim that one tier outranks the other (the Python enum has no ordering to mirror). Completed checkpoint: #255 PR 1/3 — model + comparison identity Remaining #255 acceptance: canonical message text and the Python-pinned ordering contract (PR 2); full OWN/DI/EFF corpus ledger with stale/orphan fixture failures (PR 3) Python source of truth: ownlang/diagnostics.py (Diagnostic, Evidence, Severity), ownlang/evidence.py, ownlang/ownir.py::Finding.column (#317) Fixture subset: 11 identity cases, 28 records, 27 distinct Regeneration command: python tests/test_diag_model_fixtures.py --write Zero-Python replay command: cd rust && cargo test -p own-diagnostics Acceptance changed: no Behavior changed: no Unexplained differences: 0 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJF7MBi1ijU5m9cJVWgQsM
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds structural identity for located diagnostics in Rust, including ordered evidence and severity ordering. It adds a Python-generated JSON fixture and Rust replay tests for equality, serialization, ordering, path handling, columns, Unicode, and evidence distinctions. ChangesDiagnostic identity parity
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PythonGenerator
participant DiagnosticFixture
participant RustReplay
participant LocatedDiagnostic
participant DiagIdentity
PythonGenerator->>DiagnosticFixture: generate serialized identity cases
RustReplay->>DiagnosticFixture: load fixture without Python
RustReplay->>LocatedDiagnostic: deserialize diagnostic records
LocatedDiagnostic->>DiagIdentity: construct canonical identities
RustReplay->>DiagIdentity: verify equality and deterministic ordering
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rust/crates/own-diagnostics/tests/model_replay.rs (1)
152-169: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe round trip cannot detect fixture-shape drift.
This test serializes a Rust value and deserializes the Rust output. Both sides use the same serde attributes, so the assertion holds even if the Rust shape diverges from the Python writer. Rust omits
subject,resource_kind,filewhenNoneand omits emptyevidence, while the fixture emitsnulland[]. Add a check against the fixture JSON values to pin the read contract.🧪 Proposed additional assertion
#[test] fn fixture_json_keys_are_all_consumed() { let raw = std::fs::read_to_string(FIXTURE).expect("fixture readable"); let root: Value = serde_json::from_str(&raw).expect("diag_model.json parses"); for case in root.get("cases").and_then(Value::as_array).expect("'cases'") { for record in case .get("records") .and_then(Value::as_array) .expect("'records'") { let loaded: LocatedDiagnostic = serde_json::from_value(record.clone()).expect("record loads"); let obj = record.as_object().expect("record is an object"); // Every key Python writes must be represented on the Rust value. assert!(obj.contains_key("path") && obj.contains_key("column")); let diag = obj["diagnostic"].as_object().expect("diagnostic object"); for key in ["code", "message", "line", "severity", "subject", "resource_kind", "evidence"] { assert!(diag.contains_key(key), "fixture lost key {key:?}"); } assert_eq!( loaded.diagnostic.evidence.len(), diag["evidence"].as_array().expect("evidence array").len(), "evidence arity drifted" ); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/crates/own-diagnostics/tests/model_replay.rs` around lines 152 - 169, Extend the tests in model_replay.rs with a fixture-driven test that parses the raw fixture JSON, deserializes each record into LocatedDiagnostic, and verifies the fixture’s required path/column and diagnostic keys—including null-valued optional fields and evidence—are present and consumed, while preserving the existing round-trip test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/crates/own-diagnostics/src/located.rs`:
- Around line 44-60: Update rust/crates/own-diagnostics/src/located.rs:44-60 to
describe LocatedDiagnostic as deserialising the fixture shape, noting that
serialization omits None fields and empty evidence. Update
rust/crates/own-diagnostics/tests/model_replay.rs:152-169 to assert the fixture
JSON keys and evidence arity, ensuring Python-side shape changes fail replay
rather than only exercising a Rust-to-Rust round trip.
---
Nitpick comments:
In `@rust/crates/own-diagnostics/tests/model_replay.rs`:
- Around line 152-169: Extend the tests in model_replay.rs with a fixture-driven
test that parses the raw fixture JSON, deserializes each record into
LocatedDiagnostic, and verifies the fixture’s required path/column and
diagnostic keys—including null-valued optional fields and evidence—are present
and consumed, while preserving the existing round-trip test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 56390bf0-d062-490c-b730-2d0fb1865465
📒 Files selected for processing (6)
rust/crates/own-diagnostics/src/diagnostic.rsrust/crates/own-diagnostics/src/lib.rsrust/crates/own-diagnostics/src/located.rsrust/crates/own-diagnostics/tests/model_replay.rstests/fixtures/diag_model.jsontests/test_diag_model_fixtures.py
![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: ad979f45de
ℹ️ 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".
| /// The 1-based source column (#317), when the producer reported one. | ||
| /// `None` means *not reported*; it is never substituted with a placeholder. | ||
| #[serde(default)] | ||
| pub column: Option<u32>, |
There was a problem hiding this comment.
When a producer supplies column: 0, Serde accepts it as Some(0), and with_column(0) constructs the same invalid value. This violates the stated 1-based contract and differs from ownlang/ownir.py::_check_column, which deliberately rejects zero because it is not a valid SARIF coordinate; validate deserialization and construction or represent reported columns with a non-zero type.
Useful? React with 👍 / 👎.
…PR 1/3) Two real defects from bot review on ad979f4, both confirmed against the reference before fixing. 1. The doc claimed the serde shape "matches diag_model.json exactly". It does not, in the WRITE direction, and the round-trip test could not have noticed: it serialises a Rust value and reads Rust's own output back, so both sides carry the same attributes. Measured: Python: {"path":...,"column":null,"diagnostic":{...,"subject":null, "resource_kind":null,"evidence":[]}} Rust: {"path":...,"column":null,"diagnostic":{...}} The ported Diagnostic inherits `skip_serializing_if` from step 4, so Rust omits `subject`/`resource_kind`/empty `evidence` where Python writes `null`/`[]`. Reading is unaffected — which is exactly what made the drift invisible: if a later regeneration dropped a key, the replay would still have passed. Fixed where the claim belongs, not by loosening it: the doc now states this type defines the READ contract and that serialization is deliberately asymmetric, and `fixture_shape_is_pinned_key_for_key` asserts the writer's key sets EXACTLY at all three levels (record, diagnostic, evidence step) plus evidence arity. Exact, not subset: a lost key means the fixture stopped carrying the contract, an added key means Python grew a field Rust silently discards — serde ignores unknown fields, so nothing else would notice. Verified by mutation: deleting one `resource_kind` from the fixture fails the assertion with the offending case named. Not fixed by changing the step-4 attributes: making the write side canonical is #256's job (it owns .ownreport.json serialization), and altering a shipped type for a step-5a fixture's convenience is the move this migration forbids. Not fixed by `deny_unknown_fields` either: the exactness belongs to this fixture, while the types may later be fed by producers whose tolerance is a separate decision. 2. `column` accepted 0, via serde and via `with_column(0)`, while the module doc promised 1-based. `ownlang.ownir::_check_column` rejects 0 outright — "SARIF columns start at 1, so a 0 is a producer bug, and silently reading it as absent would hide the bug while looking correct". Accepting it was a divergence from a documented reference contract, and a fabricated coordinate is the one thing #317 refuses. Now `Option<NonZeroU32>`: the illegal state is unrepresentable rather than checked, so serde rejects `0` and `with_column` cannot build one. Pinned by `column_zero_is_rejected_like_the_reference_does`. Completed checkpoint: unchanged — #255 PR 1/3, model + comparison identity Remaining #255 acceptance: unchanged — rendering/ordering (PR 2), full corpus ledger (PR 3) Python source of truth: ownlang/diagnostics.py, ownlang/ownir.py::_check_column Fixture subset: unchanged — 11 cases, 28 records, 27 distinct Regeneration command: python tests/test_diag_model_fixtures.py --write Zero-Python replay command: cd rust && cargo test -p own-diagnostics Acceptance changed: no Behavior changed: no Unexplained differences: 0 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJF7MBi1ijU5m9cJVWgQsM
|
Обе находки ботов проверены против эталона и оказались настоящими. Исправлено в 1. Форма фикстуры (CodeRabbit,
|
| PR | Содержимое | Статус |
|---|---|---|
| 1/3 (этот) | модель + идентичность сравнения | здесь |
| 2/3 | канонический текст сообщений + полный контракт упорядочивания | следующий |
| 3/3 | полный corpus-ledger по всем семействам OWN/DI/EFF, нулевые счётчики | закрывает #255 |
Поэтому в теле стоит Refs #255, а не Closes — issue остаётся открытым до PR 3. Каждый PR самостоятельно зелёный и оставляет дерево в законченной промежуточной точке; один PR на весь #255 ревьюился бы заметно хуже.
Гейты после правок
cargo fmt --check ✅ · cargo clippy --workspace --all-targets 0 warnings ✅ · cargo test --workspace 0 failed ✅ · cargo test -p own-diagnostics 14 unit + 8 replay ✅ · ruff check . ✅ · python tests/run_tests.py ✅
Отдельно: GitHub Actions на этом PR не запускались вообще (0 check runs), и это не следствие диффа — последний прогон CI в репозитории датирован 2026-07-29, как и последний коммит на main. Гейты выше прогнаны локально. Стоит заглянуть в Settings → Actions.
Generated by Claude Code
CI:
|
| Шаг | Ожидалось | Получено |
|---|---|---|
test_ownts.py (pin the spike) |
exit 0 | ✅ leaky=3xOWN001+EFF001, clean=0, … |
Dashboard.tsx → OwnIR → core |
3×OWN001 + 1×EFF001 = 4, якорь на .tsx, resource: timer |
✅ 3 / 4 / оба grep'а |
EffectStorm.tsx |
ровно 2×EFF001 + «derives from» | ✅ 2 + verdict на месте |
EffectEdges.tsx |
ровно 1 находка | ✅ 1 |
2. Job шёл ровно 45 минут — 20:36:29Z → 21:21:29Z. Локально те же шаги отрабатывают за секунды. Это профиль таймаута, а не падения ассерта.
3. Логи job'а отдают HTTP 404 — content недоступен, что типично для убитого, а не завершившегося прогона.
4. 18 из 19 job'ов всё ещё queued с 19:58:34Z, то есть больше полутора часов. Похоже на голодание раннеров — тем же, вероятно, объясняется и то, что в репозитории вообще не было прогонов CI с 29 июля до сегодняшнего вечера.
5. Пересечения с диффом нет. PR трогает tests/test_diag_model_fixtures.py, tests/fixtures/diag_model.json и rust/crates/own-diagnostics/*. Job гоняет frontend/ownts/* и python -m ownlang ownir; пакет ownlang/ этот PR не меняет ни строкой.
Перезапустить не могу — токен сессии без actions: write, и rerun-failed-jobs, и workflow_dispatch отвечают 403 Resource not accessible by integration. Нужен ручной re-run, когда раннеры разгребут очередь.
Остаюсь подписан: как только очередь пройдёт, доведу до зелёного. Если после re-run этот job упадёт снова — разберу уже как настоящее падение.
Generated by Claude Code
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/crates/own-diagnostics/tests/model_replay.rs`:
- Around line 152-169: Extend fixture_shape_is_pinned_key_for_key to define
exact ROOT_KEYS and CASE_KEYS sets, then validate the root object and every case
object before accessing cases and records. Preserve the existing record,
diagnostic, and evidence key checks so added Python metadata fields are rejected
at every fixture level.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cc92270-ab41-48f2-99e7-261a35194c78
📒 Files selected for processing (2)
rust/crates/own-diagnostics/src/located.rsrust/crates/own-diagnostics/tests/model_replay.rs
…PR 1/3) `fixture_shape_is_pinned_key_for_key` asserted exact key sets at three of the fixture's five levels — record, diagnostic, evidence step — and left the root object and each case object unchecked. Python could therefore grow a top-level or per-case field and the replay would pass, since serde ignores unknown fields. That is not merely a gap; it contradicts the rule the test itself states two lines above the constants: "an added key means Python grew a field Rust is silently discarding — serde ignores unknown fields, so nothing else would notice". The envelope around the records is part of the same contract: `schema_version` is what the replay refuses to run against when it moves, and `distinct_records` IS the reference behaviour every identity case is compared to. Neither had anything watching its key. Adds ROOT_KEYS and CASE_KEYS and asserts both before reading `cases` and `records`. Verified by mutation, each level independently: a stray top-level `generated_by` fails with "fixture root key set drifted", and a stray per-case `note` (root untouched) fails with the offending case named, so the root assertion is not masking the case one. Completed checkpoint: unchanged — #255 PR 1/3, model + comparison identity Remaining #255 acceptance: unchanged — rendering/ordering (PR 2), full corpus ledger (PR 3) Python source of truth: tests/test_diag_model_fixtures.py (the writer whose shape this pins) Fixture subset: unchanged — 11 cases, 28 records, 27 distinct Regeneration command: python tests/test_diag_model_fixtures.py --write Zero-Python replay command: cd rust && cargo test -p own-diagnostics Acceptance changed: no Behavior changed: no Unexplained differences: 0 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJF7MBi1ijU5m9cJVWgQsM
Three of four CodeRabbit findings on 2cb3822, each checked against the tree before acting (rule 1 of the section this PR adds). - Rule 4 declared `churn == 0` AND `delta == 1` as normative acceptance while `_insertion_churn()` gates only the first. A generator that dropped the new record entirely would pass every executable check. The norm stays as written; the gap is now stated rather than implied, which is the whole point of the status-drift rule this PR introduces. - `## Implementation status`: every other top-level section in the file is `##`; this one was the only `###` hanging directly off the h1. - Reflowed two paragraphs so `#258` and `#319` no longer begin a line. Renders the same on GFM (ATX needs a space), but the pattern is fragile on non-CommonMark renderers and trips MD018. The fourth finding is rejected in the PR thread: "in execution" is pre-existing on main, matches the proposal's own Status line, and changing only the index row would manufacture the exact drift this PR removes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJF7MBi1ijU5m9cJVWgQsM
#322) Two halves of one change, deliberately together: the rules for not letting status drift happen again, and the reconciliation that removes the drift those rules were learned on. Parity-work discipline — four rules, each paid for by a real defect during step 5a (#255, PRs #319/#320/#321), worded wider than this port so they outlive P-022: oracle over reviewer prose; mutation over plausible tests; no fail-fast during mutation campaigns; insertion-stable generated goldens. Rule 4's law is the invariant (churn == 0, delta == 1), not the content hash that satisfies it today. Recorded only in P-022 — two copies of one law drift. Status reconciliation — written fresh against the tree at fdcb222, not carried over from an earlier stale block. Corrects #258 (closed completed, both spec documents on main), the missing own-lowered/own-bridge workspace members, step 5a (closed completed), and own-diagnostics (full normalized contract, not a data-only layer). Statuses are checkpoint-level, separating each open step's normative blocker from #250's preferred sequencing. The proposals index row is fixed in the same change — a third status surface of the same fact. Review round on 2cb3822: four findings, each checked against the tree first. Three fixed in 267e38c, including the one that matters — the rule-4 section declared both acceptance lines while the executable guard gates only churn, so the enforcement gap is now stated rather than implied. The fourth was rejected with evidence and withdrawn by the reviewer. Refs #250, #255, #256, #258, #259.
2 participants
Что и зачем
Ключ паритета из шага 4 — это
(line, code), и он схлопывает две находки с общим якорем, различающиеся толькоsubject,resource_kind,severityили срезом Evidence. Ровно этот остаток и есть предмет шага 5a, поэтому здесь появляется ключ, который их различает:LocatedDiagnostic(конверт с первичной локацией) иDiagIdentity(каноничный неразрушающий ключ сравнения).Это PR 1 из 3 по #255. Он не закрывает issue: канонический текст сообщений и контракт упорядочивания идут в PR 2, полный corpus-ledger по всем семействам OWN/DI/EFF — в PR 3. Дерево остаётся в законченной промежуточной точке: типы не лежат мёртвой мебелью, у них есть вертикальный потребитель — Python-генератор фикстуры и zero-Python replay.
Тип изменения
Как проверено
python tests/run_tests.pyruff check .иmypypython scripts/<...>.py --selftest)Связанные issue
Refs #255 (PR 1 из 3 — не закрывает). Refs #250.
Closes #255появится только в PR 3, после полного ledger'а и нулевых счётчиков.Чеклист
feat:,fix:,docs:…)Несущее решение схемы (зафиксировано здесь, чтобы PR 2/3 его не переделывали)
Локация — это конверт, а не два новых поля на
Diagnostic. У Python-dataclass нет ни того, ни другого:path— это идентичность входа: ядро отчитывается по файлу, и вызывающий сам знает, какой файл передал. Именно поэтому oracle-поверхность пишется как(path, line, code), а замороженная пара —(line, code).columnживёт наownir.Finding(OwnIR: carry real Roslyn source columns into own-check SARIF #317), опционален и никогда не подставляется — ни 0, ни 1.Diagnostic._caret_colперечитывает строку исходника, чтобы поставить каретку; это эвристика рендерера, а не исходная колонка.Добавить их в
Diagnosticозначало бы, что Rust-тип перестаёт быть зеркалом эталона ради удобства фикстуры — единственное, что правила миграции запрещают прямо.Пути сравниваются буквально:
src\A.csиsrc/A.cs— две записи. Python нормализует разделители только на SARIF-шве (evidence._phys), поэтому нормализация на этом слое выдумала бы поведение, которого у эталона здесь нет. Это зона проекции — #256.Почему дифференциал настоящий, а не декоративный
Каждый кейс несёт почти одинаковые записи и число
distinct_records, посчитанное собственным equality Python-dataclass. Rust обязан прийти к тому же числу своим ключом: меньше — ключ схлопывает то, что эталон различает; больше — расщепляет то, что эталон считает равным.Негативный контроль
identical_records_are_oneсуществует затем, чтобы ключ, хеширующий идентичность объекта, не смог случайно пройти остальные десять кейсов.Контроли: разный
subjectпри общем якоре · разныйresource_kind· разнаяseverity· отсутствующие опциональные поля · значимость порядка Evidence ·role/labelкак часть идентичности · multi-file Evidence (file=None≠ явный тот же путь) · формы путей Windows/Unix · присутствие и значение колонки · Unicode в путях, сообщениях, subject и label (включая пару путей, различающихся одной буквойU+0451/U+0435).Что здесь намеренно НЕ утверждается
Упорядочивание проверяется только как свойство — тотальность, антисимметрия, воспроизводимость из любой перестановки входа. Какую именно последовательность выдаёт поверхность — контракт PR 2, приколоченный к Python. Тотальный порядок заявлен сейчас лишь потому, что нетотальный ключ сделал бы саму идентичность несостоятельной.
SeverityполучаетOrdисключительно ради сортируемости ключа; в доке прямо сказано, что порядок объявления не несёт утверждения о старшинстве тиров — у Python-enum'а порядка нет, и зеркалить нечего.messageпереносится в схему как непрозрачная строка, чтобы схема была финальной уже сейчас и PR 2 мог утверждать рендеринг в неё, не переделывая фикстуру.Generated by Claude Code
Summary by CodeRabbit
New Features
Tests