test(diagnostics): close the rule-4 acceptance gap and three ledger nitpicks - #323
Conversation
…itpicks P-022 rule 4 states two lines as normative acceptance; the executable guard implemented only the first. This makes the guard match the norm already accepted in #322, and no more. _insertion_effect() now returns both halves. They fail in opposite directions and neither implies the other: * churn == 0 — an existing record changed. Paid for: an index-derived shape scored 42 of 47 before the seed was made position-independent. * delta == 1 — the new member produced exactly one record. Zero means the generator never covered the new family, and a generator that skips the new code has *perfect* churn, so the first half cannot see it. Above one means a record count derives from the vocabulary's size rather than its members. Both halves mutation-proved, each caught by exactly one half: * seed reverted to sorted index -> churn 42, delta 1; * generator iterating the committed golden's codes instead of TITLES -> churn 0, delta 0. The churn gate sits behind the staleness check, so it was additionally verified after regenerating under the mutation, where it fires with its own message. Three nitpicks from the #321 review, also mutation-checked: * analyzer_corpus read with expect, matching every other accessor in the file. unwrap_or(false) would silently under-count coverage if the generator stopped emitting the flag; dropping the key from one case now fails naming it. * changed == unexplained asserted explicitly. They move together by construction because no explanation mechanism exists, and the assertion forces that to be revisited deliberately if one is added. Recorded honestly: while the tree is green this is 0 == 0 and proves nothing. Verified by a compound mutation (corrupt one recorded text so a divergence exists, drop the unexplained increment) — it fires first, naming the counter split. * every_optional_field_shape_is_still_exercised guards the nine arms the generator rotates. Collapsing the two-evidence arm and regenerating fails only this test: the divergence check still passes 47/47, because it asks whether each case matches its own recorded text, never whether a shape disappeared. Refs #255, #250. 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)
📝 WalkthroughWalkthroughLedger replay validation now covers collapsed optional-field shapes, counter equality, strict analyzer-corpus flags, and insertion stability. Fixture checks measure churn, additions, and removals separately. ChangesLedger validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
![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: aee79ecc44
ℹ️ 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".
| shared = (set(before) & set(after)) - {probe} | ||
| return _InsertionEffect( | ||
| churn=sum(1 for code in shared if before[code] != after[code]), | ||
| delta=len(set(after) - set(before)), |
There was a problem hiding this comment.
If the generator emits the probe but simultaneously drops an existing record—for example, after being refactored to cap the ledger at its previous size—this set difference is still 1, while the dropped record is excluded from shared, so _insertion_effect() returns the accepted (churn=0, delta=1) despite the ledger not growing. Require the post-insertion keys to equal set(before) | {probe} (or separately reject removed keys) so the new Rule 4 guard actually enforces a one-record addition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and reproduced before fixing. Fixed in 3aecec5.
Ran your exact scenario — generator capped at the previous size:
churn=0 added=1 removed=1 -> guard ACCEPTS
before=47 after=47 added=['DI999'] REMOVED=['OWN052']
Both numbers read clean while the ledger did not grow. The dropped record is absent from after, so it never enters shared and the churn comparison cannot see it; the addition still counts one. The hole sits precisely between the two halves I had, which is why neither noticed.
_insertion_effect() now returns churn / added / removed. Each mutation is caught by exactly one number, with the other two structurally blind:
| Mutation | churn | added | removed | Caught by |
|---|---|---|---|---|
seed back to sorted(TITLES).index(code) |
42 | 1 | 0 | churn |
| generator iterates the committed golden's codes | 0 | 0 | 0 | added |
| ledger capped at its previous size (yours) | 0 | 1 | 1 | removed |
| baseline | 0 | 1 | 0 | — accepts |
I took the third number rather than your set(after) == set(before) | {probe} formulation for one reason: an equality check collapses "added the wrong count" and "lost a record" into a single boolean, and those are different defects with different causes. Keeping them separate means the failure message names which one happened. The guarantee is identical.
This does not widen P-022 rule 4. The norm reads new-record delta == 1, and a delta is a gain — a run that adds one record while dropping another has gained nothing. Counting only additions was measuring the stated norm wrong, so the measurement is split, not the rule.
Worth recording that I had seen this and talked myself out of it: while choosing mutations I tried a size-cap, observed it pass, and filed it as "bad mutation choice" instead of "the guard is blind here". That is the failure mode P-022 rule 2 exists for — a mutation that doesn't fail is a claim about the test, not about the mutation.
Generated by Claude Code
![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
🤖 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 `@tests/test_diag_ledger_fixtures.py`:
- Around line 37-40: Update the probe code used by _insertion_effect in both
affected cases from the trailing DI999 value to an unused code such as DI000, so
sorted(TITLES) inserts it before existing entries and genuinely tests unchanged
existing records plus exactly one addition.
🪄 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: 6f76c487-752c-4739-99d4-e417d63c7008
📒 Files selected for processing (2)
rust/crates/own-diagnostics/tests/ledger_replay.rstests/test_diag_ledger_fixtures.py
Codex found a real hole in the guard this PR adds, and it sits exactly between the two numbers already measured. A generator refactored to cap the ledger at its previous size emits the probe AND drops an existing code. The dropped record is absent from `after`, so it never enters the churn comparison; the addition still counts one. Both original numbers read clean while the ledger did not grow at all. Measured, not argued: probe added, OWN052 silently gone, guard ACCEPTS. `_insertion_effect()` now returns churn / added / removed. Each of the three mutations is caught by exactly one number, and the other two are structurally blind to it: seed = sorted index churn=42 added=1 removed=0 iterate the golden's codes churn=0 added=0 removed=0 cap ledger at previous size churn=0 added=1 removed=1 (baseline) churn=0 added=1 removed=0 This does not widen P-022 rule 4. The norm says "new-record delta == 1", and a delta is a gain: a run that adds one record while dropping another has gained nothing. Counting only additions was measuring the norm wrong, so the measurement is split, not the rule. Refs #255, #250. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJF7MBi1ijU5m9cJVWgQsM
|
@coderabbitai review Requesting a pass on Scope note for the pass: this PR is deliberately narrow — bring the executable guard up to P-022 rule 4's already-accepted acceptance, plus three nitpicks carried over from #321. Suggestions to refactor further in these files are out of scope by design and will be declined. Generated by Claude Code |
|
✅ Action performedReview finished.
|
2 participants
Что и зачем
P-022 правило 4 объявляет нормативным acceptance «churn == 0, delta == 1», а исполняемый guard реализовывал только первую строку. Этот PR доводит guard до уже принятой в #322 нормы — и ничего сверх того.
Норма не меняется. Меняется только то, насколько она проверяема.
Тип изменения
Как проверено
python tests/run_tests.pyruff check .иmypycargo fmt+cargo clippy --all-targets+cargo test -p own-diagnostics --no-fail-fastИзменение test-only: production-код не тронут ни на одной стороне.
Связанные issue
Refs #250, #255. Ничего не закрывает.
Чеклист
feat:,fix:,docs:…)Три числа, а не одно
_insertion_effect()возвращаетchurn/added/removed. Ключевое: каждое ловит свой дефект, и остальные два к нему структурно слепы.churn == 0added == 1removed == 0after, поэтому вообще не попадает в сравнение churn, а добавление при этом честно считается за одноadded > 1— отдельный случай: какое-то количество записей выведено из размера словаря, а не из его членов. Тот же дефект, что и churn, в другой одежде.Третье число появилось после ревью — см. ниже. Оно не расширяет правило 4: норма говорит
new-record delta == 1, а delta это прирост; прогон, который добавил одну запись и потерял другую, не вырос. Считать только добавления значило измерять норму неправильно, поэтому разделено измерение, а не правило.Mutation-проверка
Каждая мутация ловится ровно одним числом:
sorted(TITLES).index(code)TITLESВторая и третья — правдоподобные рефакторинги («оптимизируем регенерацию», «не давать ledger'у расти»), а не выдуманные поломки.
Отдельно: churn-гейт стоит после проверки на staleness, поэтому под первой мутацией скрипт сначала падал на устаревшем фикстуре. Проверено дополнительно — с регенерацией под мутацией staleness проходит, и churn падает уже своим сообщением (
rewrote 42 existing ledger record(s); rule 4 requires 0).Три nitpick'а из ревью #321
Все три тоже проверены мутацией.
expectвместоunwrap_or(false)дляanalyzer_corpus. Default молча занижал бы покрытие, если генератор перестанет писать флаг. Удаление ключа у одного case → тест падает, называя ключ.changed == unexplainedпо построению. Счётчики двигаются вместе, потому что механизма «объяснённого» расхождения не существует; ассерт заставит разделить их осознанно, если такой механизм появится.Честно про охват — и это стоило отдельной проверки: на зелёном дереве ассерт есть
0 == 0и не доказывает ничего, ветка расхождения не исполняется. Первая мутация (убрать инкрементunexplained) его не уронила. Доказан составным сценарием — испортить записанный текст одного case, чтобы расхождение существовало, и убрать инкремент: тогда падает именно он, первым, называя рассинхрон счётчиков.every_optional_field_shape_is_still_exercised— девять арок, которые ротирует генератор. Убрать арку с двумя evidence и регенерировать → падает только этот тест; проверка расхождений честно проходит 47/47, потому что спрашивает, совпадает ли каждый case со своим записанным текстом, а не исчезла ли форма.Раунд ревью
Две находки, указывающие в разные стороны. Обе проверены прогоном против дерева до любых правок.
Codex — верна, исправлена в
3aecec5Дыра ровно между двумя числами, которые уже измерялись. Генератор, обрезанный до прежнего размера, выдаёт probe и роняет существующий код:
Оба исходных числа читаются чисто, при том что ledger не вырос. Отсюда третье число.
Стоит зафиксировать: я это видел и сам себя переубедил. Подбирая мутации, попробовал обрезание по размеру, увидел, что оно проходит, и записал как «неудачный выбор мутации» вместо «guard здесь слеп». Это ровно тот режим отказа, ради которого существует правило 2 — мутация, которая не роняет тест, это утверждение о тесте, а не о мутации.
CodeRabbit — неверна, отклонена
Утверждение:
DI999сортируется послеDI…, поэтому probe дописывается в конец, и index-derived форма могла бы пройтиchurn == 0.Измерение говорит обратное:
Probe попадает в середину: словарь продолжается EFF/OBL/OWN, так что сдвигаются 42 из 47. И это ровно те 42, которые index-мутация даёт эмпирически — 5 DI + 1 EFF + 5 OBL + 36 OWN = 47, probe на позиции 5 сдвигает следующие 42. Те же 42 уже записаны в докстринге
_seedи в записи правила 4 в P-022.DI000сдвигал бы 47 вместо 42; guard падает уже на 42, так что замена не покупает ничего. Докстринг уточнён, чтобы такое прочтение больше не возникало.Отмечу: CodeRabbit пометил свою находку «✅ Addressed in commit
3aecec5» и закрыл тред, хотя probe не менялся. Поправлено в треде, чтобы запись не читалась как принятый совет.Что НЕ входит
Ничего. PR намеренно узкий: guard догоняет норму, и это всё.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests